On 4/8/23 14:21, Jonathan Cameron wrote:
On Fri, 31 Mar 2023 15:41:58 +0300
Matti Vaittinen <mazziesaccount@xxxxxxxxx> wrote:
ROHM BU27034 is an ambient light sensor with 3 channels and 3 photo diodes
capable of detecting a very wide range of illuminance. Typical application
is adjusting LCD and backlight power of TVs and mobile phones.
Add initial support for the ROHM BU27034 ambient light sensor.
NOTE:
- Driver exposes 4 channels. One IIO_LIGHT channel providing the
calculated lux values based on measured data from diodes #0 and
#1. In addition, 3 IIO_INTENSITY channels are emitting the raw
register data from all diodes for more intense user-space
computations.
- Sensor has GAIN values that can be adjusted from 1x to 4096x.
- Sensor has adjustible measurement times of 5, 55, 100, 200 and
400 mS. Driver does not support 5 mS which has special
limitations.
- Driver exposes standard 'scale' adjustment which is
implemented by:
1) Trying to adjust only the GAIN
2) If GAIN adjustment alone can't provide requested
scale, adjusting both the time and the gain is
attempted.
- Driver exposes writable INT_TIME property that can be used
for adjusting the measurement time. Time adjustment will also
cause the driver to try to adjust the GAIN so that the
overall scale is kept as close to the original as possible.
Signed-off-by: Matti Vaittinen <mazziesaccount@xxxxxxxxx>
A few trivial comments inline but nothing that stops me applying this version.
+static int bu27034_read_raw(struct iio_dev *idev,
+ struct iio_chan_spec const *chan,
+ int *val, int *val2, long mask)
+{
+ struct bu27034_data *data = iio_priv(idev);
+ int ret;
+
+ switch (mask) {
+ case IIO_CHAN_INFO_INT_TIME:
+ *val = bu27034_get_int_time(data);
+ if (*val < 0)
+ return *val;
+
+ return IIO_VAL_INT;
+
+ case IIO_CHAN_INFO_SCALE:
+ return bu27034_get_scale(data, chan->channel, val, val2);
+
+ case IIO_CHAN_INFO_RAW:
+ {
+ int (*result_get)(struct bu27034_data *data, int chan, int *val);
I don't care that much about this, but the function pointer dance seems
unnecessary.
My memory is getting worse and worse over the time... It's hard for me
to remember why I added a function pointer here - but it probably was to
keep the if-else if-else construct out of the mutex-protected section.
You could probably also drop the paranoid checking (or default to one of the
choices).
If this was done, then the function-pointer had no use. If you think the
check is paranoid, then I agree, we should go with the suggestion you
have below.
So at call sight of result_get() currently have
if (chan->type == IIO_INTENSITY)
ret = bu27034_get_single-result()...
else
ret = bu27034_get_mlux()
etc
meh. I didn't raise it earlier so I'll leave this alone.
+
+ if (chan->type == IIO_INTENSITY)
+ result_get = bu27034_get_single_result;
+ else if (chan->type == IIO_LIGHT)
+ result_get = bu27034_get_mlux;
+ else
+ return -EINVAL;
+
+ /* Don't mess with measurement enabling while buffering */
+ ret = iio_device_claim_direct_mode(idev);
+ if (ret)
+ return ret;
+
+ mutex_lock(&data->mutex);
+ /*
+ * Reading one channel at a time is inefficient but we
+ * don't care here. Buffered version should be used if
+ * performance is an issue.
+ */
+ ret = result_get(data, chan->channel, val);
+
+ mutex_unlock(&data->mutex);
+ iio_device_release_direct_mode(idev);
+
+ if (ret)
+ return ret;
+
+ return IIO_VAL_INT;
+ }
+ default:
+ return -EINVAL;
+
+ }
+}
I will fix the excess blank line above though. Serves no purpose
so gone in the version I applied.
Thanks!
Yours,
-- Matti
--
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland
~~ When things go utterly wrong vim users can always type :help! ~~
