[PATCH 1/5] staging: r8188eu: remove unused efuse hal components

[PATCH 1/5] staging: r8188eu: remove unused efuse hal components

[Martin Kaiser]

struct hal_data_8188e contains some components related to efuses which
are not used for rl8188eu.

Signed-off-by: Martin Kaiser <martin@kaiser.cx>
---
 drivers/staging/r8188eu/include/rtl8188e_hal.h | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/staging/r8188eu/include/rtl8188e_hal.h b/drivers/staging/r8188eu/include/rtl8188e_hal.h
index ea879572d6e1..3939bf053de1 100644
--- a/drivers/staging/r8188eu/include/rtl8188e_hal.h
+++ b/drivers/staging/r8188eu/include/rtl8188e_hal.h
@@ -263,9 +263,6 @@ struct hal_data_8188e {
 	u8	bAPKThermalMeterIgnore;
 
 	bool	EepromOrEfuse;
-	/* 92C:256bytes, 88E:512bytes, we use union set (512bytes) */
-	u8	EfuseMap[2][HWSET_MAX_SIZE_512];
-	u8	EfuseUsedPercentage;
 	struct efuse_hal	EfuseHal;
 
 	u8	Index24G_CCK_Base[RF_PATH_MAX][CHANNEL_MAX_NUMBER];
-- 
2.20.1

[PATCH 2/5] staging: r8188eu: remove unused function parameters

[Martin Kaiser]

rtw_usb_if1_init and chip_by_usb_id do not need a
struct usb_device_id parameter.

Signed-off-by: Martin Kaiser <martin@kaiser.cx>
---
 drivers/staging/r8188eu/os_dep/usb_intf.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/drivers/staging/r8188eu/os_dep/usb_intf.c b/drivers/staging/r8188eu/os_dep/usb_intf.c
index 8f1e33d2fff7..4bf89b78a74a 100644
--- a/drivers/staging/r8188eu/os_dep/usb_intf.c
+++ b/drivers/staging/r8188eu/os_dep/usb_intf.c
@@ -250,8 +250,7 @@ static void usb_dvobj_deinit(struct usb_interface *usb_intf)
 
 }
 
-static void chip_by_usb_id(struct adapter *padapter,
-			   const struct usb_device_id *pdid)
+static void chip_by_usb_id(struct adapter *padapter)
 {
 	padapter->chip_type = NULL_CHIP_TYPE;
 	hal_set_hw_type(padapter);
@@ -569,7 +568,7 @@ int rtw_resume_process(struct adapter *padapter)
  */
 
 static struct adapter *rtw_usb_if1_init(struct dvobj_priv *dvobj,
-	struct usb_interface *pusb_intf, const struct usb_device_id *pdid)
+	struct usb_interface *pusb_intf)
 {
 	struct adapter *padapter = NULL;
 	struct net_device *pnetdev = NULL;
@@ -587,7 +586,7 @@ static struct adapter *rtw_usb_if1_init(struct dvobj_priv *dvobj,
 
 	/* step 1-1., decide the chip_type via vid/pid */
 	padapter->interface_type = RTW_USB;
-	chip_by_usb_id(padapter, pdid);
+	chip_by_usb_id(padapter);
 
 	if (rtw_handle_dualmac(padapter, 1) != _SUCCESS)
 		goto free_adapter;
@@ -722,7 +721,7 @@ static int rtw_drv_init(struct usb_interface *pusb_intf, const struct usb_device
 	if (!dvobj)
 		goto exit;
 
-	if1 = rtw_usb_if1_init(dvobj, pusb_intf, pdid);
+	if1 = rtw_usb_if1_init(dvobj, pusb_intf);
 	if (!if1) {
 		DBG_88E("rtw_init_primarystruct adapter Failed!\n");
 		goto free_dvobj;
-- 
2.20.1

[PATCH 3/5] staging: r8188eu: (trivial) remove a duplicate debug print

[Martin Kaiser]

Keep the one that shows the wakeup capability.

Signed-off-by: Martin Kaiser <martin@kaiser.cx>
---
 drivers/staging/r8188eu/os_dep/usb_intf.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/staging/r8188eu/os_dep/usb_intf.c b/drivers/staging/r8188eu/os_dep/usb_intf.c
index 4bf89b78a74a..6e4bf623f788 100644
--- a/drivers/staging/r8188eu/os_dep/usb_intf.c
+++ b/drivers/staging/r8188eu/os_dep/usb_intf.c
@@ -624,7 +624,6 @@ static struct adapter *rtw_usb_if1_init(struct dvobj_priv *dvobj,
 		dvobj->pusbdev->do_remote_wakeup = 1;
 		pusb_intf->needs_remote_wakeup = 1;
 		device_init_wakeup(&pusb_intf->dev, 1);
-		DBG_88E("\n  padapter->pwrctrlpriv.bSupportRemoteWakeup~~~~~~\n");
 		DBG_88E("\n  padapter->pwrctrlpriv.bSupportRemoteWakeup~~~[%d]~~~\n",
 			device_may_wakeup(&pusb_intf->dev));
 	}
-- 
2.20.1

[PATCH 4/5] staging: r8188eu: use proper way to build a module

[Martin Kaiser]

It seems that for now, we can only build this driver as a module.

Use the same mechanism as other drivers (such as rtl8723bs or the
deprecated rtl8188eu) to enforce building as a module, i.e. depend on m
in Kconfig instead of setting CONFIG_R8188EU = m in the Makefile.

If we set CONFIG_R8188EU in the Makefile, this setting will not be visible
in .config.

Signed-off-by: Martin Kaiser <martin@kaiser.cx>
---
 drivers/staging/r8188eu/Kconfig  | 1 +
 drivers/staging/r8188eu/Makefile | 2 --
 2 files changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/staging/r8188eu/Kconfig b/drivers/staging/r8188eu/Kconfig
index 6323d63a4a1b..dc1719d3f2e4 100644
--- a/drivers/staging/r8188eu/Kconfig
+++ b/drivers/staging/r8188eu/Kconfig
@@ -2,6 +2,7 @@
 config R8188EU
 	tristate "Realtek RTL8188EU Wireless LAN NIC driver"
 	depends on WLAN && USB && CFG80211
+	depends on m
 	select WIRELESS_EXT
 	select WEXT_PRIV
 	select LIB80211
diff --git a/drivers/staging/r8188eu/Makefile b/drivers/staging/r8188eu/Makefile
index 7f6658f931d1..cca7a58c5f29 100644
--- a/drivers/staging/r8188eu/Makefile
+++ b/drivers/staging/r8188eu/Makefile
@@ -4,8 +4,6 @@ EXTRA_CFLAGS += -O1
 
 ccflags-y += -D__CHECK_ENDIAN__
 
-CONFIG_R8188EU = m
-
 CONFIG_USB_HCI = y
 
 CONFIG_BT_COEXIST = n
-- 
2.20.1

[PATCH 5/5] staging: r8188eu: remove CONFIG_USB_HCI from Makefile

[Martin Kaiser]

We already depend on USB. There's no need to set CONFIG_USB_HCI in the
Makefile.

Some other Realtek drivers use #ifdef CONFIG_USB_HCI in their code, the
r8188 driver doesn't.

Signed-off-by: Martin Kaiser <martin@kaiser.cx>
---
 drivers/staging/r8188eu/Makefile | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/staging/r8188eu/Makefile b/drivers/staging/r8188eu/Makefile
index cca7a58c5f29..6bd3a0590aa3 100644
--- a/drivers/staging/r8188eu/Makefile
+++ b/drivers/staging/r8188eu/Makefile
@@ -4,8 +4,6 @@ EXTRA_CFLAGS += -O1
 
 ccflags-y += -D__CHECK_ENDIAN__
 
-CONFIG_USB_HCI = y
-
 CONFIG_BT_COEXIST = n
 CONFIG_WOWLAN = n
 
-- 
2.20.1

Re: [PATCH 1/5] staging: r8188eu: remove unused efuse hal components

[Phillip Potter]

On Wed, 11 Aug 2021 at 21:15, Martin Kaiser <martin@kaiser.cx> wrote:
>
> struct hal_data_8188e contains some components related to efuses which
> are not used for rl8188eu.
>
> Signed-off-by: Martin Kaiser <martin@kaiser.cx>
> ---
>  drivers/staging/r8188eu/include/rtl8188e_hal.h | 3 ---
>  1 file changed, 3 deletions(-)
>
> diff --git a/drivers/staging/r8188eu/include/rtl8188e_hal.h b/drivers/staging/r8188eu/include/rtl8188e_hal.h
> index ea879572d6e1..3939bf053de1 100644
> --- a/drivers/staging/r8188eu/include/rtl8188e_hal.h
> +++ b/drivers/staging/r8188eu/include/rtl8188e_hal.h
> @@ -263,9 +263,6 @@ struct hal_data_8188e {
>         u8      bAPKThermalMeterIgnore;
>
>         bool    EepromOrEfuse;
> -       /* 92C:256bytes, 88E:512bytes, we use union set (512bytes) */
> -       u8      EfuseMap[2][HWSET_MAX_SIZE_512];
> -       u8      EfuseUsedPercentage;
>         struct efuse_hal        EfuseHal;
>
>         u8      Index24G_CCK_Base[RF_PATH_MAX][CHANNEL_MAX_NUMBER];
> --
> 2.20.1
>

Dear Martin,

Looks good.

Acked-by: Phillip Potter <phil@philpotter.co.uk>

Regards,
Phil

Re: [PATCH 1/5] staging: r8188eu: remove unused efuse hal components

[Phillip Potter]

On Wed, 11 Aug 2021 at 21:15, Martin Kaiser <martin@kaiser.cx> wrote:
>
> struct hal_data_8188e contains some components related to efuses which
> are not used for rl8188eu.
>
> Signed-off-by: Martin Kaiser <martin@kaiser.cx>
> ---
>  drivers/staging/r8188eu/include/rtl8188e_hal.h | 3 ---
>  1 file changed, 3 deletions(-)
>
> diff --git a/drivers/staging/r8188eu/include/rtl8188e_hal.h b/drivers/staging/r8188eu/include/rtl8188e_hal.h
> index ea879572d6e1..3939bf053de1 100644
> --- a/drivers/staging/r8188eu/include/rtl8188e_hal.h
> +++ b/drivers/staging/r8188eu/include/rtl8188e_hal.h
> @@ -263,9 +263,6 @@ struct hal_data_8188e {
>         u8      bAPKThermalMeterIgnore;
>
>         bool    EepromOrEfuse;
> -       /* 92C:256bytes, 88E:512bytes, we use union set (512bytes) */
> -       u8      EfuseMap[2][HWSET_MAX_SIZE_512];
> -       u8      EfuseUsedPercentage;
>         struct efuse_hal        EfuseHal;
>
>         u8      Index24G_CCK_Base[RF_PATH_MAX][CHANNEL_MAX_NUMBER];
> --
> 2.20.1
>

Dear Martin,

Looks good.

Acked-by: Phillip Potter <phil@philpotter.co.uk>

Regards,
Phil

Re: [PATCH 2/5] staging: r8188eu: remove unused function parameters

[Phillip Potter]

On Wed, 11 Aug 2021 at 21:15, Martin Kaiser <martin@kaiser.cx> wrote:
>
> rtw_usb_if1_init and chip_by_usb_id do not need a
> struct usb_device_id parameter.
>
> Signed-off-by: Martin Kaiser <martin@kaiser.cx>
> ---
>  drivers/staging/r8188eu/os_dep/usb_intf.c | 9 ++++-----
>  1 file changed, 4 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/staging/r8188eu/os_dep/usb_intf.c b/drivers/staging/r8188eu/os_dep/usb_intf.c
> index 8f1e33d2fff7..4bf89b78a74a 100644
> --- a/drivers/staging/r8188eu/os_dep/usb_intf.c
> +++ b/drivers/staging/r8188eu/os_dep/usb_intf.c
> @@ -250,8 +250,7 @@ static void usb_dvobj_deinit(struct usb_interface *usb_intf)
>
>  }
>
> -static void chip_by_usb_id(struct adapter *padapter,
> -                          const struct usb_device_id *pdid)
> +static void chip_by_usb_id(struct adapter *padapter)
>  {
>         padapter->chip_type = NULL_CHIP_TYPE;
>         hal_set_hw_type(padapter);
> @@ -569,7 +568,7 @@ int rtw_resume_process(struct adapter *padapter)
>   */
>
>  static struct adapter *rtw_usb_if1_init(struct dvobj_priv *dvobj,
> -       struct usb_interface *pusb_intf, const struct usb_device_id *pdid)
> +       struct usb_interface *pusb_intf)
>  {
>         struct adapter *padapter = NULL;
>         struct net_device *pnetdev = NULL;
> @@ -587,7 +586,7 @@ static struct adapter *rtw_usb_if1_init(struct dvobj_priv *dvobj,
>
>         /* step 1-1., decide the chip_type via vid/pid */
>         padapter->interface_type = RTW_USB;
> -       chip_by_usb_id(padapter, pdid);
> +       chip_by_usb_id(padapter);
>
>         if (rtw_handle_dualmac(padapter, 1) != _SUCCESS)
>                 goto free_adapter;
> @@ -722,7 +721,7 @@ static int rtw_drv_init(struct usb_interface *pusb_intf, const struct usb_device
>         if (!dvobj)
>                 goto exit;
>
> -       if1 = rtw_usb_if1_init(dvobj, pusb_intf, pdid);
> +       if1 = rtw_usb_if1_init(dvobj, pusb_intf);
>         if (!if1) {
>                 DBG_88E("rtw_init_primarystruct adapter Failed!\n");
>                 goto free_dvobj;
> --
> 2.20.1
>

Dear Martin,

Looks good.

Acked-by: Phillip Potter <phil@philpotter.co.uk>

Regards,
Phil

Re: [PATCH 2/5] staging: r8188eu: remove unused function parameters

[Phillip Potter]

On Wed, 11 Aug 2021 at 21:15, Martin Kaiser <martin@kaiser.cx> wrote:
>
> rtw_usb_if1_init and chip_by_usb_id do not need a
> struct usb_device_id parameter.
>
> Signed-off-by: Martin Kaiser <martin@kaiser.cx>
> ---
>  drivers/staging/r8188eu/os_dep/usb_intf.c | 9 ++++-----
>  1 file changed, 4 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/staging/r8188eu/os_dep/usb_intf.c b/drivers/staging/r8188eu/os_dep/usb_intf.c
> index 8f1e33d2fff7..4bf89b78a74a 100644
> --- a/drivers/staging/r8188eu/os_dep/usb_intf.c
> +++ b/drivers/staging/r8188eu/os_dep/usb_intf.c
> @@ -250,8 +250,7 @@ static void usb_dvobj_deinit(struct usb_interface *usb_intf)
>
>  }
>
> -static void chip_by_usb_id(struct adapter *padapter,
> -                          const struct usb_device_id *pdid)
> +static void chip_by_usb_id(struct adapter *padapter)
>  {
>         padapter->chip_type = NULL_CHIP_TYPE;
>         hal_set_hw_type(padapter);
> @@ -569,7 +568,7 @@ int rtw_resume_process(struct adapter *padapter)
>   */
>
>  static struct adapter *rtw_usb_if1_init(struct dvobj_priv *dvobj,
> -       struct usb_interface *pusb_intf, const struct usb_device_id *pdid)
> +       struct usb_interface *pusb_intf)
>  {
>         struct adapter *padapter = NULL;
>         struct net_device *pnetdev = NULL;
> @@ -587,7 +586,7 @@ static struct adapter *rtw_usb_if1_init(struct dvobj_priv *dvobj,
>
>         /* step 1-1., decide the chip_type via vid/pid */
>         padapter->interface_type = RTW_USB;
> -       chip_by_usb_id(padapter, pdid);
> +       chip_by_usb_id(padapter);
>
>         if (rtw_handle_dualmac(padapter, 1) != _SUCCESS)
>                 goto free_adapter;
> @@ -722,7 +721,7 @@ static int rtw_drv_init(struct usb_interface *pusb_intf, const struct usb_device
>         if (!dvobj)
>                 goto exit;
>
> -       if1 = rtw_usb_if1_init(dvobj, pusb_intf, pdid);
> +       if1 = rtw_usb_if1_init(dvobj, pusb_intf);
>         if (!if1) {
>                 DBG_88E("rtw_init_primarystruct adapter Failed!\n");
>                 goto free_dvobj;
> --
> 2.20.1
>

Dear Martin,

Looks good.

Acked-by: Phillip Potter <phil@philpotter.co.uk>

Regards,
Phil

Re: [PATCH 3/5] staging: r8188eu: (trivial) remove a duplicate debug print

[Phillip Potter]

On Wed, 11 Aug 2021 at 21:15, Martin Kaiser <martin@kaiser.cx> wrote:
>
> Keep the one that shows the wakeup capability.
>
> Signed-off-by: Martin Kaiser <martin@kaiser.cx>
> ---
>  drivers/staging/r8188eu/os_dep/usb_intf.c | 1 -
>  1 file changed, 1 deletion(-)
>
> diff --git a/drivers/staging/r8188eu/os_dep/usb_intf.c b/drivers/staging/r8188eu/os_dep/usb_intf.c
> index 4bf89b78a74a..6e4bf623f788 100644
> --- a/drivers/staging/r8188eu/os_dep/usb_intf.c
> +++ b/drivers/staging/r8188eu/os_dep/usb_intf.c
> @@ -624,7 +624,6 @@ static struct adapter *rtw_usb_if1_init(struct dvobj_priv *dvobj,
>                 dvobj->pusbdev->do_remote_wakeup = 1;
>                 pusb_intf->needs_remote_wakeup = 1;
>                 device_init_wakeup(&pusb_intf->dev, 1);
> -               DBG_88E("\n  padapter->pwrctrlpriv.bSupportRemoteWakeup~~~~~~\n");
>                 DBG_88E("\n  padapter->pwrctrlpriv.bSupportRemoteWakeup~~~[%d]~~~\n",
>                         device_may_wakeup(&pusb_intf->dev));
>         }
> --
> 2.20.1
>

Dear Martin,

Just my personal opinion, but I'd be inclined to strip out all DBG_88E
calls totally. If there are necessary functions being called such as
device_may_wakeup() we can always just keep this part and remove the
macro call (not checked this function out myself yet). Thanks.

Regards,
Phil

Re: [PATCH 3/5] staging: r8188eu: (trivial) remove a duplicate debug print

[Phillip Potter]

On Wed, 11 Aug 2021 at 21:15, Martin Kaiser <martin@kaiser.cx> wrote:
>
> Keep the one that shows the wakeup capability.
>
> Signed-off-by: Martin Kaiser <martin@kaiser.cx>
> ---
>  drivers/staging/r8188eu/os_dep/usb_intf.c | 1 -
>  1 file changed, 1 deletion(-)
>
> diff --git a/drivers/staging/r8188eu/os_dep/usb_intf.c b/drivers/staging/r8188eu/os_dep/usb_intf.c
> index 4bf89b78a74a..6e4bf623f788 100644
> --- a/drivers/staging/r8188eu/os_dep/usb_intf.c
> +++ b/drivers/staging/r8188eu/os_dep/usb_intf.c
> @@ -624,7 +624,6 @@ static struct adapter *rtw_usb_if1_init(struct dvobj_priv *dvobj,
>                 dvobj->pusbdev->do_remote_wakeup = 1;
>                 pusb_intf->needs_remote_wakeup = 1;
>                 device_init_wakeup(&pusb_intf->dev, 1);
> -               DBG_88E("\n  padapter->pwrctrlpriv.bSupportRemoteWakeup~~~~~~\n");
>                 DBG_88E("\n  padapter->pwrctrlpriv.bSupportRemoteWakeup~~~[%d]~~~\n",
>                         device_may_wakeup(&pusb_intf->dev));
>         }
> --
> 2.20.1
>

Dear Martin,

Just my personal opinion, but I'd be inclined to strip out all DBG_88E
calls totally. If there are necessary functions being called such as
device_may_wakeup() we can always just keep this part and remove the
macro call (not checked this function out myself yet). Thanks.

Regards,
Phil

Re: [PATCH 4/5] staging: r8188eu: use proper way to build a module

[Phillip Potter]

On Wed, 11 Aug 2021 at 21:15, Martin Kaiser <martin@kaiser.cx> wrote:
>
> It seems that for now, we can only build this driver as a module.
>
> Use the same mechanism as other drivers (such as rtl8723bs or the
> deprecated rtl8188eu) to enforce building as a module, i.e. depend on m
> in Kconfig instead of setting CONFIG_R8188EU = m in the Makefile.
>
> If we set CONFIG_R8188EU in the Makefile, this setting will not be visible
> in .config.
>
> Signed-off-by: Martin Kaiser <martin@kaiser.cx>
> ---
>  drivers/staging/r8188eu/Kconfig  | 1 +
>  drivers/staging/r8188eu/Makefile | 2 --
>  2 files changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/drivers/staging/r8188eu/Kconfig b/drivers/staging/r8188eu/Kconfig
> index 6323d63a4a1b..dc1719d3f2e4 100644
> --- a/drivers/staging/r8188eu/Kconfig
> +++ b/drivers/staging/r8188eu/Kconfig
> @@ -2,6 +2,7 @@
>  config R8188EU
>         tristate "Realtek RTL8188EU Wireless LAN NIC driver"
>         depends on WLAN && USB && CFG80211
> +       depends on m
>         select WIRELESS_EXT
>         select WEXT_PRIV
>         select LIB80211
> diff --git a/drivers/staging/r8188eu/Makefile b/drivers/staging/r8188eu/Makefile
> index 7f6658f931d1..cca7a58c5f29 100644
> --- a/drivers/staging/r8188eu/Makefile
> +++ b/drivers/staging/r8188eu/Makefile
> @@ -4,8 +4,6 @@ EXTRA_CFLAGS += -O1
>
>  ccflags-y += -D__CHECK_ENDIAN__
>
> -CONFIG_R8188EU = m
> -
>  CONFIG_USB_HCI = y
>
>  CONFIG_BT_COEXIST = n
> --
> 2.20.1
>

Dear Martin,

Looks good.

Acked-by: Phillip Potter <phil@philpotter.co.uk>

Regards,
Phil

Re: [PATCH 4/5] staging: r8188eu: use proper way to build a module

[Phillip Potter]

On Wed, 11 Aug 2021 at 21:15, Martin Kaiser <martin@kaiser.cx> wrote:
>
> It seems that for now, we can only build this driver as a module.
>
> Use the same mechanism as other drivers (such as rtl8723bs or the
> deprecated rtl8188eu) to enforce building as a module, i.e. depend on m
> in Kconfig instead of setting CONFIG_R8188EU = m in the Makefile.
>
> If we set CONFIG_R8188EU in the Makefile, this setting will not be visible
> in .config.
>
> Signed-off-by: Martin Kaiser <martin@kaiser.cx>
> ---
>  drivers/staging/r8188eu/Kconfig  | 1 +
>  drivers/staging/r8188eu/Makefile | 2 --
>  2 files changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/drivers/staging/r8188eu/Kconfig b/drivers/staging/r8188eu/Kconfig
> index 6323d63a4a1b..dc1719d3f2e4 100644
> --- a/drivers/staging/r8188eu/Kconfig
> +++ b/drivers/staging/r8188eu/Kconfig
> @@ -2,6 +2,7 @@
>  config R8188EU
>         tristate "Realtek RTL8188EU Wireless LAN NIC driver"
>         depends on WLAN && USB && CFG80211
> +       depends on m
>         select WIRELESS_EXT
>         select WEXT_PRIV
>         select LIB80211
> diff --git a/drivers/staging/r8188eu/Makefile b/drivers/staging/r8188eu/Makefile
> index 7f6658f931d1..cca7a58c5f29 100644
> --- a/drivers/staging/r8188eu/Makefile
> +++ b/drivers/staging/r8188eu/Makefile
> @@ -4,8 +4,6 @@ EXTRA_CFLAGS += -O1
>
>  ccflags-y += -D__CHECK_ENDIAN__
>
> -CONFIG_R8188EU = m
> -
>  CONFIG_USB_HCI = y
>
>  CONFIG_BT_COEXIST = n
> --
> 2.20.1
>

Dear Martin,

Looks good.

Acked-by: Phillip Potter <phil@philpotter.co.uk>

Regards,
Phil

Re: [PATCH 5/5] staging: r8188eu: remove CONFIG_USB_HCI from Makefile

[Phillip Potter]

On Wed, 11 Aug 2021 at 21:15, Martin Kaiser <martin@kaiser.cx> wrote:
>
> We already depend on USB. There's no need to set CONFIG_USB_HCI in the
> Makefile.
>
> Some other Realtek drivers use #ifdef CONFIG_USB_HCI in their code, the
> r8188 driver doesn't.
>
> Signed-off-by: Martin Kaiser <martin@kaiser.cx>
> ---
>  drivers/staging/r8188eu/Makefile | 2 --
>  1 file changed, 2 deletions(-)
>
> diff --git a/drivers/staging/r8188eu/Makefile b/drivers/staging/r8188eu/Makefile
> index cca7a58c5f29..6bd3a0590aa3 100644
> --- a/drivers/staging/r8188eu/Makefile
> +++ b/drivers/staging/r8188eu/Makefile
> @@ -4,8 +4,6 @@ EXTRA_CFLAGS += -O1
>
>  ccflags-y += -D__CHECK_ENDIAN__
>
> -CONFIG_USB_HCI = y
> -
>  CONFIG_BT_COEXIST = n
>  CONFIG_WOWLAN = n
>
> --
> 2.20.1
>

Dear Martin,

Looks good.

Acked-by: Phillip Potter <phil@philpotter.co.uk>

Regards,
Phil

Re: [PATCH 5/5] staging: r8188eu: remove CONFIG_USB_HCI from Makefile

[Phillip Potter]

On Wed, 11 Aug 2021 at 21:15, Martin Kaiser <martin@kaiser.cx> wrote:
>
> We already depend on USB. There's no need to set CONFIG_USB_HCI in the
> Makefile.
>
> Some other Realtek drivers use #ifdef CONFIG_USB_HCI in their code, the
> r8188 driver doesn't.
>
> Signed-off-by: Martin Kaiser <martin@kaiser.cx>
> ---
>  drivers/staging/r8188eu/Makefile | 2 --
>  1 file changed, 2 deletions(-)
>
> diff --git a/drivers/staging/r8188eu/Makefile b/drivers/staging/r8188eu/Makefile
> index cca7a58c5f29..6bd3a0590aa3 100644
> --- a/drivers/staging/r8188eu/Makefile
> +++ b/drivers/staging/r8188eu/Makefile
> @@ -4,8 +4,6 @@ EXTRA_CFLAGS += -O1
>
>  ccflags-y += -D__CHECK_ENDIAN__
>
> -CONFIG_USB_HCI = y
> -
>  CONFIG_BT_COEXIST = n
>  CONFIG_WOWLAN = n
>
> --
> 2.20.1
>

Dear Martin,

Looks good.

Acked-by: Phillip Potter <phil@philpotter.co.uk>

Regards,
Phil

Re: [PATCH 3/5] staging: r8188eu: (trivial) remove a duplicate debug print

[Dan Carpenter]

On Thu, Aug 12, 2021 at 12:53:16AM +0100, Phillip Potter wrote:
> On Wed, 11 Aug 2021 at 21:15, Martin Kaiser <martin@kaiser.cx> wrote:
> >
> > Keep the one that shows the wakeup capability.
> >

Please think of the subject and the commit message as two different
things.  Often it's people reviewing on email will only read one or the
other.  In other words just restate the subject:

  These two debug messages have the same information.  Delete the less
  informative one and keep the other.


> Dear Martin,
> 
> Just my personal opinion, but I'd be inclined to strip out all DBG_88E
> calls totally. If there are necessary functions being called such as
> device_may_wakeup() we can always just keep this part and remove the
> macro call (not checked this function out myself yet). Thanks.
> 

Yeah.  I agree.  The DBG_88E() doesn't call device_may_wakeup() unless
the module is loaded with a high enough value "rtw_debug" module option
so hopefully device_may_wakeup() does not have side effects.  (It does
not).

Thanks for reviewing these patches.  I do think it's nice to have
positive reviews instead of just me complaining and pointing out the
negative things.  We are trying to get code merged, not trying to put up
roadblocks so maybe encouraging more Acked-by reviews is a good thing...

regards,
dan carpenter

Re: [PATCH 3/5] staging: r8188eu: (trivial) remove a duplicate debug print

[Martin Kaiser]

Hi Dan and Phil,

Thus wrote Dan Carpenter (dan.carpenter@oracle.com):

> Please think of the subject and the commit message as two different
> things.  Often it's people reviewing on email will only read one or the
> other.  In other words just restate the subject:

ok, I'll keep that in mind for further patches.

> > Dear Martin,

> > Just my personal opinion, but I'd be inclined to strip out all DBG_88E
> > calls totally. If there are necessary functions being called such as
> > device_may_wakeup() we can always just keep this part and remove the
> > macro call (not checked this function out myself yet). Thanks.

I'd agree with you, Phil. Most DBG_88E prints don't say anything useful.

This comment from Greg made me drop the DBG_88E removal for now

https://lore.kernel.org/linux-staging/20210803201511.29000-1-martin@kaiser.cx/T/#m05d82a0ca8ed36180ebdc987114b4d892445c52d

A compromise would be to remove only those DBG_88E prints which are
really not helpful.

Best regards,
Martin

Re: [PATCH 3/5] staging: r8188eu: (trivial) remove a duplicate debug print

[Fabio M. De Francesco]

On Friday, August 13, 2021 12:05:36 PM CEST Martin Kaiser wrote:
> Hi Dan and Phil,
> 
> Thus wrote Dan Carpenter (dan.carpenter@oracle.com):
> > Please think of the subject and the commit message as two different
> > things.  Often it's people reviewing on email will only read one or the
> 
> > other.  In other words just restate the subject:
> OK, I'll keep that in mind for further patches.
> 
> > > Dear Martin,
> > > 
> > > Just my personal opinion, but I'd be inclined to strip out all DBG_88E
> > > calls totally. If there are necessary functions being called such as
> > > device_may_wakeup() we can always just keep this part and remove the
> > > macro call (not checked this function out myself yet). Thanks.
> 
> I'd agree with you, Phil. Most DBG_88E prints don't say anything useful.
> 
> This comment from Greg made me drop the DBG_88E removal for now
> 
> https://lore.kernel.org/linux-staging/20210803201511.29000-1-martin@kaiser.cx/T/#m05d82a
> 0ca8ed36180ebdc987114b4d892445c52d
> 
Hi Martin,

I think you misunderstood what Greg was trying to convey with the above-
mentioned message.

Well, he doesn't like to feed developers with little spoons :-)

I'm pretty sure that, by "Why not use the proper debugging calls instead of 
just deleting them?", he meant you should research, understand, and use the 
proper APIs for printing debug messages.

Please check out pr_debug(), dev_dbg(), netdev_dbg(). Use them appropriately, 
according to the subsystem you're working in and to the different types of 
arguments they take.

Thanks,

Fabio
>
> A compromise would be to remove only those DBG_88E prints which are
> really not helpful.
> 
> Best regards,
> Martin

Re: [PATCH 3/5] staging: r8188eu: (trivial) remove a duplicate debug print

[Phillip Potter]

On Fri, 13 Aug 2021 at 13:42, Fabio M. De Francesco
<fmdefrancesco@gmail.com> wrote:
>
> On Friday, August 13, 2021 12:05:36 PM CEST Martin Kaiser wrote:
> > Hi Dan and Phil,
> >
> > Thus wrote Dan Carpenter (dan.carpenter@oracle.com):
> > > Please think of the subject and the commit message as two different
> > > things.  Often it's people reviewing on email will only read one or the
> >
> > > other.  In other words just restate the subject:
> > OK, I'll keep that in mind for further patches.
> >
> > > > Dear Martin,
> > > >
> > > > Just my personal opinion, but I'd be inclined to strip out all DBG_88E
> > > > calls totally. If there are necessary functions being called such as
> > > > device_may_wakeup() we can always just keep this part and remove the
> > > > macro call (not checked this function out myself yet). Thanks.
> >
> > I'd agree with you, Phil. Most DBG_88E prints don't say anything useful.
> >
> > This comment from Greg made me drop the DBG_88E removal for now
> >
> > https://lore.kernel.org/linux-staging/20210803201511.29000-1-martin@kaiser.cx/T/#m05d82a
> > 0ca8ed36180ebdc987114b4d892445c52d
> >
> Hi Martin,
>
> I think you misunderstood what Greg was trying to convey with the above-
> mentioned message.
>
> Well, he doesn't like to feed developers with little spoons :-)
>
> I'm pretty sure that, by "Why not use the proper debugging calls instead of
> just deleting them?", he meant you should research, understand, and use the
> proper APIs for printing debug messages.
>
> Please check out pr_debug(), dev_dbg(), netdev_dbg(). Use them appropriately,
> according to the subsystem you're working in and to the different types of
> arguments they take.
>
> Thanks,
>
> Fabio
> >
> > A compromise would be to remove only those DBG_88E prints which are
> > really not helpful.
> >
> > Best regards,
> > Martin
>
>
>
>

The problem I see is that this driver is so littered with unnecessary
macro calls, how do we decide which ones to keep? In my mind, the
better option is to remove them all and then come up with some new
ones in the vein of netdev_dbg() and friends. I could be wrong of
course :-) I tried going down the route of keeping/converting some to
proper calls such as netdev_dbg() and the issue is a lot of the calls
don't have an obvious value anyway.

Regards,
Phil

Re: [PATCH 3/5] staging: r8188eu: (trivial) remove a duplicate debug print

[Phillip Potter]

On Fri, 13 Aug 2021 at 13:42, Fabio M. De Francesco
<fmdefrancesco@gmail.com> wrote:
>
> On Friday, August 13, 2021 12:05:36 PM CEST Martin Kaiser wrote:
> > Hi Dan and Phil,
> >
> > Thus wrote Dan Carpenter (dan.carpenter@oracle.com):
> > > Please think of the subject and the commit message as two different
> > > things.  Often it's people reviewing on email will only read one or the
> >
> > > other.  In other words just restate the subject:
> > OK, I'll keep that in mind for further patches.
> >
> > > > Dear Martin,
> > > >
> > > > Just my personal opinion, but I'd be inclined to strip out all DBG_88E
> > > > calls totally. If there are necessary functions being called such as
> > > > device_may_wakeup() we can always just keep this part and remove the
> > > > macro call (not checked this function out myself yet). Thanks.
> >
> > I'd agree with you, Phil. Most DBG_88E prints don't say anything useful.
> >
> > This comment from Greg made me drop the DBG_88E removal for now
> >
> > https://lore.kernel.org/linux-staging/20210803201511.29000-1-martin@kaiser.cx/T/#m05d82a
> > 0ca8ed36180ebdc987114b4d892445c52d
> >
> Hi Martin,
>
> I think you misunderstood what Greg was trying to convey with the above-
> mentioned message.
>
> Well, he doesn't like to feed developers with little spoons :-)
>
> I'm pretty sure that, by "Why not use the proper debugging calls instead of
> just deleting them?", he meant you should research, understand, and use the
> proper APIs for printing debug messages.
>
> Please check out pr_debug(), dev_dbg(), netdev_dbg(). Use them appropriately,
> according to the subsystem you're working in and to the different types of
> arguments they take.
>
> Thanks,
>
> Fabio
> >
> > A compromise would be to remove only those DBG_88E prints which are
> > really not helpful.
> >
> > Best regards,
> > Martin
>
>
>
>

The problem I see is that this driver is so littered with unnecessary
macro calls, how do we decide which ones to keep? In my mind, the
better option is to remove them all and then come up with some new
ones in the vein of netdev_dbg() and friends. I could be wrong of
course :-) I tried going down the route of keeping/converting some to
proper calls such as netdev_dbg() and the issue is a lot of the calls
don't have an obvious value anyway.

Regards,
Phil

Re: [PATCH 3/5] staging: r8188eu: (trivial) remove a duplicate debug print

[Fabio M. De Francesco]

On Saturday, August 14, 2021 6:54:40 PM CEST Phillip Potter wrote:
> On Fri, 13 Aug 2021 at 13:42, Fabio M. De Francesco
> 
> <fmdefrancesco@gmail.com> wrote:
> > On Friday, August 13, 2021 12:05:36 PM CEST Martin Kaiser wrote:
> > > Hi Dan and Phil,
> > > [...] 
> > > > > Just my personal opinion, but I'd be inclined to strip out all 
DBG_88E
> > > > > calls totally. If there are necessary functions being called such as
> > > > > device_may_wakeup() we can always just keep this part and remove the
> > > > > macro call (not checked this function out myself yet). Thanks.
> > > 
> > > I'd agree with you, Phil. Most DBG_88E prints don't say anything useful.
> > > 
> > > This comment from Greg made me drop the DBG_88E removal for now
> > > 
> > > https://lore.kernel.org/linux-staging/20210803201511.29000-1-martin@kaiser.cx/T/#m05
> > > d82a 0ca8ed36180ebdc987114b4d892445c52d
> > 
> > Hi Martin,
> > 
> > I think you misunderstood what Greg was trying to convey with the above-
> > mentioned message.
> > 
> > Well, he doesn't like to feed developers with little spoons :-)
> > 
> > I'm pretty sure that, by "Why not use the proper debugging calls instead 
of
> > just deleting them?", he meant you should research, understand, and use 
the
> > proper APIs for printing debug messages.
> > 
> > Please check out pr_debug(), dev_dbg(), netdev_dbg(). Use them 
appropriately,
> > according to the subsystem you're working in and to the different types of
> > arguments they take.
> > 
> > Thanks,
> > 
> > Fabio
> > 
> > > A compromise would be to remove only those DBG_88E prints which are
> > > really not helpful.
> > > 
> > > Best regards,
> > > Martin
> 
> The problem I see is that this driver is so littered with unnecessary
> macro calls, how do we decide which ones to keep? In my mind, the
> better option is to remove them all and then come up with some new
> ones in the vein of netdev_dbg() and friends. I could be wrong of
> course :-) I tried going down the route of keeping/converting some to
> proper calls such as netdev_dbg() and the issue is a lot of the calls
> don't have an obvious value anyway.
> 
> Regards,
> Phil

I think that you'd better remove only the ones that "have no obvious value" 
and convert the others to using netdev_dbg(). Obviously, telling which have no 
value is at the discretion of whoever wants to carry on this work.

Regards,

Fabio