| 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: <AM9PR06MB7844D0B9E09060EF6A922B6BD7D59@AM9PR06MB7844.eurprd06.prod.outlook.com>
Date: Sun, 22 May 2022 13:23:28 +0000
From: LI Qingwu <qing-wu.li@...ca-geosystems.com.cn>
To: Jonathan Cameron <jic23@...nel.org>
CC: "lars@...afoo.de" <lars@...afoo.de>,
"robh+dt@...nel.org" <robh+dt@...nel.org>,
"tomas.melin@...sala.com" <tomas.melin@...sala.com>,
"andy.shevchenko@...il.com" <andy.shevchenko@...il.com>,
"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"linux-iio@...r.kernel.org" <linux-iio@...r.kernel.org>
Subject: RE: [PATCH V7 5/5] iio: accel: sca3300: Add inclination channels
> -----Original Message-----
> From: Jonathan Cameron <jic23@...nel.org>
> Sent: Sunday, May 22, 2022 7:18 PM
> 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
>
> This email is not from Hexagon’s Office 365 instance. Please be careful while
> clicking links, opening attachments, or replying to this email.
>
>
> 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.
Thanks you Jonathan, Then I will delete it and also following incl scale available read and incli set related.
>
> > + .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];
I will delete incli_scale_map as well, change to:
*val = data->chip->incli_scale[0];
*val2 = data->chip->incli_scale[1];
> > + *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.
Thanks Jonathan!
Then I will back to " bool angle" in sca3300_chip_info, do you agree? 😊
>
>
> > + 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;
I will delete case IIO_INCLI
> > case IIO_ACCEL:
> > *vals = (const int *)data->chip->accel_scale;
> > *length = data->chip->num_accel_scales;
Powered by blists - more mailing lists