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