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

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

[Axel Lin]

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 <axel.lin@ingics.com>
---
 sound/soc/codecs/cs42l52.c | 4 ++--
 sound/soc/codecs/cs42l52.h | 7 ++++---
 2 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/sound/soc/codecs/cs42l52.c b/sound/soc/codecs/cs42l52.c
index f0ca6be..277df17 100644
--- a/sound/soc/codecs/cs42l52.c
+++ b/sound/soc/codecs/cs42l52.c
@@ -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)
diff --git a/sound/soc/codecs/cs42l52.h b/sound/soc/codecs/cs42l52.h
index 6fb8f00..ce7ce5a 100644
--- a/sound/soc/codecs/cs42l52.h
+++ b/sound/soc/codecs/cs42l52.h
@@ -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
-- 
1.8.3.2

Re: [PATCH] ASoC: cs42l52: Report correct chip id and revision

[Brian Austin]

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.

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

-Brian

Re: [PATCH] ASoC: cs42l52: Report correct chip id and revision

[Brian Austin]

> On Fri, 4 Apr 2014, Axel Lin wrote:
>
>>
>> 	dev_info(&i2c_client->dev, "Cirrus Logic CS42L52, Revision: %02X\n",
>> -			reg & 0xFF);
>> +			reg & CS42L52_CHIP_REV_MASK);

>> #define CS42L52_CHIP_REV_B0			0x02
>> -#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

Re: [PATCH] ASoC: cs42l52: Report correct chip id and revision

[Brian Austin]

On Fri, 4 Apr 2014, Brian Austin wrote:

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

Re: [PATCH] ASoC: cs42l52: Report correct chip id and revision

[Axel Lin]

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, &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

Re: [PATCH] ASoC: cs42l52: Report correct chip id and revision

[Axel Lin]

2014-04-05 5:50 GMT+08:00 Brian Austin <brian.austin@cirrus.com>:
>
>> On Fri, 4 Apr 2014, Axel Lin wrote:
>>
>>>
>>>         dev_info(&i2c_client->dev, "Cirrus Logic CS42L52, Revision:
>>> %02X\n",
>>> -                       reg & 0xFF);
>>> +                       reg & CS42L52_CHIP_REV_MASK);
>
>
>>> #define CS42L52_CHIP_REV_B0                     0x02
>>> -#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

Re: [PATCH] ASoC: cs42l52: Report correct chip id and revision

[Austin, Brian]

[-- Attachment #1.1: Type: text/plain, Size: 1111 bytes --]


On Apr 4, 2014, at 9:04 PM, Axel Lin <axel.lin@ingics.com> wrote:

> 2014-04-05 5:50 GMT+08:00 Brian Austin <brian.austin@cirrus.com>:
>> 
>>> On Fri, 4 Apr 2014, Axel Lin wrote:
>>> 
>>>> 
>>>>        dev_info(&i2c_client->dev, "Cirrus Logic CS42L52, Revision:
>>>> %02X\n",
>>>> -                       reg & 0xFF);
>>>> +                       reg & CS42L52_CHIP_REV_MASK);
>> 
>> 
>>>> #define CS42L52_CHIP_REV_B0                     0x02
>>>> -#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
I missed that last line, sorry about that.

Thanks

[-- Attachment #1.2: Message signed with OpenPGP using GPGMail --]
[-- Type: application/pgp-signature, Size: 496 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH] ASoC: cs42l52: Report correct chip id and revision

[Austin, Brian]

[-- Attachment #1.1: Type: text/plain, Size: 3016 bytes --]


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, &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


[-- Attachment #1.2: Message signed with OpenPGP using GPGMail --]
[-- Type: application/pgp-signature, Size: 496 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH] ASoC: cs42l52: Report correct chip id and revision

[Axel Lin]

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, &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
>

Re: [PATCH] ASoC: cs42l52: Report correct chip id and revision

[Austin, Brian]

> On Apr 5, 2014, at 11:33, "Axel Lin" <axel.lin@ingics.com> wrote:
> 
> 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, &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.
> 
I did not write that driver and will be doing a review of cirrus devices to ensure they are consistent when I have time. 

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

>> Thanks
>>