Re: [PATCH v6 2/2] iio: pressure: add Sensirion SDP31 driver

[Marcelo Schmitt]

On 10/08, Muhammad Abu Bakar wrote:
> Add an IIO driver for the Sensirion SDP3x family of differential pressure
> sensors (SDP31, SDP32 and SDP33). The devices are accessed over I2C and
> report differential pressure and temperature. Each measurement is
> validated using the sensor's CRC-8 checksum. The scale factor is read
> from the sensor at runtime, so all three parts are supported.
> 
> Tested on an SDP31 connected to a Raspberry Pi 4 I2C bus.
> 
> Assisted-by: LLM
> Reviewed-by: Andy Shevchenko <andriy.shevchenko@xxxxxxxxx>
> Signed-off-by: Muhammad Abu Bakar <m.abubakar365@xxxxxxxxx>
> ---
...
> +static int sdp31_probe(struct i2c_client *client)
> +{
> +	struct device *dev = &client->dev;
> +	struct iio_dev *indio_dev;
> +	struct sdp31_data *data;
> +	int ret;
> +
> +	ret = devm_regulator_get_enable(dev, "vdd");
> +	if (ret)
> +		return dev_err_probe(dev, ret, "failed to enable regulator\n");
> +
> +	msleep(SDP31_POWERUP_TIME_MS);
> +
> +	indio_dev = devm_iio_device_alloc(dev, sizeof(*data));
> +	if (!indio_dev)
> +		return -ENOMEM;
> +
> +	data = iio_priv(indio_dev);
> +	data->client = client;
> +
> +	ret = devm_mutex_init(dev, &data->lock);
> +	if (ret)
> +		return ret;
> +
> +	DO_ONCE(crc8_populate_msb, sdp31_crc8_table, SDP31_CRC8_POLY);
> +
> +	ret = sdp31_read_scale(data, &data->dp_scale);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "failed to read from sensor\n");
> +	if (!data->dp_scale)
> +		return dev_err_probe(dev, -EINVAL, "invalid scale factor\n");
> +
> +	indio_dev->name = "sdp31";
Hmm, I'm not sure whether it is a big deal or not but, the hardcoded name might
cause confusion if the part connected is sdp32 or sdp33. That's because the
device name will be sdp31, but the scale will be SDP32 scale for differential
pressure (or SDP33 scale if one of those would be connected). The full
disambiguation solution would probably be to read the product identifier bits
and match those against a table of supported devices. It could have some sort of
chip_info struct to hold the device name and maybe the pressure scale as well.
That will probably require some boilerplate code that might not have been
necessary otherwise.

The other parts of the device driver look good to me. So, if the naming thing is
not needed,

Reviewed-by: Marcelo Schmitt <marcelo.schmitt1@xxxxxxxxx>

> +	indio_dev->info = &sdp31_info;
> +	indio_dev->modes = INDIO_DIRECT_MODE;
> +	indio_dev->channels = sdp31_channels;
> +	indio_dev->num_channels = ARRAY_SIZE(sdp31_channels);
> +
> +	return devm_iio_device_register(dev, indio_dev);
> +}
> +
> +static const struct i2c_device_id sdp31_id[] = {
> +	{ .name = "sdp31" },
> +	{ }
> +};