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
BayLibre
