[PATCH 0/8] Melfas MMS114 touchscreen cleanups

[PATCH 0/8] Melfas MMS114 touchscreen cleanups

[Andi Shyti]

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

[PATCH 1/8] Input: mms114 - use smbus functions whenever possible

[Andi Shyti]

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 <andi.shyti@samsung.com>
---
 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

[PATCH 2/8] Input: mms114 - get read of custm i2c read/write functions

[Andi Shyti]

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 <andi.shyti@samsung.com>
---
 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

Re: [PATCH 2/8] Input: mms114 - get read of custm i2c read/write functions

[Dmitry Torokhov]

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 <andi.shyti@samsung.com>
> ---
>  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

Re: [PATCH 2/8] Input: mms114 - get read of custm i2c read/write functions

[Andi Shyti]

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 <andi.shyti@samsung.com>
> > ---
> >  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

[PATCH 3/8] Input: mms114 - replace mdelay with msleep

[Andi Shyti]

200ms seconds is a very long time to keep the CPU busy looping.
Use msleep instead.

Signed-off-by: Andi Shyti <andi.shyti@samsung.com>
---
 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

Re: [PATCH 3/8] Input: mms114 - replace mdelay with msleep

[Dmitry Torokhov]

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 <andi.shyti@samsung.com>

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

[PATCH 4/8] Input: mms114 - remove unused variable

[Andi Shyti]

'__packed' is not used anywhere, remove it.

Signed-off-by: Andi Shyti <andi.shyti@samsung.com>
---
 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

Re: [PATCH 4/8] Input: mms114 - remove unused variable

[Dmitry Torokhov]

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 <andi.shyti@samsung.com>
> ---
>  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

Re: [PATCH 4/8] Input: mms114 - remove unused variable

[Andi Shyti]

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

[PATCH 5/8] Input: mms114 - use lower case for hexadecimal values

[Andi Shyti]

Signed-off-by: Andi Shyti <andi.shyti@samsung.com>
---
 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

Re: [PATCH 5/8] Input: mms114 - use lower case for hexadecimal values

[Dmitry Torokhov]

On Mon, Jan 29, 2018 at 08:33:20PM +0900, Andi Shyti wrote:
> Signed-off-by: Andi Shyti <andi.shyti@samsung.com>

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

Re: [PATCH 5/8] Input: mms114 - use lower case for hexadecimal values

[Andi Shyti]

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 <andi.shyti@samsung.com>
> 
> 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
> 

Re: [PATCH 5/8] Input: mms114 - use lower case for hexadecimal values

[Dmitry Torokhov]

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 <andi.shyti@samsung.com>
> > 
> > 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

Re: [PATCH 5/8] Input: mms114 - use lower case for hexadecimal values

[Andi Shyti]

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 <andi.shyti@samsung.com>
> > > 
> > > 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

[PATCH 6/8] Input: mms114 - Use BIT() macro instead of explicit shifting

[Andi Shyti]

Signed-off-by: Andi Shyti <andi.shyti@samsung.com>
---
 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

Re: [PATCH 6/8] Input: mms114 - Use BIT() macro instead of explicit shifting

[Dmitry Torokhov]

On Mon, Jan 29, 2018 at 08:33:21PM +0900, Andi Shyti wrote:
> Signed-off-by: Andi Shyti <andi.shyti@samsung.com>

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

[PATCH 7/8] Input: mms114 - add SPDX identifier

[Andi Shyti]

Replace the original license statement with the SPDX identifier.

Signed-off-by: Andi Shyti <andi.shyti@samsung.com>
---
 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 <jy0922.shim@samsung.com>
- *
- * 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 <jy0922.shim@samsung.com>
 
 #include <linux/module.h>
 #include <linux/delay.h>
-- 
2.15.1

Re: [PATCH 7/8] Input: mms114 - add SPDX identifier

[Dmitry Torokhov]

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 <andi.shyti@samsung.com>

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 <jy0922.shim@samsung.com>
> - *
> - * 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 <jy0922.shim@samsung.com>
>  
>  #include <linux/module.h>
>  #include <linux/delay.h>
> -- 
> 2.15.1
> 

-- 
Dmitry

Re: [PATCH 7/8] Input: mms114 - add SPDX identifier

[Andi Shyti]

Hi Dmitry,

> > -/*
> > - * Copyright (C) 2012 Samsung Electronics Co.Ltd
> > - * Author: Joonyoung Shim <jy0922.shim@samsung.com>
> > - *
> > - * 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

Re: [PATCH 7/8] Input: mms114 - add SPDX identifier

[Marcus Folkesson]

[-- Attachment #1: Type: text/plain, Size: 1070 bytes --]

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 <jy0922.shim@samsung.com>
> > > - *
> > > - * 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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 7/8] Input: mms114 - add SPDX identifier

[Dmitry Torokhov]

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 <jy0922.shim@samsung.com>
> > > > - *
> > > > - * 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

Re: [PATCH 7/8] Input: mms114 - add SPDX identifier

[Andi Shyti]

Hi Dmitry,

> > > > > - * Copyright (C) 2012 Samsung Electronics Co.Ltd
> > > > > - * Author: Joonyoung Shim <jy0922.shim@samsung.com>
> > > > > - *
> > > > > - * 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

Re: [PATCH 7/8] Input: mms114 - add SPDX identifier

[Andi Shyti]

Hi Marcus,

> > > > - * Copyright (C) 2012 Samsung Electronics Co.Ltd
> > > > - * Author: Joonyoung Shim <jy0922.shim@samsung.com>
> > > > - *
> > > > - * 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

[PATCH 8/8] Input: mms114 - fix typo in definition

[Andi Shyti]

It's 'MMS114_INFORMATION', not 'MMS114_INFOMATION'

Signed-off-by: Andi Shyti <andi.shyti@samsung.com>
---
 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

Re: [PATCH 8/8] Input: mms114 - fix typo in definition

[Dmitry Torokhov]

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 <andi.shyti@samsung.com>

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