| Message ID | 20191007205021.104402-1-stephan@gerhold.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add support for Melfas MMS345L touchscreen | expand |
Hi Stephan, On Mon, Oct 07, 2019 at 10:50:21PM +0200, Stephan Gerhold wrote: > MMS345L is another first generation touch screen from Melfas, > which uses the same registers as MMS152. > > However, using I2C_M_NOSTART for it causes errors when reading: > > i2c i2c-0: sendbytes: NAK bailout. > mms114 0-0048: __mms114_read_reg: i2c transfer failed (-5) > > The driver works fine as soon as I2C_M_NOSTART is removed. > > Add a separate melfas,mms345l binding, and make use of I2C_M_NOSTART > only for MMS114 and MMS152. > > Signed-off-by: Stephan Gerhold <stephan@gerhold.net> Reviewed-by: Andi Shyti <andi@etezian.org> just a nitpick in case you will resend it (but you don't need to). > - if (!i2c_check_functionality(client->adapter, > - I2C_FUNC_PROTOCOL_MANGLING)) { > + type = (enum mms_type)device_get_match_data(&client->dev); you don't need any cast here. Thanks, Andi
> > - if (!i2c_check_functionality(client->adapter, > > - I2C_FUNC_PROTOCOL_MANGLING)) { > > + type = (enum mms_type)device_get_match_data(&client->dev); > > you don't need any cast here. sorry, please ignore :) Andi
On Mon, Oct 07, 2019 at 10:50:21PM +0200, Stephan Gerhold wrote: > MMS345L is another first generation touch screen from Melfas, > which uses the same registers as MMS152. > > However, using I2C_M_NOSTART for it causes errors when reading: > > i2c i2c-0: sendbytes: NAK bailout. > mms114 0-0048: __mms114_read_reg: i2c transfer failed (-5) > > The driver works fine as soon as I2C_M_NOSTART is removed. > > Add a separate melfas,mms345l binding, and make use of I2C_M_NOSTART > only for MMS114 and MMS152. > > Signed-off-by: Stephan Gerhold <stephan@gerhold.net> > --- > Note: I was not able to find a datasheet for any of the models, > so this change is merely based on testing and comparison with > the downstream driver [1]. > > There was a related patch [2] that removes I2C_M_NOSTART for all models, > but it seems abandoned and I do not have any other model for testing. > Therefore, this patch implements the least instrusive solution > and only removes I2C_M_NOSTART for MMS345L. Hmm, at this point I am inclined to pick up Andi's patch since it seems to work for you and him and it looks like Android drivers are not using I2C_M_NOSTART. I wonder if this was some quirk/big on the platform where it was originally developed. Any objections? Thanks.
Hi Dmitry, > > There was a related patch [2] that removes I2C_M_NOSTART for all models, > > but it seems abandoned and I do not have any other model for testing. > > Therefore, this patch implements the least instrusive solution > > and only removes I2C_M_NOSTART for MMS345L. > > Hmm, at this point I am inclined to pick up Andi's patch since it seems > to work for you and him and it looks like Android drivers are not using > I2C_M_NOSTART. I wonder if this was some quirk/big on the platform where > it was originally developed. I completely forgot about that patch :) I should refresh some old work on that device which was left undone. > Any objections? It's OK for me. If you can just update my e-mail, please, when applying the patch to "andi@etezian.org". Thanks! Thank you, Andi
On Tue, Oct 08, 2019 at 03:00:14PM -0700, Dmitry Torokhov wrote: > On Mon, Oct 07, 2019 at 10:50:21PM +0200, Stephan Gerhold wrote: > > MMS345L is another first generation touch screen from Melfas, > > which uses the same registers as MMS152. > > > > However, using I2C_M_NOSTART for it causes errors when reading: > > > > i2c i2c-0: sendbytes: NAK bailout. > > mms114 0-0048: __mms114_read_reg: i2c transfer failed (-5) > > > > The driver works fine as soon as I2C_M_NOSTART is removed. > > > > Add a separate melfas,mms345l binding, and make use of I2C_M_NOSTART > > only for MMS114 and MMS152. > > > > Signed-off-by: Stephan Gerhold <stephan@gerhold.net> > > --- > > Note: I was not able to find a datasheet for any of the models, > > so this change is merely based on testing and comparison with > > the downstream driver [1]. > > > > There was a related patch [2] that removes I2C_M_NOSTART for all models, > > but it seems abandoned and I do not have any other model for testing. > > Therefore, this patch implements the least instrusive solution > > and only removes I2C_M_NOSTART for MMS345L. > > Hmm, at this point I am inclined to pick up Andi's patch since it seems > to work for you and him and it looks like Android drivers are not using > I2C_M_NOSTART. I wonder if this was some quirk/big on the platform where > it was originally developed. > > Any objections? I cannot really speak for any of the other models, but no objections for removing I2C_M_NOSTART from my side. I'm actually rather confused by it since it is used on the first partial message. The documentation [1] says: If you set the I2C_M_NOSTART variable for the first partial message, we do not generate Addr, but we do generate the startbit S. ** This will probably confuse all other clients on your bus, so don't try this. ** Yet, someone felt like trying this here. ;) I have tested the following two patches from Andy on MMS345L: - Input: mms114 - use smbus functions whenever possible [2] - Input: mms114 - get read of custm i2c read/write functions [3] Indeed, with I2C_M_NOSTART removed I can actually use "melfas,mms152" and the touchscreen appears to work without further changes. (The only weird thing is that it displays an empty "Compat group" in "TSP FW Rev: bootloader 0x6 / core 0x26 / config 0x26, Compat group: ", I suspect that register does not exist on MMS345L...) But there is a limitation that won't let us take these two patches as-is: i2c_smbus_read_i2c_block_data() is limited to I2C_SMBUS_BLOCK_MAX, which is: 32 /* As specified in SMBus standard */ According to "Input: mms114 - use smbus functions whenever possible": > 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. Since i2c_smbus_read_i2c_block_data() is limited to 32, larger packets won't be read correctly. For example, if you use more than 4 fingers you can easily trigger a situation where one of the fingers gets "stuck", together with: mms114 4-0048: Wrong touch type (0) (It attempts to read uninitialized data in this case...) So we still need the custom functions for reading the touch packet, or maybe change the driver to use regmap instead. [1]: https://www.kernel.org/doc/html/latest/i2c/i2c-protocol.html [2]: https://patchwork.kernel.org/patch/10189551/ [3]: https://patchwork.kernel.org/patch/10189541/
On Wed, Oct 09, 2019 at 10:26:12AM +0300, Andi Shyti wrote: > Hi Dmitry, > > > > There was a related patch [2] that removes I2C_M_NOSTART for all models, > > > but it seems abandoned and I do not have any other model for testing. > > > Therefore, this patch implements the least instrusive solution > > > and only removes I2C_M_NOSTART for MMS345L. > > > > Hmm, at this point I am inclined to pick up Andi's patch since it seems > > to work for you and him and it looks like Android drivers are not using > > I2C_M_NOSTART. I wonder if this was some quirk/big on the platform where > > it was originally developed. > > I completely forgot about that patch :) > > I should refresh some old work on that device which was left > undone. > > > Any objections? > > It's OK for me. If you can just update my e-mail, please, when > applying the patch to "andi@etezian.org". Thanks! Andi, any chance you could resend it with the new email? One thing that I try to avoid modifying whenever I can is S-O-B strings... Thanks.
Hi Stephan, > > > There was a related patch [2] that removes I2C_M_NOSTART for all models, > > > but it seems abandoned and I do not have any other model for testing. > > > Therefore, this patch implements the least instrusive solution > > > and only removes I2C_M_NOSTART for MMS345L. > > > > Hmm, at this point I am inclined to pick up Andi's patch since it seems > > to work for you and him and it looks like Android drivers are not using > > I2C_M_NOSTART. I wonder if this was some quirk/big on the platform where > > it was originally developed. > > > > Any objections? > > I cannot really speak for any of the other models, but no objections for > removing I2C_M_NOSTART from my side. I'm actually rather confused by it > since it is used on the first partial message. > > The documentation [1] says: > If you set the I2C_M_NOSTART variable for the first partial message, > we do not generate Addr, but we do generate the startbit S. > ** This will probably confuse all other clients on your bus, > so don't try this. ** > > Yet, someone felt like trying this here. ;) still it should be specified in the i2c protocol of the device, if it's not, then most probably it's not needed, I guess. Andi
Hi Dmitry, > > > > There was a related patch [2] that removes I2C_M_NOSTART for all models, > > > > but it seems abandoned and I do not have any other model for testing. > > > > Therefore, this patch implements the least instrusive solution > > > > and only removes I2C_M_NOSTART for MMS345L. > > > > > > Hmm, at this point I am inclined to pick up Andi's patch since it seems > > > to work for you and him and it looks like Android drivers are not using > > > I2C_M_NOSTART. I wonder if this was some quirk/big on the platform where > > > it was originally developed. > > > > I completely forgot about that patch :) > > > > I should refresh some old work on that device which was left > > undone. > > > > > Any objections? > > > > It's OK for me. If you can just update my e-mail, please, when > > applying the patch to "andi@etezian.org". Thanks! > > Andi, any chance you could resend it with the new email? One thing that > I try to avoid modifying whenever I can is S-O-B strings... sure, I will resend the patches, but it might take some time (a few days I hope) because I don't have the devices with me right now for testing (and I added some small fixes, as well) Thanks, Andi
diff --git a/drivers/input/touchscreen/mms114.c b/drivers/input/touchscreen/mms114.c index 69c6d559eeb0..d9f45755d073 100644 --- a/drivers/input/touchscreen/mms114.c +++ b/drivers/input/touchscreen/mms114.c @@ -54,6 +54,7 @@ enum mms_type { TYPE_MMS114 = 114, TYPE_MMS152 = 152, + TYPE_MMS345L = 345, }; struct mms114_data { @@ -91,9 +92,14 @@ static int __mms114_read_reg(struct mms114_data *data, unsigned int reg, if (reg <= MMS114_MODE_CONTROL && reg + len > MMS114_MODE_CONTROL) BUG(); - /* Write register: use repeated start */ + /* Write register */ xfer[0].addr = client->addr; - xfer[0].flags = I2C_M_TEN | I2C_M_NOSTART; + if (data->type != TYPE_MMS345L) + /* use repeated start */ + xfer[0].flags = I2C_M_TEN | I2C_M_NOSTART; + else + xfer[0].flags = client->flags & I2C_M_TEN; + xfer[0].len = 1; xfer[0].buf = &buf; @@ -250,6 +256,15 @@ static int mms114_get_version(struct mms114_data *data) int error; switch (data->type) { + case TYPE_MMS345L: + error = __mms114_read_reg(data, MMS152_FW_REV, 3, buf); + if (error) + return error; + + dev_info(dev, "TSP FW Rev: bootloader 0x%x / core 0x%x / config 0x%x\n", + buf[0], buf[1], buf[2]); + break; + case TYPE_MMS152: error = __mms114_read_reg(data, MMS152_FW_REV, 3, buf); if (error) @@ -287,8 +302,8 @@ static int mms114_setup_regs(struct mms114_data *data) if (error < 0) return error; - /* MMS152 has no configuration or power on registers */ - if (data->type == TYPE_MMS152) + /* Only MMS114 has configuration and power on registers */ + if (data->type != TYPE_MMS114) return 0; error = mms114_set_active(data, true); @@ -425,11 +440,16 @@ static int mms114_probe(struct i2c_client *client, { struct mms114_data *data; struct input_dev *input_dev; - const void *match_data; + enum mms_type type; int error; - if (!i2c_check_functionality(client->adapter, - I2C_FUNC_PROTOCOL_MANGLING)) { + type = (enum mms_type)device_get_match_data(&client->dev); + if (!type) + return -EINVAL; + + if (type != TYPE_MMS345L && + !i2c_check_functionality(client->adapter, + I2C_FUNC_PROTOCOL_MANGLING)) { dev_err(&client->dev, "Need i2c bus that supports protocol mangling\n"); return -ENODEV; @@ -446,11 +466,7 @@ static int mms114_probe(struct i2c_client *client, data->client = client; data->input_dev = input_dev; - match_data = device_get_match_data(&client->dev); - if (!match_data) - return -EINVAL; - - data->type = (enum mms_type)match_data; + data->type = type; input_set_capability(input_dev, EV_ABS, ABS_MT_POSITION_X); input_set_capability(input_dev, EV_ABS, ABS_MT_POSITION_Y); @@ -599,6 +615,9 @@ static const struct of_device_id mms114_dt_match[] = { }, { .compatible = "melfas,mms152", .data = (void *)TYPE_MMS152, + }, { + .compatible = "melfas,mms345l", + .data = (void *)TYPE_MMS345L, }, { } };
MMS345L is another first generation touch screen from Melfas, which uses the same registers as MMS152. However, using I2C_M_NOSTART for it causes errors when reading: i2c i2c-0: sendbytes: NAK bailout. mms114 0-0048: __mms114_read_reg: i2c transfer failed (-5) The driver works fine as soon as I2C_M_NOSTART is removed. Add a separate melfas,mms345l binding, and make use of I2C_M_NOSTART only for MMS114 and MMS152. Signed-off-by: Stephan Gerhold <stephan@gerhold.net> --- Note: I was not able to find a datasheet for any of the models, so this change is merely based on testing and comparison with the downstream driver [1]. There was a related patch [2] that removes I2C_M_NOSTART for all models, but it seems abandoned and I do not have any other model for testing. Therefore, this patch implements the least instrusive solution and only removes I2C_M_NOSTART for MMS345L. [1]: https://github.com/msm8916-mainline/android_kernel_qcom_msm8916/blob/SM-A500FU/drivers/input/touchscreen/mms300_a.c [2]: https://patchwork.kernel.org/patch/10189541/ --- drivers/input/touchscreen/mms114.c | 43 +++++++++++++++++++++--------- 1 file changed, 31 insertions(+), 12 deletions(-)