Message-ID: <qdwaM7uxN2Sm0zSi_JF5cMYkIhlMFdluiUK6bLU1XqiHDMaSnMcVpCOseEfvdJKu-3uRr_iAuEBMJPn6Qbix-FaNG0EF8n82K3E7lQ5Zg5M=@protonmail.com>
Date: Fri, 20 Sep 2024 13:37:23 +0000
From: Dominik Karol Piątkowski <dominik.karol.piatkowski@...tonmail.com>
To: Philipp Hortmann <philipp.g.hortmann@...il.com>
Cc: gregkh@...uxfoundation.org, tdavies@...kphysics.net, dan.carpenter@...aro.org, linux-staging@...ts.linux.dev, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] staging: rtl8192e: Fix alignment to open parentheses

Hi Philipp,

On Thursday, September 19th, 2024 at 22:15, Philipp Hortmann <philipp.g.hortmann@...il.com> wrote:
<cut>
> > diff --git a/drivers/staging/rtl8192e/rtl8192e/r8192E_dev.c b/drivers/staging/rtl8192e/rtl8192e/r8192E_dev.c
> > index 2672b1ddf88e..cf1231fe5319 100644
> > --- a/drivers/staging/rtl8192e/rtl8192e/r8192E_dev.c
> > +++ b/drivers/staging/rtl8192e/rtl8192e/r8192E_dev.c
<cut>
> > @@ -335,8 +334,9 @@ static void _rtl92e_read_eeprom_info(struct net_device *dev)
> > 
> > for (i = 0; i < 14; i += 2) {
> > if (!priv->autoload_fail_flag)
> > - usValue = rtl92e_eeprom_read(dev,
> > - (EEPROM_TxPwIndex_CCK + i) >> 1);
> > + usValue =
> > + rtl92e_eeprom_read(dev,
> > + (EEPROM_TxPwIndex_CCK + i) >> 1);
> 
> 
> Sorry this is not really increasing readability.

You are right. The problem with this line is that it has 5 levels of nesting,
and this makes it hard to fix the readability without having too long lines.

A some kind of rewrite could be helpful in the future, moving the deeply
nested code into its own functions. For now, I think it is not sanely fixable.

> 
> > else
> > usValue = EEPROM_Default_TxPower;
> > *((u16 *)(&priv->eeprom_tx_pwr_level_cck[i])) =
> > @@ -345,7 +345,8 @@ static void _rtl92e_read_eeprom_info(struct net_device *dev)
> > for (i = 0; i < 14; i += 2) {
> > if (!priv->autoload_fail_flag)
> > usValue = rtl92e_eeprom_read(dev,
> > - (EEPROM_TxPwIndex_OFDM_24G + i) >> 1);
> > + (EEPROM_TxPwIndex_OFDM_24G + i)
> > + >> 1);
> 
> 
> Sorry this is not really increasing readability. The >> 1 in the next
> 
> line is net nice.

I agree, this was a bad idea. This line is a similar case to the previous one,
with it also having 5 levels of nesting. For now, I think it is also not sanely
fixable.

> 
> > else
> > usValue = EEPROM_Default_TxPower;
> > *((u16 *)(&priv->eeprom_tx_pwr_level_ofdm24g[i]))
> > @@ -1325,8 +1326,8 @@ static void _rtl92e_query_rxphystatus(struct r8192_priv *priv,
> > } else {
> > if (rf_rx_num != 0)
> > pstats->signal_strength = precord_stats->signal_strength =
> > - _rtl92e_signal_scale_mapping(priv,
> > - (long)(total_rssi /= rf_rx_num));
> > + _rtl92e_signal_scale_mapping(priv, (long)
> > + (total_rssi /= rf_rx_num));
> 
> 
> I am not happy with this either. There are two = in the same line...

Right. The double assignment could be split into separate line, improving
readability, and probably allowing for nicer formatting.

<cut>
> > diff --git a/drivers/staging/rtl8192e/rtl8192e/rtl_core.c b/drivers/staging/rtl8192e/rtl8192e/rtl_core.c
> > index dc1301f1a0c1..82a1b19fa1b3 100644
> > --- a/drivers/staging/rtl8192e/rtl8192e/rtl_core.c
> > +++ b/drivers/staging/rtl8192e/rtl8192e/rtl_core.c
<cut>
> > @@ -406,8 +405,8 @@ static int _rtl92e_qos_assoc_resp(struct r8192_priv *priv,
> > }
> > 
> > static int _rtl92e_handle_assoc_response(struct net_device *dev,
> > - struct rtllib_assoc_response_frame *resp,
> > - struct rtllib_network *network)
> > + struct rtllib_assoc_response_frame *resp,
> > + struct rtllib_network *network)
> 
> 
> one space missing...

Is it missing? In my editor it looks okay.

<cut>
> > diff --git a/drivers/staging/rtl8192e/rtl819x_HTProc.c b/drivers/staging/rtl8192e/rtl819x_HTProc.c
> > index 9c9c0bc0cfde..ec43b5fda06e 100644
> > --- a/drivers/staging/rtl8192e/rtl819x_HTProc.c
> > +++ b/drivers/staging/rtl8192e/rtl819x_HTProc.c
<cut>
> > @@ -418,8 +416,8 @@ static u8 ht_filter_mcs_rate(struct rtllib_device *ieee, u8 *pSupportMCS,
> > }
> > 
> > void ht_set_connect_bw_mode(struct rtllib_device *ieee,
> > - enum ht_channel_width bandwidth,
> > - enum ht_extchnl_offset Offset);
> > + enum ht_channel_width bandwidth,
> > + enum ht_extchnl_offset Offset);
> 
> 
> This one is not correct.

Is it incorrect? In my editor it looks okay.

> 
> 
> Hi Dominik,
> 
> this patch is to long. 1200 Lines are long for a patch. It might work
> out when it can be checked automatically. But in this case I need go
> through it line by line.

Fair point - I will keep that in mind and try to keep the patches short in
the future.

> 
> Another issue is that I cannot apply it on top of the following patch
> series that I see likely to be accepted.
> https://lore.kernel.org/linux-staging/Zung-0ClV_527-_e@kernel-710/T/#t
> 
> Here a trick to ensure this is not happening. I would look into the
> coverletter of above mentioned patch series. There are only 8 files
> changed so there are plenty left untouched. You can work on them.

Thank you for making me aware of this. I will prepare a v2 fixing the files
that are untouched by the mentioned series.

> 
> My opinion is that people who are knew to the kernel community should
> start with simple patches and then evolve.
> 
> I propose to look for unused macros in:
> 
> r8192E_hw.h
> 
> This is the output of a program I wrote. You need to carefully check if
> they are really good for removal.
> 
> BCN_TCFG_CW_SHIFT
> BCN_TCFG_IFS
> IMR_BcnInt
> MSR_LINK_ADHOC
> MSR_LINK_MASTER
> RRSR_SHORT_OFFSET
> 
> You can find patches in git about the removal of the macros as an example.

Thank you, I will look into these macros.

Thanks,

Dominik Karol

> 
> Thanks for your support.
> 
> Bye Philipp

Powered by blists - more mailing lists