| Message ID | 20240505214754.891700-4-andreas@kemnade.info |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Input: Add ektf2232 support | expand |
On Mon, May 6, 2024 at 12:48 AM Andreas Kemnade <andreas@kemnade.info> wrote: > > The chip is similar, but has status bits at different positions, > so use the correct bits. ... > @@ -46,6 +47,11 @@ struct ektf2127_ts { > struct input_dev *input; > struct gpio_desc *power_gpios; > struct touchscreen_properties prop; > + int status_shift; > +}; > + > +struct ektf2127_i2c_chip_data { > + int status_shift; > }; > > static void ektf2127_parse_coordinates(const u8 *buf, unsigned int touch_count, I'm wondering if you are using --histogram diff algo when preparing the patches. ... > + chip_data = device_get_match_data(&client->dev); > + if (!chip_data) > + chip_data = (const struct ektf2127_i2c_chip_data *)id->driver_data; > + if (!chip_data) { > + dev_err(&client->dev, "missing chip data\n"); > + return -EINVAL; First of all, there is a special combined API for this, please use it instead. Second, use dev_err_probe(). > + } ... > #ifdef CONFIG_OF Side note: Ideally we should kill this ugliness. But it can be done later on. -- With Best Regards, Andy Shevchenko
On Mon, 6 May 2024 15:05:52 +0300 Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > From: Andy Shevchenko <andy.shevchenko@gmail.com> > To: Andreas Kemnade <andreas@kemnade.info> > Cc: dmitry.torokhov@gmail.com, robh@kernel.org, krzk+dt@kernel.org, conor+dt@kernel.org, u.kleine-koenig@pengutronix.de, hdegoede@redhat.com, siebren.vroegindeweij@hotmail.com, linux-input@vger.kernel.org, devicetree@vger.kernel.org, linux-kernel@vger.kernel.org > Subject: Re: [PATCH v2 3/3] Input: ektf2127 - add ektf2232 support > Date: Mon, 6 May 2024 15:05:52 +0300 > > On Mon, May 6, 2024 at 12:48 AM Andreas Kemnade <andreas@kemnade.info> wrote: > > > > The chip is similar, but has status bits at different positions, > > so use the correct bits. > > ... > > > @@ -46,6 +47,11 @@ struct ektf2127_ts { > > struct input_dev *input; > > struct gpio_desc *power_gpios; > > struct touchscreen_properties prop; > > + int status_shift; > > +}; > > + > > +struct ektf2127_i2c_chip_data { > > + int status_shift; > > }; > > > > static void ektf2127_parse_coordinates(const u8 *buf, unsigned int touch_count, > > I'm wondering if you are using --histogram diff algo when preparing the patches. No, I am not using that, it seems to not make that chunk nicer. Yes, we want + int status_shift; }; + +struct ektf2127_i2c_chip_data { + int status_shift; +}; But that is not shorter or simpler, just more readable. Regards, Andreas
On Mon, May 6, 2024 at 7:21 PM Andreas Kemnade <andreas@kemnade.info> wrote: > On Mon, 6 May 2024 15:05:52 +0300 > Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > > From: Andy Shevchenko <andy.shevchenko@gmail.com> > > To: Andreas Kemnade <andreas@kemnade.info> > > Date: Mon, 6 May 2024 15:05:52 +0300 > > On Mon, May 6, 2024 at 12:48 AM Andreas Kemnade <andreas@kemnade.info> wrote: ... > > I'm wondering if you are using --histogram diff algo when preparing the patches. > > No, I am not using that, it seems to not make that chunk nicer. > Yes, we want > > + int status_shift; > }; > + > +struct ektf2127_i2c_chip_data { > + int status_shift; > +}; > > But that is not shorter or simpler, just more readable. And that's exactly what histogram is about. I suggest making it default for the Linux kernel project (or globally in your ~/.gitconfig).
On Mon, 6 May 2024 21:43:14 +0300 Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > On Mon, May 6, 2024 at 7:21 PM Andreas Kemnade <andreas@kemnade.info> wrote: > > On Mon, 6 May 2024 15:05:52 +0300 > > Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > > > From: Andy Shevchenko <andy.shevchenko@gmail.com> > > > To: Andreas Kemnade <andreas@kemnade.info> > > > Date: Mon, 6 May 2024 15:05:52 +0300 > > > On Mon, May 6, 2024 at 12:48 AM Andreas Kemnade <andreas@kemnade.info> wrote: > > ... > > > > I'm wondering if you are using --histogram diff algo when preparing the patches. > > > > No, I am not using that, it seems to not make that chunk nicer. > > Yes, we want > > > > + int status_shift; > > }; > > + > > +struct ektf2127_i2c_chip_data { > > + int status_shift; > > +}; > > > > But that is not shorter or simpler, just more readable. > > And that's exactly what histogram is about. I suggest making it > default for the Linux kernel project (or globally in your > ~/.gitconfig). > > again, it does not do anything helpful in this case, I tried to run git format-patch --histogram with no improvements. But it might help in other places. Regards, Andreas
diff --git a/drivers/input/touchscreen/ektf2127.c b/drivers/input/touchscreen/ektf2127.c index cc3103b9cbfba..028b605d1bb74 100644 --- a/drivers/input/touchscreen/ektf2127.c +++ b/drivers/input/touchscreen/ektf2127.c @@ -13,6 +13,7 @@ * Hans de Goede <hdegoede@redhat.com> */ +#include <linux/bits.h> #include <linux/gpio/consumer.h> #include <linux/interrupt.h> #include <linux/i2c.h> @@ -46,6 +47,11 @@ struct ektf2127_ts { struct input_dev *input; struct gpio_desc *power_gpios; struct touchscreen_properties prop; + int status_shift; +}; + +struct ektf2127_i2c_chip_data { + int status_shift; }; static void ektf2127_parse_coordinates(const u8 *buf, unsigned int touch_count, @@ -112,8 +118,8 @@ static void ektf2127_report2_contact(struct ektf2127_ts *ts, int slot, static void ektf2127_report2_event(struct ektf2127_ts *ts, const u8 *buf) { - ektf2127_report2_contact(ts, 0, &buf[1], !!(buf[7] & 2)); - ektf2127_report2_contact(ts, 1, &buf[4], !!(buf[7] & 4)); + ektf2127_report2_contact(ts, 0, &buf[1], !!(buf[7] & BIT(ts->status_shift))); + ektf2127_report2_contact(ts, 1, &buf[4], !!(buf[7] & BIT(ts->status_shift + 1))); input_mt_sync_frame(ts->input); input_sync(ts->input); @@ -246,7 +252,9 @@ static int ektf2127_query_dimension(struct i2c_client *client, bool width) static int ektf2127_probe(struct i2c_client *client) { + const struct i2c_device_id *id = i2c_client_get_device_id(client); struct device *dev = &client->dev; + const struct ektf2127_i2c_chip_data *chip_data; struct ektf2127_ts *ts; struct input_dev *input; u8 buf[4]; @@ -303,6 +311,17 @@ static int ektf2127_probe(struct i2c_client *client) return error; ts->input = input; + + chip_data = device_get_match_data(&client->dev); + if (!chip_data) + chip_data = (const struct ektf2127_i2c_chip_data *)id->driver_data; + if (!chip_data) { + dev_err(&client->dev, "missing chip data\n"); + return -EINVAL; + } + + ts->status_shift = chip_data->status_shift; + input_set_drvdata(input, ts); error = devm_request_threaded_irq(dev, client->irq, @@ -325,18 +344,28 @@ static int ektf2127_probe(struct i2c_client *client) return 0; } +static const struct ektf2127_i2c_chip_data ektf2127_data = { + .status_shift = 1, +}; + +static const struct ektf2127_i2c_chip_data ektf2232_data = { + .status_shift = 0, +}; + #ifdef CONFIG_OF static const struct of_device_id ektf2127_of_match[] = { - { .compatible = "elan,ektf2127" }, - { .compatible = "elan,ektf2132" }, + { .compatible = "elan,ektf2127", .data = &ektf2127_data}, + { .compatible = "elan,ektf2132", .data = &ektf2127_data}, + { .compatible = "elan,ektf2232", .data = &ektf2232_data}, {} }; MODULE_DEVICE_TABLE(of, ektf2127_of_match); #endif static const struct i2c_device_id ektf2127_i2c_id[] = { - { "ektf2127", 0 }, - { "ektf2132", 0 }, + { .name = "ektf2127", .driver_data = (long)&ektf2127_data }, + { .name = "ektf2132", .driver_data = (long)&ektf2127_data }, + { .name = "ektf2232", .driver_data = (long)&ektf2232_data }, {} }; MODULE_DEVICE_TABLE(i2c, ektf2127_i2c_id);
The chip is similar, but has status bits at different positions, so use the correct bits. Signed-off-by: Andreas Kemnade <andreas@kemnade.info> --- drivers/input/touchscreen/ektf2127.c | 41 ++++++++++++++++++++++++---- 1 file changed, 35 insertions(+), 6 deletions(-)