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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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! ~~




[Index of Archives]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Input]     [Linux Kernel]     [Linux SCSI]     [X.org]

  Powered by Linux