On Tue, 7 Apr 2020 15:14:27 +0200 (CEST) yarl-baudig@xxxxxxxxxx wrote: > ---- Message d'origine ---- > > De : Jonathan Cameron <jic23@xxxxxxxxxx> > > À : yarl-baudig@xxxxxxxxxx > > Sujet : Re: lsm303dlhc magnetometer: please help > > Date : 05/04/2020 15:03:56 Europe/Paris > > Copie à : denis.ciocca@xxxxxx; > > linux-iio@xxxxxxxxxxxxxxx > > > > On Sat, 4 Apr 2020 14:43:04 +0200 (CEST) > > yarl-baudig@xxxxxxxxxx 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; > > > }; > > > > > > > > > > > >
