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

[Subhajit Ghosh <subhajit.ghosh@tweaklogic.com>]

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