Message ID | owrt-patch-20151205-1@vittgam.net |
---|---|
State | Rejected, archived |
Headers | show |
On 05/12/2015 12:03, Vittorio G (VittGam) wrote: > Also avoid using two variables where one can be used. > Hi, several issues 1) the subject and description dont match. 2) why would we want to intermingle the portmap and register init variables. they are 2 different things. imho that is not correct John > Signed-off-by: Vittorio Gambaletta <openwrt@vittgam.net> > > --- > > Please apply to both trunk and CC. > > --- a/target/linux/ramips/files/drivers/net/ethernet/ralink/esw_rt3052.c > +++ b/target/linux/ramips/files/drivers/net/ethernet/ralink/esw_rt3052.c > @@ -1373,7 +1373,7 @@ static int esw_probe(struct platform_device *pdev) > { > struct device_node *np = pdev->dev.of_node; > const struct rt305x_esw_platform_data *pdata; > - const __be32 *port_map, *reg_init; > + const __be32 *reg_init; > struct rt305x_esw *esw; > struct switch_dev *swdev; > struct resource *res, *irq; > @@ -1416,20 +1416,20 @@ static int esw_probe(struct platform_device *pdev) > goto free_esw; > } > > - port_map = of_get_property(np, "ralink,portmap", NULL); > - if (port_map) > - esw->port_map = be32_to_cpu(*port_map); > + reg_init = of_get_property(np, "ralink,portmap", NULL); > + if (reg_init) > + esw->port_map = be32_to_cpu(*reg_init); > > reg_init = of_get_property(np, "ralink,fct2", NULL); > - if (reg_init) > + if (reg_init) > esw->reg_initval_fct2 = be32_to_cpu(*reg_init); > > reg_init = of_get_property(np, "ralink,fpa2", NULL); > - if (reg_init) > + if (reg_init) > esw->reg_initval_fpa2 = be32_to_cpu(*reg_init); > > reg_init = of_get_property(np, "ralink,led_polarity", NULL); > - if (reg_init) > + if (reg_init) > esw->reg_led_polarity = be32_to_cpu(*reg_init); > > swdev = &esw->swdev; > _______________________________________________ > openwrt-devel mailing list > openwrt-devel@lists.openwrt.org > https://lists.openwrt.org/cgi-bin/mailman/listinfo/openwrt-devel >
Hi, On 11/12/2015 10:32:08 CET, John Crispin wrote: > 1) the subject and description dont match. Well, the description is an addition to the subject, but okay. > 2) why would we want to intermingle the portmap and register init > variables. they are 2 different things. imho that is not correct Because there are two __be32's used to fetch data from the device tree; one is used only once for the portmap, while the other is used three times, for the two registers and for the led_polarity. Maybe the name should be tmp instead, but I think that the variable should be just one here. Cheers, Vittorio G
On 11/12/2015 15:03, Vittorio G (VittGam) wrote: > > Because there are two __be32's used to fetch data from the device > tree; one is used only once for the portmap, while the other is > used three times, for the two registers and for the led_polarity. > Maybe the name should be tmp instead, but I think that the variable > should be just one here. yeah if we use a more generic name we can use one variable. that sounds like a good plan.
Citeren "Vittorio G (VittGam)" <openwrt@vittgam.net>: > Hi, > > On 11/12/2015 10:32:08 CET, John Crispin wrote: >> 1) the subject and description dont match. > > Well, the description is an addition to the subject, but okay. > >> 2) why would we want to intermingle the portmap and register init >> variables. they are 2 different things. imho that is not correct > > Because there are two __be32's used to fetch data from the device > tree; one is used only once for the portmap, while the other is > used three times, for the two registers and for the led_polarity. > Maybe the name should be tmp instead, but I think that the variable > should be just one here. You're trying to outsmart the compiler in order to save space? Any compiler from the last decade will take notice of the scope in which variables are used and will reuse stack (or register) space if the value they hold is no longer needed/used. I'm with John here, clarity in the naming of variables is much more important.
Hi, On 11/12/2015 15:12:32 CET, Arjen de Korte wrote: >> Because there are two __be32's used to fetch data from the device >> tree; one is used only once for the portmap, while the other is >> used three times, for the two registers and for the led_polarity. >> Maybe the name should be tmp instead, but I think that the variable >> should be just one here. > > You're trying to outsmart the compiler in order to save space? Any compiler from the last decade will take notice of the scope in which variables are used and will reuse stack (or register) space if the value they hold is no longer needed/used. I'm with John here, clarity in the naming of variables is much more important. Well, if the single "tmp" approach is not good, the current approach that uses reg_init even for the led_polarity is not good either... Cheers, Vittorio G
--- a/target/linux/ramips/files/drivers/net/ethernet/ralink/esw_rt3052.c +++ b/target/linux/ramips/files/drivers/net/ethernet/ralink/esw_rt3052.c @@ -1373,7 +1373,7 @@ static int esw_probe(struct platform_device *pdev) { struct device_node *np = pdev->dev.of_node; const struct rt305x_esw_platform_data *pdata; - const __be32 *port_map, *reg_init; + const __be32 *reg_init; struct rt305x_esw *esw; struct switch_dev *swdev; struct resource *res, *irq; @@ -1416,20 +1416,20 @@ static int esw_probe(struct platform_device *pdev) goto free_esw; } - port_map = of_get_property(np, "ralink,portmap", NULL); - if (port_map) - esw->port_map = be32_to_cpu(*port_map); + reg_init = of_get_property(np, "ralink,portmap", NULL); + if (reg_init) + esw->port_map = be32_to_cpu(*reg_init); reg_init = of_get_property(np, "ralink,fct2", NULL); - if (reg_init) + if (reg_init) esw->reg_initval_fct2 = be32_to_cpu(*reg_init); reg_init = of_get_property(np, "ralink,fpa2", NULL); - if (reg_init) + if (reg_init) esw->reg_initval_fpa2 = be32_to_cpu(*reg_init); reg_init = of_get_property(np, "ralink,led_polarity", NULL); - if (reg_init) + if (reg_init) esw->reg_led_polarity = be32_to_cpu(*reg_init); swdev = &esw->swdev;
Also avoid using two variables where one can be used. Signed-off-by: Vittorio Gambaletta <openwrt@vittgam.net> --- Please apply to both trunk and CC.