Re: [PATCH v3] iio: cros_ec_sensors: Flush when changing the FIFO timeout

[Jonathan Cameron <jic23@kernel.org>]

On Tue,  8 Apr 2025 08:56:19 -0700
Gwendal Grignou <gwendal@chromium.org> wrote:

> |hwfifo_timeout| is used by the EC firmware only when new samples are
> available.
> When the timeout changes, espcially when the new timeout is shorter than
> the current one, send the samples waiting in the FIFO to the host.
> Inline the call to transmit |hwfifo_timeout| value to the firmware.
> 
> Now flush when a sensor is suspended (ODR set to 0) as well.
> 
> Signed-off-by: Gwendal Grignou <gwendal@chromium.org>
Firstly.  Don't send a new version in reply to an old one.
In general the reasons for this are:
1 - it can get very confusing if a thread gets deeply nested.
2 - at least some well known kernel folk start at their most recent emails
    and work backwards when looking for what to review.  They will probably
    never get to your thread!

There may be other parts of the kernel that prefer this style, but most
do not and I've never had anyone 'ask' for it (as opposed to not object).

Various minor comments inline.

Thanks,
Jonathan

> ---
> Changes in v3:
> - Fix error detection when setting the sensor frequency
> 
> Changes in v2:
> - Fix sysfs attribute in commit message
> - Indicated the function to send the content of the attribute is now
>   inline.
> - Improve error detection when setting the sensor frequency and flusing
>   the FIFO.
>  .../cros_ec_sensors/cros_ec_sensors_core.c    | 50 ++++++++++++-------
>  1 file changed, 33 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/iio/common/cros_ec_sensors/cros_ec_sensors_core.c b/drivers/iio/common/cros_ec_sensors/cros_ec_sensors_core.c
> index b1abd6e16c4ba..67ffe88df7b23 100644
> --- a/drivers/iio/common/cros_ec_sensors/cros_ec_sensors_core.c
> +++ b/drivers/iio/common/cros_ec_sensors/cros_ec_sensors_core.c
> @@ -103,22 +103,6 @@ static void get_default_min_max_freq(enum motionsensor_type type,
>  	}
>  }
>  
> -static int cros_ec_sensor_set_ec_rate(struct cros_ec_sensors_core_state *st,
> -				      int rate)
> -{
> -	int ret;
> -
> -	if (rate > U16_MAX)
> -		rate = U16_MAX;
> -
> -	mutex_lock(&st->cmd_lock);
> -	st->param.cmd = MOTIONSENSE_CMD_EC_RATE;
> -	st->param.ec_rate.data = rate;
> -	ret = cros_ec_motion_send_host_cmd(st, 0);
> -	mutex_unlock(&st->cmd_lock);
> -	return ret;
> -}
> -
>  static ssize_t cros_ec_sensor_set_report_latency(struct device *dev,
>  						 struct device_attribute *attr,
>  						 const char *buf, size_t len)
> @@ -134,7 +118,25 @@ static ssize_t cros_ec_sensor_set_report_latency(struct device *dev,
>  
>  	/* EC rate is in ms. */
>  	latency = integer * 1000 + fract / 1000;
> -	ret = cros_ec_sensor_set_ec_rate(st, latency);
> +
> +	mutex_lock(&st->cmd_lock);
Maybe use cleanup.h and
	guard(mutex)(&st->cmd_lock);
mostly because it simplifies code and...
> +	st->param.cmd = MOTIONSENSE_CMD_EC_RATE;
> +	st->param.ec_rate.data = min(U16_MAX, latency);
> +	ret = cros_ec_motion_send_host_cmd(st, 0);
> +	mutex_unlock(&st->cmd_lock);

I'm not sure why you unlock briefly here?

> +	if (ret < 0)
> +		return ret;
> +
> +	/*
> +	 * Flush samples currently in the FIFO, especially when the new latency
> +	 * is shorter than the old one: new timeout value is only considered when
> +	 * there is a new sample available. It can take a while for a slow
> +	 * sensor.
> +	 */
> +	mutex_lock(&st->cmd_lock);
> +	st->param.cmd = MOTIONSENSE_CMD_FIFO_FLUSH;
> +	ret = cros_ec_motion_send_host_cmd(st, 0);
> +	mutex_unlock(&st->cmd_lock);
>  	if (ret < 0)
>  		return ret;
>  
> @@ -764,6 +766,8 @@ EXPORT_SYMBOL_GPL(cros_ec_sensors_capture);
>   * @mask:	specifies which values to be requested
>   *
>   * Return:	the type of value returned by the device
> + *
> + * cmd_lock mutex held.

Maybe true, but has that changed in a fashion that means this should
be in this fix patch rather than a follow up improving documentation?

>   */
>  int cros_ec_sensors_core_read(struct cros_ec_sensors_core_state *st,
>  			  struct iio_chan_spec const *chan,
> @@ -836,6 +840,8 @@ EXPORT_SYMBOL_GPL(cros_ec_sensors_core_read_avail);
>   * @mask:	specifies which values to write
>   *
>   * Return:	the type of value returned by the device
> + *
> + * cmd_lock mutex held.
As above.

>   */
>  int cros_ec_sensors_core_write(struct cros_ec_sensors_core_state *st,
>  			       struct iio_chan_spec const *chan,
> @@ -853,6 +859,16 @@ int cros_ec_sensors_core_write(struct cros_ec_sensors_core_state *st,
>  		st->param.sensor_odr.roundup = 1;
>  
>  		ret = cros_ec_motion_send_host_cmd(st, 0);
I'd rather see
		if (ret)
			break;
given local style.

Ideal would actually be to make this whole function do direct returns on error
as that is easier to follow in cases like this where there is no cleanup.
However that change doesn't belong in a fix. Something for another day!

> +
> +		/* Flush the FIFO in case we are stopping a sensor.
Comment syntax slightly wrong.

		/*
		 * Flush the FIFO

> +		 * If the FIFO has just been emptied, pending samples will be
> +		 * stuck until new samples are available. It will not happen
> +		 * when all the sensors are stopped.
> +		 */
> +		if (ret == 0 && frequency == 0) {

With the break above, only need to check frequency here.

> +			st->param.cmd = MOTIONSENSE_CMD_FIFO_FLUSH;
> +			ret = cros_ec_motion_send_host_cmd(st, 0);
> +		}
>  		break;
>  	default:
>  		ret = -EINVAL;