Messages in this thread | | | Date | Sun, 23 Sep 2012 23:38:19 +0200 | From | Éric Piel <> | Subject | Re: [PATCH v2] lis3lv02d: Add STMicroelectronics lis331dlh digital accelerometer |
| |
On 22-08-12 08:30, AnilKumar Ch wrote: > This patch adds support for lis331dlh digital accelerometer to the > lis3lv02d driver family. Adds ID field for detecting the lis331dlh > module, based on this ID field lis3lv02d driver will export the > lis331dlh module functionality.
Hello AnilKumar,
Sorry for taking so long to review your patch. Overall, it looks great :-)
There are just a few points that almost code-style/comment that needs to be fixed. See below. Could you fix these small issues, and submit a new patch for Andrew to pick it in his queue?
Here is my: Reviewed-by: Éric Piel <eric.piel@tremplin-utc.net>
> > Signed-off-by: AnilKumar Ch <anilkumar@ti.com> > --- > Changes from v1: > - Removed G range sysfs entry from v1 > - Modified documentation to specify lis331dlh is supported > > Documentation/misc-devices/lis3lv02d | 3 ++- > drivers/misc/lis3lv02d/lis3lv02d.c | 42 +++++++++++++++++++++++++++++- > drivers/misc/lis3lv02d/lis3lv02d.h | 44 +++++++++++++++++++++++++++----- > drivers/misc/lis3lv02d/lis3lv02d_i2c.c | 7 ++++- > 4 files changed, 87 insertions(+), 9 deletions(-) > > diff --git a/Documentation/misc-devices/lis3lv02d b/Documentation/misc-devices/lis3lv02d > index f1a4ec8..af815b9 100644 > --- a/Documentation/misc-devices/lis3lv02d > +++ b/Documentation/misc-devices/lis3lv02d > @@ -4,7 +4,8 @@ Kernel driver lis3lv02d > Supported chips: > > * STMicroelectronics LIS3LV02DL, LIS3LV02DQ (12 bits precision) > - * STMicroelectronics LIS302DL, LIS3L02DQ, LIS331DL (8 bits) > + * STMicroelectronics LIS302DL, LIS3L02DQ, LIS331DL (8 bits) and > + LIS331DLH (16 bits) > > Authors: > Yan Burman <burman.yan@gmail.com> > diff --git a/drivers/misc/lis3lv02d/lis3lv02d.c b/drivers/misc/lis3lv02d/lis3lv02d.c > index 29d12a7..c0a9199 100644 > --- a/drivers/misc/lis3lv02d/lis3lv02d.c > +++ b/drivers/misc/lis3lv02d/lis3lv02d.c > @@ -80,6 +80,14 @@ > #define LIS3_SENSITIVITY_12B ((LIS3_ACCURACY * 1000) / 1024) > #define LIS3_SENSITIVITY_8B (18 * LIS3_ACCURACY) > > +/* > + * LIS3331DLH spec says 1LSBs corresponds 4G/1024 -> 1LSB is 1000/1024 mG. > + * Sensitivity values for +/-2G, outdata in 12 bits for +/-2G scale. so 4 > + * bits adjustment is required > + */ > +#define LIS3DLH_SENSITIVITY_2G ((LIS3_ACCURACY * 1000) / 1024) > +#define SHIFT_ADJ_2G 4 > + You said later: > > Typo mistake this should be 4G/4096 Could you fix the comment. Even better, change LIS3331DLH to LIS331DLH
Also, if I understand correctly, there is currently no support for changing the sensitivity. It stays at 2G all the time, right? In such case, please add a sentence in this comment that the driver does only support the 2G sensitivity for now.
> #define LIS3_DEFAULT_FUZZ_12B 3 > #define LIS3_DEFAULT_FLAT_12B 3 > #define LIS3_DEFAULT_FUZZ_8B 1 > @@ -133,6 +141,19 @@ static s16 lis3lv02d_read_12(struct lis3lv02d *lis3, int reg) > return (s16)((hi << 8) | lo); > } > > +/* 12bits for 2G range, 13 bits for 4G range and 14 bits for 8G range */ > +static s16 lis3lv02d_read_16(struct lis3lv02d *lis3, int reg) > +{ > + u8 lo, hi; > + int v; > + > + lis3->read(lis3, reg - 1, &lo); > + lis3->read(lis3, reg, &hi); > + v = (int) ((hi << 8) | lo); > + > + return (s16) v >> lis3->shift_adj; > +} As the method reads 12, 13, or 14 bits, it's a bit tricky to call it "*_read_16". I'd suggest maybe "lis3lv02d_read_12_14", "lis3lv02d_read_adj_16", or even "lis331dlh_read_data". Pick the one you prefer :-)
> /** > * lis3lv02d_get_axis - For the given axis, give the value converted > * @axis: 1,2,3 - can also be negative > @@ -193,6 +214,7 @@ static void lis3lv02d_get_xyz(struct lis3lv02d *lis3, int *x, int *y, int *z) > static int lis3_12_rates[4] = {40, 160, 640, 2560}; > static int lis3_8_rates[2] = {100, 400}; > static int lis3_3dc_rates[16] = {0, 1, 10, 25, 50, 100, 200, 400, 1600, 5000}; > +static int lis3_3dlh_rates[4] = {50, 100, 400, 1000}; > > /* ODR is Output Data Rate */ > static int lis3lv02d_get_odr(struct lis3lv02d *lis3) > @@ -265,7 +287,7 @@ static int lis3lv02d_selftest(struct lis3lv02d *lis3, s16 results[3]) > (LIS3_IRQ1_DATA_READY | LIS3_IRQ2_DATA_READY)); > } > > - if (lis3->whoami == WAI_3DC) { > + if ((lis3->whoami == WAI_3DC) || (lis3->whoami == WAI_3DLH)) { > ctlreg = CTRL_REG4; > selftest = CTRL4_ST0; > } else { > @@ -396,9 +418,17 @@ int lis3lv02d_poweron(struct lis3lv02d *lis3) > lis3->read(lis3, CTRL_REG2, ®); > if (lis3->whoami == WAI_12B) > reg |= CTRL2_BDU | CTRL2_BOOT; > + else if (lis3->whoami == WAI_3DLH) > + reg |= CTRL2_BOOT_3DLH; > else > reg |= CTRL2_BOOT_8B; > lis3->write(lis3, CTRL_REG2, reg); > + > + if (lis3->whoami == WAI_3DLH) { > + lis3->read(lis3, CTRL_REG4, ®); > + reg |= CTRL4_BDU; > + lis3->write(lis3, CTRL_REG4, reg); > + } > } > > err = lis3lv02d_get_pwron_wait(lis3); > @@ -954,6 +984,16 @@ int lis3lv02d_init_device(struct lis3lv02d *lis3) > lis3->odr_mask = CTRL1_ODR0|CTRL1_ODR1|CTRL1_ODR2|CTRL1_ODR3; > lis3->scale = LIS3_SENSITIVITY_8B; > break; > + case WAI_3DLH: > + pr_info("16 bits 3DLH sensor found\n"); > + lis3->read_data = lis3lv02d_read_16; > + lis3->mdps_max_val = 2048; /* 12 bits for 2G */ > + lis3->shift_adj = SHIFT_ADJ_2G; > + lis3->pwron_delay = LIS3_PWRON_DELAY_WAI_8B; > + lis3->odrs = lis3_3dlh_rates; > + lis3->odr_mask = CTRL1_DR0 | CTRL1_DR1; > + lis3->scale = LIS3DLH_SENSITIVITY_2G; > + break; > default: > pr_err("unknown sensor type 0x%X\n", lis3->whoami); > return -EINVAL; > diff --git a/drivers/misc/lis3lv02d/lis3lv02d.h b/drivers/misc/lis3lv02d/lis3lv02d.h > index 2b1482a..c1a545e 100644 > --- a/drivers/misc/lis3lv02d/lis3lv02d.h > +++ b/drivers/misc/lis3lv02d/lis3lv02d.h > @@ -26,12 +26,12 @@ > /* > * This driver tries to support the "digital" accelerometer chips from > * STMicroelectronics such as LIS3LV02DL, LIS302DL, LIS3L02DQ, LIS331DL, > - * LIS35DE, or LIS202DL. They are very similar in terms of programming, with > - * almost the same registers. In addition to differing on physical properties, > - * they differ on the number of axes (2/3), precision (8/12 bits), and special > - * features (freefall detection, click...). Unfortunately, not all the > - * differences can be probed via a register. > - * They can be connected either via I²C or SPI. > + * LIS331DLH, LIS35DE, or LIS202DL. They are very similar in terms of > + * programming, with almost the same registers. In addition to differing > + * on physical properties, they differ on the number of axes (2/3), > + * precision (8/12 bits), and special features (freefall detection, > + * click...). Unfortunately, not all the differences can be probed via > + * a register. They can be connected either via I²C or SPI. > */ > > #include <linux/lis3lv02d.h> > @@ -96,12 +96,22 @@ enum lis3lv02d_reg { > }; > > enum lis3_who_am_i { > + WAI_3DLH = 0x32, /* 16 bits: LIS331DLH */ > WAI_3DC = 0x33, /* 8 bits: LIS3DC, HP3DC */ > WAI_12B = 0x3A, /* 12 bits: LIS3LV02D[LQ]... */ > WAI_8B = 0x3B, /* 8 bits: LIS[23]02D[LQ]... */ > WAI_6B = 0x52, /* 6 bits: LIS331DLF - not supported */ > }; > > +enum lis3_type { > + LIS3DC, > + HP3DC, > + LIS3LV02D, > + LIS2302D, > + LIS331DLF, > + LIS331DLH, > +}; > + > enum lis3lv02d_ctrl1_12b { > CTRL1_Xen = 0x01, > CTRL1_Yen = 0x02, > @@ -129,6 +139,27 @@ enum lis3lv02d_ctrl1_3dc { > CTRL1_ODR3 = 0x80, > }; > > +enum lis331dlh_ctrl1 { > + CTRL1_DR0 = 0x08, > + CTRL1_DR1 = 0x10, > + CTRL1_PM0 = 0x20, > + CTRL1_PM1 = 0x40, > + CTRL1_PM2 = 0x80, > +}; > + > +enum lis331dlh_ctrl2 { > + CTRL2_HPEN1 = 0x04, > + CTRL2_HPEN2 = 0x08, > + CTRL2_FDS_3DLH = 0x10, > + CTRL2_BOOT_3DLH = 0x80, > +}; > + > +enum lis331dlh_ctrl4 { > + CTRL4_STSIGN = 0x08, > + CTRL4_BLE = 0x40, > + CTRL4_BDU = 0x80, > +}; > + > enum lis3lv02d_ctrl2 { > CTRL2_DAS = 0x01, > CTRL2_SIM = 0x02, > @@ -279,6 +310,7 @@ struct lis3lv02d { > int data_ready_count[2]; > atomic_t wake_thread; > unsigned char irq_cfg; > + unsigned int shift_adj; > > struct lis3lv02d_platform_data *pdata; /* for passing board config */ > struct mutex mutex; /* Serialize poll and selftest */ > diff --git a/drivers/misc/lis3lv02d/lis3lv02d_i2c.c b/drivers/misc/lis3lv02d/lis3lv02d_i2c.c > index c02fea0..98cf9ba 100644 > --- a/drivers/misc/lis3lv02d/lis3lv02d_i2c.c > +++ b/drivers/misc/lis3lv02d/lis3lv02d_i2c.c > @@ -90,7 +90,11 @@ static int lis3_i2c_init(struct lis3lv02d *lis3) > if (ret < 0) > return ret; > > - reg |= CTRL1_PD0 | CTRL1_Xen | CTRL1_Yen | CTRL1_Zen; > + if (lis3->whoami == WAI_3DLH) > + reg |= CTRL1_PM0 | CTRL1_Xen | CTRL1_Yen | CTRL1_Zen; > + else > + reg |= CTRL1_PD0 | CTRL1_Xen | CTRL1_Yen | CTRL1_Zen; > + > return lis3->write(lis3, CTRL_REG1, reg); > } > > @@ -232,6 +236,7 @@ static int lis3_i2c_runtime_resume(struct device *dev) > > static const struct i2c_device_id lis3lv02d_id[] = { > {"lis3lv02d", 0 }, > + {"lis331dlh", LIS331DLH}, I'm not fully grasping the meaning of this table. But as there seems to be no user of the second value, I imagine this value just has to be different for each entry? If so, I'd recommend to change the 0 to LIS3LV02D, to make it clear that LIS331DLH != 0. Or is this value meaningful for the user, in which case we cannot change it anymore?
Cheers, Éric -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |