Re: [PATCH v15 2/2] i2c: core: support bus regulator controlling in adapter

[Tomasz Figa <tfiga@chromium.org>]

Hi Grygorii,

On Fri, May 22, 2020 at 7:59 PM Grygorii Strashko
<grygorii.strashko@ti.com> wrote:
>
>
>
> On 19/05/2020 10:27, Bibby Hsieh 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@mediatek.com>
> > ---
> >   drivers/i2c/i2c-core-base.c | 84 +++++++++++++++++++++++++++++++++++++
> >   include/linux/i2c.h         |  2 +
> >   2 files changed, 86 insertions(+)
> >
> > diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
> > index 5cc0b0ec5570..e1cc8d46bc51 100644
> > --- a/drivers/i2c/i2c-core-base.c
> > +++ b/drivers/i2c/i2c-core-base.c
> > @@ -313,12 +313,14 @@ 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;
> >       struct i2c_driver       *driver;
> >       int status;
> >
> >       if (!client)
> >               return 0;
> >
> > +     adap = client->adapter;
> >       driver = to_i2c_driver(dev->driver);
> >
> >       client->irq = client->init_irq;
> > @@ -378,6 +380,12 @@ static int i2c_device_probe(struct device *dev)
> >
> >       dev_dbg(dev, "probe\n");
> >
> > +     status = regulator_enable(adap->bus_regulator);
> > +     if (status < 0) {
> > +             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,12 +422,14 @@ 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;
> >       struct i2c_driver       *driver;
> >       int status = 0;
> >
> >       if (!client || !dev->driver)
> >               return 0;
> >
> > +     adap = client->adapter;
> >       driver = to_i2c_driver(dev->driver);
> >       if (driver->remove) {
> >               dev_dbg(dev, "remove\n");
> > @@ -427,6 +437,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);
>
> Not sure this check is correct.
>
> i2c_device_probe()
>   - regulator_enable - 1
>
> pm_runtime_get()
>   - regulator_enable - 2
>

I believe regulator_enable() wouldn't be called again, because the
device was already active in probe. However, I've been having
difficulties keeping track of runtime PM semantics under various
circumstances (e.g. ACPI vs DT), so can't tell for sure anymore.

> i2c_device_remove()
>   - pm_runtime_status_suspended() flase
>     - regulator_disable() - 1 --> still active?
>
> Sorry, I probably missing smth.
>
> >
> >       dev_pm_clear_wake_irq(&client->dev);
> >       device_init_wakeup(&client->dev, false);
> > @@ -438,6 +450,72 @@ static int i2c_device_remove(struct device *dev)
> >       return status;
> >   }
> >
>
> [...]
>
> --
> Best regards,
> grygorii