Re: [PATCH v5] iio: add ad5761 DAC driver

[Jonathan Cameron <jic23@kernel.org>]

On 11/01/16 16:23, Ricardo Ribalda Delgado wrote:
> Hello Jonathan
> 
> Thanks for your review
> 
> On Sat, Jan 9, 2016 at 5:15 PM, Jonathan Cameron <jic23@kernel.org> wrote:
>> I'm a little unsure that we shouldn't just fail the probe if the
>> range is supplied.  Even the default (the best option available)
>> could in theory do damage to a circuit with a maximum of 3V though
>> it's probably unlikely...
> 
> I personally hate when a driver needs a mandatory pdata. I expect a
> default behaviour on the absence of it.
That's fair enough, though in this particular case, in theory it could
cause damage.  Let's take the view that's unlikely.
> 
>>> +     st->vref_reg = devm_regulator_get_optional(&st->spi->dev, "vref");
>>> +     if (!st->vref_reg || PTR_ERR(st->vref_reg) == -ENODEV) {
>> This is a little unusual... Only one of those errors is returned by
>> devm_regulator_get_optional if the regulator is not specified.
>> I believe from a quick look at the regulator code that it returns the
>> -ENODEV part.
>>
>> So how can it be null?
>>
> 
> After a fast look:
> 
> devm_regulator_get_optional-> _devm_regulator_get -> regulator_get
> (regulator/consumer.h)
> 
> Probably consumer.h cannot be reached at the same time than
> _devm_regulator_get. But for safety I rather leave the check.
Hmm. In this particular case it's relatively clear that one of
the checks must be meaningless so chances of patches turning up
to 'fix' this in future makes me think it's better to save time
and drop it now.
> 
> 
> 
> I send v6 right away.
> 
> 
> Thanks!
> 
> 
>