staging:iio:meter: Checkpatch cleanup for meter

The macro IIO_DEV_ATTR_CH_OFF is a wrapper for IIO_DEVICE_ATTR, with a tiny change in the name definition. This extra macro does not improve the readability and also creates some checkpatch errors.

This patch fixes the checkpatch.pl errors:

staging/iio/meter/ade7753.c:391: ERROR: Use 4 digit octal (0777) not decimal permissions
staging/iio/meter/ade7753.c:395: ERROR: Use 4 digit octal (0777) not decimal permissions
staging/iio/meter/ade7759.c:331: ERROR: Use 4 digit octal (0777) not decimal permissions
staging/iio/meter/ade7759.c:335: ERROR: Use 4 digit octal (0777) not decimal permissions

Signed-off-by Rodrigo Siqueira <[email protected]>

---
 drivers/staging/iio/meter/ade7753.c | 18 ++++++++++--------
 drivers/staging/iio/meter/ade7759.c | 18 ++++++++++--------
 2 files changed, 20 insertions(+), 16 deletions(-)

drivers/staging/iio/meter/ade7753.c +10 -8

index c44eb577dc35..275e8dfff836 100644
@@ -388,14 +388,16 @@ static IIO_DEV_ATTR_VPERIOD(0444,
 		ade7753_read_16bit,
 		NULL,
 		ADE7753_PERIOD);
-static IIO_DEV_ATTR_CH_OFF(1, 0644,
-		ade7753_read_8bit,
-		ade7753_write_8bit,
-		ADE7753_CH1OS);
-static IIO_DEV_ATTR_CH_OFF(2, 0644,
-		ade7753_read_8bit,
-		ade7753_write_8bit,
-		ADE7753_CH2OS);
+
+static IIO_DEVICE_ATTR(choff_1, 0644,
+			ade7753_read_8bit,
+			ade7753_write_8bit,
+			ADE7753_CH1OS);
+
+static IIO_DEVICE_ATTR(choff_2, 0644,
+			ade7753_read_8bit,
+			ade7753_write_8bit,
+			ADE7753_CH2OS);
 
 static int ade7753_set_irq(struct device *dev, bool enable)
 {

drivers/staging/iio/meter/ade7759.c +3 -8

index 1decb2b8afab..c078b770fa53 100644
@@ -328,14 +328,16 @@ static IIO_DEV_ATTR_ACTIVE_POWER_GAIN(0644,
 		ade7759_read_16bit,
 		ade7759_write_16bit,
 		ADE7759_APGAIN);
-static IIO_DEV_ATTR_CH_OFF(1, 0644,
-		ade7759_read_8bit,
-		ade7759_write_8bit,
-		ADE7759_CH1OS);
-static IIO_DEV_ATTR_CH_OFF(2, 0644,
-		ade7759_read_8bit,
-		ade7759_write_8bit,
-		ADE7759_CH2OS);
+
+static IIO_DEVICE_ATTR(choff_1, 0644,
+			ade7759_read_8bit,

Quoted 15 lines · On Tue, 6 Mar 2018 21:43:47 -0300 Rodrigo Siqueira…

The macro IIO_DEV_ATTR_CH_OFF is a wrapper for IIO_DEVICE_ATTR, with a tiny change in the name definition. This extra macro does not improve the readability and also creates some checkpatch errors.

This patch fixes the checkpatch.pl errors:

staging/iio/meter/ade7753.c:391: ERROR: Use 4 digit octal (0777) not decimal permissions
staging/iio/meter/ade7753.c:395: ERROR: Use 4 digit octal (0777) not decimal permissions
staging/iio/meter/ade7759.c:331: ERROR: Use 4 digit octal (0777) not decimal permissions
staging/iio/meter/ade7759.c:335: ERROR: Use 4 digit octal (0777) not decimal permissions

Quoted 1 line

Signed-off-by Rodrigo Siqueira <[email protected]>

Hmm. I wondered a bit about this one. It's a correct patch in of itself but the interface in question doesn't even vaguely conform to any of defined IIO ABI. Anyhow, it's still and improvement so I'll take it.

Applied to the togreg branch of iio.git and pushed out as testing for the autobuilders to play with it.

I also added the removal of the header define which is no longer used.

Please note, following discussions with Michael, I am going to send an email announcing an intent to drop these meter drivers next cycle unless someone can provide testing for any attempt to move them out of staging. I'm still taking patches on the basis that 'might' happen - but I wouldn't focus on these until we have some certainty on whether they will be around long term!

Jonathan

Quoted 61 lines

---
 drivers/staging/iio/meter/ade7753.c | 18 ++++++++++--------
 drivers/staging/iio/meter/ade7759.c | 18 ++++++++++--------
 2 files changed, 20 insertions(+), 16 deletions(-)

drivers/staging/iio/meter/ade7753.c +10 -8

index c44eb577dc35..275e8dfff836 100644
@@ -388,14 +388,16 @@ static IIO_DEV_ATTR_VPERIOD(0444,
 		ade7753_read_16bit,
 		NULL,
 		ADE7753_PERIOD);
-static IIO_DEV_ATTR_CH_OFF(1, 0644,
-		ade7753_read_8bit,
-		ade7753_write_8bit,
-		ADE7753_CH1OS);
-static IIO_DEV_ATTR_CH_OFF(2, 0644,
-		ade7753_read_8bit,
-		ade7753_write_8bit,
-		ADE7753_CH2OS);
+
+static IIO_DEVICE_ATTR(choff_1, 0644,
+			ade7753_read_8bit,
+			ade7753_write_8bit,
+			ADE7753_CH1OS);
+
+static IIO_DEVICE_ATTR(choff_2, 0644,
+			ade7753_read_8bit,
+			ade7753_write_8bit,
+			ADE7753_CH2OS);
 
 static int ade7753_set_irq(struct device *dev, bool enable)
 {

drivers/staging/iio/meter/ade7759.c +10 -8

index 1decb2b8afab..c078b770fa53 100644
@@ -328,14 +328,16 @@ static IIO_DEV_ATTR_ACTIVE_POWER_GAIN(0644,
 		ade7759_read_16bit,
 		ade7759_write_16bit,
 		ADE7759_APGAIN);
-static IIO_DEV_ATTR_CH_OFF(1, 0644,
-		ade7759_read_8bit,
-		ade7759_write_8bit,
-		ADE7759_CH1OS);
-static IIO_DEV_ATTR_CH_OFF(2, 0644,
-		ade7759_read_8bit,
-		ade7759_write_8bit,
-		ADE7759_CH2OS);
+
+static IIO_DEVICE_ATTR(choff_1, 0644,
+			ade7759_read_8bit,
+			ade7759_write_8bit,
+			ADE7759_CH1OS);
+
+static IIO_DEVICE_ATTR(choff_2, 0644,
+			ade7759_read_8bit,
+			ade7759_write_8bit,
+			ADE7759_CH2OS);
 

drivers/staging/iio/meter/ade7759.c static IIO_DEV_ATTR_ACTIVE_POWER_GAIN(0644, · line 343

Quoted 2 lines

drivers/staging/iio/meter/ade7759.c +0 -0

 static int ade7759_set_irq(struct device *dev, bool enable)
 {

Quoted 18 lines · Jonathan Cameron

Quoted 15 lines · On Tue, 6 Mar 2018 21:43:47 -0300 Rodrigo Siqueira…

The macro IIO_DEV_ATTR_CH_OFF is a wrapper for IIO_DEVICE_ATTR, with a tiny change in the name definition. This extra macro does not improve the readability and also creates some checkpatch errors.

This patch fixes the checkpatch.pl errors:

staging/iio/meter/ade7753.c:391: ERROR: Use 4 digit octal (0777) not decimal permissions
staging/iio/meter/ade7753.c:395: ERROR: Use 4 digit octal (0777) not decimal permissions
staging/iio/meter/ade7759.c:331: ERROR: Use 4 digit octal (0777) not decimal permissions
staging/iio/meter/ade7759.c:335: ERROR: Use 4 digit octal (0777) not decimal permissions

Quoted 6 lines

Quoted 1 line

Signed-off-by Rodrigo Siqueira <[email protected]>

Hmm. I wondered a bit about this one. It's a correct patch in of itself but the interface in question doesn't even vaguely conform to any of defined IIO ABI. Anyhow, it's still and improvement so I'll take it.

I am not sure if I understood the comment about the ABI. The meter interface is wrong because it uses things like IIO_DEVICE_ATTR? It should use iio_info together with *write_raw and *read_raw. Right? Is it the ABI problem that you refer?

Thanks :)

Quoted 76 lines

Applied to the togreg branch of iio.git and pushed out as testing for the autobuilders to play with it.

I also added the removal of the header define which is no longer used.

Please note, following discussions with Michael, I am going to send an email announcing an intent to drop these meter drivers next cycle unless someone can provide testing for any attempt to move them out of staging. I'm still taking patches on the basis that 'might' happen - but I wouldn't focus on these until we have some certainty on whether they will be around long term!

Jonathan

Quoted 61 lines

---
 drivers/staging/iio/meter/ade7753.c | 18 ++++++++++--------
 drivers/staging/iio/meter/ade7759.c | 18 ++++++++++--------
 2 files changed, 20 insertions(+), 16 deletions(-)

drivers/staging/iio/meter/ade7753.c +10 -8

index c44eb577dc35..275e8dfff836 100644
@@ -388,14 +388,16 @@ static IIO_DEV_ATTR_VPERIOD(0444,
 		ade7753_read_16bit,
 		NULL,
 		ADE7753_PERIOD);
-static IIO_DEV_ATTR_CH_OFF(1, 0644,
-		ade7753_read_8bit,
-		ade7753_write_8bit,
-		ADE7753_CH1OS);
-static IIO_DEV_ATTR_CH_OFF(2, 0644,
-		ade7753_read_8bit,
-		ade7753_write_8bit,
-		ADE7753_CH2OS);
+
+static IIO_DEVICE_ATTR(choff_1, 0644,
+			ade7753_read_8bit,
+			ade7753_write_8bit,
+			ADE7753_CH1OS);
+
+static IIO_DEVICE_ATTR(choff_2, 0644,
+			ade7753_read_8bit,
+			ade7753_write_8bit,
+			ADE7753_CH2OS);
 
 static int ade7753_set_irq(struct device *dev, bool enable)
 {

drivers/staging/iio/meter/ade7759.c +10 -8

index 1decb2b8afab..c078b770fa53 100644
@@ -328,14 +328,16 @@ static IIO_DEV_ATTR_ACTIVE_POWER_GAIN(0644,
 		ade7759_read_16bit,
 		ade7759_write_16bit,
 		ADE7759_APGAIN);
-static IIO_DEV_ATTR_CH_OFF(1, 0644,
-		ade7759_read_8bit,
-		ade7759_write_8bit,
-		ADE7759_CH1OS);
-static IIO_DEV_ATTR_CH_OFF(2, 0644,
-		ade7759_read_8bit,
-		ade7759_write_8bit,
-		ADE7759_CH2OS);
+
+static IIO_DEVICE_ATTR(choff_1, 0644,
+			ade7759_read_8bit,
+			ade7759_write_8bit,
+			ADE7759_CH1OS);
+
+static IIO_DEVICE_ATTR(choff_2, 0644,
+			ade7759_read_8bit,
+			ade7759_write_8bit,
+			ADE7759_CH2OS);
 

Quoted 3 lines

drivers/staging/iio/meter/ade7759.c static IIO_DEV_ATTR_ACTIVE_POWER_GAIN(0644, · line 343

Quoted 2 lines

drivers/staging/iio/meter/ade7759.c +0 -0

 static int ade7759_set_irq(struct device *dev, bool enable)
 {  

--
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 26 lines · On Thu, 8 Mar 2018 21:37:33 -0300 Rodrigo Siqueira…

Quoted 18 lines · Jonathan Cameron

Quoted 15 lines · On Tue, 6 Mar 2018 21:43:47 -0300 Rodrigo Siqueira…

The macro IIO_DEV_ATTR_CH_OFF is a wrapper for IIO_DEVICE_ATTR, with a tiny change in the name definition. This extra macro does not improve the readability and also creates some checkpatch errors.

This patch fixes the checkpatch.pl errors:

staging/iio/meter/ade7753.c:391: ERROR: Use 4 digit octal (0777) not decimal permissions
staging/iio/meter/ade7753.c:395: ERROR: Use 4 digit octal (0777) not decimal permissions
staging/iio/meter/ade7759.c:331: ERROR: Use 4 digit octal (0777) not decimal permissions
staging/iio/meter/ade7759.c:335: ERROR: Use 4 digit octal (0777) not decimal permissions

Quoted 6 lines

Quoted 1 line

Signed-off-by Rodrigo Siqueira <[email protected]>

Hmm. I wondered a bit about this one. It's a correct patch in of itself but the interface in question doesn't even vaguely conform to any of defined IIO ABI. Anyhow, it's still and improvement so I'll take it.

Quoted 4 lines

I am not sure if I understood the comment about the ABI. The meter interface is wrong because it uses things like IIO_DEVICE_ATTR? It should use iio_info together with *write_raw and *read_raw. Right? Is it the ABI problem that you refer?

The ABI is about the userspace interface of IIO. It is defined in Documentation/ABI/testing/sysfs-bus-iio*
So this documents the naming of sysfs attributes and (more or less) describes a consistent interface to userspace across lots of different types of devices.

A lot of these older drivers in staging involve a good deal of ABI that was not reviewed or discussed. That is one of the biggest reasons we didn't take them out of staging in the first place.

In order for generic userspace programs to have any idea what to do with these devices this all needs to be fixed.

There may well be cases where we need to expand the existing ABI to
cover new things. That's fine, but it has to be done with full
review of the relevant documentation patches.

Incidentally if you want an easy driver to work on moving out of staging then first thing to do is to compare what it shows to userspace with these docs. If it's totally different then you have a big job on your hands as often ABI can take a lot of discussion and a long time to establish a consensus.

Jonathan

Quoted 79 lines

Thanks :)

Quoted 76 lines

Applied to the togreg branch of iio.git and pushed out as testing for the autobuilders to play with it.

I also added the removal of the header define which is no longer used.

Please note, following discussions with Michael, I am going to send an email announcing an intent to drop these meter drivers next cycle unless someone can provide testing for any attempt to move them out of staging. I'm still taking patches on the basis that 'might' happen - but I wouldn't focus on these until we have some certainty on whether they will be around long term!

Jonathan

Quoted 61 lines

---
drivers/staging/iio/meter/ade7753.c | 18 ++++++++++-------- drivers/staging/iio/meter/ade7759.c | 18 ++++++++++--------

2 files changed, 20 insertions(+), 16 deletions(-)

drivers/staging/iio/meter/ade7753.c +10 -8

index c44eb577dc35..275e8dfff836 100644
@@ -388,14 +388,16 @@ static IIO_DEV_ATTR_VPERIOD(0444,
 		ade7753_read_16bit,
 		NULL,
 		ADE7753_PERIOD);
-static IIO_DEV_ATTR_CH_OFF(1, 0644,
-		ade7753_read_8bit,
-		ade7753_write_8bit,
-		ADE7753_CH1OS);
-static IIO_DEV_ATTR_CH_OFF(2, 0644,
-		ade7753_read_8bit,
-		ade7753_write_8bit,
-		ADE7753_CH2OS);
+
+static IIO_DEVICE_ATTR(choff_1, 0644,
+			ade7753_read_8bit,
+			ade7753_write_8bit,
+			ADE7753_CH1OS);
+
+static IIO_DEVICE_ATTR(choff_2, 0644,
+			ade7753_read_8bit,
+			ade7753_write_8bit,
+			ADE7753_CH2OS);
 
 static int ade7753_set_irq(struct device *dev, bool enable)
 {

drivers/staging/iio/meter/ade7759.c +10 -8

index 1decb2b8afab..c078b770fa53 100644
@@ -328,14 +328,16 @@ static IIO_DEV_ATTR_ACTIVE_POWER_GAIN(0644,
 		ade7759_read_16bit,
 		ade7759_write_16bit,
 		ADE7759_APGAIN);
-static IIO_DEV_ATTR_CH_OFF(1, 0644,
-		ade7759_read_8bit,
-		ade7759_write_8bit,
-		ADE7759_CH1OS);
-static IIO_DEV_ATTR_CH_OFF(2, 0644,
-		ade7759_read_8bit,
-		ade7759_write_8bit,
-		ADE7759_CH2OS);
+
+static IIO_DEVICE_ATTR(choff_1, 0644,
+			ade7759_read_8bit,
+			ade7759_write_8bit,
+			ADE7759_CH1OS);
+
+static IIO_DEVICE_ATTR(choff_2, 0644,
+			ade7759_read_8bit,
+			ade7759_write_8bit,
+			ADE7759_CH2OS);
 

Quoted 7 lines

Quoted 3 lines

drivers/staging/iio/meter/ade7759.c static IIO_DEV_ATTR_ACTIVE_POWER_GAIN(0644, · line 343

Quoted 2 lines

drivers/staging/iio/meter/ade7759.c +0 -0

 static int ade7759_set_irq(struct device *dev, bool enable)
 {  

--
To unsubscribe from this list: send the line "unsubscribe linux-iio" in
the body of a message to [email protected] <mailto:[email protected]>
More majordomo info at http://vger.kernel.org/majordomo-info.html <http://vger.kernel.org/majordomo-info.html>

_______________________________________________
devel mailing list
[email protected] <mailto:[email protected]>
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel <http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel>

On Mar 10, 2018, at 7:10 AM, Jonathan Cameron <[email protected]> =

=20
On Thu, 8 Mar 2018 21:37:33 -0300
Rodrigo Siqueira <[email protected]> wrote:
=20

Quoted 5 lines · Jonathan Cameron

Quoted 4 lines

On Tue, 6 Mar 2018 21:43:47 -0300
Rodrigo Siqueira <[email protected]> wrote:
=20

Quoted 1 line

The macro IIO_DEV_ATTR_CH_OFF is a wrapper for IIO_DEVICE_ATTR, =

Quoted 1 line

Quoted 1 line

Quoted 1 line

tiny change in the name definition. This extra macro does not =

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

the readability and also creates some checkpatch errors. =20
This patch fixes the checkpatch.pl errors:
=20
staging/iio/meter/ade7753.c:391: ERROR: Use 4 digit octal (0777) =

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

decimal permissions
staging/iio/meter/ade7753.c:395: ERROR: Use 4 digit octal (0777) =

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

decimal permissions
staging/iio/meter/ade7759.c:331: ERROR: Use 4 digit octal (0777) =

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

decimal permissions
staging/iio/meter/ade7759.c:335: ERROR: Use 4 digit octal (0777) =

Quoted 8 lines

Quoted 8 lines

Quoted 3 lines

decimal permissions
=20

Signed-off-by Rodrigo Siqueira <[email protected]> =20

=20
Hmm. I wondered a bit about this one. It's a correct patch in of itself but the interface in question doesn't even vaguely conform to any of defined IIO ABI. Anyhow, it's still and improvement so I'll take it. =20

Quoted 4 lines

=20
I am not sure if I understood the comment about the ABI. The meter interface is wrong because it uses things like IIO_DEVICE_ATTR? It should use iio_info together with *write_raw and *read_raw. Right? Is =

Quoted 1 line

the ABI problem that you refer?

The ABI is about the userspace interface of IIO. It is defined in Documentation/ABI/testing/sysfs-bus-iio*
So this documents the naming of sysfs attributes and (more or less) describes a consistent interface to userspace across lots of different types of devices.
=20
A lot of these older drivers in staging involve a good deal of ABI =

was not reviewed or discussed. That is one of the biggest reasons we didn't take them out of staging in the first place.
=20
In order for generic userspace programs to have any idea what to do with these devices this all needs to be fixed.
=20
There may well be cases where we need to expand the existing ABI to
cover new things. That's fine, but it has to be done with full
review of the relevant documentation patches.
=20
Incidentally if you want an easy driver to work on moving out of =

then first thing to do is to compare what it shows to userspace with =

docs. If it's totally different then you have a big job on your hands as often ABI can take a lot of discussion and a long time to establish a consensus.
=20
Jonathan
=20
=20

Quoted 18 lines

=20
Thanks :)
=20

Quoted 15 lines

Applied to the togreg branch of iio.git and pushed out as testing for the autobuilders to play with it.
=20
I also added the removal of the header define which is no longer used.
=20
Please note, following discussions with Michael, I am going to send an email announcing an intent to drop these meter drivers next cycle unless someone can provide testing for any attempt to move them out of staging. I'm still taking patches on the basis that 'might' happen - but I wouldn't focus on these until we have some certainty on whether they will be around long term! =20
Jonathan
=20

Quoted 6 lines

Quoted 6 lines

=

Quoted 6 lines

---
drivers/staging/iio/meter/ade7753.c | 18 ++++++++++-------- drivers/staging/iio/meter/ade7759.c | 18 ++++++++++--------

2 files changed, 20 insertions(+), 16 deletions(-)

=20

= renamed from drivers/staging/iio/meter/ade7753.c +0 -0

Quoted 21 lines

Quoted 21 lines

Quoted 21 lines

index c44eb577dc35..275e8dfff836 100644

drivers/staging/iio/meter/ade7753.c +6 -8

@@ -388,14 +388,16 @@ static IIO_DEV_ATTR_VPERIOD(0444,
 		ade7753_read_16bit,
 		NULL,
 		ADE7753_PERIOD);
-static IIO_DEV_ATTR_CH_OFF(1, 0644,
-		ade7753_read_8bit,
-		ade7753_write_8bit,
-		ADE7753_CH1OS);
-static IIO_DEV_ATTR_CH_OFF(2, 0644,
-		ade7753_read_8bit,
-		ade7753_write_8bit,
-		ADE7753_CH2OS);
+
+static IIO_DEVICE_ATTR(choff_1, 0644,
+			ade7753_read_8bit,
+			ade7753_write_8bit,
+			ADE7753_CH1OS);
+

Quoted 8 lines

Quoted 8 lines

=

Quoted 8 lines

drivers/staging/iio/meter/ade7753.c +4 -0

+static IIO_DEVICE_ATTR(choff_2, 0644,
+			ade7753_read_8bit,
+			ade7753_write_8bit,
+			ADE7753_CH2OS);

=20
static int ade7753_set_irq(struct device *dev, bool enable) {

= renamed from drivers/staging/iio/meter/ade7759.c +0 -0

Quoted 29 lines

Quoted 29 lines

Quoted 25 lines

index 1decb2b8afab..c078b770fa53 100644

drivers/staging/iio/meter/ade7759.c +10 -8

@@ -328,14 +328,16 @@ static IIO_DEV_ATTR_ACTIVE_POWER_GAIN(0644,
 		ade7759_read_16bit,
 		ade7759_write_16bit,
 		ADE7759_APGAIN);
-static IIO_DEV_ATTR_CH_OFF(1, 0644,
-		ade7759_read_8bit,
-		ade7759_write_8bit,
-		ADE7759_CH1OS);
-static IIO_DEV_ATTR_CH_OFF(2, 0644,
-		ade7759_read_8bit,
-		ade7759_write_8bit,
-		ADE7759_CH2OS);
+
+static IIO_DEVICE_ATTR(choff_1, 0644,
+			ade7759_read_8bit,
+			ade7759_write_8bit,
+			ADE7759_CH1OS);
+
+static IIO_DEVICE_ATTR(choff_2, 0644,
+			ade7759_read_8bit,
+			ade7759_write_8bit,
+			ADE7759_CH2OS);

Quoted 3 lines

=20
static int ade7759_set_irq(struct device *dev, bool enable) { =20

=20

Quoted 2 lines

--
To unsubscribe from this list: send the line "unsubscribe linux-iio" =

Quoted 2 lines

the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

=20
_______________________________________________
devel mailing list
[email protected]
=

Hi Jonathan,

I have been looking at the IIO ABI docs and if I understand correctly, the idea is to use consistent naming conventions? So for example, looking at the ADE7854 datasheet, the naming matching the ADE7854 registers would be as follows:

{direction}_{type}_{index}_{modifier}_{info_mask}

AIGAIN - In_current_a_gain

AVGAIN - in_voltage_a_gain
BIGAIN - in_current_b_gain
BVGAIN - in_voltage_b_gain
—
How do we represent the rms and offset
AIRMSOS - in_current_a_rmsoffset
AVRMSOS - in_voltage_a_rmsoffset

—
Here I don’t understand how to represent both the phase and the active/reactive/apparent power components. Do we combine the phase and quadrature part like this
AVAGAIN - in_power_a_gain /* apparent power */
—
AWGAIN - in_power_ai_gain /* active power */

—
AVARGAIN - in_power_aq_gain /* reactive power */
—
Now here it becomes more complicated. Not sure how this gets handled.
AFWATTOS - in_power_a_active/fundamental/offset

—
AWATTHR - in_energy_ai_accumulation

—
AVARHR - in_energy_aq_accumulation
—
IPEAK - in_current_peak

—

I’ll leave it there, because there are some even more complicated register naming issues.

Regards,
John

Quoted 152 lines · Jonathan Cameron

Quoted 26 lines · On Thu, 8 Mar 2018 21:37:33 -0300 Rodrigo Siqueira…

Quoted 18 lines · Jonathan Cameron

Quoted 15 lines · On Tue, 6 Mar 2018 21:43:47 -0300 Rodrigo Siqueira…

The macro IIO_DEV_ATTR_CH_OFF is a wrapper for IIO_DEVICE_ATTR, with a tiny change in the name definition. This extra macro does not improve the readability and also creates some checkpatch errors.

This patch fixes the checkpatch.pl errors:

staging/iio/meter/ade7753.c:391: ERROR: Use 4 digit octal (0777) not decimal permissions
staging/iio/meter/ade7753.c:395: ERROR: Use 4 digit octal (0777) not decimal permissions
staging/iio/meter/ade7759.c:331: ERROR: Use 4 digit octal (0777) not decimal permissions
staging/iio/meter/ade7759.c:335: ERROR: Use 4 digit octal (0777) not decimal permissions

Quoted 6 lines

Quoted 1 line

Signed-off-by Rodrigo Siqueira <[email protected]>

Hmm. I wondered a bit about this one. It's a correct patch in of itself but the interface in question doesn't even vaguely conform to any of defined IIO ABI. Anyhow, it's still and improvement so I'll take it.

Quoted 4 lines

I am not sure if I understood the comment about the ABI. The meter interface is wrong because it uses things like IIO_DEVICE_ATTR? It should use iio_info together with *write_raw and *read_raw. Right? Is it the ABI problem that you refer?

The ABI is about the userspace interface of IIO. It is defined in Documentation/ABI/testing/sysfs-bus-iio*
So this documents the naming of sysfs attributes and (more or less) describes a consistent interface to userspace across lots of different types of devices.

A lot of these older drivers in staging involve a good deal of ABI that was not reviewed or discussed. That is one of the biggest reasons we didn't take them out of staging in the first place.

In order for generic userspace programs to have any idea what to do with these devices this all needs to be fixed.

There may well be cases where we need to expand the existing ABI to
cover new things. That's fine, but it has to be done with full
review of the relevant documentation patches.

Incidentally if you want an easy driver to work on moving out of staging then first thing to do is to compare what it shows to userspace with these docs. If it's totally different then you have a big job on your hands as often ABI can take a lot of discussion and a long time to establish a consensus.

Jonathan

Quoted 79 lines

Thanks :)

Quoted 76 lines

Applied to the togreg branch of iio.git and pushed out as testing for the autobuilders to play with it.

I also added the removal of the header define which is no longer used.

Please note, following discussions with Michael, I am going to send an email announcing an intent to drop these meter drivers next cycle unless someone can provide testing for any attempt to move them out of staging. I'm still taking patches on the basis that 'might' happen - but I wouldn't focus on these until we have some certainty on whether they will be around long term!

Jonathan

Quoted 61 lines

---
drivers/staging/iio/meter/ade7753.c | 18 ++++++++++-------- drivers/staging/iio/meter/ade7759.c | 18 ++++++++++--------

2 files changed, 20 insertions(+), 16 deletions(-)

drivers/staging/iio/meter/ade7753.c +10 -8

index c44eb577dc35..275e8dfff836 100644
@@ -388,14 +388,16 @@ static IIO_DEV_ATTR_VPERIOD(0444,
 		ade7753_read_16bit,
 		NULL,
 		ADE7753_PERIOD);
-static IIO_DEV_ATTR_CH_OFF(1, 0644,
-		ade7753_read_8bit,
-		ade7753_write_8bit,
-		ADE7753_CH1OS);
-static IIO_DEV_ATTR_CH_OFF(2, 0644,
-		ade7753_read_8bit,
-		ade7753_write_8bit,
-		ADE7753_CH2OS);
+
+static IIO_DEVICE_ATTR(choff_1, 0644,
+			ade7753_read_8bit,
+			ade7753_write_8bit,
+			ADE7753_CH1OS);
+
+static IIO_DEVICE_ATTR(choff_2, 0644,
+			ade7753_read_8bit,
+			ade7753_write_8bit,
+			ADE7753_CH2OS);
 
 static int ade7753_set_irq(struct device *dev, bool enable)
 {

drivers/staging/iio/meter/ade7759.c +10 -8

index 1decb2b8afab..c078b770fa53 100644
@@ -328,14 +328,16 @@ static IIO_DEV_ATTR_ACTIVE_POWER_GAIN(0644,
 		ade7759_read_16bit,
 		ade7759_write_16bit,
 		ADE7759_APGAIN);
-static IIO_DEV_ATTR_CH_OFF(1, 0644,
-		ade7759_read_8bit,
-		ade7759_write_8bit,
-		ADE7759_CH1OS);
-static IIO_DEV_ATTR_CH_OFF(2, 0644,
-		ade7759_read_8bit,
-		ade7759_write_8bit,
-		ADE7759_CH2OS);
+
+static IIO_DEVICE_ATTR(choff_1, 0644,
+			ade7759_read_8bit,
+			ade7759_write_8bit,
+			ADE7759_CH1OS);
+
+static IIO_DEVICE_ATTR(choff_2, 0644,
+			ade7759_read_8bit,
+			ade7759_write_8bit,
+			ADE7759_CH2OS);
 

Quoted 7 lines

Quoted 3 lines

drivers/staging/iio/meter/ade7759.c static IIO_DEV_ATTR_ACTIVE_POWER_GAIN(0644, · line 343

Quoted 2 lines

drivers/staging/iio/meter/ade7759.c +0 -0

 static int ade7759_set_irq(struct device *dev, bool enable)
 {    

--
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

_______________________________________________
devel mailing list
[email protected]
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

On Mar 17, 2018, at 1:30 PM, Jonathan Cameron <[email protected]> =

=20
On Wed, 14 Mar 2018 23:12:02 -0700
John Syne <[email protected]> wrote:
=20

Quoted 6 lines

Hi Jonathan,
=20
I have been looking at the IIO ABI docs and if I understand correctly, the idea is to use consistent naming conventions? So for example, looking at the ADE7854 datasheet, the naming matching the ADE7854 registers would be as follows:

=20
Welcome to one of the big reasons no one tidied these drivers up originally. Still we have moved on somewhat since then so similar circumstances have come up in other types of sensor. =20

Quoted 4 lines

=20
{direction}_{type}_{index}_{modifier}_{info_mask}
=20
AIGAIN - In_current_a_gain

Other than the fact that gain isn't an ABI element and that index doesn't have
_ before it that is right.
in_voltageA_scale
=20
That was a weird quirk a long time back which we should probably not have done (copied it from hwmon)
=20

Quoted 7 lines

AVGAIN - in_voltage_a_gain
BIGAIN - in_current_b_gain
BVGAIN - in_voltage_b_gain
=E2=80=94
How do we represent the rms and offset
AIRMSOS - in_current_a_rmsoffset
AVRMSOS - in_voltage_a_rmsoffset

=20
Right now we can't unfortunately though this one is easier to fix. We already have modifiers for multi axis devices doing sum_squared so add one of those for root_mean_square - this one is well known enough that rms is fine in the string.
=20
It's a effectively a different channel be it one derived from a simple one. This is going to get tricky however as we would normally use modifier to specialise a channel type - thoughts on this below. =20

Quoted 2 lines

=E2=80=94
Here I don=E2=80=99t understand how to represent both the phase and =

Quoted 1 line

AVAGAIN - in_power_a_gain =

Quoted 2 lines

=E2=80=94
AWGAIN - in_power_ai_gain =

And that is the problem. How do we represent the various power types. Hmm. We could do it with modifiers but above we show that we have =

=20
It would be easy enough to add yet another field to the channel spec =

this but there is a problem - Events. The event format is already =

so where do we put this extra element if we need to define a channel =

only by it.
=20
One thought is we could instead define these as different top level IIO_CHAN_TYPES in a similar fashion to we do for relative humidity vs the proposed absolute humidity.

=20
in_powerreactiveA_scale
in_poweractivceA_scale
(or in_powerrealA_scale to match with what I got taught years ago?) =20
I presume we keep in_powerA_scale etc for the apparent power and modify any docs to make that clear.
=20

Quoted 2 lines

=E2=80=94
AVARGAIN - in_power_aq_gain =

Quoted 4 lines

=E2=80=94
Now here it becomes more complicated. Not sure how this gets handled.=20=

AFWATTOS - in_power_a_active/fundamental/offset

Yeah, some of these are getting odd.
If I read this correctly this is the active power estimate based on =

the fundamental frequency - so no harmonics?
=20
Hmm. This then becomes a separate channel with additional properties specifying it is only the fundamental. This feels a bit like a filter be it an unusual one? Might just be necessary to add a =

element on the end (would be info_mask if this is common enough to justify that rather than using the extended methods to define it.). =20
=20

Quoted 2 lines

=E2=80=94
AWATTHR - in_energy_ai_accumulation

Great, just when I thought we had gone far enough they define reactive energy which is presumably roughly the same as reactivepower * time? In that case we need types for that as well. in_energyreactiveA_*
in_energyactiveA_*
=20

Quoted 4 lines

=E2=80=94
AVARHR - in_energy_aq_accumulation
=E2=80=94
IPEAK - in_current_peak

That one is easy as we have an info_mask element for peak and one for peak_scale that has always been a bit odd but was needed =

=20

Quoted 3 lines

=E2=80=94
=20
I=E2=80=99ll leave it there, because there are some even more =

Something to look forward to. Gah, I always hated power engineering though I taught it very briefly (I really pity those students :( =20
Jonathan
=20

Quoted 9 lines

=20
Regards,
John
=20
=20
=20
=20
=20

Quoted 1 line

On Mar 10, 2018, at 7:10 AM, Jonathan Cameron <[email protected]> =

Quoted 9 lines

Quoted 9 lines

=20
On Thu, 8 Mar 2018 21:37:33 -0300
Rodrigo Siqueira <[email protected]> wrote:
=20

Quoted 5 lines

On 03/07, Jonathan Cameron wrote: =20

Quoted 4 lines

On Tue, 6 Mar 2018 21:43:47 -0300
Rodrigo Siqueira <[email protected]> wrote:
=20

Quoted 1 line

The macro IIO_DEV_ATTR_CH_OFF is a wrapper for IIO_DEVICE_ATTR, =

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

tiny change in the name definition. This extra macro does not =

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

the readability and also creates some checkpatch errors. =20
This patch fixes the checkpatch.pl errors:
=20
staging/iio/meter/ade7753.c:391: ERROR: Use 4 digit octal (0777) =

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

decimal permissions
staging/iio/meter/ade7753.c:395: ERROR: Use 4 digit octal (0777) =

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

decimal permissions
staging/iio/meter/ade7759.c:331: ERROR: Use 4 digit octal (0777) =

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

decimal permissions
staging/iio/meter/ade7759.c:335: ERROR: Use 4 digit octal (0777) =

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

decimal permissions
=20

Signed-off-by Rodrigo Siqueira <[email protected]> =

Quoted 9 lines

Quoted 9 lines

Quoted 9 lines

Quoted 5 lines

=20
Hmm. I wondered a bit about this one. It's a correct patch in of itself but the interface in question doesn't even vaguely conform to any of defined IIO ABI. Anyhow, it's still and improvement so
I'll take it. =20

=20
I am not sure if I understood the comment about the ABI. The meter interface is wrong because it uses things like IIO_DEVICE_ATTR? It should use iio_info together with *write_raw and *read_raw. Right? =

Quoted 5 lines

Quoted 5 lines

Quoted 1 line

the ABI problem that you refer? =20

The ABI is about the userspace interface of IIO. It is defined in Documentation/ABI/testing/sysfs-bus-iio*
So this documents the naming of sysfs attributes and (more or less) describes a consistent interface to userspace across lots of =

Quoted 3 lines

Quoted 3 lines

types of devices.
=20
A lot of these older drivers in staging involve a good deal of ABI =

Quoted 1 line

Quoted 1 line

was not reviewed or discussed. That is one of the biggest reasons =

Quoted 10 lines

Quoted 10 lines

didn't take them out of staging in the first place.
=20
In order for generic userspace programs to have any idea what to do with these devices this all needs to be fixed.
=20
There may well be cases where we need to expand the existing ABI to
cover new things. That's fine, but it has to be done with full
review of the relevant documentation patches.
=20
Incidentally if you want an easy driver to work on moving out of =

Quoted 1 line

Quoted 1 line

then first thing to do is to compare what it shows to userspace with =

Quoted 1 line

Quoted 1 line

docs. If it's totally different then you have a big job on your =

Quoted 1 line

Quoted 1 line

as often ABI can take a lot of discussion and a long time to =

Quoted 8 lines

Quoted 8 lines

a consensus.
=20
Jonathan
=20
=20

Quoted 3 lines

=20
Thanks :)
=20

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

Applied to the togreg branch of iio.git and pushed out as testing for the autobuilders to play with it.
=20
I also added the removal of the header define which is no longer used.
=20
Please note, following discussions with Michael, I am going to =

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

an email announcing an intent to drop these meter drivers next cycle unless someone can provide testing for any attempt to move them out of staging. I'm still taking patches on the basis that 'might' happen - but I wouldn't focus on these until we have some certainty on whether they will be around long term! =20
Jonathan
=20

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

=

Quoted 6 lines

---
drivers/staging/iio/meter/ade7753.c | 18 ++++++++++-------- drivers/staging/iio/meter/ade7759.c | 18 ++++++++++--------

2 files changed, 20 insertions(+), 16 deletions(-)

=20

= renamed from drivers/staging/iio/meter/ade7753.c +0 -0

Quoted 21 lines

Quoted 21 lines

Quoted 21 lines

Quoted 21 lines

Quoted 21 lines

index c44eb577dc35..275e8dfff836 100644

drivers/staging/iio/meter/ade7753.c +6 -8

@@ -388,14 +388,16 @@ static IIO_DEV_ATTR_VPERIOD(0444,
 		ade7753_read_16bit,
 		NULL,
 		ADE7753_PERIOD);
-static IIO_DEV_ATTR_CH_OFF(1, 0644,
-		ade7753_read_8bit,
-		ade7753_write_8bit,
-		ADE7753_CH1OS);
-static IIO_DEV_ATTR_CH_OFF(2, 0644,
-		ade7753_read_8bit,
-		ade7753_write_8bit,
-		ADE7753_CH2OS);
+
+static IIO_DEVICE_ATTR(choff_1, 0644,
+			ade7753_read_8bit,
+			ade7753_write_8bit,
+			ADE7753_CH1OS);
+

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

=

Quoted 8 lines

drivers/staging/iio/meter/ade7753.c +4 -0

+static IIO_DEVICE_ATTR(choff_2, 0644,
+			ade7753_read_8bit,
+			ade7753_write_8bit,
+			ADE7753_CH2OS);

=20
static int ade7753_set_irq(struct device *dev, bool enable) {

= renamed from drivers/staging/iio/meter/ade7759.c +0 -0

Quoted 29 lines

Quoted 29 lines

Quoted 29 lines

Quoted 29 lines

Quoted 25 lines

index 1decb2b8afab..c078b770fa53 100644

drivers/staging/iio/meter/ade7759.c +10 -8

@@ -328,14 +328,16 @@ static IIO_DEV_ATTR_ACTIVE_POWER_GAIN(0644,
 		ade7759_read_16bit,
 		ade7759_write_16bit,
 		ADE7759_APGAIN);
-static IIO_DEV_ATTR_CH_OFF(1, 0644,
-		ade7759_read_8bit,
-		ade7759_write_8bit,
-		ADE7759_CH1OS);
-static IIO_DEV_ATTR_CH_OFF(2, 0644,
-		ade7759_read_8bit,
-		ade7759_write_8bit,
-		ADE7759_CH2OS);
+
+static IIO_DEVICE_ATTR(choff_1, 0644,
+			ade7759_read_8bit,
+			ade7759_write_8bit,
+			ADE7759_CH1OS);
+
+static IIO_DEVICE_ATTR(choff_2, 0644,
+			ade7759_read_8bit,
+			ade7759_write_8bit,
+			ADE7759_CH2OS);

Quoted 3 lines

=20
static int ade7759_set_irq(struct device *dev, bool enable)
{ =20

=20

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

--
To unsubscribe from this list: send the line "unsubscribe =

Quoted 8 lines

Quoted 8 lines

Quoted 2 lines

the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html =20=

=20
_______________________________________________
devel mailing list
[email protected]
=

Quoted 1 line

=20

=20

Hi Jonathan,

Here is the complete list of registers for the ADE7878 which I copied from the data sheet. I added a column “IIO Attribute” which I hope follows your IIO ABI. Please make any changes you feel are incorrect. BTW, there are several registers that cannot be generalized and are used purely for chip configuration. I think we should add a new naming convention, namely {register}_{<chip-register-name>}. Also, I see in the sys_bus_iio doc in_accel_x_peak_raw

so shouldn’t the phase be represented as follows:

in_current_A_scale

But for now, I followed your instructions from your reply.

After finalizing this one, I will work on the ADE9000, which as way more registers ;-)

Once we can agree on the register naming, I will update the ADE7854 driver for Rodrigo, which will go a long way to getting it out of staging.

Address Register IIO Attribute R/W Bit Length Bit Length During Communications Type Default Value Description
0x4380 AIGAIN in_current0_phaseA_scale R/W 24 32 ZPSE S 0x000000 Phase A current gain adjust.

0x4381 AVGAIN in_voltage0_phaseA_scale R/W 24 32 ZPSE S 0x000000 Phase A voltage gain adjust.
0x4382 BIGAIN in_current0_phaseB_scale R/W 24 32 ZPSE S 0x000000 Phase B current gain adjust.
0x4383 BVGAIN in_voltage0_phaseB_scale R/W 24 32 ZPSE S 0x000000 Phase B voltage gain adjust.
0x4384 CIGAIN in_current0_phaseC_scale R/W 24 32 ZPSE S 0x000000 Phase C current gain adjust.
0x4385 CVGAIN in_voltage0_phaseC_scale R/W 24 32 ZPSE S 0x000000 Phase C voltage gain adjust.
0x4386 NIGAIN in_current0_neutral_scale R/W 24 32 ZPSE S 0x000000 Neutral current gain adjust (ADE7868 and ADE7878 only).
0x4387 AIRMSOS in_current0_phaseA_rms_offset R/W 24 32 ZPSE S 0x000000 Phase A current rms offset.
0x4388 AVRMSOS in_voltage0_phaseA_rms_offset R/W 24 32 ZPSE S 0x000000 Phase A voltage rms offset.
0x4389 BIRMSOS in_current0_phaseB_rms_offset R/W 24 32 ZPSE S 0x000000 Phase B current rms offset.
0x438A BVRMSOS in_voltage0_phaseB_rms_offset R/W 24 32 ZPSE S 0x000000 Phase B voltage rms offset.
0x438B CIRMSOS in_current0_phaseC_rms_offset R/W 24 32 ZPSE S 0x000000 Phase C current rms offset.
0x438C CVRMSOS in_voltage0_phaseC_rms_offset R/W 24 32 ZPSE S 0x000000 Phase C voltage rms offset.
0x438D NIRMSOS in_current0_neutral_rms_offset R/W 24 32 ZPSE S 0x000000 Neutral current rms offset (ADE7868 and ADE7878 only).
0x438E AVAGAIN in_powerapparent0_phaseA_scale R/W 24 32 ZPSE S 0x000000 Phase A apparent power gain adjust.
0x438F BVAGAIN in_powerapparent0_phaseB_scale R/W 24 32 ZPSE S 0x000000 Phase B apparent power gain adjust.
0x4390 CVAGAIN in_powerapparent0_phaseC_scale R/W 24 32 ZPSE S 0x000000 Phase C apparent power gain adjust.
0x4391 AWGAIN in_power0_phaseA_scale R/W 24 32 ZPSE S 0x000000 Phase A total active power gain adjust.
0x4392 AWATTOS in_power0_phaseA_offset R/W 24 32 ZPSE S 0x000000 Phase A total active power offset adjust.
0x4393 BWGAIN in_power0_phaseB_scale R/W 24 32 ZPSE S 0x000000 Phase B total active power gain adjust.
0x4394 BWATTOS in_power0_phaseB_offset R/W 24 32 ZPSE S 0x000000 Phase B total active power offset adjust.
0x4395 CWGAIN in_power0_PhaseC_scale R/W 24 32 ZPSE S 0x000000 Phase C total active power gain adjust.

0x4396 CWATTOS in_power0_phaseC_offset R/W 24 32 ZPSE S 0x000000 Phase C total active power offset adjust.
0x4397 AVARGAIN in_powerreactive0_phaseA_scale R/W 24 32 ZPSE S 0x000000 Phase A total reactive power gain adjust (ADE7858, ADE7868, and ADE7878).
0x4398 AVAROS in_powerreactive0_phaseA_offset R/W 24 32 ZPSE S 0x000000 Phase A total reactive power offset adjust (ADE7858, ADE7868, and ADE7878).
0x4399 BVARGAIN in_powerreactive0_phaseB_scale R/W 24 32 ZPSE S 0x000000 Phase B total reactive power gain adjust (ADE7858, ADE7868, and ADE7878).
0x439A BVAROS in_powerreactive0_phaseB_offset R/W 24 32 ZPSE S 0x000000 Phase B total reactive power offset adjust (ADE7858, ADE7868, and ADE7878).
0x439B CVARGAIN in_powerreactive0_phaseC_scale R/W 24 32 ZPSE S 0x000000 Phase C total reactive power gain adjust (ADE7858, ADE7868, and ADE7878).
0x439C CVAROS in_powerreactive0_phaseC_offset R/W 24 32 ZPSE S 0x000000 Phase C total reactive power offset adjust (ADE7858, ADE7868, and ADE7878).
0x439D AFWGAIN in_power0_phaseA_fundamental_scale R/W 24 32 ZPSE S 0x000000 Phase A fundamental active power gain adjust. Location reserved for ADE7854, ADE7858, and ADE7868.

0x439E AFWATTOS in_power0_phaseA_fundamental_offset R/W 24 32 ZPSE S 0x000000 Phase A fundamental active power offset adjust. Location reserved for ADE7854, ADE7858, and ADE7868.
0x439F BFWGAIN in_power0_phaseB_fundamental_scale R/W 24 32 ZPSE S 0x000000 Phase B fundamental active power gain adjust (ADE7878 only).
0x43A0 BFWATTOS in_power0_phaseB_fundamental_offset R/W 24 32 ZPSE S 0x000000 Phase B fundamental active power offset adjust (ADE7878 only).
0x43A1 CFWGAIN in_power0_phaseC_fundamental_scale R/W 24 32 ZPSE S 0x000000 Phase C fundamental active power gain adjust.

0x43A2 CFWATTOS in_power0_phaseC_fundamental_offset R/W 24 32 ZPSE S 0x000000 Phase C fundamental active power offset adjust (ADE7878 only).
0x43A3 AFVARGAIN in_powerreactive0_phaseA_fundamental_scale R/W 24 32 ZPSE S 0x000000 Phase A fundamental reactive power gain adjust (ADE7878 only).
0x43A4 AFVAROS in_powerreactive0_phaseA_fundamental_offset R/W 24 32 ZPSE S 0x000000 Phase A fundamental reactive power offset adjust (ADE7878 only).
0x43A5 BFVARGAIN in_powerreactive0_phaseB_fundamental_scale R/W 24 32 ZPSE S 0x000000 Phase B fundamental reactive power gain adjust (ADE7878 only).
0x43A6 BFVAROS in_powerreactive0_phaseB_fundamental_offset R/W 24 32 ZPSE S 0x000000 Phase B fundamental reactive power offset adjust (ADE7878 only).
0x43A7 CFVARGAIN in_powerreactive0_phaseC_fundamental_scale R/W 24 32 ZPSE S 0x000000 Phase C fundamental reactive power gain adjust (ADE7878 only).
0x43A8 CFVAROS in_powerreactive0_phaseC_fundamental_offset R/W 24 32 ZPSE S 0x000000 Phase C fundamental reactive power offset adjust (ADE7878 only).
0x43A9 VATHR1 regiister_VATHR1 R/W 24 32 ZP U 0x000000 Most significant 24 bits of VATHR[47:0] threshold used in phase apparent power datapath.

0x43AA VATHR0 register_VATHR0 R/W 24 32 ZP U 0x000000 Less significant 24 bits of VATHR[47:0] threshold used in phase apparent power datapath.
0x43AB WTHR1 register_WTHR1 R/W 24 32 ZP U 0x000000 Most significant 24 bits of WTHR[47:0] threshold used in phase total/fundamental active power datapath.
0x43AC WTHR0 register_WTHR0 R/W 24 32 ZP U 0x000000 Less significant 24 bits of WTHR[47:0] threshold used in phase total/fundamental active power datapath.
0x43AD VARTHR1 register_VARTHR1 R/W 24 32 ZP U 0x000000 Most significant 24 bits of VARTHR[47:0] threshold used in phase total/fundamental reactive power datapath (ADE7858, ADE7868, and ADE7878).
0x43AE VARTHR0 register_VARTHR0 R/W 24 32 ZP U 0x000000 Less significant 24 bits of VARTHR[47:0] threshold used in phase total/fundamental reactive power datapath (ADE7858, ADE7868, and ADE7878).
0x43AF Reserved N/A4 N/A4 N/A4 N/A4 0x000000 This memory location should be kept at 0x000000 for proper operation.
0x43B0 VANOLOAD register_VANOLOAD R/W 24 32 ZPSE S 0x0000000 No load threshold in the apparent power datapath.

0x43B1 APNOLOAD register_APNOLOAD R/W 24 32 ZPSE S 0x0000000 No load threshold in the total/fundamental active power datapath.

0x43B2 VARNOLOAD register_VARNOLOAD R/W 24 32 ZPSE S 0x0000000 No load threshold in the total/fundamental reactive power datapath. Location reserved for ADE7854.

0x43B3 VLEVEL register_VLEVEL R/W 24 32 ZPSE S 0x000000 Register used in the algorithm that computes the fundamental active and reactive powers (ADE7878 only).

0x43B5 DICOEFF register_DICOEFF R/W 24 32 ZPSE S 0x0000000 Register used in the digital integrator algorithm. If the integrator is turned on, it must be set at 0xFF8000. In practice, it is transmitted as 0xFFF8000.

0x43B6 HPFDIS register_HPFDIS R/W 24 32 ZP U 0x000000 Disables/enables the HPF in the current datapath (see Table 34).

0x43B8 ISUMLVL register_ISUMLVL R/W 24 32 ZPSE S 0x000000 Threshold used in comparison between the sum of phase currents and the neutral current (ADE7868 and ADE7878 only).

0x43BF ISUM register_ISUM R 28 32 ZP S N/A4 Sum of IAWV, IBWV, and ICWV registers (ADE7868 and ADE7878 only).

0x43C0 AIRMS in_current0_phaseA_rms R 24 32 ZP S N/A4 Phase A current rms value.
0x43C1 AVRMS in_voltage0_phaseA_rms R 24 32 ZP S N/A4 Phase A voltage rms value.
0x43C2 BIRMS in_current0_phaseB_rms R 24 32 ZP S N/A4 Phase B current rms value.
0x43C3 BVRMS in_voltage0_phaseB_rms R 24 32 ZP S N/A4 Phase B voltage rms value.
0x43C4 CIRMS in_current0_phaseC_rms R 24 32 ZP S N/A4 Phase C current rms value.
0x43C5 CVRMS in_voltage0_phaseC_rms R 24 32 ZP S N/A4 Phase C voltage rms value.
0x43C6 NIRMS in_current0_neutral_rms R 24 32 ZP S N/A4 Neutral current rms value (ADE7868 and ADE7878 only).
0xE228 Run register_Run R/W 16 16 U 0x0000 Run register starts and stops the DSP. See the Digital Signal Processor section for more details.

0xE400 AWATTHR in_energy0_phaseA_raw R 32 32 S 0x00000000 Phase A total active energy accumulation.
0xE401 BWATTHR in_energy0_phaseB_raw R 32 32 S 0x00000000 Phase B total active energy accumulation.
0xE402 CWATTHR in_energy0_phaseC_raw R 32 32 S 0x00000000 Phase C total active energy accumulation.
0xE403 AFWATTHR in_energy0_phaseA_fundamental_raw R 32 32 S 0x00000000 Phase A fundamental active energy accumulation (ADE7878 only).
0xE404 BFWATTHR in_energy0_phaseB_fundamental_raw R 32 32 S 0x00000000 Phase B fundamental active energy accumulation (ADE7878 only).
0xE405 CFWATTHR in_energy0_phaseC_fundamental_raw R 32 32 S 0x00000000 Phase C fundamental active energy accumulation (ADE7878 only).
0xE406 AVARHR in_energyreactive0_phaseA_raw R 32 32 S 0x00000000 Phase A total reactive energy accumulation (ADE7858, ADE7868, and ADE7878 only).
0xE407 BVARHR in_energyreactive0_phaseB_raw R 32 32 S 0x00000000 Phase B total reactive energy accumulation (ADE7858, ADE7868, and ADE7878 only).

0xE408 CVARHR in_energyreactive0_phaseC_raw R 32 32 S 0x00000000 Phase C total reactive energy accumulation (ADE7858, ADE7868, and ADE7878 only).
0xE409 AFVARHR in_energyreactive0_phaseA_fundamental_raw R 32 32 S 0x00000000 Phase A fundamental reactive energy accumulation (ADE7878 only).
0xE40A BFVARHR in_energyreactive0_phaseB_fundamental_raw R 32 32 S 0x00000000 Phase B fundamental reactive energy accumulation (ADE7878 only).
0xE40B CFVARHR in_energyreactive0_phaseC_fundamental_raw R 32 32 S 0x00000000 Phase C fundamental reactive energy accumulation (ADE7878 only).
0xE40C AVAHR in_energyapparent0_phaseA_raw R 32 32 S 0x00000000 Phase A apparent energy accumulation.
0xE40D BVAHR in_energyapparent0_phaseB_raw R 32 32 S 0x00000000 Phase B apparent energy accumulation.
0xE40E CVAHR in_energyapparent0_phaseC_raw R 32 32 S 0x00000000 Phase C apparent energy accumulation.
0xE500 IPEAK in_current0_peak R 32 32 U N/A Current peak register. See Figure 50 and Table 35 for details about its composition.

0xE501 VPEAK in_voltage_peak R 32 32 U N/A Voltage peak register. See Figure 50 and Table 36 for details about its composition.

0xE502 STATUS0 register_STATUS0 R/W 32 32 U N/A Interrupt Status Register 0. See Table 37.
0xE503 STATUS1 register_STATUS1 R/W 32 32 U N/A Interrupt Status Register 1. See Table 38.

0xE504 AIMAV in_currentA_mav R 20 32 ZP U N/A Phase A current mean absolute value computed during PSM0 and PSM1 modes (ADE7868 and ADE7878 only).

0xE505 BIMAV in_currentB_mav R 20 32 ZP U N/A Phase B current mean absolute value computed during PSM0 and PSM1 modes (ADE7868 and ADE7878 only).
0xE506 CIMAV in_currentC_mav R 20 32 ZP U N/A Phase C current mean absolute value computed during PSM0 and PSM1 modes (ADE7868 and ADE7878 only).
0xE507 OILVL register_OILVL R/W 24 32 ZP U 0xFFFFFF Overcurrent threshold.
0xE508 OVLVL register_OVLVL R/W 24 32 ZP U 0xFFFFFF Overvoltage threshold.

0xE509 SAGLVL register_SAGLVL R/W 24 32 ZP U 0x000000 Voltage SAG level threshold.

0xE50A MASK0 register_MASK0 R/W 32 32 U 0x00000000 Interrupt Enable Register 0. See Table 39.
0xE50B MASK1 register_MASK1 R/W 32 32 U 0x00000000 Interrupt Enable Register 1. See Table 40.

0xE50C IAWV in_currentA_instantaneous R 24 32 SE S N/A Instantaneous value of Phase A current.
0xE50D IBWV in_currentB_instantaneous R 24 32 SE S N/A Instantaneous value of Phase B current.
0xE50E ICWV in_currentC_instantaneous R 24 32 SE S N/A Instantaneous value of Phase C current.
0xE50F INWV in_currentN_instantaneous R 24 32 SE S N/A Instantaneous value of neutral current (ADE7868 and ADE7878 only).
0xE510 VAWV in_voltageA_instantaneous R 24 32 SE S N/A Instantaneous value of Phase A voltage.
0xE511 VBWV in_voltageB_instantaneous R 24 32 SE S N/A Instantaneous value of Phase B voltage.
0xE512 VCWV in_voltageC_instantaneous R 24 32 SE S N/A Instantaneous value of Phase C voltage.

0xE513 AWATT in_powerA_instantaneous R 24 32 SE S N/A Instantaneous value of Phase A total active power.
0xE514 BWATT in_powerB_instantaneous R 24 32 SE S N/A Instantaneous value of Phase B total active power.
0xE515 CWATT in_powerC_instantaneous R 24 32 SE S N/A Instantaneous value of Phase C total active power.
0xE516 AVAR in_powerreactiveA_instantaneous R 24 32 SE S N/A Instantaneous value of Phase A total reactive power (ADE7858, ADE7868, and ADE7878 only).
0xE517 BVAR in_powerreactiveB_instantaneous R 24 32 SE S N/A Instantaneous value of Phase B total reactive power (ADE7858, ADE7868, and ADE7878 only).
0xE518 CVAR in_powerreactiveC_instantaneous R 24 32 SE S N/A Instantaneous value of Phase C total reactive power (ADE7858, ADE7868, and ADE7878 only).
0xE519 AVA in_powerapparentA_instantaneous R 24 32 SE S N/A Instantaneous value of Phase A apparent power.
0xE51A BVA in_powerapparentB_instantaneous R 24 32 SE S N/A Instantaneous value of Phase B apparent power.
0xE51B CVA in_powerappatentC_instantaneous R 24 32 SE S N/A Instantaneous value of Phase C apparent power.

0xE51F CHECKSUM register_CHECKSUM R 32 32 U 0x33666787 Checksum verification. See the Checksum Register section for details.

0xE520 VNOM in_voltage_rms_nominal R/W 24 32 ZP S 0x000000 Nominal phase voltage rms used in the alternative computation of the apparent power. When the VNOMxEN bit is set, the applied voltage input in the corresponding phase is ignored and all corresponding rms voltage instances are replaced by the value in the VNOM register.

0xE600 PHSTATUS in_current_phase_peak R 16 16 U N/A Phase peak register. See Table 41.
0xE601 ANGLE0 register_ANGLE0 R 16 16 U N/A Time Delay 0. See the Time Interval Between Phases section for details.
0xE602 ANGLE1 register_ANGLE1 R 16 16 U N/A Time Delay 1. See the Time Interval Between Phases section for details.
0xE603 ANGLE2 register_ANGLE2 R 16 16 U N/A Time Delay 2. See the Time Interval Between Phases section for details.

0xE604 to 0xE606 Reserved These addresses should not be written for proper operation.
0xE607 PERIOD register_PERIOD R 16 16 U N/A Network line period.

0xE608 PHNOLOAD register_PHNOLOAD R 16 16 U N/A Phase no load register. See Table 42.

0xE60C LINECYC register_LINECYC R/W 16 16 U 0xFFFF Line cycle accumulation mode count.

0xE60D ZXTOUT register_ZXTOUT R/W 16 16 U 0xFFFF Zero-crossing timeout count.

0xE60E COMPMODE register_COMPMODE R/W 16 16 U 0x01FF Computation-mode register. See Table 43.

0xE60F Gain register_Gain R/W 16 16 U 0x0000 PGA gains at ADC inputs. See Table 44.

0xE610 CFMODE register_CFMODE R/W 16 16 U 0x0E88 CFx configuration register. See Table 45.
0xE611 CF1DEN register_CF1DEN R/W 16 16 U 0x0000 CF1 denominator.
0xE612 CF2DEN register_CF2DEN R/W 16 16 U 0x0000 CF2 denominator.
0xE613 CF3DEN register_CF3DEN R/W 16 16 U 0x0000 CF3 denominator.

0xE614 APHCAL register_APHCAL R/W 10 16 ZP S 0x0000 Phase calibration of Phase A. See Table 46.
0xE615 BPHCAL register_BPHCAL R/W 10 16 ZP S 0x0000 Phase calibration of Phase B. See Table 46.
0xE616 CPHCAL register__CPHCAL R/W 10 16 ZP S 0x0000 Phase calibration of Phase C. See Table 46.

0xE617 PHSIGN register_PHSIGN R 16 16 U N/A Power sign register. See Table 47.
0xE618 CONFIG register_CONFIG R/W 16 16 U 0x0000 ADE7878 configuration register. See Table 48.
0xE700 MMODE register__MMODE R/W 8 8 U 0x1C Measurement mode register. See Table 49.
0xE701 ACCMODE register__ACCMODE R/W 8 8 U 0x00 Accumulation mode register. See Table 50.
0xE702 LCYCMODE register_LCYCMODE R/W 8 8 U 0x78 Line accumulation mode behavior. See Table 52.
0xE703 PEAKCYC register_PEAKCYC R/W 8 8 U 0x00 Peak detection half line cycles.
0xE704 SAGCYC register_SAGCYC R/W 8 8 U 0x00 SAG detection half line cycles.

0xE705 CFCYC register_CFCYC R/W 8 8 U 0x01 Number of CF pulses between two consecutive energy latches. See the Synchronizing Energy Registers with CFx Outputs section.
0xE706 HSDC_CFG register_HSDC_CFG R/W 8 8 U 0x00 HSDC configuration register. See Table 53.
0xE707 Version register_Version R 8 8 U Version of die.
0xEBFF Reserved 8 8 This address can be used in manipulating the SS/HSA pin when SPI is chosen as the active port. See the Serial Interfaces section for details.
0xEC00 LPOILVL register_LPOILVL R/W 8 8 U 0x07 "Overcurrent threshold used during PSM2 mode (ADE7868 and ADE7878
only). See Table 54 in which the register is detailed."
0xEC01 CONFIG2 register_CONFIG2 R/W 8 8 U 0x00 Configuration register used during PSM1 mode. See Table 55.

Regards,
John

Quoted 270 lines · Jonathan Cameron

Quoted 6 lines · On Wed, 14 Mar 2018 23:12:02 -0700 John Syne <[email protected]> wrote:

Hi Jonathan,

I have been looking at the IIO ABI docs and if I understand correctly, the idea is to use consistent naming conventions? So for example, looking at the ADE7854 datasheet, the naming matching the ADE7854 registers would be as follows:

Welcome to one of the big reasons no one tidied these drivers up originally. Still we have moved on somewhat since then so similar circumstances have come up in other types of sensor.

Quoted 4 lines

{direction}_{type}_{index}_{modifier}_{info_mask}

AIGAIN - In_current_a_gain

Other than the fact that gain isn't an ABI element and that index doesn't have
_ before it that is right.
in_voltageA_scale

That was a weird quirk a long time back which we should probably not have done (copied it from hwmon)

Quoted 7 lines

AVGAIN - in_voltage_a_gain
BIGAIN - in_current_b_gain
BVGAIN - in_voltage_b_gain
—
How do we represent the rms and offset
AIRMSOS - in_current_a_rmsoffset
AVRMSOS - in_voltage_a_rmsoffset

Right now we can't unfortunately though this one is easier to fix. We already have modifiers for multi axis devices doing sum_squared so add one of those for root_mean_square - this one is well known enough that rms is fine in the string.

It's a effectively a different channel be it one derived from a simple one. This is going to get tricky however as we would normally use modifier to specialise a channel type - thoughts on this below.

Quoted 5 lines

—
Here I don’t understand how to represent both the phase and the active/reactive/apparent power components. Do we combine the phase and quadrature part like this
AVAGAIN - in_power_a_gain /* apparent power */
—
AWGAIN - in_power_ai_gain /* active power */

And that is the problem. How do we represent the various power types. Hmm. We could do it with modifiers but above we show that we have already used them.

It would be easy enough to add yet another field to the channel spec to specify this but there is a problem - Events. The event format is already pretty full so where do we put this extra element if we need to define a channel separated only by it.

One thought is we could instead define these as different top level IIO_CHAN_TYPES in a similar fashion to we do for relative humidity vs the proposed absolute humidity.

in_powerreactiveA_scale
in_poweractivceA_scale
(or in_powerrealA_scale to match with what I got taught years ago?)

I presume we keep in_powerA_scale etc for the apparent power and modify any docs to make that clear.

Quoted 5 lines

—
AVARGAIN - in_power_aq_gain /* reactive power */
—
Now here it becomes more complicated. Not sure how this gets handled.
AFWATTOS - in_power_a_active/fundamental/offset

Yeah, some of these are getting odd.
If I read this correctly this is the active power estimate based on only the fundamental frequency - so no harmonics?

Hmm. This then becomes a separate channel with additional properties specifying it is only the fundamental. This feels a bit like a filter be it an unusual one? Might just be necessary to add a _fundamental_only element on the end (would be info_mask if this is common enough to justify that rather than using the extended methods to define it.).

Quoted 2 lines

—
AWATTHR - in_energy_ai_accumulation

Great, just when I thought we had gone far enough they define reactive energy which is presumably roughly the same as reactivepower * time? In that case we need types for that as well. in_energyreactiveA_*
in_energyactiveA_*

Quoted 4 lines

—
AVARHR - in_energy_aq_accumulation
—
IPEAK - in_current_peak

That one is easy as we have an info_mask element for peak and one for peak_scale that has always been a bit odd but was needed somewhere.

Quoted 3 lines

—

I’ll leave it there, because there are some even more complicated register naming issues.

Something to look forward to. Gah, I always hated power engineering though I taught it very briefly (I really pity those students :(

Jonathan

Quoted 161 lines

Regards,
John

Quoted 152 lines · Jonathan Cameron

Quoted 26 lines · On Thu, 8 Mar 2018 21:37:33 -0300 Rodrigo Siqueira…

Quoted 18 lines · Jonathan Cameron

Quoted 15 lines · On Tue, 6 Mar 2018 21:43:47 -0300 Rodrigo Siqueira…

The macro IIO_DEV_ATTR_CH_OFF is a wrapper for IIO_DEVICE_ATTR, with a tiny change in the name definition. This extra macro does not improve the readability and also creates some checkpatch errors.

This patch fixes the checkpatch.pl errors:

staging/iio/meter/ade7753.c:391: ERROR: Use 4 digit octal (0777) not decimal permissions
staging/iio/meter/ade7753.c:395: ERROR: Use 4 digit octal (0777) not decimal permissions
staging/iio/meter/ade7759.c:331: ERROR: Use 4 digit octal (0777) not decimal permissions
staging/iio/meter/ade7759.c:335: ERROR: Use 4 digit octal (0777) not decimal permissions

Quoted 6 lines

Quoted 1 line

Signed-off-by Rodrigo Siqueira <[email protected]>

Hmm. I wondered a bit about this one. It's a correct patch in of itself but the interface in question doesn't even vaguely conform to any of defined IIO ABI. Anyhow, it's still and improvement so I'll take it.

Quoted 4 lines

I am not sure if I understood the comment about the ABI. The meter interface is wrong because it uses things like IIO_DEVICE_ATTR? It should use iio_info together with *write_raw and *read_raw. Right? Is it the ABI problem that you refer?

The ABI is about the userspace interface of IIO. It is defined in Documentation/ABI/testing/sysfs-bus-iio*
So this documents the naming of sysfs attributes and (more or less) describes a consistent interface to userspace across lots of different types of devices.

A lot of these older drivers in staging involve a good deal of ABI that was not reviewed or discussed. That is one of the biggest reasons we didn't take them out of staging in the first place.

In order for generic userspace programs to have any idea what to do with these devices this all needs to be fixed.

There may well be cases where we need to expand the existing ABI to
cover new things. That's fine, but it has to be done with full
review of the relevant documentation patches.

Incidentally if you want an easy driver to work on moving out of staging then first thing to do is to compare what it shows to userspace with these docs. If it's totally different then you have a big job on your hands as often ABI can take a lot of discussion and a long time to establish a consensus.

Jonathan

Quoted 79 lines

Thanks :)

Quoted 76 lines

Applied to the togreg branch of iio.git and pushed out as testing for the autobuilders to play with it.

I also added the removal of the header define which is no longer used.

Please note, following discussions with Michael, I am going to send an email announcing an intent to drop these meter drivers next cycle unless someone can provide testing for any attempt to move them out of staging. I'm still taking patches on the basis that 'might' happen - but I wouldn't focus on these until we have some certainty on whether they will be around long term!

Jonathan

Quoted 61 lines

---
drivers/staging/iio/meter/ade7753.c | 18 ++++++++++-------- drivers/staging/iio/meter/ade7759.c | 18 ++++++++++--------

2 files changed, 20 insertions(+), 16 deletions(-)

drivers/staging/iio/meter/ade7753.c +10 -8

index c44eb577dc35..275e8dfff836 100644
@@ -388,14 +388,16 @@ static IIO_DEV_ATTR_VPERIOD(0444,
 		ade7753_read_16bit,
 		NULL,
 		ADE7753_PERIOD);
-static IIO_DEV_ATTR_CH_OFF(1, 0644,
-		ade7753_read_8bit,
-		ade7753_write_8bit,
-		ADE7753_CH1OS);
-static IIO_DEV_ATTR_CH_OFF(2, 0644,
-		ade7753_read_8bit,
-		ade7753_write_8bit,
-		ADE7753_CH2OS);
+
+static IIO_DEVICE_ATTR(choff_1, 0644,
+			ade7753_read_8bit,
+			ade7753_write_8bit,
+			ADE7753_CH1OS);
+
+static IIO_DEVICE_ATTR(choff_2, 0644,
+			ade7753_read_8bit,
+			ade7753_write_8bit,
+			ADE7753_CH2OS);
 
 static int ade7753_set_irq(struct device *dev, bool enable)
 {

drivers/staging/iio/meter/ade7759.c +10 -8

index 1decb2b8afab..c078b770fa53 100644
@@ -328,14 +328,16 @@ static IIO_DEV_ATTR_ACTIVE_POWER_GAIN(0644,
 		ade7759_read_16bit,
 		ade7759_write_16bit,
 		ADE7759_APGAIN);
-static IIO_DEV_ATTR_CH_OFF(1, 0644,
-		ade7759_read_8bit,
-		ade7759_write_8bit,
-		ADE7759_CH1OS);
-static IIO_DEV_ATTR_CH_OFF(2, 0644,
-		ade7759_read_8bit,
-		ade7759_write_8bit,
-		ADE7759_CH2OS);
+
+static IIO_DEVICE_ATTR(choff_1, 0644,
+			ade7759_read_8bit,
+			ade7759_write_8bit,
+			ADE7759_CH1OS);
+
+static IIO_DEVICE_ATTR(choff_2, 0644,
+			ade7759_read_8bit,
+			ade7759_write_8bit,
+			ADE7759_CH2OS);
 

Quoted 7 lines

Quoted 3 lines

drivers/staging/iio/meter/ade7759.c static IIO_DEV_ATTR_ACTIVE_POWER_GAIN(0644, · line 343

Quoted 2 lines

drivers/staging/iio/meter/ade7759.c +0 -0

 static int ade7759_set_irq(struct device *dev, bool enable)
 {      

--
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

_______________________________________________
devel mailing list
[email protected]
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

On Mar 18, 2018, at 5:23 AM, Jonathan Cameron <[email protected]> =

=20
On Sat, 17 Mar 2018 23:11:45 -0700
John Syne <[email protected]> wrote:
=20

Quoted 1 line

Hi Jonathan,

Hi John and All,
=20
I'd love to get some additional input on this from anyone interested. There are a lot of weird and wonderful derived quantities in an energy meter and it seems we need to make some fundamental changes to support them - including potentially a userspace breaking change to the event code description.
=20

Quoted 2 lines

=20
Here is the complete list of registers for the ADE7878 which I copied =

Quoted 5 lines

in_accel_x_peak_raw
=20
so shouldn=E2=80=99t the phase be represented as follows:
=20
in_current_A_scale

I'm still confused. What does A represent here? I assumed that was a =

card for the channel number before but clearly not.
=20
Ah, you are labelling the 3 separate phases as A, B and C. Hmm. I guess they sort of look like axis, and sort of like independent =

So could be indexed or done via modifiers depending on how you look at =

=20
Hmm. With neutral in there as well I guess we need to make them modifiers (but might change my mind later ;)
=20
Particularly as we are using the the modifier for RMS under the =

plan... It appears we should treat that instead like we did for peak and do it as an additional info mask element. The problem with doing =

on a continuous measurement is that we can't treat it as a channel to be output through the buffered interface.

=20
So again we have run out of space. It's increasingly looking like we =

room for another field in the events - to cleanly represent computed =

=20
Hmm. What is the current usage? - it's been a while so I had to go look in the header.
=20
0-15 Channel (lots of channels)
31-16 Channel 2 (36 modifiers - lots of channels)
47-32 Channel Type (31 used so far - this looks most likely to run out =

space in the long run so leave this one alone).
54-48 Event Direction (4 used)
55 Differential (1: channel 2 as differential pair 0: as a modifier) 63-56 Event Type (6 used)
=20
So I think we can pinch bit 53 as another flag to indicate we have

a computed value or possibly bit 63 as event types are few and far between as well.
=20
Probably reasonable to assume we never have 16 bits worth of channels and computed channels at the same time? Hence I think we can steal bits off the top of Channel. How many do we need? Not sure unfortunately but feels like 8 should be plenty.

=20
The other element of this is we add a new field to iio_chan_spec to contain 'derived_type' or something like that which has rms and sum squared etc. Over time we can move some of those from the modifiers and free up a few entires there.
So modifier might be "X and Y and Z" with a derived_type of=20 sum_squared to give existing sum_squared_x_y_z but no rush on that.
=20
Anyhow so now we have an extra element to play that will result in a different channel.
=20
Whilst here we should think about any other mods needed to

that event structure. It is a little unfortunate that this will be a breaking change for any old userspace code playing with new drivers but it can't be helped as we didn't have reserved values in the original definition (oops). =20
At somepoint we may need to add the 'shared by derived_value' info mask but I think we can ignore that for now (seems moderately unlikely to have anything in it!)

Quoted 4 lines

=20
But for now, I followed your instructions from your reply. =20
After finalizing this one, I will work on the ADE9000, which as way =

Quoted 2 lines

=20
Once we can agree on the register naming, I will update the ADE7854 =

I'll edit to fit with new scheme and insert indexes which I think =

preferred though optional under the ABI as we only have one of each =

Quoted 2 lines

=20
Address Register IIO Attribute R/W Bit Length =

Quoted 1 line

0x4380 AIGAIN in_current0_phaseA_scale R/W 24 =

A, B, C, N aren't obvious to the lay reader so I suggest we burn a few =

the neutral one. Not sure about capitalization or not though. =20

Quoted 1 line

0x4381 AVGAIN in_voltage0_phaseA_scale R/W 24 =

Quoted 1 line

0x4382 BIGAIN in_current0_phaseB_scale R/W 24 =

Quoted 1 line

0x4383 BVGAIN in_voltage0_phaseB_scale R/W 24 =

Quoted 1 line

0x4384 CIGAIN in_current0_phaseC_scale R/W 24 =

Quoted 1 line

0x4385 CVGAIN in_voltage0_phaseC_scale R/W 24 =

Quoted 1 line

0x4386 NIGAIN in_current0_neutral_scale R/W 24 =

Quoted 1 line

0x4387 AIRMSOS in_current0_phaseA_rms_offset R/W 24 =

Quoted 1 line

0x4388 AVRMSOS in_voltage0_phaseA_rms_offset R/W 24 =

Quoted 1 line

0x4389 BIRMSOS in_current0_phaseB_rms_offset R/W 24 =

Quoted 1 line

0x438A BVRMSOS in_voltage0_phaseB_rms_offset R/W 24 =

Quoted 1 line

0x438B CIRMSOS in_current0_phaseC_rms_offset R/W 24 =

Quoted 1 line

0x438C CVRMSOS in_voltage0_phaseC_rms_offset R/W 24 =

Quoted 1 line

0x438D NIRMSOS in_current0_neutral_rms_offset R/W 24 =

Quoted 1 line

0x438E AVAGAIN in_powerapparent0_phaseA_scale R/W 24 =

Quoted 1 line

0x438F BVAGAIN in_powerapparent0_phaseB_scale R/W 24 =

Quoted 1 line

0x4390 CVAGAIN in_powerapparent0_phaseC_scale R/W 24 =

Quoted 1 line

0x4391 AWGAIN in_power0_phaseA_scale R/W 24 32 ZPSE =

Quoted 1 line

0x4392 AWATTOS in_power0_phaseA_offset R/W 24 32 ZPSE =

Quoted 1 line

0x4393 BWGAIN in_power0_phaseB_scale R/W 24 32 ZPSE =

Quoted 1 line

0x4394 BWATTOS in_power0_phaseB_offset R/W 24 32 ZPSE =

Quoted 1 line

0x4395 CWGAIN in_power0_PhaseC_scale R/W 24 32 ZPSE =

Quoted 1 line

0x4396 CWATTOS in_power0_phaseC_offset R/W 24 32 ZPSE =

Quoted 1 line

0x4397 AVARGAIN in_powerreactive0_phaseA_scale R/W =

Quoted 1 line

0x4398 AVAROS in_powerreactive0_phaseA_offset R/W 24 =

Quoted 1 line

0x4399 BVARGAIN in_powerreactive0_phaseB_scale R/W =

Quoted 1 line

0x439A BVAROS in_powerreactive0_phaseB_offset R/W 24 =

Quoted 1 line

0x439B CVARGAIN in_powerreactive0_phaseC_scale R/W =

Quoted 1 line

0x439C CVAROS in_powerreactive0_phaseC_offset R/W 24 =

Quoted 1 line

0x439D AFWGAIN in_power0_phaseA_fundamental_scale R/W =

Hmm. fundamental is the oddity here. I here because it is sort of a =

and sort of a filter applied. Can it be sensible combined with RMS? =

it can be combined with peak for example (which I'd also ideally move =

the derived representation.).

=20

Quoted 1 line

0x439E AFWATTOS in_power0_phaseA_fundamental_offset =

Quoted 1 line

0x439F BFWGAIN in_power0_phaseB_fundamental_scale R/W =

Quoted 1 line

0x43A0 BFWATTOS in_power0_phaseB_fundamental_offset =

Quoted 1 line

0x43A1 CFWGAIN in_power0_phaseC_fundamental_scale R/W =

Quoted 1 line

0x43A2 CFWATTOS in_power0_phaseC_fundamental_offset =

Quoted 1 line

0x43A3 AFVARGAIN =

Quoted 1 line

0x43A4 AFVAROS in_powerreactive0_phaseA_fundamental_offset =

Quoted 1 line

0x43A5 BFVARGAIN =

Quoted 1 line

0x43A6 BFVAROS in_powerreactive0_phaseB_fundamental_offset =

Quoted 1 line

0x43A7 CFVARGAIN =

Quoted 1 line

0x43A8 CFVAROS in_powerreactive0_phaseC_fundamental_offset =

Quoted 1 line

0x43A9 VATHR1 regiister_VATHR1 R/W 24 32 ZP =

Do not expose these to userspace. Why would it care?

=20

Quoted 1 line

0x43AA VATHR0 register_VATHR0 R/W 24 32 ZP U =

Quoted 1 line

0x43AB WTHR1 register_WTHR1 R/W 24 32 ZP U =

Quoted 1 line

0x43AC WTHR0 register_WTHR0 R/W 24 32 ZP U =

Quoted 1 line

0x43AD VARTHR1 register_VARTHR1 R/W 24 32 ZP =

Quoted 1 line

0x43AE VARTHR0 register_VARTHR0 R/W 24 32 ZP =

Quoted 1 line

0x43AF Reserved N/A4 N/A4 N/A4 N/A4 =

Quoted 1 line

0x43B0 VANOLOAD register_VANOLOAD R/W 24 =

This one is kind of an event parameter, but one that controls internal =

This will be a driver specific attr I think for now. We may add it to =

later if we get lots of meter drivers.=20
Something like
in_power0_no_load_thresh though I haven't really thought about it or =

for similar precedence.

=20
=20

Quoted 1 line

0x43B1 APNOLOAD register_APNOLOAD R/W 24 =

in_activepower0_no_load_thresh

Quoted 1 line

0x43B2 VARNOLOAD register_VARNOLOAD R/W 24 =

in_reactivpower0_no_load_thresh

=20

Quoted 1 line

0x43B3 VLEVEL register_VLEVEL R/W 24 32 ZPSE S =

This one looks like a characteristic of the circuit attached. So =

or similar and not exposed to userspace.

=20

Quoted 1 line

0x43B5 DICOEFF register_DICOEFF R/W 24 32 ZPSE =

no userspace interface.

=20

Quoted 1 line

0x43B6 HPFDIS register_HPFDIS R/W 24 32 ZP U =

We have controls for high pass filters, you'll need to map on to them. Disable is usually setting 3DB point to 0 IIRC.

=20

Quoted 1 line

0x43B8 ISUMLVL register_ISUMLVL R/W 24 32 ZPSE =

This is an event threshold so needs to map to the events =

as best we can. It's actually a pain to describe so may be device =

=20

Quoted 1 line

0x43BF ISUM register_ISUM R 28 32 ZP S =

So this would be using a modifier for AandBandC phases (similar to the =

a derived value of sum I think So would look something like. in_current0_phaseA&phaseB&phaseC_sum and yet another channel =20

Quoted 1 line

0x43C0 AIRMS in_current0_phaseA_rms R 24 32 ZP =

Quoted 1 line

0x43C1 AVRMS in_voltage0_phaseA_rms R 24 32 ZP =

Quoted 1 line

0x43C2 BIRMS in_current0_phaseB_rms R 24 32 ZP =

Quoted 1 line

0x43C3 BVRMS in_voltage0_phaseB_rms R 24 32 ZP =

Quoted 1 line

0x43C4 CIRMS in_current0_phaseC_rms R 24 32 ZP =

Quoted 1 line

0x43C5 CVRMS in_voltage0_phaseC_rms R 24 32 ZP =

Quoted 1 line

0x43C6 NIRMS in_current0_neutral_rms R 24 32 ZP =

Quoted 1 line

0xE228 Run register_Run R/W 16 16 U =

Not exposed to userspace.

=20

Quoted 1 line

0xE400 AWATTHR in_energy0_phaseA_raw R 32 32 =

Quoted 1 line

0xE401 BWATTHR in_energy0_phaseB_raw R 32 32 =

Quoted 1 line

0xE402 CWATTHR in_energy0_phaseC_raw R 32 32 =

Quoted 1 line

0xE403 AFWATTHR in_energy0_phaseA_fundamental_raw =

Quoted 1 line

0xE404 BFWATTHR in_energy0_phaseB_fundamental_raw =

Quoted 1 line

0xE405 CFWATTHR in_energy0_phaseC_fundamental_raw =

Quoted 1 line

0xE406 AVARHR in_energyreactive0_phaseA_raw R 32 =

Quoted 1 line

0xE407 BVARHR in_energyreactive0_phaseB_raw R 32 =

Quoted 1 line

0xE408 CVARHR in_energyreactive0_phaseC_raw R 32 =

Quoted 1 line

0xE409 AFVARHR in_energyreactive0_phaseA_fundamental_raw =

Quoted 1 line

0xE40A BFVARHR in_energyreactive0_phaseB_fundamental_raw =

Quoted 1 line

0xE40B CFVARHR in_energyreactive0_phaseC_fundamental_raw =

Quoted 1 line

0xE40C AVAHR in_energyapparent0_phaseA_raw R 32 =

Quoted 1 line

0xE40D BVAHR in_energyapparent0_phaseB_raw R 32 =

Quoted 1 line

0xE40E CVAHR in_energyapparent0_phaseC_raw R 32 =

Quoted 1 line

0xE500 IPEAK in_current0_peak R 32 32 =

Oh goody. I have no idea how we expose the which phase element of this cleanly. One option I suppose is to have in_current0_phaseA_peak etc and have all but the current peak return an error when read? It is a =

nasty but only so much we can do and keep with a consistent interface. =20

Quoted 1 line

0xE501 VPEAK in_voltage_peak R 32 32 U =

Same as peak.
=20

Quoted 1 line

0xE502 STATUS0 register_STATUS0 R/W 32 32 =

Quoted 1 line

0xE503 STATUS1 register_STATUS1 R/W 32 32 =

No userspace interface except via events interface or error reports.

=20

Quoted 1 line

0xE504 AIMAV in_currentA_mav R 20 32 ZP U =

Probably a longer name as mav is cryptic. in_current0_phaseA_meanabs_raw - it could have a scale and all sorts =

So I think this needs to be using the new derived infrastructure =

rather than being an info_mask element.

=20

Quoted 1 line

0xE505 BIMAV in_currentB_mav R 20 32 ZP U =

Quoted 1 line

0xE506 CIMAV in_currentC_mav R 20 32 ZP U =

Quoted 1 line

0xE507 OILVL register_OILVL R/W 24 32 ZP U =

Quoted 1 line

0xE508 OVLVL register_OVLVL R/W 24 32 ZP U =

These presumably result in interrupts? (I'm running out of time so not =

In which case standard event interface should work. =20

Quoted 1 line

0xE509 SAGLVL register_SAGLVL R/W 24 32 ZP U =

That's another event I think=E2=80=A6=20

=20

Quoted 1 line

0xE50A MASK0 register_MASK0 R/W 32 32 U =

Quoted 1 line

0xE50B MASK1 register_MASK1 R/W 32 32 U =

=20

Quoted 1 line

0xE50C IAWV in_currentA_instantaneous R 24 =

Quoted 1 line

0xE50D IBWV in_currentB_instantaneous R 24 =

Quoted 1 line

0xE50E ICWV in_currentC_instantaneous R 24 =

Quoted 1 line

0xE50F INWV in_currentN_instantaneous R 24 =

Quoted 1 line

0xE510 VAWV in_voltageA_instantaneous R 24 =

Quoted 1 line

0xE511 VBWV in_voltageB_instantaneous R 24 =

Quoted 1 line

0xE512 VCWV in_voltageC_instantaneous R 24 =

OK, this is getting silly. I presume this means the values above are =

aren't? If so you need to have channels for both and different filter =

=20

Quoted 1 line

0xE513 AWATT in_powerA_instantaneous R 24 32 SE =

Quoted 1 line

0xE514 BWATT in_powerB_instantaneous R 24 32 SE =

Quoted 1 line

0xE515 CWATT in_powerC_instantaneous R 24 32 SE =

Quoted 1 line

0xE516 AVAR in_powerreactiveA_instantaneous R 24 =

Quoted 1 line

0xE517 BVAR in_powerreactiveB_instantaneous R 24 =

Quoted 1 line

0xE518 CVAR in_powerreactiveC_instantaneous R 24 =

Quoted 1 line

0xE519 AVA in_powerapparentA_instantaneous R 24 =

Quoted 1 line

0xE51A BVA in_powerapparentB_instantaneous R 24 =

Quoted 1 line

0xE51B CVA in_powerappatentC_instantaneous R 24 =

Same for all of these.
=20

Quoted 1 line

0xE51F CHECKSUM register_CHECKSUM R 32 =

Not exposed to userspace.

=20

Quoted 1 line

0xE520 VNOM in_voltage_rms_nominal R/W 24 32 ZP =

Why would this be done? Sounds like something that is a circuit =

decision so a job for DT to me.

=20

Quoted 1 line

0xE600 PHSTATUS in_current_phase_peak R 16 =

Quoted 1 line

0xE601 ANGLE0 register_ANGLE0 R 16 16 U =

Quoted 1 line

0xE602 ANGLE1 register_ANGLE1 R 16 16 U =

Quoted 1 line

0xE603 ANGLE2 register_ANGLE2 R 16 16 U =

Hmm. More fun. These are derived values between to phase =

The phase as a modifier falls down a bit here - if we had just treated them as channels we could have done this a differential angle channel. Right now I'm not sure how we do this, could do it as a derived values =

in_angle0_phaseA&phaseB_diff_raw etc but that feels odd. This one needs more thought.
=20

Quoted 1 line

0xE604 to 0xE606 Reserved =

Quoted 1 line

0xE607 PERIOD register_PERIOD R 16 16 U =

Superficially this sounds like a channel free element so =

=20

Quoted 1 line

0xE608 PHNOLOAD register_PHNOLOAD R 16 =

Hmm. So this is kind of a set of events with but without I think an =

Odd.
=20

Quoted 1 line

0xE60C LINECYC register_LINECYC R/W 16 16 =

in_count0_raw probably though it's a bit of a stretch. =20

Quoted 1 line

0xE60D ZXTOUT register_ZXTOUT R/W 16 16 U =

This is going to be another top level one I think and device specific =

=20

Quoted 1 line

0xE60E COMPMODE register_COMPMODE R/W 16 =

If there is stuff to control in here it need breaking out. =20

Quoted 1 line

0xE60F Gain register_Gain R/W 16 16 U =

Oh goody another scale value. Needs breaking up into separate =

Do these directly effect the measured output voltage etc? If they do =

I'm not sure how to separate the two gains, there ought to be a =

answer. If this is about matching to the circuit present then they should probably be coming from DT or simillar.
=20
=20

Quoted 1 line

0xE610 CFMODE register_CFMODE R/W 16 16 U =

Quoted 1 line

0xE611 CF1DEN register_CF1DEN R/W 16 16 U =

Quoted 1 line

0xE612 CF2DEN register_CF2DEN R/W 16 16 U =

Quoted 1 line

0xE613 CF3DEN register_CF3DEN R/W 16 16 U =

Are these things that should be in DT? Look very quickly like they =

=20

Quoted 1 line

0xE614 APHCAL register_APHCAL R/W 10 16 ZP S =

Quoted 1 line

0xE615 BPHCAL register_BPHCAL R/W 10 16 ZP S =

Quoted 1 line

0xE616 CPHCAL register__CPHCAL R/W 10 16 ZP =

I'm not actually sure how you would set these. Per circuit design?

=20

Quoted 1 line

0xE617 PHSIGN register_PHSIGN R 16 16 U =

Quoted 1 line

0xE618 CONFIG register_CONFIG R/W 16 16 U =

Quoted 1 line

0xE700 MMODE register__MMODE R/W 8 8 U =

Quoted 1 line

0xE701 ACCMODE register__ACCMODE R/W 8 8 =

Quoted 1 line

0xE702 LCYCMODE register_LCYCMODE R/W 8 =

Quoted 1 line

0xE703 PEAKCYC register_PEAKCYC R/W 8 8 =

Quoted 1 line

0xE704 SAGCYC register_SAGCYC R/W 8 8 U =

Some of these are event controls. Map them as such.

=20

Quoted 1 line

0xE705 CFCYC register_CFCYC R/W 8 8 U =

Quoted 1 line

0xE706 HSDC_CFG register_HSDC_CFG R/W 8 =

Quoted 1 line

0xE707 Version register_Version R 8 8 =

Quoted 1 line

0xEBFF Reserved 8 8 =

Quoted 1 line

0xEC00 LPOILVL register_LPOILVL R/W 8 8 =

Quoted 2 lines

only). See Table 54 in which the register is detailed."
0xEC01 CONFIG2 register_CONFIG2 R/W 8 8 =

=20
As you can guess I was running out of stamina towards the end of that. =20
I'm not totally sure of the answer I provided. It may take some more =

Ideally some others will give input on this question.

=20
Jonathan

Quoted 9 lines

=20
Regards,
John
=20
=20
=20
=20
=20

Quoted 1 line

On Mar 17, 2018, at 1:30 PM, Jonathan Cameron <[email protected]> =

Quoted 34 lines

Quoted 34 lines

=20
On Wed, 14 Mar 2018 23:12:02 -0700
John Syne <[email protected]> wrote:
=20

Quoted 6 lines

Hi Jonathan,
=20
I have been looking at the IIO ABI docs and if I understand correctly, the idea is to use consistent naming conventions? So for example, looking at the ADE7854 datasheet, the naming matching the ADE7854 registers would be as follows: =20

=20
Welcome to one of the big reasons no one tidied these drivers up originally. Still we have moved on somewhat since then so similar circumstances have come up in other types of sensor. =20

Quoted 4 lines

=20
{direction}_{type}_{index}_{modifier}_{info_mask}
=20
AIGAIN - In_current_a_gain =20

Other than the fact that gain isn't an ABI element and that index doesn't have
_ before it that is right.
in_voltageA_scale
=20
That was a weird quirk a long time back which we should probably not have done (copied it from hwmon)
=20

Quoted 7 lines

AVGAIN - in_voltage_a_gain
BIGAIN - in_current_b_gain
BVGAIN - in_voltage_b_gain
=E2=80=94
How do we represent the rms and offset
AIRMSOS - in_current_a_rmsoffset
AVRMSOS - in_voltage_a_rmsoffset =20

Quoted 7 lines

Quoted 7 lines

=20
Right now we can't unfortunately though this one is easier to fix. We already have modifiers for multi axis devices doing sum_squared so add one of those for root_mean_square - this one is well known enough that rms is fine in the string.
=20
It's a effectively a different channel be it one derived from a =

Quoted 5 lines

Quoted 5 lines

one. This is going to get tricky however as we would normally use modifier to specialise a channel type - thoughts on this below. =20

Quoted 2 lines

=E2=80=94
Here I don=E2=80=99t understand how to represent both the phase and =

Quoted 1 line

Quoted 1 line

Quoted 1 line

AVAGAIN - in_power_a_gain =

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

=E2=80=94
AWGAIN - in_power_ai_gain =

Quoted 1 line

Quoted 1 line

And that is the problem. How do we represent the various power =

Quoted 1 line

Quoted 1 line

Hmm. We could do it with modifiers but above we show that we have =

Quoted 2 lines

Quoted 2 lines

=20
It would be easy enough to add yet another field to the channel spec =

Quoted 1 line

Quoted 1 line

this but there is a problem - Events. The event format is already =

Quoted 1 line

Quoted 1 line

so where do we put this extra element if we need to define a channel =

Quoted 4 lines

Quoted 4 lines

only by it.
=20
One thought is we could instead define these as different top level IIO_CHAN_TYPES in a similar fashion to we do for relative humidity =

Quoted 11 lines

Quoted 11 lines

the proposed absolute humidity.
=20
in_powerreactiveA_scale
in_poweractivceA_scale
(or in_powerrealA_scale to match with what I got taught years ago?) =20
I presume we keep in_powerA_scale etc for the apparent power and modify any docs to make that clear.
=20

Quoted 2 lines

=E2=80=94
AVARGAIN - in_power_aq_gain =

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

=E2=80=94
Now here it becomes more complicated. Not sure how this gets =

Quoted 3 lines

Quoted 3 lines

Quoted 1 line

AFWATTOS - in_power_a_active/fundamental/offset =20

Yeah, some of these are getting odd.
If I read this correctly this is the active power estimate based on =

Quoted 3 lines

Quoted 3 lines

the fundamental frequency - so no harmonics?
=20
Hmm. This then becomes a separate channel with additional =

Quoted 1 line

Quoted 1 line

specifying it is only the fundamental. This feels a bit like a =

Quoted 1 line

Quoted 1 line

be it an unusual one? Might just be necessary to add a =

Quoted 7 lines

Quoted 7 lines

element on the end (would be info_mask if this is common enough to justify that rather than using the extended methods to define it.). =20
=20

Quoted 2 lines

=E2=80=94
AWATTHR - in_energy_ai_accumulation =20

Great, just when I thought we had gone far enough they define =

Quoted 11 lines

Quoted 11 lines

energy which is presumably roughly the same as reactivepower * time? In that case we need types for that as well. in_energyreactiveA_*
in_energyactiveA_*
=20

Quoted 4 lines

=E2=80=94
AVARHR - in_energy_aq_accumulation
=E2=80=94
IPEAK - in_current_peak =20

That one is easy as we have an info_mask element for peak and one for peak_scale that has always been a bit odd but was needed =

Quoted 4 lines

Quoted 4 lines

=20

Quoted 3 lines

=E2=80=94
=20
I=E2=80=99ll leave it there, because there are some even more =

Quoted 14 lines

Quoted 14 lines

Something to look forward to. Gah, I always hated power engineering though I taught it very briefly (I really pity those students :( =20
Jonathan
=20

Quoted 9 lines

=20
Regards,
John
=20
=20
=20
=20
=20

Quoted 1 line

On Mar 10, 2018, at 7:10 AM, Jonathan Cameron <[email protected]> =

Quoted 9 lines

Quoted 9 lines

Quoted 9 lines

Quoted 9 lines

=20
On Thu, 8 Mar 2018 21:37:33 -0300
Rodrigo Siqueira <[email protected]> wrote:
=20

Quoted 5 lines

On 03/07, Jonathan Cameron wrote: =20

Quoted 4 lines

On Tue, 6 Mar 2018 21:43:47 -0300
Rodrigo Siqueira <[email protected]> wrote:
=20

Quoted 1 line

The macro IIO_DEV_ATTR_CH_OFF is a wrapper for IIO_DEVICE_ATTR, =

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

tiny change in the name definition. This extra macro does not =

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

the readability and also creates some checkpatch errors. =20
This patch fixes the checkpatch.pl errors:
=20
staging/iio/meter/ade7753.c:391: ERROR: Use 4 digit octal =

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

decimal permissions staging/iio/meter/ade7753.c:395: ERROR: Use 4 digit octal =

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

decimal permissions staging/iio/meter/ade7759.c:331: ERROR: Use 4 digit octal =

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

decimal permissions staging/iio/meter/ade7759.c:335: ERROR: Use 4 digit octal =

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

decimal permissions
=20

Signed-off-by Rodrigo Siqueira <[email protected]> =

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

=20
Hmm. I wondered a bit about this one. It's a correct patch in of itself but the interface in question doesn't even vaguely =

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

to any of defined IIO ABI. Anyhow, it's still and improvement =

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 1 line

I'll take it. =20

=20
I am not sure if I understood the comment about the ABI. The =

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

interface is wrong because it uses things like IIO_DEVICE_ATTR? =

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

should use iio_info together with *write_raw and *read_raw. =

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

Quoted 1 line

the ABI problem that you refer? =20

The ABI is about the userspace interface of IIO. It is defined in Documentation/ABI/testing/sysfs-bus-iio*
So this documents the naming of sysfs attributes and (more or =

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

describes a consistent interface to userspace across lots of =

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

types of devices.
=20
A lot of these older drivers in staging involve a good deal of ABI =

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

was not reviewed or discussed. That is one of the biggest reasons =

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

didn't take them out of staging in the first place.
=20
In order for generic userspace programs to have any idea what to =

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

with these devices this all needs to be fixed.
=20
There may well be cases where we need to expand the existing ABI =

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

cover new things. That's fine, but it has to be done with full
review of the relevant documentation patches.
=20
Incidentally if you want an easy driver to work on moving out of =

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

then first thing to do is to compare what it shows to userspace =

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

docs. If it's totally different then you have a big job on your =

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

as often ABI can take a lot of discussion and a long time to =

Quoted 9 lines

Quoted 9 lines

Quoted 9 lines

Quoted 9 lines

a consensus.
=20
Jonathan
=20
=20

Quoted 4 lines

=20
Thanks :)
=20

Quoted 1 line

Applied to the togreg branch of iio.git and pushed out as =

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

for the autobuilders to play with it.
=20
I also added the removal of the header define which is no longer used.
=20
Please note, following discussions with Michael, I am going to =

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

an email announcing an intent to drop these meter drivers next cycle unless someone can provide testing for any attempt to move them out of staging. I'm still taking patches on the basis that 'might' happen - but I wouldn't focus on these until we have some certainty on whether they will be around long term! =20
Jonathan
=20

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

=

Quoted 6 lines

---
drivers/staging/iio/meter/ade7753.c | 18 ++++++++++-------- drivers/staging/iio/meter/ade7759.c | 18 ++++++++++--------

2 files changed, 20 insertions(+), 16 deletions(-)

=20

= renamed from drivers/staging/iio/meter/ade7753.c +0 -0

Quoted 21 lines

Quoted 21 lines

Quoted 21 lines

Quoted 21 lines

Quoted 21 lines

Quoted 21 lines

Quoted 21 lines

index c44eb577dc35..275e8dfff836 100644

drivers/staging/iio/meter/ade7753.c +6 -8

@@ -388,14 +388,16 @@ static IIO_DEV_ATTR_VPERIOD(0444,
 		ade7753_read_16bit,
 		NULL,
 		ADE7753_PERIOD);
-static IIO_DEV_ATTR_CH_OFF(1, 0644,
-		ade7753_read_8bit,
-		ade7753_write_8bit,
-		ADE7753_CH1OS);
-static IIO_DEV_ATTR_CH_OFF(2, 0644,
-		ade7753_read_8bit,
-		ade7753_write_8bit,
-		ADE7753_CH2OS);
+
+static IIO_DEVICE_ATTR(choff_1, 0644,
+			ade7753_read_8bit,
+			ade7753_write_8bit,
+			ADE7753_CH1OS);
+

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

=

Quoted 8 lines

drivers/staging/iio/meter/ade7753.c +4 -0

+static IIO_DEVICE_ATTR(choff_2, 0644,
+			ade7753_read_8bit,
+			ade7753_write_8bit,
+			ADE7753_CH2OS);

=20
static int ade7753_set_irq(struct device *dev, bool enable) {

= renamed from drivers/staging/iio/meter/ade7759.c +0 -0

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

drivers/staging/iio/meter/ade7759.c static =

Quoted 4 lines

index 1decb2b8afab..c078b770fa53 100644

drivers/staging/iio/meter/ade7759.c +0 -0

@@ -328,14 +328,16 @@ static =

Quoted 25 lines

Quoted 25 lines

Quoted 25 lines

Quoted 25 lines

Quoted 25 lines

Quoted 25 lines

Quoted 21 lines

drivers/staging/iio/meter/ade7759.c +10 -8

 		ade7759_read_16bit,
 		ade7759_write_16bit,
 		ADE7759_APGAIN);
-static IIO_DEV_ATTR_CH_OFF(1, 0644,
-		ade7759_read_8bit,
-		ade7759_write_8bit,
-		ADE7759_CH1OS);
-static IIO_DEV_ATTR_CH_OFF(2, 0644,
-		ade7759_read_8bit,
-		ade7759_write_8bit,
-		ADE7759_CH2OS);
+
+static IIO_DEVICE_ATTR(choff_1, 0644,
+			ade7759_read_8bit,
+			ade7759_write_8bit,
+			ADE7759_CH1OS);
+
+static IIO_DEVICE_ATTR(choff_2, 0644,
+			ade7759_read_8bit,
+			ade7759_write_8bit,
+			ADE7759_CH2OS);

Quoted 3 lines

=20
static int ade7759_set_irq(struct device *dev, bool enable)
{ =20

=20

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

--
To unsubscribe from this list: send the line "unsubscribe =

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

the body of a message to [email protected] More majordomo info at =

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

=20
_______________________________________________
devel mailing list
[email protected]
=

Quoted 3 lines

Quoted 2 lines

Quoted 1 line

=20

=20

=20

=20

Hi Jonathan,

Thank you for all your hard work. Your feedback is really helpful. I’m surprised that no one from Analog Device has offered any suggestions.

Anyway, please see my comments inline below

Regards,
John

Quoted 24 lines · Jonathan Cameron

Quoted 1 line · On Sat, 17 Mar 2018 23:11:45 -0700 John Syne <[email protected]> wrote:

Hi Jonathan,

Hi John and All,

I'd love to get some additional input on this from anyone interested. There are a lot of weird and wonderful derived quantities in an energy meter and it seems we need to make some fundamental changes to support them - including potentially a userspace breaking change to the event code description.

Quoted 10 lines

Here is the complete list of registers for the ADE7878 which I copied from the data sheet. I added a column “IIO Attribute” which I hope follows your IIO ABI. Please make any changes you feel are incorrect. BTW, there are several registers that cannot be generalized and are used purely for chip configuration. I think we should add a new naming convention, namely {register}_{<chip-register-name>}. Also, I see in the sys_bus_iio doc

in_accel_x_peak_raw

so shouldn’t the phase be represented as follows:

Quoted 7 lines

Quoted 1 line

in_current_A_scale

I'm still confused. What does A represent here? I assumed that was a wild card for the channel number before but clearly not.

Ah, you are labelling the 3 separate phases as A, B and C. Hmm. I guess they sort of look like axis, and sort of like independent channels. So could be indexed or done via modifiers depending on how you look at it.

In metering terminology, the three phases are commonly referred to as RED, GREEN, BLUE or PhaseA, PhaseB, PhaseC and Neutral. Analog Devices uses the ABCN nomenclature so anyone using this driver will probably have referenced the Analog Devices datasheet so this terminology should be familiar.

Quoted 4 lines

Hmm. With neutral in there as well I guess we need to make them modifiers (but might change my mind later ;)

Quoted 5 lines

Particularly as we are using the the modifier for RMS under the previous plan... It appears we should treat that instead like we did for peak and do it as an additional info mask element. The problem with doing that on a continuous measurement is that we can't treat it as a channel to be output through the buffered interface.

I’ve changed the layout of the spreadsheet to breakout the Direction, Type, Index, Modifier and Info Mask to make sure there is no miss-understanding and will help identify all the items we need to add.

The ADE7878 channels that will use the buffer are IAWV, VAWV, IBWV, VBWV, ICWV, VCWV, INWV, AVA, BVA, CVA, AWATT, BWATT, CWATT, AVAR, BVAR, and CVAR. So I guess we have to add an index

Address Register IIO Attribute Dir Type Index Modifier Info Mask R/W Bit Bit Length Type Default Description
Length During Comm Value
0xE50C IAWV in_current0_phaseA_instantaneous in current 0 phaseA instantaneous R 24 32 SE S N/A Instantaneous value of Phase A current.

0xE50D IBWV in_current1_phaseB_instantaneous in current 1 phaseB instantaneous R 24 32 SE S N/A Instantaneous value of Phase B current.
0xE50E ICWV in_current2_phaseC_instantaneous in current 2 phaseC instantaneous R 24 32 SE S N/A Instantaneous value of Phase C current.
0xE50F INWV in_current3_phaseN_instantaneous in current 3 neutral instantaneous R 24 32 SE S N/A Instantaneous value of neutral current (ADE7868 and ADE7878 only).

0xE510 VAWV in_voltage4_phaseA_instantaneous in voltage 4 phaseA instantaneous R 24 32 SE S N/A Instantaneous value of Phase A voltage.
0xE511 VBWV in_voltage5_phaseB_instantaneous in voltage 5 phaseB instantaneous R 24 32 SE S N/A Instantaneous value of Phase B voltage.
0xE512 VCWV in_voltage6_phaseC_instantaneous in voltage 6 phaseC instantaneous R 24 32 SE S N/A Instantaneous value of Phase C voltage.
0xE513 AWATT in_power7_phaseA_instantaneous in power 7 phaseA instantaneous R 24 32 SE S N/A Instantaneous value of Phase A total active power.
0xE514 BWATT in_power8_phaseB_instantaneous in power 8 phaseB instantaneous R 24 32 SE S N/A Instantaneous value of Phase B total active power.
0xE515 CWATT in_power9_phaseC_instantaneous in power 9 phaseC instantaneous R 24 32 SE S N/A Instantaneous value of Phase C total active power.

0xE516 AVAR in_powerreactive10_phaseA_instantaneous in powerreactive 10 phaseA instantaneous R 24 32 SE S N/A Instantaneous value of Phase A total reactive power (ADE7858, ADE7868, and ADE7878 only).

0xE517 BVAR in_powerreactive11_phaseB_instantaneous in powerreactive 11 phaseB instantaneous R 24 32 SE S N/A Instantaneous value of Phase B total reactive power (ADE7858, ADE7868, and ADE7878 only).
0xE518 CVAR in_powerreactive12_phaseC_instantaneous in powerreactive 12 phaseC instantaneous R 24 32 SE S N/A Instantaneous value of Phase C total reactive power (ADE7858, ADE7868, and ADE7878 only).
0xE519 AVA in_powerapparent13_phaseA_instantaneous in powerapparent 13 phaseA instantaneous R 24 32 SE S N/A Instantaneous value of Phase A apparent power.
0xE51A BVA in_powerapparent14_phaseB_instantaneous in powerapparent 14 phaseB instantaneous R 24 32 SE S N/A Instantaneous value of Phase B apparent power.
0xE51B CVA in_powerappatent15_phaseC_instantaneous in powerapparent 15 phaseC instantaneous R 24 32 SE S N/A Instantaneous value of Phase C apparent power.

The ADE9000 channels that use the buffer are IA, VA, IB, VB, IC, VC and IN

Quoted 19 lines

So again we have run out of space. It's increasingly looking like we need room for another field in the events - to cleanly represent computed values.

Hmm. What is the current usage? - it's been a while so I had to go look in the header.

0-15 Channel (lots of channels)
31-16 Channel 2 (36 modifiers - lots of channels)
47-32 Channel Type (31 used so far - this looks most likely to run out of space in the long run so leave this one alone).
54-48 Event Direction (4 used)
55 Differential (1: channel 2 as differential pair 0: as a modifier) 63-56 Event Type (6 used)

So I think we can pinch bit 53 as another flag to indicate we have a computed value or possibly bit 63 as event types are few and far between as well.

Quoted 5 lines

Probably reasonable to assume we never have 16 bits worth of channels and computed channels at the same time? Hence I think we can steal bits off the top of Channel. How many do we need? Not sure unfortunately but feels like 8 should be plenty.

When you speak of Channels here, are you referring to measurements that will use the buffer? Even when counting all the IIO attributes, the ADE9000 has 462 registers, and not all of those would be defined as attributes. I think 8 Bits should be more than enough.

Quoted 64 lines

The other element of this is we add a new field to iio_chan_spec to contain 'derived_type' or something like that which has rms and sum squared etc. Over time we can move some of those from the modifiers and free up a few entires there.
So modifier might be "X and Y and Z" with a derived_type of sum_squared to give existing sum_squared_x_y_z but no rush on that.

Anyhow so now we have an extra element to play that will result in a different channel.

Whilst here we should think about any other mods needed to that event structure. It is a little unfortunate that this will be a breaking change for any old userspace code playing with new drivers but it can't be helped as we didn't have reserved values in the original definition (oops).

At somepoint we may need to add the 'shared by derived_value' info mask but I think we can ignore that for now (seems moderately unlikely to have anything in it!)

Quoted 6 lines

But for now, I followed your instructions from your reply.

After finalizing this one, I will work on the ADE9000, which as way more registers ;-)

Once we can agree on the register naming, I will update the ADE7854 driver for Rodrigo, which will go a long way to getting it out of staging.

I'll edit to fit with new scheme and insert indexes which I think would be preferred though optional under the ABI as we only have one of each type/

Quoted 3 lines

Address Register IIO Attribute R/W Bit Length Bit Length During Communications Type Default Value Description
0x4380 AIGAIN in_current0_phaseA_scale R/W 24 32 ZPSE S 0x000000 Phase A current gain adjust.

A, B, C, N aren't obvious to the lay reader so I suggest we burn a few characters and stick phase in for ABC and just have neutral for the neutral one. Not sure about capitalization or not though.

Quoted 29 lines

0x4381 AVGAIN in_voltage0_phaseA_scale R/W 24 32 ZPSE S 0x000000 Phase A voltage gain adjust.
0x4382 BIGAIN in_current0_phaseB_scale R/W 24 32 ZPSE S 0x000000 Phase B current gain adjust.
0x4383 BVGAIN in_voltage0_phaseB_scale R/W 24 32 ZPSE S 0x000000 Phase B voltage gain adjust.
0x4384 CIGAIN in_current0_phaseC_scale R/W 24 32 ZPSE S 0x000000 Phase C current gain adjust.
0x4385 CVGAIN in_voltage0_phaseC_scale R/W 24 32 ZPSE S 0x000000 Phase C voltage gain adjust.
0x4386 NIGAIN in_current0_neutral_scale R/W 24 32 ZPSE S 0x000000 Neutral current gain adjust (ADE7868 and ADE7878 only).
0x4387 AIRMSOS in_current0_phaseA_rms_offset R/W 24 32 ZPSE S 0x000000 Phase A current rms offset.
0x4388 AVRMSOS in_voltage0_phaseA_rms_offset R/W 24 32 ZPSE S 0x000000 Phase A voltage rms offset.
0x4389 BIRMSOS in_current0_phaseB_rms_offset R/W 24 32 ZPSE S 0x000000 Phase B current rms offset.
0x438A BVRMSOS in_voltage0_phaseB_rms_offset R/W 24 32 ZPSE S 0x000000 Phase B voltage rms offset.
0x438B CIRMSOS in_current0_phaseC_rms_offset R/W 24 32 ZPSE S 0x000000 Phase C current rms offset.
0x438C CVRMSOS in_voltage0_phaseC_rms_offset R/W 24 32 ZPSE S 0x000000 Phase C voltage rms offset.
0x438D NIRMSOS in_current0_neutral_rms_offset R/W 24 32 ZPSE S 0x000000 Neutral current rms offset (ADE7868 and ADE7878 only).
0x438E AVAGAIN in_powerapparent0_phaseA_scale R/W 24 32 ZPSE S 0x000000 Phase A apparent power gain adjust.
0x438F BVAGAIN in_powerapparent0_phaseB_scale R/W 24 32 ZPSE S 0x000000 Phase B apparent power gain adjust.
0x4390 CVAGAIN in_powerapparent0_phaseC_scale R/W 24 32 ZPSE S 0x000000 Phase C apparent power gain adjust.
0x4391 AWGAIN in_power0_phaseA_scale R/W 24 32 ZPSE S 0x000000 Phase A total active power gain adjust.
0x4392 AWATTOS in_power0_phaseA_offset R/W 24 32 ZPSE S 0x000000 Phase A total active power offset adjust.
0x4393 BWGAIN in_power0_phaseB_scale R/W 24 32 ZPSE S 0x000000 Phase B total active power gain adjust.
0x4394 BWATTOS in_power0_phaseB_offset R/W 24 32 ZPSE S 0x000000 Phase B total active power offset adjust.
0x4395 CWGAIN in_power0_PhaseC_scale R/W 24 32 ZPSE S 0x000000 Phase C total active power gain adjust.
0x4396 CWATTOS in_power0_phaseC_offset R/W 24 32 ZPSE S 0x000000 Phase C total active power offset adjust.
0x4397 AVARGAIN in_powerreactive0_phaseA_scale R/W 24 32 ZPSE S 0x000000 Phase A total reactive power gain adjust (ADE7858, ADE7868, and ADE7878).
0x4398 AVAROS in_powerreactive0_phaseA_offset R/W 24 32 ZPSE S 0x000000 Phase A total reactive power offset adjust (ADE7858, ADE7868, and ADE7878).
0x4399 BVARGAIN in_powerreactive0_phaseB_scale R/W 24 32 ZPSE S 0x000000 Phase B total reactive power gain adjust (ADE7858, ADE7868, and ADE7878).
0x439A BVAROS in_powerreactive0_phaseB_offset R/W 24 32 ZPSE S 0x000000 Phase B total reactive power offset adjust (ADE7858, ADE7868, and ADE7878).
0x439B CVARGAIN in_powerreactive0_phaseC_scale R/W 24 32 ZPSE S 0x000000 Phase C total reactive power gain adjust (ADE7858, ADE7868, and ADE7878).
0x439C CVAROS in_powerreactive0_phaseC_offset R/W 24 32 ZPSE S 0x000000 Phase C total reactive power offset adjust (ADE7858, ADE7868, and ADE7878).
0x439D AFWGAIN in_power0_phaseA_fundamental_scale R/W 24 32 ZPSE S 0x000000 Phase A fundamental active power gain adjust. Location reserved for ADE7854, ADE7858, and ADE7868.