Messages in this thread | | | From | Linus Walleij <> | Date | Sat, 9 Jan 2021 01:45:50 +0100 | Subject | Re: [PATCH 09/15] gpio: support ROHM BD71815 GPOs |
| |
On Fri, Jan 8, 2021 at 2:39 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 > 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>
Overall this looks good!
> + depends on MFD_ROHM_BD71828
I suppose this makes i possible to merge out-of-order with the core patches actually.
> +#define DEBUG
Why? Development artifact?
> +#include <linux/kthread.h>
You certainly do not need this.
> +#include <linux/mfd/rohm-bd71815.h> > +#include <linux/mfd/rohm-generic.h>
I guess registers come from these? Do you need both? Add a comment about what they provide.
> + g->chip.ngpio = 1; > + if (g->e5_pin_is_gpo) > + g->chip.ngpio = 2;
Overwriting value, how not elegant.
if (g->e5_pin_is_gpo) g->chip.ngpio = 2; else g->chip.ngpio = 1;
> + g->chip.parent = pdev->dev.parent; > + g->chip.of_node = pdev->dev.parent->of_node; > + g->regmap = dev_get_regmap(pdev->dev.parent, NULL); > + g->dev = &pdev->dev; > + > + ret = devm_gpiochip_add_data(&pdev->dev, &g->chip, g); > + if (ret < 0) { > + dev_err(&pdev->dev, "could not register gpiochip, %d\n", ret); > + return ret; > + }
It's a bit confusing how you use pdev->dev.parent for some stuff and &pdev->dev for some.
What about assinging
struct device *dev = pdev->dev.parent;
and use dev for all the calls, it looks like it'd work fine.
Yours, Linus Walleij
| |