staging:iio:ad799x make use of platform_data optional
Subsystems (4)
PATCH 1/1 staging:iio:ad799x make use of platform_data optional
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.
---
drivers/staging/iio/adc/ad799x_core.c +6 -4
@@ -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)) {
Jonathan Cameron
maintainerQuoted 5 lines · Hartmut Knaack
Setting Vref with
platform_datais a neat feature, though it is not essential for operating these devices. So make the use ofplatform_dataoptional and set default value of 1000 mV if nothing else is defined.Signed-off-by Hartmut Knaack <[email protected]>
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
Quoted 27 lines
---
drivers/staging/iio/adc/ad799x_core.c +6
-5@@ -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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Quoted 7 lines · Jonathan Cameron
Quoted 5 lines · Hartmut Knaack
Setting Vref with
platform_datais a neat feature, though it is not essential for operating these devices. So make the use ofplatform_dataoptional and set default value of 1000 mV if nothing else is defined.Signed-off-by Hartmut Knaack <[email protected]>Hi Hartmut,
Hi
Quoted 2 lines
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.
Quoted 8 lines
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).
Quoted 36 lines
Jonathan
Quoted 23 lines
---
drivers/staging/iio/adc/ad799x_core.c +6
-5@@ -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)) { --Quoted 4 lines
To unsubscribe from this list: send the line "unsubscribe linux-iio" in the body of a message to [email protected]
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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Jonathan Cameron
maintainerQuoted 9 lines · Hartmut Knaack
Quoted 8 lines · Jonathan Cameron
Quoted 6 lines · Hartmut Knaack
Setting Vref with
platform_datais a neat feature, though it is not essential for operating these devices. So make the use ofplatform_dataoptional and set default value of 1000 mV if nothing else is defined.Signed-off-by Hartmut Knaack <[email protected]>Hi Hartmut,
Quoted 8 lines
Hi
Quoted 3 lines
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.
Quoted 6 lines
Quoted 6 lines
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.
Quoted 6 lines
Quoted 2 lines
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
Quoted 38 lines
Quoted 20 lines
Quoted 20 lines
---
drivers/staging/iio/adc/ad799x_core.c +6
-4@@ -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;Quoted 12 lines
Quoted 7 lines
drivers/staging/iio/adc/ad799x_core.c +0
-1st->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 [email protected]
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 [email protected]
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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Quoted 16 lines · Jonathan Cameron
Quoted 13 lines · Hartmut Knaack
Quoted 8 lines · Jonathan Cameron
Quoted 6 lines · Hartmut Knaack
Setting Vref with
platform_datais a neat feature, though it is not essential for operating these devices. So make the use ofplatform_dataoptional and set default value of 1000 mV if nothing else is defined.Signed-off-by Hartmut Knaack <[email protected]>Hi Hartmut,
Hi
Quoted 3 lines
Why a default of 1V? Does that correspond to anything in particular?
Quoted 7 lines
Quoted 4 lines
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
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
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.
Quoted 8 lines
Quoted 6 lines
Quoted 2 lines
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
Quoted 22 lines · Lars-Peter Clausen
Quoted 14 lines · Jonathan Cameron
Quoted 12 lines · Hartmut Knaack
Quoted 8 lines · Jonathan Cameron
Quoted 6 lines · Hartmut Knaack
Setting Vref with
platform_datais a neat feature, though it is not essential for operating these devices. So make the use ofplatform_dataoptional and set default value of 1000 mV if nothing else is defined.Signed-off-by Hartmut Knaack <[email protected]>Hi Hartmut,
Hi
Quoted 2 lines
Why a default of 1V? Does that correspond to anything in particular?
Quoted 7 lines
Quoted 4 lines
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.
Quoted 6 lines
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
Quoted 21 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
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.
Quoted 8 lines
Quoted 6 lines
Quoted 2 lines
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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html