[2/2] i2c: mv64xxx: add an optional reset-gpios property

  Some hardware designs have a GPIO used to control the reset of all the
devices on and I2C bus. It's not possible for every child node to
declare a reset-gpios property as only the first device probed would be
able to successfully request it (the others will get -EBUSY). Represent
this kind of hardware design by associating the reset-gpios with the
parent I2C bus. The reset line will be released prior to the child I2C
devices being probed.

Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
---
 drivers/i2c/busses/i2c-mv64xxx.c | 11 +++++++++++
 1 file changed, 11 insertions(+)
  

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?

Andi
  

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
  

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
  

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
  

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.
  

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

Hi Chris,

kernel test robot noticed the following build errors:

[auto build test ERROR on wsa/i2c/for-next]
[also build test ERROR on linus/master v6.6-rc6 next-20231020]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Chris-Packham/dt-bindings-i2c-mv64xxx-add-reset-gpios-property/20231017-102540
base:   https://git.kernel.org/pub/scm/linux/kernel/git/wsa/linux.git i2c/for-next
patch link:    https://lore.kernel.org/r/20231012035838.2804064-3-chris.packham%40alliedtelesis.co.nz
patch subject: [PATCH 2/2] i2c: mv64xxx: add an optional reset-gpios property
config: i386-buildonly-randconfig-001-20231021 (https://download.01.org/0day-ci/archive/20231021/202310211508.57kpcEto-lkp@intel.com/config)
compiler: gcc-12 (Debian 12.2.0-14) 12.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231021/202310211508.57kpcEto-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202310211508.57kpcEto-lkp@intel.com/

All errors (new ones prefixed by >>):

   drivers/i2c/busses/i2c-mv64xxx.c: In function 'mv64xxx_i2c_probe':
>> drivers/i2c/busses/i2c-mv64xxx.c:1028:32: error: implicit declaration of function 'devm_gpiod_get_optional'; did you mean 'devm_clk_get_optional'? [-Werror=implicit-function-declaration]
    1028 |         drv_data->reset_gpio = devm_gpiod_get_optional(&pd->dev, "reset", GPIOD_OUT_HIGH);
         |                                ^~~~~~~~~~~~~~~~~~~~~~~
         |                                devm_clk_get_optional
>> drivers/i2c/busses/i2c-mv64xxx.c:1028:75: error: 'GPIOD_OUT_HIGH' undeclared (first use in this function)
    1028 |         drv_data->reset_gpio = devm_gpiod_get_optional(&pd->dev, "reset", GPIOD_OUT_HIGH);
         |                                                                           ^~~~~~~~~~~~~~
   drivers/i2c/busses/i2c-mv64xxx.c:1028:75: note: each undeclared identifier is reported only once for each function it appears in
>> drivers/i2c/busses/i2c-mv64xxx.c:1068:17: error: implicit declaration of function 'gpiod_set_value_cansleep' [-Werror=implicit-function-declaration]
    1068 |                 gpiod_set_value_cansleep(drv_data->reset_gpio, 0);
         |                 ^~~~~~~~~~~~~~~~~~~~~~~~
   cc1: some warnings being treated as errors


vim +1028 drivers/i2c/busses/i2c-mv64xxx.c

   983	
   984	static int
   985	mv64xxx_i2c_probe(struct platform_device *pd)
   986	{
   987		struct mv64xxx_i2c_data		*drv_data;
   988		struct mv64xxx_i2c_pdata	*pdata = dev_get_platdata(&pd->dev);
   989		int	rc;
   990	
   991		if ((!pdata && !pd->dev.of_node))
   992			return -ENODEV;
   993	
   994		drv_data = devm_kzalloc(&pd->dev, sizeof(struct mv64xxx_i2c_data),
   995					GFP_KERNEL);
   996		if (!drv_data)
   997			return -ENOMEM;
   998	
   999		drv_data->reg_base = devm_platform_ioremap_resource(pd, 0);
  1000		if (IS_ERR(drv_data->reg_base))
  1001			return PTR_ERR(drv_data->reg_base);
  1002	
  1003		strscpy(drv_data->adapter.name, MV64XXX_I2C_CTLR_NAME " adapter",
  1004			sizeof(drv_data->adapter.name));
  1005	
  1006		init_waitqueue_head(&drv_data->waitq);
  1007		spin_lock_init(&drv_data->lock);
  1008	
  1009		/* Not all platforms have clocks */
  1010		drv_data->clk = devm_clk_get(&pd->dev, NULL);
  1011		if (IS_ERR(drv_data->clk)) {
  1012			if (PTR_ERR(drv_data->clk) == -EPROBE_DEFER)
  1013				return -EPROBE_DEFER;
  1014			drv_data->clk = NULL;
  1015		}
  1016	
  1017		drv_data->reg_clk = devm_clk_get(&pd->dev, "reg");
  1018		if (IS_ERR(drv_data->reg_clk)) {
  1019			if (PTR_ERR(drv_data->reg_clk) == -EPROBE_DEFER)
  1020				return -EPROBE_DEFER;
  1021			drv_data->reg_clk = NULL;
  1022		}
  1023	
  1024		drv_data->irq = platform_get_irq(pd, 0);
  1025		if (drv_data->irq < 0)
  1026			return drv_data->irq;
  1027	
> 1028		drv_data->reset_gpio = devm_gpiod_get_optional(&pd->dev, "reset", GPIOD_OUT_HIGH);
  1029		if (IS_ERR(drv_data->reset_gpio))
  1030			return PTR_ERR(drv_data->reset_gpio);
  1031	
  1032		if (pdata) {
  1033			drv_data->freq_m = pdata->freq_m;
  1034			drv_data->freq_n = pdata->freq_n;
  1035			drv_data->adapter.timeout = msecs_to_jiffies(pdata->timeout);
  1036			drv_data->offload_enabled = false;
  1037			memcpy(&drv_data->reg_offsets, &mv64xxx_i2c_regs_mv64xxx, sizeof(drv_data->reg_offsets));
  1038		} else if (pd->dev.of_node) {
  1039			rc = mv64xxx_of_config(drv_data, &pd->dev);
  1040			if (rc)
  1041				return rc;
  1042		}
  1043	
  1044		rc = mv64xxx_i2c_init_recovery_info(drv_data, &pd->dev);
  1045		if (rc == -EPROBE_DEFER)
  1046			return rc;
  1047	
  1048		drv_data->adapter.dev.parent = &pd->dev;
  1049		drv_data->adapter.algo = &mv64xxx_i2c_algo;
  1050		drv_data->adapter.owner = THIS_MODULE;
  1051		drv_data->adapter.class = I2C_CLASS_DEPRECATED;
  1052		drv_data->adapter.nr = pd->id;
  1053		drv_data->adapter.dev.of_node = pd->dev.of_node;
  1054		platform_set_drvdata(pd, drv_data);
  1055		i2c_set_adapdata(&drv_data->adapter, drv_data);
  1056	
  1057		pm_runtime_set_autosuspend_delay(&pd->dev, MSEC_PER_SEC);
  1058		pm_runtime_use_autosuspend(&pd->dev);
  1059		pm_runtime_enable(&pd->dev);
  1060		if (!pm_runtime_enabled(&pd->dev)) {
  1061			rc = mv64xxx_i2c_runtime_resume(&pd->dev);
  1062			if (rc)
  1063				goto exit_disable_pm;
  1064		}
  1065	
  1066		if (drv_data->reset_gpio) {
  1067			udelay(1);
> 1068			gpiod_set_value_cansleep(drv_data->reset_gpio, 0);
  1069			udelay(1);
  1070		}
  1071	
  1072		rc = request_irq(drv_data->irq, mv64xxx_i2c_intr, 0,
  1073				 MV64XXX_I2C_CTLR_NAME, drv_data);
  1074		if (rc) {
  1075			dev_err(&drv_data->adapter.dev,
  1076				"mv64xxx: Can't register intr handler irq%d: %d\n",
  1077				drv_data->irq, rc);
  1078			goto exit_disable_pm;
  1079		} else if ((rc = i2c_add_numbered_adapter(&drv_data->adapter)) != 0) {
  1080			dev_err(&drv_data->adapter.dev,
  1081				"mv64xxx: Can't add i2c adapter, rc: %d\n", -rc);
  1082			goto exit_free_irq;
  1083		}
  1084	
  1085		return 0;
  1086	
  1087	exit_free_irq:
  1088		free_irq(drv_data->irq, drv_data);
  1089	exit_disable_pm:
  1090		pm_runtime_disable(&pd->dev);
  1091		if (!pm_runtime_status_suspended(&pd->dev))
  1092			mv64xxx_i2c_runtime_suspend(&pd->dev);
  1093	
  1094		return rc;
  1095	}
  1096