Re: [PATCH v2 10/14] iio: accel: adxl345: add g-range configuration

[Jonathan Cameron]

On Mon, 10 Feb 2025 11:01:15 +0000
Lothar Rubusch <l.rubusch@xxxxxxxxx> wrote:

> Introduce means to configure and work with the available g-ranges
> keeping the precision of 13 digits.
> 
> This is in preparation for the activity/inactivity feature.
> 
> Signed-off-by: Lothar Rubusch <l.rubusch@xxxxxxxxx>
A few really trivial comments on this one.

> ---
>  drivers/iio/accel/adxl345_core.c | 92 ++++++++++++++++++++++++++++++--
>  1 file changed, 89 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/iio/accel/adxl345_core.c b/drivers/iio/accel/adxl345_core.c
> index 08ad71875c5e..ea7bfe193d31 100644
> --- a/drivers/iio/accel/adxl345_core.c
> +++ b/drivers/iio/accel/adxl345_core.c
> @@ -86,6 +86,13 @@ enum adxl345_odr {
>  	ADXL345_ODR_3200HZ,
>  };
>  
> +enum adxl345_range {
> +	ADXL345_2G_RANGE = 0,
> +	ADXL345_4G_RANGE,
> +	ADXL345_8G_RANGE,
> +	ADXL345_16G_RANGE,
> +};
> +
>  /* Certain features recommend 12.5 Hz - 400 Hz ODR */
>  static const int adxl345_odr_tbl[][2] = {
>  	[ADXL345_ODR_0P10HZ]	= {   0,  97000},
> @@ -106,6 +113,33 @@ static const int adxl345_odr_tbl[][2] = {
>  	[ADXL345_ODR_3200HZ]	= {3200, 0},
>  };
>  
> +/*
> + * Full resolution frequency table:
> + * (g * 2 * 9.80665) / (2^(resolution) - 1)
> + *
> + * resolution := 13 (full)
> + * g := 2|4|8|16
> + *
> + *  2g at 13bit: 0.004789
> + *  4g at 13bit: 0.009578
> + *  8g at 13bit: 0.019156
> + * 16g at 16bit: 0.038312
> + */
> +static const int adxl345_fullres_range_tbl[][2] = {
> +	[ADXL345_2G_RANGE]  = {0, 4789},

I'm slowly trying to standardize a few more corners of the kernel style for IIO at least.
This hits one of them. So if you don't mind adding spaces after { and before }
it will one day save me time on a cleanup series.

> +	[ADXL345_4G_RANGE]  = {0, 9578},
> +	[ADXL345_8G_RANGE]  = {0, 19156},
> +	[ADXL345_16G_RANGE] = {0, 38312},
> +};


> @@ -1252,6 +1335,9 @@ int adxl345_core_probe(struct device *dev, struct regmap *regmap,
>  	 * undesired behavior.
>  	 */
>  	ret = adxl345_set_odr(st, ADXL345_ODR_200HZ);
> +	if (ret)
> +		return ret;

Trivial but I'd put a blank line here for slightly improved readability.

> +	ret = adxl345_set_range(st, ADXL345_16G_RANGE);
>  	if (ret)
>  		return ret;
>