| Message ID | 118a6160880a212d20d0251f763cad295c741b4d.1617020713.git.matti.vaittinen@fi.rohmeurope.com |
|---|---|
| State | New |
| Headers | show |
| Series | Support ROHM BD71815 PMIC | expand |
On Mon, Mar 29, 2021 at 3:58 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 It seems you ignored my comments about the commit message. :-( > +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. ... > + default: > + break; > + } > + return -ENOTSUPP; Here is a waste of line. Why break instead of direct return? ... > +/* 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). > +}; ... > +#define BD71815_TWO_GPIOS 0x3UL > +#define BD71815_ONE_GPIO 0x1UL Are they masks? Can you use BIT() and GENMASK()? ... > +/* > + * 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". > + */ ... > + /* > + * 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? > + * > + * 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; This entire piece can be simplified by return devm_gpiochip_add_data(...); ... > +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. > + }, > + .probe = gpo_bd71815_probe, > +}; > + Extra blank line. Drop it. > +module_platform_driver(gpo_bd71815_driver);
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@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 > > 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
On Tue, Mar 30, 2021 at 1:43 PM Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com> wrote: > On Tue, 2021-03-30 at 13:11 +0300, Andy Shevchenko wrote: ... > 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. I understand that. I'm not a maintainer, but my personal view is that it can be fixed in follow ups. The problem as usual here is that people often forget to cook / send follow up. That's why lately I'm more insisting on changes to be done as soon as possible.
On Tue, 2021-03-30 at 13:54 +0300, Andy Shevchenko wrote: > On Tue, Mar 30, 2021 at 1:43 PM Matti Vaittinen > <matti.vaittinen@fi.rohmeurope.com> wrote: > > On Tue, 2021-03-30 at 13:11 +0300, Andy Shevchenko wrote: > > ... > > > 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. > > I understand that. I'm not a maintainer, but my personal view is that > it can be fixed in follow ups. Thanks Andy. The series already had acks from Bartosz and Linus so I hope they are also Ok with fixing these when reworking for regmap_gpio (I intend to do that during 5.13-rc cycle). Best Regards Matti Vaittinen
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@fi.rohmeurope.com> wrote: > > > > +struct bd71815_gpio { > > + struct gpio_chip chip; > > + struct device *dev; > > Wondering why you need this. Is it the same as chip.parent? > This is exactly the reason why I had the comments you objected in the probe. dev is pointer to the platform device - which should be used for prints and any potential devm stuff. chip.parent is the MFD device which provides the regmap access and DT node. Best Regards Matti Vaittinen
On Tue, Mar 30, 2021 at 3:06 PM Vaittinen, Matti <Matti.Vaittinen@fi.rohmeurope.com> wrote: > 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@fi.rohmeurope.com> wrote: > > > > > > +struct bd71815_gpio { > > > + struct gpio_chip chip; > > > + struct device *dev; > > > > Wondering why you need this. Is it the same as chip.parent? > > > > This is exactly the reason why I had the comments you objected in the > probe. dev is pointer to the platform device - which should be used for > prints and any potential devm stuff. > > chip.parent is the MFD device which provides the regmap access and DT > node. We have a kernel doc for such things. If you commented it in the first place around this structure, it will be obvious. Now you have dangling comment somewhere and no clue for reader why you have struct device pointer here.
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..c7f37813d629 --- /dev/null +++ b/drivers/gpio/gpio-bd71815.c @@ -0,0 +1,193 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * Support to GPOs on ROHM BD71815 + * Copyright 2021 ROHM Semiconductors. + * Author: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com> + * + * Copyright 2014 Embest Technology Co. Ltd. Inc. + * Author: yanglsh@embest-tech.com + */ + +#include <linux/gpio/driver.h> +#include <linux/init.h> +#include <linux/irq.h> +#include <linux/module.h> +#include <linux/of.h> +#include <linux/platform_device.h> +/* For the BD71815 register definitions */ +#include <linux/mfd/rohm-bd71815.h> + +struct bd71815_gpio { + struct gpio_chip chip; + struct device *dev; + struct regmap *regmap; +}; + +static int bd71815gpo_get(struct gpio_chip *chip, unsigned int offset) +{ + struct bd71815_gpio *bd71815 = gpiochip_get_data(chip); + int ret, 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; + + 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); + + 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 -ENOTSUPP; +} + +/* 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, +}; + +#define BD71815_TWO_GPIOS 0x3UL +#define BD71815_ONE_GPIO 0x1UL + +/* + * 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 + * 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". + */ +static int bd71815_init_valid_mask(struct gpio_chip *gc, + unsigned long *valid_mask, + unsigned int ngpios) +{ + if (ngpios != 2) + return 0; + + if (gc->parent && device_property_present(gc->parent, + "rohm,enable-hidden-gpo")) + *valid_mask = BD71815_TWO_GPIOS; + else + *valid_mask = BD71815_ONE_GPIO; + + return 0; +} + +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->chip = bd71815gpo_chip; + + /* + * 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. + * + * For now it is safest to just set the ngpios though. + */ + if (device_property_present(parent, "rohm,enable-hidden-gpo")) + g->chip.ngpio = 2; + else + g->chip.ngpio = 1; + + g->chip.init_valid_mask = bd71815_init_valid_mask; + g->chip.base = -1; + g->chip.parent = parent; + 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 struct platform_driver gpo_bd71815_driver = { + .driver = { + .name = "bd71815-gpo", + .owner = THIS_MODULE, + }, + .probe = gpo_bd71815_probe, +}; + +module_platform_driver(gpo_bd71815_driver); + +MODULE_ALIAS("platform:bd71815-gpo"); +MODULE_AUTHOR("Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>"); +MODULE_AUTHOR("Peter Yang <yanglsh@embest-tech.com>"); +MODULE_DESCRIPTION("GPO interface for BD71815"); +MODULE_LICENSE("GPL");