| Message ID | a667a64d0cbeef00baed2d4b117ba9f50eaf3988.1677620677.git.jahau@rocketmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | Add RT5033 charger device driver | expand |
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@rocketmail.com> > --- > 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? > + 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? > 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 > /* 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 >
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@rocketmail.com> >> --- >> 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
On Sun, 05 Mar 2023, Jakob Hauser wrote: > 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@rocketmail.com> > > > --- > > > 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. You're reading from a register called RT5033_REG_DEVICE_ID. I don't see any reason why the variable you read into can't reflect that. > [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
Hi Lee, On 06.03.23 10:18, Lee Jones wrote: > On Sun, 05 Mar 2023, Jakob Hauser wrote: > >> 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@rocketmail.com> >>>> --- >>>> 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. > > You're reading from a register called RT5033_REG_DEVICE_ID. I don't see > any reason why the variable you read into can't reflect that. OK, I'll use "dev_id" and "chip_rev" for the variable names. ... Kind regards, Jakob
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; + 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); 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 { /* 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
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@rocketmail.com> --- drivers/mfd/rt5033.c | 8 +++++--- include/linux/mfd/rt5033-private.h | 4 ++++ 2 files changed, 9 insertions(+), 3 deletions(-)