Re: [PATCH v3 5/6] iio: light: ROHM BU27034 Ambient Light Sensor
From: Matti Vaittinen
Date: Mon Mar 13 2023 - 09:34:40 EST
On 3/12/23 19:39, Jonathan Cameron wrote:
On Mon, 6 Mar 2023 11:23:50 +0200
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>
---
Please note: There are few "unfinished" discussions regarding at least:
- PROCESSED channel scale.
Hopefully the reply in the other thread covered this.
It's not PROCESSED as you need to apply the * 1000 so just
make it _RAW.
Your reply in the other thread was just fine :) I think I'll round the
result to luxes (which should be sufficient to many users if I am not
mistaken) - and provide the PROCESSED values to user-space for the sake
of the ultimate simplicity for users.
- Implementation details when avoiding division by zero.
I have implemented those for this version in a way that I see the best.
It would have been better to wait for discussions to finish - but I will
be away from the computer for a week - so I wanted to send out the v3
with fixes so that people who are interested can do a review while I am
away.
I'm getting behind with review anyway so a week delay on this sounds great to
me ;) I might get to your other series as a result though don't think I'll get there today.
Oh, the week elapsed already ;) But don't sweat on it - this series grew
quite large so spending time on reviewing makes perfect sense. I _know_
reviewing, and especially careful reviewing, takes time. And energy.
Sometimes we lack of both, occasionally just the other :) Thanks for all
the effort you and Andy have put on this so far!
+static int bu27034_calc_mlux(struct bu27034_data *data, __le16 *res, int *val)
+{
+ unsigned int gain0, gain1, meastime;
+ unsigned int d1_d0_ratio_scaled;
+ u16 ch0, ch1;
+ u64 helper64;
+ int ret;
+
+ /*
+ * We return 0 luxes if calculation fails. This should be reasonably
+ * easy to spot from the buffers especially if raw-data channels show
+ * valid values
+ */
+ *val = 0;
+
+ /* Avoid div by zero. */
+ if (!res[0])
+ ch0 = 1;
+ else
+ ch0 = le16_to_cpu(res[0]);
+
+ if (!res[1])
+ ch1 = 1;
+ else
+ ch1 = le16_to_cpu(res[1]);
+
As per other thread. Really don't like the check on 0 before
the endian conversion. Sure it can be done, but it's a micro optimization that
seems unnecessary.
Ugh... Andy pointed me the max_t(). I'll use it even though I do really
dislike that change a lot.
...
+
+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;
+
+ /*
+ * We use 50000 uS internally for all calculations and only
+ * convert it to 55000 before returning it to the user.
+ *
+ * This is because the data-sheet says the time is 55 mS - but
+ * vendor provided computations used 50 mS.
No chance of a clarification? would be lovely to not do this!
Actually, since the current code uses the multiplier-field in
gts-helpers - it may be we can just use the 55000 in the tables now. I
need to check it. So perhaps we can avoid this one now! Thanks!
+ */
+ if (*val == 50000)
+ *val = 55000;
+
+ return IIO_VAL_INT;
+
+ case IIO_CHAN_INFO_SCALE:
+ return bu27034_get_scale(data, chan->channel, val, val2);
+
+ case IIO_CHAN_INFO_RAW:
+ {
+ if (chan->type != IIO_INTENSITY)
+ return -EINVAL;
+
+ if (chan->channel < BU27034_CHAN_DATA0 ||
+ chan->channel > BU27034_CHAN_DATA2)
+ 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 ineffiecient but we don't
spell check comments.
+ * care here. Buffered version should be used if performance is
+ * an issue.
+ */
+ ret = bu27034_get_single_result(data, chan->channel, val);
+
+ mutex_unlock(&data->mutex);
+ iio_device_release_direct_mode(idev);
+
+ if (ret)
+ return ret;
+
+ return IIO_VAL_INT;
+ }
+
+ case IIO_CHAN_INFO_PROCESSED:
+ if (chan->type != IIO_LIGHT)
+ 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);
+
+ ret = bu27034_get_mlux(data, val);
+
+ mutex_unlock(&data->mutex);
+ iio_device_release_direct_mode(idev);
+
+ if (ret)
+ return ret;
+
+ return IIO_VAL_INT;
+
+ default:
+ return -EINVAL;
+
+ }
+
+ return ret;
I would hope you can't get here. If you can fix that so
that the lack of return here allows the compiler to know you didn't
intend to get here and hence complain about it.
Sigh. I personally still think functions should return from the end...
Well, I will revise this for v4.
+static int bu27034_probe(struct i2c_client *i2c)
+{
+ struct device *dev = &i2c->dev;
+ struct bu27034_data *data;
+ struct regmap *regmap;
+ struct iio_dev *idev;
+ unsigned int part_id, reg;
+ int ret;
+
+ regmap = devm_regmap_init_i2c(i2c, &bu27034_regmap);
+ if (IS_ERR(regmap))
+ return dev_err_probe(dev, PTR_ERR(regmap),
+ "Failed to initialize Regmap\n");
+
+ idev = devm_iio_device_alloc(dev, sizeof(*data));
+ if (!idev)
+ return -ENOMEM;
+
+ ret = devm_regulator_get_enable(dev, "vdd");
+ if (ret && ret != -ENODEV)
Why the special -ENODEV handling?
You should get a stub regulator if one isn't provided by firmware.
If you don't get a stub, or a real regulator that's a failure so we should
return the error code and fail the probe.
I am actually not sure. Maybe my test setup didn't have the dummy
regulator assigned. I will re-check this.
The comments which I just skipped are something I more or less agree with :)
Best Regards
-- Matti
--
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland
~~ When things go utterly wrong vim users can always type :help! ~~