[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <150ebd3af04045efe4d4ee7f298a57044e0f448e.camel@axis.com>
Date: Mon, 30 Sep 2024 07:53:59 +0000
From: Emil Gedenryd <Emil.Gedenryd@...s.com>
To: "jic23@...nel.org" <jic23@...nel.org>
CC: "linux-iio@...r.kernel.org" <linux-iio@...r.kernel.org>,
"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>, "robh@...nel.org"
<robh@...nel.org>, "linux-kernel@...r.kernel.org"
<linux-kernel@...r.kernel.org>, "dannenberg@...com" <dannenberg@...com>,
"krzk+dt@...nel.org" <krzk+dt@...nel.org>, "lars@...afoo.de"
<lars@...afoo.de>, "conor+dt@...nel.org" <conor+dt@...nel.org>, Kernel
<Kernel@...s.com>
Subject: Re: [PATCH v3 3/3] iio: light: opt3001: add support for TI's opt3002
light sensor
On Sat, 2024-09-28 at 17:05 +0100, Jonathan Cameron wrote:
> On Mon, 16 Sep 2024 16:56:39 +0200
> Emil Gedenryd <emil.gedenryd@...s.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@...s.com>
> Hi Emil,
>
> A few things inline,
>
> Thanks,
>
> Jonathan
Hi Jonathan,
Thank you for the comments, I'll start working on fixing them during
the week.
Best Regards,
Emil
>
> >
> > @@ -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.
Good to know! I'll keep it since it's minor but I'll remember this for
the future.
>
> > 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.
Okay!
> >
> > 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.
Good catch and thanks for the suggestion, I'll do it that way!
>
> > 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);
> >
>
Powered by blists - more mailing lists