mbox series

[v4,00/16] Support ROHM BD71815 PMIC

Message ID cover.1616566395.git.matti.vaittinen@fi.rohmeurope.com
Headers show
Series Support ROHM BD71815 PMIC | expand

Message

Matti Vaittinen March 24, 2021, 7:21 a.m. UTC
Patch series introducing support for ROHM BD71815 PMIC

ROHM BD71815 is a power management IC used in some battery powered
systems. It contains regulators, GPO(s), charger + coulomb counter, RTC
and a clock gate.

All regulators can be controlled via I2C. LDO4 can additionally be set to
be enabled/disabled by a GPIO. LDO3 voltage could be selected from two
voltages written into separate VSEL reisters using GPIO but this mode is
not supported by driver. On top of that the PMIC has the typical HW
state machine which is present also on many other ROHM PMICs.

IC contains two GPOs - but one of the GPOs is marked as GND in
data-sheet. Thus the driver by default only exposes one GPO. The second
GPO can be enabled by special DT property.

RTC is almost similar to what is on BD71828. For currently used features
only the register address offset to RTC block differs.

The charger driver is not included in this series. ROHM has a charger
driver with some fuel-gauging logig written in but this is not included
here. I am working on separating the logic from HW specific driver and
supporting both BD71815 and BD71828 chargers in separate patch series.

Changelog v4:
  - Sorted ROHM chip ID enum
  - Statcized DVS structures in regulator driver
  - Minor styling for regulator driver
  - rebased on v5.12-rc4
Changelog v3:
  - GPIO clean-up as suggested by Bartosz
  - MFD clean-up as suggested by Lee
  - clk-mode dt-binding handling in MFD driver corrected to reflect new
    property values.
  - Dropped already applied patches
  - Rebased on v5.12-rc2
Changelog v2:
  - Rebased on top of v5.11-rc3
  - Added another "preliminary patch" which fixes HW-dvs voltage
    handling (patch 1)
  - split regulator patch to two.
  - changed dt-binding patch ordering.
  regulators:
    - staticized probe
    - removed some unnecessary defines
    - updated comments
    - split rohm-regulator patch adding SNVS and supporting simple
      linear mapping into two - one adding support for mapping, other
      adding SNVS.
  GPIO:
    - removed unnecessary headers
    - clarified dev/parent->dev usage
    - removed forgotten #define DEBUG
  dt-bindings:
    - changed patch order to meet ref-dependencies
    - added missing regulator nodes
    - changed string property for clk mode to tristated
  MFD:
    - header cleanups.
  CLK:
    - fixed commit message

--

Matti Vaittinen (16):
  rtc: bd70528: Do not require parent data
  mfd: bd718x7: simplify by cleaning unnecessary device data
  dt_bindings: bd71828: Add clock output mode
  dt_bindings: regulator: Add ROHM BD71815 PMIC regulators
  dt_bindings: mfd: Add ROHM BD71815 PMIC
  mfd: Add ROHM BD71815 ID
  mfd: Sort ROHM chip ID list for better readability
  mfd: Support for ROHM BD71815 PMIC core
  gpio: support ROHM BD71815 GPOs
  regulator: helpers: Export helper voltage listing
  regulator: rohm-regulator: linear voltage support
  regulator: rohm-regulator: Support SNVS HW state.
  regulator: Support ROHM BD71815 regulators
  clk: bd718x7: Add support for clk gate on ROHM BD71815 PMIC
  rtc: bd70528: Support RTC on ROHM BD71815
  MAINTAINERS: Add ROHM BD71815AGW

 .../bindings/mfd/rohm,bd71815-pmic.yaml       | 201 ++++++
 .../bindings/mfd/rohm,bd71828-pmic.yaml       |   6 +
 .../regulator/rohm,bd71815-regulator.yaml     | 116 +++
 MAINTAINERS                                   |   3 +
 drivers/clk/clk-bd718x7.c                     |   9 +-
 drivers/gpio/Kconfig                          |  10 +
 drivers/gpio/Makefile                         |   1 +
 drivers/gpio/gpio-bd71815.c                   | 176 +++++
 drivers/mfd/Kconfig                           |  15 +-
 drivers/mfd/rohm-bd71828.c                    | 486 +++++++++----
 drivers/mfd/rohm-bd718x7.c                    |  43 +-
 drivers/regulator/Kconfig                     |  11 +
 drivers/regulator/Makefile                    |   1 +
 drivers/regulator/bd71815-regulator.c         | 676 ++++++++++++++++++
 drivers/regulator/helpers.c                   |  36 +-
 drivers/regulator/rohm-regulator.c            |  23 +-
 drivers/rtc/Kconfig                           |   6 +-
 drivers/rtc/rtc-bd70528.c                     | 104 +--
 include/linux/mfd/rohm-bd71815.h              | 562 +++++++++++++++
 include/linux/mfd/rohm-bd71828.h              |   3 +
 include/linux/mfd/rohm-bd718x7.h              |  13 -
 include/linux/mfd/rohm-generic.h              |  15 +-
 include/linux/regulator/driver.h              |   2 +
 23 files changed, 2286 insertions(+), 232 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/mfd/rohm,bd71815-pmic.yaml
 create mode 100644 Documentation/devicetree/bindings/regulator/rohm,bd71815-regulator.yaml
 create mode 100644 drivers/gpio/gpio-bd71815.c
 create mode 100644 drivers/regulator/bd71815-regulator.c
 create mode 100644 include/linux/mfd/rohm-bd71815.h


base-commit: 0d02ec6b3136c73c09e7859f0d0e4e2c4c07b49b

Comments

Linus Walleij March 25, 2021, 9:35 a.m. UTC | #1
On Wed, Mar 24, 2021 at 8:29 AM Matti Vaittinen
<matti.vaittinen@fi.rohmeurope.com> wrote:

> Support GPO(s) found from ROHM BD71815 power management IC. The IC has two
> GPO pins but only one is properly documented in data-sheet. The driver
> exposes by default only the documented GPO. The second GPO is connected to
> E5 pin and is marked as GND in data-sheet. Control for this undocumented
> pin can be enabled using a special DT property.
>
> This driver is derived from work by Peter Yang <yanglsh@embest-tech.com>
> although not so much of original is left.
>
> Signed-off-by: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>
> ---
> Changes since v3:
>   - No changes

This looks OK to me:
Acked-by: Linus Walleij <linus.walleij@linaro.org>

It could potentially (like the other Rohm GPIO MFD PMIC drivers)
make some use of the gpio regmap library, but we have some
pending changes for that so look into it after the next merge
window.

I.e. for your TODO: look at the GPIO_REGMAP helper.

Yours,
Linus Walleij
Matti Vaittinen March 25, 2021, 10:32 a.m. UTC | #2
Hello Linus,

On Thu, 2021-03-25 at 10:35 +0100, Linus Walleij wrote:
> On Wed, Mar 24, 2021 at 8:29 AM Matti Vaittinen
> <matti.vaittinen@fi.rohmeurope.com> wrote:
> 
> > Support GPO(s) found from ROHM BD71815 power management IC. The IC
> > has two
> > GPO pins but only one is properly documented in data-sheet. The
> > driver
> > exposes by default only the documented GPO. The second GPO is
> > connected to
> > E5 pin and is marked as GND in data-sheet. Control for this
> > undocumented
> > pin can be enabled using a special DT property.
> > 
> > This driver is derived from work by Peter Yang <
> > yanglsh@embest-tech.com>
> > although not so much of original is left.
> > 
> > Signed-off-by: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>
> > ---
> > Changes since v3:
> >   - No changes
> 
> This looks OK to me:
> Acked-by: Linus Walleij <linus.walleij@linaro.org>
> 
> It could potentially (like the other Rohm GPIO MFD PMIC drivers)
> make some use of the gpio regmap library, but we have some
> pending changes for that so look into it after the next merge
> window.
> 
> I.e. for your TODO: look at the GPIO_REGMAP helper.

I just took a quick peek at gpio_regmap and it looks pretty good to me!

Any particular reason why gpio_regmap is not just part of gpio_chip? I
guess providing the 'gpio_regmap_direction_*()', 'gpio_regmap_get()',
'gpio_regmap_set()' as exported helpers and leaving calling the
(devm_)gpiochip_add_data() to IC driver would have allowed more
flexibility. Drivers could then use the gpio_regamap features which fit
the IC (by providing pointers to helper functions in gpio_chip) - and
handle potential oddball-features by using pointers to some customized
functions in gpio_chip.

Anyways, definitely worth getting familiar with! Thanks for the pointer
:]

Best Regards,
	Matti Vaittinen
Andy Shevchenko March 26, 2021, 11:26 a.m. UTC | #3
On Wed, Mar 24, 2021 at 12:20 PM Matti Vaittinen
<matti.vaittinen@fi.rohmeurope.com> wrote:
>
> Support GPO(s) found from ROHM BD71815 power management IC. The IC has two
> GPO pins but only one is properly documented in data-sheet. The driver

in the datasheet

> exposes by default only the documented GPO. The second GPO is connected to
> E5 pin and is marked as GND in data-sheet. Control for this undocumented

in the datasheet

> pin can be enabled using a special DT property.
>
> This driver is derived from work by Peter Yang <yanglsh@embest-tech.com>
> although not so much of original is left.

of the original

Below my comments independently on the fact if this driver will be
completely rewritten, consider them as a good practice for your new
contribution.

...

> +/*
> + * Support to GPOs on ROHM BD71815
> + */

This is effectively one line.

...

> +/* For the BD71815 register definitions */
> +#include <linux/mfd/rohm-bd71815.h>

Since it's component specific header(s) I would move it to a separate
group and locate...

> +#include <linux/module.h>

> +#include <linux/of.h>

You may do better than be OF-centric. See below.

> +#include <linux/platform_device.h>
> +

...somewhere here.

...

> +       /*
> +        * Sigh. The BD71815 and BD71817 were originally designed to support two
> +        * GPO pins. At some point it was noticed the second GPO pin which is
> +        * the E5 pin located at the center of IC is hard to use on PCB (due to
> +        * the location). It was decided to not promote this second GPO and pin
> +        * is marked as GND on the data-sheet. The functionality is still there
> +        * though! I guess driving GPO connected to ground is a bad idea. Thus

a GPO
to the ground

> +        * we do not support it by default. OTOH - the original driver written
> +        * by colleagues at Embest did support controlling this second GPO. It
> +        * is thus possible this is used in some of the products.
> +        *
> +        * This driver does not by default support configuring this second GPO
> +        * but allows using it by providing the DT property
> +        * "rohm,enable-hidden-gpo".
> +        */

...

> +       int ret = 0;

Redundant assignment.

> +       int val;
> +
> +       ret = regmap_read(bd71815->regmap, BD71815_REG_GPO, &val);
> +       if (ret)
> +               return ret;

> +       return (val >> offset) & 1;

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

...

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

I wonder if you can use valid_mask instead of this.

...

> +       bit = BIT(offset);

Can be moved to the definition block.

...

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

As above.

...

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

You may return directly from default.

...

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

Reversed xmas tree order.

...

> +       /*
> +        * Bind devm lifetime to this platform device => use dev for devm.
> +        * also the prints should originate from this device.
> +        */

Why is this comment needed?

...

> +       dev = &pdev->dev;

Can be done in the definition block.

...

> +       /* The device-tree and regmap come from MFD => use parent for that */

Why do you need this comment?

> +       parent = dev->parent;

Ditto, can be moved to the definition block.

...

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

You may use device_property_read_bool().

...

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

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

...

> +       ret = devm_gpiochip_add_data(dev, &g->chip, g);
> +       if (ret < 0) {
> +               dev_err(dev, "could not register gpiochip, %d\n", ret);
> +               return ret;
> +       }
> +
> +       return ret;

It's as simply as
return devm_gpiochip_add_data(...);

...

> +static const struct platform_device_id bd7181x_gpo_id[] = {
> +       { "bd71815-gpo" },

> +       { },

No comma for the terminator line.

> +};
> +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.

> +static struct platform_driver gpo_bd71815_driver = {
> +       .driver = {
> +               .name   = "bd71815-gpo",

> +               .owner  = THIS_MODULE,

This is done by module_*_driver() macros, drop it.

> +       },
> +       .probe          = gpo_bd71815_probe,
> +       .id_table       = bd7181x_gpo_id,
> +};

> +

Extra blank line.

> +module_platform_driver(gpo_bd71815_driver);

> +/* Note:  this hardware lives inside an I2C-based multi-function device. */
> +MODULE_ALIAS("platform:bd71815-gpo");

> +

Ditto.

> +MODULE_AUTHOR("Peter Yang <yanglsh@embest-tech.com>");

And I don't see a match with a committer/submitter/co-developer/etc.
Please, make corresponding fields and this macro (or macros, you may
have as many MODULE_AUTHOR() entries as developers of the code)
aligned to each other.

> +MODULE_DESCRIPTION("GPO interface for BD71815");
> +MODULE_LICENSE("GPL");
Andy Shevchenko March 26, 2021, 11:27 a.m. UTC | #4
On Fri, Mar 26, 2021 at 1:26 PM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
> On Wed, Mar 24, 2021 at 12:20 PM Matti Vaittinen
> <matti.vaittinen@fi.rohmeurope.com> wrote:


> > +#include <linux/of.h>
>
> You may do better than be OF-centric. See below.

Ah, yep, when you switch to unified device property API, you would
need property.h and mod_devicetable.h instead of this.
Matti Vaittinen March 26, 2021, 1:33 p.m. UTC | #5
Hi Andy,

On Fri, 2021-03-26 at 13:26 +0200, Andy Shevchenko wrote:
> On Wed, Mar 24, 2021 at 12:20 PM Matti Vaittinen
> <matti.vaittinen@fi.rohmeurope.com> wrote:
> > Support GPO(s) found from ROHM BD71815 power management IC. The IC
> > has two
> > GPO pins but only one is properly documented in data-sheet. The
> > driver
> 
> in the datasheet
> 
> > exposes by default only the documented GPO. The second GPO is
> > connected to
> > E5 pin and is marked as GND in data-sheet. Control for this
> > undocumented
> 
> in the datasheet
> 
> > pin can be enabled using a special DT property.
> > 
> > This driver is derived from work by Peter Yang <
> > yanglsh@embest-tech.com>
> > although not so much of original is left.
> 
> of the original
> 
> Below my comments independently on the fact if this driver will be
> completely rewritten, consider them as a good practice for your new
> contribution.

Thank you for the review. I appreciate your help although I don't
always agree necessity regarding all of the styling suggestions :) I
did not respond here to the suggestions I agreed with.

As Linus pointed out, few of the ROHM drivers should be revised for
regmap_gpio usage in near future. I will definitely go through all the
comments at that point if there is no reason to respin series earlier.


> +       int val;
> > +
> > +       ret = regmap_read(bd71815->regmap, BD71815_REG_GPO, &val);
> > +       if (ret)
> > +               return ret;
> > +       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.

> 
> ...
> 
> > +       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.

> 
> ...
> 
> > +       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.

> ...
> 
> > +       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.

> 
> ...
> 
> > +       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.

> 
> ...
> 
> > +       /*
> > +        * Bind devm lifetime to this platform device => use dev
> > for devm.
> > +        * also the prints should originate from this device.
> > +        */
> 
> Why is this comment needed?
> 
> 
> > +       /* The device-tree and regmap come from MFD => use parent
> > for that */
> 
> Why do you need this comment?
> 
> > +       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.

> ...
> 
> > +       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? ACPI support
can be added later if needed. 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.

> > +       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!

> > +       ret = devm_gpiochip_add_data(dev, &g->chip, g);
> > +       if (ret < 0) {
> > +               dev_err(dev, "could not register gpiochip, %d\n",
> > ret);
> > +               return ret;
> > +       }
> > +
> > +       return ret;
> 
> It's as simply as
> return devm_gpiochip_add_data(...);

Hm. I think you're right. The print does not add much value here.
Thanks.

> 
> ...
> 
> > +static const struct platform_device_id bd7181x_gpo_id[] = {
> > +       { "bd71815-gpo" },
> > +       { },
> 
> No comma for the terminator line.
> 
> > +};
> > +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.

> 
> > +static struct platform_driver gpo_bd71815_driver = {
> > +       .driver = {
> > +               .name   = "bd71815-gpo",
> > +               .owner  = THIS_MODULE,
> 
> This is done by module_*_driver() macros, drop it.
> 
> > +       },
> > +       .probe          = gpo_bd71815_probe,
> > +       .id_table       = bd7181x_gpo_id,
> > +};
> > +
> 
> > +MODULE_AUTHOR("Peter Yang <yanglsh@embest-tech.com>");
> 
> And I don't see a match with a committer/submitter/co-developer/etc.
> Please, make corresponding fields and this macro (or macros, you may
> have as many MODULE_AUTHOR() entries as developers of the code)
> aligned to each other.

I never knew there could be many MODULE_AUTHOR() entries. Thanks for
pointing it out.

> 
> > +MODULE_DESCRIPTION("GPO interface for BD71815");
> > +MODULE_LICENSE("GPL");

Best Regards
	--Matti
Andy Shevchenko March 26, 2021, 5:51 p.m. UTC | #6
On Fri, Mar 26, 2021 at 3:33 PM Matti Vaittinen
<matti.vaittinen@fi.rohmeurope.com> wrote:
> On Fri, 2021-03-26 at 13:26 +0200, Andy Shevchenko wrote:
> > On Wed, Mar 24, 2021 at 12:20 PM Matti Vaittinen
> > <matti.vaittinen@fi.rohmeurope.com> wrote:

...

> > > +       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.

...

> > > +       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).

...

> > > +       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.

...

> > > +       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.

...

> > > +       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.

...

> > > +       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.

...

> > > +       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.

...

> > > +       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.

...

> > > +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.
Matti Vaittinen March 28, 2021, 4:59 p.m. UTC | #7
Hi Again Andy,

On Fri, 2021-03-26 at 19:51 +0200, Andy Shevchenko wrote:
> On Fri, Mar 26, 2021 at 3:33 PM Matti Vaittinen
> <matti.vaittinen@fi.rohmeurope.com> wrote:
> > On Fri, 2021-03-26 at 13:26 +0200, Andy Shevchenko wrote:
> > > On Wed, Mar 24, 2021 at 12:20 PM Matti Vaittinen
> > > <matti.vaittinen@fi.rohmeurope.com> wrote:
> 
> 
> > > > +       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).

Right. I see there is the init_valid_mask callback which could be
populated. OTOH, I think this check is actually not needed at all if we
set the ngpios based on the DT flag. The check in set/get functions was
just a symptom of my paranoia. Anyways, I owe you as I just learned
something new :)

> > > > +       int ret;
> > > > +       struct bd71815_gpio *g;
> > > > +       struct device *dev;
> > > > +       struct device *parent;
> ...
> 
> > > > +       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.
> 

I've seen even experienced developers making mistakes by binding the
lifetime of sub-device stuff to parent device's lifetime. I've also
seen even experienced developers who haven't used to dealing with MFD
getting confused when they see parent device's dt-node or regmap being
used. My view on this is that if the comment helps next reader to avoid
a mistake or understand why something is done - then it is worthy. I
have rarely seen comments which make code less readable or
understandable.

> > > > +       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.

Ok. Thanks for pointing it out.


Best Regards
	Matti Vaittinen