ASoC: cs42l52: Report correct chip id and revision

PATCH In review Axel Lin 2 participants 10 messages Apr 4, 2014 – Apr 5, 2014

Subsystems (3)

Sound ASoC Cirrus Logic audio codecs

lore

Share & tools

lore

Axel Lin

author

2014-04-04 13:08 UTC #0 lore raw

PATCH 1/1 ASoC: cs42l52: Report correct chip id and revision

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 <[email protected]>

---
 sound/soc/codecs/cs42l52.c | 4 ++--
 sound/soc/codecs/cs42l52.h | 7 ++++---
 2 files changed, 6 insertions(+), 5 deletions(-)

sound/soc/codecs/cs42l52.c +2 -2

index f0ca6be..277df17 100644
@@ -1249,7 +1249,7 @@ static int cs42l52_i2c_probe(struct i2c_client *i2c_client,
 			 ret);
 
 	ret = regmap_read(cs42l52->regmap, CS42L52_CHIP, &reg);
-	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)

sound/soc/codecs/cs42l52.h +4 -3

index 6fb8f00..ce7ce5a 100644
@@ -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

Signature · 1 line

1.8.3.2

Brian Austin

2014-04-04 17:06 UTC #1 lore raw

sound/soc/codecs/cs42l52.c static int cs42l52_i2c_probe(struct i2c_client *i2c_client, · line 1252

Quoted 4 lines · Axel Lin

	ret = regmap_read(cs42l52->regmap, CS42L52_CHIP, &reg);

sound/soc/codecs/cs42l52.c +1 -1

-	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.

sound/soc/codecs/cs42l52.c static int cs42l52_i2c_probe(struct i2c_client *i2c_client, · line 1262

Quoted 9 lines

	if (devid != CS42L52_CHIP_ID) {
		ret = -ENODEV;
		dev_err(&i2c_client->dev,

sound/soc/codecs/cs42l52.c +1 -1

@@ -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

sound/soc/codecs/cs42l52.h line 40

Quoted 12 lines

sound/soc/codecs/cs42l52.h +2 -2

 	/* 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

sound/soc/codecs/cs42l52.h +1 -1

-#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

Brian Austin

2014-04-04 21:50 UTC #2 lore raw

Quoted 11 lines · Axel Lin

sound/soc/codecs/cs42l52.h line 40
Quoted 9 lines
	dev_info(&i2c_client->dev, "Cirrus Logic CS42L52, Revision: %02X\n",
sound/soc/codecs/cs42l52.c +1 -1
-			reg & 0xFF);
+			reg & CS42L52_CHIP_REV_MASK);
#define CS42L52_CHIP_REV_B0 0x02
sound/soc/codecs/cs42l52.h +1 -1
-#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

Brian Austin

2014-04-05 00:01 UTC #3 lore raw

Quoted 3 lines · Brian Austin

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/

Axel Lin

author

2014-04-05 02:04 UTC #4 lore raw

In reply to Brian Austin

2014-04-05 5:50 GMT+08:00 Brian Austin <[email protected]>:

Quoted 10 lines

Quoted 8 lines · Axel Lin

Quoted 6 lines


        dev_info(&i2c_client->dev, "Cirrus Logic CS42L52, Revision:

%02X\n",

sound/soc/codecs/cs42l52.c +1 -1

-                       reg & 0xFF);
+                       reg & CS42L52_CHIP_REV_MASK);

Quoted 7 lines

Quoted 3 lines
sound/soc/codecs/cs42l52.h line 40
Quoted 3 lines
#define CS42L52_CHIP_REV_B0 0x02
sound/soc/codecs/cs42l52.h +1 -1
-#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

Austin, Brian

2014-04-05 12:46 UTC #5 lore raw

Quoted 27 lines · Axel Lin

2014-04-05 5:50 GMT+08:00 Brian Austin <[email protected]>:
Quoted 10 lines
Quoted 8 lines · Axel Lin
Quoted 6 lines
       dev_info(&i2c_client->dev, "Cirrus Logic CS42L52, Revision:
%02X\n",
sound/soc/codecs/cs42l52.c +1 -1
-                       reg & 0xFF);
+                       reg & CS42L52_CHIP_REV_MASK);
Quoted 7 lines
Quoted 3 lines
sound/soc/codecs/cs42l52.h line 40
Quoted 3 lines
#define CS42L52_CHIP_REV_B0 0x02
sound/soc/codecs/cs42l52.h +1 -1
-#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).
sound/soc/codecs/cs42l52.h +2 -1
-#define CS42L52_CHIP_REV_MASK                  0x03
+#define CS42L52_CHIP_REV_B1                    0x03
+#define CS42L52_CHIP_REV_MASK                  0x07

Quoted 2 lines

Regards,
Axel

I missed that last line, sorry about that.

Thanks

Axel Lin

author

2014-04-05 01:59 UTC #6 lore raw

In reply to Brian Austin

2014-04-05 1:00 GMT+08:00 Brian Austin <[email protected]>:

Quoted 10 lines · Axel Lin

sound/soc/codecs/cs42l52.c static int cs42l52_i2c_probe(struct i2c_client *i2c_client, · line 1252
Quoted 5 lines
        ret = regmap_read(cs42l52->regmap, CS42L52_CHIP, &reg);
sound/soc/codecs/cs42l52.c +1 -1
-       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.

Quoted 34 lines

sound/soc/codecs/cs42l52.c static int cs42l52_i2c_probe(struct i2c_client · line 1264

Quoted 13 lines


        if (devid != CS42L52_CHIP_ID) {
                ret = -ENODEV;
                dev_err(&i2c_client->dev,

sound/soc/codecs/cs42l52.c +1 -1

@@ -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

sound/soc/codecs/cs42l52.h line 40

Quoted 13 lines

sound/soc/codecs/cs42l52.c +0 -0

        /* Set Platform Data */

        if (cs42l52->pdata.mica_diff_cfg)

sound/soc/codecs/cs42l52.h +2 -2

-#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

sound/soc/codecs/cs42l52.h +1 -1

-#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.

Quoted 2 lines

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).

Quoted 2 lines

-Brian

Austin, Brian

2014-04-05 12:48 UTC #7 lore raw

Quoted 17 lines · Axel Lin

2014-04-05 1:00 GMT+08:00 Brian Austin <[email protected]>:
Quoted 10 lines · Axel Lin
sound/soc/codecs/cs42l52.c static int cs42l52_i2c_probe(struct i2c_client *i2c_client, · line 1252
Quoted 5 lines
       ret = regmap_read(cs42l52->regmap, CS42L52_CHIP, &reg);
sound/soc/codecs/cs42l52.c +1 -1
-       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)

Quoted 8 lines

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.

Quoted 1 line

I think current code mixes REVID & CHIPID.

Only for the print statement. Not the DEVICE ID check.

Quoted 3 lines

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

Quoted 37 lines

we got the chip id by reading BIT[3:7] of register 01h.

Quoted 34 lines

sound/soc/codecs/cs42l52.c static int cs42l52_i2c_probe(struct i2c_client · line 1264

Quoted 13 lines


       if (devid != CS42L52_CHIP_ID) {
               ret = -ENODEV;
               dev_err(&i2c_client->dev,

sound/soc/codecs/cs42l52.c +1 -1

@@ -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

sound/soc/codecs/cs42l52.h line 40

Quoted 13 lines

sound/soc/codecs/cs42l52.c +0 -0

       /* Set Platform Data */

       if (cs42l52->pdata.mica_diff_cfg)

sound/soc/codecs/cs42l52.h +2 -2

-#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

sound/soc/codecs/cs42l52.h +1 -1

-#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.

Quoted 6 lines

Quoted 2 lines
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

Axel Lin

author

2014-04-05 16:39 UTC #8 lore raw

2014-04-05 20:48 GMT+08:00 Austin, Brian <[email protected]>:

Quoted 38 lines · Axel Lin

2014-04-05 1:00 GMT+08:00 Brian Austin <[email protected]>:
On Fri, 4 Apr 2014, Axel Lin wrote:
       ret = regmap_read(cs42l52->regmap, CS42L52_CHIP, &reg);
sound/soc/codecs/cs42l52.c +1 -1
-       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.

Quoted 8 lines

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.

Quoted 45 lines

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,

sound/soc/codecs/cs42l52.c +1 -1

@@ -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)

sound/soc/codecs/cs42l52.h +2 -2

-#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

sound/soc/codecs/cs42l52.h +1 -1

-#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.

Quoted 8 lines

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.

Quoted 3 lines

Thanks

Austin, Brian

2014-04-05 16:50 UTC #9 lore raw

Quoted 56 lines · Axel Lin

2014-04-05 20:48 GMT+08:00 Austin, Brian <[email protected]>:
Quoted 38 lines · Axel Lin
2014-04-05 1:00 GMT+08:00 Brian Austin <[email protected]>:
On Fri, 4 Apr 2014, Axel Lin wrote:
      ret = regmap_read(cs42l52->regmap, CS42L52_CHIP, &reg);
sound/soc/codecs/cs42l52.c +1 -1
-       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.
Quoted 8 lines
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.

Quoted 8 lines

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.

Quoted 50 lines

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,

sound/soc/codecs/cs42l52.c +1 -1

@@ -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)

sound/soc/codecs/cs42l52.h +2 -2

-#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

sound/soc/codecs/cs42l52.h +1 -1

-#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).

Quoted 6 lines

Quoted 3 lines
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.
Quoted 1 line

Thanks!

Quoted 2 lines

Quoted 2 lines
Thanks