Received-SPF: pass (google.com: domain of linux-wireless-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <668921d6-cff5-7b42-3406-f42f445a15ef@gmail.com>
Date:   Wed, 18 Jan 2023 15:52:59 +0200
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.3.3
Subject: Re: [PATCH 1/4] wifi: rtl8xxxu: Register the LED and make it blink
To:     Ping-Ke Shih <pkshih@realtek.com>,
        "linux-wireless@vger.kernel.org" <linux-wireless@vger.kernel.org>
Cc:     Jes Sorensen <Jes.Sorensen@gmail.com>
References: <5c03ae42-a2fd-b39a-e754-f52d8528a167@gmail.com>
 <f2f52496d4274861b5870c1f425fa8e8@realtek.com>
Content-Language: en-US
From:   Bitterblue Smith <rtl8821cerfe2@gmail.com>
In-Reply-To: <f2f52496d4274861b5870c1f425fa8e8@realtek.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 18/01/2023 02:52, Ping-Ke Shih wrote:
> 
> 
>> -----Original Message-----
>> From: Bitterblue Smith <rtl8821cerfe2@gmail.com>
>> Sent: Wednesday, January 18, 2023 6:03 AM
>> To: linux-wireless@vger.kernel.org
>> Cc: Jes Sorensen <Jes.Sorensen@gmail.com>; Ping-Ke Shih <pkshih@realtek.com>
>> 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?

>>
>> 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@gmail.com>
>> ---
>>  .../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.