Re: [PATCH v4 10/11] pinctrl: pinctrl-tps6594: Add TPS65224 PMIC pinctrl and GPIO

[Bhargav Raviprakash <bhargav.r@xxxxxxxx>]

Hi,

On Fri, 22 Mar 2024 17:03:08 +0100, Esteban Blanc wrote:
> On Wed Mar 20, 2024 at 11:25 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>
> > Acked-by: Linus Walleij <linus.walleij@xxxxxxxxxx>
> > ---
> >  drivers/pinctrl/pinctrl-tps6594.c | 258 +++++++++++++++++++++++++-----
> >  1 file changed, 215 insertions(+), 43 deletions(-)
> >
> > diff --git a/drivers/pinctrl/pinctrl-tps6594.c b/drivers/pinctrl/pinctrl-tps6594.c
> > index 66985e54b..db0f5d2a8 100644
> > --- a/drivers/pinctrl/pinctrl-tps6594.c
> > +++ b/drivers/pinctrl/pinctrl-tps6594.c
> > @@ -320,8 +451,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) {
> > +	case TPS65224:
> > +		pctrl_desc->pins = tps65224_pins;
> > +		pctrl_desc->npins = ARRAY_SIZE(tps65224_pins);
> > +		break;
> > +	case TPS6594:
> > +		pctrl_desc->pins = tps6594_pins;
> > +		pctrl_desc->npins = ARRAY_SIZE(tps6594_pins);
> > +		break;
> > +	default:
> > +		break;
> > +	}
> >  	pctrl_desc->pctlops = &tps6594_pctrl_ops;
> >  	pctrl_desc->pmxops = &tps6594_pmx_ops;
> 
> See below.
> 
> > @@ -329,8 +470,28 @@ 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) {
> 
> You could use tps->chip_id like in the previous switch.
> 
> > +	case TPS65224:
> > +		pinctrl->funcs = tps65224_pinctrl_functions;
> > +		pinctrl->func_cnt = ARRAY_SIZE(tps65224_pinctrl_functions);
> > +		pinctrl->pins = tps65224_pins;
> > +		pinctrl->num_pins = ARRAY_SIZE(tps65224_pins);
> > +		pinctrl->mux_sel_mask = TPS65224_MASK_GPIO_SEL;
> > +		pinctrl->remap = tps65224_muxval_remap;
> > +		pinctrl->remap_cnt = ARRAY_SIZE(tps65224_muxval_remap);
> > +		break;
> > +	case TPS6594:
> > +		pinctrl->funcs = pinctrl_functions;
> 
> This should be tps6594_pinctrl_functions
> 
> > +		pinctrl->func_cnt = ARRAY_SIZE(pinctrl_functions);
> > +		pinctrl->pins = tps6594_pins;
> > +		pinctrl->num_pins = ARRAY_SIZE(tps6594_pins);
> > +		pinctrl->mux_sel_mask = TPS6594_MASK_GPIO_SEL;
> > +		pinctrl->remap = tps6594_muxval_remap;
> > +		pinctrl->remap_cnt = ARRAY_SIZE(tps6594_muxval_remap);
> > +		break;
> > +	default:
> > +		break;
> > +	}
> 
> See blow.
> 
> >  	pinctrl->pctl_dev = devm_pinctrl_register(dev, pctrl_desc, pinctrl);
> >  	if (IS_ERR(pinctrl->pctl_dev))
> >  		return dev_err_probe(dev, PTR_ERR(pinctrl->pctl_dev),
> > @@ -338,8 +499,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) {
> 
> Same here, use tps->chip_id
> 
Sure, will do!
> > +	case TPS65224:
> > +		config.ngpio = ARRAY_SIZE(tps65224_gpio_func_group_names);
> > +		config.ngpio_per_reg = TPS65224_NGPIO_PER_REG;
> > +		break;
> > +	case TPS6594:
> > +		config.ngpio = ARRAY_SIZE(tps6594_gpio_func_group_names);
> > +		config.ngpio_per_reg = TPS6594_NGPIO_PER_REG;
> > +		break;
> > +	default:
> > +		break;
> > +	}
> >  	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_GPIOX_CONF(0);
> 
> Regarding all the switch case, they should be use to set all the struct
> fields that are known at runtime only. For example, pinctrl->funcs, and
> pinctrl->func_cnt are known at compile time. You should create template
> structs, one for TPS6594 the other TPS65224, initialise the allocated
> struct with the template and then fill the remaining fields with the
> runtime values. Something like this:
> 
> ```c
> struct test {
>     int a;
>     int *b;
> };
> 
> static struct test template = {
>     .a = 42,
> };
> 
> int main(void) {
>     struct test *test = malloc(sizeof(*test));
>     *test = sample;
>     test->b = NULL;
> 
>     return 0;
> }
> ```
> 
> You could also try to reduce the number of switch case, there is no good
> reason to have 2 switch instead of one for pctrl_desc and pinctrl
> structs.
> 
> Best regards,
> 
> -- 
> Esteban "Skallwar" Blanc
> BayLibre

Thank you for bringing these issues to our attention.
We will follow the template struct way as suggested and also try to reduce the number of switch
cases. These changes will be available in the next version.

Regards,
Bhargav