Re: [PATCH] iio: light: add driver support for MAX44009

From: Jonathan Cameron <jonathan.cameron@huawei.com>
To: Robert Eshleman <bobbyeshleman@gmail.com>
Cc: Peter Meerwald-Stadler <pmeerw@pmeerw.net>,
	Jonathan Cameron <jic23@kernel.org>, <linux-iio@vger.kernel.org>,
	<linux-kernel@vger.kernel.org>
Subject: Re: [PATCH] iio: light: add driver support for MAX44009
Date: Mon, 21 Jan 2019 10:44:17 +0000	[thread overview]
Message-ID: <20190121104417.00003971@huawei.com> (raw)
In-Reply-To: <20190121063508.GA21651@bobby.localdomain>

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