Hi Philipp,
On Thursday, September 19th, 2024 at 22:15, Philipp Hortmann <philipp.g.hortmann@xxxxxxxxx> 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
