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

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

[Gwendal Grignou]

fifo_timeout is used by the EC firmware only when a new sample is
available.
When the timeout changes, espcially when the new timeout is shorter than
the current one, we need to send the samples waiting in the FIFO to the
host.
We also need to flush when a sensor is suspended (ODR set to 0).

Signed-off-by: Gwendal Grignou <gwendal@chromium.org>
---
 .../cros_ec_sensors/cros_ec_sensors_core.c    | 51 ++++++++++++-------
 1 file changed, 33 insertions(+), 18 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..4486c7e1e5b42 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);
+	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;
+
+	/*
+	 * 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;
 
@@ -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);
@@ -764,6 +765,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.
  */
 int cros_ec_sensors_core_read(struct cros_ec_sensors_core_state *st,
 			  struct iio_chan_spec const *chan,
@@ -836,6 +839,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.
  */
 int cros_ec_sensors_core_write(struct cros_ec_sensors_core_state *st,
 			       struct iio_chan_spec const *chan,
@@ -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);
+		}
 		break;
 	default:
 		ret = -EINVAL;
-- 
2.49.0.472.ge94155a9ec-goog

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

[Tzung-Bi Shih]

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?

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

[Gwendal Grignou]

On Wed, Apr 2, 2025 at 2:30 AM Tzung-Bi Shih <tzungbi@kernel.org> wrote:
>
> 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`.
I mean sysfs attribute `hwfifo_timeout`: its value is only used when
samples are flowing from the sensors.
>
> > ---
>
> "drivers: " in the patch's title prefix can be dropped.
Done.
>
> > -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.
I changed it to be consistent with the flush being inline, and the
function used only once. Fixed in commit message..
>
> > @@ -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.
Removed.
>
> > @@ -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?
Done.

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

[Gwendal Grignou]

|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>
---
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..caf40738337e4 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);
+	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;
+
+	/*
+	 * 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.
  */
 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.
  */
 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);
+
+		/* 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 (ret >= 0 && frequency == 0) {
+			st->param.cmd = MOTIONSENSE_CMD_FIFO_FLUSH;
+			ret = cros_ec_motion_send_host_cmd(st, 0);
+		}
 		break;
 	default:
 		ret = -EINVAL;
-- 
2.49.0.504.g3bcea36a83-goog

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

[Tzung-Bi Shih]

On Mon, Apr 07, 2025 at 10:54:11PM -0700, Gwendal Grignou wrote:
> @@ -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);
> +
> +		/* 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 (ret >= 0 && frequency == 0) {

To disambiguate, `ret == 0` or `!ret`[1].

[1]: https://elixir.bootlin.com/linux/v6.13/source/drivers/iio/common/cros_ec_sensors/cros_ec_sensors_core.c#L411

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

[Gwendal Grignou]

On Tue, Apr 8, 2025 at 12:59 AM Tzung-Bi Shih <tzungbi@kernel.org> wrote:
>
> On Mon, Apr 07, 2025 at 10:54:11PM -0700, Gwendal Grignou wrote:
> > @@ -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);
> > +
> > +             /* 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 (ret >= 0 && frequency == 0) {
>
> To disambiguate, `ret == 0` or `!ret`[1].
Fixed in V3.
>
> [1]: https://elixir.bootlin.com/linux/v6.13/source/drivers/iio/common/cros_ec_sensors/cros_ec_sensors_core.c#L411

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

[Gwendal Grignou]

|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>
---
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);
+	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;
+
+	/*
+	 * 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.
  */
 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.
  */
 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);
+
+		/* 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 (ret == 0 && frequency == 0) {
+			st->param.cmd = MOTIONSENSE_CMD_FIFO_FLUSH;
+			ret = cros_ec_motion_send_host_cmd(st, 0);
+		}
 		break;
 	default:
 		ret = -EINVAL;
-- 
2.49.0.504.g3bcea36a83-goog

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

[Tzung-Bi Shih]

On Tue, Apr 08, 2025 at 08:56:19AM -0700, Gwendal Grignou 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>

Reviewed-by: Tzung-Bi Shih <tzungbi@kernel.org>

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

[Jonathan Cameron]

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;

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

[Gwendal Grignou]

On Sat, Apr 12, 2025 at 3:43 AM Jonathan Cameron <jic23@kernel.org> wrote:
>
> 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).
Point taken. Applied all the comments in v4.

Thanks,
Gwendal.
>
> 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?
We don't have to; the intent was to let another command waiting go
through since flush can be sent at anytime. But all in all it does not
improve multi-tasking, so removed.
>
> > +     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?
Removed for now.
>
> >   */
> >  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.
Removed for now.
>
> >   */
> >  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.
Done.
>
> 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;
>