Re: [PATCH 02/10] mfd: rt5033: Fix chip revision readout

[Jakob Hauser]

Hi Lee,

On 05.03.23 11:47, Lee Jones wrote:
> On Tue, 28 Feb 2023, Jakob Hauser wrote:
>
> > After reading the data from the DEVICE_ID register, mask 0x0f needs to be
> > applied to extract the revision of the chip [1].
> >
> > The other part of the DEVICE_ID register, mask 0xf0, is a vendor identification
> > code. That's how it is set up at similar products of Richtek, e.g. RT9455 [2]
> > page 21 top.
> >
> > [1] https://github.com/msm8916-mainline/linux-downstream/blob/GT-I9195I/drivers/mfd/rt5033_core.c#L484
> > [2] https://www.richtek.com/assets/product_file/RT9455/DS9455-00.pdf
> >
> > Signed-off-by: Jakob Hauser <jahau@xxxxxxxxxxxxxx>
> > ---
> >   drivers/mfd/rt5033.c               | 8 +++++---
> >   include/linux/mfd/rt5033-private.h | 4 ++++
> >   2 files changed, 9 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/mfd/rt5033.c b/drivers/mfd/rt5033.c
> > index 8029d444b794..d32467174cb5 100644
> > --- a/drivers/mfd/rt5033.c
> > +++ b/drivers/mfd/rt5033.c
> > @@ -55,7 +55,8 @@ static const struct regmap_config rt5033_regmap_config = {
> >   static int rt5033_i2c_probe(struct i2c_client *i2c)
> >   {
> >   	struct rt5033_dev *rt5033;
> > -	unsigned int dev_id;
> > +	unsigned int data;
>
> In terms of nomenclature, this is a regression.
>
> 'data' is a terrible variable name.  Why not keep it as-is?

While not having a datasheet for RT5033 available, in similar products 
like RT9455 the register is called "Device ID", the first part of that 
is "VENDOR_ID" and the second part "CHIP_REV", [1] page 23 top. Or in 
RT5036 preliminary data sheet the register is called "ID", the first 
part "VENDOR_ID" and the second part "CHIP_REV_ID", [2] page 27 top.

I wanted to avoid confusion between "dev_id" and "chip_rev". Therefore 
in the patch it's written as getting some "data" from the register and 
extract "chip_rev" from that data.

I could change it to "reg_data"? Or something in that direction? I still 
think that getting "chip_rev" out of "dev_id" would be confusing.

[1] https://www.richtek.com/assets/product_file/RT9455/DS9455-00.pdf
[2] 
https://media.digikey.com/pdf/Data%20Sheets/Richtek%20PDF/RT5036%20%20Preliminary.pdf

> > +	unsigned int chip_rev;
> >   	int ret;
> >   
> >   	rt5033 = devm_kzalloc(&i2c->dev, sizeof(*rt5033), GFP_KERNEL);
> > @@ -73,12 +74,13 @@ static int rt5033_i2c_probe(struct i2c_client *i2c)
> >   		return PTR_ERR(rt5033->regmap);
> >   	}
> >   
> > -	ret = regmap_read(rt5033->regmap, RT5033_REG_DEVICE_ID, &dev_id);
> > +	ret = regmap_read(rt5033->regmap, RT5033_REG_DEVICE_ID, &data);
> >   	if (ret) {
> >   		dev_err(&i2c->dev, "Device not found\n");
> >   		return -ENODEV;
> >   	}
> > -	dev_info(&i2c->dev, "Device found Device ID: %04x\n", dev_id);
> > +	chip_rev = data & RT5033_CHIP_REV_MASK;
> > +	dev_info(&i2c->dev, "Device found (rev. %d)\n", chip_rev);
>
> Why not print both?

As described above, the data "dev_id" consists of a first part which is 
a vendor ID and a second part which is the chip revision.

The vendor ID is of no interest here. These bits[7:4] contain binary 
value 1000 (decimal value 8) and I'd expect that to be the same on all 
RT5033 devices.

Contrary to this, the chip revision is an important information. The 
downstream Android driver applies some quirks depending on the chip 
revision. This seemed not yet necessary in the upstream driver. So far 
I've seen chip rev. 6 on samsung-serranove & samsung-e7 and chip rev. 5 
on samsung-grandmax & samsung-fortuna, the behavior of the chip 
revisions are slightly different.

Accordingly, the downstream Android driver as well reads [3] and prints 
[4] the chip revision only – confusingly calling it "rev id".
[3] 
https://github.com/msm8916-mainline/linux-downstream/blob/GT-I9195I/drivers/mfd/rt5033_core.c#L484
[4] 
https://github.com/msm8916-mainline/linux-downstream/blob/GT-I9195I/drivers/mfd/rt5033_core.c#L486

> >   	ret = regmap_add_irq_chip(rt5033->regmap, rt5033->irq,
> >   			IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
> > diff --git a/include/linux/mfd/rt5033-private.h b/include/linux/mfd/rt5033-private.h
> > index 2d1895c3efbf..d18cd4572208 100644
> > --- a/include/linux/mfd/rt5033-private.h
> > +++ b/include/linux/mfd/rt5033-private.h
> > @@ -71,6 +71,10 @@ enum rt5033_reg {
>
>
>
>
>
>
>
>
>
>
>
>
>
>
> g

What does the "g" mean, was this on purpose? I didn't get the meaning of it.

> >   /* RT5033 CHGCTRL2 register */
> >   #define RT5033_CHGCTRL2_CV_MASK		0xfc
> >   
> > +/* RT5033 DEVICE_ID register */
> > +#define RT5033_VENDOR_ID_MASK		0xf0
> > +#define RT5033_CHIP_REV_MASK		0x0f
> > +
> >   /* RT5033 CHGCTRL3 register */
> >   #define RT5033_CHGCTRL3_CFO_EN_MASK	0x40
> >   #define RT5033_CHGCTRL3_TIMER_MASK	0x38
> > --
> > 2.39.1

Kind regards,
Jakob