From: Jonathan Cameron <jic23@kernel.org>
To: Mehdi Djait <mehdi.djait.k@gmail.com>
Cc: mazziesaccount@gmail.com, krzysztof.kozlowski+dt@linaro.org,
andriy.shevchenko@linux.intel.com, linux-iio@vger.kernel.org,
linux-kernel@vger.kernel.org, devicetree@vger.kernel.org
Subject: Re: [PATCH 3/3] iio: accel: Add support for Kionix/ROHM KX132 accelerometer
Date: Sat, 25 Mar 2023 18:12:03 +0000 [thread overview]
Message-ID: <20230325181203.1e93df7d@jic23-huawei> (raw)
In-Reply-To: <ZBnch1tSKyR4fA7H@carbian>
On Tue, 21 Mar 2023 17:34:15 +0100
Mehdi Djait <mehdi.djait.k@gmail.com> wrote:
> Hello Jonathan,
>
> On Sun, Mar 19, 2023 at 04:22:07PM +0000, Jonathan Cameron wrote:
> > On Fri, 17 Mar 2023 00:48:37 +0100
> > Mehdi Djait <mehdi.djait.k@gmail.com> wrote:
> >
> > > Add support for the basic accelerometer features such as getting the
> > > acceleration data via IIO. (raw reads, triggered buffer [data-ready] or
> > > using the WMI IRQ).
> > >
> > > Datasheet: https://kionixfs.azureedge.net/en/document/KX132-1211-Technical-Reference-Manual-Rev-5.0.pdf
> > > Signed-off-by: Mehdi Djait <mehdi.djait.k@gmail.com>
> >
> > Nothing much specific to this patch, most changes will be as a result
> > of bringing this inline with the changes suggested for patch 2.
> >
> > thanks,
> >
> > Jonathan
> > >
> > > diff --git a/drivers/iio/accel/kionix-kx022a.h b/drivers/iio/accel/kionix-kx022a.h
> > > index 3bb40e9f5613..7e43bdb37156 100644
> > > --- a/drivers/iio/accel/kionix-kx022a.h
> > > +++ b/drivers/iio/accel/kionix-kx022a.h
> > > @@ -90,8 +90,61 @@
> > > #define KX022A_REG_SELF_TEST 0x60
> > > #define KX022A_MAX_REGISTER 0x60
> > >
> > > +
> >
> > Push these down into the c file.
>
> Do you mean all REG and MASK defines ?
> Even kx022a defines them in the h file, or am I misunderstanding your
> comment ?
Hmm. Generally we only put reg defines in a header if they
are accessed from multiple c files. Otherwise it's both noise and more
code that has to be parsed when compiling (even if it's all unused / ignored).
I'm fine with this patch set just continuing with local style given they
are already there, but if you fancy moving the existing ones down to the C file
as a precursor patch, then even better!
>
> >
> > > +#define KX132_REG_WHO 0x13
> > > +#define KX132_ID 0x3d
> > > +
> > > +#define KX132_FIFO_LENGTH 86
> > > +
> > > +#define KX132_REG_CNTL2 0x1c
> > > +#define KX132_REG_CNTL 0x1b
> > > +#define KX132_MASK_RES BIT(6)
> > > +#define KX132_GSEL_2 0x0
> > > +#define KX132_GSEL_4 BIT(3)
> > > +#define KX132_GSEL_8 BIT(4)
> > > +#define KX132_GSEL_16 GENMASK(4, 3)
> > > +
> > > +#define KX132_REG_INS2 0x17
> > > +#define KX132_MASK_INS2_WMI BIT(5)
> > > +
> > > +#define KX132_REG_XADP_L 0x02
> > > +#define KX132_REG_XOUT_L 0x08
> > > +#define KX132_REG_YOUT_L 0x0a
> > > +#define KX132_REG_ZOUT_L 0x0c
> > > +#define KX132_REG_COTR 0x12
> > > +#define KX132_REG_TSCP 0x14
> > > +#define KX132_REG_INT_REL 0x1a
> > > +
> > > +#define KX132_REG_ODCNTL 0x21
> > > +
> > > +#define KX132_REG_BTS_WUF_TH 0x4a
> > > +#define KX132_REG_MAN_WAKE 0x4d
> > > +
> > > +#define KX132_REG_BUF_CNTL1 0x5e
> > > +#define KX132_REG_BUF_CNTL2 0x5f
> > > +#define KX132_REG_BUF_STATUS_1 0x60
> > > +#define KX132_REG_BUF_STATUS_2 0x61
> > > +#define KX132_MASK_BUF_SMP_LVL GENMASK(9, 0)
> > > +#define KX132_REG_BUF_CLEAR 0x62
> > > +#define KX132_REG_BUF_READ 0x63
> > > +#define KX132_ODR_SHIFT 3
> > > +#define KX132_FIFO_MAX_WMI_TH 86
> > > +
> > > +#define KX132_REG_INC1 0x22
> > > +#define KX132_REG_INC5 0x26
> > > +#define KX132_REG_INC6 0x27
> > > +#define KX132_IPOL_LOW 0
> > > +#define KX132_IPOL_HIGH KX_MASK_IPOL
> > > +#define KX132_ITYP_PULSE KX_MASK_ITYP
> > > +
> > > +#define KX132_REG_INC4 0x25
> > > +
> > > +#define KX132_REG_SELF_TEST 0x5d
> > > +#define KX132_MAX_REGISTER 0x76
> > > +
> > > enum kx022a_device_type {
> > > KX022A,
> > > + KX132,
> > As mentioned in previous review, I think this would be neater
> > done by just exporting the chip_info structures directly rather than
> > putting them in an array.
>
> I gave the reason in a response to the previous review.
If you strongly prefer the enum indexing array that's fine, but
definitely don't use the enum for the data in the tables - that should
be the pointer to the particular element of the array.
>
> --
> Kind Regards
> Mehdi Djait
next prev parent reply other threads:[~2023-03-25 17:57 UTC|newest]
Thread overview: 30+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-03-16 23:48 [PATCH 0/3] iio: accel: Add support for Kionix/ROHM KX132 accelerometer Mehdi Djait
2023-03-16 23:48 ` [PATCH 1/3] dt-bindings: iio: Add " Mehdi Djait
2023-03-19 15:54 ` Jonathan Cameron
2023-03-21 13:22 ` Mehdi Djait
2023-03-16 23:48 ` [PATCH 2/3] iio: accel: kionix-kx022a: Add chip_info structure Mehdi Djait
2023-03-17 1:01 ` kernel test robot
2023-03-17 4:57 ` kernel test robot
2023-03-17 12:06 ` Andy Shevchenko
2023-03-18 16:12 ` Mehdi Djait
2023-03-19 16:20 ` Jonathan Cameron
2023-03-20 7:17 ` Matti Vaittinen
2023-03-20 12:24 ` Jonathan Cameron
2023-03-21 15:39 ` Mehdi Djait
2023-03-20 9:35 ` Matti Vaittinen
2023-03-20 12:02 ` Andy Shevchenko
2023-03-20 12:34 ` Jonathan Cameron
2023-03-20 12:52 ` Matti Vaittinen
2023-03-21 15:56 ` Mehdi Djait
2023-03-22 6:37 ` Matti Vaittinen
2023-03-21 1:05 ` kernel test robot
2023-03-16 23:48 ` [PATCH 3/3] iio: accel: Add support for Kionix/ROHM KX132 accelerometer Mehdi Djait
2023-03-19 16:22 ` Jonathan Cameron
2023-03-21 16:34 ` Mehdi Djait
2023-03-25 18:12 ` Jonathan Cameron [this message]
2023-03-20 9:57 ` Matti Vaittinen
2023-03-17 12:07 ` [PATCH 0/3] " Andy Shevchenko
2023-03-18 15:55 ` Mehdi Djait
2023-03-19 7:43 ` Matti Vaittinen
2023-03-21 13:16 ` Mehdi Djait
2023-03-22 7:47 ` Matti Vaittinen
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20230325181203.1e93df7d@jic23-huawei \
--to=jic23@kernel.org \
--cc=andriy.shevchenko@linux.intel.com \
--cc=devicetree@vger.kernel.org \
--cc=krzysztof.kozlowski+dt@linaro.org \
--cc=linux-iio@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=mazziesaccount@gmail.com \
--cc=mehdi.djait.k@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.