Re: [PATCH v4 2/2] iio: light: isl76682: Add ISL76682 driver

[Jonathan Cameron <jic23@kernel.org>]

On Tue, 21 Nov 2023 04:10:40 +0100
Marek Vasut <marex@denx.de> wrote:

> The ISL76682 is very basic ALS which only supports ALS or IR mode
> in four ranges, 1k/4k/16k/64k LUX. There is no IRQ support or any
> other fancy functionality.
> 
> Signed-off-by: Marek Vasut <marex@denx.de>
Nice little driver.

A few comments inline.

Thanks,

Jonathan

> diff --git a/drivers/iio/light/isl76682.c b/drivers/iio/light/isl76682.c
> new file mode 100644
> index 0000000000000..7f0ccd0d37539
> --- /dev/null
> +++ b/drivers/iio/light/isl76682.c
> @@ -0,0 +1,364 @@


> +
> +static int isl76682_read_raw(struct iio_dev *indio_dev,
> +			     struct iio_chan_spec const *chan,
> +			     int *val, int *val2, long mask)
> +{
> +	struct isl76682_chip *chip = iio_priv(indio_dev);
> +	int ret;
> +	int i;
> +
> +	if (chan->type != IIO_LIGHT && chan->type != IIO_INTENSITY)
> +		return -EINVAL;
> +
> +	guard(mutex)(&chip->lock);
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_RAW:
> +		switch (chan->type) {
> +		case IIO_LIGHT:
> +			ret = isl76682_get(chip, false, val);
> +			return (ret < 0) ? ret : IIO_VAL_INT;
> +		case IIO_INTENSITY:
> +			ret = isl76682_get(chip, true, val);
> +			return (ret < 0) ? ret : IIO_VAL_INT;
> +		default:
> +			break;
return here and drop the one below.

> +		}
> +
> +		return -EINVAL;
> +	case IIO_CHAN_INFO_SCALE:
> +		for (i = 0; i < ARRAY_SIZE(isl76682_range_table); i++) {
> +			if (chip->range != isl76682_range_table[i].range)
> +				continue;
> +
> +			*val = 0;
> +			switch (chan->type) {
> +			case IIO_LIGHT:
> +				*val2 = isl76682_range_table[i].als;
> +				return IIO_VAL_INT_PLUS_MICRO;
> +			case IIO_INTENSITY:
> +				*val2 = isl76682_range_table[i].ir;
> +				return IIO_VAL_INT_PLUS_MICRO;
> +			default:
> +				return -EINVAL;
> +			}
> +		}
> +		return -EINVAL;
> +	case IIO_CHAN_INFO_INT_TIME:
> +		*val = 0;
> +		*val2 = ISL76682_INT_TIME_US;
> +		return IIO_VAL_INT_PLUS_MICRO;
> +	default:
> +		return -EINVAL;
> +	}
> +}

> +static int isl76682_probe(struct i2c_client *client)
> +{
> +	const struct i2c_device_id *id = i2c_client_get_device_id(client);

For reason below, we should avoid accessing this directly.
If you need the associated data later, then use the access functions that
try the property.h accessors first and fallback to this only if this fails.
That ends up much less fragile.

> +	struct device *dev = &client->dev;
> +	struct isl76682_chip *chip;
> +	struct iio_dev *indio_dev;
> +	int ret;
> +
> +	indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*chip));
> +	if (!indio_dev)
> +		return -ENOMEM;
> +
> +	chip = iio_priv(indio_dev);
> +
> +	mutex_init(&chip->lock);
> +
> +	chip->regmap = devm_regmap_init_i2c(client, &isl76682_regmap_config);
> +	ret = PTR_ERR_OR_ZERO(chip->regmap);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Error initializing regmap\n");
> +
> +	chip->range = ISL76682_COMMAND_RANGE_LUX_1K;
> +
> +	ret = isl76682_clear_configure_reg(chip);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = devm_add_action_or_reset(dev, isl76682_reset_action, chip);
> +	if (ret)
> +		return ret;
> +
> +	indio_dev->info = &isl76682_info;
> +	indio_dev->channels = isl76682_channels;
> +	indio_dev->num_channels = ARRAY_SIZE(isl76682_channels);
> +	indio_dev->name = id->name;

This tends to be fragile as drivers gain more entrees in the id and of table.
So the name should always be retrieved from a chip_info structure.
Given you don't need one of those yet, I'd prefer just having the string here
directly. We've been bitten by this before so I get paranoid about it :)


> +	indio_dev->modes = INDIO_DIRECT_MODE;
> +
> +	return devm_iio_device_register(dev, indio_dev);
> +}