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

[Tzung-Bi Shih <tzungbi@kernel.org>]

On Mon, Mar 31, 2025 at 09:48:32AM -0700, Gwendal Grignou wrote:
> fifo_timeout is used by the EC firmware only when a new sample is
> available.

I guess you mean: "FIFO timeout".  There is no specific symbol called
`fifo_timeout`.

> ---

"drivers: " in the patch's title prefix can be dropped.

> -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);
> +	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);
> +	if (ret < 0)
> +		return ret;

It isn't obvious (at least irrelevant to the commit message) that
cros_ec_sensor_set_ec_rate() becomes inline here.

> @@ -152,7 +154,6 @@ static ssize_t cros_ec_sensor_get_report_latency(struct device *dev,
>  	mutex_lock(&st->cmd_lock);
>  	st->param.cmd = MOTIONSENSE_CMD_EC_RATE;
>  	st->param.ec_rate.data = EC_MOTION_SENSE_NO_VALUE;
> -
>  	ret = cros_ec_motion_send_host_cmd(st, 0);
>  	latency = st->resp->ec_rate.ret;
>  	mutex_unlock(&st->cmd_lock);

Unwanted change.

> @@ -853,6 +858,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);
> +
> +		/* Flush the FIFO in case we are stopping a sensor.
> +		 * 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 (frequency == 0) {
> +			st->param.cmd = MOTIONSENSE_CMD_FIFO_FLUSH;
> +			cros_ec_motion_send_host_cmd(st, 0);

Wouldn't it want to check `ret` from previous cros_ec_motion_send_host_cmd()
and override `ret` by the latest call?