| Message ID | 20190118134244.22253-11-brgl@bgdev.pl |
|---|---|
| State | New |
| Headers | show |
| Series | mfd: add support for max77650 PMIC | expand |
Hi Bartosz, thanks for the patch! On Fri, Jan 18, 2019 at 2:43 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote: > From: Bartosz Golaszewski <bgolaszewski@baylibre.com> > > Add GPIO support for max77650 mfd device. This PMIC exposes a single > GPIO line. > > Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com> Overall you know for sure what you're doing so not much to say about the GPIO chip etc. The .set_config() is nice and helpful (maybe you will be able to also add pull up/down using Thomas Petazzoni's new config interfaces!) However enlighten me on this: > +static int max77650_gpio_to_irq(struct gpio_chip *gc, unsigned int offset) > +{ > + struct max77650_gpio_chip *chip = gpiochip_get_data(gc); > + > + return regmap_irq_get_virq(chip->irq_chip_data, MAX77650_INT_GPI); > +} I know this may be opening the gates to a lot of coding, but isn't this IRQ hierarchical? I.e. that irqchip is not in the node of the GPIO chip but in the node of the MFD top device, with a 1:1 mapping between some of the IRQs and a certain GPIO line. Using regmap IRQ makes it confusing for me so I do not know for sure if that is the case. I am worried that you are recreating a problem (present in many drivers, including some written by me, mea culpa) that Brian Masney has been trying to solve for the gpiochip inside the SPMI GPIO (drivers/pinctrl/qcom/pinctrl-spmi-gpio.c). I have queued Brians refactoring and solution here: https://git.kernel.org/pub/scm/linux/kernel/git/linusw/linux-gpio.git/log/?h=ib-qcom-spmi And the overall description on what he's trying to achieve is here: https://marc.info/?l=linux-gpio&m=154793071511130&w=2 My problem description (which I fear will apply also to this driver): https://www.spinics.net/lists/linux-gpio/msg34655.html I plan to merge Brians patches soon-ish to devel and then from there try to construct more helpers in the gpiolib core, and if possible fix some of the old drivers who rely on .to_irq(). We will certainly fix ssbi-gpio as well, and that is a good start since the Qualdcomm platforms are so pervasive in the market. But maybe this doesn't apply here? I am not the smartest... Just want to make sure that it is possible to refer an interrupt directly to this DT node, as it is indeed marked as interrupt-controller. Yours, Linus Walleij
pon., 21 sty 2019 o 15:20 Linus Walleij <linus.walleij@linaro.org> napisał(a): > > Hi Bartosz, > > thanks for the patch! > > On Fri, Jan 18, 2019 at 2:43 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote: > > > From: Bartosz Golaszewski <bgolaszewski@baylibre.com> > > > > Add GPIO support for max77650 mfd device. This PMIC exposes a single > > GPIO line. > > > > Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com> > > Overall you know for sure what you're doing so not much to > say about the GPIO chip etc. The .set_config() is nice and > helpful (maybe you will be able to also add pull up/down > using Thomas Petazzoni's new config interfaces!) > > However enlighten me on this: > > > +static int max77650_gpio_to_irq(struct gpio_chip *gc, unsigned int offset) > > +{ > > + struct max77650_gpio_chip *chip = gpiochip_get_data(gc); > > + > > + return regmap_irq_get_virq(chip->irq_chip_data, MAX77650_INT_GPI); > > +} > > I know this may be opening the gates to a lot of coding, but > isn't this IRQ hierarchical? I.e. that irqchip is not in the > node of the GPIO chip but in the node of the MFD top > device, with a 1:1 mapping between some of the IRQs > and a certain GPIO line. > > Using regmap IRQ makes it confusing for me so I do not > know for sure if that is the case. > > I am worried that you are recreating a problem (present in many > drivers, including some written by me, mea culpa) that Brian Masney > has been trying to solve for the gpiochip inside the SPMI > GPIO (drivers/pinctrl/qcom/pinctrl-spmi-gpio.c). > > I have queued Brians refactoring and solution here: > https://git.kernel.org/pub/scm/linux/kernel/git/linusw/linux-gpio.git/log/?h=ib-qcom-spmi > > And the overall description on what he's trying to achieve is > here: > https://marc.info/?l=linux-gpio&m=154793071511130&w=2 > > My problem description (which I fear will apply also to this > driver): > https://www.spinics.net/lists/linux-gpio/msg34655.html > > I plan to merge Brians patches soon-ish to devel and then from > there try to construct more helpers in the gpiolib core, > and if possible fix some of the old drivers who rely on > .to_irq(). > > We will certainly fix ssbi-gpio as well, and that is a good start > since the Qualdcomm platforms are so pervasive in the > market. > > But maybe this doesn't apply here? I am not the smartest... > Just want to make sure that it is possible to refer an > interrupt directly to this DT node, as it is indeed marked > as interrupt-controller. > Hi Linus, Thank you for your review. While I think you're right about the issue being present in this driver, I'm not sure it's really a problem. Do we actually require every gpio-controller to also be a stand-alone interrupt-controller? The binding document for the GPIO module doesn't mention this - it only requires the gpio-controller property. Without the "interrupt-controller" property dtc will bail-out if anyone uses this node as the interrupt parent. If I'm wrong and we do require it, then I think we need to update Documentation/devicetree/bindings/gpio/gpio.txt. Best regards, Bartosz Golaszewski PS: FYI since this submission I dropped the virtual irq number lookup in sub-drivers in favor of resources setup by the parent driver[1] as suggested by Dmitry in his review of the input module driver. [1] https://github.com/brgl/linux/blob/topic/max77650_mfd_driver/drivers/gpio/gpio-max77650.c#L158
On Mon, Jan 21, 2019 at 6:07 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote: > Thank you for your review. While I think you're right about the issue > being present in this driver, I'm not sure it's really a problem. Do > we actually require every gpio-controller to also be a stand-alone > interrupt-controller? Absolutely not :D Just GPIO is fine. > The binding document for the GPIO module doesn't > mention this - it only requires the gpio-controller property. Without > the "interrupt-controller" property dtc will bail-out if anyone uses > this node as the interrupt parent. > > If I'm wrong and we do require it, then I think we need to update > Documentation/devicetree/bindings/gpio/gpio.txt. What is weird is if a driver with DT bindings not mentioning IRQ and only probing from DT start implementing IRQ support, that becomes quite inconsistent. So then max77650_gpio_to_irq() should just return -ENOTSUPP or something for now, then it's fine. We can add the (complicated) IRQ handling later. I am trying to eat my own dogfood here, I was sweating all last night trying to implement a hierarchical IRQ controller. There is no running away from that now. :/ Apparently doing hierarchical IRQs demand that all irq controllers up to the top-level SoC IRQ controller support hierarchical interrupts using the v2 version of the irqdomain API, and currently it seems like the ARM GIC seems like the only top level IRQ controller that can do that. Yours, Linus Walleij
czw., 24 sty 2019 o 11:30 Linus Walleij <linus.walleij@linaro.org> napisał(a): > > On Mon, Jan 21, 2019 at 6:07 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote: > > > Thank you for your review. While I think you're right about the issue > > being present in this driver, I'm not sure it's really a problem. Do > > we actually require every gpio-controller to also be a stand-alone > > interrupt-controller? > > Absolutely not :D > > Just GPIO is fine. > > > The binding document for the GPIO module doesn't > > mention this - it only requires the gpio-controller property. Without > > the "interrupt-controller" property dtc will bail-out if anyone uses > > this node as the interrupt parent. > > > > If I'm wrong and we do require it, then I think we need to update > > Documentation/devicetree/bindings/gpio/gpio.txt. > > What is weird is if a driver with DT bindings not mentioning IRQ > and only probing from DT start implementing IRQ support, that > becomes quite inconsistent. So then max77650_gpio_to_irq() > should just return -ENOTSUPP > or something for now, then it's fine. > I don't see it as weird at all. I see the need to define the register and interrupt resources in DT for SoC peripherals becaue SoCs often reuse IPs. But in the case of a self-contained i2c PMIC - the modules such as GPIO are tightly coupled with the core functionality. In the case of this device for example: there isn't even a separate set of mask/status registers for GPIO interrupts. Most mfd devices setup the resources in a hard-coded manner. > We can add the (complicated) IRQ handling later. > > I am trying to eat my own dogfood here, I was sweating all > last night trying to implement a hierarchical IRQ controller. > There is no running away from that now. :/ > > Apparently doing hierarchical IRQs demand that all irq > controllers up to the top-level SoC IRQ controller support > hierarchical interrupts using the v2 version of the irqdomain > API, and currently it seems like the ARM > GIC seems like the only top level IRQ controller that can > do that. > Yep, and for that reason I can't use the regmap irq_chip abstraction for now because it doesn't implement support for hierarchical interrupts either. How about the cascaded gpiochip irq_chip? Best regards, Bartosz
wt., 29 sty 2019 o 12:00 Bartosz Golaszewski <brgl@bgdev.pl> napisał(a): > > czw., 24 sty 2019 o 11:30 Linus Walleij <linus.walleij@linaro.org> napisał(a): > > > > On Mon, Jan 21, 2019 at 6:07 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote: > > > > > Thank you for your review. While I think you're right about the issue > > > being present in this driver, I'm not sure it's really a problem. Do > > > we actually require every gpio-controller to also be a stand-alone > > > interrupt-controller? > > > > Absolutely not :D > > > > Just GPIO is fine. > > > > > The binding document for the GPIO module doesn't > > > mention this - it only requires the gpio-controller property. Without > > > the "interrupt-controller" property dtc will bail-out if anyone uses > > > this node as the interrupt parent. > > > > > > If I'm wrong and we do require it, then I think we need to update > > > Documentation/devicetree/bindings/gpio/gpio.txt. > > > > What is weird is if a driver with DT bindings not mentioning IRQ > > and only probing from DT start implementing IRQ support, that > > becomes quite inconsistent. So then max77650_gpio_to_irq() > > should just return -ENOTSUPP > > or something for now, then it's fine. > > > > I don't see it as weird at all. I see the need to define the register > and interrupt resources in DT for SoC peripherals becaue SoCs often > reuse IPs. But in the case of a self-contained i2c PMIC - the modules > such as GPIO are tightly coupled with the core functionality. In the > case of this device for example: there isn't even a separate set of > mask/status registers for GPIO interrupts. > > Most mfd devices setup the resources in a hard-coded manner. > > > We can add the (complicated) IRQ handling later. > > > > I am trying to eat my own dogfood here, I was sweating all > > last night trying to implement a hierarchical IRQ controller. > > There is no running away from that now. :/ > > > > Apparently doing hierarchical IRQs demand that all irq > > controllers up to the top-level SoC IRQ controller support > > hierarchical interrupts using the v2 version of the irqdomain > > API, and currently it seems like the ARM > > GIC seems like the only top level IRQ controller that can > > do that. > > > > Yep, and for that reason I can't use the regmap irq_chip abstraction > for now because it doesn't implement support for hierarchical > interrupts either. > > How about the cascaded gpiochip irq_chip? > > Best regards, > Bartosz Nah that won't work either without a proper hierarchy... In that case, let's leave out the irq support for now. I'll send v2. Bart
diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig index b5a2845347ec..fb297fe5bfec 100644 --- a/drivers/gpio/Kconfig +++ b/drivers/gpio/Kconfig @@ -1095,6 +1095,13 @@ config GPIO_MAX77620 driver also provides interrupt support for each of the gpios. Say yes here to enable the max77620 to be used as gpio controller. +config GPIO_MAX77650 + tristate "Maxim MAX77650/77651 GPIO support" + depends on MFD_MAX77650 + help + GPIO driver for MAX77650/77651 PMIC from Maxim Semiconductor. + These chips have a single pin that can be configured as GPIO. + config GPIO_MSIC bool "Intel MSIC mixed signal gpio support" depends on (X86 || COMPILE_TEST) && MFD_INTEL_MSIC diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile index 37628f8dbf70..8bdad50db822 100644 --- a/drivers/gpio/Makefile +++ b/drivers/gpio/Makefile @@ -78,6 +78,7 @@ obj-$(CONFIG_GPIO_MAX7300) += gpio-max7300.o obj-$(CONFIG_GPIO_MAX7301) += gpio-max7301.o obj-$(CONFIG_GPIO_MAX732X) += gpio-max732x.o obj-$(CONFIG_GPIO_MAX77620) += gpio-max77620.o +obj-$(CONFIG_GPIO_MAX77650) += gpio-max77650.o obj-$(CONFIG_GPIO_MB86S7X) += gpio-mb86s7x.o obj-$(CONFIG_GPIO_MENZ127) += gpio-menz127.o obj-$(CONFIG_GPIO_MERRIFIELD) += gpio-merrifield.o diff --git a/drivers/gpio/gpio-max77650.c b/drivers/gpio/gpio-max77650.c new file mode 100644 index 000000000000..f8fe2b083e4d --- /dev/null +++ b/drivers/gpio/gpio-max77650.c @@ -0,0 +1,189 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * Copyright (C) 2018 BayLibre SAS + * Author: Bartosz Golaszewski <bgolaszewski@baylibre.com> + * + * GPIO driver for MAXIM 77650/77651 charger/power-supply. + */ + +#include <linux/gpio/driver.h> +#include <linux/i2c.h> +#include <linux/mfd/max77650.h> +#include <linux/module.h> +#include <linux/platform_device.h> +#include <linux/regmap.h> + +#define MAX77650_GPIO_DIR_MASK BIT(0) +#define MAX77650_GPIO_INVAL_MASK BIT(1) +#define MAX77650_GPIO_DRV_MASK BIT(2) +#define MAX77650_GPIO_OUTVAL_MASK BIT(3) +#define MAX77650_GPIO_DEBOUNCE_MASK BIT(4) + +#define MAX77650_GPIO_DIR_OUT 0x00 +#define MAX77650_GPIO_DIR_IN BIT(0) +#define MAX77650_GPIO_OUT_LOW 0x00 +#define MAX77650_GPIO_OUT_HIGH BIT(3) +#define MAX77650_GPIO_DRV_OPEN_DRAIN 0x00 +#define MAX77650_GPIO_DRV_PUSH_PULL BIT(2) +#define MAX77650_GPIO_DEBOUNCE BIT(4) + +#define MAX77650_GPIO_DIR_BITS(_reg) \ + ((_reg) & MAX77650_GPIO_DIR_MASK) +#define MAX77650_GPIO_INVAL_BITS(_reg) \ + (((_reg) & MAX77650_GPIO_INVAL_MASK) >> 1) + +struct max77650_gpio_chip { + struct regmap *map; + struct regmap_irq_chip_data *irq_chip_data; + struct gpio_chip gc; +}; + +static int max77650_gpio_direction_input(struct gpio_chip *gc, + unsigned int offset) +{ + struct max77650_gpio_chip *chip = gpiochip_get_data(gc); + + return regmap_update_bits(chip->map, + MAX77650_REG_CNFG_GPIO, + MAX77650_GPIO_DIR_MASK, + MAX77650_GPIO_DIR_IN); +} + +static int max77650_gpio_direction_output(struct gpio_chip *gc, + unsigned int offset, int value) +{ + struct max77650_gpio_chip *chip = gpiochip_get_data(gc); + int mask, regval; + + mask = MAX77650_GPIO_DIR_MASK | MAX77650_GPIO_OUTVAL_MASK; + regval = value ? MAX77650_GPIO_OUT_HIGH : MAX77650_GPIO_OUT_LOW; + regval |= MAX77650_GPIO_DIR_OUT; + + return regmap_update_bits(chip->map, + MAX77650_REG_CNFG_GPIO, mask, regval); +} + +static void max77650_gpio_set_value(struct gpio_chip *gc, + unsigned int offset, int value) +{ + struct max77650_gpio_chip *chip = gpiochip_get_data(gc); + int rv, regval; + + regval = value ? MAX77650_GPIO_OUT_HIGH : MAX77650_GPIO_OUT_LOW; + + rv = regmap_update_bits(chip->map, MAX77650_REG_CNFG_GPIO, + MAX77650_GPIO_OUTVAL_MASK, regval); + if (rv) + dev_err(gc->parent, "cannot set GPIO value: %d\n", rv); +} + +static int max77650_gpio_get_value(struct gpio_chip *gc, + unsigned int offset) +{ + struct max77650_gpio_chip *chip = gpiochip_get_data(gc); + unsigned int val; + int rv; + + rv = regmap_read(chip->map, MAX77650_REG_CNFG_GPIO, &val); + if (rv) + return rv; + + return MAX77650_GPIO_INVAL_BITS(val); +} + +static int max77650_gpio_get_direction(struct gpio_chip *gc, + unsigned int offset) +{ + struct max77650_gpio_chip *chip = gpiochip_get_data(gc); + unsigned int val; + int rv; + + rv = regmap_read(chip->map, MAX77650_REG_CNFG_GPIO, &val); + if (rv) + return rv; + + return MAX77650_GPIO_DIR_BITS(val); +} + +static int max77650_gpio_set_config(struct gpio_chip *gc, + unsigned int offset, unsigned long cfg) +{ + struct max77650_gpio_chip *chip = gpiochip_get_data(gc); + + switch (pinconf_to_config_param(cfg)) { + case PIN_CONFIG_DRIVE_OPEN_DRAIN: + return regmap_update_bits(chip->map, + MAX77650_REG_CNFG_GPIO, + MAX77650_GPIO_DRV_MASK, + MAX77650_GPIO_DRV_OPEN_DRAIN); + case PIN_CONFIG_DRIVE_PUSH_PULL: + return regmap_update_bits(chip->map, + MAX77650_REG_CNFG_GPIO, + MAX77650_GPIO_DRV_MASK, + MAX77650_GPIO_DRV_PUSH_PULL); + case PIN_CONFIG_INPUT_DEBOUNCE: + return regmap_update_bits(chip->map, + MAX77650_REG_CNFG_GPIO, + MAX77650_GPIO_DEBOUNCE_MASK, + MAX77650_GPIO_DEBOUNCE); + default: + return -ENOTSUPP; + } +} + +static int max77650_gpio_to_irq(struct gpio_chip *gc, unsigned int offset) +{ + struct max77650_gpio_chip *chip = gpiochip_get_data(gc); + + return regmap_irq_get_virq(chip->irq_chip_data, MAX77650_INT_GPI); +} + +static int max77650_gpio_probe(struct platform_device *pdev) +{ + struct max77650_gpio_chip *chip; + struct device *dev, *parent; + struct i2c_client *i2c; + + dev = &pdev->dev; + parent = dev->parent; + i2c = to_i2c_client(parent); + + chip = devm_kzalloc(dev, sizeof(*chip), GFP_KERNEL); + if (!chip) + return -ENOMEM; + + chip->map = dev_get_regmap(parent, NULL); + if (!chip->map) + return -ENODEV; + + chip->irq_chip_data = i2c_get_clientdata(i2c); + + chip->gc.base = -1; + chip->gc.ngpio = 1; + chip->gc.label = i2c->name; + chip->gc.parent = dev; + chip->gc.owner = THIS_MODULE; + chip->gc.can_sleep = true; + + chip->gc.direction_input = max77650_gpio_direction_input; + chip->gc.direction_output = max77650_gpio_direction_output; + chip->gc.set = max77650_gpio_set_value; + chip->gc.get = max77650_gpio_get_value; + chip->gc.get_direction = max77650_gpio_get_direction; + chip->gc.set_config = max77650_gpio_set_config; + chip->gc.to_irq = max77650_gpio_to_irq; + + return devm_gpiochip_add_data(dev, &chip->gc, chip); +} + +static struct platform_driver max77650_gpio_driver = { + .driver = { + .name = "max77650-gpio", + }, + .probe = max77650_gpio_probe, +}; +module_platform_driver(max77650_gpio_driver); + +MODULE_DESCRIPTION("MAXIM 77650/77651 GPIO driver"); +MODULE_AUTHOR("Bartosz Golaszewski <bgolaszewski@baylibre.com>"); +MODULE_LICENSE("GPL");