* [PATCH v2] iio/axp288_adc: add missing channel info mask
@ 2015-04-06 18:38 Jacob Pan
2015-04-09 13:18 ` Jonathan Cameron
0 siblings, 1 reply; 7+ messages in thread
From: Jacob Pan @ 2015-04-06 18:38 UTC (permalink / raw)
To: IIO, Jonathan Cameron
Cc: LKML, Lee Jones, Lars-Peter Clausen, Hartmut Knaack, Aaron Lu,
Todd Brandt, Jacob Pan
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@linux.intel.com>
---
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;
}
--
1.9.1
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH v2] iio/axp288_adc: add missing channel info mask
2015-04-06 18:38 [PATCH v2] iio/axp288_adc: add missing channel info mask Jacob Pan
@ 2015-04-09 13:18 ` Jonathan Cameron
2015-04-09 17:02 ` Jacob Pan
0 siblings, 1 reply; 7+ messages in thread
From: Jonathan Cameron @ 2015-04-09 13:18 UTC (permalink / raw)
To: Jacob Pan, IIO
Cc: LKML, Lee Jones, Lars-Peter Clausen, Hartmut Knaack, Aaron Lu,
Todd Brandt
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@linux.intel.com>
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?
Also this changes the exposed userspace ABI by dropping those processed
attributes that already exist..
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;
> }
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2] iio/axp288_adc: add missing channel info mask
2015-04-09 13:18 ` Jonathan Cameron
@ 2015-04-09 17:02 ` Jacob Pan
2015-04-09 17:50 ` Jonathan Cameron
0 siblings, 1 reply; 7+ messages in thread
From: Jacob Pan @ 2015-04-09 17:02 UTC (permalink / raw)
To: Jonathan Cameron
Cc: IIO, LKML, Lee Jones, Lars-Peter Clausen, Hartmut Knaack,
Aaron Lu, Todd Brandt
On Thu, 09 Apr 2015 14:18:49 +0100
Jonathan Cameron <jic23@kernel.org> 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@linux.intel.com>
>
> 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?
> 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]
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2] iio/axp288_adc: add missing channel info mask
2015-04-09 17:02 ` Jacob Pan
@ 2015-04-09 17:50 ` Jonathan Cameron
2015-04-09 17:52 ` Jonathan Cameron
0 siblings, 1 reply; 7+ messages in thread
From: Jonathan Cameron @ 2015-04-09 17:50 UTC (permalink / raw)
To: Jacob Pan
Cc: IIO, LKML, Lee Jones, Lars-Peter Clausen, Hartmut Knaack,
Aaron Lu, Todd Brandt
On 09/04/15 18:02, Jacob Pan wrote:
> On Thu, 09 Apr 2015 14:18:49 +0100
> Jonathan Cameron <jic23@kernel.org> 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@linux.intel.com>
>>
>> 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.
Thanks,
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@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2] iio/axp288_adc: add missing channel info mask
2015-04-09 17:50 ` Jonathan Cameron
@ 2015-04-09 17:52 ` Jonathan Cameron
2015-04-10 9:58 ` Jacob Pan
0 siblings, 1 reply; 7+ messages in thread
From: Jonathan Cameron @ 2015-04-09 17:52 UTC (permalink / raw)
To: Jacob Pan
Cc: IIO, LKML, Lee Jones, Lars-Peter Clausen, Hartmut Knaack,
Aaron Lu, Todd Brandt
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@kernel.org> 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@linux.intel.com>
>>>
>>> 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,
>
> 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@vger.kernel.org
>> 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@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2] iio/axp288_adc: add missing channel info mask
2015-04-09 17:52 ` Jonathan Cameron
@ 2015-04-10 9:58 ` Jacob Pan
2015-04-12 16:11 ` Jonathan Cameron
0 siblings, 1 reply; 7+ messages in thread
From: Jacob Pan @ 2015-04-10 9:58 UTC (permalink / raw)
To: Jonathan Cameron
Cc: IIO, LKML, Lee Jones, Lars-Peter Clausen, Hartmut Knaack,
Aaron Lu, Todd Brandt
On Thu, 09 Apr 2015 18:52:12 +0100
Jonathan Cameron <jic23@kernel.org> 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@kernel.org> 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@linux.intel.com>
> >>>
> >>> 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,
...
}
> > 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@vger.kernel.org
> >> 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@vger.kernel.org
> > More majordomo info at http://vger.kernel.org/majordomo-info.html
> >
>
--
Jacob Pan
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2] iio/axp288_adc: add missing channel info mask
2015-04-10 9:58 ` Jacob Pan
@ 2015-04-12 16:11 ` Jonathan Cameron
0 siblings, 0 replies; 7+ messages in thread
From: Jonathan Cameron @ 2015-04-12 16:11 UTC (permalink / raw)
To: Jacob Pan
Cc: IIO, LKML, Lee Jones, Lars-Peter Clausen, Hartmut Knaack,
Aaron Lu, Todd Brandt
On 10/04/15 10:58, Jacob Pan wrote:
> On Thu, 09 Apr 2015 18:52:12 +0100
> Jonathan Cameron <jic23@kernel.org> 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@kernel.org> 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@linux.intel.com>
>>>>>
>>>>> 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@vger.kernel.org
>>>> 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@vger.kernel.org
>>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>>>
>>
>
>
>
>
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2015-04-12 16:11 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-04-06 18:38 [PATCH v2] iio/axp288_adc: add missing channel info mask Jacob Pan
2015-04-09 13:18 ` Jonathan Cameron
2015-04-09 17:02 ` Jacob Pan
2015-04-09 17:50 ` Jonathan Cameron
2015-04-09 17:52 ` Jonathan Cameron
2015-04-10 9:58 ` Jacob Pan
2015-04-12 16:11 ` Jonathan Cameron
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.