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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <598cc050-c19d-4fb1-9875-f87e13c3c47d@alliedtelesis.co.nz>
Date:   Thu, 12 Oct 2023 20:54:57 +0000
From:   Chris Packham <Chris.Packham@...iedtelesis.co.nz>
To:     Peter Rosin <peda@...ntia.se>, Andi Shyti <andi.shyti@...nel.org>
CC:     "gregory.clement@...tlin.com" <gregory.clement@...tlin.com>,
        "robh+dt@...nel.org" <robh+dt@...nel.org>,
        "krzysztof.kozlowski+dt@...aro.org" 
        <krzysztof.kozlowski+dt@...aro.org>,
        "conor+dt@...nel.org" <conor+dt@...nel.org>,
        "linux-i2c@...r.kernel.org" <linux-i2c@...r.kernel.org>,
        "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 2/2] i2c: mv64xxx: add an optional reset-gpios property

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