Re: [PATCH v2 13/14] pinctrl: pinctrl-tps6594: Add TPS65224 PMIC pinctrl and GPIO

["Esteban Blanc" <eblanc@xxxxxxxxxxxx>]

On Fri Feb 23, 2024 at 10:37 AM CET, Bhargav Raviprakash wrote:
> From: Nirmala Devi Mal Nadar <m.nirmaladevi@xxxxxxxx>
>
> Add support for TPS65224 pinctrl and GPIOs to TPS6594 driver as they
> have significant functional overlap.
> TPS65224 PMIC has 6 GPIOS which can be configured as GPIO or other
> dedicated device functions.
>
> Signed-off-by: Nirmala Devi Mal Nadar <m.nirmaladevi@xxxxxxxx>
> Signed-off-by: Bhargav Raviprakash <bhargav.r@xxxxxxxx>
> ---
>  drivers/pinctrl/pinctrl-tps6594.c | 287 +++++++++++++++++++++++++-----
>  1 file changed, 246 insertions(+), 41 deletions(-)
>
> diff --git a/drivers/pinctrl/pinctrl-tps6594.c b/drivers/pinctrl/pinctrl-tps6594.c
> index 66985e54b..5da21aa14 100644
> --- a/drivers/pinctrl/pinctrl-tps6594.c
> +++ b/drivers/pinctrl/pinctrl-tps6594.c

> @@ -201,7 +319,21 @@ static int tps6594_gpio_regmap_xlate(struct gpio_regmap *gpio,
>  
>  static int tps6594_pmx_func_cnt(struct pinctrl_dev *pctldev)
>  {
> -	return ARRAY_SIZE(pinctrl_functions);
> +	struct tps6594_pinctrl *pinctrl = pinctrl_dev_get_drvdata(pctldev);
> +	int func_cnt = 0;
> +
> +	switch (pinctrl->tps->chip_id) {

See below.

> @@ -229,10 +361,26 @@ static int tps6594_pmx_set(struct tps6594_pinctrl *pinctrl, unsigned int pin,
>  			   u8 muxval)
>  {
>  	u8 mux_sel_val = muxval << TPS6594_OFFSET_GPIO_SEL;
> +	u8 mux_sel_mask = 0;
> +
> +	switch (pinctrl->tps->chip_id) {

See below.

> @@ -240,16 +388,28 @@ static int tps6594_pmx_set_mux(struct pinctrl_dev *pctldev,
>  {
>  	struct tps6594_pinctrl *pinctrl = pinctrl_dev_get_drvdata(pctldev);
>  	u8 muxval = pinctrl->funcs[function].muxval;
> +	unsigned int remap_cnt = 0;
> +	struct muxval_remap *remap;
>  
>  	/* 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;
> +	switch (pinctrl->tps->chip_id) {

See below.

> @@ -276,7 +436,21 @@ static const struct pinmux_ops tps6594_pmx_ops = {
>  
>  static int tps6594_groups_cnt(struct pinctrl_dev *pctldev)
>  {
> -	return ARRAY_SIZE(tps6594_pins);
> +	struct tps6594_pinctrl *pinctrl = pinctrl_dev_get_drvdata(pctldev);
> +	int num_pins = 0;
> +
> +	switch (pinctrl->tps->chip_id) {

See below.

> @@ -320,8 +494,18 @@ static int tps6594_pinctrl_probe(struct platform_device *pdev)
>  		return -ENOMEM;
>  	pctrl_desc->name = dev_name(dev);
>  	pctrl_desc->owner = THIS_MODULE;
> -	pctrl_desc->pins = tps6594_pins;
> -	pctrl_desc->npins = ARRAY_SIZE(tps6594_pins);
> +	switch (tps->chip_id) {

See below.

> @@ -329,8 +513,18 @@ static int tps6594_pinctrl_probe(struct platform_device *pdev)
>  	if (!pinctrl)
>  		return -ENOMEM;
>  	pinctrl->tps = dev_get_drvdata(dev->parent);
> -	pinctrl->funcs = pinctrl_functions;
> -	pinctrl->pins = tps6594_pins;
> +	switch (pinctrl->tps->chip_id) {

See below.

> @@ -338,8 +532,18 @@ static int tps6594_pinctrl_probe(struct platform_device *pdev)
>  
>  	config.parent = tps->dev;
>  	config.regmap = tps->regmap;
> -	config.ngpio = TPS6594_PINCTRL_PINS_NB;
> -	config.ngpio_per_reg = 8;
> +	switch (pinctrl->tps->chip_id) {

Regarding all the switch case, I think you should try and put all the
differences inside the `struct tps6594_pinctrl`. This way most of the
functions (if not all of them) could be writen without the switch case,
making them more readable and straight forward to understand.
You already have some switch case in the probe, why not fill the `struct
tps6594_pintcl` there and use these new fileds in the different
function.

It's not pretty today, imagine if in the future there is more supported
chip, it would be quite unreadable IMAO.

Other than that the changes looks fine to me. I will have to boot a
board with TPS6594 to check that whole series did not break anything.

Please add me to your Cc for the next round.

Best regards,

-- 
Esteban "Skallwar" Blanc
BayLibre