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