Re: [PATCH v2] iio/axp288_adc: add missing channel info mask

From: Jonathan Cameron
Date: Sun Apr 12 2015 - 12:11:51 EST

On 10/04/15 10:58, Jacob Pan wrote:
> On Thu, 09 Apr 2015 18:52:12 +0100
> Jonathan Cameron <jic23@xxxxxxxxxx> wrote:
>
>> On 09/04/15 18:50, Jonathan Cameron wrote:
>>> On 09/04/15 18:02, Jacob Pan wrote:
>>>> On Thu, 09 Apr 2015 14:18:49 +0100
>>>> Jonathan Cameron <jic23@xxxxxxxxxx> wrote:
>>>>
>>>>> On 06/04/15 19:38, Jacob Pan wrote:
>>>>>> Commit 65de7654d39c70c2b ("iio: iio: Fix iio_channel_read return
>>>>>> if channel havn't info") added a check for valid info masks.
>>>>>>
>>>>>> This patch adds missing channel info masks for all ADC channels.
>>>>>> Otherwise, iio_read_channel_raw() would return -EINVAL when
>>>>>> called by consumer drivers.
>>>>>>
>>>>>> Signed-off-by: Jacob Pan <jacob.jun.pan@xxxxxxxxxxxxxxx>
>>>>>
>>>>> This makes me a little nervous. Why did we have the separate raw
>>>>> and processed paths in the read_raw function in the first place?
>>>>>
>>>> Good point, I failed to explain that the reason to have separate
>>>> raw and processed was that the raw channels happened to need some
>>>> special processing in axp288_adc_set_ts(), since I have added a
>>>> check for address:
>>>> /* channels other than GPADC do not need to switch TS pin
>>>> */ if (address != AXP288_GP_ADC_H)
>>>> return 0;
>>>>
>>>> we no longer need special case the channels based on RAW flag.
>>>> Thinking back it was kind of an abuse.
>>>>
>>>>> Also this changes the exposed userspace ABI by dropping those
>>>>> processed attributes that already exist..
>>>>>
>>>> I don't know the current users outside what we develop internally.
>>>> What is your suggestion?
>>> Lets go for it on the basis of it is 'fixing' your abuse of the
>>> ABI ;) Then any users should long ago have complained about it
>>> anyway (did I say that?) We'll take the age old approach. If no one
>>> notices we broke the ABI then it's fine.
>>>
>>> Have applied to the fixes-togreg branch of iio.git with the
>>> description updated to indicate it also fixes the ABI abuse ;)
>>>
>>> Anyhow, won't go now until after the merge window anyway I'm afraid.
>>> Marked for stable as well.
>>>
>> Forgot to say. If it causes any breakage, we simply put back the
>> incorrect processed version as well. That might then cause breakage
>> for any in kernel consumers however as they will use that in
>> preference to the raw version IIRC.
>>> Thanks,
>>>
> Good to know. Thanks a lot.
> Perhaps I missed it in the kernel, I wish there were more
> comments/documentation on the enum to explain its use case for hw
> provided data format, and implication for sysfs abi etc.
>
> enum iio_chan_info_enum {
> IIO_CHAN_INFO_RAW = 0,
> IIO_CHAN_INFO_PROCESSED,
> ...
Hmm. More docs almost always good, sadly no one ever gets
round to keeping them up to date.

The sysfs side of things fairly neatly lines up with the ABI
docs, but I guess we could have more examples. Example wise
we do have the dummy driver to illustrate most of the usage.
/Documentation/ABI/testing/sysfs-bus-iio
/drivers/staging/iio/simple_dummy*

J
>
>
> }
>>> Jonathan
>>>>
>>>>> Jonathan
>>>>>> ---
>>>>>> drivers/iio/adc/axp288_adc.c | 12 ++++++------
>>>>>> 1 file changed, 6 insertions(+), 6 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/iio/adc/axp288_adc.c
>>>>>> b/drivers/iio/adc/axp288_adc.c index 08bcfb0..56008a8 100644
>>>>>> --- a/drivers/iio/adc/axp288_adc.c
>>>>>> +++ b/drivers/iio/adc/axp288_adc.c
>>>>>> @@ -53,39 +53,42 @@ static const struct iio_chan_spec const
>>>>>> axp288_adc_channels[] = { .channel = 0,
>>>>>> .address = AXP288_TS_ADC_H,
>>>>>> .datasheet_name = "TS_PIN",
>>>>>> + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
>>>>>> }, {
>>>>>> .indexed = 1,
>>>>>> .type = IIO_TEMP,
>>>>>> .channel = 1,
>>>>>> .address = AXP288_PMIC_ADC_H,
>>>>>> .datasheet_name = "PMIC_TEMP",
>>>>>> + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
>>>>>> }, {
>>>>>> .indexed = 1,
>>>>>> .type = IIO_TEMP,
>>>>>> .channel = 2,
>>>>>> .address = AXP288_GP_ADC_H,
>>>>>> .datasheet_name = "GPADC",
>>>>>> + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
>>>>>> }, {
>>>>>> .indexed = 1,
>>>>>> .type = IIO_CURRENT,
>>>>>> .channel = 3,
>>>>>> .address = AXP20X_BATT_CHRG_I_H,
>>>>>> .datasheet_name = "BATT_CHG_I",
>>>>>> - .info_mask_separate =
>>>>>> BIT(IIO_CHAN_INFO_PROCESSED),
>>>>>> + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
>>>>>> }, {
>>>>>> .indexed = 1,
>>>>>> .type = IIO_CURRENT,
>>>>>> .channel = 4,
>>>>>> .address = AXP20X_BATT_DISCHRG_I_H,
>>>>>> .datasheet_name = "BATT_DISCHRG_I",
>>>>>> - .info_mask_separate =
>>>>>> BIT(IIO_CHAN_INFO_PROCESSED),
>>>>>> + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
>>>>>> }, {
>>>>>> .indexed = 1,
>>>>>> .type = IIO_VOLTAGE,
>>>>>> .channel = 5,
>>>>>> .address = AXP20X_BATT_V_H,
>>>>>> .datasheet_name = "BATT_V",
>>>>>> - .info_mask_separate =
>>>>>> BIT(IIO_CHAN_INFO_PROCESSED),
>>>>>> + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
>>>>>> },
>>>>>> };
>>>>>>
>>>>>> @@ -151,9 +154,6 @@ static int axp288_adc_read_raw(struct iio_dev
>>>>>> *indio_dev, chan->address))
>>>>>> dev_err(&indio_dev->dev, "TS pin
>>>>>> restore\n"); break;
>>>>>> - case IIO_CHAN_INFO_PROCESSED:
>>>>>> - ret = axp288_adc_read_channel(val,
>>>>>> chan->address, info->regmap);
>>>>>> - break;
>>>>>> default:
>>>>>> ret = -EINVAL;
>>>>>> }
>>>>>>
>>>>>
>>>>
>>>> [Jacob Pan]
>>>> --
>>>> To unsubscribe from this list: send the line "unsubscribe
>>>> linux-iio" in the body of a message to majordomo@xxxxxxxxxxxxxxx
>>>> 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@xxxxxxxxxxxxxxx
>>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>>>
>>
>
>
>
>

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/