On 12/23/20 1:08 PM, Linus Walleij wrote:
> On Mon, Dec 21, 2020 at 5:15 PM Guenter Roeck <linux@xxxxxxxxxxxx> wrote:
>
>>> - ret = iio_convert_raw_to_processed(channel, raw, &uv, 1000);
>>> - if (ret < 0) {
>>> - /* Assume 12 bit ADC with vref at pullup_uv */
>>> - uv = (pdata->pullup_uv * (s64)raw) >> 12;
>>> + /*
>>> + * FIXME: This fallback to using a raw read and then right
>>> + * out assume the ADC is 12 bits and hard-coding scale
>>> + * to 1000 seems a bit dangerous. Should it simply be
>>> + * deleted?
>>> + */
>>
>> The hwmon ABI specifically supports unscaled values, which can then be
>> scaled in userspace using the sensors configuration file.
>> Given that we return the pseudo-scaled value to userspace today,
>> it seems to me that it would do more harm to change that instead of just
>> leaving it in place.
>
> I see.
>
> I tried to drill down and see the history of the driver and in the
> original commit all values are scaled with the function
> get_temp_mC() which indicates that the driver has always
> intended to return millicentigrades, not unscaled values (as
> far as I can tell).
>
Sure. That isn't the problem. I didn't say that the value reported
to userspace _shall_ be unscaled, I said that the ABI supports it.
We are not discussing your patch here, just the comment. You don't
answer the basic question: What should the driver do if the iio
subsystem only delivers raw values ? I suggested we should just keep
the current assumption in this case, ie do the fixed conversion,
and let userspace handle any necessary adjustments. You seem to object.
With your patch, we get a comment in the driver suggesting that some
code should be removed. In my opinion, such a comment comment does
not add any value. Ether we drop the code or we don't, and I dislike
removing it.
That results in my usual fallback: When in doubt, do nothing.
Guenter
