| Message ID | 20230512141755.1712358-3-eblanc@baylibre.com |
|---|---|
| State | New |
| Headers | show |
| Series | TI TPS6594 PMIC support (RTC, pinctrl, regulators) | expand |
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.
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,
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,
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)?
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.
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 --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
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