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