[PATCH 4/4] drm: bridge: simple-bridge: add tdp158 support

[Dmitry Baryshkov]

On Tue, Jun 18, 2024 at 01:48:48PM GMT, Marc Gonzalez wrote:
> On 18/06/2024 00:33, Dmitry Baryshkov wrote:
> 
> > On Mon, Jun 17, 2024 at 06:03:02PM GMT, Marc Gonzalez wrote:
> > 
> >> +	if (sbridge->vcc) {
> >> +		ret = regulator_enable(sbridge->vcc);
> >> +		msleep(100);
> > 
> > At least this should be documented or explained in the commit message.
> > Is it absolutely necessary? Can you use regulator-enable-ramp-delay or
> > any other DT property instead?
> 
> The value comes from datasheet "8.3.2 Operation Timing"
> Table 1. Power Up and Operation Timing Requirements
> VDD supply ramp up requirements, max = 100 ms
> VCC supply ramp up requirements, max = 100 ms
> 
> Did I read the spec wrong? (Very possible)

I didn't check the spec. I was pointing that that you were adding
msleeps() into a generic path, but the commit message had no explanation
for that.

> 
> Are you saying this could/should be a property of the regulator?
> What if the regulator gates several different blocks?

I agree here. Yes, it should be done in the driver.

> 
> 
> >>  	sbridge = devm_kzalloc(dev, sizeof(*sbridge), GFP_KERNEL);
> >>  	if (!sbridge)
> >>  		return -ENOMEM;
> >> -	platform_set_drvdata(pdev, sbridge);
> > 
> > I think this call can get dropped together with the remove() being
> > gone...
> 
> Oooh, it didn't occur to me that the only reason to store drvdata was
> to have it available in the remove callback...
> 
> 
> > Does this work if the driver is built as a module?
> 
> Not sure there's any point in testing since Maxime NACKed the approach.

Yep :-(

> 
> Regards
> 

-- 
With best wishes
Dmitry