Hi Sergei,

-----Original Message-----
Subject: Re: [PATCH net-next v2 1/8] ravb: Add struct ravb_hw_info to
driver data

Hello!

On 18.08.2021 9:29, Biju Das wrote:

[...]

quoted

quoted

quoted

quoted

quoted

quoted

quoted

-----Original Message-----
On Mon, Aug 2, 2021 at 12:27 PM Biju Das
[off-list ref]
wrote:

quoted

The DMAC and EMAC blocks of Gigabit Ethernet IP found on RZ/G2L
SoC are similar to the R-Car Ethernet AVB IP. With a few
changes in the driver we can support both IPs.

Currently a runtime decision based on the chip type is used to
distinguish the HW differences between the SoC families.

The number of TX descriptors for R-Car Gen3 is 1 whereas on
R-Car
Gen2 and RZ/G2L it is 2. For cases like this it is better to
select the number of TX descriptors by using a structure with a
value, rather than a runtime decision based on the chip type.

This patch adds the num_tx_desc variable to struct ravb_hw_info
and also replaces the driver data chip type with struct
ravb_hw_info by moving chip type to it.

Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
Reviewed-by: Lad Prabhakar
[off-list ref]

Thanks for your patch!

quoted

--- a/drivers/net/ethernet/renesas/ravb.h
+++ b/drivers/net/ethernet/renesas/ravb.h

@@ -988,6 +988,11 @@ enum ravb_chip_id {
         RCAR_GEN3,
  };

+struct ravb_hw_info {
+       enum ravb_chip_id chip_id;
+       int num_tx_desc;

Why not "unsigned int"? ...
This comment applies to a few more subsequent patches.

To avoid signed and unsigned comparison warnings.

quoted

quoted

+};
+
  struct ravb_private {
         struct net_device *ndev;
         struct platform_device *pdev; @@ -1040,6 +1045,8 @@
struct ravb_private {
         unsigned txcidm:1;              /* TX Clock Internal

Delay

quoted

quoted

quoted

quoted

quoted

Mode

quoted

quoted

*/

quoted

         unsigned rgmii_override:1;      /* Deprecated rgmii-*id

behavior

quoted

quoted

*/

quoted

         int num_tx_desc;                /* TX descriptors per

packet

quoted

*/

quoted

quoted

... oh, here's the original culprit.

Exactly, this the reason.

Do you want me to change this into unsigned int? Please let me

know.

quoted

quoted

quoted

quoted

quoted

Up to you (or the maintainer? ;-)

For new fields (in the other patches), I would use unsigned for
all unsigned values.  Signed values have more pitfalls related to
undefined behavior.

Sergei, What is your thoughts here? Please let me know.

Here is my plan.

I will split this patch into two as Andrew suggested and

    If you mran changing the ravb_private::num_tx_desc to *unsigned*,
it'll be a good cleanup. What's would be the 2nd part tho?

OK in that case, I will split this patch into 3.

First patch for adding struct ravb_hw_info to driver data and replace
driver data chip type with struct ravb_hw_info

    Couldn't this be a 2nd patch?..

quoted

Second patch for changing ravb_private::num_tx_desc from int to unsigned

int.

    ... and this one the 1st?

quoted

Third patch for adding aligned_tx to struct ravb_hw_info.

Sure. Will do.

Cheers,
Biju