Re: [PATCH v1 10/11] watchdog: bd9576_wdt: switch to using devm_fwnode_gpiod_get()

On Mon, Sep 05, 2022 at 08:49:58AM -0700, Guenter Roeck wrote:

On 9/5/22 08:21, Andy Shevchenko wrote:

quoted

On Mon, Sep 5, 2022 at 6:13 PM Guenter Roeck [off-list ref] wrote:

quoted

On 9/5/22 04:09, Andy Shevchenko wrote:

quoted

On Mon, Sep 5, 2022 at 9:33 AM Dmitry Torokhov
[off-list ref] wrote:

...

quoted

quoted

quoted

+       count = device_property_count_u32(dev->parent, "rohm,hw-timeout-ms");
+       if (count < 0 && count != -EINVAL)
+               return count;
+
+       if (count > 0) {

quoted

+               if (count > ARRAY_SIZE(hw_margin))
+                       return -EINVAL;

Why double check? You may move it out of the (count > 0).

quoted

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.

quoted

quoted

quoted

quoted

-       if (ret == 1)
-               hw_margin_max = hw_margin[0];

quoted

+               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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel