Re: [PATCH 2/2] iio: Add driver for Murata IRS-D200

[Lars-Peter Clausen]

On 6/16/23 08:10, Waqar Hameed wrote:
> Murata IRS-D200 is a PIR sensor for human detection. It has support for
> raw data measurements and detection event notification.
>
> Add a driver with support for triggered buffer and events. Map the
> various settings to the `iio` framework, e.g. threshold values, sampling
> frequency, filter frequencies etc.
>
> Signed-off-by: Waqar Hameed <waqar.hameed@xxxxxxxx>

Looks very good, small minor comments.

> [...]
> diff --git a/drivers/iio/proximity/Makefile b/drivers/iio/proximity/Makefile
> index cc838bb5408a..f36598380446 100644
> --- a/drivers/iio/proximity/Makefile
> +++ b/drivers/iio/proximity/Makefile
> @@ -6,6 +6,7 @@
>   # When adding new entries keep the list in alphabetical order
>   obj-$(CONFIG_AS3935)		+= as3935.o
>   obj-$(CONFIG_CROS_EC_MKBP_PROXIMITY) += cros_ec_mkbp_proximity.o
> +obj-$(CONFIG_IRSD200)		+= irsd200.o
>   obj-$(CONFIG_ISL29501)		+= isl29501.o
>   obj-$(CONFIG_LIDAR_LITE_V2)	+= pulsedlight-lidar-lite-v2.o
>   obj-$(CONFIG_MB1232)		+= mb1232.o
> diff --git a/drivers/iio/proximity/irsd200.c b/drivers/iio/proximity/irsd200.c
> new file mode 100644
> index 000000000000..699801d60295
> --- /dev/null
> +++ b/drivers/iio/proximity/irsd200.c
> @@ -0,0 +1,1051 @@
> [...]
> +/*
> + * The upper 4 bits in register IRS_REG_COUNT value is the upper count value
> + * (exceeding upper threshold value). The lower 4 is the lower count value
> + * (exceeding lower threshold value).
> + */
> +#define IRS_UPPER_COUNT(count)	(count >> 4)
> +#define IRS_LOWER_COUNT(count)	(count & GENMASK(3, 0))

Usually we add parenthesis around macro arguments to avoid issues in 
case the argument is a non-singular expression.

> [...]
>
> +static int irsd200_read_data(struct irsd200_data *data, s16 *val)
> +{
> +	unsigned int tmpval;
> +	int ret;
> +
> +	ret = regmap_read(data->regmap, IRS_REG_DATA_HI, &tmpval);
> +	if (ret < 0) {
> +		dev_err(data->dev, "Could not read hi data (%d)\n", ret);
> +		return ret;
> +	}
> +
> +	*val = (s16)(tmpval << 8);
> +
> +	ret = regmap_read(data->regmap, IRS_REG_DATA_LO, &tmpval);
> +	if (ret < 0) {
> +		dev_err(data->dev, "Could not read lo data (%d)\n", ret);
> +		return ret;
> +	}
Is there a way to bulk read those registers in one go to avoid 
inconsistent data if they change while being read?
> +	*val |= tmpval;
> +
> +	return 0;
> +}
> [...]
> +static int irsd200_write_raw(struct iio_dev *indio_dev,
> +			     struct iio_chan_spec const *chan, int val,
> +			     int val2, long mask)
> +{
> +	struct irsd200_data *data = iio_priv(indio_dev);
> +	int ret;
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_SAMP_FREQ:
> +		ret = irsd200_write_data_rate(data, val);
> +		return ret;
Maybe just `return irsd200_write_data_rate(...)`
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
>
> [...]
> +static int irsd200_probe(struct i2c_client *client)
> +{
> +	struct iio_trigger *trigger;
> +	struct irsd200_data *data;
> +	struct iio_dev *indio_dev;
> +	struct regmap *regmap;
> +	size_t i;
> +	int ret;
> +
> +	regmap = devm_regmap_init_i2c(client, &irsd200_regmap_config);
> +	if (IS_ERR(regmap)) {
> +		dev_err(&client->dev, "Could not initialize regmap\n");
dev_err_probe() is the more modern variant for error reporting in the 
probe function. Same for all the other dev_err() in this function.
> +		return PTR_ERR(regmap);
> +	}
> +
> +	indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*data));
> +	if (!indio_dev) {
> +		dev_err(&client->dev, "Could not allocate iio device\n");
> +		return -ENOMEM;
> +	}
> +
> +	data = iio_priv(indio_dev);
> +	data->regmap = regmap;
> +	data->dev = &client->dev;
> +	i2c_set_clientdata(client, indio_dev);
> +
> +	for (i = 0; i < IRS_REGF_MAX; ++i) {
> +		data->regfields[i] = devm_regmap_field_alloc(
> +			data->dev, data->regmap, irsd200_regfields[i]);
> +		if (IS_ERR(data->regfields[i])) {
> +			dev_err(data->dev,
> +				"Could not allocate register field %zu\n", i);
> +			return PTR_ERR(data->regfields[i]);
> +		}
> +	}
> +
> [...]