Re: [PATCH v3 1/3] iio: adc: Cosmic Circuits 10001 ADC driver

[Ezequiel Garcia <ezequiel.garcia@imgtec.com>]

On 11/25/2014 07:53 PM, Hartmut Knaack wrote:
> Ezequiel Garcia schrieb am 25.11.2014 23:03:
>>
>>
>> On 11/25/2014 06:41 PM, Hartmut Knaack wrote:
>>>>
>>>> Unless I'm missing something, that's exactly what XOR does.
>>>>
>>>> Example:
>>>>
>>>> reserved_channels = 0x2;
>>>> channel_map = 0x7 ^ reserved_channels -> 0x5
>>>>
>>> You miss to check ret for a valid range, which you don't need to do when masking out channels.
>>> Example:
>>> reserved_channels = 0x8;
>>> channel_map = 0x7 ^ reserved_channels -> 0xf
>>> And this is actually happening in the current revision (with the wrong channel_map assignment), that a reserved channel in DT would become usable.
>>
>> Right, definitely a check is needed.
> No, that's not needed when using AND, as it can not add bits in the channel map. The following will do the job:
> 		adc_dev->channel_map &= ~ret;

Gah, of course, that's way much better.

>>
>>>>>> +
>>>>>> +	adc_dev->reg = devm_regulator_get(&pdev->dev, "vref");
>>>>>> +	if (IS_ERR_OR_NULL(adc_dev->reg))
>>>>>> +		return -EINVAL;
>>>>> 	if (IS_ERR(adc_dev->reg))
>>>>> 		return PTR_ERR(adc_dev->reg);
>>>>
>>>> Are you sure? What if devm_regulator_get() returns NULL?
>>>>
>>> I had a look at it, but couldn't figure out a condition in which it would return NULL. But I'd be happy to be informed of anything different.
>>
>> If CONFIG_REGULATOR=n you get NULL there (although I added a select REGULATOR
>> on the v4 I just posted).
>>
> Indeed, thanks. Now I'm thinking if it would make sense to handle this special case in a special way (as it would be caused by an invalid kernel config). Well, I guess, I leave it up to you.

Well, adding the select REGULATOR that shouldn't happen, so let's better
drop the NULL check entirely.
-- 
Ezequiel