Re: [PATCH v2 3/3] iio: light: opt3001: add support for TI's opt3002 light sensor

[Jonathan Cameron]

On Fri, 13 Sep 2024 11:57:04 +0200
Emil Gedenryd <emil.gedenryd@xxxxxxxx> wrote:

> TI's opt3002 light sensor shares most properties with the opt3001
> model, with the exception of supporting a wider spectrum range.
> 
> Add support for TI's opt3002 by extending the TI opt3001 driver.
> 
> Datasheet: https://www.ti.com/product/OPT3002
> 
No blank line here. Datasheet: should be part of this tags block.
> Signed-off-by: Emil Gedenryd <emil.gedenryd@xxxxxxxx>
> ---
>  drivers/iio/light/Kconfig   |   2 +-
>  drivers/iio/light/opt3001.c | 169 +++++++++++++++++++++++++++++++++++---------
>  2 files changed, 138 insertions(+), 33 deletions(-)
> 
> diff --git a/drivers/iio/light/Kconfig b/drivers/iio/light/Kconfig
> index b68dcc1fbaca..c35bf962dae6 100644
> --- a/drivers/iio/light/Kconfig
> +++ b/drivers/iio/light/Kconfig
> @@ -461,7 +461,7 @@ config OPT3001
>  	depends on I2C
>  	help
>  	  If you say Y or M here, you get support for Texas Instruments
> -	  OPT3001 Ambient Light Sensor.
> +	  OPT3001 Ambient Light Sensor, OPT3002 Light-to-Digital Sensor.
>  
>  	  If built as a dynamically linked module, it will be called
>  	  opt3001.
> diff --git a/drivers/iio/light/opt3001.c b/drivers/iio/light/opt3001.c
> index 176e54bb48c3..83c49f4517b7 100644
> --- a/drivers/iio/light/opt3001.c
> +++ b/drivers/iio/light/opt3001.c
> @@ -70,6 +70,21 @@
>  #define OPT3001_RESULT_READY_SHORT	150
>  #define OPT3001_RESULT_READY_LONG	1000
>  
> +struct opt3001_scale {
> +	int	val;
> +	int	val2;
> +};
> +
> +struct opt300x_chip_info {

Don't use wild cards.  Just call it opt3001_chip_info.
Wild cards tend to bite us as manufacturers have an 'amusing' habit
of filling in gaps with unrelated devices.


> +	const struct iio_chan_spec (*channels)[2];
> +	enum iio_chan_type chan_type;
> +	const struct opt3001_scale (*scales)[12];
> +	int factor_whole;
> +	int factor_integer;
> +	int factor_decimal;

These three aren't obvious when just looking to fill in this
structure. Add some docs to hint at what they are.

> +	bool has_id;
> +};

> @@ -610,7 +690,7 @@ static int opt3001_read_id(struct opt3001 *opt)
>  	ret = i2c_smbus_read_word_swapped(opt->client, OPT3001_DEVICE_ID);
>  	if (ret < 0) {
>  		dev_err(opt->dev, "failed to read register %02x\n",
> -				OPT3001_DEVICE_ID);
> +			OPT3001_DEVICE_ID);

In general whitespace only changes belong in their own patch, but I guess
this one is pretty minor so we can skip that requirement this time.

>  		return ret;
>  	}

> @@ -746,7 +827,7 @@ static int opt3001_probe(struct i2c_client *client)
>  	struct iio_dev *iio;
>  	struct opt3001 *opt;
>  	int irq = client->irq;
> -	int ret;
> +	int ret = 0;
>  
>  	iio = devm_iio_device_alloc(dev, sizeof(*opt));
>  	if (!iio)
> @@ -755,12 +836,14 @@ static int opt3001_probe(struct i2c_client *client)
>  	opt = iio_priv(iio);
>  	opt->client = client;
>  	opt->dev = dev;
> +	opt->chip_info = device_get_match_data(&client->dev);
>  
>  	mutex_init(&opt->lock);
>  	init_waitqueue_head(&opt->result_ready_queue);
>  	i2c_set_clientdata(client, iio);
>  
> -	ret = opt3001_read_id(opt);
> +	if (opt->chip_info->has_id)
> +		ret = opt3001_read_id(opt);
>  	if (ret)
>  		return ret;
>  
Only check the ret if the function ran.  That way no need for the
dance with ret = 0 above.


> +static const struct iio_chan_spec opt3002_channels[] = {
> +	{
> +		.type = IIO_INTENSITY,
> +		.info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED) |
> +				BIT(IIO_CHAN_INFO_INT_TIME),

Generally intensity channels can't be processed because there are no
defined units as what you measure depends entirely on the frequency
sensitivity. There are some defined measurements such as illuminance
that use a specific sensivitiy curve, but if it's just intensity we
normally stick to _RAW..

> +		.event_spec = opt3001_event_spec,
> +		.num_event_specs = ARRAY_SIZE(opt3001_event_spec),
> +	},
> +	IIO_CHAN_SOFT_TIMESTAMP(1),
> +};
> +