Re: [PATCH 6/6] iio: pressure: hsc030pa add sleep mode

[Jonathan Cameron <Jonathan.Cameron@Huawei.com>]

On Wed, 10 Jan 2024 19:22:41 +0200
Petre Rodan <petre.rodan@subdimension.ro> wrote:

> Some custom chips from this series require a wakeup sequence before the
> measurement cycle is started.
> 
> Quote from the product datasheet:
> "Optional sleep mode available upon special request."
> 
> Signed-off-by: Petre Rodan <petre.rodan@subdimension.ro>
> ---
>  drivers/iio/pressure/hsc030pa.c     |  4 ++++
>  drivers/iio/pressure/hsc030pa.h     |  4 ++++
>  drivers/iio/pressure/hsc030pa_i2c.c | 19 +++++++++++++++++
>  drivers/iio/pressure/hsc030pa_spi.c | 32 +++++++++++++++++++++++++++--
>  4 files changed, 57 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/iio/pressure/hsc030pa.c b/drivers/iio/pressure/hsc030pa.c
> index 3faa0fd42201..9e66fd561801 100644
> --- a/drivers/iio/pressure/hsc030pa.c
> +++ b/drivers/iio/pressure/hsc030pa.c
> @@ -501,6 +501,10 @@ int hsc_common_probe(struct device *dev, hsc_recv_fn recv)
>  		return dev_err_probe(dev, -EINVAL,
>  				     "pressure limits are invalid\n");
> 
> +	ret = device_property_read_bool(dev, "honeywell,sleep-mode");
> +	if (ret)
> +		hsc->capabilities |= HSC_CAP_SLEEP;
	if (device_property_read_bool())
		hsc->cap...

The return value is not an int so it's inappropriate to stash it in ret.

> +
>  	ret = devm_regulator_get_enable(dev, "vdd");
>  	if (ret)
>  		return dev_err_probe(dev, ret, "can't get vdd supply\n");
> diff --git a/drivers/iio/pressure/hsc030pa.h b/drivers/iio/pressure/hsc030pa.h
> index 6c635c42d85d..4e356944d67d 100644
> --- a/drivers/iio/pressure/hsc030pa.h
> +++ b/drivers/iio/pressure/hsc030pa.h
> @@ -15,6 +15,8 @@
>  #define HSC_REG_MEASUREMENT_RD_SIZE 4
>  #define HSC_RESP_TIME_MS            2
> 
> +#define HSC_CAP_SLEEP               0x1
> +
>  struct device;
> 
>  struct iio_chan_spec;
> @@ -29,6 +31,7 @@ typedef int (*hsc_recv_fn)(struct hsc_data *);
>   * struct hsc_data
>   * @dev: current device structure
>   * @chip: structure containing chip's channel properties
> + * @capabilities: chip specific attributes
>   * @recv_cb: function that implements the chip reads
>   * @is_valid: true if last transfer has been validated
>   * @pmin: minimum measurable pressure limit
> @@ -45,6 +48,7 @@ typedef int (*hsc_recv_fn)(struct hsc_data *);
>  struct hsc_data {
>  	struct device *dev;
>  	const struct hsc_chip_data *chip;
> +	u32 capabilities;
>  	hsc_recv_fn recv_cb;
>  	bool is_valid;
>  	s32 pmin;
> diff --git a/drivers/iio/pressure/hsc030pa_i2c.c b/drivers/iio/pressure/hsc030pa_i2c.c
> index b3fd230e71da..62bdae272012 100644
> --- a/drivers/iio/pressure/hsc030pa_i2c.c
> +++ b/drivers/iio/pressure/hsc030pa_i2c.c
> @@ -24,8 +24,27 @@ static int hsc_i2c_recv(struct hsc_data *data)
>  {
>  	struct i2c_client *client = to_i2c_client(data->dev);
>  	struct i2c_msg msg;
> +	u8 buf;
>  	int ret;
> 
> +	if (data->capabilities & HSC_CAP_SLEEP) {
> +		/*
> +		 * Send the Full Measurement Request (FMR) command on the CS
> +		 * line in order to wake up the sensor as per
> +		 * "Sleep Mode for Use with Honeywell Digital Pressure Sensors"
> +		 * technical note (consult the datasheet link in the header).
> +		 *
> +		 * These specifications require a dummy packet comprised only by
> +		 * a single byte that contains the 7bit slave address and the
> +		 * READ bit followed by a STOP.
> +		 * Because the i2c API does not allow packets without a payload,
> +		 * the driver sends two bytes in this implementation.
> +		 */
> +		ret = i2c_master_recv(client, &buf, 1);
> +		if (ret < 0)
> +			return ret;
> +	}
> +
>  	msleep_interruptible(HSC_RESP_TIME_MS);
> 
>  	msg.addr = client->addr;
> diff --git a/drivers/iio/pressure/hsc030pa_spi.c b/drivers/iio/pressure/hsc030pa_spi.c
> index 737197eddff0..1c139cdfe856 100644
> --- a/drivers/iio/pressure/hsc030pa_spi.c
> +++ b/drivers/iio/pressure/hsc030pa_spi.c
> @@ -25,12 +25,40 @@ static int hsc_spi_recv(struct hsc_data *data)
>  	struct spi_device *spi = to_spi_device(data->dev);
>  	struct spi_transfer xfer = {
>  		.tx_buf = NULL,
> -		.rx_buf = data->buffer,
> -		.len = HSC_REG_MEASUREMENT_RD_SIZE,
> +		.rx_buf = NULL,
> +		.len = 0,
>  	};
> +	u16 orig_cs_setup_value;
> +	u8 orig_cs_setup_unit;
> +
> +	if (data->capabilities & HSC_CAP_SLEEP) {
> +		/*
> +		 * Send the Full Measurement Request (FMR) command on the CS
> +		 * line in order to wake up the sensor as per
> +		 * "Sleep Mode for Use with Honeywell Digital Pressure Sensors"
> +		 * technical note (consult the datasheet link in the header).
> +		 *
> +		 * These specifications require the CS line to be held asserted
> +		 * for at least 8µs without any payload being generated.
> +		 */
> +		orig_cs_setup_value = spi->cs_setup.value;
> +		orig_cs_setup_unit = spi->cs_setup.unit;
> +		spi->cs_setup.value = 8;
> +		spi->cs_setup.unit = SPI_DELAY_UNIT_USECS;
> +		/*
> +		 * Send a dummy 0-size packet so that CS gets toggled.
> +		 * Trying to manually call spi->controller->set_cs() instead
> +		 * does not work as expected during the second call.
> +		 */

Do you have a reference that says the CS must be toggled on 0 length transfer?
If that's not specified in the SPI core somewhere then you will need to send
something...

> +		spi_sync_transfer(spi, &xfer, 1);
> +		spi->cs_setup.value = orig_cs_setup_value;
> +		spi->cs_setup.unit = orig_cs_setup_unit;
> +	}
> 
>  	msleep_interruptible(HSC_RESP_TIME_MS);
> 
> +	xfer.rx_buf = data->buffer;
> +	xfer.len = HSC_REG_MEASUREMENT_RD_SIZE;
>  	return spi_sync_transfer(spi, &xfer, 1);
>  }
> 
> --
> 2.41.0
> 
>