Re: [PATCH 5/9] i2c: qcom-cci: initialize CCI controller after registration of adapters

[Vladimir Zapolskiy <vladimir.zapolskiy@linaro.org>]

Hi Loic,

On 2/3/22 7:29 PM, Loic Poulain wrote:
> Hi Vladimir,
> 
> On Thu, 3 Feb 2022 at 17:47, Vladimir Zapolskiy
> <vladimir.zapolskiy@linaro.org> wrote:
>>
>> The change is wanted to postpone initialization of busses on CCI controller
>> by cci_init() and cci_reset() till adapters are registered, the later is
>> needed for adding I2C bus devices and get correspondent vbus regulators.
> 
> This is odd, I don't think it's a good idea to register an adapter
> which is not yet initialized. Could you elaborate on why you need to
> do this, if you can't access the controller without this regulator
> enabled, maybe it is more than vbus supply, and, in that case, it
> should be enabled from your probe function.

thank you for review, the controller can be accessed without a vbus regulator,
but I2C devices connected to the master controller can not.

The registration of a master controller device done by i2c_add_adapter()
should be safe to defer IMO, because there shall be no communication on
the bus at this point, there are no slaves before probe completion, thus
cci_init()/cci_reset() can be safely called afterwards.

The rationale of the change is to merge two loops over busses, see change 6/9,
keeping two loops extremely complicates the proper resource management.

--
Best wishes,
Vladimir

>>
>> Signed-off-by: Vladimir Zapolskiy <vladimir.zapolskiy@linaro.org>
>> ---
>>   drivers/i2c/busses/i2c-qcom-cci.c | 17 ++++++++---------
>>   1 file changed, 8 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/i2c/busses/i2c-qcom-cci.c b/drivers/i2c/busses/i2c-qcom-cci.c
>> index cf54f1cb4c57..eebf9603d3d1 100644
>> --- a/drivers/i2c/busses/i2c-qcom-cci.c
>> +++ b/drivers/i2c/busses/i2c-qcom-cci.c
>> @@ -630,14 +630,6 @@ static int cci_probe(struct platform_device *pdev)
>>          val = readl(cci->base + CCI_HW_VERSION);
>>          dev_dbg(dev, "CCI HW version = 0x%08x", val);
>>
>> -       ret = cci_reset(cci);
>> -       if (ret < 0)
>> -               goto error;
>> -
>> -       ret = cci_init(cci);
>> -       if (ret < 0)
>> -               goto error;
>> -
>>          for (i = 0; i < cci->data->num_masters; i++) {
>>                  if (!cci->master[i].cci)
>>                          continue;
>> @@ -649,6 +641,14 @@ static int cci_probe(struct platform_device *pdev)
>>                  }
>>          }
>>
>> +       ret = cci_reset(cci);
>> +       if (ret < 0)
>> +               goto error_i2c;
>> +
>> +       ret = cci_init(cci);
>> +       if (ret < 0)
>> +               goto error_i2c;
>> +
>>          pm_runtime_set_autosuspend_delay(dev, MSEC_PER_SEC);
>>          pm_runtime_use_autosuspend(dev);
>>          pm_runtime_set_active(dev);
>> @@ -663,7 +663,6 @@ static int cci_probe(struct platform_device *pdev)
>>                          of_node_put(cci->master[i].adap.dev.of_node);
>>                  }
>>          }
>> -error:
>>          disable_irq(cci->irq);
>>   disable_clocks:
>>          cci_disable_clocks(cci);
>> --
>> 2.33.0
>>