> -----Original Message-----
> From: Bitterblue Smith <rtl8821cerfe2@xxxxxxxxx>
> Sent: Wednesday, January 18, 2023 9:53 PM
> To: Ping-Ke Shih <pkshih@xxxxxxxxxxx>; linux-wireless@xxxxxxxxxxxxxxx
> Cc: Jes Sorensen <Jes.Sorensen@xxxxxxxxx>
> Subject: Re: [PATCH 1/4] wifi: rtl8xxxu: Register the LED and make it blink
>
> On 18/01/2023 02:52, Ping-Ke Shih wrote:
> >
> >
> >> -----Original Message-----
> >> From: Bitterblue Smith <rtl8821cerfe2@xxxxxxxxx>
> >> Sent: Wednesday, January 18, 2023 6:03 AM
> >> To: linux-wireless@xxxxxxxxxxxxxxx
> >> Cc: Jes Sorensen <Jes.Sorensen@xxxxxxxxx>; Ping-Ke Shih <pkshih@xxxxxxxxxxx>
> >> Subject: [PATCH 1/4] wifi: rtl8xxxu: Register the LED and make it blink
> >>
> >> If the chip can have an LED, register a struct led_classdev and enable
> >> hardware-controlled blinking. When the chip is not transmitting or
> >> receiving anything the LED is off. Otherwise the LED will blink
> >> faster or slower according to the throughput.
> >>
> >> The LED can be controlled from userspace by writing 0, 1, or 2 to
> >> /sys/class/leds/rtl8xxxu-usbX-Y/brightness:
> >> 0 - solid off.
> >> 1 - solid on.
> >> 2 - hardware-controlled blinking.
> >
> > So, do you like
> >
> > #define RTL8XXXU_HW_LED_BLINKING 2
> >
> I'm not sure what you mean. Can you elaborate?
Originally, I would like to replace RTL8XXXU_HW_LED_CONTROL by
RTL8XXXU_HW_LED_BLINKING, because commit messages say "blinking".
After reviewing 2/4, 3/4 and 4/4, I aware that this blinking is
hardware behavior. Then, it becomes hard to me to decide which
name is clear. Anyway, original is fine to me.
>
> >>
> >> In this patch none of the chips advertise having an LED. That will be
> >> added in the next patches.
> >>
> >> Signed-off-by: Bitterblue Smith <rtl8821cerfe2@xxxxxxxxx>
> >> ---
> >> .../net/wireless/realtek/rtl8xxxu/rtl8xxxu.h | 8 +++++
> >> .../wireless/realtek/rtl8xxxu/rtl8xxxu_core.c | 34 +++++++++++++++++++
> >> 2 files changed, 42 insertions(+)
> >>
> >> diff --git a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu.h
> >> b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu.h
> >> index 90268479d3ad..c8cee4a24755 100644
> >> --- a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu.h
> >> +++ b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu.h
> >> @@ -1443,6 +1443,8 @@ struct rtl8xxxu_cfo_tracking {
> >> u32 packet_count_pre;
> >> };
> >>
> >> +#define RTL8XXXU_HW_LED_CONTROL 2
> >> +
> >> struct rtl8xxxu_priv {
> >> struct ieee80211_hw *hw;
> >> struct usb_device *udev;
> >> @@ -1564,6 +1566,10 @@ struct rtl8xxxu_priv {
> >> struct rtl8xxxu_ra_report ra_report;
> >> struct rtl8xxxu_cfo_tracking cfo_tracking;
> >> struct rtl8xxxu_ra_info ra_info;
> >> +
> >> + bool led_registered;
> >> + char led_name[32];
> >> + struct led_classdev led_cdev;
> >> };
> >>
> >> struct rtl8xxxu_rx_urb {
> >> @@ -1613,6 +1619,8 @@ struct rtl8xxxu_fileops {
> >> u32 rts_rate);
> >> void (*set_crystal_cap) (struct rtl8xxxu_priv *priv, u8 crystal_cap);
> >> s8 (*cck_rssi) (struct rtl8xxxu_priv *priv, u8 cck_agc_rpt);
> >> + int (*led_classdev_brightness_set) (struct led_classdev *led_cdev,
> >> + enum led_brightness brightness);
> >> int writeN_block_size;
> >> int rx_agg_buf_size;
> >> char tx_desc_size;
> >> diff --git a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c
> >> b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c
> >> index 35dc777c1fba..b27edd503c23 100644
> >> --- a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c
> >> +++ b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c
> >> @@ -6955,6 +6955,34 @@ static int rtl8xxxu_parse_usb(struct rtl8xxxu_priv *priv,
> >> return ret;
> >> }
> >>
> >> +static void rtl8xxxu_init_led(struct rtl8xxxu_priv *priv)
> >> +{
> >> + struct led_classdev *led = &priv->led_cdev;
> >> +
> >> + led->brightness_set_blocking = priv->fops->led_classdev_brightness_set;
> >> +
> >> + snprintf(priv->led_name, sizeof(priv->led_name),
> >> + "rtl8xxxu-usb%s", dev_name(&priv->udev->dev));
> >> + led->name = priv->led_name;
> >> + led->max_brightness = RTL8XXXU_HW_LED_CONTROL;
> >> +
> >> + if (led_classdev_register(&priv->udev->dev, led))
> >> + return;
> >> +
> >> + priv->led_registered = true;
> >> +
> >> + led->brightness = led->max_brightness;
> >> + priv->fops->led_classdev_brightness_set(led, led->brightness);
> >> +}
> >> +
> >> +static void rtl8xxxu_deinit_led(struct rtl8xxxu_priv *priv)
> >> +{
> >> + struct led_classdev *led = &priv->led_cdev;
> >> +
> >> + priv->fops->led_classdev_brightness_set(led, LED_OFF);
> >> + led_classdev_unregister(led);
> >> +}
> >> +
> >> static int rtl8xxxu_probe(struct usb_interface *interface,
> >> const struct usb_device_id *id)
> >> {
> >> @@ -7135,6 +7163,9 @@ static int rtl8xxxu_probe(struct usb_interface *interface,
> >> goto err_set_intfdata;
> >> }
> >>
> >> + if (priv->fops->led_classdev_brightness_set)
> >
> > I prefer to move this checking statement into rtl8xxxu_init_led(). Then,
> > the flow of rtl8xxxu_probe() looks clean. As well as rtl8xxxu_deinit_led().
> > Just soft suggestion.
> >
> Okay. That works too.
>
> >> + rtl8xxxu_init_led(priv);
> >> +
> >> return 0;
> >>
> >> err_set_intfdata:
> >> @@ -7159,6 +7190,9 @@ static void rtl8xxxu_disconnect(struct usb_interface *interface)
> >> hw = usb_get_intfdata(interface);
> >> priv = hw->priv;
> >>
> >> + if (priv->led_registered)
> >> + rtl8xxxu_deinit_led(priv);
> >> +
> >> ieee80211_unregister_hw(hw);
> >>
> >> priv->fops->power_off(priv);
> >> --
> >> 2.38.0
> >>
> >> ------Please consider the environment before printing this e-mail.
