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 <[email protected]>
---
drivers/misc/eeprom/at24.c | 31 +++++++++++++++++++++----------
1 file changed, 21 insertions(+), 10 deletions(-)
diff --git a/drivers/misc/eeprom/at24.c b/drivers/misc/eeprom/at24.c
index 0681d5fdd538a..5fc1162b67618 100644
--- a/drivers/misc/eeprom/at24.c
+++ b/drivers/misc/eeprom/at24.c
@@ -564,6 +564,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;
@@ -701,19 +702,24 @@ static int at24_probe(struct i2c_client *client)
i2c_set_clientdata(client, at24);
- /* enable runtime pm */
- pm_runtime_set_active(dev);
+ low_power = acpi_dev_state_low_power(&client->dev);
+ if (!low_power)
+ pm_runtime_set_active(dev);
+
pm_runtime_enable(dev);
/*
- * 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);
- pm_runtime_idle(dev);
- if (err) {
- pm_runtime_disable(dev);
- return -ENODEV;
+ if (!low_power) {
+ err = at24_read(at24, 0, &test_byte, 1);
+ pm_runtime_idle(dev);
+ if (err) {
+ pm_runtime_disable(dev);
+ return -ENODEV;
+ }
}
if (writable)
@@ -728,8 +734,12 @@ static int at24_probe(struct i2c_client *client)
static int at24_remove(struct i2c_client *client)
{
+ bool low_power;
+
pm_runtime_disable(&client->dev);
- pm_runtime_set_suspended(&client->dev);
+ low_power = acpi_dev_state_low_power(&client->dev);
+ if (!low_power)
+ pm_runtime_set_suspended(&client->dev);
return 0;
}
@@ -743,6 +753,7 @@ static struct i2c_driver at24_driver = {
.probe_new = at24_probe,
.remove = at24_remove,
.id_table = at24_ids,
+ .probe_low_power = true,
};
static int __init at24_init(void)
--
2.20.1
wt., 21 sty 2020 o 14:41 Sakari Ailus <[email protected]> napisał(a):
>
> 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 <[email protected]>
> ---
> drivers/misc/eeprom/at24.c | 31 +++++++++++++++++++++----------
> 1 file changed, 21 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/misc/eeprom/at24.c b/drivers/misc/eeprom/at24.c
> index 0681d5fdd538a..5fc1162b67618 100644
> --- a/drivers/misc/eeprom/at24.c
> +++ b/drivers/misc/eeprom/at24.c
> @@ -564,6 +564,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;
> @@ -701,19 +702,24 @@ static int at24_probe(struct i2c_client *client)
>
> i2c_set_clientdata(client, at24);
>
> - /* enable runtime pm */
> - pm_runtime_set_active(dev);
> + low_power = acpi_dev_state_low_power(&client->dev);
> + if (!low_power)
> + pm_runtime_set_active(dev);
> +
> pm_runtime_enable(dev);
>
> /*
> - * 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);
> - pm_runtime_idle(dev);
> - if (err) {
> - pm_runtime_disable(dev);
> - return -ENODEV;
> + if (!low_power) {
> + err = at24_read(at24, 0, &test_byte, 1);
> + pm_runtime_idle(dev);
> + if (err) {
> + pm_runtime_disable(dev);
> + return -ENODEV;
> + }
> }
>
> if (writable)
> @@ -728,8 +734,12 @@ static int at24_probe(struct i2c_client *client)
>
> static int at24_remove(struct i2c_client *client)
> {
> + bool low_power;
> +
> pm_runtime_disable(&client->dev);
> - pm_runtime_set_suspended(&client->dev);
> + low_power = acpi_dev_state_low_power(&client->dev);
This is inconsistent. You define the low_power field in the context
structure (BTW the name low_power is a bit vague here - without
looking at its assignment it would make me think it's about something
battery-related, how about 'off_at_probe'?) and instead of reusing
this field here, you call acpi_dev_state_low_power() again. Either
don't store the context for the life-time of the device if not
necessary or don't call acpi_dev_state_low_power() at remove, although
the commit message doesn't describe whether the latter is done on
purpose.
Bartosz
> + if (!low_power)
> + pm_runtime_set_suspended(&client->dev);
>
> return 0;
> }
> @@ -743,6 +753,7 @@ static struct i2c_driver at24_driver = {
> .probe_new = at24_probe,
> .remove = at24_remove,
> .id_table = at24_ids,
> + .probe_low_power = true,
> };
>
> static int __init at24_init(void)
> --
> 2.20.1
>
Hi Bartosz,
Thanks for the reply.
On Wed, Jan 29, 2020 at 02:36:17PM +0100, Bartosz Golaszewski wrote:
> wt., 21 sty 2020 o 14:41 Sakari Ailus <[email protected]> napisał(a):
> >
> > 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 <[email protected]>
> > ---
> > drivers/misc/eeprom/at24.c | 31 +++++++++++++++++++++----------
> > 1 file changed, 21 insertions(+), 10 deletions(-)
> >
> > diff --git a/drivers/misc/eeprom/at24.c b/drivers/misc/eeprom/at24.c
> > index 0681d5fdd538a..5fc1162b67618 100644
> > --- a/drivers/misc/eeprom/at24.c
> > +++ b/drivers/misc/eeprom/at24.c
> > @@ -564,6 +564,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;
> > @@ -701,19 +702,24 @@ static int at24_probe(struct i2c_client *client)
> >
> > i2c_set_clientdata(client, at24);
> >
> > - /* enable runtime pm */
> > - pm_runtime_set_active(dev);
> > + low_power = acpi_dev_state_low_power(&client->dev);
> > + if (!low_power)
> > + pm_runtime_set_active(dev);
> > +
> > pm_runtime_enable(dev);
> >
> > /*
> > - * 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);
> > - pm_runtime_idle(dev);
> > - if (err) {
> > - pm_runtime_disable(dev);
> > - return -ENODEV;
> > + if (!low_power) {
> > + err = at24_read(at24, 0, &test_byte, 1);
> > + pm_runtime_idle(dev);
> > + if (err) {
> > + pm_runtime_disable(dev);
> > + return -ENODEV;
> > + }
> > }
> >
> > if (writable)
> > @@ -728,8 +734,12 @@ static int at24_probe(struct i2c_client *client)
> >
> > static int at24_remove(struct i2c_client *client)
> > {
> > + bool low_power;
> > +
> > pm_runtime_disable(&client->dev);
> > - pm_runtime_set_suspended(&client->dev);
> > + low_power = acpi_dev_state_low_power(&client->dev);
>
> This is inconsistent. You define the low_power field in the context
> structure (BTW the name low_power is a bit vague here - without
> looking at its assignment it would make me think it's about something
> battery-related, how about 'off_at_probe'?) and instead of reusing
The field was called probe_powered_off in v1, but I changed it to
probe_low_power (and renamed related functions etc.) based on review
comments --- for the device may not be powered off actually.
> this field here, you call acpi_dev_state_low_power() again. Either
> don't store the context for the life-time of the device if not
> necessary or don't call acpi_dev_state_low_power() at remove, although
> the commit message doesn't describe whether the latter is done on
> purpose.
Right. probe-low-power property has the same effect on remove for
consistency, i.e. the device can remain in low power state during remove.
This is documented in probe_low_power field documentation in the first
patch.
--
Regards,
Sakari Ailus
śr., 11 mar 2020 o 09:56 Sakari Ailus <[email protected]> napisał(a):
>
> Hi Bartosz,
>
> Thanks for the reply.
>
> On Wed, Jan 29, 2020 at 02:36:17PM +0100, Bartosz Golaszewski wrote:
> > wt., 21 sty 2020 o 14:41 Sakari Ailus <[email protected]> napisał(a):
> > >
> > > 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 <[email protected]>
> > > ---
> > > drivers/misc/eeprom/at24.c | 31 +++++++++++++++++++++----------
[snip!]
> > >
> > > static int at24_remove(struct i2c_client *client)
> > > {
> > > + bool low_power;
> > > +
> > > pm_runtime_disable(&client->dev);
> > > - pm_runtime_set_suspended(&client->dev);
> > > + low_power = acpi_dev_state_low_power(&client->dev);
> >
> > This is inconsistent. You define the low_power field in the context
> > structure (BTW the name low_power is a bit vague here - without
> > looking at its assignment it would make me think it's about something
> > battery-related, how about 'off_at_probe'?) and instead of reusing
>
> The field was called probe_powered_off in v1, but I changed it to
> probe_low_power (and renamed related functions etc.) based on review
> comments --- for the device may not be powered off actually.
>
But is it actually ever low-power? What are the possible logical
states of the device? If I understood correctly: it's either off or on
at probe - not actually low-power. Am I missing something? In your
cover letter you're writing: "These patches enable calling (and
finishing) a driver's probe function without powering on the
respective device on busses where the practice is to power on the
device for probe." To me there's no mention of a low-power state,
which makes the name 'probe_low_power' seem completely unrelated.
> > this field here, you call acpi_dev_state_low_power() again. Either
> > don't store the context for the life-time of the device if not
> > necessary or don't call acpi_dev_state_low_power() at remove, although
> > the commit message doesn't describe whether the latter is done on
> > purpose.
>
> Right. probe-low-power property has the same effect on remove for
> consistency, i.e. the device can remain in low power state during remove.
> This is documented in probe_low_power field documentation in the first
> patch.
>
Just please don't store any state if you're not using it outside of
the probe() function.
Bartosz
Bartosz,
On Thu, Mar 12, 2020 at 02:10:32PM +0100, Bartosz Golaszewski wrote:
> śr., 11 mar 2020 o 09:56 Sakari Ailus <[email protected]> napisał(a):
> >
> > Hi Bartosz,
> >
> > Thanks for the reply.
> >
> > On Wed, Jan 29, 2020 at 02:36:17PM +0100, Bartosz Golaszewski wrote:
> > > wt., 21 sty 2020 o 14:41 Sakari Ailus <[email protected]> napisał(a):
> > > >
> > > > 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 <[email protected]>
> > > > ---
> > > > drivers/misc/eeprom/at24.c | 31 +++++++++++++++++++++----------
>
> [snip!]
>
> > > >
> > > > static int at24_remove(struct i2c_client *client)
> > > > {
> > > > + bool low_power;
> > > > +
> > > > pm_runtime_disable(&client->dev);
> > > > - pm_runtime_set_suspended(&client->dev);
> > > > + low_power = acpi_dev_state_low_power(&client->dev);
> > >
> > > This is inconsistent. You define the low_power field in the context
> > > structure (BTW the name low_power is a bit vague here - without
> > > looking at its assignment it would make me think it's about something
> > > battery-related, how about 'off_at_probe'?) and instead of reusing
> >
> > The field was called probe_powered_off in v1, but I changed it to
> > probe_low_power (and renamed related functions etc.) based on review
> > comments --- for the device may not be powered off actually.
> >
>
> But is it actually ever low-power? What are the possible logical
> states of the device? If I understood correctly: it's either off or on
> at probe - not actually low-power. Am I missing something? In your
> cover letter you're writing: "These patches enable calling (and
> finishing) a driver's probe function without powering on the
> respective device on busses where the practice is to power on the
> device for probe." To me there's no mention of a low-power state,
> which makes the name 'probe_low_power' seem completely unrelated.
See <URL:https://patchwork.kernel.org/patch/10938483/>
I've updated the patches according to the comments but did not update the
cover page accordingly.
Generally drivers are interested whether a device is powered on so it can
be accessed, but the actual power state of the device isn't known to the
driver when it is, well, not in an operational state. A device may be
powered from a regulator that is always enabled, for instance.
>
> > > this field here, you call acpi_dev_state_low_power() again. Either
> > > don't store the context for the life-time of the device if not
> > > necessary or don't call acpi_dev_state_low_power() at remove, although
> > > the commit message doesn't describe whether the latter is done on
> > > purpose.
> >
> > Right. probe-low-power property has the same effect on remove for
> > consistency, i.e. the device can remain in low power state during remove.
> > This is documented in probe_low_power field documentation in the first
> > patch.
> >
>
> Just please don't store any state if you're not using it outside of
> the probe() function.
What exactly are you referring to? The patch adds a local variable to the
driver's probe and remove functions.
--
Kind regards,
Sakari Ailus
pon., 23 mar 2020 o 22:31 Sakari Ailus <[email protected]>
napisał(a):
>
> Bartosz,
>
> On Thu, Mar 12, 2020 at 02:10:32PM +0100, Bartosz Golaszewski wrote:
> > śr., 11 mar 2020 o 09:56 Sakari Ailus <[email protected]> napisał(a):
> > >
> > > Hi Bartosz,
> > >
> > > Thanks for the reply.
> > >
> > > On Wed, Jan 29, 2020 at 02:36:17PM +0100, Bartosz Golaszewski wrote:
> > > > wt., 21 sty 2020 o 14:41 Sakari Ailus <[email protected]> napisał(a):
> > > > >
> > > > > 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 <[email protected]>
> > > > > ---
> > > > > drivers/misc/eeprom/at24.c | 31 +++++++++++++++++++++----------
> >
> > [snip!]
> >
> > > > >
> > > > > static int at24_remove(struct i2c_client *client)
> > > > > {
> > > > > + bool low_power;
> > > > > +
> > > > > pm_runtime_disable(&client->dev);
> > > > > - pm_runtime_set_suspended(&client->dev);
> > > > > + low_power = acpi_dev_state_low_power(&client->dev);
> > > >
> > > > This is inconsistent. You define the low_power field in the context
> > > > structure (BTW the name low_power is a bit vague here - without
> > > > looking at its assignment it would make me think it's about something
> > > > battery-related, how about 'off_at_probe'?) and instead of reusing
> > >
> > > The field was called probe_powered_off in v1, but I changed it to
> > > probe_low_power (and renamed related functions etc.) based on review
> > > comments --- for the device may not be powered off actually.
> > >
> >
> > But is it actually ever low-power? What are the possible logical
> > states of the device? If I understood correctly: it's either off or on
> > at probe - not actually low-power. Am I missing something? In your
> > cover letter you're writing: "These patches enable calling (and
> > finishing) a driver's probe function without powering on the
> > respective device on busses where the practice is to power on the
> > device for probe." To me there's no mention of a low-power state,
> > which makes the name 'probe_low_power' seem completely unrelated.
>
> See <URL:https://patchwork.kernel.org/patch/10938483/>
>
> I've updated the patches according to the comments but did not update the
> cover page accordingly.
>
I see.
Rafael: I think that there are two issues with patch 1/5:
1. It adds a very specific boolean flag to a structure that's meant to
be very general. As I pointed out in the i2c patch: at the very least
this could be made into an int storing flag values, instead of a
boolean field. But rather than that - it looks to me more like a
device (or bus) feature than a driver feature. Is there any ACPI flag
we could use to pass this information to the driver model without
changing the driver structure?
2. The name is still misleading: probe_low_power doesn't correspond
with what it actually does at all (neither did power_off). I'd go with
something like probe_allow_low_power.
> Generally drivers are interested whether a device is powered on so it can
> be accessed, but the actual power state of the device isn't known to the
> driver when it is, well, not in an operational state. A device may be
> powered from a regulator that is always enabled, for instance.
>
> >
> > > > this field here, you call acpi_dev_state_low_power() again. Either
> > > > don't store the context for the life-time of the device if not
> > > > necessary or don't call acpi_dev_state_low_power() at remove, although
> > > > the commit message doesn't describe whether the latter is done on
> > > > purpose.
> > >
> > > Right. probe-low-power property has the same effect on remove for
> > > consistency, i.e. the device can remain in low power state during remove.
> > > This is documented in probe_low_power field documentation in the first
> > > patch.
> > >
> >
> > Just please don't store any state if you're not using it outside of
> > the probe() function.
>
> What exactly are you referring to? The patch adds a local variable to the
> driver's probe and remove functions.
>
Yes, sorry, I looked at the patch and somehow thought it adds a new
field to the data structure and then doesn't reuse it. My bad. Maybe
it was a previous version IDK.
Bartosz
> --
> Kind regards,
>
> Sakari Ailus
Hi Bartosz,
Apologies for the late reply --- I was expecting more discussion which
never happened...
On Wed, Mar 25, 2020 at 02:48:47PM +0100, Bartosz Golaszewski wrote:
> pon., 23 mar 2020 o 22:31 Sakari Ailus <[email protected]>
> napisał(a):
> >
> > Bartosz,
> >
> > On Thu, Mar 12, 2020 at 02:10:32PM +0100, Bartosz Golaszewski wrote:
> > > śr., 11 mar 2020 o 09:56 Sakari Ailus <[email protected]> napisał(a):
> > > >
> > > > Hi Bartosz,
> > > >
> > > > Thanks for the reply.
> > > >
> > > > On Wed, Jan 29, 2020 at 02:36:17PM +0100, Bartosz Golaszewski wrote:
> > > > > wt., 21 sty 2020 o 14:41 Sakari Ailus <[email protected]> napisał(a):
> > > > > >
> > > > > > 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 <[email protected]>
> > > > > > ---
> > > > > > drivers/misc/eeprom/at24.c | 31 +++++++++++++++++++++----------
> > >
> > > [snip!]
> > >
> > > > > >
> > > > > > static int at24_remove(struct i2c_client *client)
> > > > > > {
> > > > > > + bool low_power;
> > > > > > +
> > > > > > pm_runtime_disable(&client->dev);
> > > > > > - pm_runtime_set_suspended(&client->dev);
> > > > > > + low_power = acpi_dev_state_low_power(&client->dev);
> > > > >
> > > > > This is inconsistent. You define the low_power field in the context
> > > > > structure (BTW the name low_power is a bit vague here - without
> > > > > looking at its assignment it would make me think it's about something
> > > > > battery-related, how about 'off_at_probe'?) and instead of reusing
> > > >
> > > > The field was called probe_powered_off in v1, but I changed it to
> > > > probe_low_power (and renamed related functions etc.) based on review
> > > > comments --- for the device may not be powered off actually.
> > > >
> > >
> > > But is it actually ever low-power? What are the possible logical
> > > states of the device? If I understood correctly: it's either off or on
> > > at probe - not actually low-power. Am I missing something? In your
> > > cover letter you're writing: "These patches enable calling (and
> > > finishing) a driver's probe function without powering on the
> > > respective device on busses where the practice is to power on the
> > > device for probe." To me there's no mention of a low-power state,
> > > which makes the name 'probe_low_power' seem completely unrelated.
> >
> > See <URL:https://patchwork.kernel.org/patch/10938483/>
> >
> > I've updated the patches according to the comments but did not update the
> > cover page accordingly.
> >
>
> I see.
>
> Rafael: I think that there are two issues with patch 1/5:
> 1. It adds a very specific boolean flag to a structure that's meant to
> be very general. As I pointed out in the i2c patch: at the very least
> this could be made into an int storing flag values, instead of a
> boolean field. But rather than that - it looks to me more like a
> device (or bus) feature than a driver feature. Is there any ACPI flag
> we could use to pass this information to the driver model without
> changing the driver structure?
To my knowledge there isn't. The fact that I²C devices are powered on for
probe in ACPI based systems is specific to Linux kernel and not ACPI as
such.
The reason this needs to be in a generic struct is that the device's power
state will be changed before any interaction with the driver takes place as
it's the I²C framework that powers on the device.
The firmware hint is there in order to make sure as this is intended, as
this could have unwanted effects if it were just up to driver support.
Think of the at24 driver, for instance: we probably want probe to fail if
the device isn't accessible in most cases.
> 2. The name is still misleading: probe_low_power doesn't correspond
> with what it actually does at all (neither did power_off). I'd go with
> something like probe_allow_low_power.
I agree. I'll rename the property accrodingly as well, for that's what it
really suggests.
--
Sakari Ailus
On Mon, Aug 10, 2020 at 10:26 AM Sakari Ailus <[email protected]> wrote:
>
[snip]
> >
> > Rafael: I think that there are two issues with patch 1/5:
> > 1. It adds a very specific boolean flag to a structure that's meant to
> > be very general. As I pointed out in the i2c patch: at the very least
> > this could be made into an int storing flag values, instead of a
> > boolean field. But rather than that - it looks to me more like a
> > device (or bus) feature than a driver feature. Is there any ACPI flag
> > we could use to pass this information to the driver model without
> > changing the driver structure?
>
> To my knowledge there isn't. The fact that I²C devices are powered on for
> probe in ACPI based systems is specific to Linux kernel and not ACPI as
> such.
>
> The reason this needs to be in a generic struct is that the device's power
> state will be changed before any interaction with the driver takes place as
> it's the I²C framework that powers on the device.
>
I'm not sure I'm following. Looking at patch 1/6 struct device already
exists so why can't this information be conveyed "per device" as
opposed to "per driver"?
[snip]
Bartosz
Hi Bartosz,
On Mon, Aug 10, 2020 at 08:12:00PM +0200, Bartosz Golaszewski wrote:
> On Mon, Aug 10, 2020 at 10:26 AM Sakari Ailus <[email protected]> wrote:
> >
>
> [snip]
>
> > >
> > > Rafael: I think that there are two issues with patch 1/5:
> > > 1. It adds a very specific boolean flag to a structure that's meant to
> > > be very general. As I pointed out in the i2c patch: at the very least
> > > this could be made into an int storing flag values, instead of a
> > > boolean field. But rather than that - it looks to me more like a
> > > device (or bus) feature than a driver feature. Is there any ACPI flag
> > > we could use to pass this information to the driver model without
> > > changing the driver structure?
> >
> > To my knowledge there isn't. The fact that I?C devices are powered on for
> > probe in ACPI based systems is specific to Linux kernel and not ACPI as
> > such.
> >
> > The reason this needs to be in a generic struct is that the device's power
> > state will be changed before any interaction with the driver takes place as
> > it's the I?C framework that powers on the device.
> >
>
> I'm not sure I'm following. Looking at patch 1/6 struct device already
> exists so why can't this information be conveyed "per device" as
> opposed to "per driver"?
It's both driver and device.
Suppose there's no indication of driver support. If you add the property
telling the device shouldn't be powered on for probe, it won't be. And if
the driver doesn't support that, probe will fail. That could happen e.g.
when running an older kernel on a system that happens to specify this
property for a given device.
You could view this as a driver bug of course. I still think it's better to
make driver support for this explicit, and avoid making this a practical
problem anywhere.
--
Kind regards,
Sakari Ailus
On Tue, Aug 11, 2020 at 10:00 AM Sakari Ailus <[email protected]> wrote:
>
> Hi Bartosz,
>
> On Mon, Aug 10, 2020 at 08:12:00PM +0200, Bartosz Golaszewski wrote:
> > On Mon, Aug 10, 2020 at 10:26 AM Sakari Ailus <[email protected]> wrote:
> > >
> >
> > [snip]
> >
> > > >
> > > > Rafael: I think that there are two issues with patch 1/5:
> > > > 1. It adds a very specific boolean flag to a structure that's meant to
> > > > be very general. As I pointed out in the i2c patch: at the very least
> > > > this could be made into an int storing flag values, instead of a
> > > > boolean field. But rather than that - it looks to me more like a
> > > > device (or bus) feature than a driver feature. Is there any ACPI flag
> > > > we could use to pass this information to the driver model without
> > > > changing the driver structure?
> > >
> > > To my knowledge there isn't. The fact that I涎 devices are powered on for
> > > probe in ACPI based systems is specific to Linux kernel and not ACPI as
> > > such.
> > >
> > > The reason this needs to be in a generic struct is that the device's power
> > > state will be changed before any interaction with the driver takes place as
> > > it's the I涎 framework that powers on the device.
> > >
> >
> > I'm not sure I'm following. Looking at patch 1/6 struct device already
> > exists so why can't this information be conveyed "per device" as
> > opposed to "per driver"?
>
> It's both driver and device.
>
> Suppose there's no indication of driver support. If you add the property
> telling the device shouldn't be powered on for probe, it won't be. And if
> the driver doesn't support that, probe will fail. That could happen e.g.
> when running an older kernel on a system that happens to specify this
> property for a given device.
>
> You could view this as a driver bug of course. I still think it's better to
> make driver support for this explicit, and avoid making this a practical
> problem anywhere.
>
I see. I'm not sure this is the correct solution but let's see what
Wolfram says. From my side: I'd prefer to see the
disable_i2c_core_irq_mapping converted to flags first and then the
flags extended with whatever you need. disable_i2c_core_irq_mapping
could also be removed AFAICT - nobody uses it.
Bart
> Wolfram says. From my side: I'd prefer to see the
> disable_i2c_core_irq_mapping converted to flags first and then the
> flags extended with whatever you need. disable_i2c_core_irq_mapping
> could also be removed AFAICT - nobody uses it.
I haven't read the details here, just saying that
'disable_i2c_core_irq_mapping' is already removed in -next and also
within the next days in Linus' tree.
On Wed, Aug 12, 2020 at 9:25 PM Wolfram Sang <[email protected]> wrote:
>
>
> > Wolfram says. From my side: I'd prefer to see the
> > disable_i2c_core_irq_mapping converted to flags first and then the
> > flags extended with whatever you need. disable_i2c_core_irq_mapping
> > could also be removed AFAICT - nobody uses it.
>
> I haven't read the details here, just saying that
> 'disable_i2c_core_irq_mapping' is already removed in -next and also
> within the next days in Linus' tree.
>
Ok, then nevermind my previous comment.
Bart