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

[Austin, Brian]

On Apr 4, 2014, at 8:59 PM, Axel Lin <axel.lin at ingics.com> wrote:

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

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 496 bytes
Desc: Message signed with OpenPGP using GPGMail
URL: <http://mailman.alsa-project.org/pipermail/alsa-devel/attachments/20140405/1f9c4223/attachment.sig>