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

[Jonathan Cameron <jic23@kernel.org>]

On Mon, 16 Sep 2024 16:56:39 +0200
Emil Gedenryd <emil.gedenryd@axis.com> 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
> Signed-off-by: Emil Gedenryd <emil.gedenryd@axis.com>
Hi Emil,

A few things inline,

Thanks,

Jonathan

> diff --git a/drivers/iio/light/opt3001.c b/drivers/iio/light/opt3001.c
> index 176e54bb48c3..5e3fe42c5b59 100644
> --- a/drivers/iio/light/opt3001.c
> +++ b/drivers/iio/light/opt3001.c


> @@ -479,6 +565,9 @@ static int opt3001_write_event_value(struct iio_dev *iio,
>  {
>  	struct opt3001 *opt = iio_priv(iio);
>  	int ret;
> +	int whole;
> +	int integer;
> +	int decimal;
>  
>  	u16 mantissa;
>  	u16 value;
> @@ -497,7 +586,12 @@ static int opt3001_write_event_value(struct iio_dev *iio,
>  		goto err;
>  	}
>  
> -	mantissa = (((val * 1000) + (val2 / 1000)) / 10) >> exponent;
> +	whole = opt->chip_info->factor_whole;
> +	integer = opt->chip_info->factor_integer;
> +	decimal = opt->chip_info->factor_decimal;
> +
> +	mantissa = (((val * integer) + (val2 / decimal)) / whole) >> exponent;
> +
>  	value = (exponent << 12) | mantissa;
>  
>  	switch (dir) {
> @@ -610,7 +704,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);

Unrelated change so in theory should be in a separate patch but
meh, it's trivial so leave it here if you like.

>  		return ret;
>  	}

> @@ -755,22 +850,25 @@ 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);

Use the i2c specific way to to do this.
https://elixir.bootlin.com/linux/v6.11/source/drivers/i2c/i2c-core-base.c#L120
i2c_get_match_data() because it will also handle falling back to matching
via the i2c_match_id() path against the old style match tables in a few
cases.

>  
>  	mutex_init(&opt->lock);
>  	init_waitqueue_head(&opt->result_ready_queue);
>  	i2c_set_clientdata(client, iio);
>  
> -	ret = opt3001_read_id(opt);
> -	if (ret)
> -		return ret;
> +	if (opt->chip_info->has_id) {
> +		ret = opt3001_read_id(opt);
> +		if (ret)
> +			return ret;
> +	}
>  
>  	ret = opt3001_configure(opt);
>  	if (ret)
>  		return ret;
>  
>  	iio->name = client->name;
> -	iio->channels = opt3001_channels;
> -	iio->num_channels = ARRAY_SIZE(opt3001_channels);
> +	iio->channels = *opt->chip_info->channels;
> +	iio->num_channels = ARRAY_SIZE(*opt->chip_info->channels);
This won't work as it has no way to perform a sizeof
through a pointer.

Add a num_channels filed to your opt3001_chip_info structure
as then it can be ARRAY_SIZE(&opt3001_channels) which can work.

>  	iio->modes = INDIO_DIRECT_MODE;
>  	iio->info = &opt3001_info;
>  
> @@ -825,14 +923,36 @@ static void opt3001_remove(struct i2c_client *client)
>  	}
>  }
>  
> +static const struct opt3001_chip_info opt3001_chip_information = {
> +	.channels = &opt3001_channels,
> +	.chan_type = IIO_LIGHT,
> +	.scales = &opt3001_scales,
> +	.factor_whole = 10,
> +	.factor_integer = 1000,
> +	.factor_decimal = 1000,
> +	.has_id = true,
> +};
> +
> +static const struct opt3001_chip_info opt3002_chip_information = {
> +	.channels = &opt3002_channels,
> +	.chan_type = IIO_INTENSITY,
> +	.scales = &opt3002_scales,
> +	.factor_whole = 12,
> +	.factor_integer = 10,
> +	.factor_decimal = 100000,
> +	.has_id = false,
> +};
> +
>  static const struct i2c_device_id opt3001_id[] = {
> -	{ "opt3001" },
> +	{ "opt3001", (kernel_ulong_t)&opt3001_chip_information },
> +	{ "opt3002", (kernel_ulong_t)&opt3002_chip_information },
>  	{ } /* Terminating Entry */
>  };
>  MODULE_DEVICE_TABLE(i2c, opt3001_id);
>  
>  static const struct of_device_id opt3001_of_match[] = {
> -	{ .compatible = "ti,opt3001" },
> +	{ .compatible = "ti,opt3001", .data = &opt3001_chip_information },
> +	{ .compatible = "ti,opt3002", .data = &opt3002_chip_information },
>  	{ }
>  };
>  MODULE_DEVICE_TABLE(of, opt3001_of_match);
>