Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
From:   LI Qingwu <qing-wu.li@leica-geosystems.com.cn>
To:     Jonathan Cameron <jic23@kernel.org>
CC:     "lars@metafoo.de" <lars@metafoo.de>,
        "tomas.melin@vaisala.com" <tomas.melin@vaisala.com>,
        "nuno.sa@analog.com" <nuno.sa@analog.com>,
        "linux-iio@vger.kernel.org" <linux-iio@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        GEO-CHHER-bsp-development 
        <bsp-development.geo@leica-geosystems.com>
Subject: RE: [PATCH V1 1/1] iio: accel: sca3300: Extend the trigger buffer
 from 16 to 32 bytes
Thread-Topic: [PATCH V1 1/1] iio: accel: sca3300: Extend the trigger buffer
 from 16 to 32 bytes
Thread-Index: AQHYjPKNuiUUBllm20O/wlqzm/hoCa1pujMAgAOuCXA=
Date:   Mon, 4 Jul 2022 01:01:46 +0000
Message-ID: <AM9PR06MB78447C75482FA9179A601BFDD7BE9@AM9PR06MB7844.eurprd06.prod.outlook.com>
References: <20220701023030.2527019-1-Qing-wu.Li@leica-geosystems.com.cn>
        <20220701023030.2527019-2-Qing-wu.Li@leica-geosystems.com.cn>
 <20220701174602.68b20753@jic23-huawei>
In-Reply-To: <20220701174602.68b20753@jic23-huawei>
Accept-Language: zh-CN, en-US
Content-Language: en-US
x-ms-exchange-antispam-messagedata-chunkcount: 1
x-ms-exchange-antispam-messagedata-0: =?us-ascii?Q?apUc6KYy60Za0GSBT7gTPYvz7OEOUnbJ3uZXs4guFexzLElA4Klfh8hn8Nb3?=
 =?us-ascii?Q?ZCGljkqetKOZRp1EeagtNAEVmcPfI/fQz6FwCrn79MEADKRpjdZQpbB/AT5+?=
 =?us-ascii?Q?ZwAGT59ckXRXKizxEkV5nCGADq0L01DsVE8UDWkfkfGeYCtgbWadmtyzSQPz?=
 =?us-ascii?Q?QEY6hjbPBRpFMTTQC5qObrecy0zCskWl3pOb1vw8GSMT9LVbJKJHnm+b+Xib?=
 =?us-ascii?Q?9vPm3r5eu7Xx+mHM99shHUXQg0Y1Iuz2VX5SLrmsVWSIQri08zMVz9sl8jGN?=
 =?us-ascii?Q?1RIWtBmXJo5GmmUNBye2Mff53Nqd000GUvRUpbgAtlZcCml+4VC3DvXEmGX5?=
 =?us-ascii?Q?tBrvLEtQu2FA6kMoNhyQUfhzEtdMuJKtdlrKxw+bSuqxa9X0l1CF1MKl52of?=
 =?us-ascii?Q?7s/HlT0rPqwJ33iFLz3CICg4MSsxkCQy/3MtJ5bDMFW9Ottuk78tFaIZ34ED?=
 =?us-ascii?Q?zc8KReKOqHDuJrODqfaXqgEx/SF2zyOjvu7DNhNzi502GWxwOvzYQ38Qtf5P?=
 =?us-ascii?Q?3rdHWmJn79znL1ea1P5g2WxP3zLAR3/13s7lmi6vwumC4kr4pZ7+78t8Ijyt?=
 =?us-ascii?Q?99f4yDjGeB4YHyKWPrC9Tc5za8VtaUCezEZDZdjh7LUHKDp6t7/yxYKqoPnO?=
 =?us-ascii?Q?A2hT4pPmxwN9Bhjr23lWLA1rt0rDbAtNL1VS84m8DX2TQuqiyPCkP7y64UKW?=
 =?us-ascii?Q?EoPYNDpEQ8D09MjA5TnR7ZEP+7vesolAYo/E32Kt/ZthIYK9Ood33HbbUsfJ?=
 =?us-ascii?Q?idU/UZorBiZRTlepiixYLQMIhNwtI2xg5G73r7Hg3yHLgpHuiGJs/lGU/CNh?=
 =?us-ascii?Q?xhlS6Y8HP0Tv3na5BEtymYmSgGiQQziFD39HxltJ0/ZSpD0xyOZfky/LlILc?=
 =?us-ascii?Q?sNsyQQQB7iRQrT+6nXCajcHcpzdgDlVvzqf4d7so4wLPgWraeIUai8jUFZ+1?=
 =?us-ascii?Q?ZazL+dCtGLO930rHPvm6PqHT6OU6cpGdehshRuGaMakZzPK1oJhOh1X+FvE8?=
 =?us-ascii?Q?Oo1CZaFQTqBushpOC2AFj/Ce2zXo+RuoawKqmF90upLbLfcyp6QFM5OhIJH/?=
 =?us-ascii?Q?5AFBNQVjH7//GII1DZCidD7PQ238Bgh+ehDGcLCWFV8oEezpxvHVYOYULyd1?=
 =?us-ascii?Q?FfDSmzJAEK1v2qPlqxjxk9MSz8xnE7K89sROtuLp8Ts6A9MHkjXqwjWyQUce?=
 =?us-ascii?Q?phokKKt4bydxMhrHyTPdO169+YyNK5ffOOPMoLdgYZzOuAF/m/C37NbWeO8S?=
 =?us-ascii?Q?bZlL8lAeR8VSK5q4FEbix+u1sAiIFuJPEBGCHrZNVwcWQk+K64J6T4UgM8rT?=
 =?us-ascii?Q?BLwh1rdn0r9JTDd+NFuGPOV+dz9SNyNdYx+/gGOuWzFDVNhW+KIfO6LXun4v?=
 =?us-ascii?Q?3th+0MtER1luq7FM9WSUwXLGHvqZDgQHF/vtmccX4G1/Z+z8qRRV1y7QrQlE?=
 =?us-ascii?Q?OmXQ+cZ790dNyLxW3ihIYX/uj4cIxLPImhXeOL0voZWMwZAd8cyt1nNbcvXh?=
 =?us-ascii?Q?fKNBObAQuEzBnXUF/b3dGRSQ4jW5ujF1RJQJkOIc7RJbTeo2UyqruYjlpA8p?=
 =?us-ascii?Q?Oa0jBJ9IfxqEFwC/uoh4UDGrPFyNfE+yO7pdtqrXLbKA6679FAgazd4xS0Zq?=
 =?us-ascii?Q?uQF/856/SW15/o0JFWXflug=3D?=
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-OriginatorOrg: leica-geosystems.com.cn
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-AuthSource: AM9PR06MB7844.eurprd06.prod.outlook.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 44f681a0-6c5b-4fec-4b92-08da5d58c487
X-MS-Exchange-CrossTenant-originalarrivaltime: 04 Jul 2022 01:01:46.1428
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 1b16ab3e-b8f6-4fe3-9f3e-2db7fe549f6a
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: 8AISLDMmJ0WIczr+d+emIksBvx6kyMYSwwaVe6TrfW4i/1qQkpLP9QSQSSmwasu1wZ7w8PciZUkMmk0p7OJlFsrb88pw86UzIcdgSgsZrVSOfjsce2wXQWC1W+4R8fpK
X-MS-Exchange-Transport-CrossTenantHeadersStamped: VE1PR06MB7038
X-Spam-Status: No, score=-2.1 required=5.0 tests=BAYES_00,DKIM_SIGNED,
        DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_NONE,
        RCVD_IN_MSPIKE_H2,SPF_HELO_PASS,SPF_PASS,T_SCC_BODY_TEXT_LINE
        autolearn=ham autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org


> -----Original Message-----
> From: Jonathan Cameron <jic23@kernel.org>
> Sent: Saturday, July 2, 2022 12:46 AM
> To: LI Qingwu <Qing-wu.Li@leica-geosystems.com.cn>
> Cc: lars@metafoo.de; tomas.melin@vaisala.com; nuno.sa@analog.com;
> linux-iio@vger.kernel.org; linux-kernel@vger.kernel.org;
> GEO-CHHER-bsp-development <bsp-development.geo@leica-geosystems.com>
> Subject: Re: [PATCH V1 1/1] iio: accel: sca3300: Extend the trigger buffe=
r from
> 16 to 32 bytes
>=20
> [Some people who received this message don't often get email from
> jic23@kernel.org. Learn why this is important at
> https://aka.ms/LearnAboutSenderIdentification ]
>=20
> This email is not from Hexagon's Office 365 instance. Please be careful w=
hile
> clicking links, opening attachments, or replying to this email.
>=20
>=20
> On Fri,  1 Jul 2022 02:30:30 +0000
> LI Qingwu <Qing-wu.Li@leica-geosystems.com.cn> wrote:
>=20
> > After added inclination angle channels, the trigger buffer size is
> > insufficient. Extend the buffer size from 16 to 32 bytes, and change
> > the trigger buffer from the struct to a u8 array to adapt the sensor
> > with/without inclination angles output.
> > New trigger buffer data:
> >   - SCA3300: 3 accel channels, temp, and timestamp.
> >   - SCL3300: 3 accel channels, temp, 3 incli channels, and timestamp.
> > Readjustment the scan index to make it consistent with the buffer data.
> >
> > Signed-off-by: LI Qingwu <Qing-wu.Li@leica-geosystems.com.cn>
>=20
> Hi.
>=20
> Looks good. A trivial suggestion inline to make the code a little more 's=
elf
> documenting'.  It's a minor change so if you are happy with the suggestio=
n I
> can tweak that whilst applying.
>=20
> Thanks,
>=20
> Jonathan
>=20

Yes, sure, absolutely agree, Thank you!

LI Qingwu

> > ---
> >  drivers/iio/accel/sca3300.c | 29 ++++++++++++++++++-----------
> >  1 file changed, 18 insertions(+), 11 deletions(-)
> >
> > diff --git a/drivers/iio/accel/sca3300.c b/drivers/iio/accel/sca3300.c
> > index 3c4827bfef53..820dfb635bf1 100644
> > --- a/drivers/iio/accel/sca3300.c
> > +++ b/drivers/iio/accel/sca3300.c
> > @@ -47,12 +47,20 @@
> >  #define SCL3300_REG_ANG_CTRL 0x0C
> >  #define SCL3300_ANG_ENABLE   0x1F
> >
> > +/*
> > + * Buffer size max case:
> > + * Three accel channels, two bytes per channel.
> > + * Temperature channel, two bytes.
> > + * Three incli channels, two bytes per channel.
> > + * Timestamp channel, eight bytes.
> > + */
> > +#define SCA3300_MAX_BUFFER_SIZE (ALIGN(2 * 7, sizeof(s64)) +
> > +sizeof(s64))
>=20
> Instead of the 2 use sizeof(s16)
>  Also now you don't have timestamp in your enum sca3000_scan_indexes you
> could add a 'tail' element to the enum such as SCA3000_SCAN_MAX then use
> that instead of the 7 here.  Hopefully that would make this more self
> documenting.


>=20
> > +
> >  enum sca3300_scan_indexes {
> >       SCA3300_ACC_X =3D 0,
> >       SCA3300_ACC_Y,
> >       SCA3300_ACC_Z,
> >       SCA3300_TEMP,
> > -     SCA3300_TIMESTAMP,
> >       SCA3300_INCLI_X,
> >       SCA3300_INCLI_Y,
> >       SCA3300_INCLI_Z,
> > @@ -140,10 +148,10 @@ static const struct iio_chan_spec scl3300_channel=
s[]
> =3D {
> >       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),
> > +     IIO_CHAN_SOFT_TIMESTAMP(7),
> >  };
> >
> >  static const unsigned long sca3300_scan_masks[] =3D { @@ -184,7 +192,9
> > @@ struct sca3300_chip_info {
> >   * @spi: SPI device structure
> >   * @lock: Data buffer lock
> >   * @chip: Sensor chip specific information
> > - * @scan: Triggered buffer. Four channel 16-bit data + 64-bit
> > timestamp
> > + * @buffer: Triggered buffer:
> > + *          -SCA3300: 4 channel 16-bit data + 64-bit timestamp
> > + *          -SCL3300: 7 channel 16-bit data + 64-bit timestamp
> >   * @txbuf: Transmit buffer
> >   * @rxbuf: Receive buffer
> >   */
> > @@ -192,10 +202,7 @@ struct sca3300_data {
> >       struct spi_device *spi;
> >       struct mutex lock;
> >       const struct sca3300_chip_info *chip;
> > -     struct {
> > -             s16 channels[4];
> > -             s64 ts __aligned(sizeof(s64));
> > -     } scan;
> > +     u8 buffer[SCA3300_MAX_BUFFER_SIZE] __aligned(sizeof(s64));
> >       u8 txbuf[4] __aligned(IIO_DMA_MINALIGN);
> >       u8 rxbuf[4];
> >  };
> > @@ -484,21 +491,21 @@ static irqreturn_t sca3300_trigger_handler(int ir=
q,
> void *p)
> >       struct iio_dev *indio_dev =3D pf->indio_dev;
> >       struct sca3300_data *data =3D iio_priv(indio_dev);
> >       int bit, ret, val, i =3D 0;
> > +     s16 *channels =3D (s16 *)data->buffer;
> >
> >       for_each_set_bit(bit, indio_dev->active_scan_mask,
> >                        indio_dev->masklength) {
> > -             ret =3D sca3300_read_reg(data,
> sca3300_channels[bit].address,
> > -                                    &val);
> > +             ret =3D sca3300_read_reg(data,
> > + indio_dev->channels[bit].address, &val);
> >               if (ret) {
> >                       dev_err_ratelimited(&data->spi->dev,
> >                               "failed to read register, error: %d\n",
> ret);
> >                       /* handled, but bailing out due to errors */
> >                       goto out;
> >               }
> > -             data->scan.channels[i++] =3D val;
> > +             channels[i++] =3D val;
> >       }
> >
> > -     iio_push_to_buffers_with_timestamp(indio_dev, &data->scan,
> > +     iio_push_to_buffers_with_timestamp(indio_dev, data->buffer,
> >
> iio_get_time_ns(indio_dev));
> >  out:
> >       iio_trigger_notify_done(indio_dev->trig);