Hi Bibby,
On Fri, Mar 6, 2020 at 12:49 PM Bibby Hsieh <bibby.hsieh@xxxxxxxxxxxx> wrote:
>
> Although in the most platforms, the bus power of i2c
> are alway on, some platforms disable the i2c bus power
> in order to meet low power request.
>
> We get and enable bulk regulator in i2c adapter device.
>
> Signed-off-by: Bibby Hsieh <bibby.hsieh@xxxxxxxxxxxx>
> ---
> drivers/i2c/i2c-core-base.c | 81 +++++++++++++++++++++++++++++++++++++
> include/linux/i2c.h | 2 +
> 2 files changed, 83 insertions(+)
>
Thanks for the patch. Unfortunately there are still some issues left.
Please check my comments inline.
> diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
> index cefad0881942..4a1cee8389b4 100644
> --- a/drivers/i2c/i2c-core-base.c
> +++ b/drivers/i2c/i2c-core-base.c
> @@ -313,6 +313,7 @@ static int i2c_smbus_host_notify_to_irq(const struct i2c_client *client)
> static int i2c_device_probe(struct device *dev)
> {
> struct i2c_client *client = i2c_verify_client(dev);
> + struct i2c_adapter *adap = client->adapter;
> struct i2c_driver *driver;
> int status;
>
> @@ -378,6 +379,12 @@ static int i2c_device_probe(struct device *dev)
>
> dev_dbg(dev, "probe\n");
>
> + status = regulator_enable(adap->bus_regulator);
> + if (status != 0) {
I'd make the check < 0, as the API doesn't specify the non-negative
values explicitly.
> + dev_err(&adap->dev, "Failed to enable power regulator\n");
> + goto err_clear_wakeup_irq;
> + }
> +
> status = of_clk_set_defaults(dev->of_node, false);
> if (status < 0)
> goto err_clear_wakeup_irq;
> @@ -414,6 +421,7 @@ static int i2c_device_probe(struct device *dev)
> static int i2c_device_remove(struct device *dev)
> {
> struct i2c_client *client = i2c_verify_client(dev);
> + struct i2c_adapter *adap = client->adapter;
> struct i2c_driver *driver;
> int status = 0;
>
> @@ -427,6 +435,8 @@ static int i2c_device_remove(struct device *dev)
> }
>
> dev_pm_domain_detach(&client->dev, true);
> + if (!pm_runtime_status_suspended(&client->dev))
> + regulator_disable(adap->bus_regulator);
>
> dev_pm_clear_wake_irq(&client->dev);
> device_init_wakeup(&client->dev, false);
> @@ -438,6 +448,71 @@ static int i2c_device_remove(struct device *dev)
> return status;
> }
>
> +#ifdef CONFIG_PM_SLEEP
> +static int i2c_resume_early(struct device *dev)
> +{
> + struct i2c_client *client = i2c_verify_client(dev);
> + struct i2c_adapter *adap = client->adapter;
> + int err;
> +
> + if (pm_runtime_status_suspended(&client->dev)) {
> + err = regulator_enable(adap->bus_regulator);
> + if (err)
> + return err;
> + }
> +
> + return pm_generic_resume_early(&client->dev);
> +}
> +
> +static int i2c_suspend_late(struct device *dev)
> +{
> + struct i2c_client *client = i2c_verify_client(dev);
> + struct i2c_adapter *adap = client->adapter;
> + int err;
> +
> + if (!pm_runtime_status_suspended(&client->dev)) {
> + err = regulator_disable(adap->bus_regulator);
> + if (err)
> + return err;
> + }
> +
> + return pm_generic_suspend_late(&client->dev);
Let's think a bit more about the order of operations above. It could
end up with following sequence:
- regulator_disable()
- pm_generic_suspend_late()
- some_i2c_device_suspend_late()
- i2c_transfer() - ERROR because the bus regulator is disabled
We can only disable the regulator once the i2c client driver finished
its callback and suspended fully, which should guarantee no further
I2C accesses.
> +}
> +#endif
> +
> +#ifdef CONFIG_PM
> +static int i2c_runtime_resume(struct device *dev)
> +{
> + struct i2c_client *client = i2c_verify_client(dev);
> + struct i2c_adapter *adap = client->adapter;
> + int err;
> +
> + err = regulator_enable(adap->bus_regulator);
> + if (err)
> + return err;
> +
> + return pm_generic_runtime_resume(&client->dev);
> +}
> +
> +static int i2c_runtime_suspend(struct device *dev)
> +{
> + struct i2c_client *client = i2c_verify_client(dev);
> + struct i2c_adapter *adap = client->adapter;
> + int err;
> +
> + err = pm_generic_runtime_suspend(&client->dev);
> + if (err)
> + return err;
> +
> + return regulator_disable(adap->bus_regulator);
> +}
> +#endif
> +
> +static const struct dev_pm_ops i2c_device_pm = {
> + SET_SYSTEM_SLEEP_PM_OPS(i2c_suspend_late, i2c_resume_early)
We wanted to set the late/early ops, but this macro sets the regular
suspend/resume ops. SET_LATE_SYSTEM_SLEEP_PM_OPS should be used
instead.
> + SET_RUNTIME_PM_OPS(i2c_runtime_suspend, i2c_runtime_resume, NULL)
> +};
> +
> static void i2c_device_shutdown(struct device *dev)
> {
> struct i2c_client *client = i2c_verify_client(dev);
> @@ -495,6 +570,7 @@ struct bus_type i2c_bus_type = {
> .probe = i2c_device_probe,
> .remove = i2c_device_remove,
> .shutdown = i2c_device_shutdown,
> + .pm = &i2c_device_pm,
> };
> EXPORT_SYMBOL_GPL(i2c_bus_type);
>
> @@ -1334,6 +1410,11 @@ static int i2c_register_adapter(struct i2c_adapter *adap)
> goto out_reg;
>
> dev_dbg(&adap->dev, "adapter [%s] registered\n", adap->name);
> + adap->bus_regulator = devm_regulator_get(&adap->dev, "bus");
> + if (IS_ERR(adap->bus_regulator)) {
> + res = PTR_ERR(adap->bus_regulator);
> + goto out_reg;
> + }
Wouldn't it be better to move this code before the dev_dbg() call? It
could be confusing if the debug log says "registered" and then the
devm_regulator_get() fails.
Best regards,
Tomasz
