Message-ID: <91463df1-5aba-484a-92ea-f8979ec30535@fi.rohmeurope.com>
Date:   Tue, 2 May 2023 08:07:40 +0000
From:   "Vaittinen, Matti" <Matti.Vaittinen@...rohmeurope.com>
To:     Jonathan Cameron <jic23@...nel.org>,
        Matti Vaittinen <mazziesaccount@...il.com>
CC:     Lars-Peter Clausen <lars@...afoo.de>,
        Rob Herring <robh+dt@...nel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
        Shreeya Patel <shreeya.patel@...labora.com>,
        Zhigang Shi <Zhigang.Shi@...eon.com>,
        Paul Gazzillo <paul@...zz.com>,
        "linux-iio@...r.kernel.org" <linux-iio@...r.kernel.org>,
        "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Andi Shyti <andi.shyti@...nel.org>
Subject: Re: [PATCH v3 4/5] iio: light: ROHM BU27008 color sensor

On 5/1/23 17:20, Jonathan Cameron wrote:
> On Wed, 26 Apr 2023 11:08:17 +0300
> Matti Vaittinen <mazziesaccount@...il.com> wrote:
> 
>> The ROHM BU27008 is a sensor with 5 photodiodes (red, green, blue, clear
>> and IR) with four configurable channels. Red and green being always
>> available and two out of the rest three (blue, clear, IR) can be
>> selected to be simultaneously measured. Typical application is adjusting
>> LCD backlight of TVs, mobile phones and tablet PCs.
>>
>> Add initial support for the ROHM BU27008 color sensor.
>>   - raw_read() of RGB and clear channels
>>   - triggered buffer w/ DRDY interrtupt
>>
>> Signed-off-by: Matti Vaittinen <mazziesaccount@...il.com>
> 
> Hi Matti,
> 
> Mostly trivial stuff, but some confusion has occurred with respect to the
> two interrupts involve in an IIO trigger.  Specifically the pollfunc stuff
> occurs on the downward side of the irqchip that hides on the consumer side
> of an IIO trigger) and is set up by devm_iio_trigered_buffer_setup() not
> of the interrupt that calls iio_trigger_poll[_nested]()
> 

> 
>> +
>> +static int bu27008_try_find_new_time_gain(struct bu27008_data *data, int val,
>> +					  int val2, int *gain_sel)
>> +{
>> +	/* Could not support new scale with existing int-time */
> 
> I'd move that above the function and change it to
> 	/* Called if the new scale could not be supported with existing int-time */
> Down here it is not clear that this applies to the whole funciton.
> 
> 
>> +	int i, ret, new_time_sel;
>> +
>> +	for (i = 0; i < data->gts.num_itime; i++) {
>> +		new_time_sel = data->gts.itime_table[i].sel;
>> +		ret = iio_gts_find_gain_sel_for_scale_using_time(&data->gts,
>> +					new_time_sel, val, val2 * 1000, gain_sel);
>> +		if (!ret)
>> +			break;
>> +	}
>> +	if (i == data->gts.num_itime) {
>> +		dev_err(data->dev, "Can't support scale %u %u\n", val,
>> +			val2);
> 
> Line wrapping inconsistent.  I like short lines with appropriate flexibility where
> longer ones are more readable. However, I am fairly sure this one fits under 80
> chars as a single line.

Thanks! Both the comment and the line-wrapping were just left like this 
when I pulled this part of a functionality into this new function. Well 
spotted!

> 
>> +
>> +		return -EINVAL;
>> +	}
>> +
>> +	return bu27008_set_int_time_sel(data, new_time_sel);
>> +}
>> +
>> +static int bu27008_set_scale(struct bu27008_data *data,
>> +			     struct iio_chan_spec const *chan,
>> +			     int val, int val2)
>> +{
>> +	int ret, gain_sel, time_sel;
>> +
>> +	if (chan->scan_index == BU27008_IR)
>> +		return -EINVAL;
>> +
>> +	mutex_lock(&data->mutex);
>> +
>> +	ret = bu27008_get_int_time_sel(data, &time_sel);
>> +	if (ret < 0)
>> +		goto unlock_out;
>> +
>> +	ret = iio_gts_find_gain_sel_for_scale_using_time(&data->gts, time_sel,
>> +						val, val2 * 1000, &gain_sel);
>> +	if (ret)
>> +		ret = bu27008_try_find_new_time_gain(data, val, val2, &gain_sel);
> 
> Obviously it is code that doesn't make any functional difference, but I'd prefer to see
> 	if (ret) {
> 		ret = bu27....
> 		if (ret)
> 			goto unlock_out;
> 	}
> 	ret = bu27008_write_gain_sel();
> 
> so that each error path is out of line, but the good path is the linear flow.

... Yuck! The difference of tastes ... ;)
Well, not worth fighting I guess.

> 
>> +
>> +	if (!ret)
>> +		ret = bu27008_write_gain_sel(data, gain_sel);
>> +
>> +unlock_out:
>> +	mutex_unlock(&data->mutex);
>> +
>> +	return ret;
>> +}
> 
> 
>> +static irqreturn_t bu27008_irq_thread_handler(int irq, void *p)
>> +{
>> +	struct iio_poll_func *pf = p;
>> +	struct iio_dev *idev = pf->indio_dev;
>> +	struct bu27008_data *data = iio_priv(idev);
>> +
>> +	iio_trigger_poll_nested(data->trig);
> 
> See below but this is what alerted me to something unusual.
> It never makes sense to have iio_trigger_poll_nested() called unless
> there is a check on whether it should be called!  If there isn't
> iio_trigger_poll() in the top half is the right thing to do. >
>> +
>> +	return IRQ_HANDLED;
>> +}
>> +
> 
> 
>> +static int bu27008_probe(struct i2c_client *i2c)
>> +{
> 
> ...
> 
>> +
>> +	if (i2c->irq) {
>> +		ret = devm_iio_triggered_buffer_setup(dev, idev,
>> +						      &iio_pollfunc_store_time,
>> +						      bu27008_trigger_handler,
>> +						      &bu27008_buffer_ops);
>> +		if (ret)
>> +			return dev_err_probe(dev, ret,
>> +				     "iio_triggered_buffer_setup_ext FAIL\n");
>> +
>> +		itrig = devm_iio_trigger_alloc(dev, "%sdata-rdy-dev%d",
>> +					       idev->name, iio_device_id(idev));
>> +		if (!itrig)
>> +			return -ENOMEM;
>> +
>> +		data->trig = itrig;
>> +
>> +		itrig->ops = &bu27008_trigger_ops;
>> +		iio_trigger_set_drvdata(itrig, data);
>> +
>> +		name = devm_kasprintf(dev, GFP_KERNEL, "%s-bu27008",
>> +				      dev_name(dev));
>> +
>> +		ret = devm_request_threaded_irq(dev, i2c->irq,
>> +						iio_pollfunc_store_time,
> 
> This is on the wrong irq. 

Seems like I have some homework to do :)

Right. I now see I pass the iio_pollfunc_store_time() as top half for 
both the "real IRQ" generated by the device (here), as well as a 
top-half for the devm_iio_triggered_buffer_setup(). Ideally I like the 
idea of taking the timestamp in the top half for the device-generated 
IRQ as it is closer the moment HW did acquire the sample - but it really 
would make no difference here (even if I did it correctly).

  iio_pollfunc_store_time is used with the trigger not
> here.  Basically what happens is the caller of iio_poll_trigger() fires the input
> to a software irq chip that then signals all the of the downstream irqs (which
> are the individual consumers of the triggers).  If that's triggered from the
> top half / non threaded bit of the interrupt the iio_pollfunc_store_time()
> will be called in that non threaded context before the individual threads
> for the trigger consumer are started.

Oh. So, you mean the iio_pollfunc_store_time() is automatically called 
already before kicking the SW-IRQ? So we don't need it in 
devm_iio_triggered_buffer_setup() anymore?

> If there is nothing to do in the actual interrupt as it's a data ready
> only signal, then you should just call iio_trigger_poll() in the top half and
> use devm_request_irq() only as there is no thread in this interrupt (though
> there is one for the interrupt below the software interrupt chip).

I haven't tested this yet so please ignore me if I am writing nonsense - 
but... The BU27008 will keep the IRQ line asserted until a register is 
read. We can't read the register form HW-IRQ so we need to keep the IRQ 
disabled until the threaded trigger handler is ran. With the setup we 
have here, the IRQF_ONESHOT, took care of this. I assume that changing 
to call the iio_poll_trigger() from top-half means I need to explicitly 
disable the IRQ and re-enable it at the end of the trigger thread after 
reading the register which debounces the IRQ line?

> 
> 
>> +						&bu27008_irq_thread_handler,
>> +						IRQF_ONESHOT, name, idev->pollfunc);
>> +		if (ret)
>> +			return dev_err_probe(dev, ret,
>> +					     "Could not request IRQ\n");
>> +
>> +
>> +		ret = devm_iio_trigger_register(dev, itrig);
>> +		if (ret)
>> +			return dev_err_probe(dev, ret,
>> +					     "Trigger registration failed\n");
>> +	} else {
>> +		dev_warn(dev, "No IRQ configured\n");
> 
> Why is it a warning?  Either driver works without an IRQ, or it doesn't.
> dev_dbg() or dev_info() at most.

Some of it works. Well, maybe I'll change it to tell that device works 
in raw_read only mode.

Thanks again for the help!

Yours,
	-- Matti

-- 
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland

~~ When things go utterly wrong vim users can always type :help! ~~

Powered by blists - more mailing lists