Messages in this thread | | | Date | Thu, 1 Dec 2022 02:17:46 +0200 | From | Vladimir Oltean <> | Subject | Re: [Patch net-next v1 01/12] net: dsa: microchip: ptp: add the posix clock support |
| |
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; > +}
| |