From: Jonathan Cameron <jic23@kernel.org>
To: "Michał Mirosław" <mirq-linux@rere.qmqm.pl>
Cc: Hartmut Knaack <knaack.h@gmx.de>,
Lars-Peter Clausen <lars@metafoo.de>,
Peter Meerwald-Stadler <pmeerw@pmeerw.net>,
linux-iio@vger.kernel.org
Subject: Re: [PATCH 3/6] iio: accel: kxcjk1013: rename registers for KXTF9 compatibility
Date: Sun, 20 Aug 2017 10:52:22 +0100 [thread overview]
Message-ID: <20170820105222.36dcaa38@archlinux> (raw)
In-Reply-To: <97ed7c895e7eec28b66d4e591c30d40b00322820.1502979014.git.mirq-linux@rere.qmqm.pl>
On Thu, 17 Aug 2017 16:21:36 +0200
Michał Mirosław <mirq-linux@rere.qmqm.pl> wrote:
> Rename some registers that are shared between KXTF9 and KXCJK.
>
> Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>
Hi Michał,
I'm not keen on this change. Going for generic names is always
fragile as all it takes is another part coming along which is almost
but not quite the same as you shared register set and we end up with
a mess.
General convention for both register values and driver naming is they
should be named after one supported part rather than trying to find
a generic name that covers all supported parts.
So please revert this change and resend the series.
Sorry to be a pain, but this has gone wrong quite a lot of times in
the past!
Jonathan
> ---
> drivers/iio/accel/kxcjk-1013.c | 123 ++++++++++++++++++++++-------------------
> 1 file changed, 65 insertions(+), 58 deletions(-)
>
> diff --git a/drivers/iio/accel/kxcjk-1013.c b/drivers/iio/accel/kxcjk-1013.c
> index 80a6508d6370..27147b687471 100644
> --- a/drivers/iio/accel/kxcjk-1013.c
> +++ b/drivers/iio/accel/kxcjk-1013.c
> @@ -48,39 +48,48 @@
>
> #define KXCJK1013_REG_DCST_RESP 0x0C
> #define KXCJK1013_REG_WHO_AM_I 0x0F
> -#define KXCJK1013_REG_INT_SRC1 0x16
> +#define KXREG_INT_SRC_DATA 0x16
> #define KXCJK1013_REG_INT_SRC2 0x17
> #define KXCJK1013_REG_STATUS_REG 0x18
> #define KXCJK1013_REG_INT_REL 0x1A
> -#define KXCJK1013_REG_CTRL1 0x1B
> -#define KXCJK1013_REG_CTRL2 0x1D
> -#define KXCJK1013_REG_INT_CTRL1 0x1E
> -#define KXCJK1013_REG_INT_CTRL2 0x1F
> +#define KXREG_CTRL1 0x1B
> +#define KXREG_CTRL3 0x1D
> +#define KXREG_INT_CTRL1 0x1E
> +#define KXREG_INT_CTRL2 0x1F
> #define KXCJK1013_REG_DATA_CTRL 0x21
> #define KXCJK1013_REG_WAKE_TIMER 0x29
> #define KXCJK1013_REG_SELF_TEST 0x3A
> #define KXCJK1013_REG_WAKE_THRES 0x6A
>
> -#define KXCJK1013_REG_CTRL1_BIT_PC1 BIT(7)
> -#define KXCJK1013_REG_CTRL1_BIT_RES BIT(6)
> -#define KXCJK1013_REG_CTRL1_BIT_DRDY BIT(5)
> -#define KXCJK1013_REG_CTRL1_BIT_GSEL1 BIT(4)
> -#define KXCJK1013_REG_CTRL1_BIT_GSEL0 BIT(3)
> -#define KXCJK1013_REG_CTRL1_BIT_WUFE BIT(1)
> -#define KXCJK1013_REG_INT_REG1_BIT_IEA BIT(4)
> -#define KXCJK1013_REG_INT_REG1_BIT_IEN BIT(5)
> +#define KXREG_CTRL1_BIT_PC1 BIT(7)
> +#define KXREG_CTRL1_BIT_RES BIT(6)
> +#define KXREG_CTRL1_BIT_DRDY BIT(5)
> +#define KXREG_CTRL1_BIT_GSEL1 BIT(4)
> +#define KXREG_CTRL1_BIT_GSEL0 BIT(3)
> +#define KXREG_CTRL1_BIT_TDTE BIT(2)
> +#define KXREG_CTRL1_BIT_WUFE BIT(1)
> +#define KXREG_CTRL1_BIT_TPE BIT(0)
> +
> +#define KXREG_INT_CTRL1_BIT_IEL BIT(3)
> +#define KXREG_INT_CTRL1_BIT_IEA BIT(4)
> +#define KXREG_INT_CTRL1_BIT_IEN BIT(5)
>
> #define KXCJK1013_DATA_MASK_12_BIT 0x0FFF
> #define KXCJK1013_MAX_STARTUP_TIME_US 100000
>
> #define KXCJK1013_SLEEP_DELAY_MS 2000
>
> -#define KXCJK1013_REG_INT_SRC2_BIT_ZP BIT(0)
> -#define KXCJK1013_REG_INT_SRC2_BIT_ZN BIT(1)
> -#define KXCJK1013_REG_INT_SRC2_BIT_YP BIT(2)
> -#define KXCJK1013_REG_INT_SRC2_BIT_YN BIT(3)
> -#define KXCJK1013_REG_INT_SRC2_BIT_XP BIT(4)
> -#define KXCJK1013_REG_INT_SRC2_BIT_XN BIT(5)
> +/* KXCJK: INT_SOURCE1 */
> +#define KXREG_INT_BIT_WUFS BIT(1)
> +#define KXREG_INT_BIT_DRDY BIT(4)
> +
> +/* KXCJK: INT_SOURCE2: motion detect */
> +#define KXREG_MOTION_INT_BIT_ZP BIT(0)
> +#define KXREG_MOTION_INT_BIT_ZN BIT(1)
> +#define KXREG_MOTION_INT_BIT_YP BIT(2)
> +#define KXREG_MOTION_INT_BIT_YN BIT(3)
> +#define KXREG_MOTION_INT_BIT_XP BIT(4)
> +#define KXREG_MOTION_INT_BIT_XN BIT(5)
>
> #define KXCJK1013_DEFAULT_WAKE_THRES 1
>
> @@ -215,19 +224,19 @@ static int kxcjk1013_set_mode(struct kxcjk1013_data *data,
> {
> int ret;
>
> - ret = i2c_smbus_read_byte_data(data->client, KXCJK1013_REG_CTRL1);
> + ret = i2c_smbus_read_byte_data(data->client, KXREG_CTRL1);
> if (ret < 0) {
> dev_err(&data->client->dev, "Error reading reg_ctrl1\n");
> return ret;
> }
>
> if (mode == STANDBY)
> - ret &= ~KXCJK1013_REG_CTRL1_BIT_PC1;
> + ret &= ~KXREG_CTRL1_BIT_PC1;
> else
> - ret |= KXCJK1013_REG_CTRL1_BIT_PC1;
> + ret |= KXREG_CTRL1_BIT_PC1;
>
> ret = i2c_smbus_write_byte_data(data->client,
> - KXCJK1013_REG_CTRL1, ret);
> + KXREG_CTRL1, ret);
> if (ret < 0) {
> dev_err(&data->client->dev, "Error writing reg_ctrl1\n");
> return ret;
> @@ -241,13 +250,13 @@ static int kxcjk1013_get_mode(struct kxcjk1013_data *data,
> {
> int ret;
>
> - ret = i2c_smbus_read_byte_data(data->client, KXCJK1013_REG_CTRL1);
> + ret = i2c_smbus_read_byte_data(data->client, KXREG_CTRL1);
> if (ret < 0) {
> dev_err(&data->client->dev, "Error reading reg_ctrl1\n");
> return ret;
> }
>
> - if (ret & KXCJK1013_REG_CTRL1_BIT_PC1)
> + if (ret & KXREG_CTRL1_BIT_PC1)
> *mode = OPERATION;
> else
> *mode = STANDBY;
> @@ -259,19 +268,19 @@ static int kxcjk1013_set_range(struct kxcjk1013_data *data, int range_index)
> {
> int ret;
>
> - ret = i2c_smbus_read_byte_data(data->client, KXCJK1013_REG_CTRL1);
> + ret = i2c_smbus_read_byte_data(data->client, KXREG_CTRL1);
> if (ret < 0) {
> dev_err(&data->client->dev, "Error reading reg_ctrl1\n");
> return ret;
> }
>
> - ret &= ~(KXCJK1013_REG_CTRL1_BIT_GSEL0 |
> - KXCJK1013_REG_CTRL1_BIT_GSEL1);
> + ret &= ~(KXREG_CTRL1_BIT_GSEL0 |
> + KXREG_CTRL1_BIT_GSEL1);
> ret |= (KXCJK1013_scale_table[range_index].gsel_0 << 3);
> ret |= (KXCJK1013_scale_table[range_index].gsel_1 << 4);
>
> ret = i2c_smbus_write_byte_data(data->client,
> - KXCJK1013_REG_CTRL1,
> + KXREG_CTRL1,
> ret);
> if (ret < 0) {
> dev_err(&data->client->dev, "Error writing reg_ctrl1\n");
> @@ -299,16 +308,16 @@ static int kxcjk1013_chip_init(struct kxcjk1013_data *data)
> if (ret < 0)
> return ret;
>
> - ret = i2c_smbus_read_byte_data(data->client, KXCJK1013_REG_CTRL1);
> + ret = i2c_smbus_read_byte_data(data->client, KXREG_CTRL1);
> if (ret < 0) {
> dev_err(&data->client->dev, "Error reading reg_ctrl1\n");
> return ret;
> }
>
> /* Set 12 bit mode */
> - ret |= KXCJK1013_REG_CTRL1_BIT_RES;
> + ret |= KXREG_CTRL1_BIT_RES;
>
> - ret = i2c_smbus_write_byte_data(data->client, KXCJK1013_REG_CTRL1,
> + ret = i2c_smbus_write_byte_data(data->client, KXREG_CTRL1,
> ret);
> if (ret < 0) {
> dev_err(&data->client->dev, "Error reading reg_ctrl\n");
> @@ -329,18 +338,18 @@ static int kxcjk1013_chip_init(struct kxcjk1013_data *data)
> data->odr_bits = ret;
>
> /* Set up INT polarity */
> - ret = i2c_smbus_read_byte_data(data->client, KXCJK1013_REG_INT_CTRL1);
> + ret = i2c_smbus_read_byte_data(data->client, KXREG_INT_CTRL1);
> if (ret < 0) {
> dev_err(&data->client->dev, "Error reading reg_int_ctrl1\n");
> return ret;
> }
>
> if (data->active_high_intr)
> - ret |= KXCJK1013_REG_INT_REG1_BIT_IEA;
> + ret |= KXREG_INT_CTRL1_BIT_IEA;
> else
> - ret &= ~KXCJK1013_REG_INT_REG1_BIT_IEA;
> + ret &= ~KXREG_INT_CTRL1_BIT_IEA;
>
> - ret = i2c_smbus_write_byte_data(data->client, KXCJK1013_REG_INT_CTRL1,
> + ret = i2c_smbus_write_byte_data(data->client, KXREG_INT_CTRL1,
> ret);
> if (ret < 0) {
> dev_err(&data->client->dev, "Error writing reg_int_ctrl1\n");
> @@ -437,37 +446,37 @@ static int kxcjk1013_setup_any_motion_interrupt(struct kxcjk1013_data *data,
> if (ret < 0)
> return ret;
>
> - ret = i2c_smbus_read_byte_data(data->client, KXCJK1013_REG_INT_CTRL1);
> + ret = i2c_smbus_read_byte_data(data->client, KXREG_INT_CTRL1);
> if (ret < 0) {
> dev_err(&data->client->dev, "Error reading reg_int_ctrl1\n");
> return ret;
> }
>
> if (status)
> - ret |= KXCJK1013_REG_INT_REG1_BIT_IEN;
> + ret |= KXREG_INT_CTRL1_BIT_IEN;
> else
> - ret &= ~KXCJK1013_REG_INT_REG1_BIT_IEN;
> + ret &= ~KXREG_INT_CTRL1_BIT_IEN;
>
> - ret = i2c_smbus_write_byte_data(data->client, KXCJK1013_REG_INT_CTRL1,
> + ret = i2c_smbus_write_byte_data(data->client, KXREG_INT_CTRL1,
> ret);
> if (ret < 0) {
> dev_err(&data->client->dev, "Error writing reg_int_ctrl1\n");
> return ret;
> }
>
> - ret = i2c_smbus_read_byte_data(data->client, KXCJK1013_REG_CTRL1);
> + ret = i2c_smbus_read_byte_data(data->client, KXREG_CTRL1);
> if (ret < 0) {
> dev_err(&data->client->dev, "Error reading reg_ctrl1\n");
> return ret;
> }
>
> if (status)
> - ret |= KXCJK1013_REG_CTRL1_BIT_WUFE;
> + ret |= KXREG_CTRL1_BIT_WUFE;
> else
> - ret &= ~KXCJK1013_REG_CTRL1_BIT_WUFE;
> + ret &= ~KXREG_CTRL1_BIT_WUFE;
>
> ret = i2c_smbus_write_byte_data(data->client,
> - KXCJK1013_REG_CTRL1, ret);
> + KXREG_CTRL1, ret);
> if (ret < 0) {
> dev_err(&data->client->dev, "Error writing reg_ctrl1\n");
> return ret;
> @@ -497,37 +506,35 @@ static int kxcjk1013_setup_new_data_interrupt(struct kxcjk1013_data *data,
> if (ret < 0)
> return ret;
>
> - ret = i2c_smbus_read_byte_data(data->client, KXCJK1013_REG_INT_CTRL1);
> + ret = i2c_smbus_read_byte_data(data->client, KXREG_INT_CTRL1);
> if (ret < 0) {
> dev_err(&data->client->dev, "Error reading reg_int_ctrl1\n");
> return ret;
> }
>
> if (status)
> - ret |= KXCJK1013_REG_INT_REG1_BIT_IEN;
> + ret |= KXREG_INT_CTRL1_BIT_IEN;
> else
> - ret &= ~KXCJK1013_REG_INT_REG1_BIT_IEN;
> + ret &= ~KXREG_INT_CTRL1_BIT_IEN;
>
> - ret = i2c_smbus_write_byte_data(data->client, KXCJK1013_REG_INT_CTRL1,
> - ret);
> + ret = i2c_smbus_write_byte_data(data->client, KXREG_INT_CTRL1, ret);
> if (ret < 0) {
> dev_err(&data->client->dev, "Error writing reg_int_ctrl1\n");
> return ret;
> }
>
> - ret = i2c_smbus_read_byte_data(data->client, KXCJK1013_REG_CTRL1);
> + ret = i2c_smbus_read_byte_data(data->client, KXREG_CTRL1);
> if (ret < 0) {
> dev_err(&data->client->dev, "Error reading reg_ctrl1\n");
> return ret;
> }
>
> if (status)
> - ret |= KXCJK1013_REG_CTRL1_BIT_DRDY;
> + ret |= KXREG_CTRL1_BIT_DRDY;
> else
> - ret &= ~KXCJK1013_REG_CTRL1_BIT_DRDY;
> + ret &= ~KXREG_CTRL1_BIT_DRDY;
>
> - ret = i2c_smbus_write_byte_data(data->client,
> - KXCJK1013_REG_CTRL1, ret);
> + ret = i2c_smbus_write_byte_data(data->client, KXREG_CTRL1, ret);
> if (ret < 0) {
> dev_err(&data->client->dev, "Error writing reg_ctrl1\n");
> return ret;
> @@ -603,10 +610,10 @@ static int kxcjk1013_set_odr(struct kxcjk1013_data *data, int val, int val2)
>
> data->odr_bits = odr_setting->odr_bits;
>
> - ret = i2c_smbus_write_byte_data(data->client, KXCJK1013_REG_CTRL2,
> + ret = i2c_smbus_write_byte_data(data->client, KXREG_CTRL3,
> odr_setting->wuf_bits);
> if (ret < 0) {
> - dev_err(&data->client->dev, "Error writing reg_ctrl2\n");
> + dev_err(&data->client->dev, "Error writing reg_ctrl3\n");
> return ret;
> }
>
> @@ -1097,13 +1104,13 @@ static irqreturn_t kxcjk1013_event_handler(int irq, void *private)
> struct kxcjk1013_data *data = iio_priv(indio_dev);
> int ret;
>
> - ret = i2c_smbus_read_byte_data(data->client, KXCJK1013_REG_INT_SRC1);
> + ret = i2c_smbus_read_byte_data(data->client, KXREG_INT_SRC_DATA);
> if (ret < 0) {
> dev_err(&data->client->dev, "Error reading reg_int_src1\n");
> goto ack_intr;
> }
>
> - if (ret & 0x02) {
> + if (ret & KXREG_INT_BIT_WUFS) {
> kxcjk1013_report_motion_event(indio_dev);
> }
>
next prev parent reply other threads:[~2017-08-20 10:03 UTC|newest]
Thread overview: 15+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-08-17 14:21 [PATCH 0/6] iio: accel: kxcjk1003: support Kionix KXTF9 Michał Mirosław
2017-08-17 14:21 ` [PATCH 1/6] iio: accel: kxcjk1003: refactor ODR setting Michał Mirosław
2017-08-20 10:02 ` Jonathan Cameron
2017-08-17 14:21 ` [PATCH 2/6] iio: accel: kxcjk1013: extract report_motion_event() from interrupt handler Michał Mirosław
2017-08-20 10:01 ` Jonathan Cameron
2017-08-17 14:21 ` [PATCH 3/6] iio: accel: kxcjk1013: rename registers for KXTF9 compatibility Michał Mirosław
2017-08-20 9:52 ` Jonathan Cameron [this message]
2017-08-21 21:45 ` Michał Mirosław
2017-08-22 12:47 ` Jonathan Cameron
2017-08-17 14:21 ` [PATCH 4/6] iio: accel: kxcjk1013: make sysfs/sampling_frequency_avail dynamic Michał Mirosław
2017-08-20 9:53 ` Jonathan Cameron
2017-08-17 14:21 ` [PATCH 6/6] iio: accel: kxcjk1013: add support for KXTF9 Michał Mirosław
2017-08-20 10:00 ` Jonathan Cameron
2017-08-17 14:21 ` [PATCH 5/6] iio: accel: kxcjk1013: make sampling_frequency_avail per-type Michał Mirosław
2017-08-20 9:57 ` Jonathan Cameron
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=20170820105222.36dcaa38@archlinux \
--to=jic23@kernel.org \
--cc=knaack.h@gmx.de \
--cc=lars@metafoo.de \
--cc=linux-iio@vger.kernel.org \
--cc=mirq-linux@rere.qmqm.pl \
--cc=pmeerw@pmeerw.net \
/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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).