| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <505F814B.4040709@tremplin-utc.net>
Date: Sun, 23 Sep 2012 23:38:19 +0200
From: Éric Piel <eric.piel@...mplin-utc.net>
To: AnilKumar Ch <anilkumar@...com>
CC: arnd@...db.de, gregkh@...uxfoundation.org, jic23@....ac.uk,
greg@...ah.com, akpm@...ux-foundation.org,
broonie@...nsource.wolfsonmicro.com, dmitry.torokhov@...il.com,
linux-kernel@...r.kernel.org
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@...mplin-utc.net>
>
> Signed-off-by: AnilKumar Ch <anilkumar@...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@...il.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@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists