| Message ID | 20250113230750.124843-2-s-ramamoorthy@ti.com |
|---|---|
| State | New |
| Headers | show |
| Series | Add TI TPS65215 PMIC MFD Support | expand |
On 14/01/2025 01:07, Shree Ramamoorthy wrote: > The chipid macro/variable and regmap_read function call is not needed > because the TPS65219_REG_TI_DEV_ID register value is not a consistent value > across TPS65219 PMIC config versions. Reading from the DEV_ID register > without a consistent value to compare it to isn't useful. There isn't a > way to verify the match data ID is the same ID read from the DEV_ID device > register. 0xF0 isn't a DEV_ID value consistent across TPS65219 NVM > configurations. > > For TPS65215, there is a consistent value in bits 5-0 of the DEV_ID > register. However, there are other error checks in place within probe() > that apply to both PMICs rather than keeping this isolated check for one > PMIC. > > Signed-off-by: Shree Ramamoorthy <s-ramamoorthy@ti.com> subject should be "mfd: tps65219:.."? > --- > drivers/mfd/tps65219.c | 6 ------ > include/linux/mfd/tps65219.h | 7 +++++-- > 2 files changed, 5 insertions(+), 8 deletions(-) > > diff --git a/drivers/mfd/tps65219.c b/drivers/mfd/tps65219.c > index 081c5a30b04a..15b874ee59e5 100644 > --- a/drivers/mfd/tps65219.c > +++ b/drivers/mfd/tps65219.c > @@ -246,12 +246,6 @@ static int tps65219_probe(struct i2c_client *client) > if (ret) > return ret; > > - ret = regmap_read(tps->regmap, TPS65219_REG_TI_DEV_ID, &chipid); > - if (ret) { > - dev_err(tps->dev, "Failed to read device ID: %d\n", ret); > - return ret; > - } > - But now chipid has no users. So please drop that variable as well. > ret = devm_mfd_add_devices(tps->dev, PLATFORM_DEVID_AUTO, > tps65219_cells, ARRAY_SIZE(tps65219_cells), > NULL, 0, regmap_irq_get_domain(tps->irq_data)); > diff --git a/include/linux/mfd/tps65219.h b/include/linux/mfd/tps65219.h > index 546bceec7173..0d88e92ff8f2 100644 > --- a/include/linux/mfd/tps65219.h > +++ b/include/linux/mfd/tps65219.h > @@ -13,8 +13,11 @@ > #include <linux/regmap.h> > #include <linux/regulator/driver.h> > > -/* TPS chip id list */ > -#define TPS65219 0xF0 > +/* Chip id list*/ > +enum pmic_id { > + TPS65215, > + TPS65219, > +}; This change is not part of subject. Please merge it with next patch. > > /* I2C ID for TPS65219 part */ > #define TPS65219_I2C_ID 0x24
Hi, On 1/20/2025 6:25 AM, Roger Quadros wrote: > > On 14/01/2025 01:07, Shree Ramamoorthy wrote: >> The chipid macro/variable and regmap_read function call is not needed >> because the TPS65219_REG_TI_DEV_ID register value is not a consistent value >> across TPS65219 PMIC config versions. Reading from the DEV_ID register >> without a consistent value to compare it to isn't useful. There isn't a >> way to verify the match data ID is the same ID read from the DEV_ID device >> register. 0xF0 isn't a DEV_ID value consistent across TPS65219 NVM >> configurations. >> >> For TPS65215, there is a consistent value in bits 5-0 of the DEV_ID >> register. However, there are other error checks in place within probe() >> that apply to both PMICs rather than keeping this isolated check for one >> PMIC. >> >> Signed-off-by: Shree Ramamoorthy <s-ramamoorthy@ti.com> > subject should be "mfd: tps65219:.."? > >> --- >> drivers/mfd/tps65219.c | 6 ------ >> include/linux/mfd/tps65219.h | 7 +++++-- >> 2 files changed, 5 insertions(+), 8 deletions(-) >> >> diff --git a/drivers/mfd/tps65219.c b/drivers/mfd/tps65219.c >> index 081c5a30b04a..15b874ee59e5 100644 >> --- a/drivers/mfd/tps65219.c >> +++ b/drivers/mfd/tps65219.c >> @@ -246,12 +246,6 @@ static int tps65219_probe(struct i2c_client *client) >> if (ret) >> return ret; >> >> - ret = regmap_read(tps->regmap, TPS65219_REG_TI_DEV_ID, &chipid); >> - if (ret) { >> - dev_err(tps->dev, "Failed to read device ID: %d\n", ret); >> - return ret; >> - } >> - > But now chipid has no users. So please drop that variable as well. > >> ret = devm_mfd_add_devices(tps->dev, PLATFORM_DEVID_AUTO, >> tps65219_cells, ARRAY_SIZE(tps65219_cells), >> NULL, 0, regmap_irq_get_domain(tps->irq_data)); >> diff --git a/include/linux/mfd/tps65219.h b/include/linux/mfd/tps65219.h >> index 546bceec7173..0d88e92ff8f2 100644 >> --- a/include/linux/mfd/tps65219.h >> +++ b/include/linux/mfd/tps65219.h >> @@ -13,8 +13,11 @@ >> #include <linux/regmap.h> >> #include <linux/regulator/driver.h> >> >> -/* TPS chip id list */ >> -#define TPS65219 0xF0 >> +/* Chip id list*/ >> +enum pmic_id { >> + TPS65215, >> + TPS65219, >> +}; > This change is not part of subject. Please merge it with next patch. Thank you for reviewing! Will make these 3 changes & include it in the next version. >> >> /* I2C ID for TPS65219 part */ >> #define TPS65219_I2C_ID 0x24
diff --git a/drivers/mfd/tps65219.c b/drivers/mfd/tps65219.c index 081c5a30b04a..15b874ee59e5 100644 --- a/drivers/mfd/tps65219.c +++ b/drivers/mfd/tps65219.c @@ -246,12 +246,6 @@ static int tps65219_probe(struct i2c_client *client) if (ret) return ret; - ret = regmap_read(tps->regmap, TPS65219_REG_TI_DEV_ID, &chipid); - if (ret) { - dev_err(tps->dev, "Failed to read device ID: %d\n", ret); - return ret; - } - ret = devm_mfd_add_devices(tps->dev, PLATFORM_DEVID_AUTO, tps65219_cells, ARRAY_SIZE(tps65219_cells), NULL, 0, regmap_irq_get_domain(tps->irq_data)); diff --git a/include/linux/mfd/tps65219.h b/include/linux/mfd/tps65219.h index 546bceec7173..0d88e92ff8f2 100644 --- a/include/linux/mfd/tps65219.h +++ b/include/linux/mfd/tps65219.h @@ -13,8 +13,11 @@ #include <linux/regmap.h> #include <linux/regulator/driver.h> -/* TPS chip id list */ -#define TPS65219 0xF0 +/* Chip id list*/ +enum pmic_id { + TPS65215, + TPS65219, +}; /* I2C ID for TPS65219 part */ #define TPS65219_I2C_ID 0x24
The chipid macro/variable and regmap_read function call is not needed because the TPS65219_REG_TI_DEV_ID register value is not a consistent value across TPS65219 PMIC config versions. Reading from the DEV_ID register without a consistent value to compare it to isn't useful. There isn't a way to verify the match data ID is the same ID read from the DEV_ID device register. 0xF0 isn't a DEV_ID value consistent across TPS65219 NVM configurations. For TPS65215, there is a consistent value in bits 5-0 of the DEV_ID register. However, there are other error checks in place within probe() that apply to both PMICs rather than keeping this isolated check for one PMIC. Signed-off-by: Shree Ramamoorthy <s-ramamoorthy@ti.com> --- drivers/mfd/tps65219.c | 6 ------ include/linux/mfd/tps65219.h | 7 +++++-- 2 files changed, 5 insertions(+), 8 deletions(-)