| Message ID | 20230512141755.1712358-3-eblanc@baylibre.com |
|---|---|
| State | Handled Elsewhere |
| 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 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: ... > > > +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. Yes. ... > > > + 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. As you wish. ... > > > -#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?
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 Best regards,
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. 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.
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. > 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. Thanks again for your time. Best regards,
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).
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