| Message ID | 20240110172306.31273-7-petre.rodan@subdimension.ro (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | iio: pressure: hsc030pa new features | expand |
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 > >
Hi Jonathan, On Fri, Jan 12, 2024 at 05:13:56PM +0000, Jonathan Cameron wrote: > 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. > > [..] > > + 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; > > + } > > + [..] > > 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; > > + } first of all thank you for the review. I was afraid that this block will not be taken too well since I'm trying to achieve something that the SPI subsystem was not designed for. the code does exactly what the datasheet specs require on my SPI controller, but indeed the API might change at some point making the code non-functional. by 'sending something' you mean on the SPI bus or are you pushing me toward a patch to SPI core? unfortunately this chip feature is a special request only, there is no way for me to test what happens if the wakeup sequence also contains a payload (in both i2c and spi cases). the i2c wakeup code was inspired from the abp060mg driver, but I can't reach its maintainer to ask for details. I also can't seem to reach Honeywell. oh well. best regards, peter
On Sun, 14 Jan 2024 07:17:47 +0200 Petre Rodan <petre.rodan@subdimension.ro> wrote: > Hi Jonathan, > > On Fri, Jan 12, 2024 at 05:13:56PM +0000, Jonathan Cameron wrote: > > 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. > > > > [..] > > > + 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; > > > + } > > > + > [..] > > > 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; > > > + } > > first of all thank you for the review. > > I was afraid that this block will not be taken too well since I'm trying to > achieve something that the SPI subsystem was not designed for. > > the code does exactly what the datasheet specs require on my SPI controller, but > indeed the API might change at some point making the code non-functional. > > by 'sending something' you mean on the SPI bus or are you pushing me toward a > patch to SPI core? Should have said receive 'something' - that means that we'd clock some data whether or not the device had any to provide. > > unfortunately this chip feature is a special request only, there is no way for > me to test what happens if the wakeup sequence also contains a payload (in both > i2c and spi cases). the i2c wakeup code was inspired from the abp060mg driver, > but I can't reach its maintainer to ask for details. I also can't seem to reach > Honeywell. oh well. > +CC Mark. Discussion is about whether a zero length SPI transfer is guaranteed to pulse the chip select. If this is something we need, I'd prefer to see it as something a given SPI controller would opt in to supporting or some other way of knowing it would actually happen such as some docs that say it will work for an SPI controller (which I doubt is the case) Jonathan > best regards, > peter >
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; + 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. + */ + 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); }
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(-) -- 2.41.0