Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1699955 > unrolled thread

[PATCH net-next 0/2] Refactor lan9303_xxx_packet_processing

Back to article view | Back to linux.kernel

Contents

  [PATCH net-next 0/2] Refactor lan9303_xxx_packet_processing Egil Hjelmeland <privat@egil-hjelmeland.no> - 2017-07-31 13:40 +0200
    [PATCH net-next 2/2] net: dsa: lan9303: Simplify lan9303_xxx_packet_processing() usage Egil Hjelmeland <privat@egil-hjelmeland.no> - 2017-07-31 13:40 +0200
      Re: [PATCH net-next 2/2] net: dsa: lan9303: Simplify  lan9303_xxx_packet_processing() usage Andrew Lunn <andrew@lunn.ch> - 2017-07-31 15:50 +0200
        Re: [PATCH net-next 2/2] net: dsa: lan9303: Simplify  lan9303_xxx_packet_processing() usage Egil Hjelmeland <privat@egil-hjelmeland.no> - 2017-07-31 16:20 +0200
          Re: [PATCH net-next 2/2] net: dsa: lan9303: Simplify  lan9303_xxx_packet_processing() usage Andrew Lunn <andrew@lunn.ch> - 2017-07-31 16:50 +0200
      Re: [PATCH net-next 2/2] net: dsa: lan9303: Simplify lan9303_xxx_packet_processing() usage Vivien Didelot <vivien.didelot@savoirfairelinux.com> - 2017-07-31 16:10 +0200
        Re: [PATCH net-next 2/2] net: dsa: lan9303: Simplify  lan9303_xxx_packet_processing() usage Egil Hjelmeland <privat@egil-hjelmeland.no> - 2017-07-31 16:40 +0200
          Re: [PATCH net-next 2/2] net: dsa: lan9303: Simplify lan9303_xxx_packet_processing() usage Vivien Didelot <vivien.didelot@savoirfairelinux.com> - 2017-07-31 16:50 +0200
            Re: [PATCH net-next 2/2] net: dsa: lan9303: Simplify  lan9303_xxx_packet_processing() usage Egil Hjelmeland <privat@egil-hjelmeland.no> - 2017-07-31 17:00 +0200
              Re: [PATCH net-next 2/2] net: dsa: lan9303: Simplify  lan9303_xxx_packet_processing() usage Egil Hjelmeland <privat@egil-hjelmeland.no> - 2017-07-31 17:10 +0200
                Re: [PATCH net-next 2/2] net: dsa: lan9303: Simplify lan9303_xxx_packet_processing() usage Vivien Didelot <vivien.didelot@savoirfairelinux.com> - 2017-07-31 17:20 +0200
              Re: [PATCH net-next 2/2] net: dsa: lan9303: Simplify lan9303_xxx_packet_processing() usage Vivien Didelot <vivien.didelot@savoirfairelinux.com> - 2017-07-31 17:10 +0200
    Re: [PATCH net-next 0/2] Refactor lan9303_xxx_packet_processing Andrew Lunn <andrew@lunn.ch> - 2017-07-31 15:30 +0200
      Re: [PATCH net-next 0/2] Refactor lan9303_xxx_packet_processing Egil Hjelmeland <privat@egil-hjelmeland.no> - 2017-07-31 15:40 +0200

#1699955 — [PATCH net-next 0/2] Refactor lan9303_xxx_packet_processing

First patch: 
Change lan9303_enable_packet_processing,
lan9303_disable_packet_processing():
Pass port number (0,1,2) as parameter instead of port offset.
Plus replaced a constant 0x400 with LAN9303_SWITCH_PORT_REG()

Second patch: Simplify accordingly.

Comments welcome!

Egil Hjelmeland (2):
  net: dsa: lan9303: Refactor lan9303_xxx_packet_processing()
  net: dsa: lan9303: Simplify lan9303_xxx_packet_processing() usage

 drivers/net/dsa/lan9303-core.c | 73 ++++++++++++++++++++----------------------
 1 file changed, 35 insertions(+), 38 deletions(-)

-- 
2.11.0

[toc] | [next] | [standalone]

#1699961 — [PATCH net-next 2/2] net: dsa: lan9303: Simplify lan9303_xxx_packet_processing() usage

Simplify usage of lan9303_enable_packet_processing,
lan9303_disable_packet_processing()

Signed-off-by: Egil Hjelmeland <privat@egil-hjelmeland.no>
---
 drivers/net/dsa/lan9303-core.c | 24 ++++++++++--------------
 1 file changed, 10 insertions(+), 14 deletions(-)

diff --git a/drivers/net/dsa/lan9303-core.c b/drivers/net/dsa/lan9303-core.c
index 4d2bb8144c15..705267a1d2ba 100644
--- a/drivers/net/dsa/lan9303-core.c
+++ b/drivers/net/dsa/lan9303-core.c
@@ -559,15 +559,16 @@ static int lan9303_handle_reset(struct lan9303 *chip)
 /* stop processing packets for all ports */
 static int lan9303_disable_processing(struct lan9303 *chip)
 {
-	int ret;
+	int p;
 
-	ret = lan9303_disable_packet_processing(chip, 0);
-	if (ret)
-		return ret;
-	ret = lan9303_disable_packet_processing(chip, 1);
-	if (ret)
-		return ret;
-	return lan9303_disable_packet_processing(chip, 2);
+	for (p = 0; p <= 2; p++) {
+		int ret;
+
+		ret = lan9303_disable_packet_processing(chip, p);
+		if (ret)
+			return ret;
+	}
+	return 0;
 }
 
 static int lan9303_check_device(struct lan9303 *chip)
@@ -760,7 +761,6 @@ static int lan9303_port_enable(struct dsa_switch *ds, int port,
 	/* enable internal packet processing */
 	switch (port) {
 	case 1:
-		return lan9303_enable_packet_processing(chip, port);
 	case 2:
 		return lan9303_enable_packet_processing(chip, port);
 	default:
@@ -779,13 +779,9 @@ static void lan9303_port_disable(struct dsa_switch *ds, int port,
 	/* disable internal packet processing */
 	switch (port) {
 	case 1:
-		lan9303_disable_packet_processing(chip, port);
-		lan9303_phy_write(ds, chip->phy_addr_sel_strap + 1,
-				  MII_BMCR, BMCR_PDOWN);
-		break;
 	case 2:
 		lan9303_disable_packet_processing(chip, port);
-		lan9303_phy_write(ds, chip->phy_addr_sel_strap + 2,
+		lan9303_phy_write(ds, chip->phy_addr_sel_strap + port,
 				  MII_BMCR, BMCR_PDOWN);
 		break;
 	default:
-- 
2.11.0

[toc] | [prev] | [next] | [standalone]

#1700063 — Re: [PATCH net-next 2/2] net: dsa: lan9303: Simplify lan9303_xxx_packet_processing() usage

On Mon, Jul 31, 2017 at 01:33:55PM +0200, Egil Hjelmeland wrote:
> Simplify usage of lan9303_enable_packet_processing,
> lan9303_disable_packet_processing()
> 
> Signed-off-by: Egil Hjelmeland <privat@egil-hjelmeland.no>
> ---
>  drivers/net/dsa/lan9303-core.c | 24 ++++++++++--------------
>  1 file changed, 10 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/net/dsa/lan9303-core.c b/drivers/net/dsa/lan9303-core.c
> index 4d2bb8144c15..705267a1d2ba 100644
> --- a/drivers/net/dsa/lan9303-core.c
> +++ b/drivers/net/dsa/lan9303-core.c
> @@ -559,15 +559,16 @@ static int lan9303_handle_reset(struct lan9303 *chip)
>  /* stop processing packets for all ports */
>  static int lan9303_disable_processing(struct lan9303 *chip)
>  {
> -	int ret;
> +	int p;
>  
> -	ret = lan9303_disable_packet_processing(chip, 0);
> -	if (ret)
> -		return ret;
> -	ret = lan9303_disable_packet_processing(chip, 1);
> -	if (ret)
> -		return ret;
> -	return lan9303_disable_packet_processing(chip, 2);
> +	for (p = 0; p <= 2; p++) {
> +		int ret;
> +
> +		ret = lan9303_disable_packet_processing(chip, p);
> +		if (ret)
> +			return ret;
> +	}
> +	return 0;
>  }
>  
>  static int lan9303_check_device(struct lan9303 *chip)
> @@ -760,7 +761,6 @@ static int lan9303_port_enable(struct dsa_switch *ds, int port,
>  	/* enable internal packet processing */
>  	switch (port) {
>  	case 1:
> -		return lan9303_enable_packet_processing(chip, port);
>  	case 2:
>  		return lan9303_enable_packet_processing(chip, port);
>  	default:
> @@ -779,13 +779,9 @@ static void lan9303_port_disable(struct dsa_switch *ds, int port,
>  	/* disable internal packet processing */
>  	switch (port) {
>  	case 1:
> -		lan9303_disable_packet_processing(chip, port);
> -		lan9303_phy_write(ds, chip->phy_addr_sel_strap + 1,
> -				  MII_BMCR, BMCR_PDOWN);
> -		break;
>  	case 2:
>  		lan9303_disable_packet_processing(chip, port);
> -		lan9303_phy_write(ds, chip->phy_addr_sel_strap + 2,
> +		lan9303_phy_write(ds, chip->phy_addr_sel_strap + port,
>  				  MII_BMCR, BMCR_PDOWN);
>  		break;
>  	default:

:-)

So maybe i would squash this part into the previous patch. You were
touching the functions anyway, and the change is obvious, so easy to
review.

But it is also O.K. this way. The cover note could of helped. You
could of said something like: "Changes made in the first patch allow
some simplifications to be made in the same code in the second patch.

Breaking changes up is hard, and you cannot please everybody, all the
time.

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

    Andrew

[toc] | [prev] | [next] | [standalone]

#1700084 — Re: [PATCH net-next 2/2] net: dsa: lan9303: Simplify lan9303_xxx_packet_processing() usage

On 31. juli 2017 15:46, Andrew Lunn wrote:
> On Mon, Jul 31, 2017 at 01:33:55PM +0200, Egil Hjelmeland wrote:
>> Simplify usage of lan9303_enable_packet_processing,
>> lan9303_disable_packet_processing()
>>
>> Signed-off-by: Egil Hjelmeland <privat@egil-hjelmeland.no>
>> ---
>>   drivers/net/dsa/lan9303-core.c | 24 ++++++++++--------------
>>   1 file changed, 10 insertions(+), 14 deletions(-)
>>
>> diff --git a/drivers/net/dsa/lan9303-core.c b/drivers/net/dsa/lan9303-core.c
>> index 4d2bb8144c15..705267a1d2ba 100644
>> --- a/drivers/net/dsa/lan9303-core.c
>> +++ b/drivers/net/dsa/lan9303-core.c
>> @@ -559,15 +559,16 @@ static int lan9303_handle_reset(struct lan9303 *chip)
>>   /* stop processing packets for all ports */
>>   static int lan9303_disable_processing(struct lan9303 *chip)
>>   {
>> -	int ret;
>> +	int p;
>>   
>> -	ret = lan9303_disable_packet_processing(chip, 0);
>> -	if (ret)
>> -		return ret;
>> -	ret = lan9303_disable_packet_processing(chip, 1);
>> -	if (ret)
>> -		return ret;
>> -	return lan9303_disable_packet_processing(chip, 2);
>> +	for (p = 0; p <= 2; p++) {
>> +		int ret;
>> +
>> +		ret = lan9303_disable_packet_processing(chip, p);
>> +		if (ret)
>> +			return ret;
>> +	}
>> +	return 0;
>>   }
>>   
>>   static int lan9303_check_device(struct lan9303 *chip)
>> @@ -760,7 +761,6 @@ static int lan9303_port_enable(struct dsa_switch *ds, int port,
>>   	/* enable internal packet processing */
>>   	switch (port) {
>>   	case 1:
>> -		return lan9303_enable_packet_processing(chip, port);
>>   	case 2:
>>   		return lan9303_enable_packet_processing(chip, port);
>>   	default:
>> @@ -779,13 +779,9 @@ static void lan9303_port_disable(struct dsa_switch *ds, int port,
>>   	/* disable internal packet processing */
>>   	switch (port) {
>>   	case 1:
>> -		lan9303_disable_packet_processing(chip, port);
>> -		lan9303_phy_write(ds, chip->phy_addr_sel_strap + 1,
>> -				  MII_BMCR, BMCR_PDOWN);
>> -		break;
>>   	case 2:
>>   		lan9303_disable_packet_processing(chip, port);
>> -		lan9303_phy_write(ds, chip->phy_addr_sel_strap + 2,
>> +		lan9303_phy_write(ds, chip->phy_addr_sel_strap + port,
>>   				  MII_BMCR, BMCR_PDOWN);
>>   		break;
>>   	default:
> 
> :-)
> 
> So maybe i would squash this part into the previous patch. You were
> touching the functions anyway, and the change is obvious, so easy to
> review.
> 
> But it is also O.K. this way. The cover note could of helped. You
> could of said something like: "Changes made in the first patch allow
> some simplifications to be made in the same code in the second patch.
> 
> Breaking changes up is hard, and you cannot please everybody, all the
> time.
> 
> Reviewed-by: Andrew Lunn <andrew@lunn.ch>
> 
>      Andrew
> 

Hi Andrew

This time I took the extra effort to split my  original patch...

Your lan9303_write_switch_port suggestion (in previous reply) is fine.
And I can improve the coverletter.

So I will do a v2 of the patch. But what is your advice:
Should I squash the patch?

Egil

[toc] | [prev] | [next] | [standalone]

#1700094 — Re: [PATCH net-next 2/2] net: dsa: lan9303: Simplify lan9303_xxx_packet_processing() usage

> >Reviewed-by: Andrew Lunn <andrew@lunn.ch>
> >
> >     Andrew
> >
> 
> Hi Andrew
> 
> This time I took the extra effort to split my  original patch...
> 
> Your lan9303_write_switch_port suggestion (in previous reply) is fine.
> And I can improve the coverletter.
> 
> So I will do a v2 of the patch. But what is your advice:
> Should I squash the patch?

I already gave my Reviewed-by:, meaning i don't really care. Merged or
squashed is a minor point in this case.

	 Andrew

[toc] | [prev] | [next] | [standalone]

#1700072 — Re: [PATCH net-next 2/2] net: dsa: lan9303: Simplify lan9303_xxx_packet_processing() usage

Hi Egil,

A few nitpicks below for lan9303_disable_processing.

Egil Hjelmeland <privat@egil-hjelmeland.no> writes:

>  static int lan9303_disable_processing(struct lan9303 *chip)
>  {
> -	int ret;
> +	int p;
>  
> -	ret = lan9303_disable_packet_processing(chip, 0);
> -	if (ret)
> -		return ret;
> -	ret = lan9303_disable_packet_processing(chip, 1);
> -	if (ret)
> -		return ret;
> -	return lan9303_disable_packet_processing(chip, 2);
> +	for (p = 0; p <= 2; p++) {

Exclusive limits are often prefer, i.e. 'p < 3'.

> +		int ret;
> +
> +		ret = lan9303_disable_packet_processing(chip, p);
> +		if (ret)
> +			return ret;

When any non-zero return code means an error, we usually see 'err'
instead of 'ret'.

> +	}

blank line before return statments are appreciated.

> +	return 0;
>  }
>  
>  static int lan9303_check_device(struct lan9303 *chip)
> @@ -760,7 +761,6 @@ static int lan9303_port_enable(struct dsa_switch *ds, int port,
>  	/* enable internal packet processing */
>  	switch (port) {
>  	case 1:
> -		return lan9303_enable_packet_processing(chip, port);

Is this deletion intentional? The commit message does not explain this.

When possible, it is appreciated to separate functional from
non-functional changes. For example one commit adding the loop in
lan9303_disable_processing and another one to not enable/disable packet
processing on port 1.

>  	case 2:
>  		return lan9303_enable_packet_processing(chip, port);
>  	default:
> @@ -779,13 +779,9 @@ static void lan9303_port_disable(struct dsa_switch *ds, int port,
>  	/* disable internal packet processing */
>  	switch (port) {
>  	case 1:
> -		lan9303_disable_packet_processing(chip, port);
> -		lan9303_phy_write(ds, chip->phy_addr_sel_strap + 1,
> -				  MII_BMCR, BMCR_PDOWN);
> -		break;
>  	case 2:
>  		lan9303_disable_packet_processing(chip, port);
> -		lan9303_phy_write(ds, chip->phy_addr_sel_strap + 2,
> +		lan9303_phy_write(ds, chip->phy_addr_sel_strap + port,
>  				  MII_BMCR, BMCR_PDOWN);
>  		break;

Thanks,

        Vivien

[toc] | [prev] | [next] | [standalone]

#1700092 — Re: [PATCH net-next 2/2] net: dsa: lan9303: Simplify lan9303_xxx_packet_processing() usage

On 31. juli 2017 16:00, Vivien Didelot wrote:
> Hi Egil,
> 
> A few nitpicks below for lan9303_disable_processing.
> 
> Egil Hjelmeland <privat@egil-hjelmeland.no> writes:
> 
>>   static int lan9303_disable_processing(struct lan9303 *chip)
>>   {
>> -	int ret;
>> +	int p;
>>   
>> -	ret = lan9303_disable_packet_processing(chip, 0);
>> -	if (ret)
>> -		return ret;
>> -	ret = lan9303_disable_packet_processing(chip, 1);
>> -	if (ret)
>> -		return ret;
>> -	return lan9303_disable_packet_processing(chip, 2);
>> +	for (p = 0; p <= 2; p++) {
> 
> Exclusive limits are often prefer, i.e. 'p < 3'.
> 
OK, that can be nice when I later introduce LAN9303_NUM_PORTS = 3.

>> +		int ret;
>> +
>> +		ret = lan9303_disable_packet_processing(chip, p);
>> +		if (ret)
>> +			return ret;
> 
> When any non-zero return code means an error, we usually see 'err'
> instead of 'ret'.
> 

But 'ret' is used throughout the rest of the file. Is it not better to
be locally consistent?

>> +	}
> 
> blank line before return statments are appreciated.
> 
OK

>> +	return 0;
>>   }
>>   
>>   static int lan9303_check_device(struct lan9303 *chip)
>> @@ -760,7 +761,6 @@ static int lan9303_port_enable(struct dsa_switch *ds, int port,
>>   	/* enable internal packet processing */
>>   	switch (port) {
>>   	case 1:
>> -		return lan9303_enable_packet_processing(chip, port);
> 
> Is this deletion intentional? The commit message does not explain this.
> 
> When possible, it is appreciated to separate functional from
> non-functional changes. For example one commit adding the loop in
> lan9303_disable_processing and another one to not enable/disable packet
> processing on port 1.
> 

Case fall through, the change is purely non-functional.

You are perhaps thinking of the patch in my first series where I removed
disable of port 0. I have put that on hold. Juergen says that the
mainline driver works out of the box for him. So I will investigate
that problem bit more.


>>   	case 2:
>>   		return lan9303_enable_packet_processing(chip, port);
>>   	default:
>> @@ -779,13 +779,9 @@ static void lan9303_port_disable(struct dsa_switch *ds, int port,
>>   	/* disable internal packet processing */
>>   	switch (port) {
>>   	case 1:
>> -		lan9303_disable_packet_processing(chip, port);
>> -		lan9303_phy_write(ds, chip->phy_addr_sel_strap + 1,
>> -				  MII_BMCR, BMCR_PDOWN);
>> -		break;
>>   	case 2:
>>   		lan9303_disable_packet_processing(chip, port);
>> -		lan9303_phy_write(ds, chip->phy_addr_sel_strap + 2,
>> +		lan9303_phy_write(ds, chip->phy_addr_sel_strap + port,
>>   				  MII_BMCR, BMCR_PDOWN);
>>   		break;
> 
> Thanks,
> 
>          Vivien
> 
Egil

[toc] | [prev] | [next] | [standalone]

#1700096 — Re: [PATCH net-next 2/2] net: dsa: lan9303: Simplify lan9303_xxx_packet_processing() usage

Hi Egil,

Egil Hjelmeland <privat@egil-hjelmeland.no> writes:

>>> +	for (p = 0; p <= 2; p++) {
>> 
>> Exclusive limits are often prefer, i.e. 'p < 3'.
>> 
> OK, that can be nice when I later introduce LAN9303_NUM_PORTS = 3.

This is indeed another reason what exclusive limits are prefered ;-)

>>> +		int ret;
>>> +
>>> +		ret = lan9303_disable_packet_processing(chip, p);
>>> +		if (ret)
>>> +			return ret;
>> 
>> When any non-zero return code means an error, we usually see 'err'
>> instead of 'ret'.
>> 
>
> But 'ret' is used throughout the rest of the file. Is it not better to
> be locally consistent?

You are correct, I was missing a bit of context here.

>>>   	case 1:
>>> -		return lan9303_enable_packet_processing(chip, port);
>> 
>> Is this deletion intentional? The commit message does not explain this.
>> 
>> When possible, it is appreciated to separate functional from
>> non-functional changes. For example one commit adding the loop in
>> lan9303_disable_processing and another one to not enable/disable packet
>> processing on port 1.
>> 
>
> Case fall through, the change is purely non-functional.
>
> You are perhaps thinking of the patch in my first series where I removed
> disable of port 0. I have put that on hold. Juergen says that the
> mainline driver works out of the box for him. So I will investigate
> that problem bit more.

Correct! I misread, my bad. This is indeed cleaner with this patch. With
the LAN9303_NUM_PORTS limit and detailed commit message, the patch LGTM.


Thanks,

        Vivien

[toc] | [prev] | [next] | [standalone]

#1700099 — Re: [PATCH net-next 2/2] net: dsa: lan9303: Simplify lan9303_xxx_packet_processing() usage

On 31. juli 2017 16:43, Vivien Didelot wrote:
> Hi Egil,
> 
> Egil Hjelmeland <privat@egil-hjelmeland.no> writes:
> 
>>>> +	for (p = 0; p <= 2; p++) {
>>>
>>> Exclusive limits are often prefer, i.e. 'p < 3'.
>>>
>> OK, that can be nice when I later introduce LAN9303_NUM_PORTS = 3.
> 
> This is indeed another reason what exclusive limits are prefered ;-)
> 
>>>> +		int ret;
>>>> +
>>>> +		ret = lan9303_disable_packet_processing(chip, p);
>>>> +		if (ret)
>>>> +			return ret;
>>>
>>> When any non-zero return code means an error, we usually see 'err'
>>> instead of 'ret'.
>>>
>>
>> But 'ret' is used throughout the rest of the file. Is it not better to
>> be locally consistent?
> 
> You are correct, I was missing a bit of context here.
> 
>>>>    	case 1:
>>>> -		return lan9303_enable_packet_processing(chip, port);
>>>
>>> Is this deletion intentional? The commit message does not explain this.
>>>
>>> When possible, it is appreciated to separate functional from
>>> non-functional changes. For example one commit adding the loop in
>>> lan9303_disable_processing and another one to not enable/disable packet
>>> processing on port 1.
>>>
>>
>> Case fall through, the change is purely non-functional.
>>
>> You are perhaps thinking of the patch in my first series where I removed
>> disable of port 0. I have put that on hold. Juergen says that the
>> mainline driver works out of the box for him. So I will investigate
>> that problem bit more.
> 
> Correct! I misread, my bad. This is indeed cleaner with this patch. With
> the LAN9303_NUM_PORTS limit and detailed commit message, the patch LGTM.
> 
> 
> Thanks,
> 
>          Vivien
> 

Would doing

-	chip->ds = dsa_switch_alloc(chip->dev, DSA_MAX_PORTS);
+	chip->ds = dsa_switch_alloc(chip->dev, LAN9303_NUM_PORTS);

at the same time be good, or breaking the scope of the patch?

Egil

[toc] | [prev] | [next] | [standalone]

#1700104 — Re: [PATCH net-next 2/2] net: dsa: lan9303: Simplify lan9303_xxx_packet_processing() usage

On 31. juli 2017 17:01, Vivien Didelot wrote:
> Hi Egil,
> 
> Egil Hjelmeland <privat@egil-hjelmeland.no> writes:
> 
>> Would doing
>>
>> -	chip->ds = dsa_switch_alloc(chip->dev, DSA_MAX_PORTS);
>> +	chip->ds = dsa_switch_alloc(chip->dev, LAN9303_NUM_PORTS);
>>
>> at the same time be good, or breaking the scope of the patch?
> 
> It is indeed out of scope. You may want to add a first commit "net: dsa:
> lan9303: introduce LAN9303_NUM_PORTS" for instance.
> 

In a later series I assume? Or is allowed to add patches to a series
under review?


> 
> Thanks,
> 
>          Vivien
> 

Egil

[toc] | [prev] | [next] | [standalone]

#1700109 — Re: [PATCH net-next 2/2] net: dsa: lan9303: Simplify lan9303_xxx_packet_processing() usage

Hi Egil,

Egil Hjelmeland <privat@egil-hjelmeland.no> writes:

>> It is indeed out of scope. You may want to add a first commit "net: dsa:
>> lan9303: introduce LAN9303_NUM_PORTS" for instance.
>
> In a later series I assume? Or is allowed to add patches to a series
> under review?

If it makes sense to include this first limit patch in this series under
review, you can totally add it. A simple "Changes in v2: - add
LAN9303_NUM_PORTS" list in the cover letter will do the job.


Thanks,

        Vivien

[toc] | [prev] | [next] | [standalone]

#1700105 — Re: [PATCH net-next 2/2] net: dsa: lan9303: Simplify lan9303_xxx_packet_processing() usage

Hi Egil,

Egil Hjelmeland <privat@egil-hjelmeland.no> writes:

> Would doing
>
> -	chip->ds = dsa_switch_alloc(chip->dev, DSA_MAX_PORTS);
> +	chip->ds = dsa_switch_alloc(chip->dev, LAN9303_NUM_PORTS);
>
> at the same time be good, or breaking the scope of the patch?

It is indeed out of scope. You may want to add a first commit "net: dsa:
lan9303: introduce LAN9303_NUM_PORTS" for instance.


Thanks,

        Vivien

[toc] | [prev] | [next] | [standalone]

#1700040

On Mon, Jul 31, 2017 at 01:33:53PM +0200, Egil Hjelmeland wrote:
> First patch: 
> Change lan9303_enable_packet_processing,
> lan9303_disable_packet_processing():
> Pass port number (0,1,2) as parameter instead of port offset.
> Plus replaced a constant 0x400 with LAN9303_SWITCH_PORT_REG()

Hi Egil

The cover note and the commit message should concentrate on the Why,
not the What. I can see the What by reading the patch.

Why do you want to pass port numbers?

Note that for netdev, the cover note also ends up in git as part of
the history.

   Andrew

[toc] | [prev] | [next] | [standalone]

#1700051

On 31. juli 2017 15:22, Andrew Lunn wrote:
> On Mon, Jul 31, 2017 at 01:33:53PM +0200, Egil Hjelmeland wrote:
>> First patch:
>> Change lan9303_enable_packet_processing,
>> lan9303_disable_packet_processing():
>> Pass port number (0,1,2) as parameter instead of port offset.
>> Plus replaced a constant 0x400 with LAN9303_SWITCH_PORT_REG()
> 
> Hi Egil
> 
> The cover note and the commit message should concentrate on the Why,
> not the What. I can see the What by reading the patch.
> 
> Why do you want to pass port numbers?
> 
Because other functions in the module pass port numbers. And to enable
the simplifications.

> Note that for netdev, the cover note also ends up in git as part of
> the history.
> 

OK, I did not know that netdev does that.

>     Andrew
> 

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web