| Message ID | b2164e5965218f270e17bf29e00ad5c5a0b54bcf.1616566395.git.matti.vaittinen@fi.rohmeurope.com |
|---|---|
| State | New |
| Headers | show |
| Series | Support ROHM BD71815 PMIC | expand |
On Wed, Mar 24, 2021 at 8:29 AM 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> > --- > Changes since v3: > - No changes This looks OK to me: Acked-by: Linus Walleij <linus.walleij@linaro.org> 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. Yours, Linus Walleij
Hello Linus, On Thu, 2021-03-25 at 10:35 +0100, Linus Walleij wrote: > On Wed, Mar 24, 2021 at 8:29 AM 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> > > --- > > Changes since v3: > > - No changes > > This looks OK to me: > Acked-by: Linus Walleij <linus.walleij@linaro.org> > > 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. Anyways, definitely worth getting familiar with! Thanks for the pointer :] Best Regards, Matti Vaittinen
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. ... > +/* > + * Support to GPOs on ROHM BD71815 > + */ This is effectively one line. ... > +/* For the BD71815 register definitions */ > +#include <linux/mfd/rohm-bd71815.h> Since it's component specific header(s) I would move it to a separate group and locate... > +#include <linux/module.h> > +#include <linux/of.h> You may do better than be OF-centric. See below. > +#include <linux/platform_device.h> > + ...somewhere here. ... > + /* > + * 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 a GPO to the ground > + * 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". > + */ ... > + int ret = 0; Redundant assignment. > + 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. ... > + if (!bd71815->e5_pin_is_gpo && offset) > + return; I wonder if you can use valid_mask instead of this. ... > + bit = BIT(offset); Can be moved to the definition block. ... > + if (!bdgpio->e5_pin_is_gpo && offset) > + return -EOPNOTSUPP; As above. ... > + default: > + break; > + } > + return -EOPNOTSUPP; You may return directly from default. ... > + int ret; > + struct bd71815_gpio *g; > + struct device *dev; > + struct device *parent; Reversed xmas tree order. ... > + /* > + * Bind devm lifetime to this platform device => use dev for devm. > + * also the prints should originate from this device. > + */ Why is this comment needed? ... > + dev = &pdev->dev; Can be done in the definition block. ... > + /* The device-tree and regmap come from MFD => use parent for that */ Why do you need this comment? > + parent = dev->parent; Ditto, can be moved to the definition block. ... > + g->e5_pin_is_gpo = of_property_read_bool(parent->of_node, > + "rohm,enable-hidden-gpo"); You may use device_property_read_bool(). ... > + g->chip.of_node = parent->of_node; Redundant. GPIO library does it for you and even better. ... > + 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(...); ... > +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. > +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, > +}; > + Extra blank line. > +module_platform_driver(gpo_bd71815_driver); > +/* Note: this hardware lives inside an I2C-based multi-function device. */ > +MODULE_ALIAS("platform:bd71815-gpo"); > + Ditto. > +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. > +MODULE_DESCRIPTION("GPO interface for BD71815"); > +MODULE_LICENSE("GPL"); -- With Best Regards, Andy Shevchenko
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
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");
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