Re: [PATCH v4 09/16] gpio: support ROHM BD71815 GPOs

On Fri, Mar 26, 2021 at 3:33 PM Matti Vaittinen
[off-list ref] wrote:

On Fri, 2021-03-26 at 13:26 +0200, Andy Shevchenko wrote:

quoted

On Wed, Mar 24, 2021 at 12:20 PM Matti Vaittinen
[off-list ref] wrote:

...

quoted

quoted

+       return (val >> offset) & 1;

!!(val & BIT(offset)) can also work and be in alignment with the
below code.

This is an opinion, but to me !!(val & BIT(offset)) looks more
confusing. I don't see the benefit from the change.

I always try to find a compromise between two: your own style and
common practice used in the subsystem in question. AFAIR my proposal
is (recommended?) style for new code.

...

quoted

quoted

+       if (!bd71815->e5_pin_is_gpo && offset)
+               return;

I wonder if you can use valid_mask instead of this.

Do you mean re-naming the e5_pin_is_gpo to valid_mask? Or do you mean
some GPIO framework internal feature allowing to define valid pins? (If
my memory serves me right one can set invalid pins from DT - but by
default all pins are valid and here we want to invalidate a pin by
default). For renaming I don't see the value - if internal feature can
be used then there may be value. Thanks for the pointer, I'll look what
I find.

I mean to utilize internal valid_mask bitmap. Yes, you may fill it as
valid at the start of the driver and then simply call __clear_bit() /
clear_bit() against one you wanted to disable. Then core will take
care of the rest (AFAIR).

...

quoted

quoted

+       bit = BIT(offset);

Can be moved to the definition block.

I don't like doing the assignment before we check if the operation is
valid. And, making assignments which are not plain constants in
declaration make reading the declaration much harder.

OK.

...

quoted

quoted

+       default:
+               break;
+       }
+       return -EOPNOTSUPP;

You may return directly from default.

I think there used to be compilers which didn't like having the return
inside a block - even if the block was a default. I also prefer seeing
return at the end of function which should return a value.

I prefer less LOCs in the file when it makes sense. And here you seem
appealing to compilers from last century.

...

quoted

quoted

+       int ret;
+       struct bd71815_gpio *g;
+       struct device *dev;
+       struct device *parent;

Reversed xmas tree order.

What is the added value here? I understand alphabetical sorting - it
helps looking if particular entry is included. I also understand type-
based sorting. But reverse Xmas tree? I thin I have heard it eases
reading declarations - which is questionable in this case. Double so
when you also suggest moving assignments to declaration block which
makes it _much_ harder to read? I won't change this unless it is
mandated by the maintainers.

Compare to:

       struct bd71815_gpio *g;
       struct device *parent;
       struct device *dev;
       int ret;

It's easier to read, esp. taking into account that ret is going last.
It seems to me more natural, so we have a disagreement here, but I'm
not a maintainer, it's up to them.

...

quoted

quoted

+       parent = dev->parent;

It is not always obvious (especially for someone not reading MFD driver
code frequently) why we use parent device for some things and the
device being probed to some other stuff. Typically this is not needed
if the device is not MFD sub-device. And again, the comments in the
middle of declaration block look confusing to me. I think removing
comments and moving these to declaration make readability _much_ worse.

I disagree with you here again. To me it's like completely unneeded churn.

...

quoted

quoted

+       g->e5_pin_is_gpo = of_property_read_bool(parent->of_node,
+                                                "rohm,enable-
hidden-gpo");

You may use device_property_read_bool().

Out of the curiosity - is there any other reason but ACPI?

We might have another property provider (by the fact we already have
the third one, but it's a specific one, called software nodes).

 ACPI support
can be added later if needed.

Yes, but doing something OF centric which might have been used on
non-OF platforms is to do double effort and waste time and resources.

I still think you're correct. This is
definitely one of the points that fall in the category of things "I
must consider as a good practice for (my) new contribution". So I try
to keep this in mind in the future.

...

quoted

quoted

+       g->chip.of_node = parent->of_node;

Redundant. GPIO library does it for you and even better.

So I can nowadays just omit this? Thanks!

For a long time. I haven't checked the date when it started like this,
but couple of years sounds like a good approximation.

...

quoted

quoted

+MODULE_DEVICE_TABLE(platform, bd7181x_gpo_id);

Why do you need this ID table exactly?
You have the same name as in the platform driver structure below.

This driver was also supporting another IC (BD71817) - but as far as I
know the BD71817 is no longer used too much so I dropped it. The ID
table was left with this one entry only. I will see if this is any more
needed. Thanks.

Yes, but in that case you have to have driver data to differentiate
the chips, right? Otherwise for platform drivers this makes a little
sense b/c it effectively repeats .name from gpo_bd71815_driver.

-- 
With Best Regards,
Andy Shevchenko