lkml.org 
[lkml]   [2023]   [Jan]   [26]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
SubjectRe: [PATCH 2/2] iio: light: Add support for AMS TCS3490 light sensor
From
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

\
 
 \ /
  Last update: 2023-03-26 23:57    [W:0.439 / U:0.104 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site