RE: [PATCH 02/10] ravb: Rename "no_ptp_cfg_active" and "ptp_cfg_active" variables

Hi Sergei,

Subject: Re: [PATCH 02/10] ravb: Rename "no_ptp_cfg_active" and
"ptp_cfg_active" variables

Hello!

   Damn, DaveM continues ignoring my review efforts... :-( will finish
reviewing the series anyway.

On 10/2/21 10:53 AM, Biju Das wrote:

quoted

quoted

Subject: Re: [PATCH 02/10] ravb: Rename "no_ptp_cfg_active" and
"ptp_cfg_active" variables

On 10/1/21 6:06 PM, Biju Das wrote:

quoted

Rename the variable "no_ptp_cfg_active" with "gptp" and

   This shouldn't be a rename but the extension of the meaning

instead...

quoted

This is the original ptp support for both R-Car Gen3 and R-Car Gen2

without config in active mode. Later we added feature support active in
config mode for R-Car Gen3 by patch[1].

quoted

[1]
https://jpn01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgit.
kernel.org%2Fpub%2Fscm%2Flinux%2Fkernel%2Fgit%2Ftorvalds%2Flinux.git%2
Fcommit%2Fdrivers%2Fnet%2Fethernet%2Frenesas%2Fravb_main.c%3Fh%3Dv5.15
-rc3%26id%3Df5d7837f96e53a8c9b6c49e1bc95cf0ae88b99e8&data=04%7C01%
7Cbiju.das.jz%40bp.renesas.com%7Cb4a62982865a4f7cf38408d985d11fef%7C53
d82571da1947e49cb4625a166a4a2a%7C0%7C0%7C637687955521294093%7CUnknown%
7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJX
VCI6Mn0%3D%7C1000&sdata=6rpdh6hEAUl1yMng2ruFrKflBiDGmq6RylI90Ije3t
4%3D&reserved=0

   And? Do you think I don't remember the driver development history? :-)

quoted

quoted

quoted

"ptp_cfg_active" with "ccc_gac" to match the HW features.

There is no functional change.

Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
Suggested-by: Sergey Shtylyov <redacted>
Reviewed-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
---
RFc->v1:
 * Renamed the variable "no_ptp_cfg_active" with "gptp" and
   "ptp_cfg_active" with "ccc_gac
---
 drivers/net/ethernet/renesas/ravb.h      |  4 ++--
 drivers/net/ethernet/renesas/ravb_main.c | 26
++++++++++++------------
 2 files changed, 15 insertions(+), 15 deletions(-)

[...]

quoted

diff --git a/drivers/net/ethernet/renesas/ravb_main.c

b/drivers/net/ethernet/renesas/ravb_main.c
index 8f2358caef34..dc7654abfe55 100644

--- a/drivers/net/ethernet/renesas/ravb_main.c
+++ b/drivers/net/ethernet/renesas/ravb_main.c

@@ -1274,7 +1274,7 @@ static int ravb_set_ringparam(struct

net_device

*ndev,

quoted

 	if (netif_running(ndev)) {
 		netif_device_detach(ndev);
 		/* Stop PTP Clock driver */
-		if (info->no_ptp_cfg_active)
+		if (info->gptp)

   Where have you lost !info->ccc_gac?

  As per patch[1], the check is for R-Car Gen2. Why do you need
additional check as per the current driver?

   Because the driver now supports not only gen2, but also gen3, and
RZ/G2L, finally.

quoted

I see below you are proposing to enable both "gptp" and "ccc_gac" for
R-Car Gen3,

   Yes, this is how the hardware evolved. gPTP hardware can (optionally)
be active outside the config mode, otherwise there's no difference b/w
gen2 and gen3.

quoted

According to me it is a feature improvement for R-Car Gen3 in which,
you can have

1) gPTP support active in config mode
2) gPTP support not active in config mode

   Right.

quoted

But the existing driver code just support "gPTP support active in config

mode" for R-Car Gen3.

   And?

quoted

Do you want me to do feature improvement as well, as part of Gbethernet

support?

   I thought we agreed on this patch in the previous iteration, To be more
clear, by asking to remove the "double negation", I meant using:

I never thought of adding feature improvements as part of Gbethernet support. Any feature improvements after adding GbEthernet support.

If you expressed your ideas like adding gptp, ccc_gac for R-Car Gen3 earlier, then
I should have responded it is feature improvement. So please share your ideas in advance.

Regards,
Biju

	if (info->gptp && !info->ccc_gac)

versus your:

	if (!info->no_gptp && !info->ccc_gac)

quoted

Please let me know your thoughts.

The same comments applies to all the comments you have mentioned below.

Regards,
Biju

[...]

MBR, Sergey