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

[Waqar Hameed]

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()`.