Received-SPF: pass (google.com: domain of linux-kernel+bounces-24906-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Date: Fri, 12 Jan 2024 17:13:56 +0000
From: Jonathan Cameron <Jonathan.Cameron@Huawei.com>
To: Petre Rodan <petre.rodan@subdimension.ro>
CC: <linux-iio@vger.kernel.org>, <linux-kernel@vger.kernel.org>, "Jonathan
 Cameron" <jic23@kernel.org>, Lars-Peter Clausen <lars@metafoo.de>
Subject: Re: [PATCH 6/6] iio: pressure: hsc030pa add sleep mode
Message-ID: <20240112171356.00003e88@Huawei.com>
In-Reply-To: <20240110172306.31273-7-petre.rodan@subdimension.ro>
References: <20240110172306.31273-1-petre.rodan@subdimension.ro>
	<20240110172306.31273-7-petre.rodan@subdimension.ro>
Organization: Huawei Technologies Research and Development (UK) Ltd.
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset="ISO-8859-1"
Content-Transfer-Encoding: quoted-printable

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.
>=20
> Quote from the product datasheet:
> "Optional sleep mode available upon special request."
>=20
> 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(-)
>=20
> diff --git a/drivers/iio/pressure/hsc030pa.c b/drivers/iio/pressure/hsc03=
0pa.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");
>=20
> +	ret =3D device_property_read_bool(dev, "honeywell,sleep-mode");
> +	if (ret)
> +		hsc->capabilities |=3D 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 =3D 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/hsc03=
0pa.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
>=20
> +#define HSC_CAP_SLEEP               0x1
> +
>  struct device;
>=20
>  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/h=
sc030pa_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 =3D to_i2c_client(data->dev);
>  	struct i2c_msg msg;
> +	u8 buf;
>  	int ret;
>=20
> +	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 =3D i2c_master_recv(client, &buf, 1);
> +		if (ret < 0)
> +			return ret;
> +	}
> +
>  	msleep_interruptible(HSC_RESP_TIME_MS);
>=20
>  	msg.addr =3D client->addr;
> diff --git a/drivers/iio/pressure/hsc030pa_spi.c b/drivers/iio/pressure/h=
sc030pa_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 =3D to_spi_device(data->dev);
>  	struct spi_transfer xfer =3D {
>  		.tx_buf =3D NULL,
> -		.rx_buf =3D data->buffer,
> -		.len =3D HSC_REG_MEASUREMENT_RD_SIZE,
> +		.rx_buf =3D NULL,
> +		.len =3D 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=B5s without any payload being generated.
> +		 */
> +		orig_cs_setup_value =3D spi->cs_setup.value;
> +		orig_cs_setup_unit =3D spi->cs_setup.unit;
> +		spi->cs_setup.value =3D 8;
> +		spi->cs_setup.unit =3D 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 transf=
er?
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 =3D orig_cs_setup_value;
> +		spi->cs_setup.unit =3D orig_cs_setup_unit;
> +	}
>=20
>  	msleep_interruptible(HSC_RESP_TIME_MS);
>=20
> +	xfer.rx_buf =3D data->buffer;
> +	xfer.len =3D HSC_REG_MEASUREMENT_RD_SIZE;
>  	return spi_sync_transfer(spi, &xfer, 1);
>  }
>=20
> --
> 2.41.0
>=20
>=20