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

[Jakub Kicinski <kuba@kernel.org>]

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.

>  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

> +	/* 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?
-- 
pw-bot: cr