| Message ID | 20240408124047.191895-1-bhargav.r@ltts.com |
|---|---|
| Headers | show |
| Series | Add support for TI TPS65224 PMIC | expand |
On Mon, Apr 08, 2024 at 06:10:44PM +0530, Bhargav Raviprakash wrote: > Add support for TPS65224 PFSM in the TPS6594 PFSM driver as they share > significant functionality. > > Signed-off-by: Bhargav Raviprakash <bhargav.r@ltts.com> > Acked-by: Julien Panis <jpanis@baylibre.com> > --- > drivers/misc/tps6594-pfsm.c | 48 +++++++++++++++++++++++++++---------- > 1 file changed, 35 insertions(+), 13 deletions(-) > Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
On Mon, 08 Apr 2024, Bhargav Raviprakash wrote: > Introduces a new struct tps6594_match_data. This struct holds fields for > chip id and regmap config. Using this struct in of_device_id data field. > This helps in adding support for TPS65224 PMIC. > > Signed-off-by: Bhargav Raviprakash <bhargav.r@ltts.com> > Acked-by: Julien Panis <jpanis@baylibre.com> > --- > drivers/mfd/tps6594-i2c.c | 24 ++++++++++++++++-------- > drivers/mfd/tps6594-spi.c | 24 ++++++++++++++++-------- > include/linux/mfd/tps6594.h | 11 +++++++++++ > 3 files changed, 43 insertions(+), 16 deletions(-) > > diff --git a/drivers/mfd/tps6594-i2c.c b/drivers/mfd/tps6594-i2c.c > index c125b474b..9e2ed48b7 100644 > --- a/drivers/mfd/tps6594-i2c.c > +++ b/drivers/mfd/tps6594-i2c.c > @@ -192,10 +192,16 @@ static const struct regmap_config tps6594_i2c_regmap_config = { > .write = tps6594_i2c_write, > }; > > +static const struct tps6594_match_data match_data[] = { > + [TPS6594] = {TPS6594, &tps6594_i2c_regmap_config}, > + [TPS6593] = {TPS6593, &tps6594_i2c_regmap_config}, > + [LP8764] = {LP8764, &tps6594_i2c_regmap_config}, Nit: There should be spaces after the '{' and before the '}'. > +}; > + > static const struct of_device_id tps6594_i2c_of_match_table[] = { > - { .compatible = "ti,tps6594-q1", .data = (void *)TPS6594, }, > - { .compatible = "ti,tps6593-q1", .data = (void *)TPS6593, }, > - { .compatible = "ti,lp8764-q1", .data = (void *)LP8764, }, > + { .compatible = "ti,tps6594-q1", .data = &match_data[TPS6594], }, > + { .compatible = "ti,tps6593-q1", .data = &match_data[TPS6593], }, > + { .compatible = "ti,lp8764-q1", .data = &match_data[LP8764], }, Not keen on this. Why do you pass the regmap data through here and leave everything else to be matched on device ID? It would be better to keep passing the device ID through and match everything off of that.
Hello, On Wed, 14 Feb 2024 10:10:17 -0800, Lee Jones wrote: > On Mon, 08 Apr 2024, Bhargav Raviprakash wrote: > > > Introduces a new struct tps6594_match_data. This struct holds fields for > > chip id and regmap config. Using this struct in of_device_id data field. > > This helps in adding support for TPS65224 PMIC. > > > > Signed-off-by: Bhargav Raviprakash <bhargav.r@ltts.com> > > Acked-by: Julien Panis <jpanis@baylibre.com> > > --- > > drivers/mfd/tps6594-i2c.c | 24 ++++++++++++++++-------- > > drivers/mfd/tps6594-spi.c | 24 ++++++++++++++++-------- > > include/linux/mfd/tps6594.h | 11 +++++++++++ > > 3 files changed, 43 insertions(+), 16 deletions(-) > > > > diff --git a/drivers/mfd/tps6594-i2c.c b/drivers/mfd/tps6594-i2c.c > > index c125b474b..9e2ed48b7 100644 > > --- a/drivers/mfd/tps6594-i2c.c > > +++ b/drivers/mfd/tps6594-i2c.c > > @@ -192,10 +192,16 @@ static const struct regmap_config tps6594_i2c_regmap_config = { > > .write = tps6594_i2c_write, > > }; > > > > +static const struct tps6594_match_data match_data[] = { > > + [TPS6594] = {TPS6594, &tps6594_i2c_regmap_config}, > > + [TPS6593] = {TPS6593, &tps6594_i2c_regmap_config}, > > + [LP8764] = {LP8764, &tps6594_i2c_regmap_config}, > > Nit: There should be spaces after the '{' and before the '}'. > Sure! will fix it in the next version. > > +}; > > + > > static const struct of_device_id tps6594_i2c_of_match_table[] = { > > - { .compatible = "ti,tps6594-q1", .data = (void *)TPS6594, }, > > - { .compatible = "ti,tps6593-q1", .data = (void *)TPS6593, }, > > - { .compatible = "ti,lp8764-q1", .data = (void *)LP8764, }, > > + { .compatible = "ti,tps6594-q1", .data = &match_data[TPS6594], }, > > + { .compatible = "ti,tps6593-q1", .data = &match_data[TPS6593], }, > > + { .compatible = "ti,lp8764-q1", .data = &match_data[LP8764], }, > > Not keen on this. Why do you pass the regmap data through here and > leave everything else to be matched on device ID? It would be better to > keep passing the device ID through and match everything off of that. > > > -- > Lee Jones [李琼斯] Thanks for the feedback! These changes were made because of the following message: https://lore.kernel.org/all/7hcysy6ho6.fsf@baylibre.com/ Please let us know which one to follow. Regards, Bhargav
On Mon, 15 Apr 2024, Bhargav Raviprakash wrote: > Hello, > > On Wed, 14 Feb 2024 10:10:17 -0800, Lee Jones wrote: > > On Mon, 08 Apr 2024, Bhargav Raviprakash wrote: > > > > > Introduces a new struct tps6594_match_data. This struct holds fields for > > > chip id and regmap config. Using this struct in of_device_id data field. > > > This helps in adding support for TPS65224 PMIC. > > > > > > Signed-off-by: Bhargav Raviprakash <bhargav.r@ltts.com> > > > Acked-by: Julien Panis <jpanis@baylibre.com> > > > --- > > > drivers/mfd/tps6594-i2c.c | 24 ++++++++++++++++-------- > > > drivers/mfd/tps6594-spi.c | 24 ++++++++++++++++-------- > > > include/linux/mfd/tps6594.h | 11 +++++++++++ > > > 3 files changed, 43 insertions(+), 16 deletions(-) > > > > > > diff --git a/drivers/mfd/tps6594-i2c.c b/drivers/mfd/tps6594-i2c.c > > > index c125b474b..9e2ed48b7 100644 > > > --- a/drivers/mfd/tps6594-i2c.c > > > +++ b/drivers/mfd/tps6594-i2c.c > > > @@ -192,10 +192,16 @@ static const struct regmap_config tps6594_i2c_regmap_config = { > > > .write = tps6594_i2c_write, > > > }; > > > > > > +static const struct tps6594_match_data match_data[] = { > > > + [TPS6594] = {TPS6594, &tps6594_i2c_regmap_config}, > > > + [TPS6593] = {TPS6593, &tps6594_i2c_regmap_config}, > > > + [LP8764] = {LP8764, &tps6594_i2c_regmap_config}, > > > > Nit: There should be spaces after the '{' and before the '}'. > > > > Sure! will fix it in the next version. > > > > +}; > > > + > > > static const struct of_device_id tps6594_i2c_of_match_table[] = { > > > - { .compatible = "ti,tps6594-q1", .data = (void *)TPS6594, }, > > > - { .compatible = "ti,tps6593-q1", .data = (void *)TPS6593, }, > > > - { .compatible = "ti,lp8764-q1", .data = (void *)LP8764, }, > > > + { .compatible = "ti,tps6594-q1", .data = &match_data[TPS6594], }, > > > + { .compatible = "ti,tps6593-q1", .data = &match_data[TPS6593], }, > > > + { .compatible = "ti,lp8764-q1", .data = &match_data[LP8764], }, > > > > Not keen on this. Why do you pass the regmap data through here and > > leave everything else to be matched on device ID? It would be better to > > keep passing the device ID through and match everything off of that. > > > > > > -- > > Lee Jones [李琼斯] > > Thanks for the feedback! > > These changes were made because of the following message: > https://lore.kernel.org/all/7hcysy6ho6.fsf@baylibre.com/ > > Please let us know which one to follow. Right, except this doesn't eliminate "any \"if (chip_id)\" checking". Instead you have a hodge-podge of passing a little bit of (Regmap) data via match and the rest via "if (chip_id)". So either pass all platform type data via .data or just the chip ID. My suggestion 99% of the time is the latter.
Hello, On Tue, 16 Apr 2024 13:25:04 +0100, Lee Jones wrote: > > Hello, > > > > On Wed, 14 Feb 2024 10:10:17 -0800, Lee Jones wrote: > > > On Mon, 08 Apr 2024, Bhargav Raviprakash wrote: > > > > > > > Introduces a new struct tps6594_match_data. This struct holds fields for > > > > chip id and regmap config. Using this struct in of_device_id data field. > > > > This helps in adding support for TPS65224 PMIC. > > > > > > > > Signed-off-by: Bhargav Raviprakash <bhargav.r@ltts.com> > > > > Acked-by: Julien Panis <jpanis@baylibre.com> > > > > --- > > > > drivers/mfd/tps6594-i2c.c | 24 ++++++++++++++++-------- > > > > drivers/mfd/tps6594-spi.c | 24 ++++++++++++++++-------- > > > > include/linux/mfd/tps6594.h | 11 +++++++++++ > > > > 3 files changed, 43 insertions(+), 16 deletions(-) > > > > > > > > diff --git a/drivers/mfd/tps6594-i2c.c b/drivers/mfd/tps6594-i2c.c > > > > index c125b474b..9e2ed48b7 100644 > > > > --- a/drivers/mfd/tps6594-i2c.c > > > > +++ b/drivers/mfd/tps6594-i2c.c > > > > @@ -192,10 +192,16 @@ static const struct regmap_config tps6594_i2c_regmap_config = { > > > > .write = tps6594_i2c_write, > > > > }; > > > > > > > > +static const struct tps6594_match_data match_data[] = { > > > > + [TPS6594] = {TPS6594, &tps6594_i2c_regmap_config}, > > > > + [TPS6593] = {TPS6593, &tps6594_i2c_regmap_config}, > > > > + [LP8764] = {LP8764, &tps6594_i2c_regmap_config}, > > > > > > Nit: There should be spaces after the '{' and before the '}'. > > > > > > > Sure! will fix it in the next version. > > > > > > +}; > > > > + > > > > static const struct of_device_id tps6594_i2c_of_match_table[] = { > > > > - { .compatible = "ti,tps6594-q1", .data = (void *)TPS6594, }, > > > > - { .compatible = "ti,tps6593-q1", .data = (void *)TPS6593, }, > > > > - { .compatible = "ti,lp8764-q1", .data = (void *)LP8764, }, > > > > + { .compatible = "ti,tps6594-q1", .data = &match_data[TPS6594], }, > > > > + { .compatible = "ti,tps6593-q1", .data = &match_data[TPS6593], }, > > > > + { .compatible = "ti,lp8764-q1", .data = &match_data[LP8764], }, > > > > > > Not keen on this. Why do you pass the regmap data through here and > > > leave everything else to be matched on device ID? It would be better to > > > keep passing the device ID through and match everything off of that. > > > > > > > > > -- > > > Lee Jones [李琼斯] > > > > Thanks for the feedback! > > > > These changes were made because of the following message: > > https://lore.kernel.org/all/7hcysy6ho6.fsf@baylibre.com/ > > > > Please let us know which one to follow. > > Right, except this doesn't eliminate "any \"if (chip_id)\" checking". > Instead you have a hodge-podge of passing a little bit of (Regmap) data > via match and the rest via "if (chip_id)". So either pass all platform > type data via .data or just the chip ID. My suggestion 99% of the time > is the latter. Got it. Thanks! Will revert back to .data having chip_id. Regards, Bhargav
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 v5 -> v6: - Refactoring regulator driver: pass both array and size in tps6594_regulator_irq_handler function 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 | 275 ++++++++++--- drivers/regulator/Kconfig | 4 +- drivers/regulator/tps6594-regulator.c | 243 ++++++++++-- include/linux/mfd/tps6594.h | 362 +++++++++++++++++- 10 files changed, 1221 insertions(+), 144 deletions(-) base-commit: 2863b714f3ad0a9686f2de1b779228ad8c7a8052