Date:   Tue, 24 Jan 2023 12:22:34 +0200
From:   Andy Shevchenko <andriy.shevchenko@linux.intel.com>
To:     Markuss Broks <markuss.broks@gmail.com>
Cc:     linux-kernel@vger.kernel.org, Jonathan Cameron <jic23@kernel.org>,
        Lars-Peter Clausen <lars@metafoo.de>,
        Rob Herring <robh+dt@kernel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
        Zhigang Shi <Zhigang.Shi@liteon.com>,
        Dmitry Osipenko <dmitry.osipenko@collabora.com>,
        Paul Gazzillo <paul@pgazz.com>,
        Shreeya Patel <shreeya.patel@collabora.com>,
        linux-iio@vger.kernel.org, devicetree@vger.kernel.org
Subject: Re: [PATCH 2/2] iio: light: Add support for AMS TCS3490 light sensor
Message-ID: <Y8+xamtH/U4vK75e@smile.fi.intel.com>
References: <20230123231028.26073-1-markuss.broks@gmail.com>
 <20230123231028.26073-3-markuss.broks@gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20230123231028.26073-3-markuss.broks@gmail.com>
Organization: Intel Finland Oy - BIC 0357606-4 - Westendinkatu 7, 02160 Espoo
Precedence: bulk

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?

> +};

...

> +	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);

-- 
With Best Regards,
Andy Shevchenko