Re: [PATCH v1 2/3] iio: light: ROHM BU27008 color sensor

[Matti Vaittinen]

On 4/23/23 15:57, Jonathan Cameron wrote:
> On Fri, 21 Apr 2023 12:39:36 +0300
> Matti Vaittinen <mazziesaccount@xxxxxxxxx> 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@xxxxxxxxx>
> > +
> > +static int bu27008_meas_set(struct bu27008_data *data, bool enable)
> > +{
> > +	if (enable)
> > +		return regmap_set_bits(data->regmap, BU27008_REG_MODE_CONTROL3,
> > +				       BU27008_MASK_MEAS_EN);
> > +
> > +	return regmap_clear_bits(data->regmap, BU27008_REG_MODE_CONTROL3,
> > +				 BU27008_MASK_MEAS_EN);
>
> Might be cleaner with regmap_update_bits()
>
> > +}

Hm. I need to disagree on this although I think it depends on what one 
is used to :)

For me adding a variable for value to be used is slightly more complex 
than just using clear or set function depending on the enable/disable. I 
remember thinking the same as you and preferring the update_bits also on 
enable/disable cases - until I wrote my first power-supply driver and 
Sebasian Reichel told me to not do:

int val;

if (foo)
	val = mask;
else
	val = 0;

return regmap_update_bits(regmap, reg, mask, val);

but use set/clear bits. This allows killing the 'int val;'. I remember I 
had to sleep over night on it but I later started seeing the set/clear 
bits as a simpler thing.

Sure we could also do

if (foo)
	return regmap_update_bits(map, reg, mask, mask);
else
	return regmap_update_bits(map, reg, mask, 0);

- but here we just replace:

regmap_set_bits(map, reg, mask) with
regmap_update_bits(map, reg, mask, mask)

and

regmap_clear_bits(map, reg, mask)
regmap_update_bits(map, reg, mask, 0)

with longer but functionally same variants - which kind of says "I think 
the "regmap_set_bits() and regmap_clear_bits()" are useless ;)

After saying this - I can use the regmap_update_bits() if you insist, 
but in my (not always so) humble opinion this does not improve the function.


> > +
> > +static int bu27008_set_drdy_irq(struct bu27008_data *data, bool state)
> > +{
> > +	if (state)
> > +		return regmap_set_bits(data->regmap, BU27008_REG_MODE_CONTROL3,
> > +					BU27008_MASK_INT_EN);
> > +	return regmap_clear_bits(data->regmap, BU27008_REG_MODE_CONTROL3,
> > +				 BU27008_MASK_INT_EN);
> regmap_update_bits() maybe with the mask and value supplied.

Same weak objection here as was with the bu27008_meas_set(). Eg, can 
change if required but please reconsider :)

> > +}
> > +
>
> > +static irqreturn_t bu27008_irq_handler(int irq, void *private)
> > +{
> > +	struct iio_dev *idev = private;
> > +	struct bu27008_data *data = iio_priv(idev);
> > +
> > +	data->old_timestamp = data->timestamp;
>
> What is old_timestamp for?  Without out setting that, this
> is the same as iio_pollfunc_store_time() with the timestamp
> stored in a slightly difference place and always waking the thread
> (which probably doesn't matter)

Thanks. I just re-used the logic from a driver which had some other 
options but the data-ready IRQ as well. As we don't have any such in 
bu27008, I think we can drop the custom stuff as you suggest and clean 
up this for quite a bit :) Thanks!


> > +
> > +static irqreturn_t bu27008_irq_thread_handler(int irq, void *private)
> > +{
> > +	struct iio_dev *idev = private;
> > +	struct bu27008_data *data = iio_priv(idev);
> > +	irqreturn_t ret = IRQ_NONE;
> > +
> > +	mutex_lock(&data->mutex);
> > +	if (data->trigger_enabled) {
> > +		iio_trigger_poll_nested(data->trig);
>
> Add a comment here on why it makes sense to hold the mutex whilst
> calling this.

After revising this - I don't think it makes. Nor do I think we need the 
trigger_enable flag so we don't propably need the mutex in buffer enable 
either as all raw-write configs are claiming the direct mode.

I'll cook the v2 soon(ish). Thanks!

Yours,
	--Matti


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

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