diff mbox series

[10/13] gpio: max77650: add GPIO support

Message ID 20190118134244.22253-11-brgl@bgdev.pl (mailing list archive)
State Superseded
Headers show
Series mfd: add support for max77650 PMIC | expand

Commit Message

Bartosz Golaszewski Jan. 18, 2019, 1:42 p.m. UTC
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>
---
 drivers/gpio/Kconfig         |   7 ++
 drivers/gpio/Makefile        |   1 +
 drivers/gpio/gpio-max77650.c | 189 +++++++++++++++++++++++++++++++++++
 3 files changed, 197 insertions(+)
 create mode 100644 drivers/gpio/gpio-max77650.c

Comments

Linus Walleij Jan. 21, 2019, 2:20 p.m. UTC | #1
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
Bartosz Golaszewski Jan. 21, 2019, 5:07 p.m. UTC | #2
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
Linus Walleij Jan. 24, 2019, 10:30 a.m. UTC | #3
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
Bartosz Golaszewski Jan. 29, 2019, 11 a.m. UTC | #4
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
Bartosz Golaszewski Jan. 29, 2019, 1:22 p.m. UTC | #5
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 mbox series

Patch

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");