diff mbox series

[net-next] net: dsa: mv88e6xxx: cosmetic fix

Message ID 20210319143149.823-1-kabel@kernel.org (mailing list archive)
State Changes Requested
Delegated to: Netdev Maintainers
Headers show
Series [net-next] net: dsa: mv88e6xxx: cosmetic fix | expand

Checks

Context Check Description
netdev/cover_letter success Link
netdev/fixes_present success Link
netdev/patch_count success Link
netdev/tree_selection success Clearly marked for net-next
netdev/subject_prefix success Link
netdev/cc_maintainers fail 2 blamed authors not CCed: pavana.sharma@digi.com ashkan.boldaji@digi.com; 6 maintainers not CCed: andrew@lunn.ch olteanv@gmail.com vivien.didelot@gmail.com pavana.sharma@digi.com f.fainelli@gmail.com ashkan.boldaji@digi.com
netdev/source_inline success Was 0 now: 0
netdev/verify_signedoff success Link
netdev/module_param success Was 0 now: 0
netdev/build_32bit success Errors and warnings before: 0 this patch: 0
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/verify_fixes success Link
netdev/checkpatch warning WARNING: 'occurances' may be misspelled - perhaps 'occurrences'?
netdev/build_allmodconfig_warn success Errors and warnings before: 0 this patch: 0
netdev/header_inline success Link

Commit Message

Marek Behún March 19, 2021, 2:31 p.m. UTC
We know that the `lane == MV88E6393X_PORT0_LANE`, so we can pass `lane`
to mv88e6390_serdes_read() instead of MV88E6393X_PORT0_LANE.

All other occurances in this function are using the `lane` variable.

It seems I forgot to change it at this one place after refactoring.

Signed-off-by: Marek Behún <kabel@kernel.org>
Fixes: de776d0d316f7 ("net: dsa: mv88e6xxx: add support for ...")
---
 drivers/net/dsa/mv88e6xxx/serdes.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

Comments

Andrew Lunn March 19, 2021, 6:35 p.m. UTC | #1
On Fri, Mar 19, 2021 at 03:31:49PM +0100, Marek Behún wrote:
> We know that the `lane == MV88E6393X_PORT0_LANE`, so we can pass `lane`
> to mv88e6390_serdes_read() instead of MV88E6393X_PORT0_LANE.
> 
> All other occurances in this function are using the `lane` variable.
> 
> It seems I forgot to change it at this one place after refactoring.
> 
> Signed-off-by: Marek Behún <kabel@kernel.org>
> Fixes: de776d0d316f7 ("net: dsa: mv88e6xxx: add support for ...")

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew
Vladimir Oltean March 19, 2021, 6:58 p.m. UTC | #2
On Fri, Mar 19, 2021 at 03:31:49PM +0100, Marek Behún wrote:
> We know that the `lane == MV88E6393X_PORT0_LANE`, so we can pass `lane`
> to mv88e6390_serdes_read() instead of MV88E6393X_PORT0_LANE.
> 
> All other occurances in this function are using the `lane` variable.
> 
> It seems I forgot to change it at this one place after refactoring.
> 
> Signed-off-by: Marek Behún <kabel@kernel.org>
> Fixes: de776d0d316f7 ("net: dsa: mv88e6xxx: add support for ...")
> ---

Either do the Fixes tag according to the documented convention:
git show de776d0d316f7 --pretty=fixes

Fixes: de776d0d316f ("net: dsa: mv88e6xxx: add support for mv88e6393x family")

or even better, drop it.
Marek Behún March 19, 2021, 7:47 p.m. UTC | #3
On Fri, 19 Mar 2021 20:58:20 +0200
Vladimir Oltean <olteanv@gmail.com> wrote:

> On Fri, Mar 19, 2021 at 03:31:49PM +0100, Marek Behún wrote:
> > We know that the `lane == MV88E6393X_PORT0_LANE`, so we can pass `lane`
> > to mv88e6390_serdes_read() instead of MV88E6393X_PORT0_LANE.
> > 
> > All other occurances in this function are using the `lane` variable.
> > 
> > It seems I forgot to change it at this one place after refactoring.
> > 
> > Signed-off-by: Marek Behún <kabel@kernel.org>
> > Fixes: de776d0d316f7 ("net: dsa: mv88e6xxx: add support for ...")
> > ---  
> 
> Either do the Fixes tag according to the documented convention:
> git show de776d0d316f7 --pretty=fixes

THX, did not know about this.

> Fixes: de776d0d316f ("net: dsa: mv88e6xxx: add support for mv88e6393x family")
>
> or even better, drop it.

Why better to drop it?
Florian Fainelli March 19, 2021, 10:14 p.m. UTC | #4
On 3/19/2021 12:47 PM, Marek Behún wrote:
> On Fri, 19 Mar 2021 20:58:20 +0200
> Vladimir Oltean <olteanv@gmail.com> wrote:
> 
>> On Fri, Mar 19, 2021 at 03:31:49PM +0100, Marek Behún wrote:
>>> We know that the `lane == MV88E6393X_PORT0_LANE`, so we can pass `lane`
>>> to mv88e6390_serdes_read() instead of MV88E6393X_PORT0_LANE.
>>>
>>> All other occurances in this function are using the `lane` variable.
>>>
>>> It seems I forgot to change it at this one place after refactoring.
>>>
>>> Signed-off-by: Marek Behún <kabel@kernel.org>
>>> Fixes: de776d0d316f7 ("net: dsa: mv88e6xxx: add support for ...")
>>> ---  
>>
>> Either do the Fixes tag according to the documented convention:
>> git show de776d0d316f7 --pretty=fixes
> 
> THX, did not know about this.
> 
>> Fixes: de776d0d316f ("net: dsa: mv88e6xxx: add support for mv88e6393x family")
>>
>> or even better, drop it.
> 
> Why better to drop it?

To differentiate an essential/functional fix from a cosmetic fix. If all
cosmetic fixes got Fixes: tag that would get out of hands quickly.
Marek Behún March 19, 2021, 10:54 p.m. UTC | #5
On Fri, 19 Mar 2021 15:14:52 -0700
Florian Fainelli <f.fainelli@gmail.com> wrote:

> On 3/19/2021 12:47 PM, Marek Behún wrote:
> > On Fri, 19 Mar 2021 20:58:20 +0200
> > Vladimir Oltean <olteanv@gmail.com> wrote:
> >   
> >> On Fri, Mar 19, 2021 at 03:31:49PM +0100, Marek Behún wrote:  
> >>> We know that the `lane == MV88E6393X_PORT0_LANE`, so we can pass `lane`
> >>> to mv88e6390_serdes_read() instead of MV88E6393X_PORT0_LANE.
> >>>
> >>> All other occurances in this function are using the `lane` variable.
> >>>
> >>> It seems I forgot to change it at this one place after refactoring.
> >>>
> >>> Signed-off-by: Marek Behún <kabel@kernel.org>
> >>> Fixes: de776d0d316f7 ("net: dsa: mv88e6xxx: add support for ...")
> >>> ---    
> >>
> >> Either do the Fixes tag according to the documented convention:
> >> git show de776d0d316f7 --pretty=fixes  
> > 
> > THX, did not know about this.
> >   
> >> Fixes: de776d0d316f ("net: dsa: mv88e6xxx: add support for mv88e6393x family")
> >>
> >> or even better, drop it.  
> > 
> > Why better to drop it?  
> 
> To differentiate an essential/functional fix from a cosmetic fix. If all
> cosmetic fixes got Fixes: tag that would get out of hands quickly.

IMO in this case the Fixes tag is not necessary beacuse the base commit
is not in any stable kernel yet.

But if the base commit was in a stable kernel already, and this
cosmetic fix was sent into net-next / net, I think the Fixes tag should
be there, in order for it to get applied into stable releases so that
future fixes could be applied cleanly.

Or am I wrong? This is how I understand this whole system...

Marek
Florian Fainelli March 20, 2021, 12:46 a.m. UTC | #6
On 3/19/2021 3:54 PM, Marek Behún wrote:
> On Fri, 19 Mar 2021 15:14:52 -0700
> Florian Fainelli <f.fainelli@gmail.com> wrote:
> 
>> On 3/19/2021 12:47 PM, Marek Behún wrote:
>>> On Fri, 19 Mar 2021 20:58:20 +0200
>>> Vladimir Oltean <olteanv@gmail.com> wrote:
>>>   
>>>> On Fri, Mar 19, 2021 at 03:31:49PM +0100, Marek Behún wrote:  
>>>>> We know that the `lane == MV88E6393X_PORT0_LANE`, so we can pass `lane`
>>>>> to mv88e6390_serdes_read() instead of MV88E6393X_PORT0_LANE.
>>>>>
>>>>> All other occurances in this function are using the `lane` variable.
>>>>>
>>>>> It seems I forgot to change it at this one place after refactoring.
>>>>>
>>>>> Signed-off-by: Marek Behún <kabel@kernel.org>
>>>>> Fixes: de776d0d316f7 ("net: dsa: mv88e6xxx: add support for ...")
>>>>> ---    
>>>>
>>>> Either do the Fixes tag according to the documented convention:
>>>> git show de776d0d316f7 --pretty=fixes  
>>>
>>> THX, did not know about this.
>>>   
>>>> Fixes: de776d0d316f ("net: dsa: mv88e6xxx: add support for mv88e6393x family")
>>>>
>>>> or even better, drop it.  
>>>
>>> Why better to drop it?  
>>
>> To differentiate an essential/functional fix from a cosmetic fix. If all
>> cosmetic fixes got Fixes: tag that would get out of hands quickly.
> 
> IMO in this case the Fixes tag is not necessary beacuse the base commit
> is not in any stable kernel yet.

This is not necessarily an argument that I would use, even if the commit
you are fixing is only in net-next, when it is a functional, and the
emphasis on the functional aspect of the code, providing a Fixes: tag is
really nice as it allows people that do backports or else to identify
the commits as an ensemble.

> 
> But if the base commit was in a stable kernel already, and this
> cosmetic fix was sent into net-next / net, I think the Fixes tag should
> be there, in order for it to get applied into stable releases so that
> future fixes could be applied cleanly.
> 
> Or am I wrong? This is how I understand this whole system...

Your reasoning is not wrong, for cosmetic changes that do not result in
functional changes, I would say that the Fixes: is optional.
diff mbox series

Patch

diff --git a/drivers/net/dsa/mv88e6xxx/serdes.c b/drivers/net/dsa/mv88e6xxx/serdes.c
index 470856bcd2f3..f96c6ece4d75 100644
--- a/drivers/net/dsa/mv88e6xxx/serdes.c
+++ b/drivers/net/dsa/mv88e6xxx/serdes.c
@@ -1285,8 +1285,7 @@  static int mv88e6393x_serdes_port_errata(struct mv88e6xxx_chip *chip, int lane)
 	 * powered up (the bit is cleared), so power it down.
 	 */
 	if (lane == MV88E6393X_PORT0_LANE) {
-		err = mv88e6390_serdes_read(chip, MV88E6393X_PORT0_LANE,
-					    MDIO_MMD_PHYXS,
+		err = mv88e6390_serdes_read(chip, lane, MDIO_MMD_PHYXS,
 					    MV88E6393X_SERDES_POC, &reg);
 		if (err)
 			return err;