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: <63403365-2d23-b4a0-d869-070686d62ab5@axentia.se>
Date:   Thu, 12 Oct 2023 12:49:07 +0200
From:   Peter Rosin <peda@...ntia.se>
To:     Andi Shyti <andi.shyti@...nel.org>,
        Chris Packham <chris.packham@...iedtelesis.co.nz>
Cc:     gregory.clement@...tlin.com, robh+dt@...nel.org,
        krzysztof.kozlowski+dt@...aro.org, conor+dt@...nel.org,
        linux-i2c@...r.kernel.org, devicetree@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/2] i2c: mv64xxx: add an optional reset-gpios property

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