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

[Peter Rosin]

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