Messages in this thread | | | Date | Thu, 26 Jan 2023 17:54:56 +0200 | Subject | Re: [PATCH 2/2] iio: light: Add support for AMS TCS3490 light sensor | From | Markuss Broks <> |
| |
Hi Andy,
On 1/24/23 12:22, Andy Shevchenko wrote: > On Tue, Jan 24, 2023 at 01:10:25AM +0200, Markuss Broks wrote: >> Add a driver for AMS TCS3490 Color Light-to-Digital Converter. This >> device provides color and IR (red, green, blue, clear and IR) light >> sensing. The color sensing can be used for light source detection and >> color temperature measurements. > ... > >> +AMS TCS3490 DRIVER >> +M: Markuss Broks <markuss.broks@gmail.com> >> +L: linux-iio@vger.kernel.org >> +S: Maintained >> +F: Documentation/devicetree/bindings/iio/light/ams,tcs3490.yaml > Shouldn't actually be added with the schema patch? > >> +F: drivers/iio/light/tcs3490.c > I dunno what's the rules but it feels a bit inconsistent in case the schema > will leave while driver got, for example, rewritten (as brand new) and reverted > (as old one). In such (quite unlikely) circumstances we may end up with the > dangling file. > > Rob, Krzysztof, Jonathan, what is yours take from this? > > ... > >> +config TCS3490 >> + tristate "AMS TCS3490 color light-to-digital converter" >> + depends on I2C > Hmm... Where is the select REGMAP_I2C? > >> + select IIO_BUFFER >> + select IIO_TRIGGERED_BUFFER >> + help >> + Say Y here if you have an AMS TCS3490 color light-to digital converter >> + which provides RGB color and IR light sensing. >> + >> + This driver can also be built as a module. If so, the module >> + will be called tcs3490. > ... > >> +struct tcs3490 { >> + struct i2c_client *client; > Why do you need this? > >> + struct regmap *regmap; >> + struct regulator *vdd_supply; >> +}; > ... > >> +static const struct regmap_config tcs3490_regmap_config = { >> + .reg_bits = 8, >> + .val_bits = 8, > Seems you are using regmap internal serialization, but does it guarantee the > serialization on the transaction level? Or why is it not a problem? Well, other drivers seem to have it this way too. I don't really understand why it should be a problem, could you please clarify? > >> +}; > ... > >> + do { >> + usleep_range(3000, 4000); >> + >> + ret = regmap_read(data->regmap, TCS3490_STATUS, &status); >> + if (ret) >> + return ret; >> + if (status & TCS3490_STATUS_RGBC_VALID) >> + break; >> + } while (--tries); >> + >> + if (!tries) >> + return -ETIMEDOUT; > regmap_read_poll_timeout()? > > ... > >> + ret = regmap_read(data->regmap, chan->address, &val_l); >> + if (ret) >> + return ret; >> + >> + ret = regmap_read(data->regmap, chan->address + 1, &val_h); >> + if (ret) >> + return ret; > Why not a bulk read into __le16 val? > >> + *val = (val_h << 8) | val_l; > Use le16_to_cpu(). > >> + ret = regmap_write(data->regmap, TCS3490_ENABLE, TCS3490_SUSPEND); >> + if (ret) >> + return ret; >> + >> + return 0; > Can be simply > > return regmap_write(...); > >> +} > ... > >> +static int tcs3490_read_raw(struct iio_dev *indio_dev, >> + const struct iio_chan_spec *chan, >> + int *val, int *val2, long mask) >> +{ >> + struct tcs3490 *data = iio_priv(indio_dev); >> + int ret; >> + >> + switch (mask) { >> + case IIO_CHAN_INFO_RAW: >> + ret = tcs3490_read_channel(data, chan, val); >> + if (ret < 0) >> + return ret; >> + ret = IIO_VAL_INT; >> + break; > return directly. > >> + case IIO_CHAN_INFO_CALIBSCALE: >> + ret = tcs3490_get_gain(data, val); > Missing error check. > >> + ret = IIO_VAL_INT; >> + break; > Return directly. > >> + default: >> + ret = -EINVAL; >> + break; > Ditto. > >> + } >> + if (ret < 0) >> + return ret; >> + return IIO_VAL_INT; > Redundant, see above. > >> +} > ... > >> +static const struct of_device_id tcs3490_of_match[] = { >> + { .compatible = "ams,tcs3490", }, > Inner comma is not needed. > >> + { }, > Terminator entries should go without a comma. > >> +}; > ... > >> +static struct i2c_driver tcs3490_driver = { >> + .driver = { >> + .name = "tcs3490", >> + .of_match_table = of_match_ptr(tcs3490_of_match), > Kill of_match_ptr(). Its usage is wrong in 99% of the cases. > >> + }, >> + .probe_new = tcs3490_probe, >> +}; >> + > Redundant blank line. > >> +module_i2c_driver(tcs3490_driver); - Markuss
| |