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

[Axel Lin]

2014-04-05 20:48 GMT+08:00 Austin, Brian <Brian.Austin at cirrus.com>:
>
> 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

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
>