Re: [PATCH] Input: atkbd - fix LED state at suspend/resume

[Qiang Ma]

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, &param,
> > 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.