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
