Re: [PATCH v9 7/7] at24: Support probing while off

[Bartosz Golaszewski]

On Fri, Jan 29, 2021 at 1:20 PM Sakari Ailus
<sakari.ailus@xxxxxxxxxxxxxxx> wrote:
>
> Hi Bartosz,
>
> Thanks for the review.
>
> On Fri, Jan 29, 2021 at 11:56:00AM +0100, Bartosz Golaszewski wrote:
> > On Fri, Jan 29, 2021 at 12:27 AM Sakari Ailus
> > <sakari.ailus@xxxxxxxxxxxxxxx> wrote:
> > >
> > > In certain use cases (where the chip is part of a camera module, and the
> > > camera module is wired together with a camera privacy LED), powering on
> > > the device during probe is undesirable. Add support for the at24 to
> > > execute probe while being powered off. For this to happen, a hint in form
> > > of a device property is required from the firmware.
> > >
> > > Signed-off-by: Sakari Ailus <sakari.ailus@xxxxxxxxxxxxxxx>
> > > ---
> > >  drivers/misc/eeprom/at24.c | 43 +++++++++++++++++++++++---------------
> > >  1 file changed, 26 insertions(+), 17 deletions(-)
> > >
> > > diff --git a/drivers/misc/eeprom/at24.c b/drivers/misc/eeprom/at24.c
> > > index 926408b41270c..dd0b3f24e3808 100644
> > > --- a/drivers/misc/eeprom/at24.c
> > > +++ b/drivers/misc/eeprom/at24.c
> > > @@ -595,6 +595,7 @@ static int at24_probe(struct i2c_client *client)
> > >         bool i2c_fn_i2c, i2c_fn_block;
> > >         unsigned int i, num_addresses;
> > >         struct at24_data *at24;
> > > +       bool low_power;
> > >         struct regmap *regmap;
> > >         bool writable;
> > >         u8 test_byte;
> > > @@ -750,14 +751,16 @@ static int at24_probe(struct i2c_client *client)
> > >
> > >         i2c_set_clientdata(client, at24);
> > >
> > > -       err = regulator_enable(at24->vcc_reg);
> > > -       if (err) {
> > > -               dev_err(dev, "Failed to enable vcc regulator\n");
> > > -               return err;
> > > -       }
> > > +       low_power = acpi_dev_state_low_power(&client->dev);
> >
> > I've raised my concern about the naming of this before but no
> > discussion followed. Do we really want to name it: "low power"? This
> > is misleading as the device can actually be powered off at probe().
> > "Low power" suggests some low-power state or even low battery IMO.
>
> This was suggested by Rafael in place of "powered off" as it's not know the
> device is powered off. The same terms should be used in all contexts (ACPI
> and I²C frameworks and drivers). Others haven't expressed concerns.
>

So we're describing a situation where "device may be powered off" by
calling it "low_power". This doesn't make sense. Why not something
like: acpi_dev_may_be_off(), acpi_dev_powerdown_possible(),
acpi_dev_possibly_off(). If I'm reading a driver's code an see
"acpi_dev_state_low_power()", I would have never guessed it refers to
a situation where the device may be potentially powered-down.

> ACPI spec appears to be using terms "on" and "off".
>
> The use of the function is not limited to driver probe time.
>
> >
> > If anything: I'd prefer the 'low_power' local variable be changed to
> > "no_test_read".
>
> That misses the power management related suggestion now present in the name
> --- the device needs to be suspended using runtime PM if probe fails and
> it's not in "low power state".
>
> How about "off_during_probe"?
>

Yes, this is much better than low_power.

Bartosz

> --
> Kind regards,
>
> Sakari Ailus