[<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.
Powered by blists - more mailing lists