Re: lsm303dlhc magnetometer: please help

[Jonathan Cameron <jic23@kernel.org>]

On Tue,  7 Apr 2020 15:14:27 +0200 (CEST)
yarl-baudig@mailoo.org wrote:

> ---- Message d'origine ----
> > De : Jonathan Cameron <jic23@kernel.org>
> > À : yarl-baudig@mailoo.org
> > Sujet : Re: lsm303dlhc magnetometer: please help
> > Date : 05/04/2020 15:03:56 Europe/Paris
> > Copie à : denis.ciocca@st.com;
> >       linux-iio@vger.kernel.org
> > 
> > On Sat,  4 Apr 2020 14:43:04 +0200 (CEST)
> > yarl-baudig@mailoo.org wrote:
> >   
> > > Good afternoun,
> > > 
> > > I have an lsm303dlhc that I'm trying to get to work with a triggered   
> > buffer, the magnetometer part of it.  
> > > The problem with this sensor is that the dataready signal has, I think, a   
> > different  
> > > meaning than the one expected by the ST sensor driver set.
> > > On this sensor the signal is always high except when the sensor is writing   
> > new values to its data  
> > > registers. The problem with the driver is that it expects the sensor to   
> > have a register  
> > > to check if new data is available:
> > > https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git/tree/drivers/  
> > iio/common/st_sensors/st_sensors_trigger.c?h=testing#n36  
> > > the lsm303dlhc magnetometer is not configured with such a register:
> > > https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git/tree/drivers/  
> > iio/magnetometer/st_magn_core.c?h=testing#n183  
> > > There is one in the sensor but the dataready bit is just the value of the   
> > signal, so  
> > > even if I added the address and mask for this information, the meaning   
> > would be  
> > > wrong from the point of view of  function and the while loop would run   
> > endlessly:  
> > > https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git/tree/drivers/  
> > iio/common/st_sensors/st_sensors_trigger.c?h=testing#n113  
> > >   
> > Looking at the datasheet, it is a rather dumb device isn't it and the 
> > datasheet
> > doesn't supply anywhere near enough info..  It says it's a system in 
> > package
> > so in theory we could find out what the sensor actually is and that might
> > have a better datasheet.
> > 
> > Then we have the data ready bit as you say and a lock bit that fires when 
> > we
> > successfully start reading (ensuring we get a full set from the same 
> > 'scan')
> > The dataready bit description says:
> > "Data-ready bit. This bit is when a new set of measurements is available."
> > I admit I'm reading between the lines, but that sounds like it will clear 
> > when
> > the data has been read.  If it clears on it's own then there isn't much
> > we can do reliably.
> > 
> > We could just treat the irq as a iio-trigger-interrupt trigger as long as we 
> > definitely don't
> > need to do anything to clear it.  That will not hit any of the paths you 
> > are
> > looking at as will directly call st_sensors_trigger_handler.
> > 
> > Unfortunately that doesn't seem to have a device tree binding yet 
> > (obviously
> > not much used except on very old platforms). I don't have a handy platform 
> > to
> > test it on at the moment unfortunately but should just be a case of adding
> > a device tree id table to the driver.  Then the dt entry will simply 
> > contain
> > the interrupt line.
> > 
> > Denis, any more info on this part and whether the using a dumb interrupt 
> > trigger
> > is the way to go or if we can do anything more clever?
> > 
> > It may be a case where the best bet is to ignore that line and use a high
> > resolution time trigger running at half the raw sampling frequency of the 
> > device
> > (to avoid repeated values)
> > 
> > Thanks,
> > 
> > Jonathan
> >   
> 
> Thank you for answering. I will probably try as you say, use an hrtimer.
> 
> Anyway I want to precise one thing I just saw using the patch I showed you.
> The lsm303dlhc contains an accelerometer and a magnetometer.
> When the buffer for the accelerometer is disabled, the magnetometer "works"
> the way I described in my last mail: one out of two times.
> When the buffer for the accelerometer is enabled: the magnetometer works,
> every time...

That is odd.  Any chance the interrupt lines are shared - i.e. wired
to the same pin?  Otherwise maybe there is an electrical short causing
problems...

Jonathan

> >   
> > > I then modified a bit, patch below.
> > > 
> > > Let me first tell you that it work one out of two time:
> > > I boot, load this device-tree:
> > > ---
> > > /dts-v1/;
> > > /plugin/;
> > > 
> > > / {
> > >   compatible = "brcm,bcm2708";
> > > 
> > >   fragment@0 {
> > >     target = <&gpio>;
> > >     __overlay__ {
> > >       magn_pins: nine_dof_pins {
> > >         brcm,pins = <27>;
> > >         brcm,function = <0>;
> > >         brcm,pull = <1>;
> > >         status = "okay";
> > >       };
> > >     };
> > >   };
> > > 
> > >   fragment@1 {
> > >     target = <&i2c_arm>;
> > >     __overlay__ {
> > >       status = "okay";
> > >       #address-cells = <1>;
> > >       #size-cells = <0>;
> > >       magn@1e {
> > >         compatible = "st,lsm303dlhc-magn";
> > >         reg = <0x1e>;
> > >         status = "okay";
> > >         interrupt-parent = <&gpio>;
> > >         interrupts = <27 1>;
> > >       };
> > >     };
> > >   };
> > > };
> > > ---
> > > I then enable scan_elements, enable buffer (echo 1 > buffer/enable)
> > > interrupts are coming regularly at sampling_frequency. It works fine.
> > > If I now disable the buffer then re-enable it, one and only interrupt,
> > > doesn't work fine..
> > > re-disable, re-re-enable: works fine!
> > > and it seem to be always that, it works modulo 2.
> > > 
> > > On my first try I didn't change st_magn_buffer_preenable and   
> > st_magn_buffer_postenable  
> > > But I thought that maybe, the problem was some sort of bad writting,   
> > reading timing  
> > > because 
> > > (1) request_threaded_irq is called for a rising signal while it is already   
> > high.  
> > > 
> > > I make a break here and ask you a question:
> > > As you read, I am a kernel newbie: is doing (1) bad?  
> > Interesting question.  There is so little info in the datasheet that it is 
> > hard
> > to tell. It might be a level_high interrupt.
> >   
> > > end of break.
> > > 
> > > So I added st_magn_buffer_preenable and modified st_magn_buffer_postenable   
> > to  
> > > try to mask the irq during the arppropriate interval.
> > > 
> > > No visible change.
> > > I never almost never wrote kernel code before.
> > > I tried to get closer to what was happening using gdb/kgdb, first time I   
> > used this.  
> > > I am now pretty discouraged and any suggestions are welcome.
> > > 
> > > Thank you.
> > > 
> > > ---
> > >  drivers/iio/common/st_sensors/st_sensors_core.c | 11 +++++++++++
> > >  .../iio/common/st_sensors/st_sensors_trigger.c  |  9 +++++++++
> > >  drivers/iio/magnetometer/st_magn_buffer.c       | 17 ++++++++++++++++-
> > >  drivers/iio/magnetometer/st_magn_core.c         |  3 +++
> > >  include/linux/iio/common/st_sensors.h           |  2 ++
> > >  5 files changed, 41 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/iio/common/st_sensors/st_sensors_core.c   
> > b/drivers/iio/common/st_sensors/st_sensors_core.c  
> > > index 09279e40c55c..fef6b70976b4 100644
> > > --- a/drivers/iio/common/st_sensors/st_sensors_core.c
> > > +++ b/drivers/iio/common/st_sensors/st_sensors_core.c
> > > @@ -480,6 +480,17 @@ int st_sensors_set_dataready_irq(struct iio_dev   
> > *indio_dev, bool enable)  
> > >  	u8 drdy_addr, drdy_mask;
> > >  	struct st_sensor_data *sdata = iio_priv(indio_dev);
> > >  
> > > +	if (sdata->sensor_settings->drdy_irq.simple) {
> > > +		/*
> > > +		 * some devices very simple. No register to enable, disable
> > > +		 * or configure the signal. Actually, when it is low it means that
> > > +		 * sensor is writing data to its register, when it is high it
> > > +		 * means that data can be read. i.e when rising new data is available.
> > > +		 */
> > > +		sdata->hw_irq_trigger = enable;
> > > +		return 0;
> > > +	}
> > > +
> > >  	if (!sdata->sensor_settings->drdy_irq.int1.addr &&
> > >  	    !sdata->sensor_settings->drdy_irq.int2.addr) {
> > >  		/*
> > > diff --git a/drivers/iio/common/st_sensors/st_sensors_trigger.c   
> > b/drivers/iio/common/st_sensors/st_sensors_trigger.c  
> > > index fdcc5a891958..146aaae0a85c 100644
> > > --- a/drivers/iio/common/st_sensors/st_sensors_trigger.c
> > > +++ b/drivers/iio/common/st_sensors/st_sensors_trigger.c
> > > @@ -30,6 +30,13 @@ static int st_sensors_new_samples_available(struct   
> > iio_dev *indio_dev,  
> > >  	u8 status;
> > >  	int ret;
> > >  
> > > +	/* We simply trust the signal */
> > > +	if (sdata->sensor_settings->drdy_irq.simple) {
> > > +		if (indio_dev->active_scan_mask)
> > > +			return 1;
> > > +		return 0;
> > > +	}
> > > +
> > >  	/* How would I know if I can't check it? */
> > >  	if (!sdata->sensor_settings->drdy_irq.stat_drdy.addr)
> > >  		return -EINVAL;
> > > @@ -90,6 +97,8 @@ static irqreturn_t st_sensors_irq_thread(int irq, void   
> > *p)  
> > >  	if (sdata->hw_irq_trigger &&
> > >  	    st_sensors_new_samples_available(indio_dev, sdata)) {
> > >  		iio_trigger_poll_chained(p);
> > > +		if (sdata->sensor_settings->drdy_irq.simple)
> > > +			return IRQ_HANDLED;
> > >  	} else {
> > >  		dev_dbg(sdata->dev, "spurious IRQ\n");
> > >  		return IRQ_NONE;
> > > diff --git a/drivers/iio/magnetometer/st_magn_buffer.c   
> > b/drivers/iio/magnetometer/st_magn_buffer.c  
> > > index 37ab30566464..ae13e4339127 100644
> > > --- a/drivers/iio/magnetometer/st_magn_buffer.c
> > > +++ b/drivers/iio/magnetometer/st_magn_buffer.c
> > > @@ -30,6 +30,16 @@ int st_magn_trig_set_state(struct iio_trigger *trig,   
> > bool state)  
> > >  	return st_sensors_set_dataready_irq(indio_dev, state);
> > >  }
> > >  
> > > +static int st_magn_buffer_preenable(struct iio_dev *indio_dev)
> > > +{
> > > +	struct st_sensor_data *mdata = iio_priv(indio_dev);
> > > +
> > > +	if (mdata->sensor_settings->drdy_irq.simple) {
> > > +		disable_irq(mdata->get_irq_data_ready(indio_dev));
> > > +	}
> > > +	return 0;
> > > +}
> > > +
> > >  static int st_magn_buffer_postenable(struct iio_dev *indio_dev)
> > >  {
> > >  	int err;
> > > @@ -45,7 +55,11 @@ static int st_magn_buffer_postenable(struct iio_dev   
> > *indio_dev)  
> > >  	if (err < 0)
> > >  		goto st_magn_buffer_postenable_error;
> > >  
> > > -	return st_sensors_set_enable(indio_dev, true);
> > > +	err = st_sensors_set_enable(indio_dev, true);
> > > +
> > > +	enable_irq(mdata->get_irq_data_ready(indio_dev));
> > > +
> > > +	return err;
> > >  
> > >  st_magn_buffer_postenable_error:
> > >  	kfree(mdata->buffer_data);
> > > @@ -70,6 +84,7 @@ static int st_magn_buffer_predisable(struct iio_dev   
> > *indio_dev)  
> > >  }
> > >  
> > >  static const struct iio_buffer_setup_ops st_magn_buffer_setup_ops = {
> > > +	.preenable = &st_magn_buffer_preenable,
> > >  	.postenable = &st_magn_buffer_postenable,
> > >  	.predisable = &st_magn_buffer_predisable,
> > >  };
> > > diff --git a/drivers/iio/magnetometer/st_magn_core.c   
> > b/drivers/iio/magnetometer/st_magn_core.c  
> > > index 72f6d1335a04..0fb0915529e9 100644
> > > --- a/drivers/iio/magnetometer/st_magn_core.c
> > > +++ b/drivers/iio/magnetometer/st_magn_core.c
> > > @@ -259,6 +259,9 @@ static const struct st_sensor_settings   
> > st_magn_sensors_settings[] = {  
> > >  				},
> > >  			},
> > >  		},
> > > +    .drdy_irq = {
> > > +      .simple = true,
> > > +    },
> > >  		.multi_read_bit = false,
> > >  		.bootime = 2,
> > >  	},
> > > diff --git a/include/linux/iio/common/st_sensors.h   
> > b/include/linux/iio/common/st_sensors.h  
> > > index f9bd6e8ab138..e25b5f033557 100644
> > > --- a/include/linux/iio/common/st_sensors.h
> > > +++ b/include/linux/iio/common/st_sensors.h
> > > @@ -154,6 +154,7 @@ struct st_sensor_int_drdy {
> > >   * struct ig1 - represents the Interrupt Generator 1 of sensors.
> > >   * @en_addr: address of the enable ig1 register.
> > >   * @en_mask: mask to write the on/off value for enable.
> > > + * @simple: the data-ready is a "very simple implementation".
> > >   */
> > >  struct st_sensor_data_ready_irq {
> > >  	struct st_sensor_int_drdy int1;
> > > @@ -168,6 +169,7 @@ struct st_sensor_data_ready_irq {
> > >  		u8 en_addr;
> > >  		u8 en_mask;
> > >  	} ig1;
> > > +	bool simple;
> > >  };
> > > 
> > >   
> > 
> >   
> 
>