fixes, cleanup and improvements for m62332
cover fixes, cleanup and improvements for m62332
A set of patches to address some issues I spotted during review.
Hartmut Knaack (6):
iio:dac:m62332: share scale and offset
iio:dac:m62332: shutdown on remove
iio:dac:m62332: use ARRAY_SIZE
iio:dac:m62332: drop unrequired variable
iio:dac:m62332: address some style issues
iio:dac:m62332: use dynamic scale
drivers/iio/dac/m62332.c | 63 +++++++++++++++++++++++++----------------------- 1 file changed, 33 insertions(+), 30 deletions(-)
Signature · 1 line
2.4.6
PATCH 2/6 iio:dac:m62332: shutdown on remove
The regulator framework requests to balance regulator_enable() calls with regulator_disable() calls. To meet this requirement, set channels to 0 on remove, which implies a regulator_disable() call in case that channel was enabled.
--- drivers/iio/dac/m62332.c | 2 ++ 1 file changed, 2 insertions(+)
drivers/iio/dac/m62332.c +2 -0
@@ -243,6 +243,8 @@ static int m62332_remove(struct i2c_client *client)iio_device_unregister (indio_dev);iio_map_array_unregister (indio_dev); +m62332_set_value (indio_dev,0 ,0 ); +m62332_set_value (indio_dev,0 ,1 );return 0 ; }
Signature · 1 line
2.4.6
PATCH 1/6 iio:dac:m62332: share scale and offset
In reply to Hartmut KnaackThis device simply uses its Vcc as reference voltage, so the same scale applies for all channels. Also offset doesn't appear to be different for any channel. Represent this by switching these two attributes to info_mask_shared_by_type.
---
Hope you don't mind too much, that I inserted an extra tab in the whole block rather than messing up style and cleaning it up later.
drivers/iio/dac/m62332.c | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-)
drivers/iio/dac/m62332.c +9 -9
@@ -173,15 +173,15 @@ static const struct iio_info m62332_info = { .driver_module = THIS_MODULE, }; -#define M62332_CHANNEL (chan) { \ - .type = IIO_VOLTAGE, \ - .indexed = 1, \ - .output = 1, \ - .channel = (chan), \ - .datasheet_name = "CH" #chan, \ - .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | \ - BIT(IIO_CHAN_INFO_SCALE) | \ - BIT(IIO_CHAN_INFO_OFFSET), \ +#define M62332_CHANNEL (chan) { \ + .type = IIO_VOLTAGE, \ + .indexed = 1, \ + .output = 1, \ + .channel = (chan), \ + .datasheet_name = "CH" #chan, \ + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \ + .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE) | \ + BIT(IIO_CHAN_INFO_OFFSET), \ }static const struct iio_chan_spec m62332_channels[M62332_CHANNELS ] = {
Signature · 1 line
2.4.6
PATCH 5/6 iio:dac:m62332: address some style issues
In reply to Hartmut KnaackFix some indentation issues and separate returns by empty lines (IIO style). Also rename the channel mask in _read_raw() to mask.
--- drivers/iio/dac/m62332.c | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-)
drivers/iio/dac/m62332.c +11 -5
@@ -40,8 +40,7 @@ struct m62332_data {#endif }; -static int m62332_set_value (struct iio_dev *indio_dev, -u8 val,int channel) +static int m62332_set_value (struct iio_dev *indio_dev,u8 val,int channel) {struct m62332_data *data =iio_priv (indio_dev);struct i2c_client *client = data->client;
@@ -87,30 +86,35 @@ static int m62332_read_raw(struct iio_dev *indio_dev,struct iio_chan_spec const *chan, int *val, int *val2, - long m) + long mask) { struct m62332_data *data = iio_priv(indio_dev); -switch (m) { +switch (mask) {case IIO_CHAN_INFO_SCALE :/* Corresponds to Vref / 2^(bits) */ *val = data->vref_mv; *val2 =8 ; +return IIO_VAL_FRACTIONAL_LOG2 ;case IIO_CHAN_INFO_RAW : *val = data->raw[chan->channel]; +return IIO_VAL_INT ;case IIO_CHAN_INFO_OFFSET : *val =1 ; +return IIO_VAL_INT ;default :break ; } +return -EINVAL ; }static int m62332_write_raw (struct iio_dev *indio_dev, -struct iio_chan_spec const *chan,int val,int val2,long mask) +struct iio_chan_spec const *chan,int val,int val2, +long mask) {switch (mask) {case IIO_CHAN_INFO_RAW :
@@ -195,6 +199,7 @@ static int m62332_probe(struct i2c_client *client, indio_dev =devm_iio_device_alloc (&client->dev,sizeof (*data));if (!indio_dev)return -ENOMEM ; + data =iio_priv (indio_dev);i2c_set_clientdata (client, indio_dev); data->client = client;
@@ -230,6 +235,7 @@ static int m62332_probe(struct i2c_client *client, err:iio_map_array_unregister (indio_dev); +return ret; }
Signature · 1 line
2.4.6
PATCH 3/6 iio:dac:m62332: use ARRAY_SIZE
In reply to Hartmut KnaackMake use of ARRAY_SIZE to prevent buffer issues.
--- drivers/iio/dac/m62332.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
drivers/iio/dac/m62332.c +3 -3
@@ -62,8 +62,8 @@ static int m62332_set_value(struct iio_dev *indio_dev, goto out; } - res =i2c_master_send (client, outbuf,2 ); -if (res >=0 && res !=2 ) + res =i2c_master_send (client, outbuf,ARRAY_SIZE (outbuf)); +if (res >=0 && res !=ARRAY_SIZE (outbuf)) res = -EIO ;if (res <0 ) goto out;
@@ -212,7 +212,7 @@ static int m62332_probe(struct i2c_client *client,/* establish that the iio_dev is a child of the i2c device */ indio_dev->dev.parent = &client->dev; - indio_dev->num_channels =M62332_CHANNELS ; + indio_dev->num_channels =ARRAY_SIZE (m62332_channels); indio_dev->channels = m62332_channels; indio_dev->modes =INDIO_DIRECT_MODE ; indio_dev->info = &m62332_info;
Signature · 1 line
2.4.6
PATCH 6/6 iio:dac:m62332: use dynamic scale
In reply to Hartmut KnaackSome regulators can supply multiple voltages. To take changing voltages into account, the scale needs to be calculated on every read access.
--- drivers/iio/dac/m62332.c | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-)
drivers/iio/dac/m62332.c +6 -7
@@ -31,7 +31,6 @@struct m62332_data {struct i2c_client *client; -u16 vref_mv;struct regulator *vcc;struct mutex mutex;u8 raw[M62332_CHANNELS ];
@@ -89,11 +88,16 @@ static int m62332_read_raw(struct iio_dev *indio_dev, long mask) {struct m62332_data *data =iio_priv (indio_dev); +int ret;switch (mask) {case IIO_CHAN_INFO_SCALE :/* Corresponds to Vref / 2^(bits) */ - *val = data->vref_mv; + ret =regulator_get_voltage (data->vcc); +if (ret <0 ) +return ret; + + *val = ret /1000 ;/* mV */ *val2 =8 ;return IIO_VAL_FRACTIONAL_LOG2 ;
@@ -218,11 +222,6 @@ static int m62332_probe(struct i2c_client *client, indio_dev->modes =INDIO_DIRECT_MODE ; indio_dev->info = &m62332_info; - ret =regulator_get_voltage (data->vcc); -if (ret <0 ) -return ret; - data->vref_mv = ret /1000 ;/* mV */ - ret =iio_map_array_register (indio_dev, client->dev.platform_data);if (ret <0 )return ret;
Signature · 1 line
2.4.6
PATCH 4/6 iio:dac:m62332: drop unrequired variable
In reply to Hartmut KnaackA return variable is not required in _write_raw(), and dropping it reduces complexity, as well.
--- drivers/iio/dac/m62332.c | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-)
drivers/iio/dac/m62332.c +2 -6
@@ -112,21 +112,17 @@ static int m62332_read_raw(struct iio_dev *indio_dev,static int m62332_write_raw (struct iio_dev *indio_dev,struct iio_chan_spec const *chan,int val,int val2,long mask) { -int ret; -switch (mask) {case IIO_CHAN_INFO_RAW :if (val <0 || val >255 )return -EINVAL ; - ret =m62332_set_value (indio_dev, val, chan->channel); -break ; +return m62332_set_value (indio_dev, val, chan->channel);default : - ret = -EINVAL ;break ; } -return ret; +return -EINVAL ; }#ifdef CONFIG_PM_SLEEP
Signature · 1 line
2.4.6
Re: [PATCH 2/6] iio:dac:m62332: shutdown on remove
In reply to Hartmut KnaackQuoted 6 lines · Hartmut Knaack
The regulator framework requests to balance
regulator_enable()calls withregulator_disable()calls. To meet this requirement, set channels to 0 on remove, which implies aregulator_disable()call in case that channel was enabled.Signed-off-by Hartmut Knaack <[email protected]>
Quoted 9 lines
--- drivers/iio/dac/m62332.c | 2 ++ 1 file changed, 2 insertions(+)drivers/iio/dac/m62332.c +0
-0
drivers/iio/dac/m62332.c static int m62332_remove(struct i2c_client *client) · line 250
Quoted 8 lines
drivers/iio/dac/m62332.c +2
-0@@ -243,6 +243,8 @@ static int m62332_remove(struct i2c_client *client)iio_device_unregister (indio_dev);iio_map_array_unregister (indio_dev); +m62332_set_value (indio_dev,0 ,0 ); +m62332_set_value (indio_dev,0 ,1 );return 0 ; }
Wouldn't be nice to factor this two calls in a separate function?
thanks,
Daniel.
Re: [PATCH 3/6] iio:dac:m62332: use ARRAY_SIZE
In reply to Hartmut KnaackQuoted 3 lines · Hartmut Knaack
Make use of
ARRAY_SIZEto prevent buffer issues.Signed-off-by Hartmut Knaack <[email protected]>
Quoted 35 lines
--- drivers/iio/dac/m62332.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)drivers/iio/dac/m62332.c +3
-4@@ -62,8 +62,8 @@ static int m62332_set_value(struct iio_dev *indio_dev, goto out; } - res =i2c_master_send (client, outbuf,2 ); -if (res >=0 && res !=2 ) + res =i2c_master_send (client, outbuf,ARRAY_SIZE (outbuf)); +if (res >=0 && res !=ARRAY_SIZE (outbuf)) res = -EIO ;if (res <0 ) goto out;@@ -212,7 +212,7 @@ static int m62332_probe(struct i2c_client *client,/* establish that the iio_dev is a child of the i2c device */ indio_dev->dev.parent = &client->dev; - indio_dev->num_channels =M62332_CHANNELS ; + indio_dev->num_channels =ARRAY_SIZE (m62332_channels); indio_dev->channels = m62332_channels; indio_dev->modes =INDIO_DIRECT_MODE ; indio_dev->info = &m62332_info; --2.4.6
--
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
maintainerRe: [PATCH 1/6] iio:dac:m62332: share scale and offset
In reply to Hartmut KnaackQuoted 6 lines · Hartmut Knaack
This device simply uses its Vcc as reference voltage, so the same scale applies for all channels. Also offset doesn't appear to be different for any channel. Represent this by switching these two attributes to
info_mask_shared_by_type.Signed-off-by Hartmut Knaack <[email protected]>
This sort of tidy up is always a little interesting. Technically it is an ABI change (be it one that changes from one possible representation to a better one). Unfortunately technically we can't rely on users using a library or similar that would hide this detail for them.
Still we can make the change if no one notices. Perhaps
Dmitry will want to comment on this however, so I'll let it sit for a while longer.
Jonathan
Quoted 37 lines
---
Hope you don't mind too much, that I inserted an extra tab in the whole block rather than messing up style and cleaning it up later.drivers/iio/dac/m62332.c | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-)drivers/iio/dac/m62332.c +9
-9@@ -173,15 +173,15 @@ static const struct iio_info m62332_info = { .driver_module = THIS_MODULE, }; -#define M62332_CHANNEL (chan) { \ - .type = IIO_VOLTAGE, \ - .indexed = 1, \ - .output = 1, \ - .channel = (chan), \ - .datasheet_name = "CH" #chan, \ - .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | \ - BIT(IIO_CHAN_INFO_SCALE) | \ - BIT(IIO_CHAN_INFO_OFFSET), \ +#define M62332_CHANNEL (chan) { \ + .type = IIO_VOLTAGE, \ + .indexed = 1, \ + .output = 1, \ + .channel = (chan), \ + .datasheet_name = "CH" #chan, \ + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \ + .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE) | \ + BIT(IIO_CHAN_INFO_OFFSET), \ }static const struct iio_chan_spec m62332_channels[M62332_CHANNELS ] = {
Jonathan Cameron
maintainerQuoted 10 lines · Hartmut Knaack
A set of patches to address some issues I spotted during review.
Hartmut Knaack (6):
iio:dac:m62332: share scale and offset
iio:dac:m62332: shutdown on remove
iio:dac:m62332: useARRAY_SIZE
iio:dac:m62332: drop unrequired variable
iio:dac:m62332: address some style issues
iio:dac:m62332: use dynamic scale
Quoted 3 lines
drivers/iio/dac/m62332.c | 63 +++++++++++++++++++++++++----------------------- 1 file changed, 33 insertions(+), 30 deletions(-)
Other than letting this series sit on the list a few days longer, for Dmitry or others to take a look at it, I'm happy with this series.
Thanks,
Jonathan
Hello,
2015-08-31 18:00 GMT+03:00 Jonathan Cameron <[email protected]>:
Quoted 11 lines · Hartmut Knaack
Quoted 10 lines
A set of patches to address some issues I spotted during review.
Hartmut Knaack (6):
iio:dac:m62332: share scale and offset
iio:dac:m62332: shutdown on remove
iio:dac:m62332: useARRAY_SIZE
iio:dac:m62332: drop unrequired variable
iio:dac:m62332: address some style issues
iio:dac:m62332: use dynamic scale
Quoted 5 lines
Quoted 3 lines
drivers/iio/dac/m62332.c | 63 +++++++++++++++++++++++++----------------------- 1 file changed, 33 insertions(+), 30 deletions(-)Other than letting this series sit on the list a few days longer, for Dmitry or others to take a look at it, I'm happy with this series.
The changes look good, however I'd like to test them on my hardware first.
Signature · 2 lines
With best wishes Dmitry
Re: [PATCH 2/6] iio:dac:m62332: shutdown on remove
In reply to Daniel BalutaDaniel Baluta schrieb am 31.08.2015 um 14:56:
Quoted 25 lines · Hartmut Knaack
Quoted 6 lines
The regulator framework requests to balance
regulator_enable()calls withregulator_disable()calls. To meet this requirement, set channels to 0 on remove, which implies aregulator_disable()call in case that channel was enabled.Signed-off-by Hartmut Knaack <[email protected]>Acked-by Daniel Baluta <[email protected]>Quoted 15 lines
--- drivers/iio/dac/m62332.c | 2 ++ 1 file changed, 2 insertions(+)drivers/iio/dac/m62332.c +2
-0@@ -243,6 +243,8 @@ static int m62332_remove(struct i2c_client *client)iio_device_unregister (indio_dev);iio_map_array_unregister (indio_dev); +m62332_set_value (indio_dev,0 ,0 ); +m62332_set_value (indio_dev,0 ,1 );
Quoted 5 lines
drivers/iio/dac/m62332.c static int m62332_remove(struct i2c_client *client) · line 250
Quoted 2 lines
drivers/iio/dac/m62332.c +0
-0return 0 ; }Wouldn't be nice to factor this two calls in a separate function?
I thought about factoring out the same two calls in _susped() and reuse it here, as well (discarding error codes). But in contrast to the suspend call, which gets aborted on an error, both regulators have to be disabled in _remove() unconditionally.
And to just have a call here like _powerdown(), I'm not really convinced the overhead/effort is worth the slight benefit. But I don't really mind, either.
Quoted 7 lines
thanks,
Daniel.
--
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
Re: [PATCH 4/6] iio:dac:m62332: drop unrequired variable
In reply to Hartmut KnaackQuoted 4 lines · Hartmut Knaack
A return variable is not required in
_write_raw(), and dropping it reduces complexity, as well.Signed-off-by Hartmut Knaack <[email protected]>
Initially forgot to CC linux-iio.
Quoted 39 lines
--- drivers/iio/dac/m62332.c | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-)drivers/iio/dac/m62332.c +2
-7@@ -112,21 +112,17 @@ static int m62332_read_raw(struct iio_dev *indio_dev,static int m62332_write_raw (struct iio_dev *indio_dev,struct iio_chan_spec const *chan,int val,int val2,long mask) { -int ret; -switch (mask) {case IIO_CHAN_INFO_RAW :if (val <0 || val >255 )return -EINVAL ; - ret =m62332_set_value (indio_dev, val, chan->channel); -break ; +return m62332_set_value (indio_dev, val, chan->channel);default : - ret = -EINVAL ;break ; } -return ret; +return -EINVAL ; }#ifdef CONFIG_PM_SLEEP --2.4.6
--
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
Re: [PATCH 1/6] iio:dac:m62332: share scale and offset
In reply to Jonathan CameronQuoted 12 lines · Jonathan Cameron
Quoted 6 lines · Hartmut Knaack
This device simply uses its Vcc as reference voltage, so the same scale applies for all channels. Also offset doesn't appear to be different for any channel. Represent this by switching these two attributes to
info_mask_shared_by_type.Signed-off-by Hartmut Knaack <[email protected]>This sort of tidy up is always a little interesting. Technically it is an ABI change (be it one that changes from one possible representation to a better one). Unfortunately technically we can't rely on users using a library or similar that would hide this detail for them.
Quoted 5 lines
Still we can make the change if no one notices. Perhaps
Dmitry will want to comment on this however, so I'll let it sit for a while longer.Jonathan
Applied to the togreg branch of iio.git. As ever, pushed out first as testing for the autobuilders to play with it.
Thanks,
Jonathan
Quoted 43 lines
Quoted 27 lines
---
Hope you don't mind too much, that I inserted an extra tab in the whole block rather than messing up style and cleaning it up later.drivers/iio/dac/m62332.c | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-)drivers/iio/dac/m62332.c +2
-9@@ -173,15 +173,15 @@ static const struct iio_info m62332_info = { .driver_module = THIS_MODULE, }; -#define M62332_CHANNEL (chan) { \ - .type = IIO_VOLTAGE, \ - .indexed = 1, \ - .output = 1, \ - .channel = (chan), \ - .datasheet_name = "CH" #chan, \ - .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | \ - BIT(IIO_CHAN_INFO_SCALE) | \ - BIT(IIO_CHAN_INFO_OFFSET), \ +#define M62332_CHANNEL (chan) { \ + .type = IIO_VOLTAGE, \drivers/iio/dac/m62332.c static const struct iio_info m62332_info = { · line 187
Quoted 10 lines
drivers/iio/dac/m62332.c +7
-0+ .indexed = 1, \ + .output = 1, \ + .channel = (chan), \ + .datasheet_name = "CH" #chan, \ + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \ + .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE) | \ + BIT(IIO_CHAN_INFO_OFFSET), \ }static const struct iio_chan_spec m62332_channels[M62332_CHANNELS ] = {--
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 14 lines · Dmitry Eremin-Solenikov
Hello,
2015-08-31 18:00 GMT+03:00 Jonathan Cameron <[email protected]>:
Quoted 11 lines · Hartmut Knaack
Quoted 10 lines
A set of patches to address some issues I spotted during review.
Hartmut Knaack (6):
iio:dac:m62332: share scale and offset
iio:dac:m62332: shutdown on remove
iio:dac:m62332: useARRAY_SIZE
iio:dac:m62332: drop unrequired variable
iio:dac:m62332: address some style issues
iio:dac:m62332: use dynamic scale
Quoted 8 lines
Quoted 5 lines
Quoted 3 lines
drivers/iio/dac/m62332.c | 63 +++++++++++++++++++++++++----------------------- 1 file changed, 33 insertions(+), 30 deletions(-)Other than letting this series sit on the list a few days longer, for Dmitry or others to take a look at it, I'm happy with this series.
The changes look good, however I'd like to test them on my hardware first.
Hi Dmitry,
Sorry I'd forgotten you sent this so have just backed out the first couple for now. Any idea when you might get a chance to test these out? We aren't in a rush given timing relative to the cycle, but always good to have some idea of how long things are likely to take!
Thanks,
Jonathan
Jonathan Cameron
maintainerRe: [PATCH 1/6] iio:dac:m62332: share scale and offset
In reply to Jonathan CameronQuoted 13 lines · Jonathan Cameron
Quoted 12 lines · Jonathan Cameron
Quoted 6 lines · Hartmut Knaack
This device simply uses its Vcc as reference voltage, so the same scale applies for all channels. Also offset doesn't appear to be different for any channel. Represent this by switching these two attributes to
info_mask_shared_by_type.Signed-off-by Hartmut Knaack <[email protected]>This sort of tidy up is always a little interesting. Technically it is an ABI change (be it one that changes from one possible representation to a better one). Unfortunately technically we can't rely on users using a library or similar that would hide this detail for them.
Quoted 7 lines
Quoted 5 lines
Still we can make the change if no one notices. Perhaps
Dmitry will want to comment on this however, so I'll let it sit for a while longer.Jonathan
Applied to the togreg branch of iio.git. As ever, pushed out first as testing for the autobuilders to play with it.
Oops, had forgotten these were waiting on a tested-by from Dmitry. Backed out again for now.
Sorry about that.
Jonathan
Quoted 53 lines
Thanks,
Jonathan
Quoted 35 lines
Quoted 35 lines
---
Hope you don't mind too much, that I inserted an extra tab in the whole block rather than messing up style and cleaning it up later.drivers/iio/dac/m62332.c | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-)drivers/iio/dac/m62332.c +9
-9@@ -173,15 +173,15 @@ static const struct iio_info m62332_info = { .driver_module = THIS_MODULE, }; -#define M62332_CHANNEL (chan) { \ - .type = IIO_VOLTAGE, \ - .indexed = 1, \ - .output = 1, \ - .channel = (chan), \ - .datasheet_name = "CH" #chan, \ - .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | \ - BIT(IIO_CHAN_INFO_SCALE) | \ - BIT(IIO_CHAN_INFO_OFFSET), \ +#define M62332_CHANNEL (chan) { \ + .type = IIO_VOLTAGE, \ + .indexed = 1, \ + .output = 1, \ + .channel = (chan), \ + .datasheet_name = "CH" #chan, \ + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \ + .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE) | \ + BIT(IIO_CHAN_INFO_OFFSET), \ }Quoted 8 lines
drivers/iio/dac/m62332.c static const struct iio_info m62332_info = { · line 187
Quoted 2 lines
drivers/iio/dac/m62332.c +0
-0static const struct iio_chan_spec m62332_channels[M62332_CHANNELS ] = {--
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
maintainerRe: [PATCH 1/6] iio:dac:m62332: share scale and offset
Quoted 18 lines · Jonathan Cameron
Quoted 17 lines · Jonathan Cameron
Quoted 16 lines · Jonathan Cameron
Quoted 6 lines · Hartmut Knaack
This device simply uses its Vcc as reference voltage, so the same scale applies for all channels. Also offset doesn't appear to be different for any channel. Represent this by switching these two attributes to
info_mask_shared_by_type.Signed-off-by Hartmut Knaack <[email protected]>This sort of tidy up is always a little interesting. Technically it is an ABI change (be it one that changes from one possible representation to a better one). Unfortunately technically we can't rely on users using a library or similar that would hide this detail for them.
Still we can make the change if no one notices. Perhaps
Dmitry will want to comment on this however, so I'll let it sit for a while longer.
Quoted 8 lines
Quoted 3 lines
Quoted 1 line
Jonathan
Applied to the togreg branch of iio.git. As ever, pushed out first as testing for the autobuilders to play with it.
Oops, had forgotten these were waiting on a tested-by from Dmitry. Backed out again for now.
Sorry about that.
Dmitry, any timescale for when you might get a chance to test this series?
Jonathan
Quoted 60 lines
Jonathan
Quoted 48 lines
Thanks,
Jonathan
Quoted 35 lines
Quoted 35 lines
---
Hope you don't mind too much, that I inserted an extra tab in the whole block rather than messing up style and cleaning it up later.drivers/iio/dac/m62332.c | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-)drivers/iio/dac/m62332.c +9
-9@@ -173,15 +173,15 @@ static const struct iio_info m62332_info = { .driver_module = THIS_MODULE, }; -#define M62332_CHANNEL (chan) { \ - .type = IIO_VOLTAGE, \ - .indexed = 1, \ - .output = 1, \ - .channel = (chan), \ - .datasheet_name = "CH" #chan, \ - .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | \ - BIT(IIO_CHAN_INFO_SCALE) | \ - BIT(IIO_CHAN_INFO_OFFSET), \ +#define M62332_CHANNEL (chan) { \ + .type = IIO_VOLTAGE, \ + .indexed = 1, \ + .output = 1, \ + .channel = (chan), \ + .datasheet_name = "CH" #chan, \ + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \ + .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE) | \ + BIT(IIO_CHAN_INFO_OFFSET), \ }Quoted 8 lines
drivers/iio/dac/m62332.c static const struct iio_info m62332_info = { · line 187
Quoted 2 lines
drivers/iio/dac/m62332.c +0
-0static const struct iio_chan_spec m62332_channels[M62332_CHANNELS ] = {--
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.htmlQuoted 5 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 21 lines · Jonathan Cameron
Quoted 20 lines · Dmitry Eremin-Solenikov
Hello,
2015-08-31 18:00 GMT+03:00 Jonathan Cameron <[email protected]>:
Quoted 16 lines · Hartmut Knaack
Quoted 10 lines
A set of patches to address some issues I spotted during review.
Hartmut Knaack (6):
iio:dac:m62332: share scale and offset
iio:dac:m62332: shutdown on remove
iio:dac:m62332: useARRAY_SIZE
iio:dac:m62332: drop unrequired variable
iio:dac:m62332: address some style issues
iio:dac:m62332: use dynamic scaleQuoted 3 lines
drivers/iio/dac/m62332.c | 63 +++++++++++++++++++++++++----------------------- 1 file changed, 33 insertions(+), 30 deletions(-)Other than letting this series sit on the list a few days longer, for Dmitry or others to take a look at it, I'm happy with this series.
Quoted 9 lines
Quoted 2 lines
The changes look good, however I'd like to test them on my hardware first.
Hi Dmitry,
Sorry I'd forgotten you sent this so have just backed out the first couple for now. Any idea when you might get a chance to test these out? We aren't in a rush given timing relative to the cycle, but always good to have some idea of how long things are likely to take!
Hi Dmitry,
Any timescale on running the tests you wanted to on these? If I don't hear from you before next weekend, I'll probably pick them up and we can deal with any issues they are in.
Jonathan
Jonathan Cameron
maintainerQuoted 31 lines · Jonathan Cameron
Quoted 21 lines · Jonathan Cameron
Quoted 20 lines · Dmitry Eremin-Solenikov
Hello,
2015-08-31 18:00 GMT+03:00 Jonathan Cameron <[email protected]>:
Quoted 16 lines · Hartmut Knaack
Quoted 10 lines
A set of patches to address some issues I spotted during review.
Hartmut Knaack (6):
iio:dac:m62332: share scale and offset
iio:dac:m62332: shutdown on remove
iio:dac:m62332: useARRAY_SIZE
iio:dac:m62332: drop unrequired variable
iio:dac:m62332: address some style issues
iio:dac:m62332: use dynamic scaleQuoted 3 lines
drivers/iio/dac/m62332.c | 63 +++++++++++++++++++++++++----------------------- 1 file changed, 33 insertions(+), 30 deletions(-)Other than letting this series sit on the list a few days longer, for Dmitry or others to take a look at it, I'm happy with this series.
Quoted 9 lines
Quoted 2 lines
The changes look good, however I'd like to test them on my hardware first.
Hi Dmitry,
Sorry I'd forgotten you sent this so have just backed out the first couple for now. Any idea when you might get a chance to test these out? We aren't in a rush given timing relative to the cycle, but always good to have some idea of how long things are likely to take!
Quoted 8 lines
Hi Dmitry,
Any timescale on running the tests you wanted to on these? If I don't hear from you before next weekend, I'll probably pick them up and we can deal with any issues they are in.
Jonathan
Ah well. Dmitry has it seems unfortunately dropped out of sight. As I'm fairly happy these are straight forward I'm going to apply them now.
Jonathan
Quoted 5 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
Re: [PATCH 1/6] iio:dac:m62332: share scale and offset
In reply to Hartmut KnaackQuoted 6 lines · Hartmut Knaack
This device simply uses its Vcc as reference voltage, so the same scale applies for all channels. Also offset doesn't appear to be different for any channel. Represent this by switching these two attributes to
info_mask_shared_by_type.Signed-off-by Hartmut Knaack <[email protected]>
Applied to the togreg branch of iio.git.
Thanks,
Jonathan
Quoted 37 lines
---
Hope you don't mind too much, that I inserted an extra tab in the whole block rather than messing up style and cleaning it up later.drivers/iio/dac/m62332.c | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-)drivers/iio/dac/m62332.c +9
-9@@ -173,15 +173,15 @@ static const struct iio_info m62332_info = { .driver_module = THIS_MODULE, }; -#define M62332_CHANNEL (chan) { \ - .type = IIO_VOLTAGE, \ - .indexed = 1, \ - .output = 1, \ - .channel = (chan), \ - .datasheet_name = "CH" #chan, \ - .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | \ - BIT(IIO_CHAN_INFO_SCALE) | \ - BIT(IIO_CHAN_INFO_OFFSET), \ +#define M62332_CHANNEL (chan) { \ + .type = IIO_VOLTAGE, \ + .indexed = 1, \ + .output = 1, \ + .channel = (chan), \ + .datasheet_name = "CH" #chan, \ + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \ + .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE) | \ + BIT(IIO_CHAN_INFO_OFFSET), \ }static const struct iio_chan_spec m62332_channels[M62332_CHANNELS ] = {
Re: [PATCH 2/6] iio:dac:m62332: shutdown on remove
In reply to Daniel BalutaQuoted 9 lines · Daniel Baluta
Quoted 6 lines · Hartmut Knaack
The regulator framework requests to balance
regulator_enable()calls withregulator_disable()calls. To meet this requirement, set channels to 0 on remove, which implies aregulator_disable()call in case that channel was enabled.Signed-off-by Hartmut Knaack <[email protected]>Acked-by Daniel Baluta <[email protected]>
Applied to the togreg branch of iio.git - as ever initially pushed out as testing for the autobuilders to mess around with it.
Jonathan
Quoted 28 lines
Quoted 10 lines
--- drivers/iio/dac/m62332.c | 2 ++ 1 file changed, 2 insertions(+)drivers/iio/dac/m62332.c +0
-0drivers/iio/dac/m62332.c static int m62332_remove(struct i2c_client *client) · line 250
Quoted 8 lines
drivers/iio/dac/m62332.c +2
-0@@ -243,6 +243,8 @@ static int m62332_remove(struct i2c_client *client)iio_device_unregister (indio_dev);iio_map_array_unregister (indio_dev); +m62332_set_value (indio_dev,0 ,0 ); +m62332_set_value (indio_dev,0 ,1 );return 0 ; }Wouldn't be nice to factor this two calls in a separate function?
thanks,
Daniel.
--
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
maintainerRe: [PATCH 3/6] iio:dac:m62332: use ARRAY_SIZE
In reply to Daniel BalutaQuoted 6 lines · Daniel Baluta
Quoted 3 lines · Hartmut Knaack
Make use of
ARRAY_SIZEto prevent buffer issues.Signed-off-by Hartmut Knaack <[email protected]>Acked-by Daniel Baluta <[email protected]>
Applied to the togreg branch of iio.git - pushed out as testing for now.
J
Quoted 41 lines
Quoted 32 lines
--- drivers/iio/dac/m62332.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)drivers/iio/dac/m62332.c +3
-4@@ -62,8 +62,8 @@ static int m62332_set_value(struct iio_dev *indio_dev, goto out; } - res =i2c_master_send (client, outbuf,2 ); -if (res >=0 && res !=2 ) + res =i2c_master_send (client, outbuf,ARRAY_SIZE (outbuf)); +if (res >=0 && res !=ARRAY_SIZE (outbuf)) res = -EIO ;if (res <0 ) goto out;@@ -212,7 +212,7 @@ static int m62332_probe(struct i2c_client *client,/* establish that the iio_dev is a child of the i2c device */ indio_dev->dev.parent = &client->dev; - indio_dev->num_channels =M62332_CHANNELS ; + indio_dev->num_channels =ARRAY_SIZE (m62332_channels); indio_dev->channels = m62332_channels; indio_dev->modes =INDIO_DIRECT_MODE ; indio_dev->info = &m62332_info; --2.4.6
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
Re: [PATCH 4/6] iio:dac:m62332: drop unrequired variable
In reply to Daniel BalutaQuoted 9 lines · Daniel Baluta
Quoted 4 lines · Hartmut Knaack
A return variable is not required in
_write_raw(), and dropping it reduces complexity, as well.Signed-off-by Hartmut Knaack <[email protected]>Acked-by Daniel Baluta <[email protected]>Initially forgot to CC linux-iio.
Applied.
Quoted 45 lines
Quoted 36 lines
--- drivers/iio/dac/m62332.c | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-)drivers/iio/dac/m62332.c +2
-7@@ -112,21 +112,17 @@ static int m62332_read_raw(struct iio_dev *indio_dev,static int m62332_write_raw (struct iio_dev *indio_dev,struct iio_chan_spec const *chan,int val,int val2,long mask) { -int ret; -switch (mask) {case IIO_CHAN_INFO_RAW :if (val <0 || val >255 )return -EINVAL ; - ret =m62332_set_value (indio_dev, val, chan->channel); -break ; +return m62332_set_value (indio_dev, val, chan->channel);default : - ret = -EINVAL ;break ; } -return ret; +return -EINVAL ; }#ifdef CONFIG_PM_SLEEP --2.4.6
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
Re: [PATCH 5/6] iio:dac:m62332: address some style issues
In reply to Hartmut KnaackQuoted 4 lines · Hartmut Knaack
Fix some indentation issues and separate returns by empty lines (IIO style). Also rename the channel mask in
_read_raw()to mask.Signed-off-by Hartmut Knaack <[email protected]>
Applied to the togreg branch of iio.git
Thanks,
Jonathan
Quoted 74 lines
--- drivers/iio/dac/m62332.c | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-)drivers/iio/dac/m62332.c +11
-5@@ -40,8 +40,7 @@ struct m62332_data {#endif }; -static int m62332_set_value (struct iio_dev *indio_dev, -u8 val,int channel) +static int m62332_set_value (struct iio_dev *indio_dev,u8 val,int channel) {struct m62332_data *data =iio_priv (indio_dev);struct i2c_client *client = data->client;@@ -87,30 +86,35 @@ static int m62332_read_raw(struct iio_dev *indio_dev,struct iio_chan_spec const *chan, int *val, int *val2, - long m) + long mask) { struct m62332_data *data = iio_priv(indio_dev); -switch (m) { +switch (mask) {case IIO_CHAN_INFO_SCALE :/* Corresponds to Vref / 2^(bits) */ *val = data->vref_mv; *val2 =8 ; +return IIO_VAL_FRACTIONAL_LOG2 ;case IIO_CHAN_INFO_RAW : *val = data->raw[chan->channel]; +return IIO_VAL_INT ;case IIO_CHAN_INFO_OFFSET : *val =1 ; +return IIO_VAL_INT ;default :break ; } +return -EINVAL ; }static int m62332_write_raw (struct iio_dev *indio_dev, -struct iio_chan_spec const *chan,int val,int val2,long mask) +struct iio_chan_spec const *chan,int val,int val2, +long mask) {switch (mask) {case IIO_CHAN_INFO_RAW :@@ -195,6 +199,7 @@ static int m62332_probe(struct i2c_client *client, indio_dev =devm_iio_device_alloc (&client->dev,sizeof (*data));if (!indio_dev)return -ENOMEM ; + data =iio_priv (indio_dev);i2c_set_clientdata (client, indio_dev); data->client = client;@@ -230,6 +235,7 @@ static int m62332_probe(struct i2c_client *client, err:iio_map_array_unregister (indio_dev); +return ret; }
Re: [PATCH 6/6] iio:dac:m62332: use dynamic scale
In reply to Hartmut KnaackQuoted 4 lines · Hartmut Knaack
Some regulators can supply multiple voltages. To take changing voltages into account, the scale needs to be calculated on every read access.
Signed-off-by Hartmut Knaack <[email protected]>
This is a pretty common situation, but just because it is done less than ideally in a lot of places, doesn't mean we shouldn't fix this one!
Applied.
Jonathan
Quoted 47 lines
--- drivers/iio/dac/m62332.c | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-)drivers/iio/dac/m62332.c +6
-7@@ -31,7 +31,6 @@struct m62332_data {struct i2c_client *client; -u16 vref_mv;struct regulator *vcc;struct mutex mutex;u8 raw[M62332_CHANNELS ];@@ -89,11 +88,16 @@ static int m62332_read_raw(struct iio_dev *indio_dev, long mask) {struct m62332_data *data =iio_priv (indio_dev); +int ret;switch (mask) {case IIO_CHAN_INFO_SCALE :/* Corresponds to Vref / 2^(bits) */ - *val = data->vref_mv; + ret =regulator_get_voltage (data->vcc); +if (ret <0 ) +return ret; + + *val = ret /1000 ;/* mV */ *val2 =8 ;return IIO_VAL_FRACTIONAL_LOG2 ;@@ -218,11 +222,6 @@ static int m62332_probe(struct i2c_client *client, indio_dev->modes =INDIO_DIRECT_MODE ; indio_dev->info = &m62332_info; - ret =regulator_get_voltage (data->vcc); -if (ret <0 ) -return ret; - data->vref_mv = ret /1000 ;/* mV */ - ret =iio_map_array_register (indio_dev, client->dev.platform_data);if (ret <0 )return ret;