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

[Chris Packham]

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