lkml.org 
[lkml]   [2023]   [Oct]   [11]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
SubjectRe: [PATCH 2/2] iio: light: Add support for APDS9306 Light Sensor
From
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@tweaklogic.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




\
 
 \ /
  Last update: 2023-10-11 16:38    [W:0.095 / U:0.324 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site