Message-ID: <20241214182123.22b5ede0@jic23-huawei>
Date: Sat, 14 Dec 2024 18:21:23 +0000
From: Jonathan Cameron <jic23@...nel.org>
To: Per-Daniel Olsson <perdaniel.olsson@...s.com>
Cc: Lars-Peter Clausen <lars@...afoo.de>, Rob Herring <robh@...nel.org>,
 Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley
 <conor+dt@...nel.org>, Javier Carrasco <javier.carrasco.cruz@...il.com>,
 <linux-iio@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
 <devicetree@...r.kernel.org>, <rickard.andersson@...s.com>,
 <kernel@...s.com>
Subject: Re: [PATCH v9 2/2] iio: light: Add support for TI OPT4060 color
 sensor

On Wed, 11 Dec 2024 15:04:09 +0100
Per-Daniel Olsson <perdaniel.olsson@...s.com> wrote:

> Add support for Texas Instruments OPT4060 RGBW Color sensor.
> 
> Signed-off-by: Per-Daniel Olsson <perdaniel.olsson@...s.com>

Hi Per-Daniel,

I think this is nearly there, but still some races or at least places
I can't currently convince myself aren't races.

I remember long ago being a wimp and failing to implement a similar dance
in the max1363 ADC driver. That manages one more complexity in that in
it's continuous mode if events are enabled, the data fields move.
It still only supports one of events, sysfs read back or buffered output,
not any combination.  Maybe if I can find the hardware I'll revisit that
one day.

Thanks,

Jonathan

> diff --git a/drivers/iio/light/opt4060.c b/drivers/iio/light/opt4060.c
> new file mode 100644
> index 000000000000..4a7d970c5d7c
> --- /dev/null
> +++ b/drivers/iio/light/opt4060.c


> +struct opt4060_chip {
> +	struct regmap *regmap;
> +	struct device *dev;
> +	struct iio_trigger *trig;
> +	u8 int_time;
> +	int irq;
> +	struct mutex irq_setting_lock;

General rule is all locks need a comment on what data they protect
even when they have a nice specific name.  Bit advantage is it makes
it clear what they are not designed to protect!

> +	struct completion completion;
> +	bool thresh_event_lo_active;
> +	bool thresh_event_hi_active;
> +};

> +static void opt4060_claim_irq_setting_lock(struct opt4060_chip *chip)
> +{
> +	if (chip->irq)
I'm struggling to see why you'd be messing with irqs if you don't have
any?  I can't see a path in which chip->irq isn't set (which makes sense!)
and you get here.

So I think you can just move the mutex inline which avoids the mess
of lockdep warnings etc and let's you use guard() to simplify things.

> +		mutex_lock(&chip->irq_setting_lock);
> +}
> +
> +static void opt4060_release_irq_setting_lock(struct opt4060_chip *chip)
> +{
> +	if (chip->irq)
> +		mutex_unlock(&chip->irq_setting_lock);
> +}
> +
> +static int opt4060_set_int_state(struct opt4060_chip *chip, u32 state)
> +{
> +	int ret;
> +	unsigned int regval;
> +
> +	opt4060_claim_irq_setting_lock(chip);
> +	regval = FIELD_PREP(OPT4060_INT_CTRL_INT_CFG, state);
> +	ret = regmap_update_bits(chip->regmap, OPT4060_INT_CTRL,
> +				 OPT4060_INT_CTRL_INT_CFG, regval);
> +	if (ret)
> +		dev_err(chip->dev, "Failed to set interrupt config\n");
> +	opt4060_release_irq_setting_lock(chip);
> +	return ret;
> +}
> +
> +static int opt4060_set_continuous_mode(struct opt4060_chip *chip,
> +				       bool continuous)
> +{
> +	unsigned int reg;
> +	int ret;
> +
> +	ret = regmap_read(chip->regmap, OPT4060_CTRL, &reg);

You could use a regmap_update_bits() call to simplify this like you 
do for the int_state above.


> +	if (ret < 0) {
> +		dev_err(chip->dev, "Failed to read ctrl register\n");
> +		return ret;
> +	}
> +	reg &= ~OPT4060_CTRL_OPER_MODE_MASK;
> +	if (continuous)
> +		reg |= FIELD_PREP(OPT4060_CTRL_OPER_MODE_MASK,
> +				  OPT4060_CTRL_OPER_MODE_CONTINUOUS);
> +	else
> +		reg |= FIELD_PREP(OPT4060_CTRL_OPER_MODE_MASK,
> +				  OPT4060_CTRL_OPER_MODE_ONE_SHOT);
> +
> +	/* Trigger a new conversions by writing to CRTL register. */
> +	ret = regmap_write(chip->regmap, OPT4060_CTRL, reg);
> +	if (ret)
> +		dev_err(chip->dev, "Failed to set ctrl register\n");
> +	return ret;
> +}
> +
> +static bool opt4060_event_active(struct opt4060_chip *chip)
> +{
> +	return chip->thresh_event_lo_active || chip->thresh_event_hi_active;
> +}
> +
> +static int opt4060_set_state_common(struct opt4060_chip *chip,
> +				    bool continuous_sampling,
> +				    bool continuous_irq, bool direct_mode)
> +{
> +	int ret = 0;
> +
> +	/* It is important to setup irq before sampling to avoid missing samples. */
> +	if (continuous_irq || !direct_mode)
> +		ret = opt4060_set_int_state(chip, OPT4060_INT_CTRL_ALL_CH);
> +	else if (direct_mode)
> +		ret = opt4060_set_int_state(chip, OPT4060_INT_CTRL_THRESHOLD);
> +	if (ret) {
> +		dev_err(chip->dev, "Failed to set irq state.\n");
> +		return ret;
> +	}
> +
> +	if (continuous_sampling || !direct_mode || opt4060_event_active(chip))

I think there may also a race around the event active check.  You could have
one event direction being enabled concurrently with the other being disabled.
I'm not sure it matters but worth checking.

Side effect of either claiming direct or buffered mode is that only one
caller can do it at a time, so that would close this race as well. 
Having said that, it's an implementation detail of the core (be it one that
has been there a long time) so you should really have your own driver
specific locking scheme prevent that.


> +		ret = opt4060_set_continuous_mode(chip, true);
> +	else if (direct_mode)
> +		ret = opt4060_set_continuous_mode(chip, false);
> +	if (ret)
> +		dev_err(chip->dev, "Failed to set sampling state.\n");
> +	return ret;
> +}
> +
> +/*
> + * Function for setting the driver state for sampling and irq. When disabling
> + * continuous sampling or irq, the IIO direct mode must be claimed to prevent
> + * races with buffer enabling/disabling. In the case when the direct mode is
> + * not possible to claim, the function will keep continuous mode. All
> + * functions, sysfs read, events and buffer, work in continuous mode.
> + */
> +static int opt4060_set_driver_state(struct iio_dev *indio_dev,
> +				    bool continuous_sampling,
> +				    bool continuous_irq)
> +{
> +	struct opt4060_chip *chip = iio_priv(indio_dev);
> +	bool direct_mode = false;
> +	int ret = 0;
> +
> +	if (!iio_device_claim_direct_mode(indio_dev))
> +		direct_mode = true;

Hmm. I'm dubious about this pattern. Why is it fine if the driver
leaves buffered mode right here? I was expecting this to do
the dance with claiming either direct mode or buffered mode.
(with the retry loop).  Direct mode that you pass into the next
call may well be false when it should be true.

Even if you can reason why that isn't a problem (and there are worse
dances where it switches mode multiple times during your call
of the next function to consider) I think it is easier to reason
about if we know it is definitely not changing state until we
release it.

> +
> +	ret = opt4060_set_state_common(chip, continuous_sampling,
> +				       continuous_irq, direct_mode);
> +
> +	if (direct_mode)
> +		iio_device_release_direct_mode(indio_dev);
> +	return ret;
> +}
> +
> +/*
> + * This function is called in direct mode from the framework.
> + */
> +static int opt4060_trigger_set_state(struct iio_trigger *trig, bool state)
> +{
> +	struct iio_dev *indio_dev = iio_trigger_get_drvdata(trig);
> +	struct opt4060_chip *chip = iio_priv(indio_dev);
> +	int ret = 0;
> +
> +	return ret = opt4060_set_state_common(chip, state, state, true);

return opt_set_state_common() is probably the intent.

> +}

> +static int opt4060_trigger_new_samples(struct iio_dev *indio_dev)
> +{
> +	struct opt4060_chip *chip = iio_priv(indio_dev);
> +	int ret;
> +
> +	/*
> +	 * The conversion time should be 500us startup time plus the integration time
> +	 * times the number of channels. An exact timeout isn't critical, it's better
> +	 * not to get incorrect errors in the log. Setting the timeout to double the
> +	 * theoretical time plus and extra 100ms margin.
> +	 */
> +	unsigned int timeout_us = (500 + OPT4060_NUM_CHANS *
> +				  opt4060_int_time_reg[chip->int_time][0]) * 2 + 100000;
> +
> +	/* Setting the state in one shot mode with irq on each sample. */
> +	ret = opt4060_set_driver_state(indio_dev, false, true);
> +	if (ret)
> +		return ret;
> +
> +	if (chip->irq) {
> +		reinit_completion(&chip->completion);
> +		opt4060_claim_irq_setting_lock(chip);
> +		if (wait_for_completion_timeout(&chip->completion,
> +						usecs_to_jiffies(timeout_us)) == 0) {
> +			dev_err(chip->dev, "Completion timed out.\n");
> +			opt4060_release_irq_setting_lock(chip);

This is where exposing the lock directly will simplify things as you can just use
a guard.

> +			return -ETIME;
> +		}
> +		opt4060_release_irq_setting_lock(chip);
> +	} else {
> +		unsigned int ready;
> +
> +		ret = regmap_read_poll_timeout(chip->regmap, OPT4060_RES_CTRL,
> +					       ready, (ready & OPT4060_RES_CTRL_CONV_READY),
> +					       1000, timeout_us);
> +		if (ret)
> +			dev_err(chip->dev, "Conversion ready did not finish within timeout.\n");
> +	}
> +	/* Setting the state in one shot mode with irq on thresholds. */
> +	ret = opt4060_set_driver_state(indio_dev, false, false);
> +
> +	return ret;

	return opt4060_...

> +}

> +static int opt4060_write_raw_get_fmt(struct iio_dev *indio_dev,
> +				     struct iio_chan_spec const *chan,
> +				     long mask)
> +{
> +	switch (mask) {
> +	case IIO_CHAN_INFO_INT_TIME:
> +		return IIO_VAL_INT_PLUS_MICRO;
IIRC That's the default, so you don't need to provide write_raw_get_fmt,
though no harm in doing so I guess.

> +	default:
> +		return -EINVAL;
> +	}
> +}


> +static int opt4060_get_channel_sel(struct opt4060_chip *chip, int *ch_sel)
> +{
> +	int ret;
> +	u32 regval;
> +
> +	ret = regmap_read(chip->regmap, OPT4060_INT_CTRL, &regval);
> +	if (ret)
> +		dev_err(chip->dev, "Failed to get channel selection.\n");

if you have garbage, not sure it's valid to update ch_sel.

> +	*ch_sel = FIELD_GET(OPT4060_INT_CTRL_THRESH_SEL, regval);
> +	return ret;
> +}
> +


> +static int opt4060_write_event_config(struct iio_dev *indio_dev,
> +				      const struct iio_chan_spec *chan,
> +				      enum iio_event_type type,
> +				      enum iio_event_direction dir, bool state)
> +{
> +	int ch_sel, ch_idx = chan->scan_index;
> +	struct opt4060_chip *chip = iio_priv(indio_dev);
> +	int ret;
> +
> +	if (chan->type != IIO_INTENSITY)
> +		return -EINVAL;
> +	if (type != IIO_EV_TYPE_THRESH)
> +		return -EINVAL;
> +
> +	ret = opt4060_get_channel_sel(chip, &ch_sel);
> +	if (ret)
> +		return ret;
> +
> +	if (state) {
> +		/* Only one channel can be active at the same time */
> +		if ((chip->thresh_event_lo_active ||
> +			chip->thresh_event_hi_active) && (ch_idx != ch_sel))

That's a bit nasty to ready. I'd use a slightly long line and get the || pair
on the first line.

Hmm. We've never made rules on this but some devices to fifo type
selection if they have limitations on events enabled at the same time.
With hindsight I think this scheme of just saying no is probably more
user friendly.

> +			return -EBUSY;
> +		if (dir == IIO_EV_DIR_FALLING)
> +			chip->thresh_event_lo_active = true;
> +		else if (dir == IIO_EV_DIR_RISING)
> +			chip->thresh_event_hi_active = true;
> +		ret = opt4060_set_channel_sel(chip, ch_idx);
> +		if (ret)
> +			return ret;
> +	} else {
> +		if (ch_idx == ch_sel) {
> +			if (dir == IIO_EV_DIR_FALLING)
> +				chip->thresh_event_lo_active = false;
> +			else if (dir == IIO_EV_DIR_RISING)
> +				chip->thresh_event_hi_active = false;
> +		}
> +	}
> +
> +	return opt4060_set_driver_state(indio_dev, chip->thresh_event_hi_active |
> +					chip->thresh_event_lo_active, false);
Maybe wrap it to have the | pair on lines with nothing else.  They are a little bit burried
otherwise.
	return opt4060_set_driver_state(indio_dev,
					chip->thresh_event_hi_active |
					chip->thresh_event_lo_active,
					false);

> +}
> +
> +static const struct iio_info opt4060_info = {
> +	.read_raw = opt4060_read_raw,
> +	.write_raw = opt4060_write_raw,
> +	.write_raw_get_fmt = opt4060_write_raw_get_fmt,
> +	.read_avail = opt4060_read_available,
> +	.read_event_value = opt4060_read_event,
> +	.write_event_value = opt4060_write_event,
> +	.read_event_config = opt4060_read_event_config,
> +	.write_event_config = opt4060_write_event_config,
> +};

Given you have option for no irq it is probably worth picking a version of this
info structure with all the event callbacks removed.   Technically it isn't
required but it does harden the code (by crashing horribly if you call them ;)



> +static irqreturn_t opt4060_trigger_handler(int irq, void *p)
> +{
> +	struct iio_poll_func *pf = p;
> +	struct iio_dev *idev = pf->indio_dev;
> +	struct opt4060_chip *chip = iio_priv(idev);
> +	struct  {
> +		u32 chan[OPT4060_NUM_CHANS];
> +		aligned_s64 ts;
> +	} raw;
> +	int i = 0;
> +	int chan, ret;
> +
> +	/* If the trigger is coming for a different driver, a new sample is needed.*/

from a different driver?

> +	if (iio_trigger_validate_own_device(idev->trig, idev))
> +		opt4060_trigger_new_samples(idev);
> +
> +	memset(&raw, 0, sizeof(raw));
> +
> +	iio_for_each_active_channel(idev, chan) {
> +		if (chan == OPT4060_ILLUM)
> +			ret = opt4060_calc_illuminance(chip, &raw.chan[i++]);
> +		else
> +			ret = opt4060_read_raw_value(chip,
> +						     idev->channels[chan].address,
> +						     &raw.chan[i++]);
> +		if (ret) {
> +			dev_err(chip->dev, "Reading channel data failed\n");
> +			goto err_read;
> +		}
> +	}
> +
> +	iio_push_to_buffers_with_timestamp(idev, &raw, pf->timestamp);
> +err_read:
> +	iio_trigger_notify_done(idev->trig);
> +	return IRQ_HANDLED;
> +}
> +
> +static irqreturn_t opt4060_irq_thread(int irq, void *private)
> +{
> +	struct iio_dev *idev = private;
> +	struct opt4060_chip *chip = iio_priv(idev);
> +	int ret, dummy;
> +	unsigned int int_res;
> +
> +	ret = regmap_read(chip->regmap, OPT4060_RES_CTRL, &int_res);
> +	if (ret < 0) {
> +		dev_err(chip->dev, "Failed to read interrupt reasons.\n");
> +		return IRQ_NONE;
> +	}
> +
> +	/* Read OPT4060_CTRL to clear interrupt */
> +	ret = regmap_read(chip->regmap, OPT4060_CTRL, &dummy);
> +	if (ret < 0) {
> +		dev_err(chip->dev, "Failed to clear interrupt\n");
> +		return IRQ_NONE;
> +	}
> +
> +	/* Handle events */
> +	if (int_res & (OPT4060_RES_CTRL_FLAG_H | OPT4060_RES_CTRL_FLAG_L)) {
> +		u64 code;
> +		int chan = 0;
> +
> +		ret = opt4060_get_channel_sel(chip, &chan);
> +		if (ret) {
> +			dev_err(chip->dev, "Failed to read threshold channel.\n");
> +			return IRQ_NONE;
> +		}
> +
> +		/* Check if the interrupt is from the lower threshold */
> +		if (int_res & OPT4060_RES_CTRL_FLAG_L) {
> +			code = IIO_MOD_EVENT_CODE(IIO_INTENSITY,
> +						  chan,
> +						  idev->channels[chan].channel2,
> +						  IIO_EV_TYPE_THRESH,
> +						  IIO_EV_DIR_FALLING);
> +			iio_push_event(idev, code, iio_get_time_ns(idev));
> +		}
> +		/* Check if the interrupt is from the upper threshold */
> +		if (int_res & OPT4060_RES_CTRL_FLAG_H) {
> +			code = IIO_MOD_EVENT_CODE(IIO_INTENSITY,
> +						  chan,
> +						  idev->channels[chan].channel2,
> +						  IIO_EV_TYPE_THRESH,
> +						  IIO_EV_DIR_RISING);
> +			iio_push_event(idev, code, iio_get_time_ns(idev));
> +		}
> +	}
> +
> +	/* Handle conversion ready */
> +	if (int_res & OPT4060_RES_CTRL_CONV_READY) {
> +		/* Signal completion for potentially waiting reads */
> +		complete(&chip->completion);

That looks problematic as you haven't necessarily reset the completion
if the buffer is enabled.  So you probably need a flag or something similar
to say a sysfs read has been requested.


> +
> +		/* Handle data ready triggers */
> +		if (iio_buffer_enabled(idev))
> +			iio_trigger_poll_nested(chip->trig);
> +	}
> +	return IRQ_HANDLED;
> +}

> +static int opt4060_setup_trigger(struct opt4060_chip *chip, struct iio_dev *idev)
> +{
> +	struct iio_trigger *data_trigger;
> +	char *name;
> +	int ret;
> +
> +	data_trigger = devm_iio_trigger_alloc(chip->dev, "%s-data-ready-dev%d",
> +					      idev->name, iio_device_id(idev));
> +	if (!data_trigger)
> +		return -ENOMEM;
> +
> +	/* The data trigger allows for sample capture on each new conversion ready interrupt. */

Make that a multiline comment.

> +	chip->trig = data_trigger;
> +	data_trigger->ops = &opt4060_trigger_ops;
> +	iio_trigger_set_drvdata(data_trigger, idev);
> +	ret = devm_iio_trigger_register(chip->dev, data_trigger);
> +	if (ret)
> +		return dev_err_probe(chip->dev, ret,
> +				     "Data ready trigger registration failed\n");
> +
> +	name = devm_kasprintf(chip->dev, GFP_KERNEL, "%s-opt4060",
> +			      dev_name(chip->dev));
> +	if (!name)
> +		return dev_err_probe(chip->dev, -ENOMEM, "Failed to alloc chip name\n");
> +
> +	ret = devm_request_threaded_irq(chip->dev, chip->irq, NULL, opt4060_irq_thread,
> +					IRQF_TRIGGER_FALLING | IRQF_TRIGGER_RISING |
That's unusual for a trigger type interrupt and seems likely to give a lot
of spurious interrupts.  Even if the pulse is short, some interrupt controllers
will hang on to the bonus edge and trigger again when you reenable the interrupt.

If intent is to use this for events, then I think you can configure it to latched
window mode and one edge type and it will all work.

> +					IRQF_ONESHOT, name, idev);
> +	if (ret)
> +		return dev_err_probe(chip->dev, ret, "Could not request IRQ\n");
> +
> +	init_completion(&chip->completion);
> +
> +	mutex_init(&chip->irq_setting_lock);

Trivial and I might not even bother changing it, but slightly preference for
	ret = devm_mutex_init(...)
	if (ret)
		return ret;

> +
> +	ret = regmap_write_bits(chip->regmap, OPT4060_INT_CTRL,
> +				OPT4060_INT_CTRL_OUTPUT,
> +				OPT4060_INT_CTRL_OUTPUT);
> +	if (ret)
> +		return dev_err_probe(chip->dev, ret,
> +				     "Failed to set interrupt as output\n");
> +
> +	return 0;
> +}

Powered by blists - more mailing lists