Re: [PATCH v2 1/2] iio: adc: ti-ads7950: Allow to use on ACPI platforms

[David Lechner <david@lechnology.com>]

On 08/01/2017 11:41 AM, Andy Shevchenko wrote:
> On Tue, 2017-08-01 at 11:21 -0500, David Lechner wrote:
>> On 08/01/2017 10:45 AM, Andy Shevchenko wrote:
>>>     
>>>>>> +	/* Use hard coded value for reference voltage in ACPI
>>>>>> case */
>>>>>> +	if (ACPI_COMPANION(&spi->dev))
>>>>>> +		st->vref_mv = TI_ADS7950_VA_MV_ACPI_DEFAULT;
>>>>>
>>>>> Instead of checking or ACPI, you could just say "if we have a
>>>>> dummy
>>>>> regulator, then use the default value".
>>>
>>>
>>>> Agreed. Sounds sensible to me.  Hopefully in DT people will
>>>> provide the right regulator, but chances are this won't
>>>> always happen.
>>>
>>> There is no call like
>>> regulator_is_dummy()
>>> (and, looking into the code of regulator framework, can't be)
>>>
>>> Can you elaborate a bit, maybe I'm missing something obvious?
>>>
>>
>> I haven't tested this, but shouldn't regulator_get_voltage() return
>> an
>> error for a dummy regulator? You could use this as your test.
> 
> While it would work it's very fragile.
> 
> _regulator_get_voltage() will return error code even for normal
> regulators if something happened there. And user gets an impression that
> everything is okay while it's not.
> 
> So, I wouldn't go this way.
> 
>>
>> static int ti_ads7950_get_range(struct ti_ads7950_state *st)
>> {
>> 	int vref;
>>
>> 	vref = regulator_get_voltage(st->reg);
>> 	if (vref < 0)
>> -		return vref;
>> +		vref = 2500000;
>>
>> 	vref /= 1000;
>>
>> 	if (st->settings & TI_ADS7950_CR_RANGE_5V)
>> 		vref *= 2;
>>
>> 	return vref;
>> }
>>
> 

OK. There is also devm_regulator_get_optional(). This will return 
ERR_PTR(-ENODEV) instead of a dummy regulator. Then you could add a 
check for this error and set st->reg = NULL if we get -ENODEV. Make the 
enable and disable conditional. And use the default voltage if we don't 
have a regulator.


But this is probably even messier than the original ACPI patch. :-/