| Message ID | 20180129113323.18961-3-andi.shyti@samsung.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 >
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 -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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)
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(-)