| Message ID | 1396616889.13903.2.camel@phoenix (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
On Fri, 4 Apr 2014, Axel Lin wrote: > > ret = regmap_read(cs42l52->regmap, CS42L52_CHIP, ®); > - devid = reg & CS42L52_CHIP_ID_MASK; > + devid = (reg & CS42L52_CHIP_ID_MASK) >> 3; What you added does the same thing as what was already there. The CHIP_ID is E0. > if (devid != CS42L52_CHIP_ID) { > ret = -ENODEV; > dev_err(&i2c_client->dev, > @@ -1259,7 +1259,7 @@ static int cs42l52_i2c_probe(struct i2c_client *i2c_client, > } > > dev_info(&i2c_client->dev, "Cirrus Logic CS42L52, Revision: %02X\n", > - reg & 0xFF); > + reg & CS42L52_CHIP_REV_MASK); This works > > /* Set Platform Data */ > if (cs42l52->pdata.mica_diff_cfg) > -#define CS42L52_CHIP_ID 0xE0 > -#define CS42L52_CHIP_ID_MASK 0xF8 > +#define CS42L52_CHIP_ID 0x1C > +#define CS42L52_CHIP_ID_MASK (0x1F << 3) > #define CS42L52_CHIP_REV_A0 0x00 > #define CS42L52_CHIP_REV_A1 0x01 > #define CS42L52_CHIP_REV_B0 0x02 > -#define CS42L52_CHIP_REV_MASK 0x03 > +#define CS42L52_CHIP_REV_B1 0x03 If you used this it would be cool, but your not so I don't see the need for it. So if you want to add the code to actually display the REV_ID I would ack that, but I dont see anything that this patch actually fixes. -Brian
> On Fri, 4 Apr 2014, Axel Lin wrote: > >> >> dev_info(&i2c_client->dev, "Cirrus Logic CS42L52, Revision: %02X\n", >> - reg & 0xFF); >> + reg & CS42L52_CHIP_REV_MASK); >> #define CS42L52_CHIP_REV_B0 0x02 >> -#define CS42L52_CHIP_REV_MASK 0x03 >> +#define CS42L52_CHIP_REV_B1 0x03 Correct me if I'm wrong, but it looks like you added CS42L52_CHIP_REV_MASK and then replaced that define with CS42L52_CHIP_REV_B1? I think some changes to actually display the correct rev_id based on the return value of the reg_read would be cool but this isn't it IMO. I don't think it will be either. Thanks, Brian
On Fri, 4 Apr 2014, Brian Austin wrote: > Correct me if I'm wrong, but it looks like you added CS42L52_CHIP_REV_MASK > and then replaced that define with CS42L52_CHIP_REV_B1? > > I think some changes to actually display the correct rev_id based on the > return value of the reg_read would be cool but this isn't it IMO. I don't > think it will be either. s/be/build/
2014-04-05 1:00 GMT+08:00 Brian Austin <brian.austin@cirrus.com>: > On Fri, 4 Apr 2014, Axel Lin wrote: > >> >> ret = regmap_read(cs42l52->regmap, CS42L52_CHIP, ®); >> - devid = reg & CS42L52_CHIP_ID_MASK; >> + devid = (reg & CS42L52_CHIP_ID_MASK) >> 3; > > > What you added does the same thing as what was already there. > The CHIP_ID is E0. Use CS42L52_CHIP_REV_A1 as example: 11100001 -> CS42L52_CHIP_REV_A1 BIT[0:2] REVID BIT[3:7] CHIPID (CS42L52: 11100) Current code displays: 0xE1 for REVID (because it uses 0xFF as mask) 0xE0 for CHIPID I would expect the driver displays: 01 for REVID 11100 (0x1C) for CHIPID. I think current code mixes REVID & CHIPID. That is why I change the define of CS42L52_CHIP_ID to 0x1C and change CS42L52_CHIP_ID_MASK to (0x1F << 3). In my point of view, the chip id is 0x1C rather than 0xE0, we got the chip id by reading BIT[3:7] of register 01h. > > >> if (devid != CS42L52_CHIP_ID) { >> ret = -ENODEV; >> dev_err(&i2c_client->dev, >> @@ -1259,7 +1259,7 @@ static int cs42l52_i2c_probe(struct i2c_client >> *i2c_client, >> } >> >> dev_info(&i2c_client->dev, "Cirrus Logic CS42L52, Revision: >> %02X\n", >> - reg & 0xFF); >> + reg & CS42L52_CHIP_REV_MASK); > > > This works >> >> >> /* Set Platform Data */ >> if (cs42l52->pdata.mica_diff_cfg) >> -#define CS42L52_CHIP_ID 0xE0 >> -#define CS42L52_CHIP_ID_MASK 0xF8 >> +#define CS42L52_CHIP_ID 0x1C >> +#define CS42L52_CHIP_ID_MASK (0x1F << 3) >> #define CS42L52_CHIP_REV_A0 0x00 >> #define CS42L52_CHIP_REV_A1 0x01 >> #define CS42L52_CHIP_REV_B0 0x02 >> -#define CS42L52_CHIP_REV_MASK 0x03 >> +#define CS42L52_CHIP_REV_B1 0x03 > > > If you used this it would be cool, but your not so I don't see the need for > it. > > So if you want to add the code to actually display the REV_ID I would ack > that, but I dont see anything that this patch actually fixes. I think to actually display the REV_ID or not is another topic. This patch does is to fix the display value. (see above example, both chipid and revid are different from original code). > > -Brian
2014-04-05 5:50 GMT+08:00 Brian Austin <brian.austin@cirrus.com>: > >> On Fri, 4 Apr 2014, Axel Lin wrote: >> >>> >>> dev_info(&i2c_client->dev, "Cirrus Logic CS42L52, Revision: >>> %02X\n", >>> - reg & 0xFF); >>> + reg & CS42L52_CHIP_REV_MASK); > > >>> #define CS42L52_CHIP_REV_B0 0x02 >>> -#define CS42L52_CHIP_REV_MASK 0x03 >>> +#define CS42L52_CHIP_REV_B1 0x03 > > > Correct me if I'm wrong, but it looks like you added CS42L52_CHIP_REV_MASK > and then replaced that define with CS42L52_CHIP_REV_B1? The diff looks like that but... Actually what I did is fix CS42L52_CHIP_REV_MASK, it should be 0x07. and while at it, I also add CS42L52_CHIP_REV_B1 (0x03). -#define CS42L52_CHIP_REV_MASK 0x03 +#define CS42L52_CHIP_REV_B1 0x03 +#define CS42L52_CHIP_REV_MASK 0x07 Regards, Axel
On Apr 4, 2014, at 9:04 PM, Axel Lin <axel.lin@ingics.com> wrote: > 2014-04-05 5:50 GMT+08:00 Brian Austin <brian.austin@cirrus.com>: >> >>> On Fri, 4 Apr 2014, Axel Lin wrote: >>> >>>> >>>> dev_info(&i2c_client->dev, "Cirrus Logic CS42L52, Revision: >>>> %02X\n", >>>> - reg & 0xFF); >>>> + reg & CS42L52_CHIP_REV_MASK); >> >> >>>> #define CS42L52_CHIP_REV_B0 0x02 >>>> -#define CS42L52_CHIP_REV_MASK 0x03 >>>> +#define CS42L52_CHIP_REV_B1 0x03 >> >> >> Correct me if I'm wrong, but it looks like you added CS42L52_CHIP_REV_MASK >> and then replaced that define with CS42L52_CHIP_REV_B1? > > The diff looks like that but... > Actually what I did is fix CS42L52_CHIP_REV_MASK, it should be 0x07. > and while at it, I also add CS42L52_CHIP_REV_B1 (0x03). > > -#define CS42L52_CHIP_REV_MASK 0x03 > +#define CS42L52_CHIP_REV_B1 0x03 > +#define CS42L52_CHIP_REV_MASK 0x07 > > Regards, > Axel I missed that last line, sorry about that. Thanks
On Apr 4, 2014, at 8:59 PM, Axel Lin <axel.lin@ingics.com> wrote: > 2014-04-05 1:00 GMT+08:00 Brian Austin <brian.austin@cirrus.com>: >> On Fri, 4 Apr 2014, Axel Lin wrote: >> >>> >>> ret = regmap_read(cs42l52->regmap, CS42L52_CHIP, ®); >>> - devid = reg & CS42L52_CHIP_ID_MASK; >>> + devid = (reg & CS42L52_CHIP_ID_MASK) >> 3; >> >> >> What you added does the same thing as what was already there. >> The CHIP_ID is E0. > > Use CS42L52_CHIP_REV_A1 as example: > 11100001 -> CS42L52_CHIP_REV_A1 > BIT[0:2] REVID > BIT[3:7] CHIPID (CS42L52: 11100) > > Current code displays: > 0xE1 for REVID (because it uses 0xFF as mask) > 0xE0 for CHIPID > > I would expect the driver displays: > 01 for REVID > 11100 (0x1C) for CHIPID. > Which it does not, but your addition of the correct mask for REV_ID does. But this block of code only checks devid to make sure we are this device. Nothing more. > I think current code mixes REVID & CHIPID. Only for the print statement. Not the DEVICE ID check. > That is why I change the define of CS42L52_CHIP_ID to 0x1C > and change CS42L52_CHIP_ID_MASK to (0x1F << 3). > In my point of view, the chip id is 0x1C rather than 0xE0, CHIP_ID is E0 > we got the chip id by reading BIT[3:7] of register 01h. > > >> >> >>> if (devid != CS42L52_CHIP_ID) { >>> ret = -ENODEV; >>> dev_err(&i2c_client->dev, >>> @@ -1259,7 +1259,7 @@ static int cs42l52_i2c_probe(struct i2c_client >>> *i2c_client, >>> } >>> >>> dev_info(&i2c_client->dev, "Cirrus Logic CS42L52, Revision: >>> %02X\n", >>> - reg & 0xFF); >>> + reg & CS42L52_CHIP_REV_MASK); >> >> >> This works >>> >>> >>> /* Set Platform Data */ >>> if (cs42l52->pdata.mica_diff_cfg) >>> -#define CS42L52_CHIP_ID 0xE0 >>> -#define CS42L52_CHIP_ID_MASK 0xF8 >>> +#define CS42L52_CHIP_ID 0x1C >>> +#define CS42L52_CHIP_ID_MASK (0x1F << 3) >>> #define CS42L52_CHIP_REV_A0 0x00 >>> #define CS42L52_CHIP_REV_A1 0x01 >>> #define CS42L52_CHIP_REV_B0 0x02 >>> -#define CS42L52_CHIP_REV_MASK 0x03 >>> +#define CS42L52_CHIP_REV_B1 0x03 >> >> >> If you used this it would be cool, but your not so I don't see the need for >> it. >> >> So if you want to add the code to actually display the REV_ID I would ack >> that, but I dont see anything that this patch actually fixes. > > I think to actually display the REV_ID or not is another topic. > This patch does is to fix the display value. > (see above example, both chipid and revid are different from original code). Then just fix the display value. which is the 1 line that fixes the REV_MASK. That’s a bug the rest of the current code works just fine. Thanks
2014-04-05 20:48 GMT+08:00 Austin, Brian <Brian.Austin@cirrus.com>: > > On Apr 4, 2014, at 8:59 PM, Axel Lin <axel.lin@ingics.com> wrote: > > 2014-04-05 1:00 GMT+08:00 Brian Austin <brian.austin@cirrus.com>: > > On Fri, 4 Apr 2014, Axel Lin wrote: > > > ret = regmap_read(cs42l52->regmap, CS42L52_CHIP, ®); > - devid = reg & CS42L52_CHIP_ID_MASK; > + devid = (reg & CS42L52_CHIP_ID_MASK) >> 3; > > > > What you added does the same thing as what was already there. > The CHIP_ID is E0. > > > Use CS42L52_CHIP_REV_A1 as example: > 11100001 -> CS42L52_CHIP_REV_A1 > BIT[0:2] REVID > BIT[3:7] CHIPID (CS42L52: 11100) > > Current code displays: > 0xE1 for REVID (because it uses 0xFF as mask) > 0xE0 for CHIPID > > I would expect the driver displays: > 01 for REVID > 11100 (0x1C) for CHIPID. > > Which it does not, but your addition of the correct mask for REV_ID does. > But this block of code only checks devid to make sure we are this device. > Nothing more. > > I think current code mixes REVID & CHIPID. > > > Only for the print statement. Not the DEVICE ID check. > > That is why I change the define of CS42L52_CHIP_ID to 0x1C > and change CS42L52_CHIP_ID_MASK to (0x1F << 3). > In my point of view, the chip id is 0x1C rather than 0xE0, > > > CHIP_ID is E0 You still think the chipid is E0 here for CS42L52. Then I'll say the way you interpret the chipid is inconsistent for different Cirrus Logic chips. For example: CS42L51 also has similar bit difines for REV_ID and CHIP_ID. BIT[0:2]: REV_ID BIT[3:7]: CHIP_ID (The chip id fields in binary is: 11011) In cs42l51.h: it defines CS42L51_CHIP_ID to 0x1B rather than 0xDB. > > we got the chip id by reading BIT[3:7] of register 01h. > > > > > if (devid != CS42L52_CHIP_ID) { > ret = -ENODEV; > dev_err(&i2c_client->dev, > @@ -1259,7 +1259,7 @@ static int cs42l52_i2c_probe(struct i2c_client > *i2c_client, > } > > dev_info(&i2c_client->dev, "Cirrus Logic CS42L52, Revision: > %02X\n", > - reg & 0xFF); > + reg & CS42L52_CHIP_REV_MASK); > > > > This works > > > > /* Set Platform Data */ > if (cs42l52->pdata.mica_diff_cfg) > -#define CS42L52_CHIP_ID 0xE0 > -#define CS42L52_CHIP_ID_MASK 0xF8 > +#define CS42L52_CHIP_ID 0x1C > +#define CS42L52_CHIP_ID_MASK (0x1F << 3) > #define CS42L52_CHIP_REV_A0 0x00 > #define CS42L52_CHIP_REV_A1 0x01 > #define CS42L52_CHIP_REV_B0 0x02 > -#define CS42L52_CHIP_REV_MASK 0x03 > +#define CS42L52_CHIP_REV_B1 0x03 > > > > If you used this it would be cool, but your not so I don't see the need for > it. > > So if you want to add the code to actually display the REV_ID I would ack > that, but I dont see anything that this patch actually fixes. > > > I think to actually display the REV_ID or not is another topic. > This patch does is to fix the display value. > (see above example, both chipid and revid are different from original code). > > > Then just fix the display value. which is the 1 line that fixes the > REV_MASK. That's a bug > the rest of the current code works just fine. Well, I sent a patch to simply fix the REV_MASK. > > Thanks >
> On Apr 5, 2014, at 11:33, "Axel Lin" <axel.lin@ingics.com> wrote: > > 2014-04-05 20:48 GMT+08:00 Austin, Brian <Brian.Austin@cirrus.com>: >> >> On Apr 4, 2014, at 8:59 PM, Axel Lin <axel.lin@ingics.com> wrote: >> >> 2014-04-05 1:00 GMT+08:00 Brian Austin <brian.austin@cirrus.com>: >> >> On Fri, 4 Apr 2014, Axel Lin wrote: >> >> >> ret = regmap_read(cs42l52->regmap, CS42L52_CHIP, ®); >> - devid = reg & CS42L52_CHIP_ID_MASK; >> + devid = (reg & CS42L52_CHIP_ID_MASK) >> 3; >> >> >> >> What you added does the same thing as what was already there. >> The CHIP_ID is E0. >> >> >> Use CS42L52_CHIP_REV_A1 as example: >> 11100001 -> CS42L52_CHIP_REV_A1 >> BIT[0:2] REVID >> BIT[3:7] CHIPID (CS42L52: 11100) >> >> Current code displays: >> 0xE1 for REVID (because it uses 0xFF as mask) >> 0xE0 for CHIPID >> >> I would expect the driver displays: >> 01 for REVID >> 11100 (0x1C) for CHIPID. >> >> Which it does not, but your addition of the correct mask for REV_ID does. >> But this block of code only checks devid to make sure we are this device. >> Nothing more. >> >> I think current code mixes REVID & CHIPID. >> >> >> Only for the print statement. Not the DEVICE ID check. >> >> That is why I change the define of CS42L52_CHIP_ID to 0x1C >> and change CS42L52_CHIP_ID_MASK to (0x1F << 3). >> In my point of view, the chip id is 0x1C rather than 0xE0, >> >> >> CHIP_ID is E0 > > You still think the chipid is E0 here for CS42L52. > Then I'll say the way you interpret the chipid is inconsistent for different > Cirrus Logic chips. > > For example: > CS42L51 also has similar bit difines for REV_ID and CHIP_ID. > > BIT[0:2]: REV_ID > BIT[3:7]: CHIP_ID (The chip id fields in binary is: 11011) > > In cs42l51.h: it defines CS42L51_CHIP_ID to 0x1B rather than 0xDB. > I did not write that driver and will be doing a review of cirrus devices to ensure they are consistent when I have time. >> >> we got the chip id by reading BIT[3:7] of register 01h. >> >> >> >> >> if (devid != CS42L52_CHIP_ID) { >> ret = -ENODEV; >> dev_err(&i2c_client->dev, >> @@ -1259,7 +1259,7 @@ static int cs42l52_i2c_probe(struct i2c_client >> *i2c_client, >> } >> >> dev_info(&i2c_client->dev, "Cirrus Logic CS42L52, Revision: >> %02X\n", >> - reg & 0xFF); >> + reg & CS42L52_CHIP_REV_MASK); >> >> >> >> This works >> >> >> >> /* Set Platform Data */ >> if (cs42l52->pdata.mica_diff_cfg) >> -#define CS42L52_CHIP_ID 0xE0 >> -#define CS42L52_CHIP_ID_MASK 0xF8 >> +#define CS42L52_CHIP_ID 0x1C >> +#define CS42L52_CHIP_ID_MASK (0x1F << 3) >> #define CS42L52_CHIP_REV_A0 0x00 >> #define CS42L52_CHIP_REV_A1 0x01 >> #define CS42L52_CHIP_REV_B0 0x02 >> -#define CS42L52_CHIP_REV_MASK 0x03 >> +#define CS42L52_CHIP_REV_B1 0x03 >> >> >> >> If you used this it would be cool, but your not so I don't see the need for >> it. >> >> So if you want to add the code to actually display the REV_ID I would ack >> that, but I dont see anything that this patch actually fixes. >> >> >> I think to actually display the REV_ID or not is another topic. >> This patch does is to fix the display value. >> (see above example, both chipid and revid are different from original code). >> >> >> Then just fix the display value. which is the 1 line that fixes the >> REV_MASK. That's a bug >> the rest of the current code works just fine. > > Well, I sent a patch to simply fix the REV_MASK. >> Thanks! >> Thanks >>
diff --git a/sound/soc/codecs/cs42l52.c b/sound/soc/codecs/cs42l52.c index f0ca6be..277df17 100644 --- a/sound/soc/codecs/cs42l52.c +++ b/sound/soc/codecs/cs42l52.c @@ -1249,7 +1249,7 @@ static int cs42l52_i2c_probe(struct i2c_client *i2c_client, ret); ret = regmap_read(cs42l52->regmap, CS42L52_CHIP, ®); - devid = reg & CS42L52_CHIP_ID_MASK; + devid = (reg & CS42L52_CHIP_ID_MASK) >> 3; if (devid != CS42L52_CHIP_ID) { ret = -ENODEV; dev_err(&i2c_client->dev, @@ -1259,7 +1259,7 @@ static int cs42l52_i2c_probe(struct i2c_client *i2c_client, } dev_info(&i2c_client->dev, "Cirrus Logic CS42L52, Revision: %02X\n", - reg & 0xFF); + reg & CS42L52_CHIP_REV_MASK); /* Set Platform Data */ if (cs42l52->pdata.mica_diff_cfg) diff --git a/sound/soc/codecs/cs42l52.h b/sound/soc/codecs/cs42l52.h index 6fb8f00..ce7ce5a 100644 --- a/sound/soc/codecs/cs42l52.h +++ b/sound/soc/codecs/cs42l52.h @@ -32,12 +32,13 @@ #define CS42L52_FIX_BITS_CTL 0x00 #define CS42L52_CHIP 0x01 -#define CS42L52_CHIP_ID 0xE0 -#define CS42L52_CHIP_ID_MASK 0xF8 +#define CS42L52_CHIP_ID 0x1C +#define CS42L52_CHIP_ID_MASK (0x1F << 3) #define CS42L52_CHIP_REV_A0 0x00 #define CS42L52_CHIP_REV_A1 0x01 #define CS42L52_CHIP_REV_B0 0x02 -#define CS42L52_CHIP_REV_MASK 0x03 +#define CS42L52_CHIP_REV_B1 0x03 +#define CS42L52_CHIP_REV_MASK 0x07 #define CS42L52_PWRCTL1 0x02 #define CS42L52_PWRCTL1_PDN_ALL 0x9F
According to the datasheet: Chip I.D and Revision Register (Address 01h) BIT[0:2] REVID BIT[3:7] CHIPID (CS42L52: 11100) REVID takes 3 bits, so CS42L52_CHIP_REV_MASK should be 0x07. While at it, also adds define for CS42L52_CHIP_REV_B1. The CHIPID takes BIT[3:7], so this patch updates the defines and the code to show correct chip id for this chip. Signed-off-by: Axel Lin <axel.lin@ingics.com> --- sound/soc/codecs/cs42l52.c | 4 ++-- sound/soc/codecs/cs42l52.h | 7 ++++--- 2 files changed, 6 insertions(+), 5 deletions(-)