Re: [PATCH 1/2] mfd: (tps65086): Read DEVICE ID register 1 from device

[Lee Jones <lee@kernel.org>]

On Fri, 18 Aug 2023, Andre Werner wrote:

> On Thu, 17 Aug 2023, Lee Jones wrote:
> 
> > On Wed, 09 Aug 2023, werneazc@gmail.com wrote:
> > 
> > > From: Andre Werner <andre.werner@systec-electronic.com>
> > > 
> > > This commit prepares a following commit for the regulator part of the MFD.
> > > The driver should support different device chips that differ in their
> > > register definitions, for instance to control LDOA1 and SWB2.
> > > So it is necessary to use a dedicated regulator description for a
> > > specific device variant. Thus, the content from DEVICEID Register 1 is
> > > used to choose a dedicated configuration between the different device
> > > variants.
> > > 
> > > Signed-off-by: Andre Werner <andre.werner@systec-electronic.com>
> > > ---
> > >  drivers/mfd/tps65086.c       | 37 ++++++++++++++++++++++++++++++------
> > >  include/linux/mfd/tps65086.h | 27 ++++++++++++++++++++------
> > >  2 files changed, 52 insertions(+), 12 deletions(-)
> > > 
> > > diff --git a/drivers/mfd/tps65086.c b/drivers/mfd/tps65086.c
> > > index 6a21000aad4a..38f8572c265e 100644
> > > --- a/drivers/mfd/tps65086.c
> > > +++ b/drivers/mfd/tps65086.c
> > > @@ -64,7 +64,7 @@ MODULE_DEVICE_TABLE(of, tps65086_of_match_table);
> > >  static int tps65086_probe(struct i2c_client *client)
> > >  {
> > >  	struct tps65086 *tps;
> > > -	unsigned int version;
> > > +	unsigned int version, id;
> > >  	int ret;
> > > 
> > >  	tps = devm_kzalloc(&client->dev, sizeof(*tps), GFP_KERNEL);
> > > @@ -81,16 +81,41 @@ static int tps65086_probe(struct i2c_client *client)
> > >  		return PTR_ERR(tps->regmap);
> > >  	}
> > > 
> > > -	ret = regmap_read(tps->regmap, TPS65086_DEVICEID, &version);
> > > +	ret = regmap_read(tps->regmap, TPS65086_DEVICEID1, &id);
> > >  	if (ret) {
> > > -		dev_err(tps->dev, "Failed to read revision register\n");
> > > +		dev_err(tps->dev, "Failed to read revision register 1\n");
> > > +		return ret;
> > > +	}
> > > +
> > > +	/* Store device ID to load regulator configuration that fit to IC variant */
> > > +	switch (id) {
> > > +	case TPS6508640_ID:
> > > +		tps->chip_id = TPS6508640;
> > 
> > Why not use the meaningful TPS6508640_ID for the chip_id instead of an
> > arbitrary enum?
> 
> In the regulator part for this MFD I use this enum ID to select the
> right configuration from an array. So the intention is using the enum as
> the index for this table. I can move this selection into the regulator
> part and store the meaningful TPS65086 IDs in the MFD data if you
> prefer?

That would make more sense for the reader I feel.  Thanks.

> > > +		break;
> > > +	case TPS65086401_ID:
> > > +		tps->chip_id = TPS65086401;
> > > +		break;
> > > +	case TPS6508641_ID:
> > > +		tps->chip_id = TPS6508641;
> > > +		break;
> > > +	case TPS65086470_ID:
> > > +		tps->chip_id = TPS65086470;
> > > +		break;
> > > +	default:
> > > +		dev_err(tps->dev, "Unknown device ID. Cannot determine regulator config.\n");
> > > +		return -ENODEV;
> > > +	}
> > > +
> > > +	ret = regmap_read(tps->regmap, TPS65086_DEVICEID2, &version);
> > > +	if (ret) {
> > > +		dev_err(tps->dev, "Failed to read revision register 2\n");
> > >  		return ret;
> > >  	}
> > > 
> > >  	dev_info(tps->dev, "Device: TPS65086%01lX, OTP: %c, Rev: %ld\n",
> > > -		 (version & TPS65086_DEVICEID_PART_MASK),
> > > -		 (char)((version & TPS65086_DEVICEID_OTP_MASK) >> 4) + 'A',
> > > -		 (version & TPS65086_DEVICEID_REV_MASK) >> 6);
> > > +		 (version & TPS65086_DEVICEID2_PART_MASK),
> > > +		 (char)((version & TPS65086_DEVICEID2_OTP_MASK) >> 4) + 'A',
> > > +		 (version & TPS65086_DEVICEID2_REV_MASK) >> 6);
> > > 
> > >  	if (tps->irq > 0) {
> > >  		ret = regmap_add_irq_chip(tps->regmap, tps->irq, IRQF_ONESHOT, 0,
> > > diff --git a/include/linux/mfd/tps65086.h b/include/linux/mfd/tps65086.h
> > > index 16f87cccc003..88df344b38df 100644
> > > --- a/include/linux/mfd/tps65086.h
> > > +++ b/include/linux/mfd/tps65086.h
> > > @@ -13,8 +13,9 @@
> > >  #include <linux/regmap.h>
> > > 
> > >  /* List of registers for TPS65086 */
> > > -#define TPS65086_DEVICEID		0x01
> > > -#define TPS65086_IRQ			0x02
> > > +#define TPS65086_DEVICEID1		0x00
> > > +#define TPS65086_DEVICEID2		0x01
> > > +#define TPS65086_IRQ		0x02
> > >  #define TPS65086_IRQ_MASK		0x03
> > >  #define TPS65086_PMICSTAT		0x04
> > >  #define TPS65086_SHUTDNSRC		0x05
> > > @@ -75,16 +76,29 @@
> > >  #define TPS65086_IRQ_SHUTDN_MASK	BIT(3)
> > >  #define TPS65086_IRQ_FAULT_MASK		BIT(7)
> > > 
> > > -/* DEVICEID Register field definitions */
> > > -#define TPS65086_DEVICEID_PART_MASK	GENMASK(3, 0)
> > > -#define TPS65086_DEVICEID_OTP_MASK	GENMASK(5, 4)
> > > -#define TPS65086_DEVICEID_REV_MASK	GENMASK(7, 6)
> > > +/* DEVICEID1 Register field definitions */
> > > +#define TPS6508640_ID			0x00
> > > +#define TPS65086401_ID			0x01
> > > +#define TPS6508641_ID			0x10
> > > +#define TPS65086470_ID			0x70
> > > +
> > > +/* DEVICEID2 Register field definitions */
> > > +#define TPS65086_DEVICEID2_PART_MASK	GENMASK(3, 0)
> > > +#define TPS65086_DEVICEID2_OTP_MASK	GENMASK(5, 4)
> > > +#define TPS65086_DEVICEID2_REV_MASK	GENMASK(7, 6)
> > > 
> > >  /* VID Masks */
> > >  #define BUCK_VID_MASK			GENMASK(7, 1)
> > >  #define VDOA1_VID_MASK			GENMASK(4, 1)
> > >  #define VDOA23_VID_MASK			GENMASK(3, 0)
> > > 
> > > +enum tps65086_ids {
> > > +	TPS6508640,
> > > +	TPS65086401,
> > > +	TPS6508641,
> > > +	TPS65086470,
> > > +};
> > > +
> > >  /* Define the TPS65086 IRQ numbers */
> > >  enum tps65086_irqs {
> > >  	TPS65086_IRQ_DIETEMP,
> > > @@ -100,6 +114,7 @@ enum tps65086_irqs {
> > >  struct tps65086 {
> > >  	struct device *dev;
> > >  	struct regmap *regmap;
> > > +	unsigned int chip_id;
> > > 
> > >  	/* IRQ Data */
> > >  	int irq;
> > > --
> > > 2.41.0
> > > 
> > 
> > -- 
> > Lee Jones [李琼斯]
> > 
> 
> Regards,
> 
> André


-- 
Lee Jones [李琼斯]