RE: [PATCH net 1/1] net: fec: fix the PTP periodic output sysfs interface

[Buday Csaba]

>> Fixes: bf8ca67e2167 ("net: fec: refactor PPS channel configuration")
>> 
>> Signed-off-by: Buday Csaba <buday.csaba@xxxxxxxxx>
>> ---
>>  drivers/net/ethernet/freescale/fec_ptp.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>> 
>> diff --git a/drivers/net/ethernet/freescale/fec_ptp.c
>> b/drivers/net/ethernet/freescale/fec_ptp.c
>> index 7f6b574320716..c1af81002b8fa 100644
>> --- a/drivers/net/ethernet/freescale/fec_ptp.c
>> +++ b/drivers/net/ethernet/freescale/fec_ptp.c
>> @@ -534,7 +534,7 @@ static int fec_ptp_enable(struct ptp_clock_info *ptp,
>>  		if (rq->perout.flags)
>>  			return -EOPNOTSUPP;
>> 
>> -		if (rq->perout.index != fep->pps_channel)
>> +		if (rq->perout.index != 0)
>>  			return -EOPNOTSUPP;
>> 
>>  		period.tv_sec = rq->perout.period.sec;
>> --
>> 2.39.5
>> 
> 
> I think the correct fix should update the n_per_out value like below.
> 
> @@ -756,7 +756,7 @@ void fec_ptp_init(struct platform_device *pdev, int irq_idx)
>         fep->ptp_caps.max_adj = 250000000;
>         fep->ptp_caps.n_alarm = 0;
>         fep->ptp_caps.n_ext_ts = 0;
> -       fep->ptp_caps.n_per_out = 1;
> +       fep->ptp_caps.n_per_out = 4;
> 
> The commit 566c2d83887f ("net: fec: make PPS channel configurable") added
> the property "fsl,pps-channel" to indicate which pulse channel is available.
> So we should not fix the pulse output channel to 0.
> 

But is it wise to expose four channels, when only one is usable by this 
driver?

This patch does not affect the channel selection. That can be still made
through the DT. The driver uses `fep->pps_channel` everywhere else, while
rq->perout.index is only checked in this single line, and never used
anywhere else.

So the question is whether userspace is supposed to reference the hardware
channel number directly, or the mapping is meant to be left to the devicetree
and the driver internally.

I hope the netdev maintainers and/or Richard Cochran can clarify the
intended purpose of `n_per_out` and the expected channel indexing.