diff mbox series

[v4,2/3] pinctrl: tps6594: Add driver for TPS6594 pinctrl and GPIOs

Message ID 20230512141755.1712358-3-eblanc@baylibre.com
State New
Headers show
Series TI TPS6594 PMIC support (RTC, pinctrl, regulators) | expand

Commit Message

Esteban Blanc May 12, 2023, 2:17 p.m. UTC
TI TPS6594 PMIC has 11 GPIOs which can be used
for different functions.

This patch adds a pinctrl and GPIO drivers in
order to use those functions.

Signed-off-by: Esteban Blanc <eblanc@baylibre.com>
---
 drivers/pinctrl/Kconfig           |  31 +++
 drivers/pinctrl/Makefile          |   2 +
 drivers/pinctrl/pinctrl-tps6594.c | 301 ++++++++++++++++++++++++++++++
 include/linux/mfd/tps6594.h       |   3 +-
 4 files changed, 336 insertions(+), 1 deletion(-)
 create mode 100644 drivers/pinctrl/pinctrl-tps6594.c

Comments

Andy Shevchenko May 12, 2023, 5:07 p.m. UTC | #1
Fri, May 12, 2023 at 04:17:54PM +0200, Esteban Blanc kirjoitti:
> TI TPS6594 PMIC has 11 GPIOs which can be used
> for different functions.
> 
> This patch adds a pinctrl and GPIO drivers in
> order to use those functions.

...

> +config PINCTRL_THUNDERBAY

Is it correct name? To me sounds not. The problem is that you use platform name
for the non-platform-wide pin control, i.e. for PMIC exclusively.
Did I miss anything?

> +	tristate "Generic pinctrl and GPIO driver for Intel Thunder Bay SoC"
> +	depends on ARCH_THUNDERBAY || (ARM64 && COMPILE_TEST)

This doesn't look correct, but I remember some Kconfig options that are using
this way of dependency.

> +	depends on HAS_IOMEM
> +	select PINMUX
> +	select PINCONF
> +	select GENERIC_PINCONF
> +	select GENERIC_PINCTRL_GROUPS
> +	select GENERIC_PINMUX_FUNCTIONS
> +	select GPIOLIB
> +	select GPIOLIB_IRQCHIP
> +	select GPIO_GENERIC
> +	help
> +	  This selects pin control driver for the Intel Thunder Bay SoC.
> +	  It provides pin config functions such as pull-up, pull-down,
> +	  interrupt, drive strength, sec lock, Schmitt trigger, slew
> +	  rate control and direction control. This module will be
> +	  called as pinctrl-thunderbay.

Ah, the above simply a mistake. right?
Why is it in this patch?

> +config PINCTRL_TPS6594
> +	tristate "Pinctrl and GPIO driver for TI TPS6594 PMIC"
> +	depends on MFD_TPS6594
> +	default MFD_TPS6594
> +	select PINMUX
> +	select GPIOLIB
> +	select REGMAP
> +	select GPIO_REGMAP
> +	help
> +	  This driver supports GPIOs and pinmuxing for the TPS6594
> +	  PMICs chip family.

Module name?

...

> +obj-$(CONFIG_PINCTRL_THUNDERBAY) += pinctrl-thunderbay.o

Huh?!

> +obj-$(CONFIG_PINCTRL_TPS6594)	+= pinctrl-tps6594.o

...

> +#include <linux/gpio/regmap.h>
> +#include <linux/gpio/driver.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/pinctrl/pinmux.h>

Ordered?

...

> +static const char *groups_name[TPS6594_PINCTRL_PINS_NB] = {
> +	"GPIO0", "GPIO1", "GPIO2", "GPIO3", "GPIO4", "GPIO5",
> +	"GPIO6", "GPIO7", "GPIO8", "GPIO9", "GPIO10"

Leave trailing comma even for known size.

> +};

...

> +struct tps6594_pinctrl_function {
> +	const char *name;
> +	u8 muxval;
> +	const char **groups;
> +	unsigned long ngroups;

We have struct pinfunction. Use it here (as embedded).

> +};

...

> +static const struct tps6594_pinctrl_function pinctrl_functions[] = {
> +	{ "gpio", TPS6594_PINCTRL_GPIO_FUNCTION, groups_name,
> +	  TPS6594_PINCTRL_PINS_NB },

Here and further use PINCTRL_PINFUNCTION() macro.

> +};

...

> +static int tps6594_group_pins(struct pinctrl_dev *pctldev,
> +			      unsigned int selector, const unsigned int **pins,
> +			      unsigned int *num_pins)
> +{
> +	struct tps6594_pinctrl *pinctrl = pinctrl_dev_get_drvdata(pctldev);
> +
> +	*pins = (unsigned int *)&pinctrl->pins[selector];

Why casting?

> +	*num_pins = 1;
> +
> +	return 0;
> +}

...

> +	pinctrl->pctl_dev =
> +		devm_pinctrl_register(&pdev->dev, pctrl_desc, pinctrl);

One line?

> +	if (IS_ERR(pinctrl->pctl_dev)) {
> +		dev_err(&pdev->dev, "Couldn't register pinctrl driver\n");
> +		return PTR_ERR(pinctrl->pctl_dev);

	return dev_err_probe(...);

> +	}

...

> +	pinctrl->gpio_regmap = devm_gpio_regmap_register(&pdev->dev, &config);
> +	if (IS_ERR(pinctrl->gpio_regmap)) {
> +		dev_err(&pdev->dev, "Couldn't register gpio_regmap driver\n");
> +		return PTR_ERR(pinctrl->pctl_dev);

Ditto.

> +	}
> +
> +	return 0;
> +}

...

> -#define TPS6594_REG_GPIOX_CONF(gpio_inst)		(0x31 + (gpio_inst))
> +#define TPS6594_REG_GPIO1_CONF				0x31
> +#define TPS6594_REG_GPIOX_CONF(gpio_inst)	(TPS6594_REG_GPIO1_CONF + (gpio_inst))

Why? The original code with parameter 0 will issue the same.
Esteban Blanc May 16, 2023, 1:05 p.m. UTC | #2
On Fri May 12, 2023 at 7:07 PM CEST,  wrote:
> Fri, May 12, 2023 at 04:17:54PM +0200, Esteban Blanc kirjoitti:
> > TI TPS6594 PMIC has 11 GPIOs which can be used
> > for different functions.
> > 
> > This patch adds a pinctrl and GPIO drivers in
> > order to use those functions.
>
> ...
>
> > +config PINCTRL_THUNDERBAY
>
> Is it correct name? To me sounds not. The problem is that you use platform name
> for the non-platform-wide pin control, i.e. for PMIC exclusively.
> Did I miss anything?
>
> > +	tristate "Generic pinctrl and GPIO driver for Intel Thunder Bay SoC"
> > +	depends on ARCH_THUNDERBAY || (ARM64 && COMPILE_TEST)
>
> This doesn't look correct, but I remember some Kconfig options that are using
> this way of dependency.
>
> > +	depends on HAS_IOMEM
> > +	select PINMUX
> > +	select PINCONF
> > +	select GENERIC_PINCONF
> > +	select GENERIC_PINCTRL_GROUPS
> > +	select GENERIC_PINMUX_FUNCTIONS
> > +	select GPIOLIB
> > +	select GPIOLIB_IRQCHIP
> > +	select GPIO_GENERIC
> > +	help
> > +	  This selects pin control driver for the Intel Thunder Bay SoC.
> > +	  It provides pin config functions such as pull-up, pull-down,
> > +	  interrupt, drive strength, sec lock, Schmitt trigger, slew
> > +	  rate control and direction control. This module will be
> > +	  called as pinctrl-thunderbay.
>
> Ah, the above simply a mistake. right?
> Why is it in this patch?

I respond to all comments about PINCTRL_THUNDERBAY.
It is indeed a mistake on my side. I failed my rebase on 6.4-rc1...
I will fix it for the next version.
Sorry about this...

> > +config PINCTRL_TPS6594
> > +	tristate "Pinctrl and GPIO driver for TI TPS6594 PMIC"
> > +	depends on MFD_TPS6594
> > +	default MFD_TPS6594
> > +	select PINMUX
> > +	select GPIOLIB
> > +	select REGMAP
> > +	select GPIO_REGMAP
> > +	help
> > +	  This driver supports GPIOs and pinmuxing for the TPS6594
> > +	  PMICs chip family.
>
> Module name?

I'm not sure to understand what you are looking for. Something like this
?:

To compile this driver as a module, choose M here: the
module will be called pinctrl-tps6594.

> > +obj-$(CONFIG_PINCTRL_THUNDERBAY) += pinctrl-thunderbay.o
>
> Huh?!

Same as for Kconfig file, it's a mistake.

> > +#include <linux/gpio/regmap.h>
> > +#include <linux/gpio/driver.h>
> > +#include <linux/module.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/pinctrl/pinmux.h>
>
> Ordered?
>

It's not, I fix this.

> > +static int tps6594_group_pins(struct pinctrl_dev *pctldev,
> > +			      unsigned int selector, const unsigned int **pins,
> > +			      unsigned int *num_pins)
> > +{
> > +	struct tps6594_pinctrl *pinctrl = pinctrl_dev_get_drvdata(pctldev);
> > +
> > +	*pins = (unsigned int *)&pinctrl->pins[selector];
>
> Why casting?

It's an error, thanks.

> > +	pinctrl->pctl_dev =
> > +		devm_pinctrl_register(&pdev->dev, pctrl_desc, pinctrl);
>
> One line?

I use clang-format quite extensively so I would rather keep it like
that to still be able to just run it over the whole file without having
to fix this line every time.
If you feel like I should not respect the 80 columns recommendation, I
will fix it.

> > +	if (IS_ERR(pinctrl->pctl_dev)) {
> > +		dev_err(&pdev->dev, "Couldn't register pinctrl driver\n");
> > +		return PTR_ERR(pinctrl->pctl_dev);
>
> 	return dev_err_probe(...);

Did not know this one, I will use it. Thanks.

> > -#define TPS6594_REG_GPIOX_CONF(gpio_inst)		(0x31 + (gpio_inst))
> > +#define TPS6594_REG_GPIO1_CONF				0x31
> > +#define TPS6594_REG_GPIOX_CONF(gpio_inst)	(TPS6594_REG_GPIO1_CONF + (gpio_inst))
>
> Why? The original code with parameter 0 will issue the same.

I felt that replacing 0x31 with a constant would make the computation
in TPS6594_REG_GPIOX_CONFIG more understandable. What do you think?


Thanks for your time. Sorry again about this "thunderbay" confusion...
Note for the future, don't send patches on Fridays :)

Best regards,
Esteban Blanc May 22, 2023, 8:45 a.m. UTC | #3
On Wed May 17, 2023 at 5:04 PM CEST, Andy Shevchenko wrote:
> On Wed, May 17, 2023 at 5:43 PM Esteban Blanc <eblanc@baylibre.com> wrote:
> > On Wed May 17, 2023 at 3:51 PM CEST, Andy Shevchenko wrote:
> > > On Wed, May 17, 2023 at 12:58 PM Esteban Blanc <eblanc@baylibre.com> wrote:
> > > > On Tue May 16, 2023 at 6:48 PM CEST, Andy Shevchenko wrote:
> > > > > On Tue, May 16, 2023 at 4:05 PM Esteban Blanc <eblanc@baylibre.com> wrote:
> > > > > > On Fri May 12, 2023 at 7:07 PM CEST,  wrote:
> > > > > > > Fri, May 12, 2023 at 04:17:54PM +0200, Esteban Blanc kirjoitti:
>
> ...
>
> > > > > > > > -#define TPS6594_REG_GPIOX_CONF(gpio_inst)          (0x31 + (gpio_inst))
> > > > > > > > +#define TPS6594_REG_GPIO1_CONF                             0x31
> > > > > > > > +#define TPS6594_REG_GPIOX_CONF(gpio_inst)  (TPS6594_REG_GPIO1_CONF + (gpio_inst))
> > > > > > >
> > > > > > > Why? The original code with parameter 0 will issue the same.
> > > > > >
> > > > > > I felt that replacing 0x31 with a constant would make the computation
> > > > > > in TPS6594_REG_GPIOX_CONFIG more understandable. What do you think?
> > > > >
> > > > > The question is why that register is so special that you need to have
> > > > > it as a constant explicitly?
> > > >
> > > > It is not special, it's just the first one of the serie of config
> > > > registers. I felt like just having 0x31 without context was a bit weird
> > >
> > > I'm not sure I understand what 'context' you are talking about.
> > I was trying to convey the fact that 0x31 was representing
> > TPS6594_REG_GPIO1_CONF address. This way when looking at
> > TPS6594_REG_GPIOX_CONF(...), one will better understand that this macro
> > is just about offsetting from the first GPIO_CONF register.
>
> You can add a comment on top of the macro, so anybody can read and see
> what this macro is doing.

Ok I will do that then. Thanks :)

> > > This is pretty normal to have two kind of definitions (depending on the case):
> > > 1/
> > >
> > >   #define FOO_1 ...
> > >   #define FOO_2 ...
> > >
> > > and so on
> > >
> > > 2/
> > >
> > >   #define FOO(x)  (... (x) ...)
> > >
> > > Having a mix of them seems quite unusual.
> > I did not know that. I will revert this change for next version then.
>
> Don't get me wrong, it's possible to have, but since it's unusual it
> needs to be well justified. In the change you proposed you have
> changed that, but I haven't seen where the new definition is used  (in
> *.c files).

GPIO1_CONF is only used by the GPIOX_CONF macro in the header.

Best regards,
Esteban Blanc May 23, 2023, 9:26 a.m. UTC | #4
On Wed May 17, 2023 at 5:04 PM CEST, Andy Shevchenko wrote:
> On Wed, May 17, 2023 at 5:43 PM Esteban Blanc <eblanc@baylibre.com> wrote:
> > On Wed May 17, 2023 at 3:51 PM CEST, Andy Shevchenko wrote:
> > > On Wed, May 17, 2023 at 12:58 PM Esteban Blanc <eblanc@baylibre.com> wrote:
> > > > On Tue May 16, 2023 at 6:48 PM CEST, Andy Shevchenko wrote:
> > > > > On Tue, May 16, 2023 at 4:05 PM Esteban Blanc <eblanc@baylibre.com> wrote:
> > > > > > On Fri May 12, 2023 at 7:07 PM CEST,  wrote:
> > > > > > > Fri, May 12, 2023 at 04:17:54PM +0200, Esteban Blanc kirjoitti:
>
> ...
>
> > > > > > > > -#define TPS6594_REG_GPIOX_CONF(gpio_inst)          (0x31 + (gpio_inst))
> > > > > > > > +#define TPS6594_REG_GPIO1_CONF                             0x31
> > > > > > > > +#define TPS6594_REG_GPIOX_CONF(gpio_inst)  (TPS6594_REG_GPIO1_CONF + (gpio_inst))
> > > > > > >
> > > > > > > Why? The original code with parameter 0 will issue the same.
> > > > > >
> > > > > > I felt that replacing 0x31 with a constant would make the computation
> > > > > > in TPS6594_REG_GPIOX_CONFIG more understandable. What do you think?
> > > > >
> > > > > The question is why that register is so special that you need to have
> > > > > it as a constant explicitly?
> > > >
> > > > It is not special, it's just the first one of the serie of config
> > > > registers. I felt like just having 0x31 without context was a bit weird
> > >
> > > I'm not sure I understand what 'context' you are talking about.
> > I was trying to convey the fact that 0x31 was representing
> > TPS6594_REG_GPIO1_CONF address. This way when looking at
> > TPS6594_REG_GPIOX_CONF(...), one will better understand that this macro
> > is just about offsetting from the first GPIO_CONF register.
>
> You can add a comment on top of the macro, so anybody can read and see
> what this macro is doing.

> > > This is pretty normal to have two kind of definitions (depending on the case):
> > > 1/
> > >
> > >   #define FOO_1 ...
> > >   #define FOO_2 ...
> > >
> > > and so on
> > >
> > > 2/
> > >
> > >   #define FOO(x)  (... (x) ...)
> > >
> > > Having a mix of them seems quite unusual.
> > I did not know that. I will revert this change for next version then.
>
> Don't get me wrong, it's possible to have, but since it's unusual it
> needs to be well justified. In the change you proposed you have
> changed that, but I haven't seen where the new definition is used  (in
> *.c files).

Actualy it used in 2 places:
- In the switch case of `tps6594_gpio_regmap_xlate`
- In `tps6594_pinctrl_probe` when setting `reg_dir_out_base`

I already sent a v5 with this change but I managed to fail my .config
and this driver was not compiled... and it is not compiling... I feel so
stupid.

I need to send a v6 now anyway. Should I convert all
TPS6594_REG_GPIO1_CONF to TPS6594_REG_GPIOX_CONF(0)?
Andy Shevchenko May 23, 2023, 11:03 a.m. UTC | #5
On Tue, May 23, 2023 at 12:26 PM Esteban Blanc <eblanc@baylibre.com> wrote:
> On Wed May 17, 2023 at 5:04 PM CEST, Andy Shevchenko wrote:
> > On Wed, May 17, 2023 at 5:43 PM Esteban Blanc <eblanc@baylibre.com> wrote:

...

> > Don't get me wrong, it's possible to have, but since it's unusual it
> > needs to be well justified. In the change you proposed you have
> > changed that, but I haven't seen where the new definition is used  (in
> > *.c files).
>
> Actualy it used in 2 places:
> - In the switch case of `tps6594_gpio_regmap_xlate`
> - In `tps6594_pinctrl_probe` when setting `reg_dir_out_base`
>
> I already sent a v5 with this change but I managed to fail my .config
> and this driver was not compiled... and it is not compiling... I feel so
> stupid.

People are prone to making mistakes. :-)

> I need to send a v6 now anyway. Should I convert all
> TPS6594_REG_GPIO1_CONF to TPS6594_REG_GPIOX_CONF(0)?

Again, if you want to leave that definition you need to well justify
why it's so special that code needs it. Easiest way is to use the
macro with 0 as an argument.
Esteban Blanc May 23, 2023, 11:32 a.m. UTC | #6
On Tue May 23, 2023 at 1:03 PM CEST, Andy Shevchenko wrote:
> On Tue, May 23, 2023 at 12:26 PM Esteban Blanc <eblanc@baylibre.com> wrote:
> > On Wed May 17, 2023 at 5:04 PM CEST, Andy Shevchenko wrote:
> > > On Wed, May 17, 2023 at 5:43 PM Esteban Blanc <eblanc@baylibre.com> wrote:

...

> > I need to send a v6 now anyway. Should I convert all
> > TPS6594_REG_GPIO1_CONF to TPS6594_REG_GPIOX_CONF(0)?
>
> Again, if you want to leave that definition you need to well justify
> why it's so special that code needs it. Easiest way is to use the
> macro with 0 as an argument.

Ok. Thanks for your input and your patience :)

Best regards,
diff mbox series

Patch

diff --git a/drivers/pinctrl/Kconfig b/drivers/pinctrl/Kconfig
index 5787c579dcf6..dd73df978d5c 100644
--- a/drivers/pinctrl/Kconfig
+++ b/drivers/pinctrl/Kconfig
@@ -480,6 +480,37 @@  config PINCTRL_TB10X
 	depends on OF && ARC_PLAT_TB10X
 	select GPIOLIB
 
+config PINCTRL_THUNDERBAY
+	tristate "Generic pinctrl and GPIO driver for Intel Thunder Bay SoC"
+	depends on ARCH_THUNDERBAY || (ARM64 && COMPILE_TEST)
+	depends on HAS_IOMEM
+	select PINMUX
+	select PINCONF
+	select GENERIC_PINCONF
+	select GENERIC_PINCTRL_GROUPS
+	select GENERIC_PINMUX_FUNCTIONS
+	select GPIOLIB
+	select GPIOLIB_IRQCHIP
+	select GPIO_GENERIC
+	help
+	  This selects pin control driver for the Intel Thunder Bay SoC.
+	  It provides pin config functions such as pull-up, pull-down,
+	  interrupt, drive strength, sec lock, Schmitt trigger, slew
+	  rate control and direction control. This module will be
+	  called as pinctrl-thunderbay.
+
+config PINCTRL_TPS6594
+	tristate "Pinctrl and GPIO driver for TI TPS6594 PMIC"
+	depends on MFD_TPS6594
+	default MFD_TPS6594
+	select PINMUX
+	select GPIOLIB
+	select REGMAP
+	select GPIO_REGMAP
+	help
+	  This driver supports GPIOs and pinmuxing for the TPS6594
+	  PMICs chip family.
+
 config PINCTRL_ZYNQ
 	bool "Pinctrl driver for Xilinx Zynq"
 	depends on ARCH_ZYNQ
diff --git a/drivers/pinctrl/Makefile b/drivers/pinctrl/Makefile
index e196c6e324ad..a48cbf4e6126 100644
--- a/drivers/pinctrl/Makefile
+++ b/drivers/pinctrl/Makefile
@@ -49,6 +49,8 @@  obj-$(CONFIG_PINCTRL_ST) 	+= pinctrl-st.o
 obj-$(CONFIG_PINCTRL_STMFX) 	+= pinctrl-stmfx.o
 obj-$(CONFIG_PINCTRL_SX150X)	+= pinctrl-sx150x.o
 obj-$(CONFIG_PINCTRL_TB10X)	+= pinctrl-tb10x.o
+obj-$(CONFIG_PINCTRL_THUNDERBAY) += pinctrl-thunderbay.o
+obj-$(CONFIG_PINCTRL_TPS6594)	+= pinctrl-tps6594.o
 obj-$(CONFIG_PINCTRL_ZYNQMP)	+= pinctrl-zynqmp.o
 obj-$(CONFIG_PINCTRL_ZYNQ)	+= pinctrl-zynq.o
 
diff --git a/drivers/pinctrl/pinctrl-tps6594.c b/drivers/pinctrl/pinctrl-tps6594.c
new file mode 100644
index 000000000000..322e83ce8f6e
--- /dev/null
+++ b/drivers/pinctrl/pinctrl-tps6594.c
@@ -0,0 +1,301 @@ 
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Pinmux and GPIO driver for tps6594 PMIC
+ *
+ * Copyright (C) 2022 BayLibre Incorporated - https://www.baylibre.com/
+ */
+
+#include <linux/gpio/regmap.h>
+#include <linux/gpio/driver.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/pinctrl/pinmux.h>
+
+#include <linux/mfd/tps6594.h>
+
+#define TPS6594_PINCTRL_PINS_NB 11
+
+#define TPS6594_PINCTRL_GPIO_FUNCTION 0
+#define TPS6594_PINCTRL_SCL_I2C2_CS_SPI_FUNCTION 1
+#define TPS6594_PINCTRL_TRIG_WDOG_FUNCTION 1
+#define TPS6594_PINCTRL_CLK32KOUT_FUNCTION 1
+#define TPS6594_PINCTRL_SCLK_SPMI_FUNCTION 1
+#define TPS6594_PINCTRL_SDATA_SPMI_FUNCTION 1
+#define TPS6594_PINCTRL_NERR_MCU_FUNCTION 1
+#define TPS6594_PINCTRL_PDOG_FUNCTION 1
+#define TPS6594_PINCTRL_SYNCCLKIN_FUNCTION 1
+#define TPS6594_PINCTRL_NRSTOUT_SOC_FUNCTION 2
+#define TPS6594_PINCTRL_SYNCCLKOUT_FUNCTION 2
+#define TPS6594_PINCTRL_SDA_I2C2_SDO_SPI_FUNCTION 2
+#define TPS6594_PINCTRL_NERR_SOC_FUNCTION 2
+#define TPS6594_PINCTRL_DISABLE_WDOG_FUNCTION 3
+#define TPS6594_PINCTRL_NSLEEP1_FUNCTION 4
+#define TPS6594_PINCTRL_NSLEEP2_FUNCTION 5
+#define TPS6594_PINCTRL_WKUP1_FUNCTION 6
+#define TPS6594_PINCTRL_WKUP2_FUNCTION 7
+
+/* Special muxval for recalcitrant pins */
+#define TPS6594_PINCTRL_DISABLE_WDOG_FUNCTION_GPIO8 2
+#define TPS6594_PINCTRL_SYNCCLKOUT_FUNCTION_GPIO8 3
+#define TPS6594_PINCTRL_CLK32KOUT_FUNCTION_GPIO9 3
+
+#define TPS6594_OFFSET_GPIO_SEL 5
+
+static const struct pinctrl_pin_desc tps6594_pins[TPS6594_PINCTRL_PINS_NB] = {
+	PINCTRL_PIN(0, "GPIO0"),   PINCTRL_PIN(1, "GPIO1"),
+	PINCTRL_PIN(2, "GPIO2"),   PINCTRL_PIN(3, "GPIO3"),
+	PINCTRL_PIN(4, "GPIO4"),   PINCTRL_PIN(5, "GPIO5"),
+	PINCTRL_PIN(6, "GPIO6"),   PINCTRL_PIN(7, "GPIO7"),
+	PINCTRL_PIN(8, "GPIO8"),   PINCTRL_PIN(9, "GPIO9"),
+	PINCTRL_PIN(10, "GPIO10"),
+};
+
+static const char *groups_name[TPS6594_PINCTRL_PINS_NB] = {
+	"GPIO0", "GPIO1", "GPIO2", "GPIO3", "GPIO4", "GPIO5",
+	"GPIO6", "GPIO7", "GPIO8", "GPIO9", "GPIO10"
+};
+
+struct tps6594_pinctrl_function {
+	const char *name;
+	u8 muxval;
+	const char **groups;
+	unsigned long ngroups;
+};
+
+static const struct tps6594_pinctrl_function pinctrl_functions[] = {
+	{ "gpio", TPS6594_PINCTRL_GPIO_FUNCTION, groups_name,
+	  TPS6594_PINCTRL_PINS_NB },
+	{ "nsleep1", TPS6594_PINCTRL_NSLEEP1_FUNCTION, groups_name,
+	  TPS6594_PINCTRL_PINS_NB },
+	{ "nsleep2", TPS6594_PINCTRL_NSLEEP2_FUNCTION, groups_name,
+	  TPS6594_PINCTRL_PINS_NB },
+	{ "wkup1", TPS6594_PINCTRL_WKUP1_FUNCTION, groups_name,
+	  TPS6594_PINCTRL_PINS_NB },
+	{ "wkup2", TPS6594_PINCTRL_WKUP2_FUNCTION, groups_name,
+	  TPS6594_PINCTRL_PINS_NB },
+	{ "scl_i2c2-cs_spi", TPS6594_PINCTRL_SCL_I2C2_CS_SPI_FUNCTION,
+	  (const char *[]){ "GPIO0", "GPIO1" }, 2 },
+	{ "nrstout_soc", TPS6594_PINCTRL_NRSTOUT_SOC_FUNCTION,
+	  (const char *[]){ "GPIO0", "GPIO10" }, 2 },
+	{ "trig_wdog", TPS6594_PINCTRL_TRIG_WDOG_FUNCTION,
+	  (const char *[]){ "GPIO1", "GPIO10" }, 2 },
+	{ "sda_i2c2-sdo_spi", TPS6594_PINCTRL_SDA_I2C2_SDO_SPI_FUNCTION,
+	  (const char *[]){ "GPIO1" }, 1 },
+	{ "clk32kout", TPS6594_PINCTRL_CLK32KOUT_FUNCTION,
+	  (const char *[]){ "GPIO2", "GPIO3", "GPIO7" }, 3 },
+	{ "nerr_soc", TPS6594_PINCTRL_NERR_SOC_FUNCTION,
+	  (const char *[]){ "GPIO2" }, 1 },
+	{ "sclk_spmi", TPS6594_PINCTRL_SCLK_SPMI_FUNCTION,
+	  (const char *[]){ "GPIO4" }, 1 },
+	{ "sdata_spmi", TPS6594_PINCTRL_SDATA_SPMI_FUNCTION,
+	  (const char *[]){ "GPIO5" }, 1 },
+	{ "nerr_mcu", TPS6594_PINCTRL_NERR_MCU_FUNCTION,
+	  (const char *[]){ "GPIO6" }, 1 },
+	{ "syncclkout", TPS6594_PINCTRL_SYNCCLKOUT_FUNCTION,
+	  (const char *[]){ "GPIO7", "GPIO9" }, 2 },
+	{ "disable_wdog", TPS6594_PINCTRL_DISABLE_WDOG_FUNCTION,
+	  (const char *[]){ "GPIO7", "GPIO8" }, 2 },
+	{ "pdog", TPS6594_PINCTRL_PDOG_FUNCTION, (const char *[]){ "GPIO8" },
+	  1 },
+	{ "syncclkin", TPS6594_PINCTRL_SYNCCLKIN_FUNCTION,
+	  (const char *[]){ "GPIO9" }, 1 },
+};
+
+struct tps6594_pinctrl {
+	struct tps6594 *tps;
+	struct gpio_regmap *gpio_regmap;
+	struct pinctrl_dev *pctl_dev;
+	const struct tps6594_pinctrl_function *funcs;
+	const struct pinctrl_pin_desc *pins;
+};
+
+static int tps6594_gpio_regmap_xlate(struct gpio_regmap *gpio,
+				     unsigned int base, unsigned int offset,
+				     unsigned int *reg, unsigned int *mask)
+{
+	unsigned int line = offset % 8;
+	unsigned int stride = offset / 8;
+
+	switch (base) {
+	case TPS6594_REG_GPIO1_CONF:
+		*reg = TPS6594_REG_GPIOX_CONF(offset);
+		*mask = TPS6594_BIT_GPIO_DIR;
+		break;
+	case TPS6594_REG_GPIO_IN_1:
+	case TPS6594_REG_GPIO_OUT_1:
+		*reg = base + stride;
+		*mask = BIT(line);
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+static int tps6594_pmx_func_cnt(struct pinctrl_dev *pctldev)
+{
+	return ARRAY_SIZE(pinctrl_functions);
+}
+
+static const char *tps6594_pmx_func_name(struct pinctrl_dev *pctldev,
+					 unsigned int selector)
+{
+	struct tps6594_pinctrl *pinctrl = pinctrl_dev_get_drvdata(pctldev);
+
+	return pinctrl->funcs[selector].name;
+}
+
+static int tps6594_pmx_func_groups(struct pinctrl_dev *pctldev,
+				   unsigned int selector,
+				   const char *const **groups,
+				   unsigned int *num_groups)
+{
+	struct tps6594_pinctrl *pinctrl = pinctrl_dev_get_drvdata(pctldev);
+
+	*groups = pinctrl->funcs[selector].groups;
+	*num_groups = pinctrl->funcs[selector].ngroups;
+
+	return 0;
+}
+
+static int tps6594_pmx_set(struct tps6594_pinctrl *pinctrl, unsigned int pin,
+			   u8 muxval)
+{
+	u8 mux_sel_val = muxval << TPS6594_OFFSET_GPIO_SEL;
+
+	return regmap_update_bits(pinctrl->tps->regmap,
+				  TPS6594_REG_GPIOX_CONF(pin),
+				  TPS6594_MASK_GPIO_SEL, mux_sel_val);
+}
+
+static int tps6594_pmx_set_mux(struct pinctrl_dev *pctldev,
+			       unsigned int function, unsigned int group)
+{
+	struct tps6594_pinctrl *pinctrl = pinctrl_dev_get_drvdata(pctldev);
+	u8 muxval = pinctrl->funcs[function].muxval;
+
+	/* Some pins don't have the same muxval for the same function... */
+	if (group == 8) {
+		if (muxval == TPS6594_PINCTRL_DISABLE_WDOG_FUNCTION)
+			muxval = TPS6594_PINCTRL_DISABLE_WDOG_FUNCTION_GPIO8;
+		else if (muxval == TPS6594_PINCTRL_SYNCCLKOUT_FUNCTION)
+			muxval = TPS6594_PINCTRL_SYNCCLKOUT_FUNCTION_GPIO8;
+	} else if (group == 9) {
+		if (muxval == TPS6594_PINCTRL_CLK32KOUT_FUNCTION)
+			muxval = TPS6594_PINCTRL_CLK32KOUT_FUNCTION_GPIO9;
+	}
+
+	return tps6594_pmx_set(pinctrl, group, muxval);
+}
+
+static int tps6594_pmx_gpio_set_direction(struct pinctrl_dev *pctldev,
+					  struct pinctrl_gpio_range *range,
+					  unsigned int offset, bool input)
+{
+	struct tps6594_pinctrl *pinctrl = pinctrl_dev_get_drvdata(pctldev);
+	u8 muxval = pinctrl->funcs[TPS6594_PINCTRL_GPIO_FUNCTION].muxval;
+
+	return tps6594_pmx_set(pinctrl, offset, muxval);
+}
+
+static const struct pinmux_ops tps6594_pmx_ops = {
+	.get_functions_count = tps6594_pmx_func_cnt,
+	.get_function_name = tps6594_pmx_func_name,
+	.get_function_groups = tps6594_pmx_func_groups,
+	.set_mux = tps6594_pmx_set_mux,
+	.gpio_set_direction = tps6594_pmx_gpio_set_direction,
+	.strict = true,
+};
+
+static int tps6594_groups_cnt(struct pinctrl_dev *pctldev)
+{
+	return ARRAY_SIZE(tps6594_pins);
+}
+
+static int tps6594_group_pins(struct pinctrl_dev *pctldev,
+			      unsigned int selector, const unsigned int **pins,
+			      unsigned int *num_pins)
+{
+	struct tps6594_pinctrl *pinctrl = pinctrl_dev_get_drvdata(pctldev);
+
+	*pins = (unsigned int *)&pinctrl->pins[selector];
+	*num_pins = 1;
+
+	return 0;
+}
+
+static const char *tps6594_group_name(struct pinctrl_dev *pctldev,
+				      unsigned int selector)
+{
+	struct tps6594_pinctrl *pinctrl = pinctrl_dev_get_drvdata(pctldev);
+
+	return pinctrl->pins[selector].name;
+}
+
+static const struct pinctrl_ops tps6594_pctrl_ops = {
+	.dt_node_to_map = pinconf_generic_dt_node_to_map_group,
+	.dt_free_map = pinconf_generic_dt_free_map,
+	.get_groups_count = tps6594_groups_cnt,
+	.get_group_name = tps6594_group_name,
+	.get_group_pins = tps6594_group_pins,
+};
+
+static int tps6594_pinctrl_probe(struct platform_device *pdev)
+{
+	struct tps6594 *tps = dev_get_drvdata(pdev->dev.parent);
+	struct tps6594_pinctrl *pinctrl;
+	struct pinctrl_desc *pctrl_desc;
+	struct gpio_regmap_config config = {};
+
+	pctrl_desc = devm_kzalloc(&pdev->dev, sizeof(*pctrl_desc), GFP_KERNEL);
+	if (!pctrl_desc)
+		return -ENOMEM;
+	pctrl_desc->name = dev_name(&pdev->dev);
+	pctrl_desc->owner = THIS_MODULE;
+	pctrl_desc->pins = tps6594_pins;
+	pctrl_desc->npins = ARRAY_SIZE(tps6594_pins);
+	pctrl_desc->pctlops = &tps6594_pctrl_ops;
+	pctrl_desc->pmxops = &tps6594_pmx_ops;
+
+	pinctrl = devm_kzalloc(&pdev->dev, sizeof(*pinctrl), GFP_KERNEL);
+	if (!pinctrl)
+		return -ENOMEM;
+	pinctrl->tps = dev_get_drvdata(pdev->dev.parent);
+	pinctrl->funcs = pinctrl_functions;
+	pinctrl->pins = tps6594_pins;
+	pinctrl->pctl_dev =
+		devm_pinctrl_register(&pdev->dev, pctrl_desc, pinctrl);
+	if (IS_ERR(pinctrl->pctl_dev)) {
+		dev_err(&pdev->dev, "Couldn't register pinctrl driver\n");
+		return PTR_ERR(pinctrl->pctl_dev);
+	}
+
+	config.parent = tps->dev;
+	config.regmap = tps->regmap;
+	config.ngpio = TPS6594_PINCTRL_PINS_NB;
+	config.ngpio_per_reg = 8;
+	config.reg_dat_base = TPS6594_REG_GPIO_IN_1;
+	config.reg_set_base = TPS6594_REG_GPIO_OUT_1;
+	config.reg_dir_out_base = TPS6594_REG_GPIO1_CONF;
+	config.reg_mask_xlate = tps6594_gpio_regmap_xlate;
+
+	pinctrl->gpio_regmap = devm_gpio_regmap_register(&pdev->dev, &config);
+	if (IS_ERR(pinctrl->gpio_regmap)) {
+		dev_err(&pdev->dev, "Couldn't register gpio_regmap driver\n");
+		return PTR_ERR(pinctrl->pctl_dev);
+	}
+
+	return 0;
+}
+
+static struct platform_driver tps6594_pinctrl_driver = {
+	.driver = { .name = "tps6594-pinctrl" },
+	.probe = tps6594_pinctrl_probe,
+};
+module_platform_driver(tps6594_pinctrl_driver);
+
+MODULE_ALIAS("platform:tps6594-pinctrl");
+MODULE_AUTHOR("Esteban Blanc <eblanc@baylibre.com>");
+MODULE_DESCRIPTION("TPS6594 pinctrl and GPIO driver");
+MODULE_LICENSE("GPL");
diff --git a/include/linux/mfd/tps6594.h b/include/linux/mfd/tps6594.h
index 3f7c5e23cd4c..93da782ed2b7 100644
--- a/include/linux/mfd/tps6594.h
+++ b/include/linux/mfd/tps6594.h
@@ -47,7 +47,8 @@  enum pmic_id {
 #define TPS6594_REG_VMON2_PG_WINDOW			0x2f
 #define TPS6594_REG_VMON2_PG_LEVEL			0x30
 
-#define TPS6594_REG_GPIOX_CONF(gpio_inst)		(0x31 + (gpio_inst))
+#define TPS6594_REG_GPIO1_CONF				0x31
+#define TPS6594_REG_GPIOX_CONF(gpio_inst)	(TPS6594_REG_GPIO1_CONF + (gpio_inst))
 #define TPS6594_REG_NPWRON_CONF				0x3c
 #define TPS6594_REG_GPIO_OUT_1				0x3d
 #define TPS6594_REG_GPIO_OUT_2				0x3e