staging: iio: meter: Header file maintenance

PATCH Superseded Katie Dunne 4 participants 8 messages Feb 28, 2017 – Mar 2, 2017

Subsystems (4)

Staging IIO Analog Devices IIO drivers Staging - industrial io

lore

Share & tools

lore

Katie Dunne

author

2017-02-28 03:16 UTC #0 lore raw

cover staging: iio: meter: Header file maintenance

Remove unnecessary definitions, collapse header file into source, and clean up includes in implementation.

Katie Dunne (3):
staging: iio: meter: Remove unused definitions
staging: iio: meter: Move header content to implementation file
staging: iio: meter: Clean up #includes

 drivers/staging/iio/meter/ade7753.c | 70 ++++++++++++++++++++++++++++++++----
 drivers/staging/iio/meter/ade7753.h | 72 -------------------------------------
 2 files changed, 63 insertions(+), 79 deletions(-)
 delete mode 100644 drivers/staging/iio/meter/ade7753.h

Signature · 1 line

2.7.4

Katie Dunne

author

2017-02-28 03:18 UTC #1 lore raw

PATCH 1/3 staging: iio: meter: Remove unused definitions

Remove definitions not used in kernel code.

Signed-off-by Katie Dunne <[email protected]>

---
 drivers/staging/iio/meter/ade7753.h | 14 --------------
 1 file changed, 14 deletions(-)

drivers/staging/iio/meter/ade7753.h +0 ~~-14~~

index bfe7491..ea9284a 100644
@@ -1,18 +1,12 @@
 #ifndef _ADE7753_H
 #define _ADE7753_H
 
-#define ADE7753_WAVEFORM   0x01
 #define ADE7753_AENERGY    0x02
-#define ADE7753_RAENERGY   0x03
 #define ADE7753_LAENERGY   0x04
 #define ADE7753_VAENERGY   0x05
-#define ADE7753_RVAENERGY  0x06
 #define ADE7753_LVAENERGY  0x07
-#define ADE7753_LVARENERGY 0x08
 #define ADE7753_MODE       0x09
 #define ADE7753_IRQEN      0x0A
-#define ADE7753_STATUS     0x0B
-#define ADE7753_RSTSTATUS  0x0C
 #define ADE7753_CH1OS      0x0D
 #define ADE7753_CH2OS      0x0E
 #define ADE7753_GAIN       0x0F

@@ -27,22 +21,15 @@
 #define ADE7753_IRMSOS     0x18
 #define ADE7753_VRMSOS     0x19
 #define ADE7753_VAGAIN     0x1A
-#define ADE7753_VADIV      0x1B
 #define ADE7753_LINECYC    0x1C
-#define ADE7753_ZXTOUT     0x1D
 #define ADE7753_SAGCYC     0x1E
 #define ADE7753_SAGLVL     0x1F
 #define ADE7753_IPKLVL     0x20
 #define ADE7753_VPKLVL     0x21
 #define ADE7753_IPEAK      0x22
-#define ADE7753_RSTIPEAK   0x23
 #define ADE7753_VPEAK      0x24
-#define ADE7753_RSTVPEAK   0x25
-#define ADE7753_TEMP       0x26
 #define ADE7753_PERIOD     0x27
-#define ADE7753_TMODE      0x3D
 #define ADE7753_CHKSUM     0x3E
-#define ADE7753_DIEREV     0x3F
 
 #define ADE7753_READ_REG(a)    a
 #define ADE7753_WRITE_REG(a) ((a) | 0x80)

@@ -52,7 +39,6 @@
 #define ADE7753_STARTUP_DELAY 1000
 
 #define ADE7753_SPI_SLOW	(u32)(300 * 1000)
-#define ADE7753_SPI_BURST	(u32)(1000 * 1000)
 #define ADE7753_SPI_FAST	(u32)(2000 * 1000)
 
 /**

Signature · 1 line

2.7.4

Julia Lawall

2017-02-28 06:47 UTC #2 lore raw

Re: [Outreachy kernel] [PATCH 1/3] staging: iio: meter: Remove unused definitions

Quoted 1 line · Katie Dunne

Remove definitions not used in kernel code.

Did Alison ask for this? Normally these kinds of constant definitions are kept, because they describe the device in some way. If you see macros that perform more complex computations that are not used, you can get rid of those.

julia

Quoted 70 lines

Signed-off-by Katie Dunne <[email protected]>

---
 drivers/staging/iio/meter/ade7753.h | 14 --------------
 1 file changed, 14 deletions(-)

drivers/staging/iio/meter/ade7753.h +0 ~~-15~~

index bfe7491..ea9284a 100644
@@ -1,18 +1,12 @@
 #ifndef _ADE7753_H
 #define _ADE7753_H
 
-#define ADE7753_WAVEFORM   0x01
 #define ADE7753_AENERGY    0x02
-#define ADE7753_RAENERGY   0x03
 #define ADE7753_LAENERGY   0x04
 #define ADE7753_VAENERGY   0x05
-#define ADE7753_RVAENERGY  0x06
 #define ADE7753_LVAENERGY  0x07
-#define ADE7753_LVARENERGY 0x08
 #define ADE7753_MODE       0x09
 #define ADE7753_IRQEN      0x0A
-#define ADE7753_STATUS     0x0B
-#define ADE7753_RSTSTATUS  0x0C
 #define ADE7753_CH1OS      0x0D
 #define ADE7753_CH2OS      0x0E
 #define ADE7753_GAIN       0x0F

@@ -27,22 +21,15 @@
 #define ADE7753_IRMSOS     0x18
 #define ADE7753_VRMSOS     0x19
 #define ADE7753_VAGAIN     0x1A
-#define ADE7753_VADIV      0x1B
 #define ADE7753_LINECYC    0x1C
-#define ADE7753_ZXTOUT     0x1D
 #define ADE7753_SAGCYC     0x1E
 #define ADE7753_SAGLVL     0x1F
 #define ADE7753_IPKLVL     0x20
 #define ADE7753_VPKLVL     0x21
 #define ADE7753_IPEAK      0x22
-#define ADE7753_RSTIPEAK   0x23
 #define ADE7753_VPEAK      0x24
-#define ADE7753_RSTVPEAK   0x25
-#define ADE7753_TEMP       0x26
 #define ADE7753_PERIOD     0x27
-#define ADE7753_TMODE      0x3D
 #define ADE7753_CHKSUM     0x3E
-#define ADE7753_DIEREV     0x3F
 
 #define ADE7753_READ_REG(a)    a
 #define ADE7753_WRITE_REG(a) ((a) | 0x80)

@@ -52,7 +39,6 @@
 #define ADE7753_STARTUP_DELAY 1000
 
 #define ADE7753_SPI_SLOW	(u32)(300 * 1000)
-#define ADE7753_SPI_BURST	(u32)(1000 * 1000)
 #define ADE7753_SPI_FAST	(u32)(2000 * 1000)
 
 /**
--

2.7.4

--
You received this message because you are subscribed to the Google Groups "outreachy-kernel" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [email protected].
To post to this group, send email to [email protected].
To view this discussion on the web visit https://groups.google.com/d/msgid/outreachy-kernel/972438f1c7bc46153df933c5861fac91f6793e92.1488250885.git.kdunne%40mail.ccsf.edu.
For more options, visit https://groups.google.com/d/optout.

Daniel Baluta

2017-02-28 08:13 UTC #3 lore raw

Re: [Outreachy kernel] [PATCH 1/3] staging: iio: meter: Remove unused definitions

Quoted 10 lines · Julia Lawall

Quoted 1 line · Katie Dunne
Remove definitions not used in kernel code.
Did Alison ask for this? Normally these kinds of constant definitions are kept, because they describe the device in some way. If you see macros that perform more complex computations that are not used, you can get rid of those.

Agree with Julia, lets keep this because they could be good documentation.

Daniel.

Jonathan Cameron

maintainer

2017-03-02 19:14 UTC #4 lore raw

Re: [Outreachy kernel] [PATCH 1/3] staging: iio: meter: Remove unused definitions

Quoted 5 lines · Daniel Baluta

Quoted 4 lines · Julia Lawall
On Mon, 27 Feb 2017, Katie Dunne wrote:

Quoted 8 lines

Quoted 6 lines
Quoted 1 line
Remove definitions not used in kernel code.
Did Alison ask for this? Normally these kinds of constant definitions are kept, because they describe the device in some way. If you see macros that perform more complex computations that are not used, you can get rid of those.
Agree with Julia, lets keep this because they could be good documentation.

Agreed. Katie, would you mind posting a V2 of the series with just the other two patches updated to account for this one not being there?

Thanks,

Jonathan

Quoted 7 lines

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

Katie Dunne

author

2017-02-28 03:21 UTC #5 lore raw

PATCH 2/3 staging: iio: meter: Move header content to implementation file

In reply to Katie Dunne

The contents of ade7753.h are only used in ade7753.c. Move the header contents to the implementation file, and delete the header file.

Signed-off-by Katie Dunne <[email protected]>

---
 drivers/staging/iio/meter/ade7753.c | 59 +++++++++++++++++++++++++++++++++++--
 drivers/staging/iio/meter/ade7753.h | 58 ------------------------------------
 2 files changed, 57 insertions(+), 60 deletions(-)
 delete mode 100644 drivers/staging/iio/meter/ade7753.h

drivers/staging/iio/meter/ade7753.c +57 -2

index 671dc99..f855c60 100644
@@ -21,7 +21,27 @@
 #include <linux/iio/iio.h>
 #include <linux/iio/sysfs.h>
 #include "meter.h"
-#include "ade7753.h"
+
+#define ADE7753_READ_REG(a)    a
+#define ADE7753_WRITE_REG(a) ((a) | 0x80)
+#define ADE7753_MAX_TX    4
+#define ADE7753_MAX_RX    4
+#define ADE7753_SPI_SLOW    (u32)(300 * 1000)
+#define ADE7753_SPI_FAST    (u32)(2000 * 1000)
+
+/**
+ * struct ade7753_state - device instance specific data
+ * @us:         actual spi_device
+ * @tx:         transmit buffer
+ * @rx:         receive buffer
+ * @buf_lock:       mutex to protect tx and rx
+ **/
+struct ade7753_state {
+	struct spi_device   *us;
+	struct mutex        buf_lock;
+	u8          tx[ADE7753_MAX_TX] ____cacheline_aligned;
+	u8          rx[ADE7753_MAX_RX];
+};
 
 static int ade7753_spi_write_reg_8(struct device *dev,
 				   u8 reg_address,

@@ -214,6 +234,8 @@ static ssize_t ade7753_write_16bit(struct device *dev,
 	return ret ? ret : len;
 }
 
+#define ADE7753_MODE       0x09
+
 static int ade7753_reset(struct device *dev)
 {
 	u16 val;

@@ -228,6 +250,34 @@ static int ade7753_reset(struct device *dev)
 	return ade7753_spi_write_reg_16(dev, ADE7753_MODE, val);
 }
 
+#define ADE7753_AENERGY    0x02
+#define ADE7753_LAENERGY   0x04
+#define ADE7753_VAENERGY   0x05
+#define ADE7753_LVAENERGY  0x07
+#define ADE7753_CH1OS      0x0D
+#define ADE7753_CH2OS      0x0E
+#define ADE7753_GAIN       0x0F
+#define ADE7753_PHCAL      0x10
+#define ADE7753_APOS       0x11
+#define ADE7753_WGAIN      0x12
+#define ADE7753_WDIV       0x13
+#define ADE7753_CFNUM      0x14
+#define ADE7753_CFDEN      0x15
+#define ADE7753_IRMS       0x16
+#define ADE7753_VRMS       0x17
+#define ADE7753_IRMSOS     0x18
+#define ADE7753_VRMSOS     0x19
+#define ADE7753_VAGAIN     0x1A
+#define ADE7753_LINECYC    0x1C
+#define ADE7753_SAGCYC     0x1E
+#define ADE7753_SAGLVL     0x1F
+#define ADE7753_IPKLVL     0x20
+#define ADE7753_VPKLVL     0x21
+#define ADE7753_IPEAK      0x22
+#define ADE7753_VPEAK      0x24
+#define ADE7753_PERIOD     0x27
+#define ADE7753_CHKSUM     0x3E
+
 static IIO_DEV_ATTR_AENERGY(ade7753_read_24bit, ADE7753_AENERGY);
 static IIO_DEV_ATTR_LAENERGY(ade7753_read_24bit, ADE7753_LAENERGY);
 static IIO_DEV_ATTR_VAENERGY(ade7753_read_24bit, ADE7753_VAENERGY);

@@ -322,6 +372,8 @@ static IIO_DEV_ATTR_CH_OFF(2, S_IWUSR | S_IRUGO,
 		ade7753_write_8bit,
 		ADE7753_CH2OS);
 
+#define ADE7753_IRQEN      0x0A
+
 static int ade7753_set_irq(struct device *dev, bool enable)
 {
 	int ret;

@@ -359,6 +411,8 @@ static int ade7753_stop_device(struct device *dev)
 	return ade7753_spi_write_reg_16(dev, ADE7753_MODE, val);
 }
 
+#define ADE7753_STARTUP_DELAY 1000
+
 static int ade7753_initial_setup(struct iio_dev *indio_dev)
 {
 	int ret;

@@ -389,7 +443,7 @@ static ssize_t ade7753_read_frequency(struct device *dev,
 {
 	int ret;
 	u16 t;
-	int sps;
+int sps;
 
 	ret = ade7753_spi_read_reg_16(dev, ADE7753_MODE, &t);
 	if (ret)

@@ -401,6 +455,7 @@ static ssize_t ade7753_read_frequency(struct device *dev,
 	return sprintf(buf, "%d\n", sps);
 }
 
+
 static ssize_t ade7753_write_frequency(struct device *dev,
 		struct device_attribute *attr,
 		const char *buf,

drivers/staging/iio/meter/ade7753.h deleted +0 ~~-58~~

58 lines not on this page · raw message

Signature · 1 line

2.7.4

Jonathan Cameron

maintainer

2017-03-02 19:16 UTC #6 lore raw

Re: [PATCH 2/3] staging: iio: meter: Move header content to implementation file

Quoted 5 lines · Katie Dunne

The contents of ade7753.h are only used in ade7753.c. Move the header contents to the implementation file, and delete the header file.
Signed-off-by Katie Dunne <[email protected]>

Hi Katie,

I'd actually prefer to have the register definitions all in one place (top of the file after the includes would be best place. Often when reviewing one just wants to do a quick sanity check that there are no typos in register addresses and content and this makes it easy to do.

Also, please include the driver name in the patch title. staging:iio:meter:ade7753 ... would be good here.

Thanks,

Jonathan

Quoted 183 lines

---
 drivers/staging/iio/meter/ade7753.c | 59 +++++++++++++++++++++++++++++++++++--
 drivers/staging/iio/meter/ade7753.h | 58 ------------------------------------
 2 files changed, 57 insertions(+), 60 deletions(-)
 delete mode 100644 drivers/staging/iio/meter/ade7753.h

drivers/staging/iio/meter/ade7753.c +57 -2

index 671dc99..f855c60 100644
@@ -21,7 +21,27 @@
 #include <linux/iio/iio.h>
 #include <linux/iio/sysfs.h>
 #include "meter.h"
-#include "ade7753.h"
+
+#define ADE7753_READ_REG(a)    a
+#define ADE7753_WRITE_REG(a) ((a) | 0x80)
+#define ADE7753_MAX_TX    4
+#define ADE7753_MAX_RX    4
+#define ADE7753_SPI_SLOW    (u32)(300 * 1000)
+#define ADE7753_SPI_FAST    (u32)(2000 * 1000)
+
+/**
+ * struct ade7753_state - device instance specific data
+ * @us:         actual spi_device
+ * @tx:         transmit buffer
+ * @rx:         receive buffer
+ * @buf_lock:       mutex to protect tx and rx
+ **/
+struct ade7753_state {
+	struct spi_device   *us;
+	struct mutex        buf_lock;
+	u8          tx[ADE7753_MAX_TX] ____cacheline_aligned;
+	u8          rx[ADE7753_MAX_RX];
+};
 
 static int ade7753_spi_write_reg_8(struct device *dev,
 				   u8 reg_address,

@@ -214,6 +234,8 @@ static ssize_t ade7753_write_16bit(struct device *dev,
 	return ret ? ret : len;
 }
 
+#define ADE7753_MODE       0x09
+
 static int ade7753_reset(struct device *dev)
 {
 	u16 val;

@@ -228,6 +250,34 @@ static int ade7753_reset(struct device *dev)
 	return ade7753_spi_write_reg_16(dev, ADE7753_MODE, val);
 }
 
+#define ADE7753_AENERGY    0x02
+#define ADE7753_LAENERGY   0x04
+#define ADE7753_VAENERGY   0x05
+#define ADE7753_LVAENERGY  0x07
+#define ADE7753_CH1OS      0x0D
+#define ADE7753_CH2OS      0x0E
+#define ADE7753_GAIN       0x0F
+#define ADE7753_PHCAL      0x10
+#define ADE7753_APOS       0x11
+#define ADE7753_WGAIN      0x12
+#define ADE7753_WDIV       0x13
+#define ADE7753_CFNUM      0x14
+#define ADE7753_CFDEN      0x15
+#define ADE7753_IRMS       0x16
+#define ADE7753_VRMS       0x17
+#define ADE7753_IRMSOS     0x18
+#define ADE7753_VRMSOS     0x19
+#define ADE7753_VAGAIN     0x1A
+#define ADE7753_LINECYC    0x1C
+#define ADE7753_SAGCYC     0x1E
+#define ADE7753_SAGLVL     0x1F
+#define ADE7753_IPKLVL     0x20
+#define ADE7753_VPKLVL     0x21
+#define ADE7753_IPEAK      0x22
+#define ADE7753_VPEAK      0x24
+#define ADE7753_PERIOD     0x27
+#define ADE7753_CHKSUM     0x3E
+
 static IIO_DEV_ATTR_AENERGY(ade7753_read_24bit, ADE7753_AENERGY);
 static IIO_DEV_ATTR_LAENERGY(ade7753_read_24bit, ADE7753_LAENERGY);
 static IIO_DEV_ATTR_VAENERGY(ade7753_read_24bit, ADE7753_VAENERGY);

@@ -322,6 +372,8 @@ static IIO_DEV_ATTR_CH_OFF(2, S_IWUSR | S_IRUGO,
 		ade7753_write_8bit,
 		ADE7753_CH2OS);
 
+#define ADE7753_IRQEN      0x0A
+
 static int ade7753_set_irq(struct device *dev, bool enable)
 {
 	int ret;

@@ -359,6 +411,8 @@ static int ade7753_stop_device(struct device *dev)
 	return ade7753_spi_write_reg_16(dev, ADE7753_MODE, val);
 }
 
+#define ADE7753_STARTUP_DELAY 1000
+
 static int ade7753_initial_setup(struct iio_dev *indio_dev)
 {
 	int ret;

@@ -389,7 +443,7 @@ static ssize_t ade7753_read_frequency(struct device *dev,
 {
 	int ret;
 	u16 t;
-	int sps;
+int sps;
 
 	ret = ade7753_spi_read_reg_16(dev, ADE7753_MODE, &t);
 	if (ret)

@@ -401,6 +455,7 @@ static ssize_t ade7753_read_frequency(struct device *dev,
 	return sprintf(buf, "%d\n", sps);
 }
 
+
 static ssize_t ade7753_write_frequency(struct device *dev,
 		struct device_attribute *attr,
 		const char *buf,

drivers/staging/iio/meter/ade7753.h deleted +0 ~~-58~~

deleted file mode 100644
index ea9284a..0000000
@@ -1,58 +0,0 @@
-#ifndef _ADE7753_H
-#define _ADE7753_H
-
-#define ADE7753_AENERGY    0x02
-#define ADE7753_LAENERGY   0x04
-#define ADE7753_VAENERGY   0x05
-#define ADE7753_LVAENERGY  0x07
-#define ADE7753_MODE       0x09
-#define ADE7753_IRQEN      0x0A
-#define ADE7753_CH1OS      0x0D
-#define ADE7753_CH2OS      0x0E
-#define ADE7753_GAIN       0x0F
-#define ADE7753_PHCAL      0x10
-#define ADE7753_APOS       0x11
-#define ADE7753_WGAIN      0x12
-#define ADE7753_WDIV       0x13
-#define ADE7753_CFNUM      0x14
-#define ADE7753_CFDEN      0x15
-#define ADE7753_IRMS       0x16
-#define ADE7753_VRMS       0x17
-#define ADE7753_IRMSOS     0x18
-#define ADE7753_VRMSOS     0x19
-#define ADE7753_VAGAIN     0x1A
-#define ADE7753_LINECYC    0x1C
-#define ADE7753_SAGCYC     0x1E
-#define ADE7753_SAGLVL     0x1F
-#define ADE7753_IPKLVL     0x20
-#define ADE7753_VPKLVL     0x21
-#define ADE7753_IPEAK      0x22
-#define ADE7753_VPEAK      0x24
-#define ADE7753_PERIOD     0x27
-#define ADE7753_CHKSUM     0x3E
-
-#define ADE7753_READ_REG(a)    a
-#define ADE7753_WRITE_REG(a) ((a) | 0x80)
-
-#define ADE7753_MAX_TX    4
-#define ADE7753_MAX_RX    4
-#define ADE7753_STARTUP_DELAY 1000
-
-#define ADE7753_SPI_SLOW	(u32)(300 * 1000)
-#define ADE7753_SPI_FAST	(u32)(2000 * 1000)
-
-/**
- * struct ade7753_state - device instance specific data
- * @us:			actual spi_device
- * @tx:			transmit buffer
- * @rx:			receive buffer
- * @buf_lock:		mutex to protect tx and rx
- **/
-struct ade7753_state {
-	struct spi_device	*us;
-	struct mutex		buf_lock;
-	u8			tx[ADE7753_MAX_TX] ____cacheline_aligned;
-	u8			rx[ADE7753_MAX_RX];
-};
-
-#endif

Katie Dunne

author

2017-02-28 03:23 UTC #7 lore raw

PATCH 3/3 staging: iio: meter: Clean up #includes

In reply to Katie Dunne

Alphabetize and separate kernel and subsystem headers.

Signed-off-by Katie Dunne <[email protected]>

---
 drivers/staging/iio/meter/ade7753.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

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

index f855c60..5ea3ea2 100644
@@ -6,20 +6,21 @@
  * Licensed under the GPL-2 or later.
  */
 
-#include <linux/interrupt.h>
-#include <linux/irq.h>
 #include <linux/delay.h>
-#include <linux/mutex.h>
 #include <linux/device.h>
+#include <linux/interrupt.h>
+#include <linux/irq.h>
 #include <linux/kernel.h>
+#include <linux/list.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
 #include <linux/spi/spi.h>
 #include <linux/slab.h>
 #include <linux/sysfs.h>
-#include <linux/list.h>
-#include <linux/module.h>
 
 #include <linux/iio/iio.h>
 #include <linux/iio/sysfs.h>
+
 #include "meter.h"
 
 #define ADE7753_READ_REG(a)    a

Signature · 1 line

2.7.4