Hi Dmitry,
this patchset contains some cleanups for the mms114 driver. The
first two patches are the most important because they get rid of
the custom i2c read/write functions and use the smbus instead.
The others come from a "cleanup rush" I felt into.
I tested the patchest on the mms114 touchscreen in trats2, I
would appreaciate if Simon can test it on mms152.
Thanks,
Andi
Andi Shyti (8):
Input: mms114 - use smbus functions whenever possible
Input: mms114 - get read of custm i2c read/write functions
Input: mms114 - replace mdelay with msleep
Input: mms114 - remove unused variable
Input: mms114 - use lower case for hexadecimal values
Input: mms114 - Use BIT() macro instead of explicit shifting
Input: mms114 - add SPDX identifier
Input: mms114 - fix typo in definition
drivers/input/touchscreen/mms114.c | 150 +++++++++++--------------------------
1 file changed, 44 insertions(+), 106 deletions(-)
--
2.15.1
200ms seconds is a very long time to keep the CPU busy looping.
Use msleep instead.
Signed-off-by: Andi Shyti <[email protected]>
---
drivers/input/touchscreen/mms114.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/input/touchscreen/mms114.c b/drivers/input/touchscreen/mms114.c
index 94a97049d711..fb4435ae506b 100644
--- a/drivers/input/touchscreen/mms114.c
+++ b/drivers/input/touchscreen/mms114.c
@@ -290,7 +290,7 @@ static int mms114_start(struct mms114_data *data)
return error;
}
- mdelay(MMS114_POWERON_DELAY);
+ msleep(MMS114_POWERON_DELAY);
error = mms114_setup_regs(data);
if (error < 0) {
--
2.15.1
The exchange of data to and from the mms114 touchscreen never
exceeds 256 bytes. In the worst case it goes up to 80 bytes in
the interrupt handler while reading the events.
Thus it's not needed to make use of custom read/write functions
for accessing the i2c. Replace, whenever possible, the use of
custom functions with the more standard smbus ones.
It's not possible only in one case, in the mms114_set_active()
function where the 'cache_mode_control' variable is updated
according to the value in the register 'MMS114_MODE_CONTROL'
register.
Signed-off-by: Andi Shyti <[email protected]>
---
drivers/input/touchscreen/mms114.c | 32 ++++++++++++++++++++------------
1 file changed, 20 insertions(+), 12 deletions(-)
diff --git a/drivers/input/touchscreen/mms114.c b/drivers/input/touchscreen/mms114.c
index c54f4afe1103..0b8b1f0e8ba6 100644
--- a/drivers/input/touchscreen/mms114.c
+++ b/drivers/input/touchscreen/mms114.c
@@ -207,14 +207,15 @@ static irqreturn_t mms114_interrupt(int irq, void *dev_id)
}
mutex_unlock(&input_dev->mutex);
- packet_size = mms114_read_reg(data, MMS114_PACKET_SIZE);
+ packet_size = i2c_smbus_read_byte_data(data->client,
+ MMS114_PACKET_SIZE);
if (packet_size <= 0)
goto out;
touch_size = packet_size / MMS114_PACKET_NUM;
- error = __mms114_read_reg(data, MMS114_INFOMATION, packet_size,
- (u8 *)touch);
+ error = i2c_smbus_read_i2c_block_data(data->client, MMS114_INFOMATION,
+ packet_size, (u8 *)touch);
if (error < 0)
goto out;
@@ -254,7 +255,8 @@ static int mms114_get_version(struct mms114_data *data)
switch (data->type) {
case TYPE_MMS152:
- error = __mms114_read_reg(data, MMS152_FW_REV, 3, buf);
+ error = i2c_smbus_read_i2c_block_data(data->client,
+ MMS152_FW_REV, 3, buf);
if (error)
return error;
@@ -268,7 +270,8 @@ static int mms114_get_version(struct mms114_data *data)
break;
case TYPE_MMS114:
- error = __mms114_read_reg(data, MMS114_TSP_REV, 6, buf);
+ error = i2c_smbus_read_i2c_block_data(data->client,
+ MMS114_TSP_REV, 6, buf);
if (error)
return error;
@@ -300,30 +303,35 @@ static int mms114_setup_regs(struct mms114_data *data)
val = (props->max_x >> 8) & 0xf;
val |= ((props->max_y >> 8) & 0xf) << 4;
- error = mms114_write_reg(data, MMS114_XY_RESOLUTION_H, val);
+ error = i2c_smbus_write_byte_data(data->client,
+ MMS114_XY_RESOLUTION_H, val);
if (error < 0)
return error;
val = props->max_x & 0xff;
- error = mms114_write_reg(data, MMS114_X_RESOLUTION, val);
+ error = i2c_smbus_write_byte_data(data->client,
+ MMS114_X_RESOLUTION, val);
if (error < 0)
return error;
val = props->max_x & 0xff;
- error = mms114_write_reg(data, MMS114_Y_RESOLUTION, val);
+ error = i2c_smbus_write_byte_data(data->client,
+ MMS114_Y_RESOLUTION, val);
if (error < 0)
return error;
if (data->contact_threshold) {
- error = mms114_write_reg(data, MMS114_CONTACT_THRESHOLD,
- data->contact_threshold);
+ error = i2c_smbus_write_byte_data(data->client,
+ MMS114_CONTACT_THRESHOLD,
+ data->contact_threshold);
if (error < 0)
return error;
}
if (data->moving_threshold) {
- error = mms114_write_reg(data, MMS114_MOVING_THRESHOLD,
- data->moving_threshold);
+ error = i2c_smbus_write_byte_data(data->client,
+ MMS114_MOVING_THRESHOLD,
+ data->moving_threshold);
if (error < 0)
return error;
}
--
2.15.1
Signed-off-by: Andi Shyti <[email protected]>
---
drivers/input/touchscreen/mms114.c | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/drivers/input/touchscreen/mms114.c b/drivers/input/touchscreen/mms114.c
index 11dba8bb48e3..fdf23bc416af 100644
--- a/drivers/input/touchscreen/mms114.c
+++ b/drivers/input/touchscreen/mms114.c
@@ -20,7 +20,7 @@
/* Write only registers */
#define MMS114_MODE_CONTROL 0x01
-#define MMS114_OPERATION_MODE_MASK 0xE
+#define MMS114_OPERATION_MODE_MASK 0xe
#define MMS114_ACTIVE (1 << 1)
#define MMS114_XY_RESOLUTION_H 0x02
@@ -30,12 +30,12 @@
#define MMS114_MOVING_THRESHOLD 0x06
/* Read only registers */
-#define MMS114_PACKET_SIZE 0x0F
+#define MMS114_PACKET_SIZE 0x0f
#define MMS114_INFOMATION 0x10
-#define MMS114_TSP_REV 0xF0
+#define MMS114_TSP_REV 0xf0
-#define MMS152_FW_REV 0xE1
-#define MMS152_COMPAT_GROUP 0xF2
+#define MMS152_FW_REV 0xe1
+#define MMS152_COMPAT_GROUP 0xf2
/* 200ms needs after power on */
#define MMS114_POWERON_DELAY 200
--
2.15.1
'__packed' is not used anywhere, remove it.
Signed-off-by: Andi Shyti <[email protected]>
---
drivers/input/touchscreen/mms114.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/input/touchscreen/mms114.c b/drivers/input/touchscreen/mms114.c
index fb4435ae506b..11dba8bb48e3 100644
--- a/drivers/input/touchscreen/mms114.c
+++ b/drivers/input/touchscreen/mms114.c
@@ -78,7 +78,7 @@ struct mms114_touch {
u8 width;
u8 strength;
u8 reserved[2];
-} __packed;
+};
static void mms114_process_mt(struct mms114_data *data, struct mms114_touch *touch)
{
--
2.15.1
The 'mms114_read_reg' and 'mms114_write_reg' are used when
reading or writing to the 'MMS114_MODE_CONTROL' register for
updating the 'cache_mode_control' variable.
Update the 'cache_mode_control' variable in the calling
mms114_set_active() function and get rid of all the custom i2c
read/write functions.
With this remove also the redundant sleep of MMS114_I2C_DELAY
(50us) between i2c operations. The waiting should to be handled
by the i2c driver.
Signed-off-by: Andi Shyti <[email protected]>
---
drivers/input/touchscreen/mms114.c | 87 +++++---------------------------------
1 file changed, 10 insertions(+), 77 deletions(-)
diff --git a/drivers/input/touchscreen/mms114.c b/drivers/input/touchscreen/mms114.c
index 0b8b1f0e8ba6..94a97049d711 100644
--- a/drivers/input/touchscreen/mms114.c
+++ b/drivers/input/touchscreen/mms114.c
@@ -37,9 +37,6 @@
#define MMS152_FW_REV 0xE1
#define MMS152_COMPAT_GROUP 0xF2
-/* Minimum delay time is 50us between stop and start signal of i2c */
-#define MMS114_I2C_DELAY 50
-
/* 200ms needs after power on */
#define MMS114_POWERON_DELAY 200
@@ -83,76 +80,6 @@ struct mms114_touch {
u8 reserved[2];
} __packed;
-static int __mms114_read_reg(struct mms114_data *data, unsigned int reg,
- unsigned int len, u8 *val)
-{
- struct i2c_client *client = data->client;
- struct i2c_msg xfer[2];
- u8 buf = reg & 0xff;
- int error;
-
- if (reg <= MMS114_MODE_CONTROL && reg + len > MMS114_MODE_CONTROL)
- BUG();
-
- /* Write register: use repeated start */
- xfer[0].addr = client->addr;
- xfer[0].flags = I2C_M_TEN | I2C_M_NOSTART;
- xfer[0].len = 1;
- xfer[0].buf = &buf;
-
- /* Read data */
- xfer[1].addr = client->addr;
- xfer[1].flags = I2C_M_RD;
- xfer[1].len = len;
- xfer[1].buf = val;
-
- error = i2c_transfer(client->adapter, xfer, 2);
- if (error != 2) {
- dev_err(&client->dev,
- "%s: i2c transfer failed (%d)\n", __func__, error);
- return error < 0 ? error : -EIO;
- }
- udelay(MMS114_I2C_DELAY);
-
- return 0;
-}
-
-static int mms114_read_reg(struct mms114_data *data, unsigned int reg)
-{
- u8 val;
- int error;
-
- if (reg == MMS114_MODE_CONTROL)
- return data->cache_mode_control;
-
- error = __mms114_read_reg(data, reg, 1, &val);
- return error < 0 ? error : val;
-}
-
-static int mms114_write_reg(struct mms114_data *data, unsigned int reg,
- unsigned int val)
-{
- struct i2c_client *client = data->client;
- u8 buf[2];
- int error;
-
- buf[0] = reg & 0xff;
- buf[1] = val & 0xff;
-
- error = i2c_master_send(client, buf, 2);
- if (error != 2) {
- dev_err(&client->dev,
- "%s: i2c send failed (%d)\n", __func__, error);
- return error < 0 ? error : -EIO;
- }
- udelay(MMS114_I2C_DELAY);
-
- if (reg == MMS114_MODE_CONTROL)
- data->cache_mode_control = val;
-
- return 0;
-}
-
static void mms114_process_mt(struct mms114_data *data, struct mms114_touch *touch)
{
struct i2c_client *client = data->client;
@@ -231,19 +158,25 @@ static irqreturn_t mms114_interrupt(int irq, void *dev_id)
static int mms114_set_active(struct mms114_data *data, bool active)
{
- int val;
+ int val, err;
- val = mms114_read_reg(data, MMS114_MODE_CONTROL);
+ val = i2c_smbus_read_byte_data(data->client, MMS114_MODE_CONTROL);
if (val < 0)
return val;
- val &= ~MMS114_OPERATION_MODE_MASK;
+ val = data->cache_mode_control &= ~MMS114_OPERATION_MODE_MASK;
/* If active is false, sleep mode */
if (active)
val |= MMS114_ACTIVE;
- return mms114_write_reg(data, MMS114_MODE_CONTROL, val);
+ err = i2c_smbus_write_byte_data(data->client, MMS114_MODE_CONTROL, val);
+ if (err < 0)
+ return err;
+
+ data->cache_mode_control = val;
+
+ return 0;
}
static int mms114_get_version(struct mms114_data *data)
--
2.15.1
Signed-off-by: Andi Shyti <[email protected]>
---
drivers/input/touchscreen/mms114.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/input/touchscreen/mms114.c b/drivers/input/touchscreen/mms114.c
index fdf23bc416af..d70c03adf148 100644
--- a/drivers/input/touchscreen/mms114.c
+++ b/drivers/input/touchscreen/mms114.c
@@ -21,7 +21,7 @@
/* Write only registers */
#define MMS114_MODE_CONTROL 0x01
#define MMS114_OPERATION_MODE_MASK 0xe
-#define MMS114_ACTIVE (1 << 1)
+#define MMS114_ACTIVE BIT(1)
#define MMS114_XY_RESOLUTION_H 0x02
#define MMS114_X_RESOLUTION 0x03
--
2.15.1
Replace the original license statement with the SPDX identifier.
Signed-off-by: Andi Shyti <[email protected]>
---
drivers/input/touchscreen/mms114.c | 13 +++++--------
1 file changed, 5 insertions(+), 8 deletions(-)
diff --git a/drivers/input/touchscreen/mms114.c b/drivers/input/touchscreen/mms114.c
index d70c03adf148..3230c92de1ed 100644
--- a/drivers/input/touchscreen/mms114.c
+++ b/drivers/input/touchscreen/mms114.c
@@ -1,11 +1,8 @@
-/*
- * Copyright (C) 2012 Samsung Electronics Co.Ltd
- * Author: Joonyoung Shim <[email protected]>
- *
- * This program is free software; you can redistribute it and/or modify
- * it under the terms of the GNU General Public License version 2 as
- * published by the Free Software Foundation.
- */
+// SPDX-License-Identifier: GPL-2.0
+// Samsung S6SY761 Touchscreen device driver
+//
+// Copyright (c) 2012 Samsung Electronics Co., Ltd.
+// Author: Joonyoung Shim <[email protected]>
#include <linux/module.h>
#include <linux/delay.h>
--
2.15.1
It's 'MMS114_INFORMATION', not 'MMS114_INFOMATION'
Signed-off-by: Andi Shyti <[email protected]>
---
drivers/input/touchscreen/mms114.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/input/touchscreen/mms114.c b/drivers/input/touchscreen/mms114.c
index 3230c92de1ed..5b609531b3aa 100644
--- a/drivers/input/touchscreen/mms114.c
+++ b/drivers/input/touchscreen/mms114.c
@@ -28,7 +28,7 @@
/* Read only registers */
#define MMS114_PACKET_SIZE 0x0f
-#define MMS114_INFOMATION 0x10
+#define MMS114_INFORMATION 0x10
#define MMS114_TSP_REV 0xf0
#define MMS152_FW_REV 0xe1
@@ -138,7 +138,7 @@ static irqreturn_t mms114_interrupt(int irq, void *dev_id)
touch_size = packet_size / MMS114_PACKET_NUM;
- error = i2c_smbus_read_i2c_block_data(data->client, MMS114_INFOMATION,
+ error = i2c_smbus_read_i2c_block_data(data->client, MMS114_INFORMATION,
packet_size, (u8 *)touch);
if (error < 0)
goto out;
--
2.15.1
On Mon, Jan 29, 2018 at 08:33:19PM +0900, Andi Shyti wrote:
> '__packed' is not used anywhere, remove it.
Umm, this is not a variable, this is type annotation meaning that the
structure is packed. Still not needed, as we are not using anything but
u8 data elements, but justification is completely wrong.
>
> Signed-off-by: Andi Shyti <[email protected]>
> ---
> drivers/input/touchscreen/mms114.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/input/touchscreen/mms114.c b/drivers/input/touchscreen/mms114.c
> index fb4435ae506b..11dba8bb48e3 100644
> --- a/drivers/input/touchscreen/mms114.c
> +++ b/drivers/input/touchscreen/mms114.c
> @@ -78,7 +78,7 @@ struct mms114_touch {
> u8 width;
> u8 strength;
> u8 reserved[2];
> -} __packed;
> +};
>
> static void mms114_process_mt(struct mms114_data *data, struct mms114_touch *touch)
> {
> --
> 2.15.1
>
--
Dmitry
On Mon, Jan 29, 2018 at 08:33:20PM +0900, Andi Shyti wrote:
> Signed-off-by: Andi Shyti <[email protected]>
But why?
> ---
> drivers/input/touchscreen/mms114.c | 10 +++++-----
> 1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/input/touchscreen/mms114.c b/drivers/input/touchscreen/mms114.c
> index 11dba8bb48e3..fdf23bc416af 100644
> --- a/drivers/input/touchscreen/mms114.c
> +++ b/drivers/input/touchscreen/mms114.c
> @@ -20,7 +20,7 @@
>
> /* Write only registers */
> #define MMS114_MODE_CONTROL 0x01
> -#define MMS114_OPERATION_MODE_MASK 0xE
> +#define MMS114_OPERATION_MODE_MASK 0xe
> #define MMS114_ACTIVE (1 << 1)
>
> #define MMS114_XY_RESOLUTION_H 0x02
> @@ -30,12 +30,12 @@
> #define MMS114_MOVING_THRESHOLD 0x06
>
> /* Read only registers */
> -#define MMS114_PACKET_SIZE 0x0F
> +#define MMS114_PACKET_SIZE 0x0f
> #define MMS114_INFOMATION 0x10
> -#define MMS114_TSP_REV 0xF0
> +#define MMS114_TSP_REV 0xf0
>
> -#define MMS152_FW_REV 0xE1
> -#define MMS152_COMPAT_GROUP 0xF2
> +#define MMS152_FW_REV 0xe1
> +#define MMS152_COMPAT_GROUP 0xf2
>
> /* 200ms needs after power on */
> #define MMS114_POWERON_DELAY 200
> --
> 2.15.1
>
--
Dmitry
On Mon, Jan 29, 2018 at 08:33:17PM +0900, Andi Shyti wrote:
> The 'mms114_read_reg' and 'mms114_write_reg' are used when
> reading or writing to the 'MMS114_MODE_CONTROL' register for
> updating the 'cache_mode_control' variable.
>
> Update the 'cache_mode_control' variable in the calling
> mms114_set_active() function and get rid of all the custom i2c
> read/write functions.
>
> With this remove also the redundant sleep of MMS114_I2C_DELAY
> (50us) between i2c operations. The waiting should to be handled
> by the i2c driver.
>
> Signed-off-by: Andi Shyti <[email protected]>
> ---
> drivers/input/touchscreen/mms114.c | 87 +++++---------------------------------
> 1 file changed, 10 insertions(+), 77 deletions(-)
>
> diff --git a/drivers/input/touchscreen/mms114.c b/drivers/input/touchscreen/mms114.c
> index 0b8b1f0e8ba6..94a97049d711 100644
> --- a/drivers/input/touchscreen/mms114.c
> +++ b/drivers/input/touchscreen/mms114.c
> @@ -37,9 +37,6 @@
> #define MMS152_FW_REV 0xE1
> #define MMS152_COMPAT_GROUP 0xF2
>
> -/* Minimum delay time is 50us between stop and start signal of i2c */
> -#define MMS114_I2C_DELAY 50
> -
> /* 200ms needs after power on */
> #define MMS114_POWERON_DELAY 200
>
> @@ -83,76 +80,6 @@ struct mms114_touch {
> u8 reserved[2];
> } __packed;
>
> -static int __mms114_read_reg(struct mms114_data *data, unsigned int reg,
> - unsigned int len, u8 *val)
> -{
> - struct i2c_client *client = data->client;
> - struct i2c_msg xfer[2];
> - u8 buf = reg & 0xff;
> - int error;
> -
> - if (reg <= MMS114_MODE_CONTROL && reg + len > MMS114_MODE_CONTROL)
> - BUG();
> -
> - /* Write register: use repeated start */
> - xfer[0].addr = client->addr;
> - xfer[0].flags = I2C_M_TEN | I2C_M_NOSTART;
So the chip does not use 10-bit addressing? What about I2C_M_NOSTART? It
is not needed also?
> - xfer[0].len = 1;
> - xfer[0].buf = &buf;
> -
> - /* Read data */
> - xfer[1].addr = client->addr;
> - xfer[1].flags = I2C_M_RD;
> - xfer[1].len = len;
> - xfer[1].buf = val;
> -
> - error = i2c_transfer(client->adapter, xfer, 2);
> - if (error != 2) {
> - dev_err(&client->dev,
> - "%s: i2c transfer failed (%d)\n", __func__, error);
> - return error < 0 ? error : -EIO;
> - }
> - udelay(MMS114_I2C_DELAY);
> -
> - return 0;
> -}
> -
> -static int mms114_read_reg(struct mms114_data *data, unsigned int reg)
> -{
> - u8 val;
> - int error;
> -
> - if (reg == MMS114_MODE_CONTROL)
> - return data->cache_mode_control;
> -
> - error = __mms114_read_reg(data, reg, 1, &val);
> - return error < 0 ? error : val;
> -}
> -
> -static int mms114_write_reg(struct mms114_data *data, unsigned int reg,
> - unsigned int val)
> -{
> - struct i2c_client *client = data->client;
> - u8 buf[2];
> - int error;
> -
> - buf[0] = reg & 0xff;
> - buf[1] = val & 0xff;
> -
> - error = i2c_master_send(client, buf, 2);
> - if (error != 2) {
> - dev_err(&client->dev,
> - "%s: i2c send failed (%d)\n", __func__, error);
> - return error < 0 ? error : -EIO;
> - }
> - udelay(MMS114_I2C_DELAY);
> -
> - if (reg == MMS114_MODE_CONTROL)
> - data->cache_mode_control = val;
> -
> - return 0;
> -}
> -
> static void mms114_process_mt(struct mms114_data *data, struct mms114_touch *touch)
> {
> struct i2c_client *client = data->client;
> @@ -231,19 +158,25 @@ static irqreturn_t mms114_interrupt(int irq, void *dev_id)
>
> static int mms114_set_active(struct mms114_data *data, bool active)
> {
> - int val;
> + int val, err;
>
> - val = mms114_read_reg(data, MMS114_MODE_CONTROL);
> + val = i2c_smbus_read_byte_data(data->client, MMS114_MODE_CONTROL);
If I understand the original commit for the driver, the control
register is write only and is not to be read from, that is why we have
this cached value. With your change you read form it.
By the way, have you looked into converting it all to regmap?
> if (val < 0)
> return val;
>
> - val &= ~MMS114_OPERATION_MODE_MASK;
> + val = data->cache_mode_control &= ~MMS114_OPERATION_MODE_MASK;
>
> /* If active is false, sleep mode */
> if (active)
> val |= MMS114_ACTIVE;
>
> - return mms114_write_reg(data, MMS114_MODE_CONTROL, val);
> + err = i2c_smbus_write_byte_data(data->client, MMS114_MODE_CONTROL, val);
> + if (err < 0)
> + return err;
> +
> + data->cache_mode_control = val;
> +
> + return 0;
> }
>
> static int mms114_get_version(struct mms114_data *data)
> --
> 2.15.1
>
--
Dmitry
On Mon, Jan 29, 2018 at 08:33:18PM +0900, Andi Shyti wrote:
> 200ms seconds is a very long time to keep the CPU busy looping.
> Use msleep instead.
>
> Signed-off-by: Andi Shyti <[email protected]>
Applied, thank you.
> ---
> drivers/input/touchscreen/mms114.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/input/touchscreen/mms114.c b/drivers/input/touchscreen/mms114.c
> index 94a97049d711..fb4435ae506b 100644
> --- a/drivers/input/touchscreen/mms114.c
> +++ b/drivers/input/touchscreen/mms114.c
> @@ -290,7 +290,7 @@ static int mms114_start(struct mms114_data *data)
> return error;
> }
>
> - mdelay(MMS114_POWERON_DELAY);
> + msleep(MMS114_POWERON_DELAY);
>
> error = mms114_setup_regs(data);
> if (error < 0) {
> --
> 2.15.1
>
--
Dmitry
On Mon, Jan 29, 2018 at 08:33:21PM +0900, Andi Shyti wrote:
> Signed-off-by: Andi Shyti <[email protected]>
Applied, thank you.
> ---
> drivers/input/touchscreen/mms114.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/input/touchscreen/mms114.c b/drivers/input/touchscreen/mms114.c
> index fdf23bc416af..d70c03adf148 100644
> --- a/drivers/input/touchscreen/mms114.c
> +++ b/drivers/input/touchscreen/mms114.c
> @@ -21,7 +21,7 @@
> /* Write only registers */
> #define MMS114_MODE_CONTROL 0x01
> #define MMS114_OPERATION_MODE_MASK 0xe
> -#define MMS114_ACTIVE (1 << 1)
> +#define MMS114_ACTIVE BIT(1)
>
> #define MMS114_XY_RESOLUTION_H 0x02
> #define MMS114_X_RESOLUTION 0x03
> --
> 2.15.1
>
--
Dmitry
On Mon, Jan 29, 2018 at 08:33:22PM +0900, Andi Shyti wrote:
> Replace the original license statement with the SPDX identifier.
>
> Signed-off-by: Andi Shyti <[email protected]>
Applied, thank you.
> ---
> drivers/input/touchscreen/mms114.c | 13 +++++--------
> 1 file changed, 5 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/input/touchscreen/mms114.c b/drivers/input/touchscreen/mms114.c
> index d70c03adf148..3230c92de1ed 100644
> --- a/drivers/input/touchscreen/mms114.c
> +++ b/drivers/input/touchscreen/mms114.c
> @@ -1,11 +1,8 @@
> -/*
> - * Copyright (C) 2012 Samsung Electronics Co.Ltd
> - * Author: Joonyoung Shim <[email protected]>
> - *
> - * This program is free software; you can redistribute it and/or modify
> - * it under the terms of the GNU General Public License version 2 as
> - * published by the Free Software Foundation.
> - */
> +// SPDX-License-Identifier: GPL-2.0
> +// Samsung S6SY761 Touchscreen device driver
> +//
> +// Copyright (c) 2012 Samsung Electronics Co., Ltd.
> +// Author: Joonyoung Shim <[email protected]>
>
> #include <linux/module.h>
> #include <linux/delay.h>
> --
> 2.15.1
>
--
Dmitry
On Mon, Jan 29, 2018 at 08:33:23PM +0900, Andi Shyti wrote:
> It's 'MMS114_INFORMATION', not 'MMS114_INFOMATION'
>
> Signed-off-by: Andi Shyti <[email protected]>
Applied, thank you.
> ---
> drivers/input/touchscreen/mms114.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/input/touchscreen/mms114.c b/drivers/input/touchscreen/mms114.c
> index 3230c92de1ed..5b609531b3aa 100644
> --- a/drivers/input/touchscreen/mms114.c
> +++ b/drivers/input/touchscreen/mms114.c
> @@ -28,7 +28,7 @@
>
> /* Read only registers */
> #define MMS114_PACKET_SIZE 0x0f
> -#define MMS114_INFOMATION 0x10
> +#define MMS114_INFORMATION 0x10
> #define MMS114_TSP_REV 0xf0
>
> #define MMS152_FW_REV 0xe1
> @@ -138,7 +138,7 @@ static irqreturn_t mms114_interrupt(int irq, void *dev_id)
>
> touch_size = packet_size / MMS114_PACKET_NUM;
>
> - error = i2c_smbus_read_i2c_block_data(data->client, MMS114_INFOMATION,
> + error = i2c_smbus_read_i2c_block_data(data->client, MMS114_INFORMATION,
> packet_size, (u8 *)touch);
> if (error < 0)
> goto out;
> --
> 2.15.1
>
--
Dmitry
Hi Dmitry,
On Mon, Jan 29, 2018 at 11:01:41AM -0800, Dmitry Torokhov wrote:
> On Mon, Jan 29, 2018 at 08:33:17PM +0900, Andi Shyti wrote:
> > The 'mms114_read_reg' and 'mms114_write_reg' are used when
> > reading or writing to the 'MMS114_MODE_CONTROL' register for
> > updating the 'cache_mode_control' variable.
> >
> > Update the 'cache_mode_control' variable in the calling
> > mms114_set_active() function and get rid of all the custom i2c
> > read/write functions.
> >
> > With this remove also the redundant sleep of MMS114_I2C_DELAY
> > (50us) between i2c operations. The waiting should to be handled
> > by the i2c driver.
> >
> > Signed-off-by: Andi Shyti <[email protected]>
> > ---
> > drivers/input/touchscreen/mms114.c | 87 +++++---------------------------------
> > 1 file changed, 10 insertions(+), 77 deletions(-)
> >
> > diff --git a/drivers/input/touchscreen/mms114.c b/drivers/input/touchscreen/mms114.c
> > index 0b8b1f0e8ba6..94a97049d711 100644
> > --- a/drivers/input/touchscreen/mms114.c
> > +++ b/drivers/input/touchscreen/mms114.c
> > @@ -37,9 +37,6 @@
> > #define MMS152_FW_REV 0xE1
> > #define MMS152_COMPAT_GROUP 0xF2
> >
> > -/* Minimum delay time is 50us between stop and start signal of i2c */
> > -#define MMS114_I2C_DELAY 50
> > -
> > /* 200ms needs after power on */
> > #define MMS114_POWERON_DELAY 200
> >
> > @@ -83,76 +80,6 @@ struct mms114_touch {
> > u8 reserved[2];
> > } __packed;
> >
> > -static int __mms114_read_reg(struct mms114_data *data, unsigned int reg,
> > - unsigned int len, u8 *val)
> > -{
> > - struct i2c_client *client = data->client;
> > - struct i2c_msg xfer[2];
> > - u8 buf = reg & 0xff;
> > - int error;
> > -
> > - if (reg <= MMS114_MODE_CONTROL && reg + len > MMS114_MODE_CONTROL)
> > - BUG();
> > -
> > - /* Write register: use repeated start */
> > - xfer[0].addr = client->addr;
> > - xfer[0].flags = I2C_M_TEN | I2C_M_NOSTART;
>
> So the chip does not use 10-bit addressing? What about I2C_M_NOSTART? It
> is not needed also?
From the datasheet I have no, indeed I don't understand why this
is coming. That's why I asked Simon to test it on his device as
well.
On my device this patch works just fine.
> > - xfer[0].len = 1;
> > - xfer[0].buf = &buf;
> > -
> > - /* Read data */
> > - xfer[1].addr = client->addr;
> > - xfer[1].flags = I2C_M_RD;
> > - xfer[1].len = len;
> > - xfer[1].buf = val;
> > -
> > - error = i2c_transfer(client->adapter, xfer, 2);
> > - if (error != 2) {
> > - dev_err(&client->dev,
> > - "%s: i2c transfer failed (%d)\n", __func__, error);
> > - return error < 0 ? error : -EIO;
> > - }
> > - udelay(MMS114_I2C_DELAY);
> > -
> > - return 0;
> > -}
> > -
> > -static int mms114_read_reg(struct mms114_data *data, unsigned int reg)
> > -{
> > - u8 val;
> > - int error;
> > -
> > - if (reg == MMS114_MODE_CONTROL)
> > - return data->cache_mode_control;
> > -
> > - error = __mms114_read_reg(data, reg, 1, &val);
> > - return error < 0 ? error : val;
> > -}
> > -
> > -static int mms114_write_reg(struct mms114_data *data, unsigned int reg,
> > - unsigned int val)
> > -{
> > - struct i2c_client *client = data->client;
> > - u8 buf[2];
> > - int error;
> > -
> > - buf[0] = reg & 0xff;
> > - buf[1] = val & 0xff;
> > -
> > - error = i2c_master_send(client, buf, 2);
> > - if (error != 2) {
> > - dev_err(&client->dev,
> > - "%s: i2c send failed (%d)\n", __func__, error);
> > - return error < 0 ? error : -EIO;
> > - }
> > - udelay(MMS114_I2C_DELAY);
> > -
> > - if (reg == MMS114_MODE_CONTROL)
> > - data->cache_mode_control = val;
> > -
> > - return 0;
> > -}
> > -
> > static void mms114_process_mt(struct mms114_data *data, struct mms114_touch *touch)
> > {
> > struct i2c_client *client = data->client;
> > @@ -231,19 +158,25 @@ static irqreturn_t mms114_interrupt(int irq, void *dev_id)
> >
> > static int mms114_set_active(struct mms114_data *data, bool active)
> > {
> > - int val;
> > + int val, err;
> >
> > - val = mms114_read_reg(data, MMS114_MODE_CONTROL);
> > + val = i2c_smbus_read_byte_data(data->client, MMS114_MODE_CONTROL);
>
> If I understand the original commit for the driver, the control
> register is write only and is not to be read from, that is why we have
> this cached value. With your change you read form it.
Still in my datasheet thee MMS114_MODE_CONTROL is a R/W register.
Still I don't understand the original choice, but it doesn't
change much anyway, I can keep the original idea of never reading
from it. Or I can rework this function in a better way.
> By the way, have you looked into converting it all to regmap?
I could.
Andi
Hi Dmitry,
On Mon, Jan 29, 2018 at 10:43:09AM -0800, Dmitry Torokhov wrote:
> On Mon, Jan 29, 2018 at 08:33:19PM +0900, Andi Shyti wrote:
> > '__packed' is not used anywhere, remove it.
>
> Umm, this is not a variable, this is type annotation meaning that the
> structure is packed. Still not needed, as we are not using anything but
> u8 data elements, but justification is completely wrong.
Oh dammit! :)
of course! The original idea was the alignment of the structure,
but I must have got confused while re-editing all the commits.
Sorry, I'll fix it.
Andi
Hi Dmitry,
On Mon, Jan 29, 2018 at 10:56:31AM -0800, Dmitry Torokhov wrote:
> On Mon, Jan 29, 2018 at 08:33:20PM +0900, Andi Shyti wrote:
> > Signed-off-by: Andi Shyti <[email protected]>
>
> But why?
I think this patch is trivial and the subject itself is quite
self-explanatory and the commit message would be a copy-paste of
it.
OK, if you want I will add "some comment".
Andi
> > ---
> > drivers/input/touchscreen/mms114.c | 10 +++++-----
> > 1 file changed, 5 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/input/touchscreen/mms114.c b/drivers/input/touchscreen/mms114.c
> > index 11dba8bb48e3..fdf23bc416af 100644
> > --- a/drivers/input/touchscreen/mms114.c
> > +++ b/drivers/input/touchscreen/mms114.c
> > @@ -20,7 +20,7 @@
> >
> > /* Write only registers */
> > #define MMS114_MODE_CONTROL 0x01
> > -#define MMS114_OPERATION_MODE_MASK 0xE
> > +#define MMS114_OPERATION_MODE_MASK 0xe
> > #define MMS114_ACTIVE (1 << 1)
> >
> > #define MMS114_XY_RESOLUTION_H 0x02
> > @@ -30,12 +30,12 @@
> > #define MMS114_MOVING_THRESHOLD 0x06
> >
> > /* Read only registers */
> > -#define MMS114_PACKET_SIZE 0x0F
> > +#define MMS114_PACKET_SIZE 0x0f
> > #define MMS114_INFOMATION 0x10
> > -#define MMS114_TSP_REV 0xF0
> > +#define MMS114_TSP_REV 0xf0
> >
> > -#define MMS152_FW_REV 0xE1
> > -#define MMS152_COMPAT_GROUP 0xF2
> > +#define MMS152_FW_REV 0xe1
> > +#define MMS152_COMPAT_GROUP 0xf2
> >
> > /* 200ms needs after power on */
> > #define MMS114_POWERON_DELAY 200
> > --
> > 2.15.1
> >
>
> --
> Dmitry
>
On Tue, Jan 30, 2018 at 08:29:23AM +0900, Andi Shyti wrote:
> Hi Dmitry,
>
> On Mon, Jan 29, 2018 at 10:56:31AM -0800, Dmitry Torokhov wrote:
> > On Mon, Jan 29, 2018 at 08:33:20PM +0900, Andi Shyti wrote:
> > > Signed-off-by: Andi Shyti <[email protected]>
> >
> > But why?
>
> I think this patch is trivial and the subject itself is quite
> self-explanatory and the commit message would be a copy-paste of
> it.
Here is where we disagree. Why lowercase is preferred over uppercase? Do
we have this in coding style (and I completely forgot about it)? Or is
it your preference (mine is too to be fair, but I also do not care
about churning the code for it)?
Thanks.
--
Dmitry
Hi Dmitry,
On Mon, Jan 29, 2018 at 03:33:35PM -0800, Dmitry Torokhov wrote:
> On Tue, Jan 30, 2018 at 08:29:23AM +0900, Andi Shyti wrote:
> > Hi Dmitry,
> >
> > On Mon, Jan 29, 2018 at 10:56:31AM -0800, Dmitry Torokhov wrote:
> > > On Mon, Jan 29, 2018 at 08:33:20PM +0900, Andi Shyti wrote:
> > > > Signed-off-by: Andi Shyti <[email protected]>
> > >
> > > But why?
> >
> > I think this patch is trivial and the subject itself is quite
> > self-explanatory and the commit message would be a copy-paste of
> > it.
>
> Here is where we disagree. Why lowercase is preferred over uppercase? Do
> we have this in coding style (and I completely forgot about it)? Or is
> it your preference (mine is too to be fair, but I also do not care
> about churning the code for it)?
I think this is the case where the cultural habits become law, I've
been writing lower case hex values from years and my eyes are so
used to it that it looks wrong to have them written upper case.
Many maintainers reject upper case hex values.
As well as many reject the " ? : " conditional form.
In any case, you're basically right here, at this point I
don't mind if you drop it :)
Thanks,
Andi
Hi Dmitry,
> > -/*
> > - * Copyright (C) 2012 Samsung Electronics Co.Ltd
> > - * Author: Joonyoung Shim <[email protected]>
> > - *
> > - * This program is free software; you can redistribute it and/or modify
> > - * it under the terms of the GNU General Public License version 2 as
> > - * published by the Free Software Foundation.
> > - */
> > +// SPDX-License-Identifier: GPL-2.0
> > +// Samsung S6SY761 Touchscreen device driver
I'm very sorry, but my distrcation will kill me one day.
Is it possible to revert this patch or do you want me to send a
fix?
Sorry,
Andi
Hi Andy,
On Wed, Jan 31, 2018 at 03:07:26PM +0900, Andi Shyti wrote:
> Hi Dmitry,
>
> > > -/*
> > > - * Copyright (C) 2012 Samsung Electronics Co.Ltd
> > > - * Author: Joonyoung Shim <[email protected]>
> > > - *
> > > - * This program is free software; you can redistribute it and/or modify
> > > - * it under the terms of the GNU General Public License version 2 as
> > > - * published by the Free Software Foundation.
> > > - */
> > > +// SPDX-License-Identifier: GPL-2.0
> > > +// Samsung S6SY761 Touchscreen device driver
>
> I'm very sorry, but my distrcation will kill me one day.
More coffee or sleep - your choice ;-)
>
> Is it possible to revert this patch or do you want me to send a
> fix?
When you are on it;
Change
MODULE_LICENSE("GPL");
to
MODULE_LICENSE("GPL v2");
To match the former license text and SPDX-identifier.
See include/linux/module.h:
* "GPL" [GNU Public License v2 or later]
* "GPL v2" [GNU Public License v2]
Thanks,
>
> Sorry,
> Andi
Best regards
Marcus Folkesson
On Wed, Jan 31, 2018 at 08:31:38AM +0100, Marcus Folkesson wrote:
> Hi Andy,
>
> On Wed, Jan 31, 2018 at 03:07:26PM +0900, Andi Shyti wrote:
> > Hi Dmitry,
> >
> > > > -/*
> > > > - * Copyright (C) 2012 Samsung Electronics Co.Ltd
> > > > - * Author: Joonyoung Shim <[email protected]>
> > > > - *
> > > > - * This program is free software; you can redistribute it and/or modify
> > > > - * it under the terms of the GNU General Public License version 2 as
> > > > - * published by the Free Software Foundation.
> > > > - */
> > > > +// SPDX-License-Identifier: GPL-2.0
> > > > +// Samsung S6SY761 Touchscreen device driver
> >
> > I'm very sorry, but my distrcation will kill me one day.
>
> More coffee or sleep - your choice ;-)
>
> >
> > Is it possible to revert this patch or do you want me to send a
> > fix?
>
> When you are on it;
>
> Change
> MODULE_LICENSE("GPL");
>
> to
> MODULE_LICENSE("GPL v2");
> To match the former license text and SPDX-identifier.
>
> See include/linux/module.h:
> * "GPL" [GNU Public License v2 or later]
> * "GPL v2" [GNU Public License v2]
OK, I dropped the patch, please resend the correct version.
Thanks.
--
Dmitry
Hi Marcus,
> > > > - * Copyright (C) 2012 Samsung Electronics Co.Ltd
> > > > - * Author: Joonyoung Shim <[email protected]>
> > > > - *
> > > > - * This program is free software; you can redistribute it and/or modify
> > > > - * it under the terms of the GNU General Public License version 2 as
> > > > - * published by the Free Software Foundation.
> > > > - */
> > > > +// SPDX-License-Identifier: GPL-2.0
> > > > +// Samsung S6SY761 Touchscreen device driver
> >
> > I'm very sorry, but my distrcation will kill me one day.
>
> More coffee or sleep - your choice ;-)
I see that beer is not working, indeed :)
> > Is it possible to revert this patch or do you want me to send a
> > fix?
>
> When you are on it;
>
> Change
> MODULE_LICENSE("GPL");
>
> to
> MODULE_LICENSE("GPL v2");
> To match the former license text and SPDX-identifier.
>
> See include/linux/module.h:
> * "GPL" [GNU Public License v2 or later]
> * "GPL v2" [GNU Public License v2]
I haven't noticed that. I will fix it in a separate patch.
Thanks,
Andi
Hi Dmitry,
> > > > > - * Copyright (C) 2012 Samsung Electronics Co.Ltd
> > > > > - * Author: Joonyoung Shim <[email protected]>
> > > > > - *
> > > > > - * This program is free software; you can redistribute it and/or modify
> > > > > - * it under the terms of the GNU General Public License version 2 as
> > > > > - * published by the Free Software Foundation.
> > > > > - */
> > > > > +// SPDX-License-Identifier: GPL-2.0
> > > > > +// Samsung S6SY761 Touchscreen device driver
> > >
> > > I'm very sorry, but my distrcation will kill me one day.
> > >
> > > Is it possible to revert this patch or do you want me to send a
> > > fix?
> >
>
> OK, I dropped the patch, please resend the correct version.
Thanks, will do!
Andi