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

[Grygorii Strashko <grygorii.strashko@ti.com>]

On 25/05/2020 14:34, Tomasz Figa wrote:
> 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.

True.

I've found pretty useful:
  - CONFIG_PM_ADVANCED_DEBUG
  - bind/unbind
for such testing.

for regulators - num_users\x02 can be checked.


> 
>> 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

-- 
Best regards,
grygorii