Re: [PATCH v9 5/5] iio: light: Add support for APDS9306 Light Sensor

[Jonathan Cameron <jic23@kernel.org>]

On Sat,  9 Mar 2024 21:20:31 +1030
Subhajit Ghosh <subhajit.ghosh@tweaklogic.com> 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@tweaklogic.com>

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);
> +}

...