From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
To: Lorenzo Bianconi <lorenzo.bianconi83@gmail.com>
Cc: Jonathan Cameron <jic23@kernel.org>, <linux-iio@vger.kernel.org>,
"Lorenzo BIANCONI" <lorenzo.bianconi@st.com>
Subject: Re: [PATCH v2] iio: accel: st_accel: add SPI-3wire support
Date: Fri, 7 Jul 2017 17:57:48 +0800 [thread overview]
Message-ID: <20170707175748.0000497c@huawei.com> (raw)
In-Reply-To: <CAA2SeN+nA3Psr6mQigh3javvBee5N-xt+QV+gezrovV3akCmaw@mail.gmail.com>
On Fri, 7 Jul 2017 11:12:44 +0200
Lorenzo Bianconi <lorenzo.bianconi83@gmail.com> wrote:
> > On Wed, 5 Jul 2017 20:30:01 +0200
> > Lorenzo Bianconi <lorenzo.bianconi83@gmail.com> wrote:
> >
> >> Add SPI Serial Interface Mode (SIM) register information
> >> in st_sensor_settings look up table to support devices
> >> (like LSM303AGR accel sensor) that allow just SPI-3wire
> >> communication mode. SIM mode has to be configured before any
> >> other operation since it is not enabled by default and the driver
> >> is not able to read without that configuration
> >>
> >> Fixes: ddc05fa28606 (iio: st-accel: add support for lsm303agr accel)
> >> Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@st.com>
> > Applied to the fixes-togreg branch and marked for stable.
> >
> > I added a brief note for stable maintainers saying that whilst
> > substantial logic was simple and better to have a proper fix than
> > a work around.
>
> Ack. I was wondering what is a better way to fix that issue. Do you
> mean to not use sensor name for look-up in st_accel_sensors_settings?
> Is my understanding correct?
The only alternative that immediately came to mind as a minimal fix would
be to have enforced 3 wire for the one part that doens't work any other way.
Would have turned it in two a couple of lines.
Adding 3 wire support for other devices is technically a feature enhancement.
Who cares though - fix is good as far as I'm concerned. Up to the stable
maintainers on whether they want to pick this up or not.
Jonathan
> If so we can migrate the lookup in st_{sensor}_sensors_settings to
> st_{sensor}_type like we did in st_lsm6dsx.
> Anyway we can't get any information from the device to enforce lookup
> before configuring the SIM.
>
> Regards,
> Lorenzo
>
> >
> > Jonathan
> >> ---
> >> Changes since v1:
> >> - move SIM register information in st_sensor_settings look up table
> >> - make st_sensors_init_interface_mode static and use name lookup
> >> performed by st_sensors_check_device_support
> >> ---
> >> drivers/iio/accel/st_accel_core.c | 32 +++++++++++++++++++++++++
> >> drivers/iio/common/st_sensors/st_sensors_core.c | 29 ++++++++++++++++++++++
> >> include/linux/iio/common/st_sensors.h | 7 ++++++
> >> include/linux/platform_data/st_sensors_pdata.h | 2 ++
> >> 4 files changed, 70 insertions(+)
> >>
> >> diff --git a/drivers/iio/accel/st_accel_core.c b/drivers/iio/accel/st_accel_core.c
> >> index 07d1489cd457..e44f62bf9caa 100644
> >> --- a/drivers/iio/accel/st_accel_core.c
> >> +++ b/drivers/iio/accel/st_accel_core.c
> >> @@ -166,6 +166,10 @@ static const struct st_sensor_settings st_accel_sensors_settings[] = {
> >> .mask_ihl = 0x02,
> >> .addr_stat_drdy = ST_SENSORS_DEFAULT_STAT_ADDR,
> >> },
> >> + .sim = {
> >> + .addr = 0x23,
> >> + .value = BIT(0),
> >> + },
> >> .multi_read_bit = true,
> >> .bootime = 2,
> >> },
> >> @@ -234,6 +238,10 @@ static const struct st_sensor_settings st_accel_sensors_settings[] = {
> >> .mask_od = 0x40,
> >> .addr_stat_drdy = ST_SENSORS_DEFAULT_STAT_ADDR,
> >> },
> >> + .sim = {
> >> + .addr = 0x23,
> >> + .value = BIT(0),
> >> + },
> >> .multi_read_bit = true,
> >> .bootime = 2,
> >> },
> >> @@ -316,6 +324,10 @@ static const struct st_sensor_settings st_accel_sensors_settings[] = {
> >> .en_mask = 0x08,
> >> },
> >> },
> >> + .sim = {
> >> + .addr = 0x24,
> >> + .value = BIT(0),
> >> + },
> >> .multi_read_bit = false,
> >> .bootime = 2,
> >> },
> >> @@ -379,6 +391,10 @@ static const struct st_sensor_settings st_accel_sensors_settings[] = {
> >> .mask_int1 = 0x04,
> >> .addr_stat_drdy = ST_SENSORS_DEFAULT_STAT_ADDR,
> >> },
> >> + .sim = {
> >> + .addr = 0x21,
> >> + .value = BIT(1),
> >> + },
> >> .multi_read_bit = true,
> >> .bootime = 2, /* guess */
> >> },
> >> @@ -437,6 +453,10 @@ static const struct st_sensor_settings st_accel_sensors_settings[] = {
> >> .mask_od = 0x40,
> >> .addr_stat_drdy = ST_SENSORS_DEFAULT_STAT_ADDR,
> >> },
> >> + .sim = {
> >> + .addr = 0x21,
> >> + .value = BIT(7),
> >> + },
> >> .multi_read_bit = false,
> >> .bootime = 2, /* guess */
> >> },
> >> @@ -499,6 +519,10 @@ static const struct st_sensor_settings st_accel_sensors_settings[] = {
> >> .addr_ihl = 0x22,
> >> .mask_ihl = 0x80,
> >> },
> >> + .sim = {
> >> + .addr = 0x23,
> >> + .value = BIT(0),
> >> + },
> >> .multi_read_bit = true,
> >> .bootime = 2,
> >> },
> >> @@ -547,6 +571,10 @@ static const struct st_sensor_settings st_accel_sensors_settings[] = {
> >> .mask_int1 = 0x04,
> >> .addr_stat_drdy = ST_SENSORS_DEFAULT_STAT_ADDR,
> >> },
> >> + .sim = {
> >> + .addr = 0x21,
> >> + .value = BIT(1),
> >> + },
> >> .multi_read_bit = false,
> >> .bootime = 2,
> >> },
> >> @@ -614,6 +642,10 @@ static const struct st_sensor_settings st_accel_sensors_settings[] = {
> >> .mask_ihl = 0x02,
> >> .addr_stat_drdy = ST_SENSORS_DEFAULT_STAT_ADDR,
> >> },
> >> + .sim = {
> >> + .addr = 0x23,
> >> + .value = BIT(0),
> >> + },
> >> .multi_read_bit = true,
> >> .bootime = 2,
> >> },
> >> diff --git a/drivers/iio/common/st_sensors/st_sensors_core.c b/drivers/iio/common/st_sensors/st_sensors_core.c
> >> index 274868100fd0..d99bb1460fe2 100644
> >> --- a/drivers/iio/common/st_sensors/st_sensors_core.c
> >> +++ b/drivers/iio/common/st_sensors/st_sensors_core.c
> >> @@ -581,6 +581,31 @@ int st_sensors_read_info_raw(struct iio_dev *indio_dev,
> >> }
> >> EXPORT_SYMBOL(st_sensors_read_info_raw);
> >>
> >> +static int st_sensors_init_interface_mode(struct iio_dev *indio_dev,
> >> + const struct st_sensor_settings *sensor_settings)
> >> +{
> >> + struct st_sensor_data *sdata = iio_priv(indio_dev);
> >> + struct device_node *np = sdata->dev->of_node;
> >> + struct st_sensors_platform_data *pdata;
> >> +
> >> + pdata = (struct st_sensors_platform_data *)sdata->dev->platform_data;
> >> + if (((np && of_property_read_bool(np, "spi-3wire")) ||
> >> + (pdata && pdata->spi_3wire)) && sensor_settings->sim.addr) {
> >> + int err;
> >> +
> >> + err = sdata->tf->write_byte(&sdata->tb, sdata->dev,
> >> + sensor_settings->sim.addr,
> >> + sensor_settings->sim.value);
> >> + if (err < 0) {
> >> + dev_err(&indio_dev->dev,
> >> + "failed to init interface mode\n");
> >> + return err;
> >> + }
> >> + }
> >> +
> >> + return 0;
> >> +}
> >> +
> >> int st_sensors_check_device_support(struct iio_dev *indio_dev,
> >> int num_sensors_list,
> >> const struct st_sensor_settings *sensor_settings)
> >> @@ -605,6 +630,10 @@ int st_sensors_check_device_support(struct iio_dev *indio_dev,
> >> return -ENODEV;
> >> }
> >>
> >> + err = st_sensors_init_interface_mode(indio_dev, &sensor_settings[i]);
> >> + if (err < 0)
> >> + return err;
> >> +
> >> if (sensor_settings[i].wai_addr) {
> >> err = sdata->tf->read_byte(&sdata->tb, sdata->dev,
> >> sensor_settings[i].wai_addr, &wai);
> >> diff --git a/include/linux/iio/common/st_sensors.h b/include/linux/iio/common/st_sensors.h
> >> index 1f8211b6438b..7b0fa8b5c120 100644
> >> --- a/include/linux/iio/common/st_sensors.h
> >> +++ b/include/linux/iio/common/st_sensors.h
> >> @@ -105,6 +105,11 @@ struct st_sensor_fullscale {
> >> struct st_sensor_fullscale_avl fs_avl[ST_SENSORS_FULLSCALE_AVL_MAX];
> >> };
> >>
> >> +struct st_sensor_sim {
> >> + u8 addr;
> >> + u8 value;
> >> +};
> >> +
> >> /**
> >> * struct st_sensor_bdu - ST sensor device block data update
> >> * @addr: address of the register.
> >> @@ -197,6 +202,7 @@ struct st_sensor_transfer_function {
> >> * @bdu: Block data update register.
> >> * @das: Data Alignment Selection register.
> >> * @drdy_irq: Data ready register of the sensor.
> >> + * @sim: SPI serial interface mode register of the sensor.
> >> * @multi_read_bit: Use or not particular bit for [I2C/SPI] multi-read.
> >> * @bootime: samples to discard when sensor passing from power-down to power-up.
> >> */
> >> @@ -213,6 +219,7 @@ struct st_sensor_settings {
> >> struct st_sensor_bdu bdu;
> >> struct st_sensor_das das;
> >> struct st_sensor_data_ready_irq drdy_irq;
> >> + struct st_sensor_sim sim;
> >> bool multi_read_bit;
> >> unsigned int bootime;
> >> };
> >> diff --git a/include/linux/platform_data/st_sensors_pdata.h b/include/linux/platform_data/st_sensors_pdata.h
> >> index 79b0e4cdb814..f8274b0c6888 100644
> >> --- a/include/linux/platform_data/st_sensors_pdata.h
> >> +++ b/include/linux/platform_data/st_sensors_pdata.h
> >> @@ -17,10 +17,12 @@
> >> * Available only for accelerometer and pressure sensors.
> >> * Accelerometer DRDY on LSM330 available only on pin 1 (see datasheet).
> >> * @open_drain: set the interrupt line to be open drain if possible.
> >> + * @spi_3wire: enable spi-3wire mode.
> >> */
> >> struct st_sensors_platform_data {
> >> u8 drdy_int_pin;
> >> bool open_drain;
> >> + bool spi_3wire;
> >> };
> >>
> >> #endif /* ST_SENSORS_PDATA_H */
> >
>
>
>
next prev parent reply other threads:[~2017-07-07 9:58 UTC|newest]
Thread overview: 5+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-07-05 18:30 [PATCH v2] iio: accel: st_accel: add SPI-3wire support Lorenzo Bianconi
2017-07-06 18:34 ` Jonathan Cameron
2017-07-07 9:12 ` Lorenzo Bianconi
2017-07-07 9:57 ` Jonathan Cameron [this message]
2017-07-07 10:18 ` Lorenzo Bianconi
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20170707175748.0000497c@huawei.com \
--to=jonathan.cameron@huawei.com \
--cc=jic23@kernel.org \
--cc=linux-iio@vger.kernel.org \
--cc=lorenzo.bianconi83@gmail.com \
--cc=lorenzo.bianconi@st.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.