| Message ID | 20210205132505.20173-8-sakari.ailus@linux.intel.com |
|---|---|
| State | New |
| Headers | show |
| Series | None | expand |
On Mon, Feb 8, 2021 at 5:44 PM Bartosz Golaszewski <bgolaszewski@baylibre.com> wrote: > > On Fri, Feb 5, 2021 at 2:25 PM Sakari Ailus > <sakari.ailus@linux.intel.com> 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@linux.intel.com> > > Reviewed-by: Tomasz Figa <tfiga@chromium.org> > > --- > > I'll ack this but I still claim that the name > acpi_dev_state_low_power() is super misleading for this use-case and > I've been saying that for 10 versions now with everyone just ignoring > my remarks. :/ Well, the function in question simply checks if the current ACPI power state of the device is different from "full power", so its name appears to be quite adequate to me. If the way in which it is used is confusing, though, I guess explaining what's going on would be welcome. > Acked-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
On Tue, Feb 09, 2021 at 05:42:45PM +0100, Rafael J. Wysocki wrote: > On Tue, Feb 9, 2021 at 5:23 PM Sakari Ailus > <sakari.ailus@linux.intel.com> wrote: > > > > Hi Bartosz, Rafael, > > > > On Tue, Feb 09, 2021 at 04:49:37PM +0100, Bartosz Golaszewski wrote: > > > On Mon, Feb 8, 2021 at 5:54 PM Rafael J. Wysocki <rafael@kernel.org> wrote: > > > > > > > > On Mon, Feb 8, 2021 at 5:44 PM Bartosz Golaszewski > > > > <bgolaszewski@baylibre.com> wrote: > > > > > > > > > > On Fri, Feb 5, 2021 at 2:25 PM Sakari Ailus > > > > > <sakari.ailus@linux.intel.com> 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@linux.intel.com> > > > > > > Reviewed-by: Tomasz Figa <tfiga@chromium.org> > > > > > > --- > > > > > > > > > > I'll ack this but I still claim that the name > > > > > acpi_dev_state_low_power() is super misleading for this use-case and > > > > > I've been saying that for 10 versions now with everyone just ignoring > > > > > my remarks. :/ > > > > > > > > Well, the function in question simply checks if the current ACPI power > > > > state of the device is different from "full power", so its name > > > > appears to be quite adequate to me. > > > > > > > > If the way in which it is used is confusing, though, I guess > > > > explaining what's going on would be welcome. > > > > > > > > > > Yes, I have explained it multiple time already - last time at v9 of this series: > > > > > > https://www.spinics.net/lists/kernel/msg3816807.html > > > > How about adding this to the description of acpi_dev_state_low_power(): > > > > -----------8<-------------- > > * This function is intended to be used by drivers to tell whether the device > > * is in low power state (D1--D3cold) in driver's probe or remove function. See > > * Documentation/firmware-guide/acpi/low-power-probe.rst for more information. > > -----------8<-------------- > > This information is already there in the kerneldoc description of that > function AFAICS. Ok, the D states are mentioned already. But how to use it is not, nor there's a reference to the ReST file. I think that wouldn't hurt. > > I was thinking about adding an explanation comment to the caller. I think it'd be best if the function name would convey that without a comment that should then be added to all callers. How about calling the function e.g. acpi_dev_state_d0() and negating the return value? The D0 state is well defined and we could do this without adding new terms.
On Tue, Feb 09, 2021 at 05:58:12PM +0100, Rafael J. Wysocki wrote: > On Tue, Feb 9, 2021 at 5:54 PM Sakari Ailus > <sakari.ailus@linux.intel.com> wrote: > > > > On Tue, Feb 09, 2021 at 05:42:45PM +0100, Rafael J. Wysocki wrote: > > > On Tue, Feb 9, 2021 at 5:23 PM Sakari Ailus > > > <sakari.ailus@linux.intel.com> wrote: > > > > > > > > Hi Bartosz, Rafael, > > > > > > > > On Tue, Feb 09, 2021 at 04:49:37PM +0100, Bartosz Golaszewski wrote: > > > > > On Mon, Feb 8, 2021 at 5:54 PM Rafael J. Wysocki <rafael@kernel.org> wrote: > > > > > > > > > > > > On Mon, Feb 8, 2021 at 5:44 PM Bartosz Golaszewski > > > > > > <bgolaszewski@baylibre.com> wrote: > > > > > > > > > > > > > > On Fri, Feb 5, 2021 at 2:25 PM Sakari Ailus > > > > > > > <sakari.ailus@linux.intel.com> 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@linux.intel.com> > > > > > > > > Reviewed-by: Tomasz Figa <tfiga@chromium.org> > > > > > > > > --- > > > > > > > > > > > > > > I'll ack this but I still claim that the name > > > > > > > acpi_dev_state_low_power() is super misleading for this use-case and > > > > > > > I've been saying that for 10 versions now with everyone just ignoring > > > > > > > my remarks. :/ > > > > > > > > > > > > Well, the function in question simply checks if the current ACPI power > > > > > > state of the device is different from "full power", so its name > > > > > > appears to be quite adequate to me. > > > > > > > > > > > > If the way in which it is used is confusing, though, I guess > > > > > > explaining what's going on would be welcome. > > > > > > > > > > > > > > > > Yes, I have explained it multiple time already - last time at v9 of this series: > > > > > > > > > > https://www.spinics.net/lists/kernel/msg3816807.html > > > > > > > > How about adding this to the description of acpi_dev_state_low_power(): > > > > > > > > -----------8<-------------- > > > > * This function is intended to be used by drivers to tell whether the device > > > > * is in low power state (D1--D3cold) in driver's probe or remove function. See > > > > * Documentation/firmware-guide/acpi/low-power-probe.rst for more information. > > > > -----------8<-------------- > > > > > > This information is already there in the kerneldoc description of that > > > function AFAICS. > > > > Ok, the D states are mentioned already. But how to use it is not, nor > > there's a reference to the ReST file. I think that wouldn't hurt. > > > > > > > > I was thinking about adding an explanation comment to the caller. > > > > I think it'd be best if the function name would convey that without a > > comment that should then be added to all callers. How about calling the > > function e.g. acpi_dev_state_d0() and negating the return value? The D0 > > state is well defined and we could do this without adding new terms. > > That would work for me. Bartosz, would that work for you? I'd call the temporary variable in the at24 driver e.g. "full_power".
On Wed, Feb 10, 2021 at 9:41 AM Sakari Ailus <sakari.ailus@linux.intel.com> wrote: > > On Tue, Feb 09, 2021 at 05:58:12PM +0100, Rafael J. Wysocki wrote: > > On Tue, Feb 9, 2021 at 5:54 PM Sakari Ailus > > <sakari.ailus@linux.intel.com> wrote: > > > > > > On Tue, Feb 09, 2021 at 05:42:45PM +0100, Rafael J. Wysocki wrote: > > > > On Tue, Feb 9, 2021 at 5:23 PM Sakari Ailus > > > > <sakari.ailus@linux.intel.com> wrote: > > > > > > > > > > Hi Bartosz, Rafael, > > > > > > > > > > On Tue, Feb 09, 2021 at 04:49:37PM +0100, Bartosz Golaszewski wrote: > > > > > > On Mon, Feb 8, 2021 at 5:54 PM Rafael J. Wysocki <rafael@kernel.org> wrote: > > > > > > > > > > > > > > On Mon, Feb 8, 2021 at 5:44 PM Bartosz Golaszewski > > > > > > > <bgolaszewski@baylibre.com> wrote: > > > > > > > > > > > > > > > > On Fri, Feb 5, 2021 at 2:25 PM Sakari Ailus > > > > > > > > <sakari.ailus@linux.intel.com> 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@linux.intel.com> > > > > > > > > > Reviewed-by: Tomasz Figa <tfiga@chromium.org> > > > > > > > > > --- > > > > > > > > > > > > > > > > I'll ack this but I still claim that the name > > > > > > > > acpi_dev_state_low_power() is super misleading for this use-case and > > > > > > > > I've been saying that for 10 versions now with everyone just ignoring > > > > > > > > my remarks. :/ > > > > > > > > > > > > > > Well, the function in question simply checks if the current ACPI power > > > > > > > state of the device is different from "full power", so its name > > > > > > > appears to be quite adequate to me. > > > > > > > > > > > > > > If the way in which it is used is confusing, though, I guess > > > > > > > explaining what's going on would be welcome. > > > > > > > > > > > > > > > > > > > Yes, I have explained it multiple time already - last time at v9 of this series: > > > > > > > > > > > > https://www.spinics.net/lists/kernel/msg3816807.html > > > > > > > > > > How about adding this to the description of acpi_dev_state_low_power(): > > > > > > > > > > -----------8<-------------- > > > > > * This function is intended to be used by drivers to tell whether the device > > > > > * is in low power state (D1--D3cold) in driver's probe or remove function. See > > > > > * Documentation/firmware-guide/acpi/low-power-probe.rst for more information. > > > > > -----------8<-------------- > > > > > > > > This information is already there in the kerneldoc description of that > > > > function AFAICS. > > > > > > Ok, the D states are mentioned already. But how to use it is not, nor > > > there's a reference to the ReST file. I think that wouldn't hurt. > > > > > > > > > > > I was thinking about adding an explanation comment to the caller. > > > > > > I think it'd be best if the function name would convey that without a > > > comment that should then be added to all callers. How about calling the > > > function e.g. acpi_dev_state_d0() and negating the return value? The D0 > > > state is well defined and we could do this without adding new terms. > > > > That would work for me. > > Bartosz, would that work for you? > > I'd call the temporary variable in the at24 driver e.g. "full_power". > Yes, that works, go ahead and thank you. Bartosz
On Wed, Feb 10, 2021 at 01:26:51PM +0100, Bartosz Golaszewski wrote: > On Wed, Feb 10, 2021 at 9:41 AM Sakari Ailus > <sakari.ailus@linux.intel.com> wrote: > > > > On Tue, Feb 09, 2021 at 05:58:12PM +0100, Rafael J. Wysocki wrote: > > > On Tue, Feb 9, 2021 at 5:54 PM Sakari Ailus > > > <sakari.ailus@linux.intel.com> wrote: > > > > > > > > On Tue, Feb 09, 2021 at 05:42:45PM +0100, Rafael J. Wysocki wrote: > > > > > On Tue, Feb 9, 2021 at 5:23 PM Sakari Ailus > > > > > <sakari.ailus@linux.intel.com> wrote: > > > > > > > > > > > > Hi Bartosz, Rafael, > > > > > > > > > > > > On Tue, Feb 09, 2021 at 04:49:37PM +0100, Bartosz Golaszewski wrote: > > > > > > > On Mon, Feb 8, 2021 at 5:54 PM Rafael J. Wysocki <rafael@kernel.org> wrote: > > > > > > > > > > > > > > > > On Mon, Feb 8, 2021 at 5:44 PM Bartosz Golaszewski > > > > > > > > <bgolaszewski@baylibre.com> wrote: > > > > > > > > > > > > > > > > > > On Fri, Feb 5, 2021 at 2:25 PM Sakari Ailus > > > > > > > > > <sakari.ailus@linux.intel.com> 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@linux.intel.com> > > > > > > > > > > Reviewed-by: Tomasz Figa <tfiga@chromium.org> > > > > > > > > > > --- > > > > > > > > > > > > > > > > > > I'll ack this but I still claim that the name > > > > > > > > > acpi_dev_state_low_power() is super misleading for this use-case and > > > > > > > > > I've been saying that for 10 versions now with everyone just ignoring > > > > > > > > > my remarks. :/ > > > > > > > > > > > > > > > > Well, the function in question simply checks if the current ACPI power > > > > > > > > state of the device is different from "full power", so its name > > > > > > > > appears to be quite adequate to me. > > > > > > > > > > > > > > > > If the way in which it is used is confusing, though, I guess > > > > > > > > explaining what's going on would be welcome. > > > > > > > > > > > > > > > > > > > > > > Yes, I have explained it multiple time already - last time at v9 of this series: > > > > > > > > > > > > > > https://www.spinics.net/lists/kernel/msg3816807.html > > > > > > > > > > > > How about adding this to the description of acpi_dev_state_low_power(): > > > > > > > > > > > > -----------8<-------------- > > > > > > * This function is intended to be used by drivers to tell whether the device > > > > > > * is in low power state (D1--D3cold) in driver's probe or remove function. See > > > > > > * Documentation/firmware-guide/acpi/low-power-probe.rst for more information. > > > > > > -----------8<-------------- > > > > > > > > > > This information is already there in the kerneldoc description of that > > > > > function AFAICS. > > > > > > > > Ok, the D states are mentioned already. But how to use it is not, nor > > > > there's a reference to the ReST file. I think that wouldn't hurt. > > > > > > > > > > > > > > I was thinking about adding an explanation comment to the caller. > > > > > > > > I think it'd be best if the function name would convey that without a > > > > comment that should then be added to all callers. How about calling the > > > > function e.g. acpi_dev_state_d0() and negating the return value? The D0 > > > > state is well defined and we could do this without adding new terms. > > > > > > That would work for me. > > > > Bartosz, would that work for you? > > > > I'd call the temporary variable in the at24 driver e.g. "full_power". > > > > Yes, that works, go ahead and thank you. Thanks! I'll send v11 soon.
diff --git a/drivers/misc/eeprom/at24.c b/drivers/misc/eeprom/at24.c index 926408b41270c..69a5e4023d9e1 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 off_during_probe; 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; - } + off_during_probe = acpi_dev_state_low_power(&client->dev); + if (!off_during_probe) { + err = regulator_enable(at24->vcc_reg); + if (err) { + dev_err(dev, "Failed to enable vcc regulator\n"); + return err; + } - /* enable runtime pm */ - pm_runtime_set_active(dev); + pm_runtime_set_active(dev); + } pm_runtime_enable(dev); at24->nvmem = devm_nvmem_register(dev, &nvmem_config); @@ -768,14 +771,17 @@ static int at24_probe(struct i2c_client *client) } /* - * Perform a one-byte test read to verify that the - * chip is functional. + * Perform a one-byte test read to verify that the chip is functional, + * unless powering on the device is to be avoided during probe (i.e. + * it's powered off right now). */ - err = at24_read(at24, 0, &test_byte, 1); - if (err) { - pm_runtime_disable(dev); - regulator_disable(at24->vcc_reg); - return -ENODEV; + if (!off_during_probe) { + err = at24_read(at24, 0, &test_byte, 1); + if (err) { + pm_runtime_disable(dev); + regulator_disable(at24->vcc_reg); + return -ENODEV; + } } pm_runtime_idle(dev); @@ -795,9 +801,11 @@ static int at24_remove(struct i2c_client *client) struct at24_data *at24 = i2c_get_clientdata(client); pm_runtime_disable(&client->dev); - if (!pm_runtime_status_suspended(&client->dev)) - regulator_disable(at24->vcc_reg); - pm_runtime_set_suspended(&client->dev); + if (!acpi_dev_state_low_power(&client->dev)) { + if (!pm_runtime_status_suspended(&client->dev)) + regulator_disable(at24->vcc_reg); + pm_runtime_set_suspended(&client->dev); + } return 0; } @@ -834,6 +842,7 @@ static struct i2c_driver at24_driver = { .probe_new = at24_probe, .remove = at24_remove, .id_table = at24_ids, + .flags = I2C_DRV_FL_ALLOW_LOW_POWER_PROBE, }; static int __init at24_init(void)