| Message ID | 20250103225732.196636-3-s-ramamoorthy@ti.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Add TI TPS65215 PMIC MFD Support | expand |
On 04/01/2025 00:57, 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> In that case this could be squashed with 1? > --- > drivers/mfd/tps65219.c | 6 ------ > include/linux/mfd/tps65219.h | 2 -- > 2 files changed, 8 deletions(-) > > diff --git a/drivers/mfd/tps65219.c b/drivers/mfd/tps65219.c > index 816b271990a2..d3267bf7cd77 100644 > --- a/drivers/mfd/tps65219.c > +++ b/drivers/mfd/tps65219.c > @@ -382,12 +382,6 @@ static int tps65219_probe(struct i2c_client *client) > if (ret) > return ret; > > - ret = regmap_read(tps->regmap, TPS65219_REG_TI_DEV_ID, &tps->chip_id); > - 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, > pmic->cells, pmic->n_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 9892b6e4c85c..535115bfa4a4 100644 > --- a/include/linux/mfd/tps65219.h > +++ b/include/linux/mfd/tps65219.h > @@ -15,8 +15,6 @@ > #include <linux/regmap.h> > #include <linux/regulator/driver.h> > > -/* TPS chip id list */ > -#define TPS65219 0xF0 > /* Chip id list*/ > enum pmic_id { > TPS65215, Looking at TRM, TPS65215 device_id is 0x15 and TPS6521901 device_id is 0x00. shouldn't we use that here as well?
Hi, On 1/4/2025 12:16 PM, Roger Quadros wrote: > > On 04/01/2025 00:57, 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> > In that case this could be squashed with 1? Since this change does not have to do with TPS65215 support directly and is a different type of change, I wanted to keep this patch separate. I can instead have this patch be first, then the MFD add TPS65215 support will follow this to avoid any confusion about regmap_read being modified then removed. >> --- >> drivers/mfd/tps65219.c | 6 ------ >> include/linux/mfd/tps65219.h | 2 -- >> 2 files changed, 8 deletions(-) >> >> diff --git a/drivers/mfd/tps65219.c b/drivers/mfd/tps65219.c >> index 816b271990a2..d3267bf7cd77 100644 >> --- a/drivers/mfd/tps65219.c >> +++ b/drivers/mfd/tps65219.c >> @@ -382,12 +382,6 @@ static int tps65219_probe(struct i2c_client *client) >> if (ret) >> return ret; >> >> - ret = regmap_read(tps->regmap, TPS65219_REG_TI_DEV_ID, &tps->chip_id); >> - 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, >> pmic->cells, pmic->n_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 9892b6e4c85c..535115bfa4a4 100644 >> --- a/include/linux/mfd/tps65219.h >> +++ b/include/linux/mfd/tps65219.h >> @@ -15,8 +15,6 @@ >> #include <linux/regmap.h> >> #include <linux/regulator/driver.h> >> >> -/* TPS chip id list */ >> -#define TPS65219 0xF0 >> /* Chip id list*/ >> enum pmic_id { >> TPS65215, > Looking at TRM, TPS65215 device_id is 0x15 and TPS6521901 device_id is 0x00. > > shouldn't we use that here as well? The device_id value set varies across TPS65219 hardware versions. Having the device_id as the chip_id differentiator will fail for TPS65219, even though the system engineers have now kept the TPS65215 device_id value consistent across all hardware versions.
On 07/01/2025 00:18, Shree Ramamoorthy wrote: > Hi, > > On 1/4/2025 12:16 PM, Roger Quadros wrote: >> >> On 04/01/2025 00:57, 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> >> In that case this could be squashed with 1? > > Since this change does not have to do with TPS65215 support directly > and is a different type of change, I wanted to keep this patch separate. > I can instead have this patch be first, then the MFD add TPS65215 support > will follow this to avoid any confusion about regmap_read being modified then removed. > OK thanks. >>> --- >>> drivers/mfd/tps65219.c | 6 ------ >>> include/linux/mfd/tps65219.h | 2 -- >>> 2 files changed, 8 deletions(-) >>> >>> diff --git a/drivers/mfd/tps65219.c b/drivers/mfd/tps65219.c >>> index 816b271990a2..d3267bf7cd77 100644 >>> --- a/drivers/mfd/tps65219.c >>> +++ b/drivers/mfd/tps65219.c >>> @@ -382,12 +382,6 @@ static int tps65219_probe(struct i2c_client *client) >>> if (ret) >>> return ret; >>> >>> - ret = regmap_read(tps->regmap, TPS65219_REG_TI_DEV_ID, &tps->chip_id); >>> - 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, >>> pmic->cells, pmic->n_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 9892b6e4c85c..535115bfa4a4 100644 >>> --- a/include/linux/mfd/tps65219.h >>> +++ b/include/linux/mfd/tps65219.h >>> @@ -15,8 +15,6 @@ >>> #include <linux/regmap.h> >>> #include <linux/regulator/driver.h> >>> >>> -/* TPS chip id list */ >>> -#define TPS65219 0xF0 >>> /* Chip id list*/ >>> enum pmic_id { >>> TPS65215, >> Looking at TRM, TPS65215 device_id is 0x15 and TPS6521901 device_id is 0x00. >> >> shouldn't we use that here as well? > > The device_id value set varies across TPS65219 hardware versions. Do you foresee any software quirks being applied for specific versions of TPS65219? If not then probably not worth the effort to keep track of all the versions. > Having the device_id as the chip_id differentiator will fail for TPS65219, > even though the system engineers have now kept the TPS65215 device_id value > consistent across all hardware versions. >
Hi, On 1/7/25 6:47 AM, Roger Quadros wrote: > > On 07/01/2025 00:18, Shree Ramamoorthy wrote: >> Hi, >> >> On 1/4/2025 12:16 PM, Roger Quadros wrote: >>> On 04/01/2025 00:57, 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> >>> In that case this could be squashed with 1? >> Since this change does not have to do with TPS65215 support directly >> and is a different type of change, I wanted to keep this patch separate. >> I can instead have this patch be first, then the MFD add TPS65215 support >> will follow this to avoid any confusion about regmap_read being modified then removed. >> > OK thanks. > >>>> --- >>>> drivers/mfd/tps65219.c | 6 ------ >>>> include/linux/mfd/tps65219.h | 2 -- >>>> 2 files changed, 8 deletions(-) >>>> >>>> diff --git a/drivers/mfd/tps65219.c b/drivers/mfd/tps65219.c >>>> index 816b271990a2..d3267bf7cd77 100644 >>>> --- a/drivers/mfd/tps65219.c >>>> +++ b/drivers/mfd/tps65219.c >>>> @@ -382,12 +382,6 @@ static int tps65219_probe(struct i2c_client *client) >>>> if (ret) >>>> return ret; >>>> >>>> - ret = regmap_read(tps->regmap, TPS65219_REG_TI_DEV_ID, &tps->chip_id); >>>> - 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, >>>> pmic->cells, pmic->n_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 9892b6e4c85c..535115bfa4a4 100644 >>>> --- a/include/linux/mfd/tps65219.h >>>> +++ b/include/linux/mfd/tps65219.h >>>> @@ -15,8 +15,6 @@ >>>> #include <linux/regmap.h> >>>> #include <linux/regulator/driver.h> >>>> >>>> -/* TPS chip id list */ >>>> -#define TPS65219 0xF0 >>>> /* Chip id list*/ >>>> enum pmic_id { >>>> TPS65215, >>> Looking at TRM, TPS65215 device_id is 0x15 and TPS6521901 device_id is 0x00. >>> >>> shouldn't we use that here as well? >> The device_id value set varies across TPS65219 hardware versions. > Do you foresee any software quirks being applied for specific versions of > TPS65219? If not then probably not worth the effort to keep track of all the > versions. I don't foresee any sw quirks that would need to be support for TPS65219, since there haven't been any since the driver was released. >> Having the device_id as the chip_id differentiator will fail for TPS65219, >> even though the system engineers have now kept the TPS65215 device_id value >> consistent across all hardware versions. >>
diff --git a/drivers/mfd/tps65219.c b/drivers/mfd/tps65219.c index 816b271990a2..d3267bf7cd77 100644 --- a/drivers/mfd/tps65219.c +++ b/drivers/mfd/tps65219.c @@ -382,12 +382,6 @@ static int tps65219_probe(struct i2c_client *client) if (ret) return ret; - ret = regmap_read(tps->regmap, TPS65219_REG_TI_DEV_ID, &tps->chip_id); - 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, pmic->cells, pmic->n_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 9892b6e4c85c..535115bfa4a4 100644 --- a/include/linux/mfd/tps65219.h +++ b/include/linux/mfd/tps65219.h @@ -15,8 +15,6 @@ #include <linux/regmap.h> #include <linux/regulator/driver.h> -/* TPS chip id list */ -#define TPS65219 0xF0 /* Chip id list*/ enum pmic_id { TPS65215,
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 | 2 -- 2 files changed, 8 deletions(-)