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

Message ID b2164e5965218f270e17bf29e00ad5c5a0b54bcf.1616566395.git.matti.vaittinen@fi.rohmeurope.com
State New
Headers show
Series
  • Support ROHM BD71815 PMIC
Related show

Commit Message

Vaittinen, Matti March 24, 2021, 7:29 a.m.
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

 drivers/gpio/Kconfig        |  10 ++
 drivers/gpio/Makefile       |   1 +
 drivers/gpio/gpio-bd71815.c | 176 ++++++++++++++++++++++++++++++++++++
 3 files changed, 187 insertions(+)
 create mode 100644 drivers/gpio/gpio-bd71815.c

Comments

Andy Shevchenko March 26, 2021, 11:27 a.m. | #1
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.

-- 
With Best Regards,
Andy Shevchenko
Vaittinen, Matti March 26, 2021, 1:33 p.m. | #2
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. | #3
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.

-- 
With Best Regards,
Andy Shevchenko
Vaittinen, Matti March 28, 2021, 4:59 p.m. | #4
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
Vaittinen, Matti May 10, 2021, 12:58 p.m. | #5
Hi Linus, All,

On Thu, 2021-03-25 at 12:32 +0200, Matti Vaittinen wrote:
> On Thu, 2021-03-25 at 10:35 +0100, Linus Walleij wrote:

snip

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

So, v5.13-rc1 is out. I started wondering the gpio_regamap - and same
question persists. Why hiding the gpio_chip from gpio_regmap users?

Current IF makes it very hard (impossible?) for driver to override any
of the regmap_gpio functions (or provide own alternatives) for cases
which do not fit the generic regmap_gpio model.

My first obstacle is providing gpio_chip.set_config for BD71815.

1) I guess the method fitting current design would be adding drive-mode 
register/mask(s) to the gpio_regmap_config. Certainly doable - but I
have a bad feeling of this approach. I am afraid this leads to bloating
the gpio_regmap_config with all kinds of IC specific workarounds (when
HW designers have invented new cool control registers setups) - or then
just not using the regmap_gpio for any ICs which have any quirks - even
if 90% of regmap_gpio logic would fit...

2) Other possibility is allowing IC driver to provide function pointers
for some operations (in my case for example for the set_config) - if
the default operation the regmap_gpio provides does not fit the IC.
This would require the regmap_gpio to be visible to IC drivers so that
IC drivers can access the regmap, device & register information - or
some way to convert the gpio_chip pointer to IC specific private data
pointer. Doable but still slightly bloat.

3) The last option would be adding pointer to regmap_gpio to gpio_chip
- and exporting the regmap_gpio functions as helpers - leaving the gpio
registration to be done by the IC driver. That would allow IC driver to
use the regmap_gpio helpers which suit the IC and write own functions
for rest of the stuff.

I'd like to hear opinions - should I draft some changes according to
these proposals (which one, 1,2,3 or something else?) - or as this all
been already discussed and am I just missing something?

Best Regards
	Matti Vaittinen
Andy Shevchenko May 10, 2021, 4:54 p.m. | #6
On Mon, May 10, 2021 at 4:41 PM Matti Vaittinen
<matti.vaittinen@fi.rohmeurope.com> wrote:
>

> Hi Linus, All,

>

> On Thu, 2021-03-25 at 12:32 +0200, Matti Vaittinen wrote:

> > On Thu, 2021-03-25 at 10:35 +0100, Linus Walleij wrote:

>

> snip

>

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

>

> So, v5.13-rc1 is out. I started wondering the gpio_regamap - and same

> question persists. Why hiding the gpio_chip from gpio_regmap users?


In general to me this sounds like opening a window for
non-controllable changes vs. controllable. Besides that, struct
gpio_chip has more than a few callbacks. On top of that, opening this
wide window means you won't be able to stop or refactoring become a
burden. I would be on the stricter side here.

> Current IF makes it very hard (impossible?) for driver to override any

> of the regmap_gpio functions (or provide own alternatives) for cases

> which do not fit the generic regmap_gpio model.

>

> My first obstacle is providing gpio_chip.set_config for BD71815.

>

> 1) I guess the method fitting current design would be adding drive-mode

> register/mask(s) to the gpio_regmap_config. Certainly doable - but I

> have a bad feeling of this approach. I am afraid this leads to bloating

> the gpio_regmap_config with all kinds of IC specific workarounds (when

> HW designers have invented new cool control registers setups) - or then

> just not using the regmap_gpio for any ICs which have any quirks - even

> if 90% of regmap_gpio logic would fit...

>

> 2) Other possibility is allowing IC driver to provide function pointers

> for some operations (in my case for example for the set_config) - if

> the default operation the regmap_gpio provides does not fit the IC.

> This would require the regmap_gpio to be visible to IC drivers so that

> IC drivers can access the regmap, device & register information - or

> some way to convert the gpio_chip pointer to IC specific private data

> pointer. Doable but still slightly bloat.

>

> 3) The last option would be adding pointer to regmap_gpio to gpio_chip

> - and exporting the regmap_gpio functions as helpers - leaving the gpio

> registration to be done by the IC driver. That would allow IC driver to

> use the regmap_gpio helpers which suit the IC and write own functions

> for rest of the stuff.

>

> I'd like to hear opinions - should I draft some changes according to

> these proposals (which one, 1,2,3 or something else?) - or as this all

> been already discussed and am I just missing something?

>

> Best Regards

>         Matti Vaittinen




-- 
With Best Regards,
Andy Shevchenko
Vaittinen, Matti May 11, 2021, 3:59 a.m. | #7
Morning Andy,

On Mon, 2021-05-10 at 19:54 +0300, Andy Shevchenko wrote:
> On Mon, May 10, 2021 at 4:41 PM Matti Vaittinen

> <matti.vaittinen@fi.rohmeurope.com> wrote:

> > Hi Linus, All,

> > 

> > On Thu, 2021-03-25 at 12:32 +0200, Matti Vaittinen wrote:

> > > On Thu, 2021-03-25 at 10:35 +0100, Linus Walleij wrote:

> > 

> > snip

> > 

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

> > 

> > So, v5.13-rc1 is out. I started wondering the gpio_regamap - and

> > same

> > question persists. Why hiding the gpio_chip from gpio_regmap users?

> 

> In general to me this sounds like opening a window for

> non-controllable changes vs. controllable. Besides that, struct

> gpio_chip has more than a few callbacks. On top of that, opening this

> wide window means you won't be able to stop or refactoring become a

> burden. I would be on the stricter side here.


I kind of fail to see your point Andy. Or yes, I know exposing the
gpio_chip to user allows much more flexibility. But what are the
options? What would a driver developer do when his HW does almost fir
the standard regmap_gpio - but not just quite? Say that for example the
changing of gpio direction requires some odd additional register access
- but other than that the regmap_gpio operations like setting/getting
the value, IRQ options etc. fitted the regmap_gpio logic.

If he can not override this one function - then he will need to write
wholly new GPIO driver without re-using any of the regmap-gpio stuff.
You know, if one can't use regmap-gpio, he's likely to use the already
exposed gpio_chip anyways. I'd say this is much more of a pain to
maintain. Or maybe you add another work-around option in the
gpio_regmap_config to indicate this (and every other) oddball HW -
which eventually leads to a mess.

But this is all just my thinking - I'm kind of a "bystander" here and
that's why I asked for opinions. Thanks for sharing yours, Andy. I do
appreciate all the help and discussion.

> > 3) The last option would be adding pointer to regmap_gpio to

> > gpio_chip

> > - and exporting the regmap_gpio functions as helpers - leaving the

> > gpio

> > registration to be done by the IC driver. That would allow IC

> > driver to

> > use the regmap_gpio helpers which suit the IC and write own

> > functions

> > for rest of the stuff.


I was trying to describe here the approach that has been taken in use
at the regulator subsystem - which has used the regmap helpers for
quite a while. I think that approach is scaling quite Ok even for
strange HW.


Best Regards
	Matti Vaittinen
Michael Walle May 14, 2021, 8:34 p.m. | #8
Am 2021-05-11 05:59, schrieb Matti Vaittinen:
> Morning Andy,

> 

> On Mon, 2021-05-10 at 19:54 +0300, Andy Shevchenko wrote:

>> On Mon, May 10, 2021 at 4:41 PM Matti Vaittinen

>> <matti.vaittinen@fi.rohmeurope.com> wrote:

>> > Hi Linus, All,

>> >

>> > On Thu, 2021-03-25 at 12:32 +0200, Matti Vaittinen wrote:

>> > > On Thu, 2021-03-25 at 10:35 +0100, Linus Walleij wrote:

>> >

>> > snip

>> >

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

>> >

>> > So, v5.13-rc1 is out. I started wondering the gpio_regamap - and

>> > same

>> > question persists. Why hiding the gpio_chip from gpio_regmap users?

>> 

>> In general to me this sounds like opening a window for

>> non-controllable changes vs. controllable. Besides that, struct

>> gpio_chip has more than a few callbacks. On top of that, opening this

>> wide window means you won't be able to stop or refactoring become a

>> burden. I would be on the stricter side here.


I tend to agree with Andy. Keep in mind that gpio-regmap was intended
to catch all the simple and similar controllers.

That being said, I'd still like to see new users. I've had a look
at existing drivers myself some time ago and determined that there
are quirks here and there which prevent porting that driver to
gpio-regmap, see for example gpio-mpc8xxx.c, there is a workaround
for some specific SoC which caches some values in the driver.

If we make this gpio-regmap more like a library where users can
just pick the functions they need, I fear that in the end it is
nearly impossible to change such a function because you'll always
break one or another user. But that is just a gut feeling.

> I kind of fail to see your point Andy. Or yes, I know exposing the

> gpio_chip to user allows much more flexibility. But what are the

> options? What would a driver developer do when his HW does almost fir

> the standard regmap_gpio - but not just quite? Say that for example the

> changing of gpio direction requires some odd additional register access

> - but other than that the regmap_gpio operations like setting/getting

> the value, IRQ options etc. fitted the regmap_gpio logic.

> 

> If he can not override this one function - then he will need to write

> wholly new GPIO driver without re-using any of the regmap-gpio stuff.

> You know, if one can't use regmap-gpio, he's likely to use the already

> exposed gpio_chip anyways. I'd say this is much more of a pain to

> maintain. Or maybe you add another work-around option in the

> gpio_regmap_config to indicate this (and every other) oddball HW -

> which eventually leads to a mess.


Agreed, if possible, I'd not like to see options just for one
obscure HW.

> But this is all just my thinking - I'm kind of a "bystander" here and

> that's why I asked for opinions. Thanks for sharing yours, Andy. I do

> appreciate all the help and discussion.

> 

>> > 3) The last option would be adding pointer to regmap_gpio to

>> > gpio_chip

>> > - and exporting the regmap_gpio functions as helpers - leaving the

>> > gpio

>> > registration to be done by the IC driver. That would allow IC

>> > driver to

>> > use the regmap_gpio helpers which suit the IC and write own

>> > functions

>> > for rest of the stuff.

> 

> I was trying to describe here the approach that has been taken in use

> at the regulator subsystem - which has used the regmap helpers for

> quite a while. I think that approach is scaling quite Ok even for

> strange HW.


Do you have any pointers?

-michael
Vaittinen, Matti May 17, 2021, 4:46 a.m. | #9
On Fri, 2021-05-14 at 22:34 +0200, Michael Walle wrote:
> Am 2021-05-11 05:59, schrieb Matti Vaittinen:

> > Morning Andy,

> > 

> > On Mon, 2021-05-10 at 19:54 +0300, Andy Shevchenko wrote:

> > > On Mon, May 10, 2021 at 4:41 PM Matti Vaittinen

> > > <matti.vaittinen@fi.rohmeurope.com> wrote:

> > > > Hi Linus, All,

> > > > 

> > > > On Thu, 2021-03-25 at 12:32 +0200, Matti Vaittinen wrote:

> > > > > On Thu, 2021-03-25 at 10:35 +0100, Linus Walleij wrote:

> > > > 

> > > > snip

> > > > 

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

> > > > 

> > > > So, v5.13-rc1 is out. I started wondering the gpio_regamap -

> > > > and

> > > > same

> > > > question persists. Why hiding the gpio_chip from gpio_regmap

> > > > users?

> > > 

> > > In general to me this sounds like opening a window for

> > > non-controllable changes vs. controllable. Besides that, struct

> > > gpio_chip has more than a few callbacks. On top of that, opening

> > > this

> > > wide window means you won't be able to stop or refactoring become

> > > a

> > > burden. I would be on the stricter side here.

> 

> I tend to agree with Andy. Keep in mind that gpio-regmap was intended

> to catch all the simple and similar controllers.

> 

> That being said, I'd still like to see new users. I've had a look

> at existing drivers myself some time ago and determined that there

> are quirks here and there which prevent porting that driver to

> gpio-regmap, see for example gpio-mpc8xxx.c, there is a workaround

> for some specific SoC which caches some values in the driver.


Yes. In reality we have pretty limited amount of HW which has no quirks
or special things.

> If we make this gpio-regmap more like a library where users can

> just pick the functions they need, I fear that in the end it is

> nearly impossible to change such a function because you'll always

> break one or another user. But that is just a gut feeling.


I am proposing we keep these exported helpers as simple as possible.
When we allow users to use only functions that fit their HW - and write
own functions for HW requiring quirks - then we can indeed catch all
simple and similar controllers - and simple and similar controllers
only. This should allow keeping these functions clean and well defined
in the end :) This should also mean easier to change.

> > But this is all just my thinking - I'm kind of a "bystander" here

> > and

> > that's why I asked for opinions. Thanks for sharing yours, Andy. I

> > do

> > appreciate all the help and discussion.

> > 

> > > > 3) The last option would be adding pointer to regmap_gpio to

> > > > gpio_chip

> > > > - and exporting the regmap_gpio functions as helpers - leaving

> > > > the

> > > > gpio

> > > > registration to be done by the IC driver. That would allow IC

> > > > driver to

> > > > use the regmap_gpio helpers which suit the IC and write own

> > > > functions

> > > > for rest of the stuff.

> > 

> > I was trying to describe here the approach that has been taken in

> > use

> > at the regulator subsystem - which has used the regmap helpers for

> > quite a while. I think that approach is scaling quite Ok even for

> > strange HW.

> 

> Do you have any pointers?

> 


I'm not sure how familiar you are with the regulators but:
The exported helpers are in

drivers/regulator/helpers.c

These helpers can change/list voltages, enable/disable reguators etc.
based on the hardware (register) descritpion given at regulator
registration phase in struct regulator_desc (at
include/regulator/driver.h)

Each driver can fill the description according to it's own HW and use
the provided helpers where suitable (by giving the ops at regulator
registration, see struct regulator_ops, also at driver.h). Or, if the
provided helpers are not useful, user can just write own functions.

If this was brought to GPIO world, it would seem like the gpio_regmap
was not separate GPIO-driver but collection of exported helpers which
use the HW description information embedded in GPIO core.

Let's open the ROHM regulator drivers (which I happen to know :]) as an
example of a driver which uses both the helpers as such, and for some
cases own functions:

bd70528-regulator.c (ramp-delay is not handled by helpers, but it
probably could now when we added ramp-delay helper.)
bd71815-regulator.c (bucks 1 & 2  have special voltage-configuration
cases)
The upstream driver for BD71828 uses only standard helpers - but that's
just because it does not expose all HW features. Vendor driver provides
a 'run-level' specific control options - and can't use standard
functions for all operations.
bd718x7-regulator.c: The regulators require special handling when
voltage is changed (if regulators are enabled). And for some BD71837
bucks this is not allowed at all because change may cause voltage
spikes. So yes - mixture of standard ops and own code is again needed.

So, when looking at this which is kind of an analogy - All of the ROHM
regulator drivers use generic helper code - which saves code and helps
avoiding bugs - but all of them also need(ed) some own code to provide
proper support. If the regulator framework had used as strict policy as
gpio_regmap - then none of the ROHM regulators could use the standard
framework code. I believe pretty many other drivers have same kind of
mixtures. And still, the helpers code has been modified quite a bit
during the last three years I've followed it.

I've spent last 3 years writing mostly the PMIC code - so maybe this
explains my view on the gpio_regmap design :]

It may be the GPIO HW is not _as_ bad with the quirks - but as you
propably have found out already, quite many GPIO drivers have some
quirks which do not fit the gpio_regmap even if some parts would...
This is exactly the point I am addressing here :)

Best Regards
	Matti Vaittinen


> -michael

Patch

diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index e3607ec4c2e8..d3b3de514f6e 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -1105,6 +1105,16 @@  config GPIO_BD70528
 	  This driver can also be built as a module. If so, the module
 	  will be called gpio-bd70528.
 
+config GPIO_BD71815
+	tristate "ROHM BD71815 PMIC GPIO support"
+	depends on MFD_ROHM_BD71828
+	help
+	  Support for GPO(s) on ROHM BD71815 PMIC. There are two GPOs
+	  available on the ROHM PMIC.
+
+	  This driver can also be built as a module. If so, the module
+	  will be called gpio-bd71815.
+
 config GPIO_BD71828
 	tristate "ROHM BD71828 GPIO support"
 	depends on MFD_ROHM_BD71828
diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
index c58a90a3c3b1..4c12f31db31f 100644
--- a/drivers/gpio/Makefile
+++ b/drivers/gpio/Makefile
@@ -39,6 +39,7 @@  obj-$(CONFIG_GPIO_ATH79)		+= gpio-ath79.o
 obj-$(CONFIG_GPIO_BCM_KONA)		+= gpio-bcm-kona.o
 obj-$(CONFIG_GPIO_BCM_XGS_IPROC)	+= gpio-xgs-iproc.o
 obj-$(CONFIG_GPIO_BD70528)		+= gpio-bd70528.o
+obj-$(CONFIG_GPIO_BD71815)		+= gpio-bd71815.o
 obj-$(CONFIG_GPIO_BD71828)		+= gpio-bd71828.o
 obj-$(CONFIG_GPIO_BD9571MWV)		+= gpio-bd9571mwv.o
 obj-$(CONFIG_GPIO_BRCMSTB)		+= gpio-brcmstb.o
diff --git a/drivers/gpio/gpio-bd71815.c b/drivers/gpio/gpio-bd71815.c
new file mode 100644
index 000000000000..5552a23c8e53
--- /dev/null
+++ b/drivers/gpio/gpio-bd71815.c
@@ -0,0 +1,176 @@ 
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Support to GPOs on ROHM BD71815
+ */
+
+#include <linux/gpio/driver.h>
+#include <linux/init.h>
+#include <linux/irq.h>
+/* For the BD71815 register definitions */
+#include <linux/mfd/rohm-bd71815.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+
+struct bd71815_gpio {
+	struct gpio_chip chip;
+	struct device *dev;
+	struct regmap *regmap;
+	/*
+	 * 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
+	 * 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".
+	 */
+	bool e5_pin_is_gpo;
+};
+
+static int bd71815gpo_get(struct gpio_chip *chip, unsigned int offset)
+{
+	struct bd71815_gpio *bd71815 = gpiochip_get_data(chip);
+	int ret = 0;
+	int val;
+
+	ret = regmap_read(bd71815->regmap, BD71815_REG_GPO, &val);
+	if (ret)
+		return ret;
+
+	return (val >> offset) & 1;
+}
+
+static void bd71815gpo_set(struct gpio_chip *chip, unsigned int offset,
+			   int value)
+{
+	struct bd71815_gpio *bd71815 = gpiochip_get_data(chip);
+	int ret, bit;
+
+	if (!bd71815->e5_pin_is_gpo && offset)
+		return;
+
+	bit = BIT(offset);
+
+	if (value)
+		ret = regmap_set_bits(bd71815->regmap, BD71815_REG_GPO, bit);
+	else
+		ret = regmap_clear_bits(bd71815->regmap, BD71815_REG_GPO, bit);
+
+	if (ret)
+		dev_warn(bd71815->dev, "failed to toggle GPO\n");
+}
+
+static int bd71815_gpio_set_config(struct gpio_chip *chip, unsigned int offset,
+				   unsigned long config)
+{
+	struct bd71815_gpio *bdgpio = gpiochip_get_data(chip);
+
+	if (!bdgpio->e5_pin_is_gpo && offset)
+		return -EOPNOTSUPP;
+
+	switch (pinconf_to_config_param(config)) {
+	case PIN_CONFIG_DRIVE_OPEN_DRAIN:
+		return regmap_update_bits(bdgpio->regmap,
+					  BD71815_REG_GPO,
+					  BD71815_GPIO_DRIVE_MASK << offset,
+					  BD71815_GPIO_OPEN_DRAIN << offset);
+	case PIN_CONFIG_DRIVE_PUSH_PULL:
+		return regmap_update_bits(bdgpio->regmap,
+					  BD71815_REG_GPO,
+					  BD71815_GPIO_DRIVE_MASK << offset,
+					  BD71815_GPIO_CMOS << offset);
+	default:
+		break;
+	}
+	return -EOPNOTSUPP;
+}
+
+/* BD71815 GPIO is actually GPO */
+static int bd71815gpo_direction_get(struct gpio_chip *gc, unsigned int offset)
+{
+	return GPIO_LINE_DIRECTION_OUT;
+}
+
+/* Template for GPIO chip */
+static const struct gpio_chip bd71815gpo_chip = {
+	.label			= "bd71815",
+	.owner			= THIS_MODULE,
+	.get			= bd71815gpo_get,
+	.get_direction		= bd71815gpo_direction_get,
+	.set			= bd71815gpo_set,
+	.set_config		= bd71815_gpio_set_config,
+	.can_sleep		= 1,
+};
+
+static int gpo_bd71815_probe(struct platform_device *pdev)
+{
+	int ret;
+	struct bd71815_gpio *g;
+	struct device *dev;
+	struct device *parent;
+
+	/*
+	 * Bind devm lifetime to this platform device => use dev for devm.
+	 * also the prints should originate from this device.
+	 */
+	dev = &pdev->dev;
+	/* The device-tree and regmap come from MFD => use parent for that */
+	parent = dev->parent;
+
+	g = devm_kzalloc(dev, sizeof(*g), GFP_KERNEL);
+	if (!g)
+		return -ENOMEM;
+
+	g->e5_pin_is_gpo = of_property_read_bool(parent->of_node,
+						 "rohm,enable-hidden-gpo");
+	g->chip = bd71815gpo_chip;
+	g->chip.base = -1;
+
+	if (g->e5_pin_is_gpo)
+		g->chip.ngpio = 2;
+	else
+		g->chip.ngpio = 1;
+
+	g->chip.parent = parent;
+	g->chip.of_node = parent->of_node;
+	g->regmap = dev_get_regmap(parent, NULL);
+	g->dev = dev;
+
+	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;
+}
+static const struct platform_device_id bd7181x_gpo_id[] = {
+	{ "bd71815-gpo" },
+	{ },
+};
+MODULE_DEVICE_TABLE(platform, bd7181x_gpo_id);
+
+static struct platform_driver gpo_bd71815_driver = {
+	.driver = {
+		.name	= "bd71815-gpo",
+		.owner	= THIS_MODULE,
+	},
+	.probe		= gpo_bd71815_probe,
+	.id_table	= bd7181x_gpo_id,
+};
+
+module_platform_driver(gpo_bd71815_driver);
+
+/* Note:  this hardware lives inside an I2C-based multi-function device. */
+MODULE_ALIAS("platform:bd71815-gpo");
+
+MODULE_AUTHOR("Peter Yang <yanglsh@embest-tech.com>");
+MODULE_DESCRIPTION("GPO interface for BD71815");
+MODULE_LICENSE("GPL");