Message-ID: <a782a0d9-c47e-4328-774f-6bef78161d81@gmail.com>
Date:   Thu, 26 Jan 2023 17:54:56 +0200
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.6.1
Subject: Re: [PATCH 2/2] iio: light: Add support for AMS TCS3490 light sensor
Content-Language: en-US
To:     Andy Shevchenko <andriy.shevchenko@linux.intel.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
References: <20230123231028.26073-1-markuss.broks@gmail.com>
 <20230123231028.26073-3-markuss.broks@gmail.com>
 <Y8+xamtH/U4vK75e@smile.fi.intel.com>
From:   Markuss Broks <markuss.broks@gmail.com>
In-Reply-To: <Y8+xamtH/U4vK75e@smile.fi.intel.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

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