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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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:
> 
> >> 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:
> >>
> >>> Rename the variable "no_ptp_cfg_active" with "gptp" and
> >>
> >>    This shouldn't be a rename but the extension of the meaning
> instead...
> >
> > 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].
> > [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? :-)
> 
> >>> "ptp_cfg_active" with "ccc_gac" to match the HW features.
> >>>
> >>> There is no functional change.
> >>>
> >>> Signed-off-by: Biju Das <biju.das.jz@xxxxxxxxxxxxxx>
> >>> Suggested-by: Sergey Shtylyov <s.shtylyov@xxxxxx>
> >>> Reviewed-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@xxxxxxxxxxxxxx>
> >>> ---
> >>> 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(-)
> >>
> >> [...]
> >>> 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,
> >>>  	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.
> 
> > 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.
> 
> > 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.
> 
> > But the existing driver code just support "gPTP support active in config
> mode" for R-Car Gen3.
> 
>    And?
> 
> > 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)
> 
> > Please let me know your thoughts.
> >
> > The same comments applies to all the comments you have mentioned below.
> >
> > Regards,
> > Biju
> 
> [...]
> 
> MBR, Sergey




[Index of Archives]     [Linux Samsung SOC]     [Linux Wireless]     [Linux Kernel]     [ATH6KL]     [Linux Bluetooth]     [Linux Netdev]     [Kernel Newbies]     [IDE]     [Security]     [Git]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux ATA RAID]     [Samba]     [Device Mapper]

  Powered by Linux