[PATCH 0/2] Staging: iio: cdc: Use BIT macro and remove dev_info

[PATCH 0/2] Staging: iio: cdc: Use BIT macro and remove dev_info

[Shraddha Barke]

These patches use the BIT macro and remove dev_info

Shraddha Barke (2):
  Staging: iio: cdc: Prefer using the BIT macro
  Staging: iio: cdc: Remove unnecessary dev_info

 drivers/staging/iio/cdc/ad7746.c | 14 ++++++--------
 1 file changed, 6 insertions(+), 8 deletions(-)

-- 
2.1.4

[PATCH 1/2] Staging: iio: cdc: Prefer using the BIT macro

[Shraddha Barke]

Replace bit shifting on 1 with the BIT(x) macro

Signed-off-by: Shraddha Barke <shraddha.6596@gmail.com>
---
 drivers/staging/iio/cdc/ad7746.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/staging/iio/cdc/ad7746.c b/drivers/staging/iio/cdc/ad7746.c
index 10fa372..55cc8bb 100644
--- a/drivers/staging/iio/cdc/ad7746.c
+++ b/drivers/staging/iio/cdc/ad7746.c
@@ -68,12 +68,12 @@
 #define AD7746_VTSETUP_VTCHOP		(1 << 0)
 
 /* Excitation Setup Register Bit Designations (AD7746_REG_EXC_SETUP) */
-#define AD7746_EXCSETUP_CLKCTRL		(1 << 7)
-#define AD7746_EXCSETUP_EXCON		(1 << 6)
-#define AD7746_EXCSETUP_EXCB		(1 << 5)
-#define AD7746_EXCSETUP_NEXCB		(1 << 4)
-#define AD7746_EXCSETUP_EXCA		(1 << 3)
-#define AD7746_EXCSETUP_NEXCA		(1 << 2)
+#define AD7746_EXCSETUP_CLKCTRL		BIT(7)
+#define AD7746_EXCSETUP_EXCON		BIT(6)
+#define AD7746_EXCSETUP_EXCB		BIT(5)
+#define AD7746_EXCSETUP_NEXCB		BIT(4)
+#define AD7746_EXCSETUP_EXCA		BIT(3)
+#define AD7746_EXCSETUP_NEXCA		BIT(2)
 #define AD7746_EXCSETUP_EXCLVL(x)	(((x) & 0x3) << 0)
 
 /* Config Register Bit Designations (AD7746_REG_CFG) */
-- 
2.1.4

Re: [PATCH 1/2] Staging: iio: cdc: Prefer using the BIT macro

[Lars-Peter Clausen]

On 09/25/2015 07:23 PM, Shraddha Barke wrote:
> Replace bit shifting on 1 with the BIT(x) macro
> 
> Signed-off-by: Shraddha Barke <shraddha.6596@gmail.com>

Acked-by: Lars-Peter Clausen <lars@metafoo.de>

Re: [PATCH 1/2] Staging: iio: cdc: Prefer using the BIT macro

[Jonathan Cameron]

On 26/09/15 18:14, Lars-Peter Clausen wrote:
> On 09/25/2015 07:23 PM, Shraddha Barke wrote:
>> Replace bit shifting on 1 with the BIT(x) macro
>>
>> Signed-off-by: Shraddha Barke <shraddha.6596@gmail.com>
> 
> Acked-by: Lars-Peter Clausen <lars@metafoo.de>
> 
This is a good little patch, so applied to the togreg branch
of iio.git.  This will get initially pushed out as testing
and the branch will get rebased onto Greg KH's staging-next
branch sometime soon.

However, I think there are some other places the BIT macro
can be correctly used in this driver if you are willing to take
it further.

Note that you can only do these by checking the datasheet to be
sure we aren't writing part of a large field in the register!

In the status register the bottom for bits are single bit fields.


#define AD7746_STATUS_EXCERR		(1 << 3)
#define AD7746_STATUS_RDY		(1 << 2)
#define AD7746_STATUS_RDYVT		(1 << 1)
#define AD7746_STATUS_RDYCAP		(1 << 0)

As are the cap setup register bits

#define AD7746_CAPSETUP_CAPEN		(1 << 7)
#define AD7746_CAPSETUP_CIN2		(1 << 6) /* AD7746 only */
#define AD7746_CAPSETUP_CAPDIFF		(1 << 5)
#define AD7746_CAPSETUP_CACHOP		(1 << 0)

Some of the vt setup register are also single bits but as that register
is a more complex mess and some other bits are hard to define macros
for perhaps leave it alone.  It's another one arguing in favour of
a ZERO(bit) macro but I fear persuading people of that will be an uphill
struggle :(

Anyhow the two registers mentioned above are a clear cut as the one you've
done so perhaps tidy those up as well?

Thanks,

Jonathan

Re: [PATCH 1/2] Staging: iio: cdc: Prefer using the BIT macro

[Shraddha Barke]

On Sun, 27 Sep 2015, Jonathan Cameron wrote:

> On 26/09/15 18:14, Lars-Peter Clausen wrote:
>> On 09/25/2015 07:23 PM, Shraddha Barke wrote:
>>> Replace bit shifting on 1 with the BIT(x) macro
>>>
>>> Signed-off-by: Shraddha Barke <shraddha.6596@gmail.com>
>>
>> Acked-by: Lars-Peter Clausen <lars@metafoo.de>
>>
> This is a good little patch, so applied to the togreg branch
> of iio.git.  This will get initially pushed out as testing
> and the branch will get rebased onto Greg KH's staging-next
> branch sometime soon.
>
> However, I think there are some other places the BIT macro
> can be correctly used in this driver if you are willing to take
> it further.
>
> Note that you can only do these by checking the datasheet to be
> sure we aren't writing part of a large field in the register!
>
> In the status register the bottom for bits are single bit fields.
>
>
> #define AD7746_STATUS_EXCERR		(1 << 3)
> #define AD7746_STATUS_RDY		(1 << 2)
> #define AD7746_STATUS_RDYVT		(1 << 1)
> #define AD7746_STATUS_RDYCAP		(1 << 0)
>
> As are the cap setup register bits
>
> #define AD7746_CAPSETUP_CAPEN		(1 << 7)
> #define AD7746_CAPSETUP_CIN2		(1 << 6) /* AD7746 only */
> #define AD7746_CAPSETUP_CAPDIFF		(1 << 5)
> #define AD7746_CAPSETUP_CACHOP		(1 << 0)
>
> Some of the vt setup register are also single bits but as that register
> is a more complex mess and some other bits are hard to define macros
> for perhaps leave it alone.  It's another one arguing in favour of
> a ZERO(bit) macro but I fear persuading people of that will be an uphill
> struggle :(
>
> Anyhow the two registers mentioned above are a clear cut as the one you've
> done so perhaps tidy those up as well?
>
Yes sure. But after checking I found out that AD7746_STATUS_EXCERR macro 
is not used at all. I'm sure there are other such macros in this driver. 
Wouldn't it be better to get rid of them?

Thanks,

Shraddha

> Thanks,
>
> Jonathan
>
>

Re: [PATCH 1/2] Staging: iio: cdc: Prefer using the BIT macro

[Jonathan Cameron]

On 27/09/15 20:39, Shraddha Barke wrote:
> 
> 
> On Sun, 27 Sep 2015, Jonathan Cameron wrote:
> 
>> On 26/09/15 18:14, Lars-Peter Clausen wrote:
>>> On 09/25/2015 07:23 PM, Shraddha Barke wrote:
>>>> Replace bit shifting on 1 with the BIT(x) macro
>>>>
>>>> Signed-off-by: Shraddha Barke <shraddha.6596@gmail.com>
>>>
>>> Acked-by: Lars-Peter Clausen <lars@metafoo.de>
>>>
>> This is a good little patch, so applied to the togreg branch
>> of iio.git.  This will get initially pushed out as testing
>> and the branch will get rebased onto Greg KH's staging-next
>> branch sometime soon.
>>
>> However, I think there are some other places the BIT macro
>> can be correctly used in this driver if you are willing to take
>> it further.
>>
>> Note that you can only do these by checking the datasheet to be
>> sure we aren't writing part of a large field in the register!
>>
>> In the status register the bottom for bits are single bit fields.
>>
>>
>> #define AD7746_STATUS_EXCERR        (1 << 3)
>> #define AD7746_STATUS_RDY        (1 << 2)
>> #define AD7746_STATUS_RDYVT        (1 << 1)
>> #define AD7746_STATUS_RDYCAP        (1 << 0)
>>
>> As are the cap setup register bits
>>
>> #define AD7746_CAPSETUP_CAPEN        (1 << 7)
>> #define AD7746_CAPSETUP_CIN2        (1 << 6) /* AD7746 only */
>> #define AD7746_CAPSETUP_CAPDIFF        (1 << 5)
>> #define AD7746_CAPSETUP_CACHOP        (1 << 0)
>>
>> Some of the vt setup register are also single bits but as that register
>> is a more complex mess and some other bits are hard to define macros
>> for perhaps leave it alone.  It's another one arguing in favour of
>> a ZERO(bit) macro but I fear persuading people of that will be an uphill
>> struggle :(
>>
>> Anyhow the two registers mentioned above are a clear cut as the one you've
>> done so perhaps tidy those up as well?
>> 

> Yes sure. But after checking I found out that AD7746_STATUS_EXCERR
> macro is not used at all. I'm sure there are other such macros in
> this driver. Wouldn't it be better to get rid of them?
In general yes, but always worth taking a quick look at the datasheet
to see if they are something the driver SHOULD be using.

Sometimes it is argued that these act as documentation, but that in
my view is only really valid if the datasheet isn't publicly available.

J

> Thanks,
> 
> Shraddha
> 
>> Thanks,
>>
>> Jonathan
>>
>>
> -- 
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 2/2] Staging: iio: cdc: Remove unnecessary dev_info

[Shraddha Barke]

Remove dev_info as the information can be obtained by other means

Signed-off-by: Shraddha Barke <shraddha.6596@gmail.com>
---
 drivers/staging/iio/cdc/ad7746.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/staging/iio/cdc/ad7746.c b/drivers/staging/iio/cdc/ad7746.c
index 55cc8bb..ca7d33f 100644
--- a/drivers/staging/iio/cdc/ad7746.c
+++ b/drivers/staging/iio/cdc/ad7746.c
@@ -753,8 +753,6 @@ static int ad7746_probe(struct i2c_client *client,
 	if (ret)
 		return ret;
 
-	dev_info(&client->dev, "%s capacitive sensor registered\n", id->name);
-
 	return 0;
 }
 
-- 
2.1.4

Re: [PATCH 2/2] Staging: iio: cdc: Remove unnecessary dev_info

[Lars-Peter Clausen]

On 09/25/2015 07:23 PM, Shraddha Barke wrote:
> Remove dev_info as the information can be obtained by other means
> 
> Signed-off-by: Shraddha Barke <shraddha.6596@gmail.com>

Acked-by: Lars-Peter Clausen <lars@metafoo.de>

Re: [PATCH 2/2] Staging: iio: cdc: Remove unnecessary dev_info

[Jonathan Cameron]

On 26/09/15 18:14, Lars-Peter Clausen wrote:
> On 09/25/2015 07:23 PM, Shraddha Barke wrote:
>> Remove dev_info as the information can be obtained by other means
>>
>> Signed-off-by: Shraddha Barke <shraddha.6596@gmail.com>
> 
> Acked-by: Lars-Peter Clausen <lars@metafoo.de>
Applied, thanks Shraddha,

Keep up the good work, these drivers have been in staging way too long!

Jonathan
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>