Hi all,

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

Hi Geert,

Thanks for the feedback.

quoted

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

Hi Biju,

On Thu, Aug 12, 2021 at 9:26 AM Biju Das [off-list ref]
wrote:

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

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.

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 

Then on the second patch will add as info->unaligned_tx as Sergei suggested.

Now the only open point is related to the data type of "int num_tx_desc"
and to align with sh_eth driver I will keep int.

Regards,
Biju