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

[Subhajit Ghosh <subhajit.ghosh@xxxxxxxxxxxxxx>]

> > +
> > +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;
> 		}
> 	}	
Yes, this is much simpler and readable. I will prepare a follow up patch for this.

Thank you for reviewing and applying the series.

Regards,
Subhajit Ghosh
> > +
> > +		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);
> > +}
>
> ...