Le 21/01/2024 à 06:17, Subhajit Ghosh a écrit :
> 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 <[email protected]>
> ---
Hi,
a few nits and a few real comment/question below.
Just my 2c.
CJ
..
> +#define APDS9306_ALS_THRES_VAL_MAX (BIT(20) - 1)
> +#define APDS9306_ALS_THRES_VAR_VAL_MAX (BIT(3) - 1)
> +#define APDS9306_ALS_PERSIST_VAL_MAX (BIT(4) - 1)
Nit: GENMASK()?
> +#define APDS9306_ALS_READ_DATA_DELAY_US 20000
> +#define APDS9306_NUM_REPEAT_RATES 7
..
> +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, intg_time_idx, repeat_rate_idx, int_src;
> + int status = 0;
> + u8 buff[3];
> +
> + ret = apds9306_runtime_power_on(data->dev);
> + if (ret)
> + return ret;
> +
> + ret = regmap_field_read(data->regfield_intg_time, &intg_time_idx);
> + if (ret)
> + return ret;
> +
> + ret = regmap_field_read(data->regfield_repeat_rate, &repeat_rate_idx);
> + if (ret)
> + return ret;
> +
> + ret = regmap_field_read(data->regfield_int_src, &int_src);
> + if (ret)
> + return ret;
> +
> + intg_time = iio_gts_find_int_time_by_sel(&data->gts, intg_time_idx);
> + if (intg_time < 0)
> + delay = apds9306_repeat_rate_period[repeat_rate_idx];
'delay' is always overwritten by the line below.
> +
> + /*
> + * Whichever is greater - integration time period or
> + * sampling period.
> + */
> + delay = max(intg_time,
> + apds9306_repeat_rate_period[repeat_rate_idx]);
> +
> + /*
> + * 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_REG,
> + status, (status & (APDS9306_ALS_DATA_STAT_MASK |
> + APDS9306_ALS_INT_STAT_MASK)) ||
> + data->read_data_available,
> + APDS9306_ALS_READ_DATA_DELAY_US, delay * 2);
> + 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, int_src,
> + IIO_EV_TYPE_THRESH, IIO_EV_DIR_EITHER),
> + iio_get_time_ns(indio_dev));
> + }
> +
> + ret = regmap_bulk_read(data->regmap, reg, buff, sizeof(buff));
> + if (ret) {
> + dev_err(dev, "read data failed\n");
Would dev_err_ratelimited() make sense here?
> + return ret;
> + }
> +
> + *val = get_unaligned_le24(&buff);
> +
> + return apds9306_runtime_power_off(dev);
> +}
..
> +static int apds9306_intg_time_set(struct apds9306_data *data, int val2)
> +{
> + struct device *dev = data->dev;
> + 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(dev, "Unsupported integration time %u\n", val2);
Would dev_err_ratelimited() make sense here?
> + return -EINVAL;
> + }
> +
> + ret = regmap_field_read(data->regfield_intg_time, &intg_time_idx);
> + if (ret)
> + return ret;
> +
> + ret = regmap_field_read(data->regfield_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);
> + if (gain_new < 0) {
> + dev_err(dev, "Unsupported gain with time\n");
Would dev_err_ratelimited() make sense here?
> + 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(data->regfield_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(data->regfield_gain, ret);
> +}
..
> +static int apds9306_event_period_get(struct apds9306_data *data, int *val)
> +{
> + int period, ret;
> +
> + ret = regmap_field_read(data->regfield_int_persist_val, &period);
> + if (ret)
> + return ret;
> +
> + if (period > APDS9306_ALS_PERSIST_VAL_MAX)
Nit: in_range() to be consistent with code below?
> + return -EINVAL;
> +
> + *val = period;
> +
> + return ret;
> +}
> +
> +static int apds9306_event_period_set(struct apds9306_data *data, int val)
> +{
> + if (!in_range(val, 0, APDS9306_ALS_PERSIST_VAL_MAX))
> + return -EINVAL;
> +
> + return regmap_field_write(data->regfield_int_persist_val, val);
> +}
..
> +static int apds9306_event_thresh_set(struct apds9306_data *data, int dir,
> + int val)
> +{
> + int var;
> + u8 buff[3];
> +
> + if (dir == IIO_EV_DIR_RISING)
> + var = APDS9306_ALS_THRES_UP_0_REG;
> + else if (dir == IIO_EV_DIR_FALLING)
> + var = APDS9306_ALS_THRES_LOW_0_REG;
> + else
> + return -EINVAL;
> +
> + if (!in_range(val, 0, APDS9306_ALS_THRES_VAL_MAX))
> + return -EINVAL;
> +
> + put_unaligned_le24(val, buff);
> +
> + return regmap_bulk_write(data->regmap, var, buff, sizeof(buff));
> +}
> +
> +static int apds9306_event_thresh_adaptive_get(struct apds9306_data *data,
> + int *val)
> +{
> + int thr_adpt, ret;
> +
> + ret = regmap_field_read(data->regfield_int_thresh_var_val, &thr_adpt);
> + if (ret)
> + return ret;
> +
> + if (thr_adpt > APDS9306_ALS_THRES_VAR_VAL_MAX)
Nit: in_range()? to be consistent with code below and above.
> + return -EINVAL;
> +
> + *val = thr_adpt;
> +
> + return ret;
> +}
> +
> +static int apds9306_event_thresh_adaptive_set(struct apds9306_data *data,
> + int val)
> +{
> + if (!in_range(val, 0, APDS9306_ALS_THRES_VAR_VAL_MAX))
> + return -EINVAL;
> +
> + return regmap_field_write(data->regfield_int_thresh_var_val, val);
> +}
..
> +static irqreturn_t apds9306_irq_handler(int irq, void *priv)
> +{
> + struct iio_dev *indio_dev = priv;
> + struct apds9306_data *data = iio_priv(indio_dev);
> + int ret, status, int_ch;
> +
> + /*
> + * The interrupt line is released and the interrupt flag is
> + * cleared as a result of reading the status register. All the
> + * status flags are cleared as a result of this read.
> + */
> + ret = regmap_read(data->regmap, APDS9306_MAIN_STATUS_REG, &status);
> + if (ret < 0) {
> + dev_err(data->dev, "status reg read failed\n");
Would dev_err_ratelimited() make sense here?
> + return IRQ_HANDLED;
> + }
> +
> + ret = regmap_field_read(data->regfield_int_src, &int_ch);
> + if (ret)
> + return ret;
> +
> + if ((status & APDS9306_ALS_INT_STAT_MASK)) {
> + iio_push_event(indio_dev, IIO_UNMOD_EVENT_CODE(IIO_LIGHT, int_ch,
> + IIO_EV_TYPE_THRESH, IIO_EV_DIR_EITHER),
> + iio_get_time_ns(indio_dev));
> + }
Nit: superfluous {}
> +
> + /*
> + * If a one-shot read through sysfs is underway at the same time
> + * as this interrupt handler is executing and a read data available
> + * flag was set, this flag is set to inform read_poll_timeout()
> + * to exit.
> + */
> + if ((status & APDS9306_ALS_DATA_STAT_MASK))
> + data->read_data_available = 1;
> +
> + return IRQ_HANDLED;
> +}
> +
> +static int apds9306_read_event(struct iio_dev *indio_dev,
> + const struct iio_chan_spec *chan,
> + enum iio_event_type type,
> + enum iio_event_direction dir,
> + enum iio_event_info info,
> + int *val, int *val2)
> +{
> + struct apds9306_data *data = iio_priv(indio_dev);
> + int ret;
Other functions below that look really similar have a:
guard(mutex)(&data->mutex);
Is it needed here?
> +
> + switch (type) {
> + case IIO_EV_TYPE_THRESH:
> + 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);
> + if (ret)
> + return ret;
> + return IIO_VAL_INT;
> + case IIO_EV_TYPE_THRESH_ADAPTIVE:
> + ret = apds9306_event_thresh_adaptive_get(data, val);
> + if (ret)
> + return ret;
> + return IIO_VAL_INT;
> + default:
> + return -EINVAL;
> + }
> +}
..
On Sun, Jan 21, 2024 at 10:22:50AM +0100, Christophe JAILLET wrote:
> Le 21/01/2024 ? 06:17, Subhajit Ghosh a ?crit?:
..
> > +#define APDS9306_ALS_THRES_VAL_MAX (BIT(20) - 1)
> > +#define APDS9306_ALS_THRES_VAR_VAL_MAX (BIT(3) - 1)
> > +#define APDS9306_ALS_PERSIST_VAL_MAX (BIT(4) - 1)
>
> Nit: GENMASK()?
I think no, these are plain numbers given in the form of BIT(x) - 1 to show the
HW limit on the values (in bits).
..
> > + intg_time = iio_gts_find_int_time_by_sel(&data->gts, intg_time_idx);
> > + if (intg_time < 0)
> > + delay = apds9306_repeat_rate_period[repeat_rate_idx];
>
> 'delay' is always overwritten by the line below.
Seems like entire conditional here is not needed as it's implied in the max().
And max() should take both signed or unsigned types, different types might have
a side effect.
> > + /*
> > + * Whichever is greater - integration time period or
> > + * sampling period.
> > + */
> > + delay = max(intg_time,
> > + apds9306_repeat_rate_period[repeat_rate_idx]);
--
With Best Regards,
Andy Shevchenko
On 21/1/24 23:22, Andy Shevchenko wrote:
> On Sun, Jan 21, 2024 at 10:22:50AM +0100, Christophe JAILLET wrote:
> Seems like entire conditional here is not needed as it's implied in the max().
> And max() should take both signed or unsigned types, different types might have
> a side effect.
Ok, understood.
>
>>> + /*
>>> + * Whichever is greater - integration time period or
>>> + * sampling period.
>>> + */
>>> + delay = max(intg_time,
>>> + apds9306_repeat_rate_period[repeat_rate_idx]);
>
Thanks for the review.
Regards,
Subhajit Ghosh
On 21/1/24 19:52, Christophe JAILLET wrote:
> Le 21/01/2024 à 06:17, Subhajit Ghosh a écrit :
>> 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 <[email protected]>
>> ---
>
> Hi,
>
> a few nits and a few real comment/question below.
>
> Just my 2c.
>
> CJ
> ...
>> +
>> +static int apds9306_read_event(struct iio_dev *indio_dev,
>> + const struct iio_chan_spec *chan,
>> + enum iio_event_type type,
>> + enum iio_event_direction dir,
>> + enum iio_event_info info,
>> + int *val, int *val2)
>> +{
>> + struct apds9306_data *data = iio_priv(indio_dev);
>> + int ret;
>
> Other functions below that look really similar have a:
> guard(mutex)(&data->mutex);
>
> Is it needed here?
You are right, don't think so. Regmap lock is being used.
I will review the locking mechanism. Acknowledging all other comments.
Thanks for the review.
Regards,
Subhajit Ghosh