Hi Nick, On Mon, May 29, 2017 at 10:41:51PM -0700, Nick Desaulniers wrote: > Clang warns: > > drivers/input/mousedev.c:653:63: error: implicit conversion from 'int' > to 'signed char' changes value from 200 to -56 > [-Wconstant-conversion] > client->ps2[1] = 0x60; client->ps2[2] = 3; client->ps2[3] = 200; > ~ ^~~ > > As far as I can tell, from > > http://www.computer-engineering.org/ps2mouse/ > > Under "Command Set" > "0xE9 (Status Request)" > > the value 200 is a valid sample rate. Using unsigned char [], rather than > signed char [], for client->ps2 silences this warning. > > Also moves some reused logic into a helper function. > > Signed-off-by: Nick Desaulniers <[email protected]> > --- > What's new in v4: > * replace mousedev_limit_delta() with update_clamped() that also updates > the ps2_data and delta values. The use of the temporary val should > avoid integral conversion and promotion issues. > > drivers/input/mousedev.c | 23 +++++++++++------------ > 1 file changed, 11 insertions(+), 12 deletions(-) > > diff --git a/drivers/input/mousedev.c b/drivers/input/mousedev.c > index 0e0ff84088fd..e645b8c6f2cb 100644 > --- a/drivers/input/mousedev.c > +++ b/drivers/input/mousedev.c > @@ -103,7 +103,7 @@ struct mousedev_client { > spinlock_t packet_lock; > int pos_x, pos_y; > > - signed char ps2[6]; > + unsigned char ps2[6]; > unsigned char ready, buffer, bufsiz; > unsigned char imexseq, impsseq; > enum mousedev_emul mode; > @@ -571,27 +571,27 @@ static int mousedev_open(struct inode *inode, struct > file *file) > return error; > } > > -static inline int mousedev_limit_delta(int delta, int limit) > +static inline void > +update_clamped(unsigned char *ps2_data, int *delta, int limit) > { > - return delta > limit ? limit : (delta < -limit ? -limit : delta); > + int val = *delta > limit ? limit : (*delta < -limit ? -limit : *delta); > + *ps2_data = (unsigned char) val; > + *delta -= val;
Since the time the code was written we got nice helpers to clamp the values. Does the following work for you or it still leaves clang unhappy? Thanks. -- Dmitry Input: mousedev - fix implicit conversion warning From: Nick Desaulniers <[email protected]> Clang warns: drivers/input/mousedev.c:653:63: error: implicit conversion from 'int' to 'signed char' changes value from 200 to -56 [-Wconstant-conversion] client->ps2[1] = 0x60; client->ps2[2] = 3; client->ps2[3] = 200; ~ ^~~ As the PS2 data is really a stream of bytes, let's switch to using u8 type for it, which silences this warning. Signed-off-by: Nick Desaulniers <[email protected]> Patchwork-Id: 9753771 Signed-off-by: Dmitry Torokhov <[email protected]> --- drivers/input/mousedev.c | 60 +++++++++++++++++++++++++--------------------- 1 file changed, 32 insertions(+), 28 deletions(-) diff --git a/drivers/input/mousedev.c b/drivers/input/mousedev.c index 0e0ff84088fd..6ef0c5314f69 100644 --- a/drivers/input/mousedev.c +++ b/drivers/input/mousedev.c @@ -15,6 +15,7 @@ #define MOUSEDEV_MINORS 31 #define MOUSEDEV_MIX 63 +#include <linux/bitops.h> #include <linux/sched.h> #include <linux/slab.h> #include <linux/poll.h> @@ -103,7 +104,7 @@ struct mousedev_client { spinlock_t packet_lock; int pos_x, pos_y; - signed char ps2[6]; + u8 ps2[6]; unsigned char ready, buffer, bufsiz; unsigned char imexseq, impsseq; enum mousedev_emul mode; @@ -291,11 +292,10 @@ static void mousedev_notify_readers(struct mousedev *mousedev, } client->pos_x += packet->dx; - client->pos_x = client->pos_x < 0 ? - 0 : (client->pos_x >= xres ? xres : client->pos_x); + client->pos_x = clamp_val(client->pos_x, 0, xres); + client->pos_y += packet->dy; - client->pos_y = client->pos_y < 0 ? - 0 : (client->pos_y >= yres ? yres : client->pos_y); + client->pos_y = clamp_val(client->pos_y, 0, yres); p->dx += packet->dx; p->dy += packet->dy; @@ -571,44 +571,48 @@ static int mousedev_open(struct inode *inode, struct file *file) return error; } -static inline int mousedev_limit_delta(int delta, int limit) -{ - return delta > limit ? limit : (delta < -limit ? -limit : delta); -} - -static void mousedev_packet(struct mousedev_client *client, - signed char *ps2_data) +static void mousedev_packet(struct mousedev_client *client, u8 *ps2_data) { struct mousedev_motion *p = &client->packets[client->tail]; + s8 dx, dy, dz; + + dx = clamp_val(p->dx, -127, 127); + p->dx -= dx; + + dy = clamp_val(p->dy, -127, 127); + p->dy -= dy; - ps2_data[0] = 0x08 | - ((p->dx < 0) << 4) | ((p->dy < 0) << 5) | (p->buttons & 0x07); - ps2_data[1] = mousedev_limit_delta(p->dx, 127); - ps2_data[2] = mousedev_limit_delta(p->dy, 127); - p->dx -= ps2_data[1]; - p->dy -= ps2_data[2]; + ps2_data[0] = BIT(3); + ps2_data[0] |= ((dx & BIT(7)) >> 3) | ((dy & BIT(7)) >> 2); + ps2_data[0] |= p->buttons & 0x07; switch (client->mode) { case MOUSEDEV_EMUL_EXPS: - ps2_data[3] = mousedev_limit_delta(p->dz, 7); - p->dz -= ps2_data[3]; - ps2_data[3] = (ps2_data[3] & 0x0f) | ((p->buttons & 0x18) << 1); + dz = clamp_val(p->dz, -7, 7); + p->dz -= dz; + + ps2_data[3] = (dz & 0x0f) | ((p->buttons & 0x18) << 1); client->bufsiz = 4; break; case MOUSEDEV_EMUL_IMPS: - ps2_data[0] |= - ((p->buttons & 0x10) >> 3) | ((p->buttons & 0x08) >> 1); - ps2_data[3] = mousedev_limit_delta(p->dz, 127); - p->dz -= ps2_data[3]; + dz = clamp_val(p->dz, -127, 127); + p->dz -= dz; + + ps2_data[0] |= ((p->buttons & 0x10) >> 3) | + ((p->buttons & 0x08) >> 1); + ps2_data[3] = dz; + client->bufsiz = 4; break; case MOUSEDEV_EMUL_PS2: default: - ps2_data[0] |= - ((p->buttons & 0x10) >> 3) | ((p->buttons & 0x08) >> 1); p->dz = 0; + + ps2_data[0] |= ((p->buttons & 0x10) >> 3) | + ((p->buttons & 0x08) >> 1); + client->bufsiz = 3; break; } @@ -714,7 +718,7 @@ static ssize_t mousedev_read(struct file *file, char __user *buffer, { struct mousedev_client *client = file->private_data; struct mousedev *mousedev = client->mousedev; - signed char data[sizeof(client->ps2)]; + u8 data[sizeof(client->ps2)]; int retval = 0; if (!client->ready && !client->buffer && mousedev->exist &&
