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

[Esteban Blanc]

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