On 28/2/24 23:38, Matti Vaittinen wrote:
On 2/28/24 14:24, Subhajit Ghosh wrote:
Driver support for Avago (Broadcom) APDS9306 Ambient Light Sensor.
It has two channels - ALS and CLEAR. The ALS (Ambient Light Sensor)
channel approximates the response of the human-eye providing direct
read out where the output count is proportional to ambient light levels.
It is internally temperature compensated and rejects 50Hz and 60Hz flicker
caused by artificial light sources. Hardware interrupt configuration is
optional. It is a low power device with 20 bit resolution and has
configurable adaptive interrupt mode and interrupt persistence mode.
The device also features inbuilt hardware gain, multiple integration time
selection options and sampling frequency selection options.
This driver also uses the IIO GTS (Gain Time Scale) Helpers Namespace for
Scales, Gains and Integration time implementation.
Signed-off-by: Subhajit Ghosh <subhajit.ghosh@xxxxxxxxxxxxxx>
---
v7 -> v8:
- Renamed APDS9306_INT_CH_CLEAR to APDS9306_INT_SRC_CLEAR macro for higher
readability
- Removed APDS9306_CHANNEL macro for higher readability
- Updated iio_push_event() functions with correct type of events (Light or Intensity)
- Updated variable name "event_ch_is_light" to "int_src" and change as per
review to fix compiler warning
- Used scope for guard() functions
- Other fixes as per reviews
https://lore.kernel.org/all/20240224151340.3f2f51e8@jic23-huawei/
https://lore.kernel.org/all/ZdycR6nr3rtrnuth@xxxxxxxxxxxxxxxxxx/
...
Hi Matti,
---
Hi Subhajit,
I just happened to notice couple of minor things. I see the series is already in a v8 and don't want to cause extra re-spins. So, perhaps consider these points if you need to do v9 but I am sending these only as 'nits'. I don't think any of my findings are very serious.
Thank for reviewing. I will do as many re-spins as it takes to get things correct if required.
It is best possible source of learning for me.
...
+static int apds9306_intg_time_set(struct apds9306_data *data, int val2)
+{
+ struct device *dev = data->dev;
+ struct apds9306_regfields *rf = &data->rf;
+ int ret, intg_old, gain_old, gain_new, gain_new_closest, intg_time_idx;
+ int gain_idx;
+ bool ok;
+
+ if (!iio_gts_valid_time(&data->gts, val2)) {
+ dev_err_ratelimited(dev, "Unsupported integration time %u\n", val2);
+ return -EINVAL;
+ }
+
+ ret = regmap_field_read(rf->intg_time, &intg_time_idx);
+ if (ret)
+ return ret;
+
+ ret = regmap_field_read(rf->gain, &gain_idx);
+ if (ret)
+ return ret;
+
+ intg_old = iio_gts_find_int_time_by_sel(&data->gts, intg_time_idx);
+ if (ret < 0)
+ return ret;
+
+ if (intg_old == val2)
+ return 0;
+
+ gain_old = iio_gts_find_gain_by_sel(&data->gts, gain_idx);
+ if (gain_old < 0)
+ return gain_old;
+
+ ret = iio_gts_find_new_gain_by_old_gain_time(&data->gts, gain_old,
+ intg_old, val2, &gain_new);
You don't use the 'ret' here, so maybe for the clarity, not assign it.
Or, maybe you wan't to try to squeeze out few cycles for succesful case and check the ret for '0' - in which case you should be able to omit the check right below as well as the call to iio_find_closest_gain_low(). OTOH, this is likely not a "hot path" so I don't care too much about the extra call if you think code is clearer this way.
I will stick to the first option and remove the unused ret. The code looks linear and clearer
that way. Although it depends upon further reviews.
Regards,
Subhajit Ghosh
+ if (gain_new < 0) {
+ dev_err_ratelimited(dev, "Unsupported gain with time\n");
+ return gain_new;
+ }
+
+ gain_new_closest = iio_find_closest_gain_low(&data->gts, gain_new, &ok);
+ if (gain_new_closest < 0) {
+ gain_new_closest = iio_gts_get_min_gain(&data->gts);
+ if (gain_new_closest < 0)
+ return gain_new_closest;
+ }
+ if (!ok)
+ dev_dbg(dev, "Unable to find optimum gain, setting minimum");
+
+ ret = iio_gts_find_sel_by_int_time(&data->gts, val2);
+ if (ret < 0)
+ return ret;
+
+ ret = regmap_field_write(rf->intg_time, ret);
+ if (ret)
+ return ret;
+
+ ret = iio_gts_find_sel_by_gain(&data->gts, gain_new_closest);
+ if (ret < 0)
+ return ret;
+
+ return regmap_field_write(rf->gain, ret);
+}
