Date:   Thu, 12 Oct 2023 01:07:10 +1030
From:   Subhajit Ghosh <subhajit.ghosh@...aklogic.com>
To:     Jonathan Cameron <jic23@...nel.org>
Cc:     Lars-Peter Clausen <lars@...afoo.de>,
        Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
        Rob Herring <robh+dt@...nel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        Matti Vaittinen <mazziesaccount@...il.com>,
        linux-iio@...r.kernel.org, devicetree@...r.kernel.org,
        linux-kernel@...r.kernel.org, Paul Gazzillo <paul@...zz.com>,
        Conor Dooley <conor+dt@...nel.org>,
        Stefan Windfeldt-Prytz <stefan.windfeldt-prytz@...s.com>
Subject: Re: [PATCH 2/2] iio: light: Add support for APDS9306 Light Sensor

On 11/10/23 01:08, Jonathan Cameron wrote:
> 
> No need to wrap the patch description quite so short. Aim
> for up to 75 char for a commit message (and 80 for the code)
> Here you are under 60.
> 
Thank you for taking time to point out these small issues.

>>
>> Datasheet at https://docs.broadcom.com/doc/AV02-4755EN
>>
> There is a tag for datasheets in the format tags block so
> Datasheet: https://docs.broadcom.com/doc/AV02-4755EN
>> Signed-off-by: Subhajit Ghosh <subhajit.ghosh@...aklogic.com>
> 
> I took a quick look at the most similar part number adps9300 and
> this does look substantially different but could you confirm you've
> taken a look at the plausible drivers to which support for this part
> could be added and perhaps mention why that doesn't make sense
> I think it will be mainly feature set being different here, but also
> it seems they have completely different register maps despite similar
> part numbers!
I have taken a look at quiet a few light sensor drivers including
apds9960 and apds9300, as you said that they are different. There are
another two drivers apds990x and apds9802als in drivers/misc which are
also very different but I can't say that I have been through all the
driver files.

>> +
>> +enum apds9306_int_channels {
>> +	CLEAR,
>> +	ALS,
>> +};
> 
> Is this used?
>
Something left from the old code. It is not needed.
  

>> + * Nano Lux per count = (340.134 * 1000000000)/ (32 * 3 * 2000) for apds9306
>> + * Nano Lux per count = (293.69 * 1000000000)/ (32 * 3 * 2000) for apds9306-065
> 
> Even though it's a comment stick to kernel maths syntax and put a space before the /
> Otherwise some script will complain it's not correctly formatted code :)
Ok, understood.

> 
>> + */
>> +static struct part_id_nlux_per_count apds9306_part_id_nlux_per_count[] = {
>> +	{.part_id = 0xB1, .nlux_per_count = 1787156},
>> +	{.part_id = 0xB3, .nlux_per_count = 1529635},
> 
> Prefer { .part_id = 0xB3, .nlux_per_count = 1629635 },
> for tables liek this as it ends up a tiny bit easier to read.
Ok.
> 
>> +};
>> +static struct iio_chan_spec apds9306_channels_without_events[] = {
>> +	{
>> +		APDS9306_CHANNEL(IIO_LIGHT)
>> +		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
>> +			BIT(IIO_CHAN_INFO_PROCESSED),
> 
> This needs an explanation for why as a comment in the code.
> We very rarely support both raw and processed for the same channel and
> mostly when we do it is due to some historical changes.
> 
Thanks for pointing it out.
> You are using the gts stuff here so it should be possible to expose
> the controls for scale necessary to have userspace perform the raw to processed
> conversion.
Yes, processed = (raw + offset) * scale. No need to do calculations in kernel
space. Ok. I will reimplement it.

>> +
>> +/* INT_PERSISTENCE available */
>> +IIO_CONST_ATTR(thresh_either_period_available, "[0 15]");
>> +
>> +/* ALS_THRESH_VAR available */
>> +IIO_CONST_ATTR(thresh_adaptive_either_values_available, "[0 7]");
> Not valid range syntax for IIO attributes, you need to include
> the step.
> 
> [0 1 7]
Got it.
> 

>> +	.cache_type = REGCACHE_RBTREE,
>> +	.disable_locking = true,
> This normally deserves a statement of what you are doing about locking instead.

I'll put it in the next version.

> The interrupt controller for starters takes to no locks and can run concurrently
> with other accesses from other CPUs.  That seems unwise.
>
Well, regarding device access, interrupt handler just reads the status registers
thereby clearing the interrupt status flag and releasing the physical interrupt line.
What can be the issue if I don't use a lock?


>> +static int apds9306_intg_time_get(struct apds9306_data *data, int *val2)
>> +{
>> +	*val2 = iio_gts_find_int_time_by_sel(&data->gts, data->intg_time_idx);
>> +	if (*val2 < 0)
>> +		return *val2;
> 
> You shouldn't have side effects on *val2 if an error occurs.
> Its not a bug in this case, but it is generally something to avoid
> 
Ok.

> 
>> +
>> +static int apds9306_sampling_freq_set(struct apds9306_data *data, int val,
>> +				int val2)
>> +{
>> +	int i, ret = -EINVAL;
>> +
>> +	for (i = 0; i < ARRAY_SIZE(apds9306_repeat_rate_freq); i++)
>> +		if (apds9306_repeat_rate_freq[i][0] == val &&
>> +				apds9306_repeat_rate_freq[i][1] == val2) {
>> +			ret = regmap_field_write(data->regfield_repeat_rate, i);
>> +			if (ret)
>> +				return ret;
>> +			data->repeat_rate_idx = i;
>> +			break;
> Might as well return here instead of break as nothing else to do.
Ok.
> 
....
>> +		ret = IIO_VAL_INT;
>> +		*val2 = 0;
> 
> As below.  No need to set *val2 to 0 if returning IIO_VAL_INT.
Ok.

>> +
>> +	if (ret)
>> +		return ret;
>> +
>> +	*val2 = 0;
> 
> The IIO core won't use val2 if you return IIO_VAL_INT, so don't bother setting it.

Ok. Got it.
> 
>> +	return IIO_VAL_INT;
>> +}
>> +

>> +
>> +static int apds9306_read_event_config(struct iio_dev *indio_dev,
>> +				      const struct iio_chan_spec *chan,
>> +				      enum iio_event_type type,
>> +				      enum iio_event_direction dir)
>> +{
>> +	struct apds9306_data *data = iio_priv(indio_dev);
>> +	unsigned int val, val2;
>> +	int ret;
>> +
>> +	mutex_lock(&data->mutex);
> As below
> 	guard(mutex)(&data->mutex);
> 
> should simplify this - I won't comment on this one above this point (reviewing backwards
> through the code).
Ok.
> 
>> +	switch (type) {
>> +	case IIO_EV_TYPE_THRESH:
>> +		ret = regmap_field_read(data->regfield_int_en, &val);
>> +		if (ret)
>> +			break;
>> +		ret = regmap_field_read(data->regfield_int_src, &val2);
>> +		if (ret)
>> +			break;
>> +		if (chan->type == IIO_LIGHT)
>> +			ret = val & val2;
>> +		else if (chan->type == IIO_INTENSITY)
>> +			ret = val & !val2;
> 
> This logic would benefit from better variable naming.
> en and src for example..
Sure.
> 
>> +		else
>> +			ret = -EINVAL;
>> +		break;
>> +	case IIO_EV_TYPE_THRESH_ADAPTIVE:
>> +		ret = regmap_field_read(data->regfield_int_thresh_var_en,
>> +					&val);
>> +		if (ret)
>> +			break;
>> +		ret = val;
>> +		break;
>> +	default:
>> +		ret = -EINVAL;
>> +	}
>> +	mutex_unlock(&data->mutex);
>> +	return ret;
>> +}
>> +
>> +static int apds9306_write_event_config(struct iio_dev *indio_dev,
>> +				       const struct iio_chan_spec *chan,
>> +				       enum iio_event_type type,
>> +				       enum iio_event_direction dir,
>> +				       int state)
>> +{
>> +	struct apds9306_data *data = iio_priv(indio_dev);
>> +	int ret;
>> +
>> +	state = !!state;
>> +	mutex_lock(&data->mutex);
> 
> Perfect place to use the new cleanup.h trickery here.
:) Absolutely...
> 
> guard(mutex)(&data->mutex);
> 
> and then you can just return in error paths which will simplify this code
> 
Got your point.
>> +	switch (type) {
>> +	case IIO_EV_TYPE_THRESH:

>> +static int get_device_id_lux_per_count(struct apds9306_data *data)
>> +{
>> +	int ret, part_id;
>> +
>> +	ret = regmap_read(data->regmap, APDS9306_PART_ID, &part_id);
>> +	if (ret)
>> +		return ret;
>> +
>> +	if (part_id == apds9306_part_id_nlux_per_count[0].part_id)
>> +		data->nlux_per_count =
>> +			apds9306_part_id_nlux_per_count[0].nlux_per_count;
>> +	else if (part_id == apds9306_part_id_nlux_per_count[1].part_id)
>> +		data->nlux_per_count =
>> +			apds9306_part_id_nlux_per_count[1].nlux_per_count;
> 
> For loop over ARRAY_SIZE(apds9306_part_id_nlux_per_count)
> would be more extensible with a return on match, so that if you
> don't we just return -ENXIO on exit from the loop.
Yes. Got it.
> 
> 
>> +	else
>> +		return -ENXIO;
>> +
>> +	return 0;
>> +}
>> +
>> +static void apds9306_powerdown(void *ptr)
>> +{
>> +	struct apds9306_data *data = (struct apds9306_data *)ptr;
>> +	struct device *dev = data->dev;
>> +	int ret;
>> +
>> +	/* Disable interrupts */
>> +	ret = regmap_field_write(data->regfield_int_thresh_var_en, 0);
>> +	if (ret)
>> +		dev_err(dev, "Failed to disable variance interrupts\n");
> 
> Muddling on when things are failing is probably not worthwhile. I'd be
> tempted to just error out of here.  Worst that happens is we leave the
> device partly enabled which is a bit of a power waste, but it's not expected
> to happen so I don't think we care.  Much easier to follow code if we
> always return on error.
Ok, makes sense. I'll do that.
> 

>> +	if (client->irq) {
>> +		indio_dev->info = &apds9306_info;
>> +		indio_dev->channels = apds9306_channels_with_events;
>> +		indio_dev->num_channels =
>> +				ARRAY_SIZE(apds9306_channels_with_events);
>> +		ret = devm_request_threaded_irq(dev, client->irq, NULL,
>> +					apds9306_irq_handler,
>> +					IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
> 
> The direction of the interrupt should come from device tree.  Sometimes people
> use level conversion by using an not gate and that flips the logic of the
> interrupt in a way that the driver can't see.  Hence we leave that
> detail for firmware, not the driver.
Ok, understood.
> 
>> +	ret = devm_add_action_or_reset(dev, apds9306_powerdown, data);
> 
> Why at this point? I'd have thought it wasn't powered up until init_device()
> which follows?  So I'd expect to see this call after that, not before.
> 
Right. I will do a bit more reading on this before using this. I assumed this
functions registers the callback which gets called at driver release by the
subsystem similar to release().

>> +	if (ret)
>> +		return dev_err_probe(dev, ret,
>> +				"failed to add action on driver unwind\n");
>> +
>> +	ret = apds9306_init_device(data);
>> +	if (ret)
>> +		return dev_err_probe(dev, ret, "failed to init device\n");
>> +
>> +	return devm_iio_device_register(dev, indio_dev);
>> +}
>> +
>> +static int apds9306_runtime_suspend(struct device *dev)
>> +{
>> +	struct iio_dev *indio_dev = dev_get_drvdata(dev);
>> +	struct apds9306_data *data = iio_priv(indio_dev);
>> +	int ret;
>> +
>> +	ret = apds9306_power_state(data, STANDBY);
>> +	if (ret)
>> +		regcache_cache_only(data->regmap, true);
> 
> What is the logic of putting the regcache into cache only mode
> if we fail to power down the device?
Yes, true. Real regs are available why use fake ones. I'll fix it.


>> +	ret = apds9306_power_state(data, ACTIVE);
>> +	if (ret)
>> +		regcache_cache_only(data->regmap, true);
> 
> If you get here an this failed we are in an unknown state where
> the device is effectively dead anyway.  I'd not bother
> with juggling the state of the regcache.  Or am I missing some path
> in which this regcache_cache_only() is called that isn't
> an error path?
> 
Yes, this is an error. I'll simply return error.

>> +
>> +static const struct i2c_device_id apds9306_id[] = {
>> +	{ "apds9306" }, { }
> 
> Put the terminator on a new line because it reduces the noise if we ever add
> more devices by removing the need to reformat this first.
> 
Ok.
>> +};
>> +MODULE_DEVICE_TABLE(i2c, apds9306_id);
>> +
>> +static const struct of_device_id apds9306_of_match[] = {
>> +	{ .compatible = "avago,apds9306" }, { }
> 
> Same as above.
> 
Ok.

Thank you Jonathan for the review. I'll get the changes done in the next version.

Regards,
Subhajit Ghosh

Powered by blists - more mailing lists