mbox series

[0/2] i2c: mv64xxx: reset-gpios

Message ID 20231012035838.2804064-1-chris.packham@alliedtelesis.co.nz
Headers show
Series i2c: mv64xxx: reset-gpios | expand

Message

Chris Packham Oct. 12, 2023, 3:58 a.m. UTC 
This series adds the ability to associate a gpio with an I2C bus so that
downstream devices can be brought out of reset when the host controller is
probed.

Chris Packham (2):
  dt-bindings: i2c: mv64xxx: add reset-gpios property
  i2c: mv64xxx: add an optional reset-gpios property

 .../devicetree/bindings/i2c/marvell,mv64xxx-i2c.yaml  |  3 +++
 drivers/i2c/busses/i2c-mv64xxx.c                      | 11 +++++++++++
 2 files changed, 14 insertions(+)

Comments

Peter Rosin Oct. 12, 2023, 10:49 a.m. UTC | #1 
Hi!

2023-10-12 at 12:21, Andi Shyti wrote:
> Hi Chris,
> 
> ...
> 
>>  static struct mv64xxx_i2c_regs mv64xxx_i2c_regs_mv64xxx = {
>> @@ -1083,6 +1084,10 @@ mv64xxx_i2c_probe(struct platform_device *pd)
>>  	if (drv_data->irq < 0)
>>  		return drv_data->irq;
>>  
>> +	drv_data->reset_gpio = devm_gpiod_get_optional(&pd->dev, "reset", GPIOD_OUT_HIGH);
>> +	if (IS_ERR(drv_data->reset_gpio))
>> +		return PTR_ERR(drv_data->reset_gpio);
> 
> if this optional why are we returning in case of error?
> 
>> +
>>  	if (pdata) {
>>  		drv_data->freq_m = pdata->freq_m;
>>  		drv_data->freq_n = pdata->freq_n;
>> @@ -1121,6 +1126,12 @@ mv64xxx_i2c_probe(struct platform_device *pd)
>>  			goto exit_disable_pm;
>>  	}
>>  
>> +	if (drv_data->reset_gpio) {
>> +		udelay(1);
>> +		gpiod_set_value_cansleep(drv_data->reset_gpio, 0);
>> +		udelay(1);
> 
> you like busy waiting :-)
> 
> What is the reason behind these waits? Is there anything
> specified by the datasheet?
> 
> If not I would do a more relaxed sleeping like an usleep_range...
> what do you think?

Since this is apparently not intended to reset the bus driver itself,
but instead various clients connected to the bus, there is not telling
which datasheet to examine. It is simply impossible to hard-code a
correct reset pulse here, when the targets of the pulse are unspecified
and unknown.

I find the reset-gpios naming extremely misleading.

Cheers,
Peter
Chris Packham Oct. 12, 2023, 8:54 p.m. UTC | #2 
Hi Andi, Peter,

(resend as plain text, sorry to those that get duplicates)

On 12/10/23 23:49, Peter Rosin wrote:
> Hi!
>
> 2023-10-12 at 12:21, Andi Shyti wrote:
>> Hi Chris,
>>
>> ...
>>
>>>   static struct mv64xxx_i2c_regs mv64xxx_i2c_regs_mv64xxx = {
>>> @@ -1083,6 +1084,10 @@ mv64xxx_i2c_probe(struct platform_device *pd)
>>>   	if (drv_data->irq < 0)
>>>   		return drv_data->irq;
>>>   
>>> +	drv_data->reset_gpio = devm_gpiod_get_optional(&pd->dev, "reset", GPIOD_OUT_HIGH);
>>> +	if (IS_ERR(drv_data->reset_gpio))
>>> +		return PTR_ERR(drv_data->reset_gpio);
>> if this optional why are we returning in case of error?
gpiod_get_optional() will return NULL if the property is not present. 
The main error I care about here is -EPROBE_DEFER but I figure other 
errors are also relevant. This same kind of pattern is used in other 
drivers.
>>> +
>>>   	if (pdata) {
>>>   		drv_data->freq_m = pdata->freq_m;
>>>   		drv_data->freq_n = pdata->freq_n;
>>> @@ -1121,6 +1126,12 @@ mv64xxx_i2c_probe(struct platform_device *pd)
>>>   			goto exit_disable_pm;
>>>   	}
>>>   
>>> +	if (drv_data->reset_gpio) {
>>> +		udelay(1);
>>> +		gpiod_set_value_cansleep(drv_data->reset_gpio, 0);
>>> +		udelay(1);
>> you like busy waiting :-)
sure do.
>> What is the reason behind these waits? Is there anything
>> specified by the datasheet?
Those particular times were lifted from the pca954x mux but they are 
fairly arbitrary.
>> If not I would do a more relaxed sleeping like an usleep_range...
>> what do you think?
> Since this is apparently not intended to reset the bus driver itself,
> but instead various clients connected to the bus, there is not telling
> which datasheet to examine. It is simply impossible to hard-code a
> correct reset pulse here, when the targets of the pulse are unspecified
> and unknown.

I could probably follow what similar code does in the pci-mvebu.c driver 
and make the delay a property as well. As you're highlighting I can't 
possibly pick a value that's right for everyone. We really need to be 
told that the hardware design requires X us of delay after reset.

> I find the reset-gpios naming extremely misleading.

I picked that mainly because that's the name of the property for 
pci-mvebu.c and a few other end-point devices. The crux of the problem 
I'm trying to solve is that I have multiple i2c muxes that share a 
common reset GPIO in hardware. I can't associate the GPIO with multiple 
devices as the ones that are probed after the first will get -EBUSY. I 
can cheat and not have a reset-gpios property on the other muxes but 
then if the GPIO is deferred (because the controller driver hasn't been 
loaded) the muxes don't get reset at all.

> Cheers,
> Peter
Andi Shyti Oct. 13, 2023, 9:34 a.m. UTC | #3 
Hi Chris,

...

>              static struct mv64xxx_i2c_regs mv64xxx_i2c_regs_mv64xxx = {
>             @@ -1083,6 +1084,10 @@ mv64xxx_i2c_probe(struct platform_device *pd)
>                     if (drv_data->irq < 0)
>                             return drv_data->irq;
> 
>             +       drv_data->reset_gpio = devm_gpiod_get_optional(&pd->dev, "reset", GPIOD_OUT_HIGH);
>             +       if (IS_ERR(drv_data->reset_gpio))
>             +               return PTR_ERR(drv_data->reset_gpio);
> 
>         if this optional why are we returning in case of error?
> 
> gpiod_get_optional() will return NULL if the property is not present. The main
> error I care about here is -EPROBE_DEFER but I figure other errors are also
> relevant. This same kind of pattern is used in other drivers.

we already discussed about this, I don't have a strong opinion,
you can leave it as it is... I recon this is a matter of pure
taste.

Would you just mind adding an error message using
dev_err_probe()?

Thanks,
Andi
Chris Packham Oct. 15, 2023, 8:20 p.m. UTC | #4 
On 13/10/23 22:34, Andi Shyti wrote:
> Hi Chris,
>
> ...
>
>>               static struct mv64xxx_i2c_regs mv64xxx_i2c_regs_mv64xxx = {
>>              @@ -1083,6 +1084,10 @@ mv64xxx_i2c_probe(struct platform_device *pd)
>>                      if (drv_data->irq < 0)
>>                              return drv_data->irq;
>>
>>              +       drv_data->reset_gpio = devm_gpiod_get_optional(&pd->dev, "reset", GPIOD_OUT_HIGH);
>>              +       if (IS_ERR(drv_data->reset_gpio))
>>              +               return PTR_ERR(drv_data->reset_gpio);
>>
>>          if this optional why are we returning in case of error?
>>
>> gpiod_get_optional() will return NULL if the property is not present. The main
>> error I care about here is -EPROBE_DEFER but I figure other errors are also
>> relevant. This same kind of pattern is used in other drivers.
> we already discussed about this, I don't have a strong opinion,
> you can leave it as it is... I recon this is a matter of pure
> taste.

I think in this case it would actually make things uglier because I'd 
have to check for -EPROBE_DEFER. So something like

     drv_data->reset_gpio = devm_gpiod_get_optional(&pd->dev, "reset", 
GPIOD_OUT_HIGH);
     if (IS_ERR(drv_data->reset_gpio) && PTR_ERR(drv_data->reset_gpio) 
== -EPROBE_DEFER)
         return PTR_ERR(drv_data->reset_gpio);
     else
         drv_data->reset_gpio = NULL;

I could probably come up with something less ugly with a local variable 
or two but nothing as tidy as just returning on error.

>
> Would you just mind adding an error message using
> dev_err_probe()?

Yep sure. Will include in the next round.
Peter Rosin Oct. 16, 2023, 7:31 a.m. UTC | #5 
Hi!

2023-10-15 at 22:20, Chris Packham wrote:
> 
> On 13/10/23 22:34, Andi Shyti wrote:
>> Hi Chris,
>>
>> ...
>>
>>>               static struct mv64xxx_i2c_regs mv64xxx_i2c_regs_mv64xxx = {
>>>              @@ -1083,6 +1084,10 @@ mv64xxx_i2c_probe(struct platform_device *pd)
>>>                      if (drv_data->irq < 0)
>>>                              return drv_data->irq;
>>>
>>>              +       drv_data->reset_gpio = devm_gpiod_get_optional(&pd->dev, "reset", GPIOD_OUT_HIGH);
>>>              +       if (IS_ERR(drv_data->reset_gpio))
>>>              +               return PTR_ERR(drv_data->reset_gpio);
>>>
>>>          if this optional why are we returning in case of error?
>>>
>>> gpiod_get_optional() will return NULL if the property is not present. The main
>>> error I care about here is -EPROBE_DEFER but I figure other errors are also
>>> relevant. This same kind of pattern is used in other drivers.
>> we already discussed about this, I don't have a strong opinion,
>> you can leave it as it is... I recon this is a matter of pure
>> taste.
> 
> I think in this case it would actually make things uglier because I'd 
> have to check for -EPROBE_DEFER. So something like
> 
>      drv_data->reset_gpio = devm_gpiod_get_optional(&pd->dev, "reset", 
> GPIOD_OUT_HIGH);
>      if (IS_ERR(drv_data->reset_gpio) && PTR_ERR(drv_data->reset_gpio) 
> == -EPROBE_DEFER)
>          return PTR_ERR(drv_data->reset_gpio);
>      else
>          drv_data->reset_gpio = NULL;
> 
> I could probably come up with something less ugly with a local variable 
> or two but nothing as tidy as just returning on error.

I disagree with this, in my opinion it should just be:

      drv_data->reset_gpio = devm_gpiod_get_optional(&pd->dev, "reset", GPIOD_OUT_HIGH);
      if (IS_ERR(drv_data->reset_gpio))
          return dev_err_probe(&pd->dev, PTR_ERR(drv_data->reset_gpio), ...);

My take is that optional gpios (or whatever) are optional because it is
optional to specify them in the devicetree (or whereever), but if the
optional item is indeed specified, it is just like any other error if
it is not working.

$0.02

Cheers,
Peter

>>
>> Would you just mind adding an error message using
>> dev_err_probe()?
> 
> Yep sure. Will include in the next round.
>