| Message ID | 20230910002038.56362-1-zenmchen@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Kalle Valo |
| Headers | show |
| Series | wifi: rtl8xxxu: fix LED control code of RTL8192FU | expand |
> -----Original Message----- > From: Zenm Chen <zenmchen@gmail.com> > Sent: Sunday, September 10, 2023 8:21 AM > To: Jes.Sorensen@gmail.com > Cc: kvalo@kernel.org; linux-wireless@vger.kernel.org; linux-kernel@vger.kernel.org; > rtl8821cerfe2@gmail.com; Ping-Ke Shih <pkshih@realtek.com>; Zenm Chen <zenmchen@gmail.com> > Subject: [PATCH] wifi: rtl8xxxu: fix LED control code of RTL8192FU > > ASUS USB-N13 C1 is a wifi adapter based on a RTL8192FU chip and its LED > doesn't blink with the rtl8xxxu driver. This patch fix this via writing > the correct value to the register "REG_LEDCFG0". > > This was tested with these two wifi adapters: > ASUS USB-N13 C1 (vid=0x0b05, pid=0x18f1, rfe_type=0x1) > MERCURY MW310UH (vid=0x0bda, pid=0xf192, rfe_type=0x5) Did you ever try Linux vendor driver for software and hardware LED? Seemingly, the vendor driver is different from yours, but I'm pretty sure which one is correct because I don't have a RTL8192FU stick. > > Signed-off-by: Zenm Chen <zenmchen@gmail.com> > --- > .../realtek/rtl8xxxu/rtl8xxxu_8192f.c | 24 ++++++++----------- > 1 file changed, 10 insertions(+), 14 deletions(-) > > diff --git a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_8192f.c > b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_8192f.c > index 28e93835e05a..ab6d4e4e228e 100644 > --- a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_8192f.c > +++ b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_8192f.c > @@ -2014,26 +2014,22 @@ static int rtl8192fu_led_brightness_set(struct led_classdev *led_cdev, > struct rtl8xxxu_priv *priv = container_of(led_cdev, > struct rtl8xxxu_priv, > led_cdev); > - u16 ledcfg; > + u32 ledcfg; > > /* Values obtained by observing the USB traffic from the Windows driver. */ > rtl8xxxu_write32(priv, REG_SW_GPIO_SHARE_CTRL_0, 0x20080); > rtl8xxxu_write32(priv, REG_SW_GPIO_SHARE_CTRL_1, 0x1b0000); > > - ledcfg = rtl8xxxu_read16(priv, REG_LEDCFG0); > - > - if (brightness == LED_OFF) { > - /* Value obtained like above. */ > - ledcfg = BIT(1) | BIT(7); > - } else if (brightness == LED_ON) { > - /* Value obtained like above. */ > - ledcfg = BIT(1) | BIT(7) | BIT(11); > - } else if (brightness == RTL8XXXU_HW_LED_CONTROL) { > - /* Value obtained by brute force. */ > - ledcfg = BIT(8) | BIT(9); > - } The definitions are used by this driver: #define BIT_LED2EN BIT(21) #define BIT_LED1SV BIT(11) // the same definition with BIT_LED0SV #define BIT_LED1CM GENMASK(10, 8) // use the same value of BIT_LED0CM #define BIT_LED0SV BIT(3) #define BIT_LED0CM GENMASK(2, 0) #define LED_MODE_TX_OR_RX_EVENTS 3 #define LED_MODE_SW_CTRL 0 > + /* Enable LED0 GPIO and turn LED0/LED1 off. */ > + ledcfg = (rtl8xxxu_read32(priv, REG_LEDCFG0) | BIT(21)) & ~0xffff; Split this statement into multiple lines and use definitions I provide above. > + > + /* Values obtained by brute force. */ > + if (brightness == LED_ON) > + ledcfg |= BIT(3) | BIT(11); > + else if (brightness == RTL8XXXU_HW_LED_CONTROL) > + ledcfg |= BIT(0) | BIT(1) | BIT(8) | BIT(9); > > - rtl8xxxu_write16(priv, REG_LEDCFG0, ledcfg); > + rtl8xxxu_write32(priv, REG_LEDCFG0, ledcfg); > > return 0; > } > -- > 2.42.0
On 10/09/2023 03:20, Zenm Chen wrote: > ASUS USB-N13 C1 is a wifi adapter based on a RTL8192FU chip and its LED > doesn't blink with the rtl8xxxu driver. This patch fix this via writing > the correct value to the register "REG_LEDCFG0". > > This was tested with these two wifi adapters: > ASUS USB-N13 C1 (vid=0x0b05, pid=0x18f1, rfe_type=0x1) > MERCURY MW310UH (vid=0x0bda, pid=0xf192, rfe_type=0x5) > > Signed-off-by: Zenm Chen <zenmchen@gmail.com> > --- > .../realtek/rtl8xxxu/rtl8xxxu_8192f.c | 24 ++++++++----------- > 1 file changed, 10 insertions(+), 14 deletions(-) > > diff --git a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_8192f.c b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_8192f.c > index 28e93835e05a..ab6d4e4e228e 100644 > --- a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_8192f.c > +++ b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_8192f.c > @@ -2014,26 +2014,22 @@ static int rtl8192fu_led_brightness_set(struct led_classdev *led_cdev, > struct rtl8xxxu_priv *priv = container_of(led_cdev, > struct rtl8xxxu_priv, > led_cdev); > - u16 ledcfg; > + u32 ledcfg; > > /* Values obtained by observing the USB traffic from the Windows driver. */ > rtl8xxxu_write32(priv, REG_SW_GPIO_SHARE_CTRL_0, 0x20080); > rtl8xxxu_write32(priv, REG_SW_GPIO_SHARE_CTRL_1, 0x1b0000); > > - ledcfg = rtl8xxxu_read16(priv, REG_LEDCFG0); > - > - if (brightness == LED_OFF) { > - /* Value obtained like above. */ > - ledcfg = BIT(1) | BIT(7); > - } else if (brightness == LED_ON) { > - /* Value obtained like above. */ > - ledcfg = BIT(1) | BIT(7) | BIT(11); > - } else if (brightness == RTL8XXXU_HW_LED_CONTROL) { > - /* Value obtained by brute force. */ > - ledcfg = BIT(8) | BIT(9); > - } > + /* Enable LED0 GPIO and turn LED0/LED1 off. */ > + ledcfg = (rtl8xxxu_read32(priv, REG_LEDCFG0) | BIT(21)) & ~0xffff; > + > + /* Values obtained by brute force. */ > + if (brightness == LED_ON) > + ledcfg |= BIT(3) | BIT(11); Can you explain in the commit message why you changed the LED_ON and LED_OFF branches? It's not obvious to me and they don't have anything to do with the hardware-controlled blinking. > + else if (brightness == RTL8XXXU_HW_LED_CONTROL) > + ledcfg |= BIT(0) | BIT(1) | BIT(8) | BIT(9); > > - rtl8xxxu_write16(priv, REG_LEDCFG0, ledcfg); > + rtl8xxxu_write32(priv, REG_LEDCFG0, ledcfg); > > return 0; > } My Comfast CF-826F still works fine with this patch.
Bitterblue Smith <rtl8821cerfe2@gmail.com> wrote： > > Can you explain in the commit message why you changed the > LED_ON and LED_OFF branches? It's not obvious to me and they > don't have anything to do with the hardware-controlled blinking. > Hi, Sorry that I didn't explain this clearly. After some researches these days, I came to some conclusions: 1. The LED of MERCURY MW310UH and COMFAST CF-826F seems to be controlled by REG_LEDCFG1, and currently rtl8xxxu controls the LED through writing some suitable values to this register, so MW310UH and CF-826F work fine with rtl8xxxu. 2. The LED of ASUS USB-N13 C1 seems to be controlled by another register "REG_LEDCFG0" for unknown reason, so when I write 1 or 2 to /sys/class/leds/rtl8xxxu-usbX-Y/brightness, the LED doesn't turn on or blink because the register "REG_LEDCFG0" is not filled with a suitable value, This is why I changed the LED_ON and LED_OFF branches. I will modify the commit message in the next version of patch, thanks.
Ping-Ke Shih <pkshih@realtek.com> wrote： > > > > > -----Original Message----- > > From: Zenm Chen <zenmchen@gmail.com> > > Sent: Sunday, September 10, 2023 8:21 AM > > To: Jes.Sorensen@gmail.com > > Cc: kvalo@kernel.org; linux-wireless@vger.kernel.org; linux-kernel@vger.kernel.org; > > rtl8821cerfe2@gmail.com; Ping-Ke Shih <pkshih@realtek.com>; Zenm Chen <zenmchen@gmail.com> > > Subject: [PATCH] wifi: rtl8xxxu: fix LED control code of RTL8192FU > > > > ASUS USB-N13 C1 is a wifi adapter based on a RTL8192FU chip and its LED > > doesn't blink with the rtl8xxxu driver. This patch fix this via writing > > the correct value to the register "REG_LEDCFG0". > > > > This was tested with these two wifi adapters: > > ASUS USB-N13 C1 (vid=0x0b05, pid=0x18f1, rfe_type=0x1) > > MERCURY MW310UH (vid=0x0bda, pid=0xf192, rfe_type=0x5) > > Did you ever try Linux vendor driver for software and hardware LED? > Seemingly, the vendor driver is different from yours, but I'm pretty > sure which one is correct because I don't have a RTL8192FU stick. > > > > > Signed-off-by: Zenm Chen <zenmchen@gmail.com> > > --- > > .../realtek/rtl8xxxu/rtl8xxxu_8192f.c | 24 ++++++++----------- > > 1 file changed, 10 insertions(+), 14 deletions(-) > > > > diff --git a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_8192f.c > > b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_8192f.c > > index 28e93835e05a..ab6d4e4e228e 100644 > > --- a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_8192f.c > > +++ b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_8192f.c > > @@ -2014,26 +2014,22 @@ static int rtl8192fu_led_brightness_set(struct led_classdev *led_cdev, > > struct rtl8xxxu_priv *priv = container_of(led_cdev, > > struct rtl8xxxu_priv, > > led_cdev); > > - u16 ledcfg; > > + u32 ledcfg; > > > > /* Values obtained by observing the USB traffic from the Windows driver. */ > > rtl8xxxu_write32(priv, REG_SW_GPIO_SHARE_CTRL_0, 0x20080); > > rtl8xxxu_write32(priv, REG_SW_GPIO_SHARE_CTRL_1, 0x1b0000); > > > > - ledcfg = rtl8xxxu_read16(priv, REG_LEDCFG0); > > - > > - if (brightness == LED_OFF) { > > - /* Value obtained like above. */ > > - ledcfg = BIT(1) | BIT(7); > > - } else if (brightness == LED_ON) { > > - /* Value obtained like above. */ > > - ledcfg = BIT(1) | BIT(7) | BIT(11); > > - } else if (brightness == RTL8XXXU_HW_LED_CONTROL) { > > - /* Value obtained by brute force. */ > > - ledcfg = BIT(8) | BIT(9); > > - } > > The definitions are used by this driver: > > #define BIT_LED2EN BIT(21) > #define BIT_LED1SV BIT(11) // the same definition with BIT_LED0SV > #define BIT_LED1CM GENMASK(10, 8) // use the same value of BIT_LED0CM > #define BIT_LED0SV BIT(3) > #define BIT_LED0CM GENMASK(2, 0) > #define LED_MODE_TX_OR_RX_EVENTS 3 > #define LED_MODE_SW_CTRL 0 > > > > + /* Enable LED0 GPIO and turn LED0/LED1 off. */ > > + ledcfg = (rtl8xxxu_read32(priv, REG_LEDCFG0) | BIT(21)) & ~0xffff; > > Split this statement into multiple lines and use definitions I provide above. > > > + > > + /* Values obtained by brute force. */ > > + if (brightness == LED_ON) > > + ledcfg |= BIT(3) | BIT(11); > > + else if (brightness == RTL8XXXU_HW_LED_CONTROL) > > + ledcfg |= BIT(0) | BIT(1) | BIT(8) | BIT(9); > > > > - rtl8xxxu_write16(priv, REG_LEDCFG0, ledcfg); > > + rtl8xxxu_write32(priv, REG_LEDCFG0, ledcfg); > > > > return 0; > > } > > -- > > 2.42.0 > Thanks for the help and suggestions, I will make these changes. :)
diff --git a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_8192f.c b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_8192f.c index 28e93835e05a..ab6d4e4e228e 100644 --- a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_8192f.c +++ b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_8192f.c @@ -2014,26 +2014,22 @@ static int rtl8192fu_led_brightness_set(struct led_classdev *led_cdev, struct rtl8xxxu_priv *priv = container_of(led_cdev, struct rtl8xxxu_priv, led_cdev); - u16 ledcfg; + u32 ledcfg; /* Values obtained by observing the USB traffic from the Windows driver. */ rtl8xxxu_write32(priv, REG_SW_GPIO_SHARE_CTRL_0, 0x20080); rtl8xxxu_write32(priv, REG_SW_GPIO_SHARE_CTRL_1, 0x1b0000); - ledcfg = rtl8xxxu_read16(priv, REG_LEDCFG0); - - if (brightness == LED_OFF) { - /* Value obtained like above. */ - ledcfg = BIT(1) | BIT(7); - } else if (brightness == LED_ON) { - /* Value obtained like above. */ - ledcfg = BIT(1) | BIT(7) | BIT(11); - } else if (brightness == RTL8XXXU_HW_LED_CONTROL) { - /* Value obtained by brute force. */ - ledcfg = BIT(8) | BIT(9); - } + /* Enable LED0 GPIO and turn LED0/LED1 off. */ + ledcfg = (rtl8xxxu_read32(priv, REG_LEDCFG0) | BIT(21)) & ~0xffff; + + /* Values obtained by brute force. */ + if (brightness == LED_ON) + ledcfg |= BIT(3) | BIT(11); + else if (brightness == RTL8XXXU_HW_LED_CONTROL) + ledcfg |= BIT(0) | BIT(1) | BIT(8) | BIT(9); - rtl8xxxu_write16(priv, REG_LEDCFG0, ledcfg); + rtl8xxxu_write32(priv, REG_LEDCFG0, ledcfg); return 0; }
ASUS USB-N13 C1 is a wifi adapter based on a RTL8192FU chip and its LED doesn't blink with the rtl8xxxu driver. This patch fix this via writing the correct value to the register "REG_LEDCFG0". This was tested with these two wifi adapters: ASUS USB-N13 C1 (vid=0x0b05, pid=0x18f1, rfe_type=0x1) MERCURY MW310UH (vid=0x0bda, pid=0xf192, rfe_type=0x5) Signed-off-by: Zenm Chen <zenmchen@gmail.com> --- .../realtek/rtl8xxxu/rtl8xxxu_8192f.c | 24 ++++++++----------- 1 file changed, 10 insertions(+), 14 deletions(-)