Re: [Patch net-next v1 01/12] net: dsa: microchip: ptp: add the posix clock support

[Vladimir Oltean <olteanv@gmail.com>]

On Mon, Nov 28, 2022 at 04:02:16PM +0530, Arun Ramadoss wrote:
> From: Christian Eggers <ceggers@arri.de>
> 
> This patch implement routines (adjfine, adjtime, gettime and settime)
> for manipulating the chip's PTP clock. It registers the ptp caps
> to posix clock register.
> 
> Signed-off-by: Christian Eggers <ceggers@arri.de>
> Co-developed-by: Arun Ramadoss <arun.ramadoss@microchip.com>
> Signed-off-by: Arun Ramadoss <arun.ramadoss@microchip.com>
> 
> ---
> RFC v2 -> Patch v1
> - Repharsed the Kconfig help text
> - Removed IS_ERR_OR_NULL check in ptp_clock_unregister
> - Add the check for ptp_data->clock in ksz_ptp_ts_info
> - Renamed MAX_DRIFT_CORR to KSZ_MAX_DRIFT_CORR
> - Removed the comments
> - Variables declaration in reverse christmas tree
> - Added the ptp_clock_optional
> ---
> diff --git a/drivers/net/dsa/microchip/ksz_common.h b/drivers/net/dsa/microchip/ksz_common.h
> index c6726cbd5465..5a6bfd42c6f9 100644
> --- a/drivers/net/dsa/microchip/ksz_common.h
> +++ b/drivers/net/dsa/microchip/ksz_common.h
> @@ -444,6 +447,19 @@ static inline int ksz_write32(struct ksz_device *dev, u32 reg, u32 value)
>  	return ret;
>  }
>  
> +static inline int ksz_rmw16(struct ksz_device *dev, u32 reg, u16 mask,
> +			    u16 value)
> +{
> +	int ret;
> +
> +	ret = regmap_update_bits(dev->regmap[1], reg, mask, value);
> +	if (ret)
> +		dev_err(dev->dev, "can't rmw 16bit reg: 0x%x %pe\n", reg,
> +			ERR_PTR(ret));

Is the colon misplaced? What do you want to say, "can't rmw 16bit reg: 0x0 -EIO",
or "can't rmw 16bit reg 0x0: -EIO"?

Reminds me of a joke:
"The inventor of the Oxford comma has died. Tributes have been led by
J.K. Rowling, his wife and the Queen of England".

> +
> +	return ret;
> +}
> +
>  static inline int ksz_write64(struct ksz_device *dev, u32 reg, u64 value)
>  {
>  	u32 val[2];
> +static int ksz_ptp_settime(struct ptp_clock_info *ptp,
> +			   const struct timespec64 *ts)
> +{
> +	struct ksz_ptp_data *ptp_data = ptp_caps_to_data(ptp);
> +	struct ksz_device *dev = ptp_data_to_ksz_dev(ptp_data);
> +	int ret;
> +
> +	mutex_lock(&ptp_data->lock);
> +
> +	/* Write to shadow registers and Load PTP clock */
> +	ret = ksz_write16(dev, REG_PTP_RTC_SUB_NANOSEC__2, PTP_RTC_0NS);
> +	if (ret)
> +		goto error_return;
> +
> +	ret = ksz_write32(dev, REG_PTP_RTC_NANOSEC, ts->tv_nsec);
> +	if (ret)
> +		goto error_return;
> +
> +	ret = ksz_write32(dev, REG_PTP_RTC_SEC, ts->tv_sec);
> +	if (ret)
> +		goto error_return;
> +
> +	ret = ksz_rmw16(dev, REG_PTP_CLK_CTRL, PTP_LOAD_TIME, PTP_LOAD_TIME);
> +
> +error_return:

I would avoid naming labels with "error_", if the success code path is
also going to run through the code they point to. "goto unlock" sounds
about right.

> +	mutex_unlock(&ptp_data->lock);
> +
> +	return ret;
> +}
> +
> +static const struct ptp_clock_info ksz_ptp_caps = {
> +	.owner		= THIS_MODULE,
> +	.name		= "Microchip Clock",
> +	.max_adj	= KSZ_MAX_DRIFT_CORR,
> +	.gettime64	= ksz_ptp_gettime,
> +	.settime64	= ksz_ptp_settime,
> +	.adjfine	= ksz_ptp_adjfine,
> +	.adjtime	= ksz_ptp_adjtime,
> +};

Is it a conscious decision to have this structure declared here in the
.rodata section (I think that's where this goes?), when it will only be
used as a blueprint for the implicit memcpy (struct assignment) in
ksz_ptp_clock_register()?

Just saying that it would be possible to initialize the fields in
ptp_data->caps even without resorting to declaring one extra structure,
which consumes space. I'll leave you alone if you ACK that you know your
assignment below is a struct copy and not a pointer assignment.

> +
> +int ksz_ptp_clock_register(struct dsa_switch *ds)
> +{
> +	struct ksz_device *dev = ds->priv;
> +	struct ksz_ptp_data *ptp_data;
> +	int ret;
> +
> +	ptp_data = &dev->ptp_data;
> +	mutex_init(&ptp_data->lock);
> +
> +	ptp_data->caps = ksz_ptp_caps;
> +
> +	ret = ksz_ptp_start_clock(dev);
> +	if (ret)
> +		return ret;
> +
> +	ptp_data->clock = ptp_clock_register(&ptp_data->caps, dev->dev);
> +	if (IS_ERR_OR_NULL(ptp_data->clock))
> +		return PTR_ERR(ptp_data->clock);
> +
> +	ret = ksz_rmw16(dev, REG_PTP_MSG_CONF1, PTP_802_1AS, PTP_802_1AS);
> +	if (ret)
> +		goto error_unregister_clock;

Registering a structure with a subsystem generally means that it becomes
immediately accessible to user space, and its (POSIX clock) ops are callable.

You haven't explained what PTP_802_1AS does, concretely, even though
I asked for a comment in the previous patch set. Is it okay for the PTP
clock to be registered while the PTP_802_1AS bit hasn't been yet written?
The first few operations might take place with it still unset.

I know what 802.1AS is, I just don't know what the register field does.

> +
> +	return 0;
> +
> +error_unregister_clock:
> +	ptp_clock_unregister(ptp_data->clock);
> +	return ret;
> +}