On Fri, Jun 16, 2023 at 18:37 -0700 Lars-Peter Clausen <lars@xxxxxxxxxx> wrote:
> 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.
Thanks!
[...]
>> 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.
Of course! Will use `FIELD_GET()` as Jonathan suggests.
>> [...]
>>
>> +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?
Yes, will use `regmap_bulk_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(...)`
Of course! (Remnant from a refactorization... Sorry!)
>> + 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.
Alright, I'll change to `dev_err_probe()`.
