stagging:iio:meter: Add essential IIO API structures for ADE7854
Subsystems (4)
cover stagging:iio:meter: Add essential IIO API structures for ADE7854
This patchset aims to update ADE7854 by adding the required IIO API components. The first patch adds the iio_chan_spec for handling seven different registers (all of them with a similar behavior). The second patch appends the read_raw function defined by the IIO API. Finally, the third patch adds the write_raw function and remove the attributes used for handling the seven registers. This patchset has the contribution of John Syne, which was responsible for mapping the correct ABI name per element in the ADE7854; additionally, John provided codes that helped to shape these patches.
Rodrigo Siqueira (3):
stagging:iio:meter: Add iio_chan_spec stagging:iio:meter: Add ade7854_read_raw function stagging:iio:meter: Add ade7854_write_raw function
drivers/staging/iio/meter/ade7854.c | 129 ++++++++++++++++++++-------- 1 file changed, 94 insertions(+), 35 deletions(-)
Signature · 1 line
2.17.0
PATCH 1/3 stagging:iio:meter: Add iio_chan_spec
This patch adds iio_chan_spec struct. Additionally, the channel adds the support for handling AIGAIN, BIGAIN, CIGAIN, NIGAIN, AVGAIN, BVGAIN, and CVGAIN.
--- drivers/staging/iio/meter/ade7854.c | 42 +++++++++++++++++++++++++++++ 1 file changed, 42 insertions(+)
drivers/staging/iio/meter/ade7854.c +42 -0
@@ -22,6 +22,48 @@#include "meter.h" #include "ade7854.h" +#define PHASEA "phaseA" +#define PHASEB "phaseB" +#define PHASEC "phaseC" +#define NEUTRAL "neutral" + +#define ADE7854_CHANNEL (_type, _name, _mask, _reg) { \ + .type = _type, \ + .indexed = 1, \ + .channel = 0, \ + .extend_name = _name, \ + .info_mask_separate = _mask, \ + .address = _reg, \ + .info_mask_shared_by_dir = BIT(IIO_CHAN_INFO_SAMP_FREQ), \ + .scan_index = -1, \ +} + +static const struct iio_chan_spec ade7854_channels[] = { +/* Current */ +ADE7854_CHANNEL (IIO_CURRENT ,PHASEA , +BIT (IIO_CHAN_INFO_SCALE ), +ADE7854_AIGAIN ), +ADE7854_CHANNEL (IIO_CURRENT ,PHASEB , +BIT (IIO_CHAN_INFO_SCALE ), +ADE7854_BIGAIN ), +ADE7854_CHANNEL (IIO_CURRENT ,PHASEC , +BIT (IIO_CHAN_INFO_SCALE ), +ADE7854_CIGAIN ), +ADE7854_CHANNEL (IIO_CURRENT ,NEUTRAL , +BIT (IIO_CHAN_INFO_SCALE ), +ADE7854_NIGAIN ), +/* Voltage */ +ADE7854_CHANNEL (IIO_VOLTAGE ,PHASEA , +BIT (IIO_CHAN_INFO_SCALE ), +ADE7854_AVGAIN ), +ADE7854_CHANNEL (IIO_VOLTAGE ,PHASEB , +BIT (IIO_CHAN_INFO_SCALE ), +ADE7854_BVGAIN ), +ADE7854_CHANNEL (IIO_VOLTAGE ,PHASEC , +BIT (IIO_CHAN_INFO_SCALE ), +ADE7854_CVGAIN ), +}; +static ssize_t ade7854_read_8bit (struct device *dev,struct device_attribute *attr,char *buf)
Signature · 1 line
2.17.0
Jonathan Cameron
maintainerRe: [PATCH 1/3] stagging:iio:meter: Add iio_chan_spec
Quoted 20 lines · On Sat, 21 Apr 2018 08:55:08 -0300 Rodrigo Siqueira…
This patch adds
iio_chan_specstruct. Additionally, the channel adds the support for handling AIGAIN, BIGAIN, CIGAIN, NIGAIN, AVGAIN, BVGAIN, and CVGAIN.Signed-off-by Rodrigo Siqueira <[email protected]>--- drivers/staging/iio/meter/ade7854.c | 42 +++++++++++++++++++++++++++++ 1 file changed, 42 insertions(+)drivers/staging/iio/meter/ade7854.c +3
-0@@ -22,6 +22,48 @@#include "meter.h" #include "ade7854.h" +#define PHASEA "phaseA" +#define PHASEB "phaseB" +#define PHASEC "phaseC"
drivers/staging/iio/meter/ade7854.c line 37
Quoted 10 lines
drivers/staging/iio/meter/ade7854.c +10
-0+#define NEUTRAL "neutral" + +#define ADE7854_CHANNEL (_type, _name, _mask, _reg) { \ + .type = _type, \ + .indexed = 1, \ + .channel = 0, \ + .extend_name = _name, \ + .info_mask_separate = _mask, \ + .address = _reg, \ + .info_mask_shared_by_dir = BIT(IIO_CHAN_INFO_SAMP_FREQ), \
This is odd. It defines the sampling frequency. whilst providing now such read_raw functionality. So you will get a sampling frequency attribute that doesn't work.
drivers/staging/iio/meter/ade7854.c line 38
Quoted 1 line
drivers/staging/iio/meter/ade7854.c +1
-0+ .scan_index = -1, \
There is no buffered support so this isn't needed.
drivers/staging/iio/meter/ade7854.c line 44
Quoted 6 lines
drivers/staging/iio/meter/ade7854.c +6
-0+} + +static const struct iio_chan_spec ade7854_channels[] = { +/* Current */ + ADE7854_CHANNEL(IIO_CURRENT, PHASEA, + BIT(IIO_CHAN_INFO_SCALE),
Each patch needs to stand on it's own (I might no apply them all). This adds the scale attributes, but the support for them to actually work is in the next patch.
It isn't sensible to break this up into 3 patches and it actually makes it harder to review (as they only make sense together).
Jonathan
Quoted 15 lines
drivers/staging/iio/meter/ade7854.c +15
-0+ ADE7854_AIGAIN), +ADE7854_CHANNEL (IIO_CURRENT ,PHASEB , +BIT (IIO_CHAN_INFO_SCALE ), +ADE7854_BIGAIN ), +ADE7854_CHANNEL (IIO_CURRENT ,PHASEC , +BIT (IIO_CHAN_INFO_SCALE ), +ADE7854_CIGAIN ), +ADE7854_CHANNEL (IIO_CURRENT ,NEUTRAL , +BIT (IIO_CHAN_INFO_SCALE ), +ADE7854_NIGAIN ), +/* Voltage */ +ADE7854_CHANNEL (IIO_VOLTAGE ,PHASEA , +BIT (IIO_CHAN_INFO_SCALE ), +ADE7854_AVGAIN ), + ADE7854_CHANNEL(IIO_VOLTAGE, PHASEB,
drivers/staging/iio/meter/ade7854.c line 69
Quoted 10 lines
drivers/staging/iio/meter/ade7854.c +7
-0+BIT (IIO_CHAN_INFO_SCALE ), + ADE7854_BVGAIN), +ADE7854_CHANNEL (IIO_VOLTAGE ,PHASEC , +BIT (IIO_CHAN_INFO_SCALE ), + ADE7854_CVGAIN), +}; +static ssize_t ade7854_read_8bit (struct device *dev,struct device_attribute *attr,char *buf)
_______________________________________________
devel mailing list
[email protected]
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
PATCH 2/3 stagging:iio:meter: Add ade7854_read_raw function
In reply to Rodrigo SiqueiraThis patch adds the ade7854_read_raw() function which is responsible for handling the read operation for registers: AIGAIN, BIGAIN, CIGAIN, NIGAIN, AVGAIN, BVGAIN, and CVGAIN. For the sake of simplicity, this patch only adds basic manipulation for current and voltage channels. Finally, this patch disables the old approach for reading data.
--- drivers/staging/iio/meter/ade7854.c | 41 ++++++++++++++++++++++++----- 1 file changed, 34 insertions(+), 7 deletions(-)
drivers/staging/iio/meter/ade7854.c +34 -7
@@ -229,31 +229,31 @@ static int ade7854_reset(struct device *dev) } static IIO_DEV_ATTR_AIGAIN(0644, - ade7854_read_24bit, +NULL , ade7854_write_24bit, ADE7854_AIGAIN); static IIO_DEV_ATTR_BIGAIN(0644, - ade7854_read_24bit, +NULL , ade7854_write_24bit, ADE7854_BIGAIN); static IIO_DEV_ATTR_CIGAIN(0644, - ade7854_read_24bit, +NULL , ade7854_write_24bit, ADE7854_CIGAIN); static IIO_DEV_ATTR_NIGAIN(0644, - ade7854_read_24bit, +NULL , ade7854_write_24bit, ADE7854_NIGAIN); static IIO_DEV_ATTR_AVGAIN(0644, - ade7854_read_24bit, +NULL , ade7854_write_24bit, ADE7854_AVGAIN); static IIO_DEV_ATTR_BVGAIN(0644, - ade7854_read_24bit, +NULL , ade7854_write_24bit, ADE7854_BVGAIN); static IIO_DEV_ATTR_CVGAIN(0644, - ade7854_read_24bit, +NULL , ade7854_write_24bit, ADE7854_CVGAIN); static IIO_DEV_ATTR_APPARENT_POWER_A_GAIN(0644,
@@ -471,6 +471,32 @@ static int ade7854_set_irq(struct device *dev, bool enable)return st->write_reg (dev,ADE7854_MASK0 , irqen,32 ); } +static int ade7854_read_raw (struct iio_dev *indio_dev, +struct iio_chan_spec const *chan, +int *val, +int *val2, +long mask) +{ +struct ade7854_state *st =iio_priv (indio_dev); +int ret; + +if (mask !=IIO_CHAN_INFO_SCALE ) +return -EINVAL ; + +switch (chan->type) { +case IIO_CURRENT : +case IIO_VOLTAGE : + ret = st->read_reg (&indio_dev->dev, chan->address, val,24 ); +if (ret <0 ) +return ret; +return IIO_VAL_INT ; +default : +break ; + } + +return -EINVAL ; +} +static int ade7854_initial_setup (struct iio_dev *indio_dev) {int ret;
@@ -572,6 +598,7 @@ static const struct attribute_group ade7854_attribute_group = {static const struct iio_info ade7854_info = { .attrs = &ade7854_attribute_group, + .read_raw = &ade7854_read_raw, };int ade7854_probe (struct iio_dev *indio_dev,struct device *dev)
Signature · 6 lines
2.17.0 _______________________________________________ devel mailing list [email protected] http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Jonathan Cameron
maintainerRe: [PATCH 2/3] stagging:iio:meter: Add ade7854_read_raw function
Quoted 7 lines · On Sat, 21 Apr 2018 08:55:52 -0300 Rodrigo Siqueira…
This patch adds the
ade7854_read_raw()function which is responsible for handling the read operation for registers: AIGAIN, BIGAIN, CIGAIN, NIGAIN, AVGAIN, BVGAIN, and CVGAIN. For the sake of simplicity, this patch only adds basic manipulation for current and voltage channels. Finally, this patch disables the old approach for reading data.Signed-off-by Rodrigo Siqueira <[email protected]>
Other than the previous mentioned issue with the way the channels are defined, this is nearly fine.
See inline.
Quoted 52 lines
--- drivers/staging/iio/meter/ade7854.c | 41 ++++++++++++++++++++++++----- 1 file changed, 34 insertions(+), 7 deletions(-)drivers/staging/iio/meter/ade7854.c +8
-7@@ -229,31 +229,31 @@ static int ade7854_reset(struct device *dev) } static IIO_DEV_ATTR_AIGAIN(0644, - ade7854_read_24bit, +NULL , ade7854_write_24bit, ADE7854_AIGAIN); static IIO_DEV_ATTR_BIGAIN(0644, - ade7854_read_24bit, +NULL , ade7854_write_24bit, ADE7854_BIGAIN); static IIO_DEV_ATTR_CIGAIN(0644, - ade7854_read_24bit, +NULL , ade7854_write_24bit, ADE7854_CIGAIN); static IIO_DEV_ATTR_NIGAIN(0644, - ade7854_read_24bit, +NULL , ade7854_write_24bit, ADE7854_NIGAIN); static IIO_DEV_ATTR_AVGAIN(0644, - ade7854_read_24bit, +NULL , ade7854_write_24bit, ADE7854_AVGAIN); static IIO_DEV_ATTR_BVGAIN(0644, - ade7854_read_24bit, +NULL , ade7854_write_24bit, ADE7854_BVGAIN); static IIO_DEV_ATTR_CVGAIN(0644, - ade7854_read_24bit, +NULL , ade7854_write_24bit, ADE7854_CVGAIN); static IIO_DEV_ATTR_APPARENT_POWER_A_GAIN(0644,@@ -471,6 +471,32 @@ static int ade7854_set_irq(struct device *dev, bool enable)return st->write_reg (dev,ADE7854_MASK0 , irqen,32 ); } +static int ade7854_read_raw(struct iio_dev *indio_dev,
drivers/staging/iio/meter/ade7854.c static int ade7854_set_irq(struct device *dev, bool enable) · line 484
Quoted 10 lines
drivers/staging/iio/meter/ade7854.c +10
-0+struct iio_chan_spec const *chan, + int *val, + int *val2, + long mask) +{ + struct ade7854_state *st = iio_priv(indio_dev); +int ret; + +if (mask !=IIO_CHAN_INFO_SCALE ) +return -EINVAL ;
Given you specified sampling frequency for the channels in the previous patch this is a little odd!
Quoted 19 lines
drivers/staging/iio/meter/ade7854.c +15
-0+ +switch (chan->type) { +case IIO_CURRENT : +case IIO_VOLTAGE : + ret = st->read_reg (&indio_dev->dev, chan->address, val,24 ); +if (ret <0 ) +return ret; +return IIO_VAL_INT ; +default : +break ; + } + +return -EINVAL ; +} +static int ade7854_initial_setup (struct iio_dev *indio_dev) {int ret;
drivers/staging/iio/meter/ade7854.c static const struct attribute_group ade7854_attribute_group = { · line 604
Quoted 7 lines
drivers/staging/iio/meter/ade7854.c +1
-0@@ -572,6 +598,7 @@ static const struct attribute_group ade7854_attribute_group = {static const struct iio_info ade7854_info = { .attrs = &ade7854_attribute_group, + .read_raw = &ade7854_read_raw, };int ade7854_probe (struct iio_dev *indio_dev,struct device *dev)
_______________________________________________
devel mailing list
[email protected]
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
PATCH 3/3 stagging:iio:meter: Add ade7854_write_raw function
In reply to Rodrigo SiqueiraThis patch adds the ade7854_write_raw() function which is responsible for handling the write operation for registers: AIGAIN, BIGAIN, CIGAIN, NIGAIN, AVGAIN, BVGAIN, and CVGAIN. Finally, this patch completely removes the old ABI used for handling the registers mentioned above.
--- drivers/staging/iio/meter/ade7854.c | 60 ++++++++++++----------------- 1 file changed, 25 insertions(+), 35 deletions(-)
drivers/staging/iio/meter/ade7854.c +25 -35
@@ -228,34 +228,6 @@ static int ade7854_reset(struct device *dev)return st->write_reg (dev,ADE7854_CONFIG , val,16 ); } -static IIO_DEV_ATTR_AIGAIN(0644, -NULL , - ade7854_write_24bit, - ADE7854_AIGAIN); -static IIO_DEV_ATTR_BIGAIN(0644, -NULL , - ade7854_write_24bit, - ADE7854_BIGAIN); -static IIO_DEV_ATTR_CIGAIN(0644, -NULL , - ade7854_write_24bit, - ADE7854_CIGAIN); -static IIO_DEV_ATTR_NIGAIN(0644, -NULL , - ade7854_write_24bit, - ADE7854_NIGAIN); -static IIO_DEV_ATTR_AVGAIN(0644, -NULL , - ade7854_write_24bit, - ADE7854_AVGAIN); -static IIO_DEV_ATTR_BVGAIN(0644, -NULL , - ade7854_write_24bit, - ADE7854_BVGAIN); -static IIO_DEV_ATTR_CVGAIN(0644, -NULL , - ade7854_write_24bit, - ADE7854_CVGAIN); static IIO_DEV_ATTR_APPARENT_POWER_A_GAIN(0644, ade7854_read_24bit, ade7854_write_24bit,
@@ -497,6 +469,30 @@ static int ade7854_read_raw(struct iio_dev *indio_dev,return -EINVAL ; } +static int ade7854_write_raw (struct iio_dev *indio_dev, +struct iio_chan_spec const *chan, +int val,int val2,long mask) +{ +struct ade7854_state *st =iio_priv (indio_dev); +int ret; + +if (mask !=IIO_CHAN_INFO_SCALE ) +return -EINVAL ; + +switch (chan->type) { +case IIO_CURRENT : +case IIO_VOLTAGE : + ret = st->write_reg (&indio_dev->dev, chan->address, val,24 ); +if (ret <0 ) +return ret; +return 0 ; +default : +break ; + } + +return -EINVAL ; +} +static int ade7854_initial_setup (struct iio_dev *indio_dev) {int ret;
@@ -521,13 +517,6 @@ static IIO_CONST_ATTR_SAMP_FREQ_AVAIL("8000"); static IIO_CONST_ATTR(name, "ade7854");static struct attribute *ade7854_attributes[] = { - &iio_dev_attr_aigain.dev_attr.attr, - &iio_dev_attr_bigain.dev_attr.attr, - &iio_dev_attr_cigain.dev_attr.attr, - &iio_dev_attr_nigain.dev_attr.attr, - &iio_dev_attr_avgain.dev_attr.attr, - &iio_dev_attr_bvgain.dev_attr.attr, - &iio_dev_attr_cvgain.dev_attr.attr, &iio_dev_attr_linecyc.dev_attr.attr, &iio_dev_attr_sagcyc.dev_attr.attr, &iio_dev_attr_cfcyc.dev_attr.attr,
@@ -599,6 +588,7 @@ static const struct attribute_group ade7854_attribute_group = {static const struct iio_info ade7854_info = { .attrs = &ade7854_attribute_group, .read_raw = &ade7854_read_raw, + .write_raw = &ade7854_write_raw, };int ade7854_probe (struct iio_dev *indio_dev,struct device *dev)
Signature · 1 line
2.17.0
Jonathan Cameron
maintainerRe: [PATCH 3/3] stagging:iio:meter: Add ade7854_write_raw function
Quoted 6 lines · On Sat, 21 Apr 2018 08:56:19 -0300 Rodrigo Siqueira…
This patch adds the
ade7854_write_raw()function which is responsible for handling the write operation for registers: AIGAIN, BIGAIN, CIGAIN, NIGAIN, AVGAIN, BVGAIN, and CVGAIN. Finally, this patch completely removes the old ABI used for handling the registers mentioned above.Signed-off-by Rodrigo Siqueira <[email protected]>
The baby steps approach here is good, but with all 3 patches as one it would have been a lot easier to follow.
This is almost fine except for the issue I had with how the channels were defined in the first place.
Also a question about scales inline...
Quoted 50 lines
--- drivers/staging/iio/meter/ade7854.c | 60 ++++++++++++----------------- 1 file changed, 25 insertions(+), 35 deletions(-)drivers/staging/iio/meter/ade7854.c +3
-28@@ -228,34 +228,6 @@ static int ade7854_reset(struct device *dev)return st->write_reg (dev,ADE7854_CONFIG , val,16 ); } -static IIO_DEV_ATTR_AIGAIN(0644, -NULL , - ade7854_write_24bit, - ADE7854_AIGAIN); -static IIO_DEV_ATTR_BIGAIN(0644, -NULL , - ade7854_write_24bit, - ADE7854_BIGAIN); -static IIO_DEV_ATTR_CIGAIN(0644, -NULL , - ade7854_write_24bit, - ADE7854_CIGAIN); -static IIO_DEV_ATTR_NIGAIN(0644, -NULL , - ade7854_write_24bit, - ADE7854_NIGAIN); -static IIO_DEV_ATTR_AVGAIN(0644, -NULL , - ade7854_write_24bit, - ADE7854_AVGAIN); -static IIO_DEV_ATTR_BVGAIN(0644, -NULL , - ade7854_write_24bit, - ADE7854_BVGAIN); -static IIO_DEV_ATTR_CVGAIN(0644, -NULL , - ade7854_write_24bit, - ADE7854_CVGAIN); static IIO_DEV_ATTR_APPARENT_POWER_A_GAIN(0644, ade7854_read_24bit, ade7854_write_24bit,@@ -497,6 +469,30 @@ static int ade7854_read_raw(struct iio_dev *indio_dev,return -EINVAL ; } +static int ade7854_write_raw (struct iio_dev *indio_dev, +struct iio_chan_spec const *chan, +int val,int val2,long mask)
drivers/staging/iio/meter/ade7854.c static int ade7854_read_raw(struct iio_dev *indio_dev, · line 484
Quoted 10 lines
drivers/staging/iio/meter/ade7854.c +10
-0+{ +struct ade7854_state *st =iio_priv (indio_dev); +int ret; + +if (mask !=IIO_CHAN_INFO_SCALE ) +return -EINVAL ; + +switch (chan->type) { +case IIO_CURRENT : +case IIO_VOLTAGE :
Probably need some range checking to make sure we have a sane value.
Also, I'm curious about units here.
you are using CHAN_INFO_SCALE which would mean this is linked to the _RAW value (the actual reading of the channel) by _RAW*_SCALE = value in defined base units for the channel type. So I'd expect to see some code here doing a conversion to the internal format for the device.
I wouldn't expect to see a value from userspace written unconverted into the register.
(I missed this in the previous patch - sorry).
Jonathan
Quoted 29 lines
drivers/staging/iio/meter/ade7854.c +11
-7+ ret = st->write_reg (&indio_dev->dev, chan->address, val,24 ); +if (ret <0 ) +return ret; +return 0 ; +default : +break ; + } + +return -EINVAL ; +} +static int ade7854_initial_setup (struct iio_dev *indio_dev) {int ret;@@ -521,13 +517,6 @@ static IIO_CONST_ATTR_SAMP_FREQ_AVAIL("8000"); static IIO_CONST_ATTR(name, "ade7854");static struct attribute *ade7854_attributes[] = { - &iio_dev_attr_aigain.dev_attr.attr, - &iio_dev_attr_bigain.dev_attr.attr, - &iio_dev_attr_cigain.dev_attr.attr, - &iio_dev_attr_nigain.dev_attr.attr, - &iio_dev_attr_avgain.dev_attr.attr, - &iio_dev_attr_bvgain.dev_attr.attr, - &iio_dev_attr_cvgain.dev_attr.attr, &iio_dev_attr_linecyc.dev_attr.attr, &iio_dev_attr_sagcyc.dev_attr.attr, &iio_dev_attr_cfcyc.dev_attr.attr,
drivers/staging/iio/meter/ade7854.c static const struct attribute_group ade7854_attribute_group = { · line 594
Quoted 7 lines
drivers/staging/iio/meter/ade7854.c +1
-0@@ -599,6 +588,7 @@ static const struct attribute_group ade7854_attribute_group = {static const struct iio_info ade7854_info = { .attrs = &ade7854_attribute_group, .read_raw = &ade7854_read_raw, + .write_raw = &ade7854_write_raw, };int ade7854_probe (struct iio_dev *indio_dev,struct device *dev)
_______________________________________________
devel mailing list
[email protected]
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 3/3] stagging:iio:meter: Add ade7854_write_raw function
Regards,
John
Quoted 81 lines · Jonathan Cameron
Quoted 6 lines · On Sat, 21 Apr 2018 08:56:19 -0300 Rodrigo Siqueira…
This patch adds the
ade7854_write_raw()function which is responsible for handling the write operation for registers: AIGAIN, BIGAIN, CIGAIN, NIGAIN, AVGAIN, BVGAIN, and CVGAIN. Finally, this patch completely removes the old ABI used for handling the registers mentioned above.Signed-off-by Rodrigo Siqueira <[email protected] <mailto:[email protected]>>The baby steps approach here is good, but with all 3 patches as one it would have been a lot easier to follow.
This is almost fine except for the issue I had with how the channels were defined in the first place.
Also a question about scales inline...
Quoted 50 lines
---
drivers/staging/iio/meter/ade7854.c| 60 ++++++++++++-----------------1 file changed, 25 insertions(+), 35 deletions(-)drivers/staging/iio/meter/ade7854.c +3
-28@@ -228,34 +228,6 @@ static int ade7854_reset(struct device *dev)return st->write_reg (dev,ADE7854_CONFIG , val,16 ); } -static IIO_DEV_ATTR_AIGAIN(0644, -NULL , - ade7854_write_24bit, - ADE7854_AIGAIN); -static IIO_DEV_ATTR_BIGAIN(0644, -NULL , - ade7854_write_24bit, - ADE7854_BIGAIN); -static IIO_DEV_ATTR_CIGAIN(0644, -NULL , - ade7854_write_24bit, - ADE7854_CIGAIN); -static IIO_DEV_ATTR_NIGAIN(0644, -NULL , - ade7854_write_24bit, - ADE7854_NIGAIN); -static IIO_DEV_ATTR_AVGAIN(0644, -NULL , - ade7854_write_24bit, - ADE7854_AVGAIN); -static IIO_DEV_ATTR_BVGAIN(0644, -NULL , - ade7854_write_24bit, - ADE7854_BVGAIN); -static IIO_DEV_ATTR_CVGAIN(0644, -NULL , - ade7854_write_24bit, - ADE7854_CVGAIN); static IIO_DEV_ATTR_APPARENT_POWER_A_GAIN(0644, ade7854_read_24bit, ade7854_write_24bit,@@ -497,6 +469,30 @@ static int ade7854_read_raw(struct iio_dev *indio_dev,return -EINVAL ; } +static int ade7854_write_raw (struct iio_dev *indio_dev, +struct iio_chan_spec const *chan, +int val,int val2,long mask)drivers/staging/iio/meter/ade7854.c static int ade7854_read_raw(struct iio_dev *indio_dev, · line 484
Quoted 10 lines
drivers/staging/iio/meter/ade7854.c +10
-0+{ +struct ade7854_state *st =iio_priv (indio_dev); +int ret; + +if (mask !=IIO_CHAN_INFO_SCALE ) +return -EINVAL ; + +switch (chan->type) { +case IIO_CURRENT : +case IIO_VOLTAGE :Probably need some range checking to make sure we have a sane value.
Quoted 6 lines
Also, I'm curious about units here.
you are using
CHAN_INFO_SCALEwhich would mean this is linked to the_RAWvalue (the actual reading of the channel) by _RAW*_SCALE = value in defined base units for the channel type. So I'd expect to see some code here doing a conversion to the internal format for the device.
These should be IIO_CHAN_INFO_HARDWAREGAIN as they are used to correct any imperfections of the sensors. A user space application calculates the error and based on a formula determines the correct register value.
Meter values such as Voltage, Current, Power, Energy use a value/LSB method, which is determined while applying a known voltage and current. After that, any parameter is multiplied by the respective value/LSB to get a real world value.
Regards,
John
Quoted 44 lines
I wouldn't expect to see a value from userspace written unconverted into the register.
(I missed this in the previous patch - sorry).
Jonathan
Quoted 36 lines
drivers/staging/iio/meter/ade7854.c +12
-7+ ret = st->write_reg (&indio_dev->dev, chan->address, val,24 ); +if (ret <0 ) +return ret; +return 0 ; +default : +break ; + } + +return -EINVAL ; +} +static int ade7854_initial_setup (struct iio_dev *indio_dev) {int ret;@@ -521,13 +517,6 @@ static IIO_CONST_ATTR_SAMP_FREQ_AVAIL("8000"); static IIO_CONST_ATTR(name, "ade7854");static struct attribute *ade7854_attributes[] = { - &iio_dev_attr_aigain.dev_attr.attr, - &iio_dev_attr_bigain.dev_attr.attr, - &iio_dev_attr_cigain.dev_attr.attr, - &iio_dev_attr_nigain.dev_attr.attr, - &iio_dev_attr_avgain.dev_attr.attr, - &iio_dev_attr_bvgain.dev_attr.attr, - &iio_dev_attr_cvgain.dev_attr.attr, &iio_dev_attr_linecyc.dev_attr.attr, &iio_dev_attr_sagcyc.dev_attr.attr, &iio_dev_attr_cfcyc.dev_attr.attr,@@ -599,6 +588,7 @@ static const struct attribute_group ade7854_attribute_group = {static const struct iio_info ade7854_info = { .attrs = &ade7854_attribute_group, .read_raw = &ade7854_read_raw, + .write_raw = &ade7854_write_raw, };int ade7854_probe (struct iio_dev *indio_dev,struct device *dev)
Re: [PATCH 3/3] stagging:iio:meter: Add ade7854_write_raw function
In reply to Jonathan CameronQuoted 83 lines · Jonathan Cameron
Quoted 6 lines · On Sat, 21 Apr 2018 08:56:19 -0300 Rodrigo Siqueira…
This patch adds the
ade7854_write_raw()function which is responsible for handling the write operation for registers: AIGAIN, BIGAIN, CIGAIN, NIGAIN, AVGAIN, BVGAIN, and CVGAIN. Finally, this patch completely removes the old ABI used for handling the registers mentioned above.Signed-off-by Rodrigo Siqueira <[email protected]>The baby steps approach here is good, but with all 3 patches as one it would have been a lot easier to follow.
This is almost fine except for the issue I had with how the channels were defined in the first place.
Also a question about scales inline...
Quoted 50 lines
---
drivers/staging/iio/meter/ade7854.c| 60 ++++++++++++-----------------1 file changed, 25 insertions(+), 35 deletions(-)drivers/staging/iio/meter/ade7854.c +3
-28@@ -228,34 +228,6 @@ static int ade7854_reset(struct device *dev)return st->write_reg (dev,ADE7854_CONFIG , val,16 ); } -static IIO_DEV_ATTR_AIGAIN(0644, -NULL , - ade7854_write_24bit, - ADE7854_AIGAIN); -static IIO_DEV_ATTR_BIGAIN(0644, -NULL , - ade7854_write_24bit, - ADE7854_BIGAIN); -static IIO_DEV_ATTR_CIGAIN(0644, -NULL , - ade7854_write_24bit, - ADE7854_CIGAIN); -static IIO_DEV_ATTR_NIGAIN(0644, -NULL , - ade7854_write_24bit, - ADE7854_NIGAIN); -static IIO_DEV_ATTR_AVGAIN(0644, -NULL , - ade7854_write_24bit, - ADE7854_AVGAIN); -static IIO_DEV_ATTR_BVGAIN(0644, -NULL , - ade7854_write_24bit, - ADE7854_BVGAIN); -static IIO_DEV_ATTR_CVGAIN(0644, -NULL , - ade7854_write_24bit, - ADE7854_CVGAIN); static IIO_DEV_ATTR_APPARENT_POWER_A_GAIN(0644, ade7854_read_24bit, ade7854_write_24bit,@@ -497,6 +469,30 @@ static int ade7854_read_raw(struct iio_dev *indio_dev,return -EINVAL ; } +static int ade7854_write_raw (struct iio_dev *indio_dev, +struct iio_chan_spec const *chan, +int val,int val2,long mask)drivers/staging/iio/meter/ade7854.c static int ade7854_read_raw(struct iio_dev *indio_dev, · line 484
Quoted 10 lines
drivers/staging/iio/meter/ade7854.c +10
-0+{ +struct ade7854_state *st =iio_priv (indio_dev); +int ret; + +if (mask !=IIO_CHAN_INFO_SCALE ) +return -EINVAL ; + +switch (chan->type) { +case IIO_CURRENT : +case IIO_VOLTAGE :Probably need some range checking to make sure we have a sane value.
Quoted 6 lines
Also, I'm curious about units here.
you are using
CHAN_INFO_SCALEwhich would mean this is linked to the_RAWvalue (the actual reading of the channel) by _RAW*_SCALE = value in defined base units for the channel type. So I'd expect to see some code here doing a conversion to the internal format for the device.
These should be IIO_CHAN_INFO_HARDWAREGAIN as they are used to correct any imperfections of the sensors. A user space application calculates the error and based on a formula determines the correct register value.
Meter values such as Voltage, Current, Power, Energy use a value/LSB method, which is determined while applying a known voltage and current. After that, any parameter is multiplied by the respective value/LSB to get a real world value.
Regards,
John
Quoted 43 lines
I wouldn't expect to see a value from userspace written unconverted into the register.
(I missed this in the previous patch - sorry).
Jonathan
Quoted 35 lines
drivers/staging/iio/meter/ade7854.c +12
-7+ ret = st->write_reg (&indio_dev->dev, chan->address, val,24 ); +if (ret <0 ) +return ret; +return 0 ; +default : +break ; + } + +return -EINVAL ; +} +static int ade7854_initial_setup (struct iio_dev *indio_dev) {int ret;@@ -521,13 +517,6 @@ static IIO_CONST_ATTR_SAMP_FREQ_AVAIL("8000"); static IIO_CONST_ATTR(name, "ade7854");static struct attribute *ade7854_attributes[] = { - &iio_dev_attr_aigain.dev_attr.attr, - &iio_dev_attr_bigain.dev_attr.attr, - &iio_dev_attr_cigain.dev_attr.attr, - &iio_dev_attr_nigain.dev_attr.attr, - &iio_dev_attr_avgain.dev_attr.attr, - &iio_dev_attr_bvgain.dev_attr.attr, - &iio_dev_attr_cvgain.dev_attr.attr, &iio_dev_attr_linecyc.dev_attr.attr, &iio_dev_attr_sagcyc.dev_attr.attr, &iio_dev_attr_cfcyc.dev_attr.attr,@@ -599,6 +588,7 @@ static const struct attribute_group ade7854_attribute_group = {static const struct iio_info ade7854_info = { .attrs = &ade7854_attribute_group, .read_raw = &ade7854_read_raw, + .write_raw = &ade7854_write_raw, };
Quoted 1 line
drivers/staging/iio/meter/ade7854.c static const struct attribute_group ade7854_attribute_group = { · line 594
Quoted 1 line
drivers/staging/iio/meter/ade7854.c +0
-0int ade7854_probe (struct iio_dev *indio_dev,struct device *dev)
_______________________________________________
devel mailing list
[email protected]
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 3/3] stagging:iio:meter: Add ade7854_write_raw function
In reply to Jonathan CameronAttached is a spreadsheet which should help explain the calibration process.
Here is the calibration guide:
http://www.analog.com/media/en/technical-documentation/application-notes/AN-1076.pdf
Regards,
John
Quoted 130 lines · Jonathan Cameron
Quoted 6 lines · On Sat, 21 Apr 2018 08:56:19 -0300 Rodrigo Siqueira…
This patch adds the
ade7854_write_raw()function which is responsible for handling the write operation for registers: AIGAIN, BIGAIN, CIGAIN, NIGAIN, AVGAIN, BVGAIN, and CVGAIN. Finally, this patch completely removes the old ABI used for handling the registers mentioned above.Signed-off-by Rodrigo Siqueira <[email protected]>The baby steps approach here is good, but with all 3 patches as one it would have been a lot easier to follow.
This is almost fine except for the issue I had with how the channels were defined in the first place.
Also a question about scales inline...
Quoted 50 lines
---
drivers/staging/iio/meter/ade7854.c| 60 ++++++++++++-----------------1 file changed, 25 insertions(+), 35 deletions(-)drivers/staging/iio/meter/ade7854.c +3
-28@@ -228,34 +228,6 @@ static int ade7854_reset(struct device *dev)return st->write_reg (dev,ADE7854_CONFIG , val,16 ); } -static IIO_DEV_ATTR_AIGAIN(0644, -NULL , - ade7854_write_24bit, - ADE7854_AIGAIN); -static IIO_DEV_ATTR_BIGAIN(0644, -NULL , - ade7854_write_24bit, - ADE7854_BIGAIN); -static IIO_DEV_ATTR_CIGAIN(0644, -NULL , - ade7854_write_24bit, - ADE7854_CIGAIN); -static IIO_DEV_ATTR_NIGAIN(0644, -NULL , - ade7854_write_24bit, - ADE7854_NIGAIN); -static IIO_DEV_ATTR_AVGAIN(0644, -NULL , - ade7854_write_24bit, - ADE7854_AVGAIN); -static IIO_DEV_ATTR_BVGAIN(0644, -NULL , - ade7854_write_24bit, - ADE7854_BVGAIN); -static IIO_DEV_ATTR_CVGAIN(0644, -NULL , - ade7854_write_24bit, - ADE7854_CVGAIN); static IIO_DEV_ATTR_APPARENT_POWER_A_GAIN(0644, ade7854_read_24bit, ade7854_write_24bit,@@ -497,6 +469,30 @@ static int ade7854_read_raw(struct iio_dev *indio_dev,return -EINVAL ; } +static int ade7854_write_raw (struct iio_dev *indio_dev, +struct iio_chan_spec const *chan, +int val,int val2,long mask)drivers/staging/iio/meter/ade7854.c static int ade7854_read_raw(struct iio_dev *indio_dev, · line 484
Quoted 10 lines
drivers/staging/iio/meter/ade7854.c +10
-0+{ +struct ade7854_state *st =iio_priv (indio_dev); +int ret; + +if (mask !=IIO_CHAN_INFO_SCALE ) +return -EINVAL ; + +switch (chan->type) { +case IIO_CURRENT : +case IIO_VOLTAGE :Probably need some range checking to make sure we have a sane value.
Also, I'm curious about units here.
you are using
CHAN_INFO_SCALEwhich would mean this is linked to the_RAWvalue (the actual reading of the channel) by _RAW*_SCALE = value in defined base units for the channel type. So I'd expect to see some code here doing a conversion to the internal format for the device.I wouldn't expect to see a value from userspace written unconverted into the register.
(I missed this in the previous patch - sorry).
Jonathan
Quoted 35 lines
drivers/staging/iio/meter/ade7854.c +12
-7+ ret = st->write_reg (&indio_dev->dev, chan->address, val,24 ); +if (ret <0 ) +return ret; +return 0 ; +default : +break ; + } + +return -EINVAL ; +} +static int ade7854_initial_setup (struct iio_dev *indio_dev) {int ret;@@ -521,13 +517,6 @@ static IIO_CONST_ATTR_SAMP_FREQ_AVAIL("8000"); static IIO_CONST_ATTR(name, "ade7854");static struct attribute *ade7854_attributes[] = { - &iio_dev_attr_aigain.dev_attr.attr, - &iio_dev_attr_bigain.dev_attr.attr, - &iio_dev_attr_cigain.dev_attr.attr, - &iio_dev_attr_nigain.dev_attr.attr, - &iio_dev_attr_avgain.dev_attr.attr, - &iio_dev_attr_bvgain.dev_attr.attr, - &iio_dev_attr_cvgain.dev_attr.attr, &iio_dev_attr_linecyc.dev_attr.attr, &iio_dev_attr_sagcyc.dev_attr.attr, &iio_dev_attr_cfcyc.dev_attr.attr,@@ -599,6 +588,7 @@ static const struct attribute_group ade7854_attribute_group = {static const struct iio_info ade7854_info = { .attrs = &ade7854_attribute_group, .read_raw = &ade7854_read_raw, + .write_raw = &ade7854_write_raw, };
Quoted 1 line
drivers/staging/iio/meter/ade7854.c static const struct attribute_group ade7854_attribute_group = { · line 594
Quoted 1 line
drivers/staging/iio/meter/ade7854.c +0
-0int ade7854_probe (struct iio_dev *indio_dev,struct device *dev)
_______________________________________________
devel mailing list
[email protected]
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Jonathan Cameron
maintainerQuoted 10 lines · On Sat, 21 Apr 2018 08:54:45 -0300 Rodrigo Siqueira…
This patchset aims to update ADE7854 by adding the required IIO API components. The first patch adds the
iio_chan_specfor handling seven different registers (all of them with a similar behavior). The second patch appends theread_rawfunction defined by the IIO API. Finally, the third patch adds thewrite_rawfunction and remove the attributes used for handling the seven registers. This patchset has the contribution of John Syne, which was responsible for mapping the correct ABI name per element in the ADE7854; additionally, John provided codes that helped to shape these patches.
Quoted 8 lines
Rodrigo Siqueira (3):
stagging:iio:meter: Addiio_chan_specstagging:iio:meter: Addade7854_read_rawfunction stagging:iio:meter: Addade7854_write_rawfunctiondrivers/staging/iio/meter/ade7854.c | 129 ++++++++++++++++++++-------- 1 file changed, 94 insertions(+), 35 deletions(-)
Hi Rodrigo,
Please don't do it like this. The original discussion with John
involved the addition of an extra chunk of core logic so we would have the ability to specify the channel without using extended_name.
Extended name is not intended to differentiate between channels (it isn't in general enough to do so - though it works in this little corner case of sysfs attributes like you have here).
So the plan is to add another field to the iio_chan_spec structure to allow for the complexity we need. See the long discussions over how we represent the myriad of channels in here.
If you need some pointers, feel free to ask but it may take me a little while to put together a full description of what needs doing.
I'll put a few more direct comments in the individual patches.
Thanks,
Jonathan
_______________________________________________
devel mailing list
[email protected]
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Hi Jonathan,
Thanks for all the feedbacks and sorry for some basic mistakes, I will work on the second version based on all your comments.
Best Regards
Rodrigo Siqueira
Quoted 39 lines · Jonathan Cameron
Quoted 10 lines · On Sat, 21 Apr 2018 08:54:45 -0300 Rodrigo Siqueira…
This patchset aims to update ADE7854 by adding the required IIO API components. The first patch adds the
iio_chan_specfor handling seven different registers (all of them with a similar behavior). The second patch appends theread_rawfunction defined by the IIO API. Finally, the third patch adds thewrite_rawfunction and remove the attributes used for handling the seven registers. This patchset has the contribution of John Syne, which was responsible for mapping the correct ABI name per element in the ADE7854; additionally, John provided codes that helped to shape these patches.Quoted 8 lines
Rodrigo Siqueira (3):
stagging:iio:meter: Addiio_chan_specstagging:iio:meter: Addade7854_read_rawfunction stagging:iio:meter: Addade7854_write_rawfunctiondrivers/staging/iio/meter/ade7854.c | 129 ++++++++++++++++++++-------- 1 file changed, 94 insertions(+), 35 deletions(-)Hi Rodrigo,
Please don't do it like this. The original discussion with John
involved the addition of an extra chunk of core logic so we would have the ability to specify the channel without usingextended_name.Extended name is not intended to differentiate between channels (it isn't in general enough to do so - though it works in this little corner case of sysfs attributes like you have here).
So the plan is to add another field to the
iio_chan_specstructure to allow for the complexity we need. See the long discussions over how we represent the myriad of channels in here.If you need some pointers, feel free to ask but it may take me a little while to put together a full description of what needs doing.
Quoted 6 lines
I'll put a few more direct comments in the individual patches.
Thanks,
Jonathan
_______________________________________________
devel mailing list
[email protected]
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel