| Message ID | f2be2f0e064bc7785f9d8f7f6a8598c325b39a45.1566894261.git.lorenzo@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | iio: imu: st_lsm6dsx: remove invalid gain value for LSM9DS1 | expand |
On Tue, 27 Aug 2019 10:26:35 +0200 Lorenzo Bianconi <lorenzo@kernel.org> wrote: > Get rid of invalid sensitivity value for LSM9DS1 gyro sensor > > Fixes: 687a60feb9c6 ("iio: imu: st_lsm6dsx: add support for accel/gyro unit of lsm9ds1") > Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org> The zero degree scale is certainly odd otherwise, so good to tidy this up. Applied to the togreg branch of iio.git. Thanks, Jonathan > --- > drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c | 17 +++++++++++------ > 1 file changed, 11 insertions(+), 6 deletions(-) > > diff --git a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c > index fd152fff0a8c..c85c8be3a024 100644 > --- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c > +++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c > @@ -151,10 +151,9 @@ static const struct st_lsm6dsx_settings st_lsm6dsx_sensor_settings[] = { > .addr = 0x10, > .mask = GENMASK(4, 3), > }, > - .fs_avl[0] = { IIO_DEGREE_TO_RAD(245), 0x0 }, > - .fs_avl[1] = { IIO_DEGREE_TO_RAD(500), 0x1 }, > - .fs_avl[2] = { IIO_DEGREE_TO_RAD(0), 0x2 }, > - .fs_avl[3] = { IIO_DEGREE_TO_RAD(2000), 0x3 }, > + .fs_avl[0] = { IIO_DEGREE_TO_RAD(245), 0x0 }, > + .fs_avl[1] = { IIO_DEGREE_TO_RAD(500), 0x1 }, > + .fs_avl[2] = { IIO_DEGREE_TO_RAD(2000), 0x3 }, > }, > }, > }, > @@ -1196,13 +1195,19 @@ static ssize_t st_lsm6dsx_sysfs_scale_avail(struct device *dev, > char *buf) > { > struct st_lsm6dsx_sensor *sensor = iio_priv(dev_get_drvdata(dev)); > + const struct st_lsm6dsx_fs_table_entry *fs_table; > enum st_lsm6dsx_sensor_id id = sensor->id; > struct st_lsm6dsx_hw *hw = sensor->hw; > int i, len = 0; > > - for (i = 0; i < ST_LSM6DSX_FS_LIST_SIZE; i++) > + fs_table = &hw->settings->fs_table[id]; > + for (i = 0; i < ST_LSM6DSX_FS_LIST_SIZE; i++) { > + if (!fs_table->fs_avl[i].gain) > + break; > + > len += scnprintf(buf + len, PAGE_SIZE - len, "0.%06u ", > - hw->settings->fs_table[id].fs_avl[i].gain); > + fs_table->fs_avl[i].gain); > + } > buf[len - 1] = '\n'; > > return len;
On 27.08.19 22:08, Jonathan Cameron wrote: > On Tue, 27 Aug 2019 10:26:35 +0200 > Lorenzo Bianconi <lorenzo@kernel.org> wrote: > >> Get rid of invalid sensitivity value for LSM9DS1 gyro sensor >> >> Fixes: 687a60feb9c6 ("iio: imu: st_lsm6dsx: add support for accel/gyro unit of lsm9ds1") >> Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org> > The zero degree scale is certainly odd otherwise, so good to tidy > this up. > > Applied to the togreg branch of iio.git. > Hi Jon, you have applied this too quickly. I've left that zero value in there for a reason, see below: > Thanks, > > Jonathan > >> --- >> drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c | 17 +++++++++++------ >> 1 file changed, 11 insertions(+), 6 deletions(-) >> >> diff --git a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c >> index fd152fff0a8c..c85c8be3a024 100644 >> --- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c >> +++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c >> @@ -151,10 +151,9 @@ static const struct st_lsm6dsx_settings st_lsm6dsx_sensor_settings[] = { >> .addr = 0x10, >> .mask = GENMASK(4, 3), >> }, >> - .fs_avl[0] = { IIO_DEGREE_TO_RAD(245), 0x0 }, >> - .fs_avl[1] = { IIO_DEGREE_TO_RAD(500), 0x1 }, >> - .fs_avl[2] = { IIO_DEGREE_TO_RAD(0), 0x2 }, >> - .fs_avl[3] = { IIO_DEGREE_TO_RAD(2000), 0x3 }, >> + .fs_avl[0] = { IIO_DEGREE_TO_RAD(245), 0x0 }, >> + .fs_avl[1] = { IIO_DEGREE_TO_RAD(500), 0x1 }, >> + .fs_avl[2] = { IIO_DEGREE_TO_RAD(2000), 0x3 }, >> }, >> }, >> }, >> @@ -1196,13 +1195,19 @@ static ssize_t st_lsm6dsx_sysfs_scale_avail(struct device *dev, >> char *buf) >> { >> struct st_lsm6dsx_sensor *sensor = iio_priv(dev_get_drvdata(dev)); >> + const struct st_lsm6dsx_fs_table_entry *fs_table; >> enum st_lsm6dsx_sensor_id id = sensor->id; >> struct st_lsm6dsx_hw *hw = sensor->hw; >> int i, len = 0; >> >> - for (i = 0; i < ST_LSM6DSX_FS_LIST_SIZE; i++) >> + fs_table = &hw->settings->fs_table[id]; >> + for (i = 0; i < ST_LSM6DSX_FS_LIST_SIZE; i++) { >> + if (!fs_table->fs_avl[i].gain) >> + break; You Ooops here and it's pretty obvious! You don't have ST_LSM6DSX_FS_LIST_SIZE number of elements in the array anymore, but you try to access it (the 4th). I suggest reverting this (if not able to delete it entirely) and start over in case this "invalid" value thing hurts and needs to get fixed. I any case, there _is_ something we should do because it's not too obvious what constraints the st_lsm6dsx_sensor_settings struct definition has. It should be mostly clear when looking at the header but a few inline comments might help. thanks, martin >> + >> len += scnprintf(buf + len, PAGE_SIZE - len, "0.%06u ", >> - hw->settings->fs_table[id].fs_avl[i].gain); >> + fs_table->fs_avl[i].gain); >> + } >> buf[len - 1] = '\n'; >> >> return len; >
> On 27.08.19 22:08, Jonathan Cameron wrote: > > On Tue, 27 Aug 2019 10:26:35 +0200 > > Lorenzo Bianconi <lorenzo@kernel.org> wrote: > > > >> Get rid of invalid sensitivity value for LSM9DS1 gyro sensor > >> > >> Fixes: 687a60feb9c6 ("iio: imu: st_lsm6dsx: add support for accel/gyro unit of lsm9ds1") > >> Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org> > > The zero degree scale is certainly odd otherwise, so good to tidy > > this up. > > > > Applied to the togreg branch of iio.git. > > > > Hi Jon, > > you have applied this too quickly. I've left that zero value in there > for a reason, see below: > > > > Thanks, > > > > Jonathan > > > >> --- [...] > > You Ooops here and it's pretty obvious! You don't have > ST_LSM6DSX_FS_LIST_SIZE number of elements in the array anymore, but you > try to access it (the 4th). Hi Martin, according to pahole (x86_64): struct st_lsm6dsx_fs { [...] /* size: 8, cachelines: 1, members: 2 * }; struct st_lsm6dsx_fs_table_entry { [...] struct st_lsm6dsx_fs fs_avl[4]; /* 4 32 */ /* size: 36, cachelines: 1, members: 2 */ }; struct st_lsm6dsx_settings { [...] struct st_lsm6dsx_fs_table_entry fs_table[2]; /* 284 72 */ /* size: 464, cachelines: 8, members: 14 */ }; struct st_lsm6dsx_fs_table_entry in st_lsm6dsx_settings will always have 4 elements for fs_avl array and since the array is defined as static the uninitialized elements are set to 0. Could you please share the ops you are getting? Regards, Lorenzo > > I suggest reverting this (if not able to delete it entirely) and start > over in case this "invalid" value thing hurts and needs to get fixed. > > I any case, there _is_ something we should do because it's not too > obvious what constraints the st_lsm6dsx_sensor_settings struct > definition has. It should be mostly clear when looking at the header but > a few inline comments might help. > > thanks, > > martin > > > > >> + > >> len += scnprintf(buf + len, PAGE_SIZE - len, "0.%06u ", > >> - hw->settings->fs_table[id].fs_avl[i].gain); > >> + fs_table->fs_avl[i].gain); > >> + } > >> buf[len - 1] = '\n'; > >> > >> return len; > > >
On 29.08.19 10:37, Lorenzo Bianconi wrote: >> On 27.08.19 22:08, Jonathan Cameron wrote: >>> On Tue, 27 Aug 2019 10:26:35 +0200 >>> Lorenzo Bianconi <lorenzo@kernel.org> wrote: >>> >>>> Get rid of invalid sensitivity value for LSM9DS1 gyro sensor >>>> >>>> Fixes: 687a60feb9c6 ("iio: imu: st_lsm6dsx: add support for accel/gyro unit of lsm9ds1") >>>> Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org> >>> The zero degree scale is certainly odd otherwise, so good to tidy >>> this up. >>> >>> Applied to the togreg branch of iio.git. >>> >> >> Hi Jon, >> >> you have applied this too quickly. I've left that zero value in there >> for a reason, see below: >> >> >>> Thanks, >>> >>> Jonathan >>> >>>> --- > > [...] > >> >> You Ooops here and it's pretty obvious! You don't have >> ST_LSM6DSX_FS_LIST_SIZE number of elements in the array anymore, but you >> try to access it (the 4th). > > Hi Martin, > > according to pahole (x86_64): > > struct st_lsm6dsx_fs { > [...] > /* size: 8, cachelines: 1, members: 2 * > }; > > struct st_lsm6dsx_fs_table_entry { > [...] > struct st_lsm6dsx_fs fs_avl[4]; /* 4 32 */ > /* size: 36, cachelines: 1, members: 2 */ > }; > > struct st_lsm6dsx_settings { > [...] > struct st_lsm6dsx_fs_table_entry fs_table[2]; /* 284 72 */ > /* size: 464, cachelines: 8, members: 14 */ > }; > > struct st_lsm6dsx_fs_table_entry in st_lsm6dsx_settings will always have 4 > elements for fs_avl array and since the array is defined as static the > uninitialized elements are set to 0. > > Could you please share the ops you are getting? > How this oops during startup can look like is appended below. I know that exactly this change causes it. Can you test this too please? Given the cleanup nature of this patch, I think it's best to revert it in case of any doubt. thanks, martin Feb 14 10:12:00 pureos kernel: [ 4.056014] Data abort info: Feb 14 10:12:00 pureos kernel: [ 4.061361] bq25890-charger 0-006b: Capacity for 4184000 is 99% Feb 14 10:12:00 pureos kernel: [ 4.064751] ISV = 0, ISS = 0x00000004 Feb 14 10:12:00 pureos kernel: [ 4.070942] CM = 0, WnR = 0 Feb 14 10:12:00 pureos kernel: [ 4.076588] user pgtable: 4k pages, 48-bit VAs, pgdp=00000000e4fa6000 Feb 14 10:12:00 pureos kernel: [ 4.086256] [0000000000000018] pgd=0000000000000000 Feb 14 10:12:00 pureos kernel: [ 4.100224] Internal error: Oops: 96000004 [#1] PREEMPT SMP Feb 14 10:12:00 pureos kernel: [ 4.101985] Goodix-TS 2-005d: ID 5688, version: 0100 Feb 14 10:12:00 pureos kernel: [ 4.106038] Modules linked in: crct10dif_ce(+) st_magn_i2c(+) st_lsm6dsx_i2c(+) st_magn st_lsm6dsx snd_soc_sgtl5000(+) st_sensors_i2c st_sensors tcpci industrialio_triggered_buffer vcnl4000 kfifo_buf snd_soc_simple_card snd_soc_fsl_sai tcpm ghash_ce bq25890_charger snd_soc_gtm601 snd_soc_simple_card_utils imx_pcm_dma goodix(+) roles snd_soc_core typec sha2_ce snd_pcm_dmaengine snd_pcm imx_sdma imx2_wdt sha1_ce snvs_pwrkey gpio_vibra qoriq_thermal virt_dma snd_timer watchdog snd soundcore usb_f_acm u_serial usb_f_rndis g_multi usb_f_mass_storage u_ether libcomposite ip_tables x_tables ipv6 nf_defrag_ipv6 xhci_plat_hcd xhci_hcd usbcore dwc3 ulpi udc_core usb_common phy_fsl_imx8mq_usb Feb 14 10:12:00 pureos kernel: [ 4.113252] Goodix-TS 2-005d: Direct firmware load for goodix_5688_cfg.bin failed with error -2 Feb 14 10:12:00 pureos kernel: [ 4.174094] CPU: 2 PID: 341 Comm: systemd-udevd Tainted: G W 5.3.0-rc2-gc652c7f46913 #141 Feb 14 10:12:00 pureos kernel: [ 4.174096] Hardware name: Purism Librem 5 devkit (DT) Feb 14 10:12:00 pureos kernel: [ 4.174100] pstate: 80000005 (Nzcv daif -PAN -UAO) Feb 14 10:12:00 pureos kernel: [ 4.174113] pc : st_lsm6dsx_i2c_probe+0x18/0x80 [st_lsm6dsx_i2c] Feb 14 10:12:00 pureos kernel: [ 4.174124] lr : i2c_device_probe+0x1f0/0x2b8 Feb 14 10:12:00 pureos kernel: [ 4.174126] sp : ffff8000a4837970 Feb 14 10:12:00 pureos kernel: [ 4.174128] x29: ffff8000a4837970 x28: 0000000000000000 Feb 14 10:12:00 pureos kernel: [ 4.174132] x27: ffff000010b70000 x26: ffff8000a4837d68 Feb 14 10:12:00 pureos kernel: [ 4.174135] x25: ffff000010860000 x24: ffff000008a94038 Feb 14 10:12:00 pureos kernel: [ 4.174139] x23: ffff000008a94038 x22: ffff000008a94000 Feb 14 10:12:00 pureos kernel: [ 4.174142] x21: ffff8000a55a0400 x20: ffff000008a92000 Feb 14 10:12:00 pureos kernel: [ 4.174146] x19: ffff8000a55a0400 x18: ffffffffffffffff Feb 14 10:12:00 pureos kernel: [ 4.174149] x17: 0000000000000000 x16: 0000000000000000 Feb 14 10:12:00 pureos kernel: [ 4.174152] x15: 0000000000040000 x14: 00000000fffffff0 Feb 14 10:12:00 pureos kernel: [ 4.174156] x13: 0000000000000001 x12: 0000000000000000 Feb 14 10:12:00 pureos kernel: [ 4.174159] x11: 0000000000000000 x10: 0101010101010101 Feb 14 10:12:00 pureos kernel: [ 4.174162] x9 : fffffffffffffffc x8 : 0000000000000008 Feb 14 10:12:00 pureos kernel: [ 4.174166] x7 : 0000000000000004 x6 : 1e0e1a00f2ade4ef Feb 14 10:12:00 pureos kernel: [ 4.174169] x5 : 6f642d72001a0e1e x4 : 8080808000000000 Feb 14 10:12:00 pureos kernel: [ 4.174173] x3 : 0000000000000000 x2 : ffff000008a93000 Feb 14 10:12:00 pureos kernel: [ 4.193031] driver: 'Goodix-TS': driver_bound: bound to device '2-005d' Feb 14 10:12:00 pureos kernel: [ 4.193105] x1 : 0000000000000000 x0 : ffff8000a55a0400 Feb 14 10:12:00 pureos kernel: [ 4.199343] bus: 'i2c': really_probe: bound device 2-005d to driver Goodix-TS Feb 14 10:12:00 pureos kernel: [ 4.203445] Call trace: Feb 14 10:12:00 pureos kernel: [ 4.203456] st_lsm6dsx_i2c_probe+0x18/0x80 [st_lsm6dsx_i2c] Feb 14 10:12:00 pureos kernel: [ 4.203462] i2c_device_probe+0x1f0/0x2b8 Feb 14 10:12:00 pureos kernel: [ 4.203468] really_probe+0x168/0x368 Feb 14 10:12:00 pureos kernel: [ 4.203471] driver_probe_device.part.2+0x10c/0x128 Feb 14 10:12:00 pureos kernel: [ 4.203475] device_driver_attach+0x74/0xa0 Feb 14 10:12:00 pureos kernel: [ 4.203478] __driver_attach+0x84/0x130 Feb 14 10:12:00 pureos kernel: [ 4.203481] bus_for_each_dev+0x68/0xc8 Feb 14 10:12:00 pureos kernel: [ 4.203484] driver_attach+0x20/0x28 Feb 14 10:12:00 pureos kernel: [ 4.203487] bus_add_driver+0xd4/0x1f8 Feb 14 10:12:00 pureos kernel: [ 4.203490] driver_register+0x60/0x110 Feb 14 10:12:00 pureos kernel: [ 4.203497] i2c_register_driver+0x44/0x98 Feb 14 10:12:00 pureos kernel: [ 4.216908] devices_kset: Moving sound to end of list Feb 14 10:12:00 pureos kernel: [ 4.217738] st_lsm6dsx_driver_init+0x1c/0x1000 [st_lsm6dsx_i2c] Feb 14 10:12:00 pureos kernel: [ 4.217744] do_one_initcall+0x58/0x1a8 Feb 14 10:12:00 pureos kernel: [ 4.217749] do_init_module+0x54/0x1d4 Feb 14 10:12:00 pureos kernel: [ 4.217752] load_module+0x1998/0x1c40 Feb 14 10:12:00 pureos kernel: [ 4.217755] __se_sys_finit_module+0xc0/0xd8 Feb 14 10:12:00 pureos kernel: [ 4.217761] __arm64_sys_finit_module+0x14/0x20 Feb 14 10:12:00 pureos kernel: [ 4.223515] PM: Moving platform:sound to end of list Feb 14 10:12:00 pureos kernel: [ 4.228823] el0_svc_common.constprop.0+0xb0/0x168 Feb 14 10:12:00 pureos kernel: [ 4.228827] el0_svc_handler+0x18/0x20 Feb 14 10:12:00 pureos kernel: [ 4.228830] el0_svc+0x8/0xc Feb 14 10:12:00 pureos kernel: [ 4.228838] Code: d2800003 910003fd a90153f3 aa0003f3 (f9400c34) Feb 14 10:12:00 pureos kernel: [ 4.228843] ---[ end trace 4933f7b108d54662 ]---
> On 29.08.19 10:37, Lorenzo Bianconi wrote: > >> On 27.08.19 22:08, Jonathan Cameron wrote: > >>> On Tue, 27 Aug 2019 10:26:35 +0200 > >>> Lorenzo Bianconi <lorenzo@kernel.org> wrote: > >>> > >>>> Get rid of invalid sensitivity value for LSM9DS1 gyro sensor > >>>> > >>>> Fixes: 687a60feb9c6 ("iio: imu: st_lsm6dsx: add support for accel/gyro unit of lsm9ds1") > >>>> Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org> > >>> The zero degree scale is certainly odd otherwise, so good to tidy > >>> this up. > >>> > >>> Applied to the togreg branch of iio.git. > >>> [...] > > struct st_lsm6dsx_fs_table_entry in st_lsm6dsx_settings will always have 4 > > elements for fs_avl array and since the array is defined as static the > > uninitialized elements are set to 0. > > > > Could you please share the ops you are getting? > > > > How this oops during startup can look like is appended below. I know > that exactly this change causes it. Can you test this too please? I did it but I have no issues > > Given the cleanup nature of this patch, I think it's best to revert it > in case of any doubt. > > thanks, > > martin > is it the full ops? It seems some parts are missing. Are you running some userspace aps reading in_anglvel_scale_available or reading/writing in in_anglvel_scale? Could you please double check if the following patch helps? (just compiled) Regards, Lorenzo --- drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h | 1 + drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c | 28 +++++++++++++------- drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_shub.c | 3 ++- 3 files changed, 22 insertions(+), 10 deletions(-) diff --git a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h index 5e3cd96b0059..a6cd736fd273 100644 --- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h +++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h @@ -100,6 +100,7 @@ struct st_lsm6dsx_fs { struct st_lsm6dsx_fs_table_entry { struct st_lsm6dsx_reg reg; struct st_lsm6dsx_fs fs_avl[ST_LSM6DSX_FS_LIST_SIZE]; + int len; }; /** diff --git a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c index 2d3495560136..37a2aa211757 100644 --- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c +++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c @@ -145,6 +145,7 @@ static const struct st_lsm6dsx_settings st_lsm6dsx_sensor_settings[] = { .fs_avl[1] = { IIO_G_TO_M_S_2(122), 0x2 }, .fs_avl[2] = { IIO_G_TO_M_S_2(244), 0x3 }, .fs_avl[3] = { IIO_G_TO_M_S_2(732), 0x1 }, + .len = 4, }, [ST_LSM6DSX_ID_GYRO] = { .reg = { @@ -154,6 +155,7 @@ static const struct st_lsm6dsx_settings st_lsm6dsx_sensor_settings[] = { .fs_avl[0] = { IIO_DEGREE_TO_RAD(245), 0x0 }, .fs_avl[1] = { IIO_DEGREE_TO_RAD(500), 0x1 }, .fs_avl[2] = { IIO_DEGREE_TO_RAD(2000), 0x3 }, + .len = 3, }, }, }, @@ -215,6 +217,7 @@ static const struct st_lsm6dsx_settings st_lsm6dsx_sensor_settings[] = { .fs_avl[1] = { IIO_G_TO_M_S_2(122), 0x2 }, .fs_avl[2] = { IIO_G_TO_M_S_2(244), 0x3 }, .fs_avl[3] = { IIO_G_TO_M_S_2(488), 0x1 }, + .len = 4, }, [ST_LSM6DSX_ID_GYRO] = { .reg = { @@ -225,6 +228,7 @@ static const struct st_lsm6dsx_settings st_lsm6dsx_sensor_settings[] = { .fs_avl[1] = { IIO_DEGREE_TO_RAD(17500), 0x1 }, .fs_avl[2] = { IIO_DEGREE_TO_RAD(35000), 0x2 }, .fs_avl[3] = { IIO_DEGREE_TO_RAD(70000), 0x3 }, + .len = 4, }, }, .decimator = { @@ -327,6 +331,7 @@ static const struct st_lsm6dsx_settings st_lsm6dsx_sensor_settings[] = { .fs_avl[1] = { IIO_G_TO_M_S_2(122), 0x2 }, .fs_avl[2] = { IIO_G_TO_M_S_2(244), 0x3 }, .fs_avl[3] = { IIO_G_TO_M_S_2(488), 0x1 }, + .len = 4, }, [ST_LSM6DSX_ID_GYRO] = { .reg = { @@ -337,6 +342,7 @@ static const struct st_lsm6dsx_settings st_lsm6dsx_sensor_settings[] = { .fs_avl[1] = { IIO_DEGREE_TO_RAD(17500), 0x1 }, .fs_avl[2] = { IIO_DEGREE_TO_RAD(35000), 0x2 }, .fs_avl[3] = { IIO_DEGREE_TO_RAD(70000), 0x3 }, + .len = 4, }, }, .decimator = { @@ -448,6 +454,7 @@ static const struct st_lsm6dsx_settings st_lsm6dsx_sensor_settings[] = { .fs_avl[1] = { IIO_G_TO_M_S_2(122), 0x2 }, .fs_avl[2] = { IIO_G_TO_M_S_2(244), 0x3 }, .fs_avl[3] = { IIO_G_TO_M_S_2(488), 0x1 }, + .len = 4, }, [ST_LSM6DSX_ID_GYRO] = { .reg = { @@ -458,6 +465,7 @@ static const struct st_lsm6dsx_settings st_lsm6dsx_sensor_settings[] = { .fs_avl[1] = { IIO_DEGREE_TO_RAD(17500), 0x1 }, .fs_avl[2] = { IIO_DEGREE_TO_RAD(35000), 0x2 }, .fs_avl[3] = { IIO_DEGREE_TO_RAD(70000), 0x3 }, + .len = 4, }, }, .decimator = { @@ -563,6 +571,7 @@ static const struct st_lsm6dsx_settings st_lsm6dsx_sensor_settings[] = { .fs_avl[1] = { IIO_G_TO_M_S_2(122), 0x2 }, .fs_avl[2] = { IIO_G_TO_M_S_2(244), 0x3 }, .fs_avl[3] = { IIO_G_TO_M_S_2(488), 0x1 }, + .len = 4, }, [ST_LSM6DSX_ID_GYRO] = { .reg = { @@ -573,6 +582,7 @@ static const struct st_lsm6dsx_settings st_lsm6dsx_sensor_settings[] = { .fs_avl[1] = { IIO_DEGREE_TO_RAD(17500), 0x1 }, .fs_avl[2] = { IIO_DEGREE_TO_RAD(35000), 0x2 }, .fs_avl[3] = { IIO_DEGREE_TO_RAD(70000), 0x3 }, + .len = 4, }, }, .batch = { @@ -693,6 +703,7 @@ static const struct st_lsm6dsx_settings st_lsm6dsx_sensor_settings[] = { .fs_avl[1] = { IIO_G_TO_M_S_2(122), 0x2 }, .fs_avl[2] = { IIO_G_TO_M_S_2(244), 0x3 }, .fs_avl[3] = { IIO_G_TO_M_S_2(488), 0x1 }, + .len = 4, }, [ST_LSM6DSX_ID_GYRO] = { .reg = { @@ -703,6 +714,7 @@ static const struct st_lsm6dsx_settings st_lsm6dsx_sensor_settings[] = { .fs_avl[1] = { IIO_DEGREE_TO_RAD(17500), 0x1 }, .fs_avl[2] = { IIO_DEGREE_TO_RAD(35000), 0x2 }, .fs_avl[3] = { IIO_DEGREE_TO_RAD(70000), 0x3 }, + .len = 4, }, }, .batch = { @@ -800,6 +812,7 @@ static const struct st_lsm6dsx_settings st_lsm6dsx_sensor_settings[] = { .fs_avl[1] = { IIO_G_TO_M_S_2(122), 0x2 }, .fs_avl[2] = { IIO_G_TO_M_S_2(244), 0x3 }, .fs_avl[3] = { IIO_G_TO_M_S_2(488), 0x1 }, + .len = 4, }, [ST_LSM6DSX_ID_GYRO] = { .reg = { @@ -810,6 +823,7 @@ static const struct st_lsm6dsx_settings st_lsm6dsx_sensor_settings[] = { .fs_avl[1] = { IIO_DEGREE_TO_RAD(17500), 0x1 }, .fs_avl[2] = { IIO_DEGREE_TO_RAD(35000), 0x2 }, .fs_avl[3] = { IIO_DEGREE_TO_RAD(70000), 0x3 }, + .len = 4, }, }, .batch = { @@ -933,11 +947,12 @@ static int st_lsm6dsx_set_full_scale(struct st_lsm6dsx_sensor *sensor, int i, err; fs_table = &sensor->hw->settings->fs_table[sensor->id]; - for (i = 0; i < ST_LSM6DSX_FS_LIST_SIZE; i++) + for (i = 0; i < fs_table->len; i++) { if (fs_table->fs_avl[i].gain == gain) break; + } - if (i == ST_LSM6DSX_FS_LIST_SIZE) + if (i == fs_table->len) return -EINVAL; data = ST_LSM6DSX_SHIFT_VAL(fs_table->fs_avl[i].val, @@ -1196,18 +1211,13 @@ static ssize_t st_lsm6dsx_sysfs_scale_avail(struct device *dev, { struct st_lsm6dsx_sensor *sensor = iio_priv(dev_get_drvdata(dev)); const struct st_lsm6dsx_fs_table_entry *fs_table; - enum st_lsm6dsx_sensor_id id = sensor->id; struct st_lsm6dsx_hw *hw = sensor->hw; int i, len = 0; - fs_table = &hw->settings->fs_table[id]; - for (i = 0; i < ST_LSM6DSX_FS_LIST_SIZE; i++) { - if (!fs_table->fs_avl[i].gain) - break; - + fs_table = &hw->settings->fs_table[sensor->id]; + for (i = 0; i < fs_table->len; i++) len += scnprintf(buf + len, PAGE_SIZE - len, "0.%06u ", fs_table->fs_avl[i].gain); - } buf[len - 1] = '\n'; return len; diff --git a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_shub.c b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_shub.c index 66fbcd94642d..dcf349278e8f 100644 --- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_shub.c +++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_shub.c @@ -61,6 +61,7 @@ static const struct st_lsm6dsx_ext_dev_settings st_lsm6dsx_ext_dev_table[] = { .gain = 1500, .val = 0x0, }, /* 1500 uG/LSB */ + .len = 1, }, .temp_comp = { .addr = 0x60, @@ -555,7 +556,7 @@ static ssize_t st_lsm6dsx_shub_scale_avail(struct device *dev, int i, len = 0; settings = sensor->ext_info.settings; - for (i = 0; i < ST_LSM6DSX_FS_LIST_SIZE; i++) { + for (i = 0; i < settings->fs_table.len; i++) { u16 val = settings->fs_table.fs_avl[i].gain; if (val > 0)
On 30.08.19 09:23, Lorenzo Bianconi wrote: >> On 29.08.19 10:37, Lorenzo Bianconi wrote: >>>> On 27.08.19 22:08, Jonathan Cameron wrote: >>>>> On Tue, 27 Aug 2019 10:26:35 +0200 >>>>> Lorenzo Bianconi <lorenzo@kernel.org> wrote: >>>>> >>>>>> Get rid of invalid sensitivity value for LSM9DS1 gyro sensor >>>>>> >>>>>> Fixes: 687a60feb9c6 ("iio: imu: st_lsm6dsx: add support for accel/gyro unit of lsm9ds1") >>>>>> Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org> >>>>> The zero degree scale is certainly odd otherwise, so good to tidy >>>>> this up. >>>>> >>>>> Applied to the togreg branch of iio.git. >>>>> > > [...] > >>> struct st_lsm6dsx_fs_table_entry in st_lsm6dsx_settings will always have 4 >>> elements for fs_avl array and since the array is defined as static the >>> uninitialized elements are set to 0. >>> >>> Could you please share the ops you are getting? >>> >> >> How this oops during startup can look like is appended below. I know >> that exactly this change causes it. Can you test this too please? > > I did it but I have no issues > >> >> Given the cleanup nature of this patch, I think it's best to revert it >> in case of any doubt. >> >> thanks, >> >> martin >> > > is it the full ops? It seems some parts are missing. > Are you running some userspace aps reading in_anglvel_scale_available or reading/writing in in_anglvel_scale? > Could you please double check if the following patch helps? (just compiled) > it does not, and you're right, the problem should be somewhere else. I've yet to debug it further. thanks, martin
> On 30.08.19 09:23, Lorenzo Bianconi wrote: > >> On 29.08.19 10:37, Lorenzo Bianconi wrote: > >>>> On 27.08.19 22:08, Jonathan Cameron wrote: > >>>>> On Tue, 27 Aug 2019 10:26:35 +0200 > >>>>> Lorenzo Bianconi <lorenzo@kernel.org> wrote: > >>>>> > >>>>>> Get rid of invalid sensitivity value for LSM9DS1 gyro sensor > >>>>>> > >>>>>> Fixes: 687a60feb9c6 ("iio: imu: st_lsm6dsx: add support for accel/gyro unit of lsm9ds1") > >>>>>> Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org> > >>>>> The zero degree scale is certainly odd otherwise, so good to tidy > >>>>> this up. > >>>>> > >>>>> Applied to the togreg branch of iio.git. > >>>>> > > > > [...] > > > >>> struct st_lsm6dsx_fs_table_entry in st_lsm6dsx_settings will always have 4 > >>> elements for fs_avl array and since the array is defined as static the > >>> uninitialized elements are set to 0. > >>> > >>> Could you please share the ops you are getting? > >>> > >> > >> How this oops during startup can look like is appended below. I know > >> that exactly this change causes it. Can you test this too please? > > > > I did it but I have no issues > > > >> > >> Given the cleanup nature of this patch, I think it's best to revert it > >> in case of any doubt. > >> > >> thanks, > >> > >> martin > >> > > > > is it the full ops? It seems some parts are missing. > > Are you running some userspace aps reading in_anglvel_scale_available or reading/writing in in_anglvel_scale? > > Could you please double check if the following patch helps? (just compiled) > > > > it does not, and you're right, the problem should be somewhere else. > I've yet to debug it further. > > thanks, Looking at the previous patch I spotted an issue (not related to the one you are facing)..actually we can set device gain to 0 forcing to 0 sensor output. I will post a formal patch to fix it. Regards, Lorenzo > > martin > >
On Sat, 31 Aug 2019 11:32:24 +0200 Lorenzo Bianconi <lorenzo@kernel.org> wrote: > > On 30.08.19 09:23, Lorenzo Bianconi wrote: > > >> On 29.08.19 10:37, Lorenzo Bianconi wrote: > > >>>> On 27.08.19 22:08, Jonathan Cameron wrote: > > >>>>> On Tue, 27 Aug 2019 10:26:35 +0200 > > >>>>> Lorenzo Bianconi <lorenzo@kernel.org> wrote: > > >>>>> > > >>>>>> Get rid of invalid sensitivity value for LSM9DS1 gyro sensor > > >>>>>> > > >>>>>> Fixes: 687a60feb9c6 ("iio: imu: st_lsm6dsx: add support for accel/gyro unit of lsm9ds1") > > >>>>>> Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org> > > >>>>> The zero degree scale is certainly odd otherwise, so good to tidy > > >>>>> this up. > > >>>>> > > >>>>> Applied to the togreg branch of iio.git. > > >>>>> > > > > > > [...] > > > > > >>> struct st_lsm6dsx_fs_table_entry in st_lsm6dsx_settings will always have 4 > > >>> elements for fs_avl array and since the array is defined as static the > > >>> uninitialized elements are set to 0. > > >>> > > >>> Could you please share the ops you are getting? > > >>> > > >> > > >> How this oops during startup can look like is appended below. I know > > >> that exactly this change causes it. Can you test this too please? > > > > > > I did it but I have no issues > > > > > >> > > >> Given the cleanup nature of this patch, I think it's best to revert it > > >> in case of any doubt. > > >> > > >> thanks, > > >> > > >> martin > > >> > > > > > > is it the full ops? It seems some parts are missing. > > > Are you running some userspace aps reading in_anglvel_scale_available or reading/writing in in_anglvel_scale? > > > Could you please double check if the following patch helps? (just compiled) > > > > > > > it does not, and you're right, the problem should be somewhere else. > > I've yet to debug it further. > > > > thanks, > > Looking at the previous patch I spotted an issue (not related to the one you > are facing)..actually we can set device gain to 0 forcing to 0 sensor output. > I will post a formal patch to fix it. > Rather than messing around with a pull request that has already gone to Greg, I'm going to leave this as it is for now. The trace doesn't make a lot of sense to me and whilst a bit messy (as fixed up by Lorenzo's follow up) I can't see why things would crash. So needs debugging and that isn't a problem given we are only looking at putting this support into rc1. Not ideal, but there is time to work out what is wrong and fix it up! Thanks, Jonathan > Regards, > Lorenzo > > > > > martin > > > >
On 01.09.19 16:50, Jonathan Cameron wrote: [...] > > Rather than messing around with a pull request that has already gone > to Greg, I'm going to leave this as it is for now. > > The trace doesn't make a lot of sense to me and whilst a bit messy (as fixed > up by Lorenzo's follow up) I can't see why things would crash. > > So needs debugging and that isn't a problem given we are only looking > at putting this support into rc1. Not ideal, but there is time > to work out what is wrong and fix it up! > > Thanks, > > Jonathan > for the record, I found the problem I've had here. It's been introduced with the DEV_NAME change and I sent a fix (and question): https://lore.kernel.org/linux-iio/20190903051802.22716-1-martin.kepplinger@puri.sm/T/#u thanks, martin
On Tue, 3 Sep 2019 07:22:20 +0200 Martin Kepplinger <martink@posteo.de> wrote: > On 01.09.19 16:50, Jonathan Cameron wrote: > > [...] > > > > > Rather than messing around with a pull request that has already gone > > to Greg, I'm going to leave this as it is for now. > > > > The trace doesn't make a lot of sense to me and whilst a bit messy (as fixed > > up by Lorenzo's follow up) I can't see why things would crash. > > > > So needs debugging and that isn't a problem given we are only looking > > at putting this support into rc1. Not ideal, but there is time > > to work out what is wrong and fix it up! > > > > Thanks, > > > > Jonathan > > > > for the record, I found the problem I've had here. It's been introduced > with the DEV_NAME change and I sent a fix (and question): > > https://lore.kernel.org/linux-iio/20190903051802.22716-1-martin.kepplinger@puri.sm/T/#u > Doh. This one is entirely my fault as I messed up that last minute edit :( Greg, this bug is in my outstanding pull request. If you would prefer to pick it from lore and add my Acked-by that's great. If not I can send this after rc1. Thanks, Jonathan
diff --git a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c index fd152fff0a8c..c85c8be3a024 100644 --- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c +++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c @@ -151,10 +151,9 @@ static const struct st_lsm6dsx_settings st_lsm6dsx_sensor_settings[] = { .addr = 0x10, .mask = GENMASK(4, 3), }, - .fs_avl[0] = { IIO_DEGREE_TO_RAD(245), 0x0 }, - .fs_avl[1] = { IIO_DEGREE_TO_RAD(500), 0x1 }, - .fs_avl[2] = { IIO_DEGREE_TO_RAD(0), 0x2 }, - .fs_avl[3] = { IIO_DEGREE_TO_RAD(2000), 0x3 }, + .fs_avl[0] = { IIO_DEGREE_TO_RAD(245), 0x0 }, + .fs_avl[1] = { IIO_DEGREE_TO_RAD(500), 0x1 }, + .fs_avl[2] = { IIO_DEGREE_TO_RAD(2000), 0x3 }, }, }, }, @@ -1196,13 +1195,19 @@ static ssize_t st_lsm6dsx_sysfs_scale_avail(struct device *dev, char *buf) { struct st_lsm6dsx_sensor *sensor = iio_priv(dev_get_drvdata(dev)); + const struct st_lsm6dsx_fs_table_entry *fs_table; enum st_lsm6dsx_sensor_id id = sensor->id; struct st_lsm6dsx_hw *hw = sensor->hw; int i, len = 0; - for (i = 0; i < ST_LSM6DSX_FS_LIST_SIZE; i++) + fs_table = &hw->settings->fs_table[id]; + for (i = 0; i < ST_LSM6DSX_FS_LIST_SIZE; i++) { + if (!fs_table->fs_avl[i].gain) + break; + len += scnprintf(buf + len, PAGE_SIZE - len, "0.%06u ", - hw->settings->fs_table[id].fs_avl[i].gain); + fs_table->fs_avl[i].gain); + } buf[len - 1] = '\n'; return len;
Get rid of invalid sensitivity value for LSM9DS1 gyro sensor Fixes: 687a60feb9c6 ("iio: imu: st_lsm6dsx: add support for accel/gyro unit of lsm9ds1") Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org> --- drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c | 17 +++++++++++------ 1 file changed, 11 insertions(+), 6 deletions(-)