Re: [PATCH v3 2/3] iio: accel: add ADXL380 driver

[Jonathan Cameron <jic23@kernel.org>]

On Thu, 27 Jun 2024 13:25:18 +0300
Antoniu Miclaus <antoniu.miclaus@analog.com> wrote:

> The ADXL380/ADXL382 is a low noise density, low power, 3-axis
> accelerometer with selectable measurement ranges. The ADXL380 supports
> the +/-4 g, +/-8 g, and +/-16 g ranges, and the ADXL382 supports
> +/-15 g, +/-30 g and +/-60 g ranges.
> The ADXL380/ADXL382 offers industry leading noise, enabling precision
> applications with minimal calibration. The low noise, and low power
> ADXL380/ADXL382 enables accurate measurement in an environment with
> high vibration, heart sounds and audio.
> 
> In addition to its low power consumption, the ADXL380/ADXL382 has many
> features to enable true system level performance. These include a
> built-in micropower temperature sensor, single / double / triple tap
> detection and a state machine to prevent a false triggering. In
> addition, the ADXL380/ADXL382 has provisions for external control of
> the sampling time and/or an external clock.
> 
> Signed-off-by: Ramona Gradinariu <ramona.gradinariu@analog.com>
> Signed-off-by: Antoniu Miclaus <antoniu.miclaus@analog.com>
Hi Antoniu, Ramona,

Some trivial stuff that I'd just have cleaned up, but one thing I don't
understand which is which entries of your iio_priv() structure you've
marked with __aligned(IIO_DMA_MINALIGN).  That's beyond the level of thing
I'll tweak whilst applying.

I might be missing a path in which they are used for DMA transfers though!

Jonathan

> ---
> changes in v3:
>  - rearrange includes in alphabetical order.
>  - rework defines clearly stating which are registers.
>  - use BIT() and FIELD_GET() where possible.
>  - convert register related enums into macro definitions.
>  - add spacings after { and before } for arrays.
>  - reorder the `adxl380_state` structure members.
>  - mark structure members with  __aligned(IIO_DMA_MINALIGN) where required.
This change has me confused because I'm not sure why it is required
for the ones you've marked (you do need one such marking at least though!)

>  - drop redundant brackets.
>  - use min() function where it applies.
>  - use put_unaligned_be24() where it applies.
>  - wrap lines where indicated by the reviewers.
>  - assign directly instead of using get_unaligned_be16 where not necessary.
>  - rework error handling for irq_handler.
>  - add missing error check.
>  - drop  != IIO_ACCEL for IIO_CHAN_INFO_SCALE
>  - use MICRO where possible.
>  - return iio_format_value() directly.
>  - check for negative values aswell.
>  - reorder local declarations.
>  - improve dev_err_probe string description.
>  - drop _ from functions naming where possible.
>  - rework chip/part id handling.
>  - improve comment for the delay required after reset.
>  - add regulators implementation for the supplies.
>  - handle both irqs.
>  - use i2c_get_match_data.
>  - use spi_get_device_match_data.
>  - include mod_devicetable.h.

> diff --git a/drivers/iio/accel/adxl380.c b/drivers/iio/accel/adxl380.c
> new file mode 100644
> index 000000000000..b569265190e6
> --- /dev/null
> +++ b/drivers/iio/accel/adxl380.c
...


> +struct adxl380_state {

...

> +	int irq;
> +	int int_map[2];
> +	int lpf_tbl[4] __aligned(IIO_DMA_MINALIGN);
> +	int hpf_tbl[7][2] __aligned(IIO_DMA_MINALIGN);
Unless you allow the driver to write these two tables at the
time dma is going on with the other one you shoudn't need
to force them into separate cachelines.

I'm more curious though - why these two?
From a quick look I can't figure out where they are potentially
used for DMA?

> +
> +	__be16 fifo_buf[ADXL380_FIFO_SAMPLES];
This one is used for regmap_no_inc_read() so if you drop
the __aligned() markings above this will need one.

> +};


> +static int adxl380_set_odr(struct adxl380_state *st, u8 odr)
> +{
> +	int ret;
> +
> +	guard(mutex)(&st->lock);
> +
> +	ret = adxl380_set_measure_en(st, false);
> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_update_bits(st->regmap, ADXL380_TRIG_CFG_REG,
> +				 ADXL380_TRIG_CFG_DEC_2X_MSK,
> +				 FIELD_PREP(ADXL380_TRIG_CFG_DEC_2X_MSK, (odr & 1)));
> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_update_bits(st->regmap, ADXL380_TRIG_CFG_REG,
> +				 ADXL380_TRIG_CFG_SINC_RATE_MSK,
> +				 FIELD_PREP(ADXL380_TRIG_CFG_SINC_RATE_MSK, (odr >> 1)));
Brackets around (odr >> 1) don't add anythign and make long lines
even longer.



...

> +
> +static ssize_t adxl380_get_fifo_enabled(struct device *dev,
> +					struct device_attribute *attr,
> +					char *buf)
> +{
> +	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> +	struct adxl380_state *st = iio_priv(indio_dev);
> +	int ret;
> +	unsigned int reg_val;
> +
> +	ret = regmap_read(st->regmap, ADXL380_DIG_EN_REG, &reg_val);
> +	if (ret)
> +		return ret;
> +
> +	return sysfs_emit(buf, "%lu\n", FIELD_GET(ADXL380_FIFO_EN_MSK, reg_val));
Long line that should be wrapped.
> +}
> +
> +static IIO_DEVICE_ATTR_RO(hwfifo_watermark_min, 0);
> +static IIO_DEVICE_ATTR_RO(hwfifo_watermark_max, 0);
> +static IIO_DEVICE_ATTR(hwfifo_watermark, 0444,
> +		       adxl380_get_fifo_watermark, NULL, 0);
> +static IIO_DEVICE_ATTR(hwfifo_enabled, 0444,
> +		       adxl380_get_fifo_enabled, NULL, 0);
> +
> +static const struct iio_dev_attr *adxl380_fifo_attributes[] = {
> +	&iio_dev_attr_hwfifo_watermark_min,
> +	&iio_dev_attr_hwfifo_watermark_max,
> +	&iio_dev_attr_hwfifo_watermark,
> +	&iio_dev_attr_hwfifo_enabled,
> +	NULL,

No comma on null terminators as we never add anything after them.

> +};

...

> +static int adxl380_config_irq(struct iio_dev *indio_dev)
> +{
> +	struct adxl380_state *st = iio_priv(indio_dev);
> +	unsigned long irq_flag;
> +	struct irq_data *desc;
> +	u32 irq_type;
> +	u8 polarity;
> +	int ret;
> +
> +	desc = irq_get_irq_data(st->irq);
> +	if (!desc)
> +		return dev_err_probe(st->dev, -EINVAL, "Could not find IRQ %d\n", st->irq);
> +
> +	irq_type = irqd_get_trigger_type(desc);
> +	if (irq_type == IRQ_TYPE_LEVEL_HIGH) {
> +		polarity = 0;
> +		irq_flag = IRQF_TRIGGER_HIGH | IRQF_ONESHOT;
> +	} else if (irq_type == IRQ_TYPE_LEVEL_LOW) {
> +		polarity = 1;
> +		irq_flag = IRQF_TRIGGER_LOW | IRQF_ONESHOT;
> +	} else {
> +		return dev_err_probe(st->dev, -EINVAL,
> +				     "Invalid interrupt 0x%x. Only level interrupts supported\n",
> +			irq_type);

Odd indentation.  

> +	}
> +
> +	ret = regmap_update_bits(st->regmap, ADXL380_INT0_REG,
> +				 ADXL380_INT0_POL_MSK,
> +				 FIELD_PREP(ADXL380_INT0_POL_MSK, polarity));
> +	if (ret)
> +		return ret;
> +
> +	return devm_request_threaded_irq(st->dev, st->irq, NULL,
> +					 adxl380_irq_handler, irq_flag,
> +					 indio_dev->name, indio_dev);
> +}
>
> +
> +int adxl380_probe(struct device *dev, struct regmap *regmap,
> +		  const struct adxl380_chip_info *chip_info)
> +{

...

> +	st->irq = fwnode_irq_get_byname(dev_fwnode(dev), "INT0");
> +	if (st->irq  > 0) {

one space only before >

> +		st->int_map[0] = ADXL380_INT0_MAP0_REG;
> +		st->int_map[1] = ADXL380_INT0_MAP1_REG;
> +	} else {
> +		st->irq = fwnode_irq_get_byname(dev_fwnode(dev), "INT1");
> +		if (st->irq  > 0)

one space only before >

> +			return dev_err_probe(dev, -ENODEV,
> +					     "no interrupt name specified");
> +		st->int_map[0] = ADXL380_INT1_MAP0_REG;
> +		st->int_map[1] = ADXL380_INT1_MAP1_REG;
> +	}
> +
> +	ret = adxl380_setup(indio_dev);
> +	if (ret)
> +		return ret;
> +
> +	ret = devm_iio_kfifo_buffer_setup_ext(st->dev, indio_dev,
> +					      &adxl380_buffer_ops,
> +					      adxl380_fifo_attributes);
> +	if (ret)
> +		return ret;
> +
> +	return devm_iio_device_register(dev, indio_dev);
> +}



> diff --git a/drivers/iio/accel/adxl380_i2c.c b/drivers/iio/accel/adxl380_i2c.c
> new file mode 100644
> index 000000000000..dad3b39a5125
> --- /dev/null
> +++ b/drivers/iio/accel/adxl380_i2c.c
> @@ -0,0 +1,64 @@

...

> +static const struct i2c_device_id adxl380_i2c_id[] = {
> +	{ "adxl380", (kernel_ulong_t)&adxl380_chip_info},
> +	{ "adxl382", (kernel_ulong_t)&adxl382_chip_info},

space before }

> +	{}
As below - be consistent. 
> +};
> +MODULE_DEVICE_TABLE(i2c, adxl380_i2c_id);
> +
> +static const struct of_device_id adxl380_of_match[] = {
> +	{ .compatible = "adi,adxl380", .data = &adxl380_chip_info},
> +	{ .compatible = "adi,adxl382", .data = &adxl382_chip_info},
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(of, adxl380_of_match);

> diff --git a/drivers/iio/accel/adxl380_spi.c b/drivers/iio/accel/adxl380_spi.c
> new file mode 100644
> index 000000000000..c244ae328714
> --- /dev/null
> +++ b/drivers/iio/accel/adxl380_spi.c
> @@ -0,0 +1,66 @@

> +
> +static const struct spi_device_id adxl380_spi_id[] = {
> +	{ "adxl380", (kernel_ulong_t)&adxl380_chip_info },
> +	{ "adxl382", (kernel_ulong_t)&adxl382_chip_info },
> +	{}

Spacing should be consistent.  I'd have { } as below.

> +};
> +MODULE_DEVICE_TABLE(spi, adxl380_spi_id);
> +
> +static const struct of_device_id adxl380_of_match[] = {
> +	{ .compatible = "adi,adxl380", .data = &adxl380_chip_info },
> +	{ .compatible = "adi,adxl382", .data = &adxl382_chip_info },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(of, adxl380_of_match);