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: <21392d77-568c-4770-ac01-cfe3f93d424c@alliedtelesis.co.nz>
Date:   Sun, 15 Oct 2023 20:20:58 +0000
From:   Chris Packham <Chris.Packham@...iedtelesis.co.nz>
To:     Andi Shyti <andi.shyti@...nel.org>
CC:     Peter Rosin <peda@...ntia.se>,
        "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


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.