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

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

On 28/04/2020 14:44, Tomasz Figa wrote:
> On Tue, Apr 28, 2020 at 1:25 PM Grygorii Strashko
> <grygorii.strashko@ti.com> wrote:
>>
>>
>>
>> On 28/04/2020 09:18, 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 | 82 +++++++++++++++++++++++++++++++++++++
>>>    include/linux/i2c.h         |  2 +
>>>    2 files changed, 84 insertions(+)
>>>
>>> diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
>>> index 5cc0b0ec5570..f81b42a4ed07 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) {
>>> +             dev_err(&adap->dev, "Failed to enable power regulator\n");
>>> +             goto err_clear_wakeup_irq;
>>> +     }
>>> +
>>
>> Sry, but this is confusing.
>> What if there is separate regulators for I2C device and bus/adapter?
>>
>> I2C bus is transaction based and usually I2C bus drivers ensures that i2c bus is
>> in proper state to perform transaction. While I2C devices can be enable, configured and
>> function without actually interacting with I2C bus unless required (irq for example).
>>
>> With you change any I2C device will enable and keep bus regulator on all the time it's active
>> even if there is no I2C interruptions.
> 
> The I2C SDA/SCL lines must stay high for all the time the bus is idle.
> The regulator in question here is exactly the regulator that drives
> the voltage rail the SDA/SCL lines are pulled up to and so it needs to
> be enabled all the time any slave device is active and expects the I2C
> bus to be in a valid state.
> 
>>
>> Following the problem description it seems
>>    - i2c bus driver should get regulator and ensure it's enabled for the duration of transaction(s)
> 
> That's not necessary given the point below, because the slave driver
> must not trigger any I2C transactions when the slave device is not
> active.
> 
>>    - i2c device should get its own regulator (or the same if shared)  ensure it's enabled for
>>      the period device is active.
> 
> This is a board design aspect and not specific to any particular I2C
> slave device, so slave drivers should not be aware of it. This patch
> exactly attempts to get this SDA/SCL regulator on behalf of the slave
> device.

Thank you for your explanation.



-- 
Best regards,
grygorii