[prev in list] [next in list] [prev in thread] [next in thread] 

List:       linux-iio
Subject:    Re: [PATCH 1/2] Staging: iio: cdc: Prefer using the BIT macro
From:       Jonathan Cameron <jic23 () kernel ! org>
Date:       2015-09-29 17:36:38
Message-ID: 560ACC26.4090403 () kernel ! org
[Download RAW message or body]

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

--
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
[prev in list] [next in list] [prev in thread] [next in thread]