Re: [PATCH net-next 2/2] net: dsa: lan9303: Simplify lan9303_xxx_packet_processing() usage
Hi Egil, Egil Hjelmeland 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
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 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
Re: [PATCH net-next 2/2] net: dsa: lan9303: Simplify lan9303_xxx_packet_processing() usage
Hi Egil, Egil Hjelmeland 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
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 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
Re: [PATCH net-next 2/2] net: dsa: lan9303: Simplify lan9303_xxx_packet_processing() usage
> >Reviewed-by: Andrew Lunn > > > > 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
Re: [PATCH net-next 2/2] net: dsa: lan9303: Simplify lan9303_xxx_packet_processing() usage
Hi Egil,
Egil Hjelmeland 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
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 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
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
---
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
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
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 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
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
> ---
> 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
