I would like to stop exporting OF-specific devm_gpiod_get_from_of_node()
so that gpiolib can be cleaned a bit, so let's switch to the generic
fwnode property API.
While at it switch the rest of the calls to read properties in
bd9576_wdt_probe() to the generic device property API as well.
Signed-off-by: Dmitry Torokhov <[email protected]>
diff --git a/drivers/watchdog/bd9576_wdt.c b/drivers/watchdog/bd9576_wdt.c
index 0b6999f3b6e8..4a20e07fbb69 100644
--- a/drivers/watchdog/bd9576_wdt.c
+++ b/drivers/watchdog/bd9576_wdt.c
@@ -9,8 +9,8 @@
#include <linux/gpio/consumer.h>
#include <linux/mfd/rohm-bd957x.h>
#include <linux/module.h>
-#include <linux/of.h>
#include <linux/platform_device.h>
+#include <linux/property.h>
#include <linux/regmap.h>
#include <linux/watchdog.h>
@@ -202,10 +202,10 @@ static int bd957x_set_wdt_mode(struct bd9576_wdt_priv *priv, int hw_margin,
static int bd9576_wdt_probe(struct platform_device *pdev)
{
struct device *dev = &pdev->dev;
- struct device_node *np = dev->parent->of_node;
struct bd9576_wdt_priv *priv;
u32 hw_margin[2];
u32 hw_margin_max = BD957X_WDT_DEFAULT_MARGIN, hw_margin_min = 0;
+ int count;
int ret;
priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
@@ -221,40 +221,51 @@ static int bd9576_wdt_probe(struct platform_device *pdev)
return -ENODEV;
}
- priv->gpiod_en = devm_gpiod_get_from_of_node(dev, dev->parent->of_node,
- "rohm,watchdog-enable-gpios",
- 0, GPIOD_OUT_LOW,
- "watchdog-enable");
+ priv->gpiod_en = devm_fwnode_gpiod_get(dev, dev_fwnode(dev->parent),
+ "rohm,watchdog-enable",
+ GPIOD_OUT_LOW,
+ "watchdog-enable");
if (IS_ERR(priv->gpiod_en))
return dev_err_probe(dev, PTR_ERR(priv->gpiod_en),
"getting watchdog-enable GPIO failed\n");
- priv->gpiod_ping = devm_gpiod_get_from_of_node(dev, dev->parent->of_node,
- "rohm,watchdog-ping-gpios",
- 0, GPIOD_OUT_LOW,
- "watchdog-ping");
+ priv->gpiod_ping = devm_fwnode_gpiod_get(dev, dev_fwnode(dev->parent),
+ "rohm,watchdog-ping",
+ GPIOD_OUT_LOW,
+ "watchdog-ping");
if (IS_ERR(priv->gpiod_ping))
return dev_err_probe(dev, PTR_ERR(priv->gpiod_ping),
"getting watchdog-ping GPIO failed\n");
- ret = of_property_read_variable_u32_array(np, "rohm,hw-timeout-ms",
- &hw_margin[0], 1, 2);
- if (ret < 0 && ret != -EINVAL)
- return ret;
+ count = device_property_count_u32(dev->parent, "rohm,hw-timeout-ms");
+ if (count < 0 && count != -EINVAL)
+ return count;
+
+ if (count > 0) {
+ if (count > ARRAY_SIZE(hw_margin))
+ return -EINVAL;
- if (ret == 1)
- hw_margin_max = hw_margin[0];
+ ret = device_property_read_u32_array(dev->parent,
+ "rohm,hw-timeout-ms",
+ hw_margin, count);
+ if (ret < 0)
+ return ret;
- if (ret == 2) {
- hw_margin_max = hw_margin[1];
- hw_margin_min = hw_margin[0];
+ if (count == 1)
+ hw_margin_max = hw_margin[0];
+
+ if (count == 2) {
+ hw_margin_max = hw_margin[1];
+ hw_margin_min = hw_margin[0];
+ }
}
ret = bd957x_set_wdt_mode(priv, hw_margin_max, hw_margin_min);
if (ret)
return ret;
- priv->always_running = of_property_read_bool(np, "always-running");
+ priv->always_running = device_property_read_bool(dev->parent,
+ "always-running");
watchdog_set_drvdata(&priv->wdd, priv);
--
b4 0.10.0-dev-fc921
On Mon, Sep 5, 2022 at 9:33 AM Dmitry Torokhov
<[email protected]> wrote:
>
> I would like to stop exporting OF-specific devm_gpiod_get_from_of_node()
> so that gpiolib can be cleaned a bit, so let's switch to the generic
> fwnode property API.
>
> While at it switch the rest of the calls to read properties in
it, switch
> bd9576_wdt_probe() to the generic device property API as well.
...
> struct device *dev = &pdev->dev;
struct device *parent = dev->parent;
can make your code slightly neater.
...
> + count = device_property_count_u32(dev->parent, "rohm,hw-timeout-ms");
> + if (count < 0 && count != -EINVAL)
> + return count;
> +
> + if (count > 0) {
> + if (count > ARRAY_SIZE(hw_margin))
> + return -EINVAL;
Why double check? You may move it out of the (count > 0).
...
> - if (ret == 1)
> - hw_margin_max = hw_margin[0];
> + ret = device_property_read_u32_array(dev->parent,
> + "rohm,hw-timeout-ms",
> + hw_margin, count);
> + if (ret < 0)
> + return ret;
So, only this needs the count > 0 check since below already has it implicitly.
> - if (ret == 2) {
> - hw_margin_max = hw_margin[1];
> - hw_margin_min = hw_margin[0];
> + if (count == 1)
> + hw_margin_max = hw_margin[0];
> +
> + if (count == 2) {
> + hw_margin_max = hw_margin[1];
> + hw_margin_min = hw_margin[0];
> + }
> }
--
With Best Regards,
Andy Shevchenko
On 9/5/22 04:09, Andy Shevchenko wrote:
> On Mon, Sep 5, 2022 at 9:33 AM Dmitry Torokhov
> <[email protected]> wrote:
>>
>> I would like to stop exporting OF-specific devm_gpiod_get_from_of_node()
>> so that gpiolib can be cleaned a bit, so let's switch to the generic
>> fwnode property API.
>>
>> While at it switch the rest of the calls to read properties in
>
> it, switch
>
>> bd9576_wdt_probe() to the generic device property API as well.
>
> ...
>
>> struct device *dev = &pdev->dev;
>
> struct device *parent = dev->parent;
>
> can make your code slightly neater.
>
> ...
>
>> + count = device_property_count_u32(dev->parent, "rohm,hw-timeout-ms");
>> + if (count < 0 && count != -EINVAL)
>> + return count;
>> +
>> + if (count > 0) {
>
>> + if (count > ARRAY_SIZE(hw_margin))
>> + return -EINVAL;
>
> Why double check? You may move it out of the (count > 0).
>
Two checks will always be needed, so I don't entirely see
how that would be better.
> ...
>
>> - if (ret == 1)
>> - hw_margin_max = hw_margin[0];
>
>> + ret = device_property_read_u32_array(dev->parent,
>> + "rohm,hw-timeout-ms",
>> + hw_margin, count);
>> + if (ret < 0)
>> + return ret;
>
> So, only this needs the count > 0 check since below already has it implicitly.
>
Sorry, I don't understand this comment.
Guenter
>> - if (ret == 2) {
>> - hw_margin_max = hw_margin[1];
>> - hw_margin_min = hw_margin[0];
>> + if (count == 1)
>> + hw_margin_max = hw_margin[0];
>> +
>> + if (count == 2) {
>> + hw_margin_max = hw_margin[1];
>> + hw_margin_min = hw_margin[0];
>> + }
>> }
>
On Mon, Sep 5, 2022 at 6:13 PM Guenter Roeck <[email protected]> wrote:
> On 9/5/22 04:09, Andy Shevchenko wrote:
> > On Mon, Sep 5, 2022 at 9:33 AM Dmitry Torokhov
> > <[email protected]> wrote:
...
> >> + count = device_property_count_u32(dev->parent, "rohm,hw-timeout-ms");
> >> + if (count < 0 && count != -EINVAL)
> >> + return count;
> >> +
> >> + if (count > 0) {
> >
> >> + if (count > ARRAY_SIZE(hw_margin))
> >> + return -EINVAL;
> >
> > Why double check? You may move it out of the (count > 0).
>
> Two checks will always be needed, so I don't entirely see
> how that would be better.
But not nested. That's my point:
if (count > ARRAY_SIZE())
return ...
if (count > 0)
...
> >> - if (ret == 1)
> >> - hw_margin_max = hw_margin[0];
> >
> >> + ret = device_property_read_u32_array(dev->parent,
> >> + "rohm,hw-timeout-ms",
> >> + hw_margin, count);
> >> + if (ret < 0)
> >> + return ret;
> >
> > So, only this needs the count > 0 check since below already has it implicitly.
> >
> Sorry, I don't understand this comment.
if (count > 0) {
ret = device_property_read_u32_array(...);
...
}
if (count == 1)
...
if (count == 2)
...
But here it might be better to have the nested conditionals.
> >> - if (ret == 2) {
> >> - hw_margin_max = hw_margin[1];
> >> - hw_margin_min = hw_margin[0];
> >> + if (count == 1)
> >> + hw_margin_max = hw_margin[0];
> >> +
> >> + if (count == 2) {
> >> + hw_margin_max = hw_margin[1];
> >> + hw_margin_min = hw_margin[0];
> >> + }
> >> }
--
With Best Regards,
Andy Shevchenko
On 9/5/22 08:21, Andy Shevchenko wrote:
> On Mon, Sep 5, 2022 at 6:13 PM Guenter Roeck <[email protected]> wrote:
>> On 9/5/22 04:09, Andy Shevchenko wrote:
>>> On Mon, Sep 5, 2022 at 9:33 AM Dmitry Torokhov
>>> <[email protected]> wrote:
>
> ...
>
>>>> + count = device_property_count_u32(dev->parent, "rohm,hw-timeout-ms");
>>>> + if (count < 0 && count != -EINVAL)
>>>> + return count;
>>>> +
>>>> + if (count > 0) {
>>>
>>>> + if (count > ARRAY_SIZE(hw_margin))
>>>> + return -EINVAL;
>>>
>>> Why double check? You may move it out of the (count > 0).
>>
>> Two checks will always be needed, so I don't entirely see
>> how that would be better.
>
> But not nested. That's my point:
>
> if (count > ARRAY_SIZE())
> return ...
> if (count > 0)
> ...
>
The old code has either 1 or two checks if there is no error.
Your suggested code has always two checks. I don't see how that
is an improvement.
>>>> - if (ret == 1)
>>>> - hw_margin_max = hw_margin[0];
>>>
>>>> + ret = device_property_read_u32_array(dev->parent,
>>>> + "rohm,hw-timeout-ms",
>>>> + hw_margin, count);
>>>> + if (ret < 0)
>>>> + return ret;
>>>
>>> So, only this needs the count > 0 check since below already has it implicitly.
>>>
>> Sorry, I don't understand this comment.
>
> if (count > 0) {
> ret = device_property_read_u32_array(...);
> ...
> }
> if (count == 1)
> ...
> if (count == 2)
> ...
>
> But here it might be better to have the nested conditionals.
>
We know that count is either 1 or 2 here, so strictly speaking
if (count == 1) {
} else {
}
would be sufficient. On the other side, that depends on ARRAY_SIZE() being
exactly 2, so
if (count == 1) {
} else if (count == 2) {
}
would also make sense. Either way is fine with me. I'll leave it up
to Dmitry to decide what he wants to do.
Thanks,
Guenter
>>>> - if (ret == 2) {
>>>> - hw_margin_max = hw_margin[1];
>>>> - hw_margin_min = hw_margin[0];
>>>> + if (count == 1)
>>>> + hw_margin_max = hw_margin[0];
>>>> +
>>>> + if (count == 2) {
>>>> + hw_margin_max = hw_margin[1];
>>>> + hw_margin_min = hw_margin[0];
>>>> + }
>>>> }
>
On Mon, Sep 05, 2022 at 08:49:58AM -0700, Guenter Roeck wrote:
> On 9/5/22 08:21, Andy Shevchenko wrote:
> > On Mon, Sep 5, 2022 at 6:13 PM Guenter Roeck <[email protected]> wrote:
> > > On 9/5/22 04:09, Andy Shevchenko wrote:
> > > > On Mon, Sep 5, 2022 at 9:33 AM Dmitry Torokhov
> > > > <[email protected]> wrote:
> >
> > ...
> >
> > > > > + count = device_property_count_u32(dev->parent, "rohm,hw-timeout-ms");
> > > > > + if (count < 0 && count != -EINVAL)
> > > > > + return count;
> > > > > +
> > > > > + if (count > 0) {
> > > >
> > > > > + if (count > ARRAY_SIZE(hw_margin))
> > > > > + return -EINVAL;
> > > >
> > > > Why double check? You may move it out of the (count > 0).
> >
> > > Two checks will always be needed, so I don't entirely see
> > > how that would be better.
> >
> > But not nested. That's my point:
> >
> > if (count > ARRAY_SIZE())
> > return ...
> > if (count > 0)
> > ...
> >
>
> The old code has either 1 or two checks if there is no error.
> Your suggested code has always two checks. I don't see how that
> is an improvement.
>
> > > > > - if (ret == 1)
> > > > > - hw_margin_max = hw_margin[0];
> > > >
> > > > > + ret = device_property_read_u32_array(dev->parent,
> > > > > + "rohm,hw-timeout-ms",
> > > > > + hw_margin, count);
> > > > > + if (ret < 0)
> > > > > + return ret;
> > > >
> > > > So, only this needs the count > 0 check since below already has it implicitly.
> > > >
> > > Sorry, I don't understand this comment.
> >
> > if (count > 0) {
> > ret = device_property_read_u32_array(...);
> > ...
> > }
> > if (count == 1)
> > ...
> > if (count == 2)
> > ...
> >
> > But here it might be better to have the nested conditionals.
> >
>
> We know that count is either 1 or 2 here, so strictly speaking
> if (count == 1) {
> } else {
> }
> would be sufficient. On the other side, that depends on ARRAY_SIZE() being
> exactly 2, so
> if (count == 1) {
> } else if (count == 2) {
> }
> would also make sense. Either way is fine with me. I'll leave it up
> to Dmitry to decide what he wants to do.
My goal is to drop usage of devm_gpiod_get_from_of_node(), beyond that I
do not have strong preferences either way really. It is probing code, so
performance is not critical, but I'm obviously satisfied with how the
code looks now, or I would not have sent it.
Thanks.
--
Dmitry
On 9/5/22 12:47, Dmitry Torokhov wrote:
[ ... ]
>> We know that count is either 1 or 2 here, so strictly speaking
>> if (count == 1) {
>> } else {
>> }
>> would be sufficient. On the other side, that depends on ARRAY_SIZE() being
>> exactly 2, so
>> if (count == 1) {
>> } else if (count == 2) {
>> }
>> would also make sense. Either way is fine with me. I'll leave it up
>> to Dmitry to decide what he wants to do.
>
> My goal is to drop usage of devm_gpiod_get_from_of_node(), beyond that I
> do not have strong preferences either way really. It is probing code, so
> performance is not critical, but I'm obviously satisfied with how the
> code looks now, or I would not have sent it.
>
Good point.
Reviewed-by: Guenter Roeck <[email protected]>
On Mon, Sep 05, 2022 at 03:09:05PM -0700, Guenter Roeck wrote:
> On 9/5/22 12:47, Dmitry Torokhov wrote:
> [ ... ]
> > > We know that count is either 1 or 2 here, so strictly speaking
> > > if (count == 1) {
> > > } else {
> > > }
> > > would be sufficient. On the other side, that depends on ARRAY_SIZE() being
> > > exactly 2, so
> > > if (count == 1) {
> > > } else if (count == 2) {
> > > }
> > > would also make sense. Either way is fine with me. I'll leave it up
> > > to Dmitry to decide what he wants to do.
> >
> > My goal is to drop usage of devm_gpiod_get_from_of_node(), beyond that I
> > do not have strong preferences either way really. It is probing code, so
> > performance is not critical, but I'm obviously satisfied with how the
> > code looks now, or I would not have sent it.
> >
>
> Good point.
>
> Reviewed-by: Guenter Roeck <[email protected]>
Guenter, individual patches are going through maintainer's trees, will
you take this one?
Thanks.
--
Dmitry
On Mon, Sep 5, 2022 at 8:31 AM Dmitry Torokhov
<[email protected]> wrote:
> I would like to stop exporting OF-specific devm_gpiod_get_from_of_node()
> so that gpiolib can be cleaned a bit, so let's switch to the generic
> fwnode property API.
>
> While at it switch the rest of the calls to read properties in
> bd9576_wdt_probe() to the generic device property API as well.
>
> Signed-off-by: Dmitry Torokhov <[email protected]>
Reviewed-by: Linus Walleij <[email protected]>
Yours,
Linus Walleij