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
