| Message ID | 20240408124047.191895-4-bhargav.r@ltts.com |
|---|---|
| State | New |
| Headers | show |
| Series | Add support for TI TPS65224 PMIC | expand |
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
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}, +}; + 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], }, {} }; MODULE_DEVICE_TABLE(of, tps6594_i2c_of_match_table); @@ -205,6 +211,7 @@ static int tps6594_i2c_probe(struct i2c_client *client) struct device *dev = &client->dev; struct tps6594 *tps; const struct of_device_id *match; + const struct tps6594_match_data *mdata; tps = devm_kzalloc(dev, sizeof(*tps), GFP_KERNEL); if (!tps) @@ -216,14 +223,15 @@ static int tps6594_i2c_probe(struct i2c_client *client) tps->reg = client->addr; tps->irq = client->irq; - tps->regmap = devm_regmap_init(dev, NULL, client, &tps6594_i2c_regmap_config); - if (IS_ERR(tps->regmap)) - return dev_err_probe(dev, PTR_ERR(tps->regmap), "Failed to init regmap\n"); - match = of_match_device(tps6594_i2c_of_match_table, dev); if (!match) return dev_err_probe(dev, -EINVAL, "Failed to find matching chip ID\n"); - tps->chip_id = (unsigned long)match->data; + mdata = (struct tps6594_match_data *)match->data; + tps->chip_id = mdata->chip_id; + + tps->regmap = devm_regmap_init(dev, NULL, client, mdata->config); + if (IS_ERR(tps->regmap)) + return dev_err_probe(dev, PTR_ERR(tps->regmap), "Failed to init regmap\n"); crc8_populate_msb(tps6594_i2c_crc_table, TPS6594_CRC8_POLYNOMIAL); diff --git a/drivers/mfd/tps6594-spi.c b/drivers/mfd/tps6594-spi.c index 5afb1736f..82a1c02e3 100644 --- a/drivers/mfd/tps6594-spi.c +++ b/drivers/mfd/tps6594-spi.c @@ -77,10 +77,16 @@ static const struct regmap_config tps6594_spi_regmap_config = { .use_single_write = true, }; +static const struct tps6594_match_data match_data[] = { + [TPS6594] = {TPS6594, &tps6594_spi_regmap_config}, + [TPS6593] = {TPS6593, &tps6594_spi_regmap_config}, + [LP8764] = {LP8764, &tps6594_spi_regmap_config}, +}; + static const struct of_device_id tps6594_spi_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], }, {} }; MODULE_DEVICE_TABLE(of, tps6594_spi_of_match_table); @@ -90,6 +96,7 @@ static int tps6594_spi_probe(struct spi_device *spi) struct device *dev = &spi->dev; struct tps6594 *tps; const struct of_device_id *match; + const struct tps6594_match_data *mdata; tps = devm_kzalloc(dev, sizeof(*tps), GFP_KERNEL); if (!tps) @@ -101,14 +108,15 @@ static int tps6594_spi_probe(struct spi_device *spi) tps->reg = spi_get_chipselect(spi, 0); tps->irq = spi->irq; - tps->regmap = devm_regmap_init(dev, NULL, spi, &tps6594_spi_regmap_config); - if (IS_ERR(tps->regmap)) - return dev_err_probe(dev, PTR_ERR(tps->regmap), "Failed to init regmap\n"); - match = of_match_device(tps6594_spi_of_match_table, dev); if (!match) return dev_err_probe(dev, -EINVAL, "Failed to find matching chip ID\n"); - tps->chip_id = (unsigned long)match->data; + mdata = (struct tps6594_match_data *)match->data; + tps->chip_id = mdata->chip_id; + + tps->regmap = devm_regmap_init(dev, NULL, spi, mdata->config); + if (IS_ERR(tps->regmap)) + return dev_err_probe(dev, PTR_ERR(tps->regmap), "Failed to init regmap\n"); crc8_populate_msb(tps6594_spi_crc_table, TPS6594_CRC8_POLYNOMIAL); diff --git a/include/linux/mfd/tps6594.h b/include/linux/mfd/tps6594.h index 16543fd4d..d781e0fe3 100644 --- a/include/linux/mfd/tps6594.h +++ b/include/linux/mfd/tps6594.h @@ -1337,6 +1337,17 @@ struct tps6594 { struct regmap_irq_chip_data *irq_data; }; +/** + * struct tps6594_match_data - of match data of PMIC + * + * @chip_id: chip ID of PMIC + * @config: regmap config of PMIC + */ +struct tps6594_match_data { + unsigned long chip_id; + const struct regmap_config *config; +}; + extern const struct regmap_access_table tps6594_volatile_table; extern const struct regmap_access_table tps65224_volatile_table;