Messages in this thread | | | Date | Mon, 10 Apr 2023 13:38:16 +0300 | Subject | Re: [PATCH v7 4/5] iio: light: ROHM BU27034 Ambient Light Sensor | From | Matti Vaittinen <> |
| |
On 4/8/23 14:21, Jonathan Cameron wrote: > On Fri, 31 Mar 2023 15:41:58 +0300 > Matti Vaittinen <mazziesaccount@gmail.com> 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@gmail.com> > > 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! ~~
| |