| Message ID | 20200810142747.12400-2-sakari.ailus@linux.intel.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Support running driver's probe for a device powered off | expand |
On Mon, Aug 10, 2020 at 05:27:42PM +0300, Sakari Ailus wrote: > Enable drivers to tell ACPI that there's no need to power on a device for > probe. Drivers should still perform this by themselves if there's a need > to. In some cases powering on the device during probe is undesirable, and > this change enables a driver to choose what fits best for it. > > Add a field called "flags" into struct i2c_driver for driver flags, and a > flag I2C_DRV_FL_ALLOW_LOW_POWER_PROBE to tell a driver supports probe in > low power state. > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> > --- > drivers/i2c/i2c-core-base.c | 17 ++++++++++++++--- > include/linux/i2c.h | 14 ++++++++++++++ > 2 files changed, 28 insertions(+), 3 deletions(-) > > diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c > index 34a9609f256da..cde9cf49a07e6 100644 > --- a/drivers/i2c/i2c-core-base.c > +++ b/drivers/i2c/i2c-core-base.c > @@ -436,6 +436,14 @@ static int i2c_smbus_host_notify_to_irq(const struct i2c_client *client) > return irq > 0 ? irq : -ENXIO; > } > > +static bool allow_low_power_probe(struct device *dev) > +{ > + struct i2c_driver *driver = to_i2c_driver(dev->driver); > + > + return driver->flags & I2C_DRV_FL_ALLOW_LOW_POWER_PROBE && > + device_property_present(dev, "allow-low-power-probe"); I assume this change makes even the DT property "allow-low-power-probe" work in the same way. Should we have proper DT binding for that ? This comment applies for any property using device_property_* but has no explicit DT binding ? Just asking the question to know the strategy followed. Sorry if this is redundant question, feel free to point me to the past discussions. -- Regards, Sudeep
Hi Sudeep, Thanks for the review. On Mon, Aug 10, 2020 at 03:41:48PM +0100, Sudeep Holla wrote: > On Mon, Aug 10, 2020 at 05:27:42PM +0300, Sakari Ailus wrote: > > Enable drivers to tell ACPI that there's no need to power on a device for > > probe. Drivers should still perform this by themselves if there's a need > > to. In some cases powering on the device during probe is undesirable, and > > this change enables a driver to choose what fits best for it. > > > > Add a field called "flags" into struct i2c_driver for driver flags, and a > > flag I2C_DRV_FL_ALLOW_LOW_POWER_PROBE to tell a driver supports probe in > > low power state. > > > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> > > --- > > drivers/i2c/i2c-core-base.c | 17 ++++++++++++++--- > > include/linux/i2c.h | 14 ++++++++++++++ > > 2 files changed, 28 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c > > index 34a9609f256da..cde9cf49a07e6 100644 > > --- a/drivers/i2c/i2c-core-base.c > > +++ b/drivers/i2c/i2c-core-base.c > > @@ -436,6 +436,14 @@ static int i2c_smbus_host_notify_to_irq(const struct i2c_client *client) > > return irq > 0 ? irq : -ENXIO; > > } > > > > +static bool allow_low_power_probe(struct device *dev) > > +{ > > + struct i2c_driver *driver = to_i2c_driver(dev->driver); > > + > > + return driver->flags & I2C_DRV_FL_ALLOW_LOW_POWER_PROBE && > > + device_property_present(dev, "allow-low-power-probe"); > > I assume this change makes even the DT property "allow-low-power-probe" > work in the same way. Should we have proper DT binding for that ? > > This comment applies for any property using device_property_* but has > no explicit DT binding ? Just asking the question to know the strategy > followed. Sorry if this is redundant question, feel free to point me > to the past discussions. It's not a redundant question, no. I²C drivers on OF are responsible for controlling device's power state already (using runtime PM or without) so I think the drivers could use the property directly on OF systems (and document the property in DT bindings first) if there's a need to. IOW this code isn't needed on OF. Note that the power_on or power_off arguments are not used by genpd_dev_pm_attach() or genpd_dev_pm_detach() so this patch only affects ACPI. I think I should check the device is an ACPI device above, for clarity. Cc also DT list. The entire set is here: <URL:https://lore.kernel.org/linux-acpi/20200810142747.12400-1-sakari.ailus@linux.intel.com/>
diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c index 34a9609f256da..cde9cf49a07e6 100644 --- a/drivers/i2c/i2c-core-base.c +++ b/drivers/i2c/i2c-core-base.c @@ -436,6 +436,14 @@ static int i2c_smbus_host_notify_to_irq(const struct i2c_client *client) return irq > 0 ? irq : -ENXIO; } +static bool allow_low_power_probe(struct device *dev) +{ + struct i2c_driver *driver = to_i2c_driver(dev->driver); + + return driver->flags & I2C_DRV_FL_ALLOW_LOW_POWER_PROBE && + device_property_present(dev, "allow-low-power-probe"); +} + static int i2c_device_probe(struct device *dev) { struct i2c_client *client = i2c_verify_client(dev); @@ -514,7 +522,8 @@ static int i2c_device_probe(struct device *dev) if (status < 0) goto err_clear_wakeup_irq; - status = dev_pm_domain_attach(&client->dev, true); + status = dev_pm_domain_attach(&client->dev, + !allow_low_power_probe(&client->dev)); if (status) goto err_clear_wakeup_irq; @@ -536,7 +545,8 @@ static int i2c_device_probe(struct device *dev) return 0; err_detach_pm_domain: - dev_pm_domain_detach(&client->dev, true); + dev_pm_domain_detach(&client->dev, + !allow_low_power_probe(&client->dev)); err_clear_wakeup_irq: dev_pm_clear_wake_irq(&client->dev); device_init_wakeup(&client->dev, false); @@ -562,7 +572,8 @@ static int i2c_device_remove(struct device *dev) status = driver->remove(client); } - dev_pm_domain_detach(&client->dev, true); + dev_pm_domain_detach(&client->dev, + !allow_low_power_probe(&client->dev)); dev_pm_clear_wake_irq(&client->dev); device_init_wakeup(&client->dev, false); diff --git a/include/linux/i2c.h b/include/linux/i2c.h index fc55ea41d3237..6824719c24ebe 100644 --- a/include/linux/i2c.h +++ b/include/linux/i2c.h @@ -11,6 +11,7 @@ #define _LINUX_I2C_H #include <linux/acpi.h> /* for acpi_handle */ +#include <linux/bits.h> #include <linux/mod_devicetable.h> #include <linux/device.h> /* for struct device */ #include <linux/sched.h> /* for completion */ @@ -217,6 +218,16 @@ enum i2c_alert_protocol { I2C_PROTOCOL_SMBUS_HOST_NOTIFY, }; +/** + * enum i2c_driver_flags - Flags for an I2C device driver + * + * @I2C_DRV_FL_ALLOW_LOW_POWER_PROBE: Let the driver manage the device's power + * state during probe and remove + */ +enum i2c_driver_flags { + I2C_DRV_FL_ALLOW_LOW_POWER_PROBE = BIT(0), +}; + /** * struct i2c_driver - represent an I2C device driver * @class: What kind of i2c device we instantiate (for detect) @@ -231,6 +242,7 @@ enum i2c_alert_protocol { * @detect: Callback for device detection * @address_list: The I2C addresses to probe (for detect) * @clients: List of detected clients we created (for i2c-core use only) + * @flags: A bitmask of flags defined in &enum i2c_driver_flags * * The driver.owner field should be set to the module owner of this driver. * The driver.name field should be set to the name of this driver. @@ -289,6 +301,8 @@ struct i2c_driver { int (*detect)(struct i2c_client *client, struct i2c_board_info *info); const unsigned short *address_list; struct list_head clients; + + unsigned int flags; }; #define to_i2c_driver(d) container_of(d, struct i2c_driver, driver)
Enable drivers to tell ACPI that there's no need to power on a device for probe. Drivers should still perform this by themselves if there's a need to. In some cases powering on the device during probe is undesirable, and this change enables a driver to choose what fits best for it. Add a field called "flags" into struct i2c_driver for driver flags, and a flag I2C_DRV_FL_ALLOW_LOW_POWER_PROBE to tell a driver supports probe in low power state. Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> --- drivers/i2c/i2c-core-base.c | 17 ++++++++++++++--- include/linux/i2c.h | 14 ++++++++++++++ 2 files changed, 28 insertions(+), 3 deletions(-)