On Sat, 9 Mar 2024 21:20:31 +1030
Subhajit Ghosh <subhajit.ghosh@xxxxxxxxxxxxxx> 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>
A few things inline, on trivial that I'll tidy up, the other a potential
suggestion for simplifying the code, but not something I'm going to
ask for a v10 for. If you like the suggestion then a follow up patch,
if not then fair enough - may be a case of "don't let perfect be the enemy
of good" or maybe you disagree.
Anyhow, series applied to the togreg-normal branch of iio.git and pushed out
for 0-day to take a look at it. This is 6.10 material now.
Thanks,
Jonathan
> +static const struct iio_chan_spec apds9306_channels_with_events[] = {
> + {
> + .type = IIO_LIGHT,
> + .info_mask_shared_by_all = BIT(IIO_CHAN_INFO_INT_TIME) |
> + BIT(IIO_CHAN_INFO_SAMP_FREQ),
> + .info_mask_shared_by_all_available = BIT(IIO_CHAN_INFO_INT_TIME) |
> + BIT(IIO_CHAN_INFO_SAMP_FREQ),
> + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
> + BIT(IIO_CHAN_INFO_SCALE),
> + .info_mask_separate_available = BIT(IIO_CHAN_INFO_SCALE),
> + .event_spec = apds9306_event_spec,
> + .num_event_specs = ARRAY_SIZE(apds9306_event_spec),
> + }, {
> + .type = IIO_INTENSITY,
> + .info_mask_shared_by_all = BIT(IIO_CHAN_INFO_INT_TIME) |
> + BIT(IIO_CHAN_INFO_SAMP_FREQ),
> + .info_mask_shared_by_all_available = BIT(IIO_CHAN_INFO_INT_TIME) |
> + BIT(IIO_CHAN_INFO_SAMP_FREQ),
> + .channel2 = IIO_MOD_LIGHT_CLEAR,
> + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
Trivial but odd field order - I'll tidy that up whilst applying.
> + .modified = 1,
> + .event_spec = apds9306_event_spec,
> + .num_event_specs = ARRAY_SIZE(apds9306_event_spec),
> + },
> +};
> +
> +static const struct iio_chan_spec apds9306_channels_without_events[] = {
> + {
> + .type = IIO_LIGHT,
> + .info_mask_shared_by_all = BIT(IIO_CHAN_INFO_INT_TIME) |
> + BIT(IIO_CHAN_INFO_SAMP_FREQ),
> + .info_mask_shared_by_all_available = BIT(IIO_CHAN_INFO_INT_TIME) |
> + BIT(IIO_CHAN_INFO_SAMP_FREQ),
> + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
> + BIT(IIO_CHAN_INFO_SCALE),
> + .info_mask_separate_available = BIT(IIO_CHAN_INFO_SCALE),
> + }, {
> + .type = IIO_INTENSITY,
> + .info_mask_shared_by_all = BIT(IIO_CHAN_INFO_INT_TIME) |
> + BIT(IIO_CHAN_INFO_SAMP_FREQ),
> + .info_mask_shared_by_all_available = BIT(IIO_CHAN_INFO_INT_TIME) |
> + BIT(IIO_CHAN_INFO_SAMP_FREQ),
> + .channel2 = IIO_MOD_LIGHT_CLEAR,
> + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
> + .modified = 1,
> + },
> +};
> +
> +/* INT_PERSISTENCE available */
> +static IIO_CONST_ATTR(thresh_either_period_available, "[0 1 15]");
> +
> +/* ALS_THRESH_VAR available */
> +static IIO_CONST_ATTR(thresh_adaptive_either_values_available, "[0 1 7]");
> +
> +static struct attribute *apds9306_event_attributes[] = {
> + &iio_const_attr_thresh_either_period_available.dev_attr.attr,
> + &iio_const_attr_thresh_adaptive_either_values_available.dev_attr.attr,
> + NULL
> +};
> +
> +static int apds9306_write_event_config(struct iio_dev *indio_dev,
> + const struct iio_chan_spec *chan,
> + enum iio_event_type type,
> + enum iio_event_direction dir,
> + int state)
> +{
> + struct apds9306_data *data = iio_priv(indio_dev);
> + struct apds9306_regfields *rf = &data->rf;
> + int ret, val;
> +
> + state = !!state;
> +
> + switch (type) {
> + case IIO_EV_TYPE_THRESH: {
> + guard(mutex)(&data->mutex);
> +
> + /*
> + * If interrupt is enabled, the channel is set before enabling
> + * the interrupt. In case of disable, no need to switch
> + * channels. In case of different channel is selected while
> + * interrupt in on, just change the channel.
> + */
> + if (state) {
> + if (chan->type == IIO_LIGHT)
> + val = 1;
> + else if (chan->type == IIO_INTENSITY)
> + val = 0;
> + else
> + return -EINVAL;
> +
> + ret = regmap_field_write(rf->int_src, val);
> + if (ret)
> + return ret;
> + }
> +
> + ret = regmap_field_read(rf->int_en, &val);
> + if (ret)
> + return ret;
> +
> + if (val == state)
> + return 0;
> +
> + ret = regmap_field_write(rf->int_en, state);
> + if (ret)
> + return ret;
> +
> + if (state)
> + return pm_runtime_resume_and_get(data->dev);
> +
> + pm_runtime_mark_last_busy(data->dev);
> + pm_runtime_put_autosuspend(data->dev);
Note this isn't a reason to do a v10, just a possible suggestion for
what I think is more readable code.
Flow here is complex, maybe we'd have been better with skipping the
state = !!state, rename val to more explicit enabled
above and something like..
ret = regmap_field_read(rf->int_en, &enabled);
if (ret)
return ret;
if (state) {
if (chan->type == IIO_LIGHT)
ret = regmap_field_write(rf->int_src, 1);
else if (chan->type == IIO_INTENSITY)
ret = regmap_field_write(rf->int_src, 0);
else
return -EINVAL;
if (ret)
return ret;
if (enabled) /* Already enabled */
return 0;
ret = regmap_field_write(rf->int_en, 1);
if (ret)
return ret;
return pm_runtime_resume_and_get(data->dev);
} else { // Could drop this else but I think it's useful to show the either or flow.
if (!enabled)
return 0;
ret = regmap_field_write(rf->int_en, 0);
if (ret)
return ret;
pm_runtime_mark_last_busy(data->dev);
pm_runtime_put_autosuspend(data->dev);
return 0;
}
}
> +
> + return 0;
> + }
> + case IIO_EV_TYPE_THRESH_ADAPTIVE:
> + return regmap_field_write(rf->int_thresh_var_en, state);
> + default:
> + return -EINVAL;
> + }
> +}
> +
> +static void apds9306_powerdown(void *ptr)
> +{
> + struct apds9306_data *data = (struct apds9306_data *)ptr;
> + struct apds9306_regfields *rf = &data->rf;
> + int ret;
> +
> + ret = regmap_field_write(rf->int_thresh_var_en, 0);
> + if (ret)
> + return;
> +
> + ret = regmap_field_write(rf->int_en, 0);
> + if (ret)
> + return;
> +
> + apds9306_power_state(data, false);
> +}
...
