On Fri, 26 Jul 2024 16:43:54 +0200
Christophe JAILLET <christophe.jaillet@xxxxxxxxxx> wrote:
> Le 26/07/2024 à 12:27, Qiang Ma a écrit :
> > After we turn on the keyboard CAPSL LED and let the system suspend,
> > the keyboard LED is not off, and the kernel log is as follows:
> >
> > [ 185.987574] i8042: [44060] ed -> i8042 (kbd-data)
> > [ 185.988057] i8042: [44061] ** <- i8042 (interrupt, 0, 1)
> > [ 185.988067] i8042: [44061] 04 -> i8042 (kbd-data)
> > [ 185.988248] i8042: [44061] ** <- i8042 (interrupt, 0, 1)
> >
> > The log shows that after the command 0xed is sent, the data
> > sent is 0x04 instead of 0x00.
> >
> > Solution:
> > Add a bitmap variable ledon in the atkbd structure, and then set
> > ledon according to code-value in the event, in the atkbd_set_leds
> > function,
>
> Hi,
>
> s/atkbd_set_leds function/atkbd_set_leds()/
>
> > first look at the value of lenon, if it is 0, there is no need to
>
> s/lenon/ledon/
>
> > look at the value of dev->led, otherwise, Need to look at dev->led
>
> s/Need/need/
>
Thank you very much for pointing it out, I will fix it in the V2 patch
version.
> > to determine the keyboard LED on/off.
> >
> > Signed-off-by: Qiang Ma <maqianga@xxxxxxxxxxxxx>
> > ---
> > drivers/input/keyboard/atkbd.c | 35
> > +++++++++++++++++++++++++--------- 1 file changed, 26
> > insertions(+), 9 deletions(-)
> >
> > diff --git a/drivers/input/keyboard/atkbd.c
> > b/drivers/input/keyboard/atkbd.c index 7f67f9f2946b..27d8f375929e
> > 100644 --- a/drivers/input/keyboard/atkbd.c
> > +++ b/drivers/input/keyboard/atkbd.c
> > @@ -237,6 +237,8 @@ struct atkbd {
> > struct mutex mutex;
> >
> > struct vivaldi_data vdata;
> > +
> > + unsigned long ledon[BITS_TO_LONGS(LED_CNT)];
>
> DECLARE_BITMAP()?
>
Refer to dev->led, instead of calling DECLARE_BITMAP(), consider using
it.
> > };
> >
> > /*
> > @@ -604,24 +606,34 @@ static int atkbd_set_repeat_rate(struct atkbd
> > *atkbd) return ps2_command(&atkbd->ps2dev, ¶m,
> > ATKBD_CMD_SETREP); }
> >
> > +#define ATKBD_DO_LED_TOGGLE(dev, atkbd, type, v, bits,
> > on) \
> > +({
> > \
> > + unsigned char __tmp =
> > 0; \
> > + if (test_bit(LED_##type,
> > atkbd->on)) \
> > + __tmp = test_bit(LED_##type, dev->bits) ? v :
> > 0; \
> > +
> > else
> > \
> > + __tmp =
> > 0; \
>
> Is it needed? __tmp is already initialized as 0.
It's really not necessary.
>
> > +
> > __tmp;
> > \ +}) +
> > static int atkbd_set_leds(struct atkbd *atkbd)
> > {
> > struct input_dev *dev = atkbd->dev;
> > - unsigned char param[2];
> > + unsigned char param[2] = {0};
>
> This extra initialization does not seem necessary. IIUC, the length
> to sent is already encoded in ATKBD_CMD_SETLEDS and
> ATKBD_CMD_EX_SETLEDS.
>
Looking again at the ps2_command function, yes, the length to sent is
already encoded in ATKBD_CMD_SETLEDS and ATKBD_CMD_EX_SETLEDS.
> >
> > - param[0] = (test_bit(LED_SCROLLL, dev->led) ? 1 : 0)
> > - | (test_bit(LED_NUML, dev->led) ? 2 : 0)
> > - | (test_bit(LED_CAPSL, dev->led) ? 4 : 0);
> > + param[0] = ATKBD_DO_LED_TOGGLE(dev, atkbd, SCROLLL, 1,
> > led, ledon)
> > + | ATKBD_DO_LED_TOGGLE(dev, atkbd, NUML, 2,
> > led, ledon)
> > + | ATKBD_DO_LED_TOGGLE(dev, atkbd, CAPSL, 4,
> > led, ledon); if (ps2_command(&atkbd->ps2dev, param,
> > ATKBD_CMD_SETLEDS)) return -1;
> >
> > if (atkbd->extra) {
> > param[0] = 0;
> > - param[1] = (test_bit(LED_COMPOSE, dev->led) ?
> > 0x01 : 0)
> > - | (test_bit(LED_SLEEP, dev->led) ?
> > 0x02 : 0)
> > - | (test_bit(LED_SUSPEND, dev->led) ?
> > 0x04 : 0)
> > - | (test_bit(LED_MISC, dev->led) ?
> > 0x10 : 0)
> > - | (test_bit(LED_MUTE, dev->led) ?
> > 0x20 : 0);
> > + param[1] = ATKBD_DO_LED_TOGGLE(dev, atkbd,
> > COMPOSE, 0x01, led, ledon)
> > + | ATKBD_DO_LED_TOGGLE(dev, atkbd,
> > SLEEP, 0x02, led, ledon)
> > + | ATKBD_DO_LED_TOGGLE(dev, atkbd,
> > SUSPEND, 0x04, led, ledon)
> > + | ATKBD_DO_LED_TOGGLE(dev, atkbd,
> > MISC, 0x10, led, ledon)
> > + | ATKBD_DO_LED_TOGGLE(dev, atkbd,
> > MUTE, 0x20, led, ledon); if (ps2_command(&atkbd->ps2dev, param,
> > ATKBD_CMD_EX_SETLEDS)) return -1;
> > }
> > @@ -695,6 +707,11 @@ static int atkbd_event(struct input_dev *dev,
> > switch (type) {
> >
> > case EV_LED:
> > + if (value)
> > + __set_bit(code, atkbd->ledon);
> > + else
> > + __clear_bit(code, atkbd->ledon);
> > +
> > atkbd_schedule_event_work(atkbd,
> > ATKBD_LED_EVENT_BIT); return 0;
> >
>
>
Thank review, I will fix them in the v2 patch version.
