Messages in this thread | | | Date | Fri, 16 Jun 2023 18:37:53 -0700 | Subject | Re: [PATCH 2/2] iio: Add driver for Murata IRS-D200 | From | 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@axis.com>
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]); > + } > + } > + > [...]
| |