| 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: <20220522121749.01807970@jic23-huawei>
Date: Sun, 22 May 2022 12:17:49 +0100
From: Jonathan Cameron <jic23@...nel.org>
To: LI Qingwu <Qing-wu.Li@...ca-geosystems.com.cn>
Cc: lars@...afoo.de, robh+dt@...nel.org, tomas.melin@...sala.com,
andy.shevchenko@...il.com, devicetree@...r.kernel.org,
linux-kernel@...r.kernel.org, linux-iio@...r.kernel.org
Subject: Re: [PATCH V7 5/5] iio: accel: sca3300: Add inclination channels
On Sun, 22 May 2022 07:04:19 +0000
LI Qingwu <Qing-wu.Li@...ca-geosystems.com.cn> wrote:
> Different from SCA3300, SCL3300 can output inclination angles.
> Angles are formed from acceleration with following equations:
> ANG_X = atan2(accx , sqrt(pow(accy , 2) + pow(accz , 2)))
> ANG_Y = atan2(accy , sqrt(pow(accx , 2) + pow(accz , 2)))
> ANG_Z = atan2(accz , sqrt(pow(accx , 2) + pow(accy , 2)))
>
> The commit adds the output of the raw value, scale
> and scale_available of angles.
>
> New interfaces:
> in_incli_scale
> in_incli_scale_available
> in_incli_x_raw
> in_incli_y_raw
> in_incli_z_raw
> Data converted by application of scale to degrees.
>
> Signed-off-by: LI Qingwu <Qing-wu.Li@...ca-geosystems.com.cn>
Hi Li,
A few small things to tidy up in this patch. See inline.
Thanks,
Jonathan
> ---
> drivers/iio/accel/sca3300.c | 76 +++++++++++++++++++++++++++++++++++--
> 1 file changed, 73 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/iio/accel/sca3300.c b/drivers/iio/accel/sca3300.c
> index 10bedb14d4f0..9094f16458de 100644
> --- a/drivers/iio/accel/sca3300.c
> +++ b/drivers/iio/accel/sca3300.c
> @@ -44,12 +44,18 @@
> #define SCA3300_VALUE_RS_ERROR 0x3
> #define SCA3300_MASK_RS_STATUS GENMASK(1, 0)
>
> +#define SCL3300_REG_ANG_CTRL 0x0C
> +#define SCL3300_ANG_ENABLE 0x1F
> +
> enum sca3300_scan_indexes {
> SCA3300_ACC_X = 0,
> SCA3300_ACC_Y,
> SCA3300_ACC_Z,
> SCA3300_TEMP,
> SCA3300_TIMESTAMP,
> + SCA3300_INCLI_X,
> + SCA3300_INCLI_Y,
> + SCA3300_INCLI_Z,
> };
>
> #define SCA3300_ACCEL_CHANNEL(index, reg, axis) { \
> @@ -73,6 +79,24 @@ enum sca3300_scan_indexes {
> }, \
> }
>
> +#define SCA3300_INCLI_CHANNEL(index, reg, axis) { \
> + .type = IIO_INCLI, \
> + .address = reg, \
> + .modified = 1, \
> + .channel2 = IIO_MOD_##axis, \
> + .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE), \
> + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \
> + .info_mask_shared_by_type_available = \
> + BIT(IIO_CHAN_INFO_SCALE), \
I don't think you need this line wrap.
> + .scan_index = index, \
> + .scan_type = { \
> + .sign = 's', \
> + .realbits = 16, \
> + .storagebits = 16, \
> + .endianness = IIO_CPU, \
> + }, \
> +}
> +
> #define SCA3300_TEMP_CHANNEL(index, reg) { \
> .type = IIO_TEMP, \
> .address = reg, \
> @@ -106,15 +130,36 @@ static const int sca3300_accel_scale_map[] = {0, 1, 2, 2};
> static const int scl3300_accel_scale[][2] = {{0, 167}, {0, 333}, {0, 83}};
> static const int scl3300_accel_scale_map[] = {0, 1, 2};
>
> +static const int scl3300_incli_scale[][2] = {{0, 5495}};
> +static const int scl3300_incli_scale_map[] = {0, 0, 0};
> +
> static const int sca3300_avail_modes_map[] = {0, 1, 2, 3};
> static const int scl3300_avail_modes_map[] = {0, 1, 3};
>
> +static const struct iio_chan_spec scl3300_channels[] = {
> + SCA3300_ACCEL_CHANNEL(SCA3300_ACC_X, 0x1, X),
> + SCA3300_ACCEL_CHANNEL(SCA3300_ACC_Y, 0x2, Y),
> + SCA3300_ACCEL_CHANNEL(SCA3300_ACC_Z, 0x3, Z),
> + SCA3300_TEMP_CHANNEL(SCA3300_TEMP, 0x05),
> + IIO_CHAN_SOFT_TIMESTAMP(4),
> + SCA3300_INCLI_CHANNEL(SCA3300_INCLI_X, 0x09, X),
> + SCA3300_INCLI_CHANNEL(SCA3300_INCLI_Y, 0x0A, Y),
> + SCA3300_INCLI_CHANNEL(SCA3300_INCLI_Z, 0x0B, Z),
> +};
> +
> static const unsigned long sca3300_scan_masks[] = {
> BIT(SCA3300_ACC_X) | BIT(SCA3300_ACC_Y) | BIT(SCA3300_ACC_Z) |
> BIT(SCA3300_TEMP),
> 0
> };
>
> +static const unsigned long scl3300_scan_masks[] = {
> + BIT(SCA3300_ACC_X) | BIT(SCA3300_ACC_Y) | BIT(SCA3300_ACC_Z) |
> + BIT(SCA3300_TEMP) |
> + BIT(SCA3300_INCLI_X) | BIT(SCA3300_INCLI_Y) | BIT(SCA3300_INCLI_Z),
> + 0
> +};
> +
> struct sca3300_chip_info {
> const char *name;
> const unsigned long *scan_masks;
> @@ -123,6 +168,9 @@ struct sca3300_chip_info {
> u8 num_accel_scales;
> const int (*accel_scale)[2];
> const int *accel_scale_map;
> + const int (*incli_scale)[2];
> + const int *incli_scale_map;
> + u8 num_incli_scales;
> u8 num_freqs;
> const int *freq_table;
> const int *freq_map;
> @@ -131,6 +179,7 @@ struct sca3300_chip_info {
> u8 chip_id;
> };
>
> +
Unrelated and unwanted white space chagne.
> /**
> * struct sca3300_data - device data
> * @spi: SPI device structure
> @@ -168,12 +217,16 @@ static const struct sca3300_chip_info sca3300_chip_tbl[] = {
> .num_avail_modes = 4,
> .chip_id = SCA3300_WHOAMI_ID,
> },
> - { .scan_masks = sca3300_scan_masks,
> - .channels = sca3300_channels,
> - .num_channels = ARRAY_SIZE(sca3300_channels),
> + {
ah. The whitespace change belongs in patch 4.
> + .scan_masks = scl3300_scan_masks,
> + .channels = scl3300_channels,
> + .num_channels = ARRAY_SIZE(scl3300_channels),
> .num_accel_scales = ARRAY_SIZE(scl3300_accel_scale)*2,
> .accel_scale = scl3300_accel_scale,
> .accel_scale_map = scl3300_accel_scale_map,
> + .incli_scale = scl3300_incli_scale,
> + .incli_scale_map = scl3300_incli_scale_map,
> + .num_incli_scales = ARRAY_SIZE(scl3300_incli_scale)*2,
> .num_freqs = ARRAY_SIZE(scl3300_lp_freq),
> .freq_table = scl3300_lp_freq,
> .freq_map = scl3300_lp_freq_map,
> @@ -400,6 +453,11 @@ static int sca3300_read_raw(struct iio_dev *indio_dev,
> if (ret)
> return ret;
> switch (chan->type) {
> + case IIO_INCLI:
> + index = data->chip->incli_scale_map[index];
> + *val = data->chip->incli_scale[index][0];
> + *val2 = data->chip->incli_scale[index][1];
> + return IIO_VAL_INT_PLUS_MICRO;
> case IIO_ACCEL:
> index = data->chip->accel_scale_map[index];
> *val = data->chip->accel_scale[index][0];
> @@ -486,6 +544,13 @@ static int sca3300_init(struct sca3300_data *sca_data,
>
> sca_data->chip = &sca3300_chip_tbl[i];
>
> + if (value == SCL3300_WHOAMI_ID) {
I don't like feature decisions being based directly on a WHOAMI.
That tends not to scale as you add more supported parts to the driver.
I think this is the only case you now have.
Better would be to add a 'inclination_supported' bool to your chip info
and make the decision based on that. I wouldn't use whether incli_scale
is set as that would hurt readability when compared to a new member
serving just this purpose.
> + ret = sca3300_write_reg(sca_data, SCL3300_REG_ANG_CTRL,
> + SCL3300_ANG_ENABLE);
> + if (ret)
> + return ret;
> + }
> +
> return 0;
> }
>
> @@ -521,6 +586,11 @@ static int sca3300_read_avail(struct iio_dev *indio_dev,
> switch (mask) {
> case IIO_CHAN_INFO_SCALE:
> switch (chan->type) {
> + case IIO_INCLI:
> + *vals = (const int *)data->chip->incli_scale;
> + *length = data->chip->num_incli_scales;
> + *type = IIO_VAL_INT_PLUS_MICRO;
> + return IIO_AVAIL_LIST;
> case IIO_ACCEL:
> *vals = (const int *)data->chip->accel_scale;
> *length = data->chip->num_accel_scales;
Powered by blists - more mailing lists