| Message ID | 20181218083606.25795-1-richard.leitner@skidata.com |
|---|---|
| Headers | show |
| Series | Input: sx8654 - reset-gpio, sx865[056] support, etc. | expand |
Hi, another 3 weeks and still no response :-( The last comment on this patchset was in October by Dimitry/Rob. In the meantime I did a "ping" and a resend... So what's up? I'm perfectly fine with a "we don't want this is mainline because XXX" or "I have no time and will come back to it in XXX"... But please give at least some kind of response... Thanks! regards;Richard.L On 18/12/2018 09:35, Richard Leitner wrote: > Add reset-gpio, sx8654[056] and common of_touchscreen functions support > for the sx8654 driver. > > Changes RESEND v2: > - added "Reviewed-by: Rob Herring <robh@kernel.org>" tags > > Changes v2: > - use devm_gpiod_get_optional in probe instead of in #ifdef CONFIG_OF > - convert flags to BIT() in a separate patch > - replace hrtimer with "regular" timer > - use of_device_get_match_data instead of of_match_device > - add driver data to i2c_device_id table for non-DT fallback > - fix sequence of common touchscreen initialization > - div. minor stlye changes > > Richard Leitner (8): > dt-bindings: input: touchscreen: sx8654: add reset-gpio property > Input: sx8654 - add reset-gpio support > dt-bindings: input: touchscreen: sx8654: add compatible models > Input: sx8654 - add sx8655 and sx8656 to compatibles > dt-bindings: input: touchscreen: sx8654: add sx8650 to comatibles > Input: sx8654 - add sx8650 support > Input: sx8654 - use common of_touchscreen functions > Input: sx8654 - convert #defined flags to BIT(x) > > .../bindings/input/touchscreen/sx8654.txt | 10 +- > drivers/input/touchscreen/sx8654.c | 245 ++++++++++++++++++--- > 2 files changed, 229 insertions(+), 26 deletions(-) >
Hi Richard, On Tue, Dec 18, 2018 at 09:39:31AM +0100, Richard Leitner wrote: > The sx8654 and sx8650 are quite similar, therefore add support for the > sx8650 within the sx8654 driver. > > Signed-off-by: Richard Leitner <richard.leitner@skidata.com> > --- > drivers/input/touchscreen/sx8654.c | 193 +++++++++++++++++++++++++++++++++---- > 1 file changed, 173 insertions(+), 20 deletions(-) > > diff --git a/drivers/input/touchscreen/sx8654.c b/drivers/input/touchscreen/sx8654.c > index afa4da138fe9..4939863efbef 100644 > --- a/drivers/input/touchscreen/sx8654.c > +++ b/drivers/input/touchscreen/sx8654.c > @@ -29,7 +29,7 @@ > > #include <linux/input.h> > #include <linux/module.h> > -#include <linux/of.h> > +#include <linux/of_device.h> > #include <linux/i2c.h> > #include <linux/interrupt.h> > #include <linux/irq.h> > @@ -44,9 +44,11 @@ > #define I2C_REG_IRQSRC 0x23 > #define I2C_REG_SOFTRESET 0x3f > > +#define I2C_REG_SX8650_STAT 0x05 > +#define SX8650_STAT_CONVIRQ 0x80 > + > /* commands */ > #define CMD_READ_REGISTER 0x40 > -#define CMD_MANUAL 0xc0 > #define CMD_PENTRG 0xe0 > > /* value for I2C_REG_SOFTRESET */ > @@ -58,6 +60,7 @@ > > /* bits for RegTouch1 */ > #define CONDIRQ 0x20 > +#define RPDNT_100K 0x00 > #define FILT_7SA 0x03 > > /* bits for I2C_REG_CHANMASK */ > @@ -71,14 +74,122 @@ > /* power delay: lower nibble of CTRL0 register */ > #define POWDLY_1_1MS 0x0b > > +/* for sx8650, as we have no pen release IRQ there: timeout in ns following the > + * last PENIRQ after which we assume the pen is lifted. > + */ > +#define SX8650_PENIRQ_TIMEOUT msecs_to_jiffies(10) > + > #define MAX_12BIT ((1 << 12) - 1) > +#define MAX_I2C_READ_LEN 10 /* see datasheet section 5.1.5 */ > + > +/* channel definition */ > +#define CH_X 0x00 > +#define CH_Y 0x01 > + > +struct sx865x_data { > + u8 cmd_manual; > + u8 chan_mask; > + u8 has_irq_penrelease; > + u8 has_reg_irqmask; I changed these 2 to bools. > + irq_handler_t irqh; > +}; > > struct sx8654 { > struct input_dev *input; > struct i2c_client *client; > struct gpio_desc *gpio_reset; > + > + spinlock_t lock; /* for input reporting from irq/timer */ > + struct timer_list timer; > + > + const struct sx865x_data *data; > }; > > +static inline void sx865x_penrelease(struct sx8654 *ts) > +{ > + struct input_dev *input_dev = ts->input; > + > + input_report_key(input_dev, BTN_TOUCH, 0); > + input_sync(input_dev); > +} > + > +static void sx865x_penrelease_timer_handler(struct timer_list *t) > +{ > + struct sx8654 *ts = from_timer(ts, t, timer); > + unsigned long flags; > + > + spin_lock_irqsave(&ts->lock, flags); > + sx865x_penrelease(ts); > + spin_unlock_irqrestore(&ts->lock, flags); > + dev_dbg(&ts->client->dev, "penrelease by timer\n"); > +} > + > +static irqreturn_t sx8650_irq(int irq, void *handle) > +{ > + struct sx8654 *ts = handle; > + struct device *dev = &ts->client->dev; > + int len, i; > + unsigned long flags; > + u8 stat; > + u16 x, y; > + u8 data[MAX_I2C_READ_LEN]; > + u16 ch; > + u16 chdata; > + u8 readlen = hweight32(ts->data->chan_mask) * 2; > + > + stat = i2c_smbus_read_byte_data(ts->client, CMD_READ_REGISTER > + | I2C_REG_SX8650_STAT); > + > + if (!(stat & SX8650_STAT_CONVIRQ)) { > + dev_dbg(dev, "%s ignore stat [0x%02x]", __func__, stat); > + return IRQ_HANDLED; > + } > + > + len = i2c_master_recv(ts->client, data, readlen); > + if (len != readlen) { > + dev_dbg(dev, "ignore short recv (%d)\n", len); > + return IRQ_HANDLED; > + } > + > + spin_lock_irqsave(&ts->lock, flags); > + > + x = 0; > + y = 0; > + for (i = 0; (i + 1) < len; i++) { > + chdata = data[i] << 8; > + i++; > + chdata += data[i]; So you reading be16 form the wire. I annotated data array as such and used be16_to_cpu() here. > + > + if (unlikely(chdata == 0xFFFF)) { > + dev_dbg(dev, "invalid qualified data @ %d\n", i); > + continue; > + } else if (unlikely(chdata & 0x8000)) { > + dev_warn(dev, "hibit @ %d [0x%04x]\n", i, chdata); > + continue; > + } > + > + ch = chdata >> 12; > + if (ch == CH_X) > + x = chdata & MAX_12BIT; > + else if (ch == CH_Y) > + y = chdata & MAX_12BIT; > + else > + dev_warn(dev, "unknown channel %d [0x%04x]\n", ch, > + chdata); > + } > + > + input_report_abs(ts->input, ABS_X, x); > + input_report_abs(ts->input, ABS_Y, y); > + input_report_key(ts->input, BTN_TOUCH, 1); > + input_sync(ts->input); > + dev_dbg(dev, "point(%4d,%4d)\n", x, y); > + > + mod_timer(&ts->timer, jiffies + SX8650_PENIRQ_TIMEOUT); > + spin_unlock_irqrestore(&ts->lock, flags); > + > + return IRQ_HANDLED; > +} > + > static irqreturn_t sx8654_irq(int irq, void *handle) > { > struct sx8654 *sx8654 = handle; > @@ -127,6 +238,22 @@ static irqreturn_t sx8654_irq(int irq, void *handle) > return IRQ_HANDLED; > } > > +static const struct sx865x_data sx8650_data = { > + .cmd_manual = 0xb0, > + .has_irq_penrelease = 0, > + .has_reg_irqmask = 0, Changed both to false. > + .chan_mask = (CONV_X | CONV_Y), > + .irqh = sx8650_irq, > +}; > + > +static const struct sx865x_data sx8654_data = { > + .cmd_manual = 0xc0, > + .has_irq_penrelease = 1, > + .has_reg_irqmask = 1, Changed both to true. > + .chan_mask = (CONV_X | CONV_Y), > + .irqh = sx8654_irq, > +}; > + Moved the structures closer to where they are used. > static int sx8654_reset(struct sx8654 *ts) > { > int err; > @@ -182,13 +309,13 @@ static void sx8654_close(struct input_dev *dev) > disable_irq(client->irq); > > /* enable manual mode mode */ > - error = i2c_smbus_write_byte(client, CMD_MANUAL); > + error = i2c_smbus_write_byte(client, sx8654->data->cmd_manual); > if (error) { > dev_err(&client->dev, "writing command CMD_MANUAL failed"); > return; > } > > - error = i2c_smbus_write_byte_data(client, I2C_REG_TOUCH0, 0); > + error = i2c_smbus_write_byte_data(client, I2C_REG_TOUCH0, RATE_MANUAL); > if (error) { > dev_err(&client->dev, "writing to I2C_REG_TOUCH0 failed"); > return; > @@ -221,6 +348,20 @@ static int sx8654_probe(struct i2c_client *client, > } > dev_dbg(&client->dev, "got GPIO reset pin\n"); > > + sx8654->data = of_device_get_match_data(&client->dev); Changed to device_get_match_data() in case someone would use it on non-OF platform. > + if (!sx8654->data) > + sx8654->data = (const struct sx865x_data *)id->driver_data; > + if (!sx8654->data) { > + dev_err(&client->dev, "invalid or missing device data\n"); > + return -EINVAL; > + } > + > + if (!sx8654->data->has_irq_penrelease) { > + dev_dbg(&client->dev, "use timer for penrelease\n"); > + timer_setup(&sx8654->timer, sx865x_penrelease_timer_handler, 0); > + spin_lock_init(&sx8654->lock); You should also make sure the timer is not running when you are tearing down the device. I added del_timer_sync() call to sx8654_close(). > + } > + > input = devm_input_allocate_device(&client->dev); > if (!input) > return -ENOMEM; > @@ -248,29 +389,31 @@ static int sx8654_probe(struct i2c_client *client, > } > > error = i2c_smbus_write_byte_data(client, I2C_REG_CHANMASK, > - CONV_X | CONV_Y); > + sx8654->data->chan_mask); > if (error) { > dev_err(&client->dev, "writing to I2C_REG_CHANMASK failed"); > return error; > } > > - error = i2c_smbus_write_byte_data(client, I2C_REG_IRQMASK, > - IRQ_PENTOUCH_TOUCHCONVDONE | > - IRQ_PENRELEASE); > - if (error) { > - dev_err(&client->dev, "writing to I2C_REG_IRQMASK failed"); > - return error; > + if (sx8654->data->has_reg_irqmask) { > + error = i2c_smbus_write_byte_data(client, I2C_REG_IRQMASK, > + IRQ_PENTOUCH_TOUCHCONVDONE | > + IRQ_PENRELEASE); > + if (error) { > + dev_err(&client->dev, "writing I2C_REG_IRQMASK failed"); > + return error; > + } > } > > error = i2c_smbus_write_byte_data(client, I2C_REG_TOUCH1, > - CONDIRQ | FILT_7SA); > + CONDIRQ | RPDNT_100K | FILT_7SA); > if (error) { > dev_err(&client->dev, "writing to I2C_REG_TOUCH1 failed"); > return error; > } > > error = devm_request_threaded_irq(&client->dev, client->irq, > - NULL, sx8654_irq, > + NULL, sx8654->data->irqh, > IRQF_TRIGGER_FALLING | IRQF_ONESHOT, > client->name, sx8654); > if (error) { > @@ -292,18 +435,28 @@ static int sx8654_probe(struct i2c_client *client, > > #ifdef CONFIG_OF > static const struct of_device_id sx8654_of_match[] = { > - { .compatible = "semtech,sx8654", }, > - { .compatible = "semtech,sx8655", }, > - { .compatible = "semtech,sx8656", }, > - { }, > + { > + .compatible = "semtech,sx8650", > + .data = &sx8650_data, > + }, { > + .compatible = "semtech,sx8654", > + .data = &sx8654_data, > + }, { > + .compatible = "semtech,sx8655", > + .data = &sx8654_data, > + }, { > + .compatible = "semtech,sx8656", > + .data = &sx8654_data, > + }, {}, > }; > MODULE_DEVICE_TABLE(of, sx8654_of_match); > #endif > > static const struct i2c_device_id sx8654_id_table[] = { > - { "semtech_sx8654", 0 }, > - { "semtech_sx8655", 0 }, > - { "semtech_sx8656", 0 }, > + { .name = "semtech_sx8650", .driver_data = (long)&sx8650_data }, > + { .name = "semtech_sx8654", .driver_data = (long)&sx8654_data }, > + { .name = "semtech_sx8655", .driver_data = (long)&sx8654_data }, > + { .name = "semtech_sx8656", .driver_data = (long)&sx8654_data }, > { }, > }; > MODULE_DEVICE_TABLE(i2c, sx8654_id_table); > -- > 2.11.0 > Applied with above mentioned changes, please take a look in case I messed up. Thanks.
On Tue, Dec 18, 2018 at 09:40:02AM +0100, Richard Leitner wrote: > Some of the #defined register values are one-bit flags. Convert them to > use the BIT(x) macro instead of 1 byte hexadecimal values. This improves > readability and clarifies the intent. > > Signed-off-by: Richard Leitner <richard.leitner@skidata.com> Applied, thank you. > --- > drivers/input/touchscreen/sx8654.c | 11 ++++++----- > 1 file changed, 6 insertions(+), 5 deletions(-) > > diff --git a/drivers/input/touchscreen/sx8654.c b/drivers/input/touchscreen/sx8654.c > index b7b263ed52af..3746ea855f94 100644 > --- a/drivers/input/touchscreen/sx8654.c > +++ b/drivers/input/touchscreen/sx8654.c > @@ -36,6 +36,7 @@ > #include <linux/gpio/consumer.h> > #include <linux/delay.h> > #include <linux/input/touchscreen.h> > +#include <linux/bitops.h> > > /* register addresses */ > #define I2C_REG_TOUCH0 0x00 > @@ -46,7 +47,7 @@ > #define I2C_REG_SOFTRESET 0x3f > > #define I2C_REG_SX8650_STAT 0x05 > -#define SX8650_STAT_CONVIRQ 0x80 > +#define SX8650_STAT_CONVIRQ BIT(7) > > /* commands */ > #define CMD_READ_REGISTER 0x40 > @@ -56,8 +57,8 @@ > #define SOFTRESET_VALUE 0xde > > /* bits for I2C_REG_IRQSRC */ > -#define IRQ_PENTOUCH_TOUCHCONVDONE 0x08 > -#define IRQ_PENRELEASE 0x04 > +#define IRQ_PENTOUCH_TOUCHCONVDONE BIT(7) > +#define IRQ_PENRELEASE BIT(6) > > /* bits for RegTouch1 */ > #define CONDIRQ 0x20 > @@ -65,8 +66,8 @@ > #define FILT_7SA 0x03 > > /* bits for I2C_REG_CHANMASK */ > -#define CONV_X 0x80 > -#define CONV_Y 0x40 > +#define CONV_X BIT(7) > +#define CONV_Y BIT(6) > > /* coordinates rate: higher nibble of CTRL0 register */ > #define RATE_MANUAL 0x00 > -- > 2.11.0 >
On Tue, Dec 18, 2018 at 09:39:46AM +0100, Richard Leitner wrote: > of_touchscreen.c provides a common interface for a axis invertion and > swapping of touchscreens. Therefore use it in the sx8654 driver. > > Signed-off-by: Richard Leitner <richard.leitner@skidata.com> Applied, thank you. > --- > drivers/input/touchscreen/sx8654.c | 11 +++++++---- > 1 file changed, 7 insertions(+), 4 deletions(-) > > diff --git a/drivers/input/touchscreen/sx8654.c b/drivers/input/touchscreen/sx8654.c > index 4939863efbef..b7b263ed52af 100644 > --- a/drivers/input/touchscreen/sx8654.c > +++ b/drivers/input/touchscreen/sx8654.c > @@ -35,6 +35,7 @@ > #include <linux/irq.h> > #include <linux/gpio/consumer.h> > #include <linux/delay.h> > +#include <linux/input/touchscreen.h> > > /* register addresses */ > #define I2C_REG_TOUCH0 0x00 > @@ -101,6 +102,7 @@ struct sx8654 { > > spinlock_t lock; /* for input reporting from irq/timer */ > struct timer_list timer; > + struct touchscreen_properties props; > > const struct sx865x_data *data; > }; > @@ -178,8 +180,7 @@ static irqreturn_t sx8650_irq(int irq, void *handle) > chdata); > } > > - input_report_abs(ts->input, ABS_X, x); > - input_report_abs(ts->input, ABS_Y, y); > + touchscreen_report_pos(ts->input, &ts->props, x, y, false); > input_report_key(ts->input, BTN_TOUCH, 1); > input_sync(ts->input); > dev_dbg(dev, "point(%4d,%4d)\n", x, y); > @@ -226,8 +227,8 @@ static irqreturn_t sx8654_irq(int irq, void *handle) > x = ((data[0] & 0xf) << 8) | (data[1]); > y = ((data[2] & 0xf) << 8) | (data[3]); > > - input_report_abs(sx8654->input, ABS_X, x); > - input_report_abs(sx8654->input, ABS_Y, y); > + touchscreen_report_pos(sx8654->input, &sx8654->props, x, y, > + false); > input_report_key(sx8654->input, BTN_TOUCH, 1); > input_sync(sx8654->input); > > @@ -377,6 +378,8 @@ static int sx8654_probe(struct i2c_client *client, > input_set_abs_params(input, ABS_X, 0, MAX_12BIT, 0, 0); > input_set_abs_params(input, ABS_Y, 0, MAX_12BIT, 0, 0); > > + touchscreen_parse_properties(input, false, &sx8654->props); > + > sx8654->client = client; > sx8654->input = input; > > -- > 2.11.0 >
On Tue, Dec 18, 2018 at 09:36:00AM +0100, Richard Leitner wrote: > The sx8654 features a NRST input which may be connected to a GPIO. > Therefore add support for hard-resetting the sx8654 via this NRST. > > If the reset-gpio property is provided the sx8654 is resetted via NRST > instead of the soft-reset via I2C. > > Signed-off-by: Richard Leitner <richard.leitner@skidata.com> > --- > drivers/input/touchscreen/sx8654.c | 40 +++++++++++++++++++++++++++++++++++--- > 1 file changed, 37 insertions(+), 3 deletions(-) > > diff --git a/drivers/input/touchscreen/sx8654.c b/drivers/input/touchscreen/sx8654.c > index ed29db3ec731..238f56b1581b 100644 > --- a/drivers/input/touchscreen/sx8654.c > +++ b/drivers/input/touchscreen/sx8654.c > @@ -33,6 +33,8 @@ > #include <linux/i2c.h> > #include <linux/interrupt.h> > #include <linux/irq.h> > +#include <linux/gpio/consumer.h> > +#include <linux/delay.h> > > /* register addresses */ > #define I2C_REG_TOUCH0 0x00 > @@ -74,6 +76,7 @@ > struct sx8654 { > struct input_dev *input; > struct i2c_client *client; > + struct gpio_desc *gpio_reset; > }; > > static irqreturn_t sx8654_irq(int irq, void *handle) > @@ -124,6 +127,27 @@ static irqreturn_t sx8654_irq(int irq, void *handle) > return IRQ_HANDLED; > } > > +static int sx8654_reset(struct sx8654 *ts) > +{ > + int err; > + > + if (ts->gpio_reset) { > + gpiod_set_value_cansleep(ts->gpio_reset, 1); > + udelay(2); /* Tpulse > 1µs */ > + gpiod_set_value_cansleep(ts->gpio_reset, 0); > + > + return 0; I rearranged code a bit to avoid this early return and applied. Thank you. > + } > + > + dev_dbg(&ts->client->dev, "NRST unavailable, try softreset\n"); > + err = i2c_smbus_write_byte_data(ts->client, I2C_REG_SOFTRESET, > + SOFTRESET_VALUE); > + if (err) > + return err; > + else > + return 0; > +} > + > static int sx8654_open(struct input_dev *dev) > { > struct sx8654 *sx8654 = input_get_drvdata(dev); > @@ -186,6 +210,17 @@ static int sx8654_probe(struct i2c_client *client, > if (!sx8654) > return -ENOMEM; > > + sx8654->gpio_reset = devm_gpiod_get_optional(&client->dev, "reset", > + GPIOD_OUT_HIGH); > + if (IS_ERR(sx8654->gpio_reset)) { > + error = PTR_ERR(sx8654->gpio_reset); > + if (error != -EPROBE_DEFER) > + dev_err(&client->dev, "unable to get reset-gpio: %d\n", > + error); > + return error; > + } > + dev_dbg(&client->dev, "got GPIO reset pin\n"); > + > input = devm_input_allocate_device(&client->dev); > if (!input) > return -ENOMEM; > @@ -206,10 +241,9 @@ static int sx8654_probe(struct i2c_client *client, > > input_set_drvdata(sx8654->input, sx8654); > > - error = i2c_smbus_write_byte_data(client, I2C_REG_SOFTRESET, > - SOFTRESET_VALUE); > + error = sx8654_reset(sx8654); > if (error) { > - dev_err(&client->dev, "writing softreset value failed"); > + dev_err(&client->dev, "reset failed"); > return error; > } > > -- > 2.11.0 >
On Tue, Dec 18, 2018 at 09:36:02AM +0100, Richard Leitner wrote: > As the sx865[456] share the same datasheet and differ only in the > presence of a "capacitive proximity detection circuit" and a "haptics > motor driver for LRA/ERM" add them to the compatbiles. As the driver > doesn't implement these features it should be no problem. > > Signed-off-by: Richard Leitner <richard.leitner@skidata.com> Applied, thank you. > --- > drivers/input/touchscreen/sx8654.c | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/drivers/input/touchscreen/sx8654.c b/drivers/input/touchscreen/sx8654.c > index 238f56b1581b..afa4da138fe9 100644 > --- a/drivers/input/touchscreen/sx8654.c > +++ b/drivers/input/touchscreen/sx8654.c > @@ -293,6 +293,8 @@ static int sx8654_probe(struct i2c_client *client, > #ifdef CONFIG_OF > static const struct of_device_id sx8654_of_match[] = { > { .compatible = "semtech,sx8654", }, > + { .compatible = "semtech,sx8655", }, > + { .compatible = "semtech,sx8656", }, > { }, > }; > MODULE_DEVICE_TABLE(of, sx8654_of_match); > @@ -300,6 +302,8 @@ MODULE_DEVICE_TABLE(of, sx8654_of_match); > > static const struct i2c_device_id sx8654_id_table[] = { > { "semtech_sx8654", 0 }, > + { "semtech_sx8655", 0 }, > + { "semtech_sx8656", 0 }, > { }, > }; > MODULE_DEVICE_TABLE(i2c, sx8654_id_table); > -- > 2.11.0 >
On Mon, 2019-01-28 at 16:25 -0800, Dmitry Torokhov wrote: > On Tue, Dec 18, 2018 at 09:40:02AM +0100, Richard Leitner wrote: > > Some of the #defined register values are one-bit flags. Convert them to > > use the BIT(x) macro instead of 1 byte hexadecimal values. This improves > > readability and clarifies the intent. > > > > Signed-off-by: Richard Leitner <richard.leitner@skidata.com> > > Applied, thank you. Not so sure this should be applied. > > diff --git a/drivers/input/touchscreen/sx8654.c b/drivers/input/touchscreen/sx8654.c [] > > @@ -46,7 +47,7 @@ [] > > /* bits for I2C_REG_IRQSRC */ > > -#define IRQ_PENTOUCH_TOUCHCONVDONE 0x08 > > -#define IRQ_PENRELEASE 0x04 > > +#define IRQ_PENTOUCH_TOUCHCONVDONE BIT(7) > > +#define IRQ_PENRELEASE BIT(6) Shouldn't this be BIT(3) and BIT(2) or did you mean to change the values too? If so, this change should be noted in the commit message.
Hi Joe, On 29/01/2019 06:40, Joe Perches wrote: > On Mon, 2019-01-28 at 16:25 -0800, Dmitry Torokhov wrote: >> On Tue, Dec 18, 2018 at 09:40:02AM +0100, Richard Leitner wrote: >>> Some of the #defined register values are one-bit flags. Convert them to >>> use the BIT(x) macro instead of 1 byte hexadecimal values. This improves >>> readability and clarifies the intent. >>> >>> Signed-off-by: Richard Leitner <richard.leitner@skidata.com> >> >> Applied, thank you. > > Not so sure this should be applied. > >>> diff --git a/drivers/input/touchscreen/sx8654.c b/drivers/input/touchscreen/sx8654.c > [] >>> @@ -46,7 +47,7 @@ > [] >>> /* bits for I2C_REG_IRQSRC */ >>> -#define IRQ_PENTOUCH_TOUCHCONVDONE 0x08 >>> -#define IRQ_PENRELEASE 0x04 >>> +#define IRQ_PENTOUCH_TOUCHCONVDONE BIT(7) >>> +#define IRQ_PENRELEASE BIT(6) > > Shouldn't this be BIT(3) and BIT(2) > or did you mean to change the values too? > > If so, this change should be noted in the commit message. > That's true, those values should stay the same. Thanks for the catch! @Dimitry: Should I send an updated version or do you fix it yourself? regards;Richard.L
On Tue, Jan 29, 2019 at 12:23:01PM +0100, Richard Leitner wrote: > Hi Joe, > > On 29/01/2019 06:40, Joe Perches wrote: > > On Mon, 2019-01-28 at 16:25 -0800, Dmitry Torokhov wrote: > > > On Tue, Dec 18, 2018 at 09:40:02AM +0100, Richard Leitner wrote: > > > > Some of the #defined register values are one-bit flags. Convert them to > > > > use the BIT(x) macro instead of 1 byte hexadecimal values. This improves > > > > readability and clarifies the intent. > > > > > > > > Signed-off-by: Richard Leitner <richard.leitner@skidata.com> > > > > > > Applied, thank you. > > > > Not so sure this should be applied. > > > > > > diff --git a/drivers/input/touchscreen/sx8654.c b/drivers/input/touchscreen/sx8654.c > > [] > > > > @@ -46,7 +47,7 @@ > > [] > > > > /* bits for I2C_REG_IRQSRC */ > > > > -#define IRQ_PENTOUCH_TOUCHCONVDONE 0x08 > > > > -#define IRQ_PENRELEASE 0x04 > > > > +#define IRQ_PENTOUCH_TOUCHCONVDONE BIT(7) > > > > +#define IRQ_PENRELEASE BIT(6) > > > > Shouldn't this be BIT(3) and BIT(2) > > or did you mean to change the values too? > > > > If so, this change should be noted in the commit message. > > > > That's true, those values should stay the same. Thanks for the catch! Thanks Joe! > > @Dimitry: Should I send an updated version or do you fix it yourself? > I updated the values and pushed out the new version. Thanks.