Re: [PATCH net-next v2] amd-xgbe: Add PPS periodic output support

["Rangoju, Raju" <raju.rangoju@amd.com>]

On 8/26/2025 5:34 AM, Jakub Kicinski wrote:
> On Fri, 22 Aug 2025 16:08:31 +0530 Raju Rangoju wrote:
>> +#define PPSx_MASK(x) ({						\
>> +	unsigned int __x = (x);					\
>> +	GENMASK(PPS_MAXIDX(__x), PPS_MINIDX(__x));		\
>> +})
>> +#define PPSCMDx(x, val) ({					\
>> +	unsigned int __x = (x);					\
>> +	GENMASK(PPS_MINIDX(__x) + 3, PPS_MINIDX(__x)) &		\
>> +	((val) << PPS_MINIDX(__x));				\
>> +})
>> +#define TRGTMODSELx(x, val) ({					\
>> +	unsigned int __x = (x);					\
>> +	GENMASK(PPS_MAXIDX(__x) - 1, PPS_MAXIDX(__x) - 2) &	\
>> +	((val) << (PPS_MAXIDX(__x) - 2));			\
>> +})
> 
> These macros are way too gnarly, please simplify them.
> For a start I'm not sure why you're making your life harder and try
> to have a shifted mask. Instead of masking the value before shifting.

Sure, will simplify them v3.

> 
>>   static int xgbe_enable(struct ptp_clock_info *info,
>>   		       struct ptp_clock_request *request, int on)
>>   {
>> -	return -EOPNOTSUPP;
>> +	struct xgbe_prv_data *pdata = container_of(info, struct xgbe_prv_data,
>> +						   ptp_clock_info);
>> +	struct xgbe_pps_config *pps_cfg;
>> +	unsigned long flags;
>> +	int ret;
>> +
>> +	dev_dbg(pdata->dev, "rq->type %d on %d\n", request->type, on);
>> +
>> +	if (request->type != PTP_CLK_REQ_PEROUT)
>> +		return -EOPNOTSUPP;
>> +
>> +	/* Reject requests with unsupported flags */
>> +	if (request->perout.flags)
>> +		return -EOPNOTSUPP;
> 
> Are you sure kernel can actually send you any flags here?
> ops->supported_perout_flags exists

Yes, it looks like ops->supported_perout_flags is already taking care of 
this. Thanks, I'll drop the above check.

> 
>> +	/* Validate index against our limit */
>> +	if (request->perout.index >= XGBE_MAX_PPS_OUT)
>> +		return -EINVAL;
> 
> Are you sure kernel can send you an index higher than what driver
> registered as supported?

looks like kernel is already handling it and this extra check isn't 
needed anymore. I'll drop it. Thanks Jakub.