On Fri, Oct 27, 2023 at 01:05:32AM +1030, Subhajit Ghosh wrote:
> Driver support for Avago (Broadcom) APDS9306 Ambient Light Sensor with als
WTH "als" is? Always think about reader of your commit message. It should be
clear to the unprepared reader.
> and clear channels with i2c interface. 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.
> v0 -> v1
> - Fixed errors as per previous review
> - Longer commit messages and descriptions
> - Updated scale calculations as per iio gts scheme to export proper scale
> values and tables to userspace
> - Removed processed attribute for the same channel for which raw is
> provided, instead, exporting proper scale and scale table to userspace so
> that userspace can do "(raw + offset) * scale" and derive Lux values
> - Fixed IIO attribute range syntax
> - Keeping the regmap lock enabled as the driver uses unlocked regfield
> accesses from interrupt handler
> - Several levels of cleanups by placing guard mutexes in proper places and
> returning immediately in case of an error
> - Using iio_device_claim_direct_mode() during raw reads so that
> configurations could not be changed during an adc conversion period
> - In case of a powerdown error, returning immediately
> - Removing the definition of direction of the hardware interrupt and
> leaving it on to device tree
> - Adding the powerdown callback after doing device initialization
> - Removed the regcache_cache_only() implementation
This is wrong location for the changelog...
> Signed-off-by: Subhajit Ghosh <subhajit.ghosh@xxxxxxxxxxxxxx>
> ---
...should go here, after cutter '---' line.
...
Missing bits.h.
Also array_size.h is pending for v6.7-rc1.
> +#include <linux/delay.h>
> +#include <linux/err.h>
> +#include <linux/i2c.h>
> +#include <linux/interrupt.h>
> +#include <linux/module.h>
> +#include <linux/mutex.h>
> +#include <linux/pm.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/regmap.h>
> +#include <linux/regulator/consumer.h>
Missing types.h.
> +#include <linux/units.h>
...
> +#define APDS9306_ALS_THRES_VAL_MAX 0xFFFFF
It seems like the limit is hardware based, then probably better to mark it via
(BIT(20) - 1) to show this limitation explicitly.
...
> +enum apds9306_power_states {
> + STANDBY,
> + ACTIVE,
Perhaps namespace?
> +};
...
> +/**
> + * struct part_id_gts_multiplier - Part no. & corresponding gts multiplier
GTS? WTH "GTS" is, btw? (see comment on the commit message)
> + * @part_id: Part ID of the device
> + * @max_scale_int: Multiplier for iio_init_iio_gts()
> + * @max_scale_nano: Multiplier for iio_init_iio_gts()
> + */
...
> +/**
Hmm... Do we really need this in kernel doc?
Ditto for the similar cases.
> + * apds9306_repeat_rate_freq - Sampling Frequency in uHz
> + */
...
> +static const int apds9306_repeat_rate_period[] = {
> + 25000, 50000, 100000, 200000, 500000, 1000000, 2000000
Leave the trailing comma.
> +};
> +static_assert(ARRAY_SIZE(apds9306_repeat_rate_freq) ==
> + ARRAY_SIZE(apds9306_repeat_rate_period));
Instead you may add a new definition and use it in [] in the respective arrays.
...
> +/**
> + * struct apds9306_data - apds9306 private data and registers definitions
> + * All regfield definitions are named exactly according to datasheet for easy
> + * search
I'm not sure this comment adds any value.
> + * @dev: Pointer to the device structure
> + * @gts: IIO Gain Time Scale structure
> + * @mutex: Lock for protecting register access, adc reads and power
> + * @regmap: Regmap structure pointer
> + * @regfield_sw_reset: Reg: MAIN_CTRL, Field: SW_Reset
> + * @regfield_en: Reg: MAIN_CTRL, Field: ALS_EN
> + * @regfield_intg_time: Reg: ALS_MEAS_RATE, Field: ALS Resolution/Bit Width
> + * @regfield_repeat_rate: Reg: ALS_MEAS_RATE, Field: ALS Measurement Rate
> + * @regfield_scale: Reg: ALS_GAIN, Field: ALS Gain Range
> + * @regfield_int_src: Reg: INT_CFG, Field: ALS Interrupt Source
> + * @regfield_int_thresh_var_en: Reg: INT_CFG, Field: ALS Var Interrupt Mode
> + * @regfield_int_en: Reg: INT_CFG, Field: ALS Interrupt Enable
> + * @regfield_int_persist_val: Reg: INT_PERSISTENCE, Field: ALS_PERSIST
> + * @regfield_int_thresh_var_val: Reg: ALS_THRSH_VAR, Field: ALS_THRES_VAR
> + * @nlux_per_count: nano lux per ADC count for a particular model
> + * @read_data_available: Flag set by IRQ handler for ADC data available
> + * @intg_time_idx: Array index for integration times
> + * @repeat_rate_idx: Array index for sampling frequency
> + * @gain_idx: Array index for gain
> + * @int_ch: Currently selected Interrupt channel
> + */
...
> +static const struct iio_itime_sel_mul apds9306_itimes[] = {
> + GAIN_SCALE_ITIME_US(400000, APDS9306_MEAS_MODE_400MS, 128),
> + GAIN_SCALE_ITIME_US(200000, APDS9306_MEAS_MODE_200MS, 64),
> + GAIN_SCALE_ITIME_US(100000, APDS9306_MEAS_MODE_100MS, 32),
> + GAIN_SCALE_ITIME_US(50000, APDS9306_MEAS_MODE_50MS, 16),
> + GAIN_SCALE_ITIME_US(25000, APDS9306_MEAS_MODE_25MS, 8),
> + GAIN_SCALE_ITIME_US(3125, APDS9306_MEAS_MODE_3125US, 1),
Hmm... Maybe BIT() in all values?
> +};
> +static const struct attribute_group apds9306_event_attr_group = {
> + .attrs = apds9306_event_attributes,
> +};
...
> + data->regfield_intg_time = devm_regmap_field_alloc(dev, regmap,
> + apds9306_regfield_intg_time);
> + if (IS_ERR(data->regfield_intg_time))
> + return PTR_ERR(data->regfield_intg_time);
> +
> + data->regfield_repeat_rate = devm_regmap_field_alloc(dev, regmap,
> + apds9306_regfield_repeat_rate);
> + if (IS_ERR(data->regfield_repeat_rate))
> + return PTR_ERR(data->regfield_repeat_rate);
> +
> + data->regfield_scale = devm_regmap_field_alloc(dev, regmap,
> + apds9306_regfield_scale);
> + if (IS_ERR(data->regfield_scale))
> + return PTR_ERR(data->regfield_scale);
> +
> + data->regfield_int_src = devm_regmap_field_alloc(dev, regmap,
> + apds9306_regfield_int_src);
> + if (IS_ERR(data->regfield_int_src))
> + return PTR_ERR(data->regfield_int_src);
> +
> + data->regfield_int_thresh_var_en = devm_regmap_field_alloc(dev, regmap,
> + apds9306_regfield_int_thresh_var_en);
> + if (IS_ERR(data->regfield_int_thresh_var_en))
> + return PTR_ERR(data->regfield_int_thresh_var_en);
> +
> + data->regfield_int_en = devm_regmap_field_alloc(dev, regmap,
> + apds9306_regfield_int_en);
> + if (IS_ERR(data->regfield_int_en))
> + return PTR_ERR(data->regfield_int_en);
> +
> + data->regfield_int_persist_val = devm_regmap_field_alloc(dev, regmap,
> + apds9306_regfield_int_persist_val);
> + if (IS_ERR(data->regfield_int_persist_val))
> + return PTR_ERR(data->regfield_int_en);
> +
> + data->regfield_int_thresh_var_val = devm_regmap_field_alloc(dev, regmap,
> + apds9306_regfield_int_thresh_var_val);
> + if (IS_ERR(data->regfield_int_thresh_var_val))
> + return PTR_ERR(data->regfield_int_thresh_var_en);
Instead I would rather do with a temporary variable all of these...
tmp = devm_regmap_field_alloc(dev, regmap, apds9306_regfield_int_thresh_var_val);
if (IS_ERR(tmp)
return PTR_ERR(tmp);
data->regfield_int_thresh_var_val = tmp;
...
> +static int apds9306_power_state(struct apds9306_data *data,
> + enum apds9306_power_states state)
> +{
> + int ret;
> +
> + /* Reset not included as it causes ugly I2C bus error */
> + switch (state) {
> + case STANDBY:
> + return regmap_field_write(data->regfield_en, 0);
> + case ACTIVE:
> + ret = regmap_field_write(data->regfield_en, 1);
> + if (ret)
> + return ret;
> + /* 5ms wake up time */
> + usleep_range(5000, 10000);
fsleep()
> + return 0;
> + default:
> + return -EINVAL;
> + }
> +}
...
> +static int apds9306_runtime_power(struct apds9306_data *data, int en)
> +{
> + struct device *dev = data->dev;
> + int ret;
> +
> + if (en) {
> + ret = pm_runtime_resume_and_get(dev);
> + if (ret < 0) {
> + dev_err(dev, "runtime resume failed: %d\n", ret);
> + return ret;
> + }
> + return 0;
> + }
> +
> + pm_runtime_mark_last_busy(dev);
> + pm_runtime_put_autosuspend(dev);
> + return 0;
> +}
Wouldn't be better to have something like
static int apds9306_runtime_power_on(struct device *dev)
{
int ret;
ret = pm_runtime_resume_and_get(dev);
if (ret < 0)
dev_err(dev, "runtime resume failed: %d\n", ret);
return ret;
}
static int apds9306_runtime_power_off(struct device *dev)
{
pm_runtime_mark_last_busy(dev);
pm_runtime_put_autosuspend(dev);
return 0;
}
// Not sure you will even need this one
static int apds9306_runtime_power(struct apds9306_data *data, bool en)
{
struct device *dev = data->dev;
if (en)
return apds9306_runtime_power_on(dev);
return apds9306_runtime_power_off(dev);
}
?
...
> +static int apds9306_read_data(struct apds9306_data *data, int *val, int reg)
> +{
> + struct device *dev = data->dev;
> + struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> + int ret, delay, intg_time, status = 0;
> + u8 buff[3];
> +
> + ret = apds9306_runtime_power(data, 1);
ret = apds9306_runtime_power_on(dev);
> + if (ret)
> + return ret;
> +
> + intg_time = iio_gts_find_int_time_by_sel(&data->gts,
> + data->intg_time_idx);
> + if (intg_time < 0)
> + delay = apds9306_repeat_rate_period[data->repeat_rate_idx];
> +
> + /*
> + * Whichever is greater - integration time period or
> + * sampling period.
> + */
> + delay = max(intg_time,
> + apds9306_repeat_rate_period[data->repeat_rate_idx]);
> +
> +
One blank line is enough.
> + /*
> + * Clear stale data flag that might have been set by the interrupt
> + * handler if it got data available flag set in the status reg.
> + */
> + data->read_data_available = 0;
> +
> + /*
> + * If this function runs parallel with the interrupt handler, either
> + * this reads and clears the status registers or the interrupt handler
> + * does. The interrupt handler sets a flag for read data available
> + * in our private structure which we read here.
> + */
> + ret = regmap_read_poll_timeout(data->regmap, APDS9306_MAIN_STATUS,
> + status, (status & (APDS9306_ALS_DATA_STAT_MASK |
> + APDS9306_ALS_INT_STAT_MASK)) ||
> + data->read_data_available,
> + APDS9306_ALS_READ_DATA_DELAY_US, delay * 2);
> +
Redundant blank line.
> + if (ret)
> + return ret;
> +
> + /* If we reach here before the interrupt handler we push an event */
> + if ((status & APDS9306_ALS_INT_STAT_MASK)) {
> + iio_push_event(indio_dev, IIO_UNMOD_EVENT_CODE(IIO_LIGHT,
> + data->int_ch,
> + IIO_EV_TYPE_THRESH, IIO_EV_DIR_EITHER),
> + iio_get_time_ns(indio_dev));
> + }
> + if ((status & APDS9306_ALS_DATA_STAT_MASK) ||
> + data->read_data_available) {
Wrong indentation, perhaps put them on one line?
> + ret = regmap_bulk_read(data->regmap, reg, buff, sizeof(buff));
> + if (ret) {
> + dev_err(dev, "read data failed\n");
> + return ret;
> + }
> + *val = get_unaligned_le24(&buff);
> + }
> +
> + return apds9306_runtime_power(data, 0);
return apds9306_runtime_power_off(dev);
> +}
...
> + 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 < 0;
Typo?
> + }
...
> +static int apds9306_sampling_freq_set(struct apds9306_data *data, int val,
> + int val2)
> +{
> + int i, ret = -EINVAL;
Use value directly.
> +
> + for (i = 0; i < ARRAY_SIZE(apds9306_repeat_rate_freq); i++)
> + if (apds9306_repeat_rate_freq[i][0] == val &&
> + apds9306_repeat_rate_freq[i][1] == val2) {
Wrong indentation.
> + ret = regmap_field_write(data->regfield_repeat_rate, i);
> + if (ret)
> + return ret;
> + data->repeat_rate_idx = i;
> + return ret;
You meant break here or return 0; ?
> + }
> +
> + return ret;
> +}
You can rewrite this as
unsigned int i;
int ret;
for (i = 0; i < ARRAY_SIZE(apds9306_repeat_rate_freq); i++) {
if (apds9306_repeat_rate_freq[i][0] == val &&
apds9306_repeat_rate_freq[i][1] == val2)
break;
}
if (i == ARRAY_SIZE(apds9306_repeat_rate_freq))
return -EINVAL;
ret = regmap_field_write(data->regfield_repeat_rate, i);
if (ret)
return ret;
data->repeat_rate_idx = i;
return 0;
...which is easier to read and understand.
...
> + ret = iio_gts_find_gain_sel_for_scale_using_time(&data->gts,
> + data->intg_time_idx, val, val2, &gain_sel);
> + if (ret) {
> + for (i = 0; i < data->gts.num_itime; i++) {
> + time_sel = data->gts.itime_table[i].sel;
> +
> + if (time_sel == data->intg_time_idx)
> + continue;
> +
> + ret = iio_gts_find_gain_sel_for_scale_using_time(&data->gts,
> + time_sel, val, val2, &gain_sel);
> + if (!ret)
> + break;
> + }
> + if (ret)
> + return -EINVAL;
> +
> + ret = apds9306_intg_time_set_hw(data, time_sel);
> + if (ret)
> + return ret;
Looks like a candidate for a helper function.
> + }
...
> + if (val < 0 || val > APDS9306_ALS_PERSIST_VAL_MAX)
in_range()
> + return -EINVAL;
...
> +static int apds9306_event_thresh_get(struct apds9306_data *data, int dir,
> + int *val)
> +{
> + int var, ret;
> + u8 buff[3];
> +
> + if (dir == IIO_EV_DIR_RISING)
> + var = APDS9306_ALS_THRES_UP_0;
> + else if (dir == IIO_EV_DIR_FALLING)
> + var = APDS9306_ALS_THRES_LOW_0;
> + else
> + return -EINVAL;
> + ret = regmap_bulk_read(data->regmap, var, buff, sizeof(buff));
> + if (ret)
> + return ret;
> + *val = get_unaligned_le24(&buff);
> + return 0;
In some cases you put blank line(s) in between, in some seems not. Please,
be consistent with your style for whatever it is: blank lines, comments,
indentation, ...
> +}
...
> + if (val < 0 || val > APDS9306_ALS_THRES_VAL_MAX)
in_range()
> + return -EINVAL;
...
> + if (val < 0 || val > APDS9306_ALS_THRES_VAR_VAL_MAX)
Ditto.
> + return -EINVAL;
...
> + return iio_gts_all_avail_scales(&data->gts, vals, type,
> + length);
It's exactly 80 character if on a single line, why wrapped?
...
> + mutex_lock(&data->mutex);
You can use guard() from cleanup.h for this kind of stuff to make your life and
maintenance easier.
> + switch (mask) {
> + case IIO_CHAN_INFO_INT_TIME:
> + if (!val)
What's wrong with positive check and even more with the usual pattern -- check
for errors first?
> + ret = apds9306_intg_time_set(data, val2);
> + else
> + ret = -EINVAL;
> + break;
With the above (i.e guard() use) this will become as simple as
if (val)
return -EINVAL;
return apds9306_intg_time_set(data, val2);
> + case IIO_CHAN_INFO_SCALE:
> + ret = apds9306_scale_set(data, val, val2);
> + break;
> + case IIO_CHAN_INFO_SAMP_FREQ:
> + ret = apds9306_sampling_freq_set(data, val, val2);
> + break;
> + default:
> + ret = -EINVAL;
> + }
> + mutex_unlock(&data->mutex);
...
> + switch (type) {
> + case IIO_EV_TYPE_THRESH:
> + mutex_lock(&data->mutex);
> + if (dir == IIO_EV_DIR_EITHER && info == IIO_EV_INFO_PERIOD)
> + ret = apds9306_event_period_get(data, val);
> + else
> + ret = apds9306_event_thresh_get(data, dir, val);
> + mutex_unlock(&data->mutex);
> + if (ret)
> + return ret;
> + return IIO_VAL_INT;
> + case IIO_EV_TYPE_THRESH_ADAPTIVE:
> + mutex_lock(&data->mutex);
> + ret = apds9306_event_thresh_adaptive_get(data, val);
> + mutex_unlock(&data->mutex);
> + if (ret)
> + return ret;
> + return IIO_VAL_INT;
> + default:
> + return -EINVAL;
> + }
This will benefit from guard() or scoped_guard().
And many other functions in your driver.
I believe ~15% of LoCs can be dropped with help of cleanup.h.
...
> +#define APDS9306_IIO_INFO \
> + .read_avail = apds9306_read_avail, \
> + .read_raw = apds9306_read_raw, \
> + .write_raw = apds9306_write_raw, \
> + .write_raw_get_fmt = apds9306_write_raw_get_fmt,
Not using this macro will only add 1 LoC, but will be much easier to read.
...
> + return dev_err_probe(dev, ret,
> + "failed to assign interrupt.\n");
Indentation issue.
...
> + return dev_err_probe(dev, ret,
> + "failed to add action on driver unwind\n");
Ditto.
...
> +
Unneeded blank line.
> +module_i2c_driver(apds9306_driver);
--
With Best Regards,
Andy Shevchenko
