[alsa-devel] [PATCH] ASoC: cs42l52: Report correct chip id and revision

[Axel Lin]

2014-04-05 1:00 GMT+08:00 Brian Austin <brian.austin at cirrus.com>:
> On Fri, 4 Apr 2014, Axel Lin wrote:
>
>>
>>         ret = regmap_read(cs42l52->regmap, CS42L52_CHIP, &reg);
>> -       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