Re: [PATCH 2/2] iio: light: Add support for APDS9306 Light Sensor
From: Subhajit Ghosh
Date: Wed Oct 11 2023 - 10:37:23 EST
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@xxxxxxxxxxxxxx>
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