* [PATCH]staging:iio:ad799x make use of platform_data optional
@ 2013-12-29 11:49 Hartmut Knaack
2014-01-01 12:55 ` Jonathan Cameron
0 siblings, 1 reply; 6+ messages in thread
From: Hartmut Knaack @ 2013-12-29 11:49 UTC (permalink / raw)
To: linux-iio
Setting Vref with platform_data is a neat feature, though it is not essential
for operating these devices. So make the use of platform_data optional and set
default value of 1000 mV if nothing else is defined.
Signed-off-by: Hartmut Knaack <knaack.h@gmx.de>
---
diff --git a/drivers/staging/iio/adc/ad799x_core.c b/drivers/staging/iio/adc/ad799x_core.c
index 9428be8..e32a555 100644
--- a/drivers/staging/iio/adc/ad799x_core.c
+++ b/drivers/staging/iio/adc/ad799x_core.c
@@ -545,10 +545,12 @@ static int ad799x_probe(struct i2c_client *client,
/* TODO: Add pdata options for filtering and bit delay */
- if (!pdata)
- return -EINVAL;
-
- st->int_vref_mv = pdata->vref_mv;
+ if ((!pdata) || (!pdata->vref_mv)) {
+ dev_warn(&client->dev,
+ "No platform data found, setting Vref to 1000 mV\n");
+ st->int_vref_mv = 1000;
+ } else
+ st->int_vref_mv = pdata->vref_mv;
st->reg = devm_regulator_get(&client->dev, "vcc");
if (!IS_ERR(st->reg)) {
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH]staging:iio:ad799x make use of platform_data optional
2013-12-29 11:49 [PATCH]staging:iio:ad799x make use of platform_data optional Hartmut Knaack
@ 2014-01-01 12:55 ` Jonathan Cameron
2014-01-01 13:17 ` Hartmut Knaack
0 siblings, 1 reply; 6+ messages in thread
From: Jonathan Cameron @ 2014-01-01 12:55 UTC (permalink / raw)
To: Hartmut Knaack, linux-iio, Lars-Peter Clausen
On 29/12/13 11:49, Hartmut Knaack wrote:
> Setting Vref with platform_data is a neat feature, though it is not essential
> for operating these devices. So make the use of platform_data optional and set
> default value of 1000 mV if nothing else is defined.
>
> Signed-off-by: Hartmut Knaack <knaack.h@gmx.de>
Hi Hartmut,
Why a default of 1V? Does that correspond to anything in particular?
Whilst the way this is set here is clunky there is a need for this
voltage to be supplied in some fashion. Now we'd do it via a regulator
to give us nice standard device tree bindings and to allow for less
simplistic hardware configurations.
So lets say we convert this to use a regulator, is there still a reason
why one might want a default value?
Jonathan
> ---
> diff --git a/drivers/staging/iio/adc/ad799x_core.c b/drivers/staging/iio/adc/ad799x_core.c
> index 9428be8..e32a555 100644
> --- a/drivers/staging/iio/adc/ad799x_core.c
> +++ b/drivers/staging/iio/adc/ad799x_core.c
> @@ -545,10 +545,12 @@ static int ad799x_probe(struct i2c_client *client,
>
> /* TODO: Add pdata options for filtering and bit delay */
>
> - if (!pdata)
> - return -EINVAL;
> -
> - st->int_vref_mv = pdata->vref_mv;
> + if ((!pdata) || (!pdata->vref_mv)) {
> + dev_warn(&client->dev,
> + "No platform data found, setting Vref to 1000 mV\n");
> + st->int_vref_mv = 1000;
> + } else
> + st->int_vref_mv = pdata->vref_mv;
>
> st->reg = devm_regulator_get(&client->dev, "vcc");
> if (!IS_ERR(st->reg)) {
> --
> 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] 6+ messages in thread* Re: [PATCH]staging:iio:ad799x make use of platform_data optional
2014-01-01 12:55 ` Jonathan Cameron
@ 2014-01-01 13:17 ` Hartmut Knaack
2014-01-01 14:14 ` Jonathan Cameron
0 siblings, 1 reply; 6+ messages in thread
From: Hartmut Knaack @ 2014-01-01 13:17 UTC (permalink / raw)
To: Jonathan Cameron, linux-iio, Lars-Peter Clausen
Jonathan Cameron schrieb:
> On 29/12/13 11:49, Hartmut Knaack wrote:
>> Setting Vref with platform_data is a neat feature, though it is not essential
>> for operating these devices. So make the use of platform_data optional and set
>> default value of 1000 mV if nothing else is defined.
>>
>> Signed-off-by: Hartmut Knaack <knaack.h@gmx.de>
> Hi Hartmut,
Hi
>
> Why a default of 1V? Does that correspond to anything in particular?
This makes it easier to calculate your real voltage, since you only need to multiply with your reference voltage. Another reason: if you are more interested in the relation of your input in the range [0V-Vref], you can simply get the per mill value.
>
> Whilst the way this is set here is clunky there is a need for this
> voltage to be supplied in some fashion. Now we'd do it via a regulator
> to give us nice standard device tree bindings and to allow for less
> simplistic hardware configurations.
>
> So lets say we convert this to use a regulator, is there still a reason
> why one might want a default value?
I'm not aware of the current state of the device tree implementation at the moment. My intention is to also be able to use this driver without the need of compiling a custom kernel (so to use the distribution of choice out of the box).
>
> Jonathan
>
>
>> ---
>> diff --git a/drivers/staging/iio/adc/ad799x_core.c b/drivers/staging/iio/adc/ad799x_core.c
>> index 9428be8..e32a555 100644
>> --- a/drivers/staging/iio/adc/ad799x_core.c
>> +++ b/drivers/staging/iio/adc/ad799x_core.c
>> @@ -545,10 +545,12 @@ static int ad799x_probe(struct i2c_client *client,
>>
>> /* TODO: Add pdata options for filtering and bit delay */
>>
>> - if (!pdata)
>> - return -EINVAL;
>> -
>> - st->int_vref_mv = pdata->vref_mv;
>> + if ((!pdata) || (!pdata->vref_mv)) {
>> + dev_warn(&client->dev,
>> + "No platform data found, setting Vref to 1000 mV\n");
>> + st->int_vref_mv = 1000;
>> + } else
>> + st->int_vref_mv = pdata->vref_mv;
>>
>> st->reg = devm_regulator_get(&client->dev, "vcc");
>> if (!IS_ERR(st->reg)) {
>> --
>> 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] 6+ messages in thread* Re: [PATCH]staging:iio:ad799x make use of platform_data optional
2014-01-01 13:17 ` Hartmut Knaack
@ 2014-01-01 14:14 ` Jonathan Cameron
2014-01-01 15:37 ` Lars-Peter Clausen
0 siblings, 1 reply; 6+ messages in thread
From: Jonathan Cameron @ 2014-01-01 14:14 UTC (permalink / raw)
To: Hartmut Knaack, linux-iio, Lars-Peter Clausen
On 01/01/14 13:17, Hartmut Knaack wrote:
> Jonathan Cameron schrieb:
>> On 29/12/13 11:49, Hartmut Knaack wrote:
>>> Setting Vref with platform_data is a neat feature, though it is
>>> not essential for operating these devices. So make the use of
>>> platform_data optional and set default value of 1000 mV if
>>> nothing else is defined.
>>>
>>> Signed-off-by: Hartmut Knaack <knaack.h@gmx.de>
>> Hi Hartmut,
> Hi
>>
>> Why a default of 1V? Does that correspond to anything in
>> particular?
> This makes it easier to calculate your real voltage, since you only
> need to multiply with your reference voltage. Another reason: if you
> are more interested in the relation of your input in the range
> [0V-Vref], you can simply get the per mill value.
Sorry, but I'm not going to take this as whilst I can see how it
is useful for your use case, it would introduce a new non standard
interface.
>>
>> Whilst the way this is set here is clunky there is a need for this
>> voltage to be supplied in some fashion. Now we'd do it via a
>> regulator to give us nice standard device tree bindings and to
>> allow for less simplistic hardware configurations.
>>
>> So lets say we convert this to use a regulator, is there still a
>> reason why one might want a default value?
> I'm not aware of the current state of the device tree implementation
> at the moment. My intention is to also be able to use this driver
> without the need of compiling a custom kernel (so to use the
> distribution of choice out of the box).
Then it needs to be device tree enabled and the configuration done
via that.
Jonathan
>>> ---
>>> diff --git a/drivers/staging/iio/adc/ad799x_core.c b/drivers/staging/iio/adc/ad799x_core.c
>>> index 9428be8..e32a555 100644
>>> --- a/drivers/staging/iio/adc/ad799x_core.c
>>> +++ b/drivers/staging/iio/adc/ad799x_core.c
>>> @@ -545,10 +545,12 @@ static int ad799x_probe(struct i2c_client *client,
>>>
>>> /* TODO: Add pdata options for filtering and bit delay */
>>>
>>> - if (!pdata)
>>> - return -EINVAL;
>>> -
>>> - st->int_vref_mv = pdata->vref_mv;
>>> + if ((!pdata) || (!pdata->vref_mv)) {
>>> + dev_warn(&client->dev,
>>> + "No platform data found, setting Vref to 1000 mV\n");
>>> + st->int_vref_mv = 1000;
>>> + } else
>>> + st->int_vref_mv = pdata->vref_mv;
>>>
>>> st->reg = devm_regulator_get(&client->dev, "vcc");
>>> if (!IS_ERR(st->reg)) {
>>> --
>>> 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
>>
>
> --
> 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] 6+ messages in thread* Re: [PATCH]staging:iio:ad799x make use of platform_data optional
2014-01-01 14:14 ` Jonathan Cameron
@ 2014-01-01 15:37 ` Lars-Peter Clausen
2014-01-01 23:19 ` Hartmut Knaack
0 siblings, 1 reply; 6+ messages in thread
From: Lars-Peter Clausen @ 2014-01-01 15:37 UTC (permalink / raw)
To: Jonathan Cameron; +Cc: Hartmut Knaack, linux-iio
On 01/01/2014 03:14 PM, Jonathan Cameron wrote:
>
>
> On 01/01/14 13:17, Hartmut Knaack wrote:
>> Jonathan Cameron schrieb:
>>> On 29/12/13 11:49, Hartmut Knaack wrote:
>>>> Setting Vref with platform_data is a neat feature, though it is
>>>> not essential for operating these devices. So make the use of
>>>> platform_data optional and set default value of 1000 mV if
>>>> nothing else is defined.
>>>>
>>>> Signed-off-by: Hartmut Knaack <knaack.h@gmx.de>
>>> Hi Hartmut,
>> Hi
>>>
>>> Why a default of 1V? Does that correspond to anything in
>>> particular?
>> This makes it easier to calculate your real voltage, since you only
>> need to multiply with your reference voltage. Another reason: if you
>> are more interested in the relation of your input in the range
>> [0V-Vref], you can simply get the per mill value.
> Sorry, but I'm not going to take this as whilst I can see how it
> is useful for your use case, it would introduce a new non standard
> interface.
Yep, this sounds like a hack. It's definitely something that we do not want
to do as it wont work with applications that assume the standard ABI. That
said, the driver should be updated to not allow specifying the reference
voltage via platform data, but only use the regulator API, like we did for
other drivers as well. I have a more or less untested patch for this, see
https://github.com/analogdevicesinc/linux/commit/18a2c9a304fea7360b10c9041995cad122fbff07
>>>
>>> Whilst the way this is set here is clunky there is a need for this
>>> voltage to be supplied in some fashion. Now we'd do it via a
>>> regulator to give us nice standard device tree bindings and to
>>> allow for less simplistic hardware configurations.
>>>
>>> So lets say we convert this to use a regulator, is there still a
>>> reason why one might want a default value?
>> I'm not aware of the current state of the device tree implementation
>> at the moment. My intention is to also be able to use this driver
>> without the need of compiling a custom kernel (so to use the
>> distribution of choice out of the box).
> Then it needs to be device tree enabled and the configuration done
> via that.
I think with the above patch the driver should work fine with devicetree.
- Lars
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH]staging:iio:ad799x make use of platform_data optional
2014-01-01 15:37 ` Lars-Peter Clausen
@ 2014-01-01 23:19 ` Hartmut Knaack
0 siblings, 0 replies; 6+ messages in thread
From: Hartmut Knaack @ 2014-01-01 23:19 UTC (permalink / raw)
To: Lars-Peter Clausen, Jonathan Cameron; +Cc: linux-iio
Lars-Peter Clausen schrieb:
> On 01/01/2014 03:14 PM, Jonathan Cameron wrote:
>>
>> On 01/01/14 13:17, Hartmut Knaack wrote:
>>> Jonathan Cameron schrieb:
>>>> On 29/12/13 11:49, Hartmut Knaack wrote:
>>>>> Setting Vref with platform_data is a neat feature, though it is
>>>>> not essential for operating these devices. So make the use of
>>>>> platform_data optional and set default value of 1000 mV if
>>>>> nothing else is defined.
>>>>>
>>>>> Signed-off-by: Hartmut Knaack <knaack.h@gmx.de>
>>>> Hi Hartmut,
>>> Hi
>>>> Why a default of 1V? Does that correspond to anything in
>>>> particular?
>>> This makes it easier to calculate your real voltage, since you only
>>> need to multiply with your reference voltage. Another reason: if you
>>> are more interested in the relation of your input in the range
>>> [0V-Vref], you can simply get the per mill value.
>> Sorry, but I'm not going to take this as whilst I can see how it
>> is useful for your use case, it would introduce a new non standard
>> interface.
> Yep, this sounds like a hack. It's definitely something that we do not want
> to do as it wont work with applications that assume the standard ABI. That
> said, the driver should be updated to not allow specifying the reference
> voltage via platform data, but only use the regulator API, like we did for
> other drivers as well. I have a more or less untested patch for this, see
> https://github.com/analogdevicesinc/linux/commit/18a2c9a304fea7360b10c9041995cad122fbff07
This attempt looks promising to me.
Yet, at line 553:
+ if (!IS_ERR(st->reg))
+ return PTR_ERR(st->reg);
Since the man-page [1] states: "Returns non-0 value if the /ptr/ is an error. Otherwise 0 if it's not an error." Shouldn't that be done without negation?
[1] http://dev.gentoo.org/~cardoe/man-pages/man9/IS_ERR.9.html
>>>> Whilst the way this is set here is clunky there is a need for this
>>>> voltage to be supplied in some fashion. Now we'd do it via a
>>>> regulator to give us nice standard device tree bindings and to
>>>> allow for less simplistic hardware configurations.
>>>>
>>>> So lets say we convert this to use a regulator, is there still a
>>>> reason why one might want a default value?
>>> I'm not aware of the current state of the device tree implementation
>>> at the moment. My intention is to also be able to use this driver
>>> without the need of compiling a custom kernel (so to use the
>>> distribution of choice out of the box).
>> Then it needs to be device tree enabled and the configuration done
>> via that.
> I think with the above patch the driver should work fine with devicetree.
>
> - Lars
> --
> 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] 6+ messages in thread
end of thread, other threads:[~2014-01-01 23:19 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-12-29 11:49 [PATCH]staging:iio:ad799x make use of platform_data optional Hartmut Knaack
2014-01-01 12:55 ` Jonathan Cameron
2014-01-01 13:17 ` Hartmut Knaack
2014-01-01 14:14 ` Jonathan Cameron
2014-01-01 15:37 ` Lars-Peter Clausen
2014-01-01 23:19 ` Hartmut Knaack
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.