lkml.org 
[lkml]   [2019]   [Jan]   [21]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH] iio: light: add driver support for MAX44009
On Sun, 20 Jan 2019 22:39:14 -0800
Robert Eshleman <bobbyeshleman@gmail.com> wrote:

> On Sat, Jan 19, 2019 at 08:41:46PM +0100, Peter Meerwald-Stadler wrote:
>
> Hey Jonathon and Peter,
>
> First, thank you for the constructive and in-depth feedback.
>
> I have a question below regarding a section of code that will need to
> be protected against a race condition if future features are added.

Comments inline. I would be paranoid now and add the lock. Less likely
that we'll accidentally forget it at some later stage!


>
> Mostly this email is just an ack that I've started working on the
> changes.
>
> Thanks again,
> Robert
>
> > On Sat, 19 Jan 2019, Jonathan Cameron wrote:
> >
> > some more comments from my side below...
> >
> > > On Wed, 16 Jan 2019 22:56:23 -0800
> > > Robert Eshleman <bobbyeshleman@gmail.com> wrote:
> > >
> > > Hi Robert,
> > >
> > > Note I review drivers backwards, so comments may make more sense that
> > > way around.
> > >
> > > > The MAX44009 is a low-power ambient light sensor from Maxim Integrated.
> > > > It differs from the MAX44000 in that it doesn't have proximity sensing and that
> > > > it requires far less current (1 micro-amp vs 5 micro-amps). The register
> > > > mapping and feature set between the two are different enough to require a new
> > > > driver for the MAX44009.
> > > >
> > > > Developed and tested with a BeagleBone Black and UDOO Neo (i.MX6SX)
> > > >
> > > > Supported features:
> > > >
> > > > * Rising and falling illuminance threshold
> > > > events
> > >
> > > Not really on this one. You support using them as a trigger, which
> > > is not how threshold events should be handled in IIO. Please
> > > report them as events. This device doesn't seem to have
> > > a trigger that can be used in general, so you shouldn't provide
> > > one.
> > >
> > > Userspace can use the event to decide to do a read if it wants to
> > > follow the classic move the thresholds so as to detect big
> > > 'changes' in light intensity.
> > >
> > > Various other comments inline. Quite a bit of style cleanup
> > > needed as well, please check the kernel docs for coding style
> > > https://www.kernel.org/doc/Documentation/process/coding-style.rst
> > >
> > > Also take a look at other IIO drivers for the bits that are
> > > noted in there as varying across the kernel.
> > >
> > > Whilst there is quite a bit to work on in here yet, great to see
> > > support for this new part! Looking forward to v2.
> > >
> > > Jonathan
> > >
>
> After seeing your notes and looking closer at how userspace leverages
> triggers, I can see now how it does not make sense to let this device
> supply one. It is, as you mentioned, events that are the right fit
> for this device.
>
Great.

..
> > >
> > > > + dev_err(&client->dev,
> > > > + "failed to read reg 0x%0x, err: %d\n", reg, ret);
> > > > + goto err;
> > > > + }
> > > > +
> > > > +err:
> > > > + mutex_unlock(&data->lock);
> > > > + return ret;
> > > > +}
> > > > +
> > > > +static int max44009_write_reg(struct max44009_data *data, char reg, char buf)
> > > > +{
> > > > + struct i2c_client *client = data->client;
> > > > + int ret;
> > > > +
> > > > + mutex_lock(&data->lock);
> > >
> > > What is this mutex protecting?
> > >
> > > > + ret = i2c_smbus_write_byte_data(client, reg, buf);
> > > > + if (ret < 0) {
> > >
> > > returns 0 on success. So do
> > > if (ret) as it'll prove simpler to follow for handling later.
> > >
> > > > + dev_err(&client->dev,
> > > > + "failed to write reg 0x%0x, err: %d\n",
> > > > + reg, ret);
> > > > + goto err;
> > > > + }
> > > > +
> > > > +err:
> > > > + mutex_unlock(&data->lock);
> > > > + return ret;
> > > > +}
> > > > +
> > > > +static int max44009_read_int_time(struct max44009_data *data)
> > > > +{
> > > > + int ret = max44009_read_reg(data, MAX44009_REG_CFG);
> > > > +
> > > > + if (ret < 0) {
> > > > + return ret;
> > > > + }
> > > > +
> > > > + return max44009_int_time_ns_array[ret & MAX44009_INT_TIME_MASK];
> > > > +}
> > > > +
> > > > +static int max44009_write_raw(struct iio_dev *indio_dev,
> > > > + struct iio_chan_spec const *chan, int val,
> > > > + int val2, long mask)
> > > > +{
> > > > + struct max44009_data *data = iio_priv(indio_dev);
> > > > + int ret, int_time;
> > > > + s64 ns;
> > > > +
> > > > + if (mask == IIO_CHAN_INFO_INT_TIME && chan->type == IIO_LIGHT) {
> > > > + ns = val * NSEC_PER_SEC + val2;
> > > > + int_time = find_closest_descending(
> > > > + ns,
> > > > + max44009_int_time_ns_array,
> > > > + ARRAY_SIZE(max44009_int_time_ns_array));
> > > > +
> >
> > a mutex might make sense here to protect the read/write combo
> >
>
> This device supports other configuration, via MAX44009_REG_CFG, that
> is not yet supported by this driver. If those configurations are
> supported in the future, then there will be a race condition that
> leads to a failure to update the register with both configurations
> (the most recent write will win). Currently, this is the only section
> that modifies this register with a read/update/write. Should this be
> future-proofed now (with a mutex), or deferred to when/if those
> features are supported in the future?
>
> As an aside, my current revision of this driver removed the
> unnecessary mutex locks, which led to not needing a mutex at all.

I would add them now. It's far too likely they'll get forgotten at some
later stage.

>
> > > > + ret = max44009_read_reg(data, MAX44009_REG_CFG);
> > > > + if (ret < 0)
> > > > + return ret;
> > > > +
> > > > + ret &= ~MAX44009_INT_TIME_MASK;
> > > > + ret |= (int_time << MAX44009_INT_TIME_SHIFT);
> > > > + ret |= MAX44009_MANUAL_MODE_MASK;
> > > > +
> > > > + return max44009_write_reg(data, MAX44009_REG_CFG, ret);
> > > > + }
> > > > + return -EINVAL;
> > > > +}

> > > > +
> > > > +static const struct iio_trigger_ops max44009_trigger_ops = {
> > > > + .set_trigger_state = max44009_set_trigger_state,
> > > > +};
> > > > +
> > > > +static irqreturn_t max44009_trigger_handler(int irq, void *p)
> > > > +{
> > > > + struct iio_dev *indio_dev = p;
> > > > + struct max44009_data *data = iio_priv(indio_dev);
> > > > + int lux, upper, lower;
> > > > + int ret;
> > > > + enum iio_event_direction direction;
> > > > +
> > > > + /* 32-bit for lux and 64-bit for timestamp */
> > > > + u32 buf[3] = {0};
> > >
> > > Except the timestamp needs to be 64 bit aligned so this isn't big enough.
> > > All elements in IIO buffers are 'naturally' aligned. so 32 bit is
> > > aligned to 32 bits, 64 to 64 bits.
> > >
> > > > +
> > > > + ret = max44009_read_reg(data, MAX44009_REG_STATUS);
> > > > + if (ret <= 0) {
> > > > + goto err;
> > > > + }
> > > > +
> > > > + ret = max44009_read_reg(data, MAX44009_REG_ENABLE);
> > > > + if (ret <= 0) {
> > > > + goto err;
> > > > + }
> > >
> > > If these reads are necessary, then add comments on why.
> > >
> > > > +
> > > > + /* Clear interrupt by disabling interrupt (see datasheet) */
> > > > + ret = max44009_write_reg(data, MAX44009_REG_ENABLE, 0);
> > > > + if (ret < 0) {
> > > > + goto err;
> > > > + }
> > > > +
> > > > + lux = max44009_read_lux_raw(data);
> > > > + if (lux < 0) {
> > > > + goto err;
> > > > + }
> > > > +
> > > > + upper = max44009_read_reg(data, MAX44009_REG_UPPER_THR);
> > > > + if (upper < 0) {
> > > > + goto err;
> > > > + }
> > > > + upper = MAX44009_THRESHOLD(upper);
> > > > +
> > > > + lower = max44009_read_reg(data, MAX44009_REG_LOWER_THR);
> > > > + if (lower < 0) {
> > > > + goto err;
> > > > + }
> > > > + lower = MAX44009_THRESHOLD(lower);
> > > > +
> > > > + /* If lux is NOT out-of-bounds then the interrupt was not triggered
> > > > + * by this device
> > > Multiline comment syntax in IIO (and most of the kernel) is
> > > /*
> > > * If...
> > > */
> > >
> > > Do we support shared interrupt lines? Doesn't look like it, which means
> > > it 'probably was' generate by this device, but we don't know why because the value
> > > has changed.
> > >
> > > Returning IRQ_NONE is a bad idea unless we are pretty sure it is a spurious
> > > interrupt, or we have a shared interrupt line. It will ultimately result
> > > in your interrupt being disable by the kernel and all activity breaking.
> > >
> > > There is also a perfectly good register to tell use if we generated the interrupt.
> > > Read that one and use that, not this racey approach.
> > >
>
> That is very good to know how IRQ_NONE is handled in this context.
> After revisiting this function and better understanding events,
> it became clear that this function could basically be reduced down
> to a read of the status register and a call to iio_push_event.
>

Sounds about right.

Jonathan

\
 
 \ /
  Last update: 2019-01-21 11:44    [W:0.051 / U:0.128 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site