mbox series

[v4,00/11] Add support for TI TPS65224 PMIC

Message ID 20240320102559.464981-1-bhargav.r@ltts.com
Headers show
Series Add support for TI TPS65224 PMIC | expand

Message

Bhargav Raviprakash March 20, 2024, 10:25 a.m. UTC 
This series modifies the existing TPS6594 drivers to add support for the
TPS65224 PMIC device that is a derivative of TPS6594. TPS65224 has a
similar register map to TPS6594 with a few differences. SPI, I2C, ESM,
PFSM, Regulators and GPIO features overlap between the two devices.

TPS65224 is a Power Management IC (PMIC) which provides regulators and
other features like GPIOs, Watchdog, Error Signal Monitor (ESM) and
Pre-configurable Finite State Machine (PFSM). The SoC and the PMIC can
communicate through the I2C or SPI interfaces. The PMIC TPS65224
additionally has a 12-bit ADC.
Data Sheet for TPS65224: https://www.ti.com/product/TPS65224-Q1

Driver re-use is applied following the advice of the following series:
https://lore.kernel.org/lkml/2f467b0a-1d11-4ec7-8ca6-6c4ba66e5887@baylibre.com/

The features implemented in this series are:
- TPS65224 Register definitions
- Core (MFD I2C and SPI entry points)
- PFSM	
- Regulators
- Pinctrl

TPS65224 Register definitions:
This patch adds macros for register field definitions of TPS65224
to the existing TPS6594 driver.  

Core description:
I2C and SPI interface protocols are implemented, with and without
the bit-integrity error detection feature (CRC mode).

PFSM description:
Strictly speaking, PFSM is not hardware. It is a piece of code.
PMIC integrates a state machine which manages operational modes.
Depending on the current operational mode, some voltage domains
remain energized while others can be off.
PFSM driver can be used to trigger transitions between configured
states.

Regulators description:
4 BUCKs and 3 LDOs.
BUCK12 can be used in dual-phase mode.

Pinctrl description:
TPS65224 family has 6 GPIOs. Those GPIOs can also serve different
functions such as I2C or SPI interface or watchdog disable functions.
The driver provides both pinmuxing for the functions and GPIO capability.

This series was tested on linux-next tag: next-20240118

Test logs can be found here:
https://gist.github.com/LeonardMH/58ec135921fb1062ffd4a8b384831eb0

Changelog v3 -> v4:
- Removed ESM MCU interrupts from tps6594.h
- Fixing indentation and minor refactoring 
  in regulator
- Regarding switch cases in pinctrl, added fields in pinctrl struct and
  using those to differentiate between chips.


Bhargav Raviprakash (8):
  mfd: tps6594: use volatile_table instead of volatile_reg
  mfd: tps6594: add regmap config in match data
  dt-bindings: mfd: ti,tps6594: Add TI TPS65224 PMIC
  mfd: tps6594-i2c: Add TI TPS65224 PMIC I2C
  mfd: tps6594-spi: Add TI TPS65224 PMIC SPI
  mfd: tps6594-core: Add TI TPS65224 PMIC core
  misc: tps6594-pfsm: Add TI TPS65224 PMIC PFSM
  arch: arm64: dts: ti: k3-am62p5-sk: Add TPS65224 PMIC support in AM62P
    dts

Nirmala Devi Mal Nadar (3):
  mfd: tps6594: Add register definitions for TI TPS65224 PMIC
  regulator: tps6594-regulator: Add TI TPS65224 PMIC regulators
  pinctrl: pinctrl-tps6594: Add TPS65224 PMIC pinctrl and GPIO

 .../devicetree/bindings/mfd/ti,tps6594.yaml   |   1 +
 arch/arm64/boot/dts/ti/k3-am62p5-sk.dts       |  95 +++++
 drivers/mfd/tps6594-core.c                    | 253 ++++++++++--
 drivers/mfd/tps6594-i2c.c                     |  41 +-
 drivers/mfd/tps6594-spi.c                     |  43 ++-
 drivers/misc/tps6594-pfsm.c                   |  48 ++-
 drivers/pinctrl/pinctrl-tps6594.c             | 258 ++++++++++---
 drivers/regulator/Kconfig                     |   4 +-
 drivers/regulator/tps6594-regulator.c         | 238 ++++++++++--
 include/linux/mfd/tps6594.h                   | 362 +++++++++++++++++-
 10 files changed, 1202 insertions(+), 141 deletions(-)


base-commit: 2863b714f3ad0a9686f2de1b779228ad8c7a8052

Comments

Mark Brown March 20, 2024, 4:38 p.m. UTC | #1 
On Wed, Mar 20, 2024 at 03:55:57PM +0530, Bhargav Raviprakash wrote:

> +static struct tps6594_regulator_irq_type tps65224_buck1_irq_types[] = {
> +	{ TPS65224_IRQ_NAME_BUCK1_UVOV, "BUCK1", "voltage out of range",
> +	  REGULATOR_EVENT_OVER_VOLTAGE_WARN },
> +};

These all look like they should be _REGULATION_OUT given that the
interrupt names are _UVOV which look like they could be either under or
over voltage.

Otherwise this all looks good.
Julien Panis March 21, 2024, 11:10 a.m. UTC | #2 
On 3/20/24 11:25, Bhargav Raviprakash wrote:
> From: Nirmala Devi Mal Nadar <m.nirmaladevi@ltts.com>
>
> 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@ltts.com>
> Signed-off-by: Bhargav Raviprakash <bhargav.r@ltts.com>
> Acked-by: Linus Walleij <linus.walleij@linaro.org>

With this patch, an issue is observed on am62a:

root@am62axx-evm:~# dmesg | grep tps
...
[   12.122631] tps6594-pinctrl tps6594-pinctrl.2.auto: error -EINVAL: Couldn't register gpio_regmap 
driver
[   12.133216] tps6594-pinctrl: probe of tps6594-pinctrl.2.auto failed with error -22

Without this patch, the issue disappears. Do you observe
the same result with your am62p ?

Julien Panis
Esteban Blanc March 22, 2024, 4:03 p.m. UTC | #3 
On Wed Mar 20, 2024 at 11:25 AM CET, Bhargav Raviprakash wrote:
> From: Nirmala Devi Mal Nadar <m.nirmaladevi@ltts.com>
>
> 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@ltts.com>
> Signed-off-by: Bhargav Raviprakash <bhargav.r@ltts.com>
> Acked-by: Linus Walleij <linus.walleij@linaro.org>
> ---
>  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

> +	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,
Bhargav Raviprakash March 28, 2024, 10:16 a.m. UTC | #4 
Hi,

On Wed, 20 Mar 2024 16:38:20 +0000, Mark Brown wrote:
> On Wed, Mar 20, 2024 at 03:55:57PM +0530, Bhargav Raviprakash wrote:
> 
> > +static struct tps6594_regulator_irq_type tps65224_buck1_irq_types[] = {
> > +	{ TPS65224_IRQ_NAME_BUCK1_UVOV, "BUCK1", "voltage out of range",
> > +	  REGULATOR_EVENT_OVER_VOLTAGE_WARN },
> > +};
> 
> These all look like they should be _REGULATION_OUT given that the
> interrupt names are _UVOV which look like they could be either under or
> over voltage.
> 
> Otherwise this all looks good.

Thanks for the feedback! We will fix it in the next version.

Regards,
Bhargav
Bhargav Raviprakash March 28, 2024, 10:27 a.m. UTC | #5 
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@ltts.com>
> >
> > 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@ltts.com>
> > Signed-off-by: Bhargav Raviprakash <bhargav.r@ltts.com>
> > Acked-by: Linus Walleij <linus.walleij@linaro.org>
> > ---
> >  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