Re: [PATCH v5 09/19] gpio: support ROHM BD71815 GPOs

[Matti Vaittinen]

Hi Andy,

On Tue, 2021-03-30 at 13:11 +0300, Andy Shevchenko wrote:
> On Mon, Mar 29, 2021 at 3:58 PM Matti Vaittinen
> <matti.vaittinen@xxxxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxx>
> > although not so much of original is left.
> 
> of the original
> 
> It seems you ignored my comments about the commit message. :-(

Sorry. I didn't do that by purpose. I forgot to reword commit.
Completely my bad.

> > +struct bd71815_gpio {
> > +       struct gpio_chip chip;
> > +       struct device *dev;
> 
> Wondering why you need this. Is it the same as chip.parent?
> 
> > +       struct regmap *regmap;
> > +};
> 
> ...
> 
> > +       int ret, bit;
> > +
> > +       bit = BIT(offset);
> 
> I prefer
>   int bit = BIT(offset);
>   int ret;
> but I think we already discussed that. OK.

Yes, we did.

> ...
> 
> > +       default:
> > +               break;
> > +       }
> > +       return -ENOTSUPP;
> 
> Here is a waste of line. Why break instead of direct return?

As we discussed last time, I do prefer functions which are supposed to
return a value, do so at the end of function. It's easier to read and
does not cause issues if someone changes switch to if-else or does
other modifications. IMO original is safer, reads better and does not
cause issues even with old compilers.

> ...
> 
> > +/* 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,
> 
> Strictly speaking this should be true (boolean type value).

true.

> 
> > +};
> 
> ...
> 
> > +#define BD71815_TWO_GPIOS      0x3UL
> > +#define BD71815_ONE_GPIO       0x1UL
> 
> Are they masks? Can you use BIT() and GENMASK()?

Yes and yes. I personally prefer 0x3 over GENMASK() as for me the value
3 as bitmask is perfectly readable. But I know others may prefer using
GENMASK(). So yes, your comment is valid.

> > +/*
> > + * 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 in the
> 
> and the pin
> 
> > + * datasheet. The functionality is still there though! I guess
> > driving a GPO
> > + * connected to the 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".
> > + */
> 

I am sorry. I think I missed this one too.

> ...
> 
> > +       /*
> > +        * As writing of this the sysfs interface for GPIO control
> > does not
> > +        * respect the valid_mask. Do not trust it but rather set
> > the ngpios
> > +        * to 1 if "rohm,enable-hidden-gpo" is not given.
> > +        *
> > +        * This check can be removed later if the sysfs export is
> > fixed and
> > +        * if the fix is backported.
> 
> So, mark this comment with the TODO/FIXME keyword?

I haven't used to use keywords like TODO/FIXME. Now that I think of it
I've seen a few FIXME comments in sources so perhaps I should start
using them where appropriate. I don't think it makes a big difference
here though as I expect to be reworking this in near future (I'll
revise ROHM PMIC GPIO drivers for regmap_gpio usage during this
spring). I added this comment so I can revise this at that point.

> 
> > +        *
> > +        * For now it is safest to just set the ngpios though.
> > +        */
> 
> ...
> 
> > +       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;
> 
> 

Sorry again. I somehow overlooked this comment as well.

> ...
> 
> > +static struct platform_driver gpo_bd71815_driver = {
> > +       .driver = {
> > +               .name   = "bd71815-gpo",
> > +               .owner  = THIS_MODULE,
> 
> Seems I commented on this. The module_*_driver() macro(s) will take
> care of it.

Yes you did. I missed this too. Sorry.

Andy, how fatal do you think these issues are? I did put these comments
on my 'things to clean-up' list.

If you don't see them as fatal, then I rather not resend whole series
of 19 patches just for these. I am anyway going to rework the ROHM PMIC
GPIO drivers which I have authored during the next couple of months for
regmap_gpio usage. This series has most of the acks except for the
regulator part - so I was about to suggest to Lee that perhaps he could
apply other but regulator stuff to MFD so I could squeeze the recipient
list and amount of patches in series. 

Best Regards
	Matti Vaittinen