diff mbox series

[v1,01/12] iio: accel: adxl345: migrate constants to core

Message ID 20250128120100.205523-2-l.rubusch@gmail.com (mailing list archive)
State Changes Requested
Headers show
Series iio: accel: adxl345: add interrupt based sensor events | expand

Commit Message

Lothar Rubusch Jan. 28, 2025, noon UTC 
The set of constants does not need to be exposed. Move constants to core
to reduce namespace polution.

Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>
---
 drivers/iio/accel/adxl345.h      | 86 ------------------------------
 drivers/iio/accel/adxl345_core.c | 91 +++++++++++++++++++++++++++++++-
 2 files changed, 89 insertions(+), 88 deletions(-)

Comments

Jonathan Cameron Feb. 1, 2025, 4:35 p.m. UTC | #1 
On Tue, 28 Jan 2025 12:00:49 +0000
Lothar Rubusch <l.rubusch@gmail.com> wrote:

> The set of constants does not need to be exposed. Move constants to core
> to reduce namespace polution.
> 
> Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>
Hi Lothar,


> -#define ADXL345_REG_INT_MAP		0x2F
> -#define ADXL345_REG_INT_SOURCE		0x30
> -#define ADXL345_REG_INT_SOURCE_MSK	0xFF
>  #define ADXL345_REG_DATA_FORMAT		0x31

Normally I'd be entirely in favour of this, but...
I'm not sure we want to leave one random register here
and move the rest.

Se can move the stuff that isn't register related, but
for the registers I'd prefer to keep them in one place
and I can't see a clean way to move them all to the core.c
file. Even separating reg address and fields within it
makes for a harder check against a datasheet etc.

So I think all we can move is the fifo size :(


> -#define ADXL345_REG_XYZ_BASE		0x32
> -#define ADXL345_REG_DATA_AXIS(index)				\
> -	(ADXL345_REG_XYZ_BASE + (index) * sizeof(__le16))
Lothar Rubusch Feb. 4, 2025, 2:13 p.m. UTC | #2 
Hi Jonathan,

On Sat, Feb 1, 2025 at 5:36 PM Jonathan Cameron <jic23@kernel.org> wrote:
>
> On Tue, 28 Jan 2025 12:00:49 +0000
> Lothar Rubusch <l.rubusch@gmail.com> wrote:
>
> > The set of constants does not need to be exposed. Move constants to core
> > to reduce namespace polution.
> >
> > Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>
> Hi Lothar,
>
>
> > -#define ADXL345_REG_INT_MAP          0x2F
> > -#define ADXL345_REG_INT_SOURCE               0x30
> > -#define ADXL345_REG_INT_SOURCE_MSK   0xFF
> >  #define ADXL345_REG_DATA_FORMAT              0x31
>
> Normally I'd be entirely in favour of this, but...
> I'm not sure we want to leave one random register here
> and move the rest.
>
> Se can move the stuff that isn't register related, but
> for the registers I'd prefer to keep them in one place
> and I can't see a clean way to move them all to the core.c
> file. Even separating reg address and fields within it
> makes for a harder check against a datasheet etc.
>
> So I think all we can move is the fifo size :(
>

I understood that it could be one of the first follow up patches to move those
defines (parts of them?) over to core, as here in this mail:
https://lore.kernel.org/linux-iio/20241214123926.0b42ea59@jic23-huawei/
Anyway, I already had presented moving the constants before, when you
had decided to keep them in the header. I thought you changed your mind
on that, but I don't want to bother you with the same issue over and over
again, probably I missunderstood that here.

I leave the constants in the .h file then, no problem. :) I can understand the
intention to keep the things rather together in one place. There seem to be
pros & cons for both.

>
> > -#define ADXL345_REG_XYZ_BASE         0x32
> > -#define ADXL345_REG_DATA_AXIS(index)                         \
> > -     (ADXL345_REG_XYZ_BASE + (index) * sizeof(__le16))
>
Jonathan Cameron Feb. 4, 2025, 2:46 p.m. UTC | #3 
On Tue, 4 Feb 2025 15:13:34 +0100
Lothar Rubusch <l.rubusch@gmail.com> wrote:

> Hi Jonathan,
> 
> On Sat, Feb 1, 2025 at 5:36 PM Jonathan Cameron <jic23@kernel.org> wrote:
> >
> > On Tue, 28 Jan 2025 12:00:49 +0000
> > Lothar Rubusch <l.rubusch@gmail.com> wrote:
> >  
> > > The set of constants does not need to be exposed. Move constants to core
> > > to reduce namespace polution.
> > >
> > > Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>  
> > Hi Lothar,
> >
> >  
> > > -#define ADXL345_REG_INT_MAP          0x2F
> > > -#define ADXL345_REG_INT_SOURCE               0x30
> > > -#define ADXL345_REG_INT_SOURCE_MSK   0xFF
> > >  #define ADXL345_REG_DATA_FORMAT              0x31  
> >
> > Normally I'd be entirely in favour of this, but...
> > I'm not sure we want to leave one random register here
> > and move the rest.
> >
> > Se can move the stuff that isn't register related, but
> > for the registers I'd prefer to keep them in one place
> > and I can't see a clean way to move them all to the core.c
> > file. Even separating reg address and fields within it
> > makes for a harder check against a datasheet etc.
> >
> > So I think all we can move is the fifo size :(
> >  
> 
> I understood that it could be one of the first follow up patches to move those
> defines (parts of them?) over to core, as here in this mail:
> https://lore.kernel.org/linux-iio/20241214123926.0b42ea59@jic23-huawei/
> Anyway, I already had presented moving the constants before, when you
> had decided to keep them in the header. I thought you changed your mind
> on that, but I don't want to bother you with the same issue over and over
> again, probably I missunderstood that here.

I'd failed to realize we had to leave one behind :( 

Sorry for the misdirection!

Jonathan
> 
> I leave the constants in the .h file then, no problem. :) I can understand the
> intention to keep the things rather together in one place. There seem to be
> pros & cons for both.
> 
> >  
> > > -#define ADXL345_REG_XYZ_BASE         0x32
> > > -#define ADXL345_REG_DATA_AXIS(index)                         \
> > > -     (ADXL345_REG_XYZ_BASE + (index) * sizeof(__le16))  
> >  
>
diff mbox series

Patch

diff --git a/drivers/iio/accel/adxl345.h b/drivers/iio/accel/adxl345.h
index 517e494ba555..b5257dafb742 100644
--- a/drivers/iio/accel/adxl345.h
+++ b/drivers/iio/accel/adxl345.h
@@ -8,94 +8,8 @@ 
 #ifndef _ADXL345_H_
 #define _ADXL345_H_
 
-#define ADXL345_REG_DEVID		0x00
-#define ADXL345_REG_THRESH_TAP		0x1D
-#define ADXL345_REG_OFSX		0x1E
-#define ADXL345_REG_OFSY		0x1F
-#define ADXL345_REG_OFSZ		0x20
-#define ADXL345_REG_OFS_AXIS(index)	(ADXL345_REG_OFSX + (index))
-
-/* Tap duration */
-#define ADXL345_REG_DUR		0x21
-/* Tap latency */
-#define ADXL345_REG_LATENT		0x22
-/* Tap window */
-#define ADXL345_REG_WINDOW		0x23
-/* Activity threshold */
-#define ADXL345_REG_THRESH_ACT		0x24
-/* Inactivity threshold */
-#define ADXL345_REG_THRESH_INACT	0x25
-/* Inactivity time */
-#define ADXL345_REG_TIME_INACT		0x26
-/* Axis enable control for activity and inactivity detection */
-#define ADXL345_REG_ACT_INACT_CTRL	0x27
-/* Free-fall threshold */
-#define ADXL345_REG_THRESH_FF		0x28
-/* Free-fall time */
-#define ADXL345_REG_TIME_FF		0x29
-/* Axis control for single tap or double tap */
-#define ADXL345_REG_TAP_AXIS		0x2A
-/* Source of single tap or double tap */
-#define ADXL345_REG_ACT_TAP_STATUS	0x2B
-/* Data rate and power mode control */
-#define ADXL345_REG_BW_RATE		0x2C
-#define ADXL345_REG_POWER_CTL		0x2D
-#define ADXL345_REG_INT_ENABLE		0x2E
-#define ADXL345_REG_INT_MAP		0x2F
-#define ADXL345_REG_INT_SOURCE		0x30
-#define ADXL345_REG_INT_SOURCE_MSK	0xFF
 #define ADXL345_REG_DATA_FORMAT		0x31
-#define ADXL345_REG_XYZ_BASE		0x32
-#define ADXL345_REG_DATA_AXIS(index)				\
-	(ADXL345_REG_XYZ_BASE + (index) * sizeof(__le16))
-
-#define ADXL345_REG_FIFO_CTL		0x38
-#define ADXL345_FIFO_CTL_SAMPLES_MSK	GENMASK(4, 0)
-/* 0: INT1, 1: INT2 */
-#define ADXL345_FIFO_CTL_TRIGGER_MSK	BIT(5)
-#define ADXL345_FIFO_CTL_MODE_MSK	GENMASK(7, 6)
-#define ADXL345_REG_FIFO_STATUS	0x39
-#define ADXL345_REG_FIFO_STATUS_MSK	0x3F
-
-#define ADXL345_INT_OVERRUN		BIT(0)
-#define ADXL345_INT_WATERMARK		BIT(1)
-#define ADXL345_INT_FREE_FALL		BIT(2)
-#define ADXL345_INT_INACTIVITY		BIT(3)
-#define ADXL345_INT_ACTIVITY		BIT(4)
-#define ADXL345_INT_DOUBLE_TAP		BIT(5)
-#define ADXL345_INT_SINGLE_TAP		BIT(6)
-#define ADXL345_INT_DATA_READY		BIT(7)
-
-/*
- * BW_RATE bits - Bandwidth and output data rate. The default value is
- * 0x0A, which translates to a 100 Hz output data rate
- */
-#define ADXL345_BW_RATE			GENMASK(3, 0)
-#define ADXL345_BW_LOW_POWER		BIT(4)
-#define ADXL345_BASE_RATE_NANO_HZ	97656250LL
-
-#define ADXL345_POWER_CTL_STANDBY	0x00
-#define ADXL345_POWER_CTL_WAKEUP	GENMASK(1, 0)
-#define ADXL345_POWER_CTL_SLEEP	BIT(2)
-#define ADXL345_POWER_CTL_MEASURE	BIT(3)
-#define ADXL345_POWER_CTL_AUTO_SLEEP	BIT(4)
-#define ADXL345_POWER_CTL_LINK		BIT(5)
-
-/* Set the g range */
-#define ADXL345_DATA_FORMAT_RANGE	GENMASK(1, 0)
-/* Data is left justified */
-#define ADXL345_DATA_FORMAT_JUSTIFY	BIT(2)
-/* Up to 13-bits resolution */
-#define ADXL345_DATA_FORMAT_FULL_RES	BIT(3)
 #define ADXL345_DATA_FORMAT_SPI_3WIRE	BIT(6)
-#define ADXL345_DATA_FORMAT_SELF_TEST	BIT(7)
-#define ADXL345_DATA_FORMAT_2G		0
-#define ADXL345_DATA_FORMAT_4G		1
-#define ADXL345_DATA_FORMAT_8G		2
-#define ADXL345_DATA_FORMAT_16G		3
-
-#define ADXL345_DEVID			0xE5
-#define ADXL345_FIFO_SIZE		32
 
 /*
  * In full-resolution mode, scale factor is maintained at ~4 mg/LSB
diff --git a/drivers/iio/accel/adxl345_core.c b/drivers/iio/accel/adxl345_core.c
index ed0291bea0f5..ffdb03ed7a25 100644
--- a/drivers/iio/accel/adxl345_core.c
+++ b/drivers/iio/accel/adxl345_core.c
@@ -14,13 +14,100 @@ 
 #include <linux/regmap.h>
 #include <linux/units.h>
 
-#include <linux/iio/iio.h>
-#include <linux/iio/sysfs.h>
 #include <linux/iio/buffer.h>
+#include <linux/iio/iio.h>
 #include <linux/iio/kfifo_buf.h>
+#include <linux/iio/sysfs.h>
 
 #include "adxl345.h"
 
+#define ADXL345_REG_DEVID		0x00
+#define ADXL345_REG_THRESH_TAP		0x1D
+#define ADXL345_REG_OFSX		0x1E
+#define ADXL345_REG_OFSY		0x1F
+#define ADXL345_REG_OFSZ		0x20
+#define ADXL345_REG_OFS_AXIS(index)	(ADXL345_REG_OFSX + (index))
+
+/* Tap duration */
+#define ADXL345_REG_DUR			0x21
+/* Tap latency */
+#define ADXL345_REG_LATENT		0x22
+/* Tap window */
+#define ADXL345_REG_WINDOW		0x23
+/* Activity threshold */
+#define ADXL345_REG_THRESH_ACT		0x24
+/* Inactivity threshold */
+#define ADXL345_REG_THRESH_INACT	0x25
+/* Inactivity time */
+#define ADXL345_REG_TIME_INACT		0x26
+/* Axis enable control for activity and inactivity detection */
+#define ADXL345_REG_ACT_INACT_CTRL	0x27
+/* Free-fall threshold */
+#define ADXL345_REG_THRESH_FF		0x28
+/* Free-fall time */
+#define ADXL345_REG_TIME_FF		0x29
+/* Axis control for single tap or double tap */
+#define ADXL345_REG_TAP_AXIS		0x2A
+/* Source of single tap or double tap */
+#define ADXL345_REG_ACT_TAP_STATUS	0x2B
+/* Data rate and power mode control */
+#define ADXL345_REG_BW_RATE		0x2C
+#define ADXL345_REG_POWER_CTL		0x2D
+#define ADXL345_REG_INT_ENABLE		0x2E
+#define ADXL345_REG_INT_MAP		0x2F
+#define ADXL345_REG_INT_SOURCE		0x30
+#define ADXL345_REG_INT_SOURCE_MSK	0xFF
+#define ADXL345_REG_XYZ_BASE		0x32
+#define ADXL345_REG_DATA_AXIS(index)				\
+	(ADXL345_REG_XYZ_BASE + (index) * sizeof(__le16))
+
+#define ADXL345_REG_FIFO_CTL		0x38
+#define ADXL345_FIFO_CTL_SAMPLES_MSK	GENMASK(4, 0)
+/* 0: INT1, 1: INT2 */
+#define ADXL345_FIFO_CTL_TRIGGER_MSK	BIT(5)
+#define ADXL345_FIFO_CTL_MODE_MSK	GENMASK(7, 6)
+#define ADXL345_REG_FIFO_STATUS	0x39
+#define ADXL345_REG_FIFO_STATUS_MSK	0x3F
+
+#define ADXL345_INT_OVERRUN		BIT(0)
+#define ADXL345_INT_WATERMARK		BIT(1)
+#define ADXL345_INT_FREE_FALL		BIT(2)
+#define ADXL345_INT_INACTIVITY		BIT(3)
+#define ADXL345_INT_ACTIVITY		BIT(4)
+#define ADXL345_INT_DOUBLE_TAP		BIT(5)
+#define ADXL345_INT_SINGLE_TAP		BIT(6)
+#define ADXL345_INT_DATA_READY		BIT(7)
+
+/*
+ * BW_RATE bits - Bandwidth and output data rate. The default value is
+ * 0x0A, which translates to a 100 Hz output data rate
+ */
+#define ADXL345_BW_RATE			GENMASK(3, 0)
+#define ADXL345_BW_LOW_POWER		BIT(4)
+#define ADXL345_BASE_RATE_NANO_HZ	97656250LL
+
+#define ADXL345_POWER_CTL_STANDBY	0x00
+#define ADXL345_POWER_CTL_WAKEUP	GENMASK(1, 0)
+#define ADXL345_POWER_CTL_SLEEP	BIT(2)
+#define ADXL345_POWER_CTL_MEASURE	BIT(3)
+#define ADXL345_POWER_CTL_AUTO_SLEEP	BIT(4)
+#define ADXL345_POWER_CTL_LINK		BIT(5)
+
+/* Set the g range */
+#define ADXL345_DATA_FORMAT_RANGE	GENMASK(1, 0)
+/* Data is left justified */
+#define ADXL345_DATA_FORMAT_JUSTIFY	BIT(2)
+/* Up to 13-bits resolution */
+#define ADXL345_DATA_FORMAT_FULL_RES	BIT(3)
+#define ADXL345_DATA_FORMAT_SELF_TEST	BIT(7)
+#define ADXL345_DATA_FORMAT_2G		0
+#define ADXL345_DATA_FORMAT_4G		1
+#define ADXL345_DATA_FORMAT_8G		2
+#define ADXL345_DATA_FORMAT_16G		3
+
+#define ADXL345_DEVID			0xE5
+#define ADXL345_FIFO_SIZE		32
+
 #define ADXL345_FIFO_BYPASS	0
 #define ADXL345_FIFO_FIFO	1
 #define ADXL345_FIFO_STREAM	2