Re: [PATCH v2 09/22] iio: accel: adxl345: unexpose private defines

[Jonathan Cameron <jic23@kernel.org>]

On Sun, 17 Nov 2024 18:26:38 +0000
Lothar Rubusch <l.rubusch@gmail.com> wrote:

> For the implementation of features like FIFO-usage, watermark, single
> tap, double tap, freefall, etc. most of the constants do not need to be
> exposed in the header file, but are rather of private nature. Reduce
> namespace pollution by moving them to the core source file.
Whilst I get where you are coming from with this, breaking up
where these are between some in the header and some in the main code
tends to hurt readability and ease of checking the definitions.

So I would prefer these remain in the header.

Jonathan

> diff --git a/drivers/iio/accel/adxl345_core.c b/drivers/iio/accel/adxl345_core.c
> index 51b229cc44..c8d9e1f9e0 100644
> --- a/drivers/iio/accel/adxl345_core.c
> +++ b/drivers/iio/accel/adxl345_core.c
> @@ -18,6 +18,114 @@
>  
>  #include "adxl345.h"

> +/* POWER_CTL bits */
Drop all the extra comments unless they add something not obvious
for the naming.
> +#define ADXL345_POWER_CTL_STANDBY	0x00
> +
> +/* NB:
> + * BIT(0), BIT(1) for explicit wakeup (not implemented)
> + * BIT(2) for explicit sleep (not implemented)
Define them and don't use them and the comment isn't needed.

> + */
> +#define ADXL345_POWER_CTL_MEASURE	BIT(3)
> +#define ADXL345_POWER_CTL_AUTO_SLEEP	BIT(4)
> +#define ADXL345_POWER_CTL_LINK	BIT(5)
> +
> +/* DATA_FORMAT bits */

The naming of the defines makes that clear. So the comment doesn't
add much.

> +#define ADXL345_DATA_FORMAT_RANGE	GENMASK(1, 0)	/* Set the g range */
> +#define ADXL345_DATA_FORMAT_JUSTIFY	BIT(2)	/* 1: left-justified (MSB) mode, 0: right-just'd */
If the comment is needed move it to the line above.
better yet, use a name that means the comment isn't needed.
ADXL345_DATA_FORMAT_LEFT_JUSTIFY
for example where a value of 0 means left and 1 doesn't (hence right)

You are just moving it though, so perhaps not worth improving.

> +#define ADXL345_DATA_FORMAT_FULL_RES	BIT(3)	/* Up to 13-bits resolution */
> +/* NB: BIT(6): 3-wire SPI mode (defined in header) */
This is the sort of comment that indicates the problem with splitting
things between header and here. I'd prefer to just keep it all in the header.

> +
> +#define ADXL345_DATA_FORMAT_SELF_TEST	BIT(7)	/* Enable a self test */
> +#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_REG_OFS_AXIS(index)	(ADXL345_REG_OFSX + (index))
> +
> +/* The ADXL345 include a 32 sample FIFO
Comment syntax 
/*
 * The ADXL345 includes a 32 sample FIFO.
> + *
> + * FIFO stores a maximum of 32 entries, which equates to a maximum of 33
> + * entries available at any given time because an additional entry is available
> + * at the output filter of the device.
> + * (see datasheet FIFO_STATUS description on "Entries Bits")
> + */
> +#define ADXL34x_FIFO_SIZE  33
> +
>  struct adxl34x_state {
>  	int irq;
>  	const struct adxl345_chip_info *info;