Received-SPF: pass (google.com: domain of linux-kernel+bounces-159016-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Date: Thu, 25 Apr 2024 20:29:51 +0200
From: Horatiu Vultur <horatiu.vultur@microchip.com>
To: Vadim Fedorenko <vadim.fedorenko@linux.dev>
CC: <andrew@lunn.ch>, <hkallweit1@gmail.com>, <linux@armlinux.org.uk>,
	<davem@davemloft.net>, <edumazet@google.com>, <kuba@kernel.org>,
	<pabeni@redhat.com>, <richardcochran@gmail.com>, <netdev@vger.kernel.org>,
	<linux-kernel@vger.kernel.org>
Subject: Re: [PATCH net-next] net: phy: micrel: Add support for PTP_PF_EXTTS
 for lan8814
Message-ID: <20240425182951.2iq2lqngkkoy2fvo@DEN-DL-M31836.microchip.com>
References: <20240423195732.3353522-1-horatiu.vultur@microchip.com>
 <adf60fa5-052f-4135-acab-91a02a9aff61@linux.dev>
 <20240424191204.h2jajp57kpgccaql@DEN-DL-M31836.microchip.com>
 <c63232b1-2ee5-487a-b987-7cc6b47d3da3@linux.dev>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset="utf-8"
Content-Disposition: inline
In-Reply-To: <c63232b1-2ee5-487a-b987-7cc6b47d3da3@linux.dev>

The 04/25/2024 01:10, Vadim Fedorenko wrote:
> > > > +static int lan8814_ptp_extts(struct ptp_clock_info *ptpci,
> > > > +                          struct ptp_clock_request *rq, int on)
> > > > +{
> > > > +     struct lan8814_shared_priv *shared = container_of(ptpci, struct lan8814_shared_priv,
> > > > +                                                       ptp_clock_info);
> > > > +     struct phy_device *phydev = shared->phydev;
> > > > +     int pin;
> > > > +
> > > > +     if (rq->extts.flags & ~(PTP_ENABLE_FEATURE |
> > > > +                             PTP_EXTTS_EDGES |
> > > > +                             PTP_STRICT_FLAGS))
> > > > +             return -EOPNOTSUPP;
> > > > +
> > > > +     pin = ptp_find_pin(shared->ptp_clock, PTP_PF_EXTTS,
> > > > +                        rq->extts.index);
> > > > +     if (pin == -1 || pin != LAN8814_PTP_EXTTS_NUM)
> > > > +             return -EINVAL;
> > > 
> > > I'm not sure how will enable request pass this check?
> > > In lan8814_ptp_probe_once pins are initialized with PTP_PF_NONE,
> > > and ptp_find_pin will always return -1, which will end up with
> > > -EINVAL here and never hit lan8814_ptp_extts_on/lan8814_ptp_extts_off
> > > 
> > 
> > Why ptp_find_pin will always return -1? Because we can set the function
> > of the pin.
> 
> ah, I see, PTP_PIN_SETFUNC + PTP_EXTTS_REQUEST ioctls will do the
> configuration. Maybe make GPIO 3 as PTP_PF_EXTTS function by default?

I would like not to make GPIO 3 to have PTP_PF_EXTTS function by default
just to keep similar with the other driver in this file.

> 
> > ...
> > 
> >   >       }
> > > > @@ -3148,6 +3263,10 @@ static int lan8814_ptpci_verify(struct ptp_clock_info *ptp, unsigned int pin,
> > > >                if (pin >= LAN8814_PTP_PEROUT_NUM || pin != chan)
> > > >                        return -1;
> > > >                break;
> > > > +     case PTP_PF_EXTTS:
> > > > +             if (pin != LAN8814_PTP_EXTTS_NUM)
> > > 
> > > Here the check states that exactly GPIO 3 can have EXTTS function, but
> > > later in the config...
> > 
> > ...
> > > 
> > > > +                     return -1;
> > > > +             break;
> > > >        default:
> > > >                return -1;
> > > >        }
> > > > 
> > > > @@ -3541,7 +3721,7 @@ static int lan8814_ptp_probe_once(struct phy_device *phydev)
> > > >        snprintf(shared->ptp_clock_info.name, 30, "%s", phydev->drv->name);
> > > >        shared->ptp_clock_info.max_adj = 31249999;
> > > >        shared->ptp_clock_info.n_alarm = 0;
> > > > -     shared->ptp_clock_info.n_ext_ts = 0;
> > > > +     shared->ptp_clock_info.n_ext_ts = LAN8814_PTP_EXTTS_NUM;
> > > 
> > > Here ptp_clock is configured to have 3 pins supporting EXTTS.
> > > Looks like it should be n_ext_ts = 1;
> > 
> > Good point, let me have a look at this.
> 
> I have checked it while checking enable part. Conditions in ptp_ioctl
> give no options to limit lowest number of pin which supports EXTTS.
> 
> I think that the ptp_clock_info documentation is misleading here:
> 
> * @n_ext_ts:  The number of external time stamp channels.
> 
> should be replaced to something like "max index of external time
> stamp channel".
> 
> With all above the patch LGTM!

Thanks for looking at this.

> 
> Reviewed-by: Vadim Fedorenko <vadim.fedorenko@linux.dev>
> 
> > 
> > > 
> > > >        shared->ptp_clock_info.n_pins = LAN8814_PTP_GPIO_NUM;
> > > >        shared->ptp_clock_info.pps = 0;
> > > >        shared->ptp_clock_info.pin_config = shared->pin_config;
> > > 
> > > 
> > 
> 

-- 
/Horatiu