Re: [PATCH net-next] net: phy: micrel: Add support for PTP_PF_PEROUT for lan8841
From: Horatiu Vultur
Date: Fri Feb 17 2023 - 10:04:12 EST
The 02/17/2023 13:30, Russell King (Oracle) wrote:
Hi Russel,
>
> On Fri, Feb 17, 2023 at 08:52:13AM +0100, Horatiu Vultur wrote:
> > +static void lan8841_ptp_perout_off(struct kszphy_ptp_priv *ptp_priv, int pin)
> > +{
> > + struct phy_device *phydev = ptp_priv->phydev;
> > + u16 tmp;
> > +
> > + tmp = phy_read_mmd(phydev, 2, LAN8841_GPIO_EN) & LAN8841_PTP_GPIO_MASK;
> > + tmp &= ~BIT(pin);
> > + phy_write_mmd(phydev, 2, LAN8841_GPIO_EN, tmp);
>
> Problem 1: doesn't check the return value of phy_read_mmd(), so a
> spurious error results in an error code written back to the register.
>
> Issue 2: please use phy_modify_mmd() and definitions for the MMD. It
> probably also makes sense to cache the mask. Thus, this whole thing
> becomes:
>
> u16 mask = ~(LAN8841_PTP_GPIO_MASK | BIT(pin));
>
> phy_modify_mmd(phydev, MDIO_MMD_WIS, LAN8841_GPIO_EN, mask, 0);
> phy_modify_mmd(phydev, MDIO_MMD_WIS, LAN8841_GPIO_DIR, mask, 0);
> phy_modify_mmd(phydev, MDIO_MMD_WIS, LAN8841_GPIO_BUF, mask, 0);
Thanks for the review.
I will look at phy_modify_mmd and the other helper functions and try to
use them in the other perout functions.
>
> although I'm not sure why you need to mask off bits 15:11.
It is not necessary, only that those bits are marked as reserved.
>
> > +
> > + tmp = phy_read_mmd(phydev, 2, LAN8841_GPIO_DIR) & LAN8841_PTP_GPIO_MASK;
> > + tmp &= ~BIT(pin);
> > + phy_write_mmd(phydev, 2, LAN8841_GPIO_DIR, tmp);
> > +
> > + tmp = phy_read_mmd(phydev, 2, LAN8841_GPIO_BUF) & LAN8841_PTP_GPIO_MASK;
> > + tmp &= ~BIT(pin);
> > + phy_write_mmd(phydev, 2, LAN8841_GPIO_BUF, tmp);
> > +}
> > +
> > +static void lan8841_ptp_perout_on(struct kszphy_ptp_priv *ptp_priv, int pin)
> > +{
> > + struct phy_device *phydev = ptp_priv->phydev;
> > + u16 tmp;
> > +
> > + tmp = phy_read_mmd(phydev, 2, LAN8841_GPIO_EN) & LAN8841_PTP_GPIO_MASK;
> > + tmp |= BIT(pin);
> > + phy_write_mmd(phydev, 2, LAN8841_GPIO_EN, tmp);
> > +
> > + tmp = phy_read_mmd(phydev, 2, LAN8841_GPIO_DIR) & LAN8841_PTP_GPIO_MASK;
> > + tmp |= BIT(pin);
> > + phy_write_mmd(phydev, 2, LAN8841_GPIO_DIR, tmp);
> > +
> > + tmp = phy_read_mmd(phydev, 2, LAN8841_GPIO_BUF) & LAN8841_PTP_GPIO_MASK;
> > + tmp |= BIT(pin);
> > + phy_write_mmd(phydev, 2, LAN8841_GPIO_BUF, tmp);
>
> Similar as above.
>
> > +static void lan8841_ptp_remove_event(struct kszphy_ptp_priv *ptp_priv, int pin,
> > + u8 event)
> > +{
> > + struct phy_device *phydev = ptp_priv->phydev;
> > + u8 offset;
> > + u16 tmp;
> > +
> > + /* Not remove pin from the event. GPIO_DATA_SEL1 contains the GPIO
> > + * pins 0-4 while GPIO_DATA_SEL2 contains GPIO pins 5-9, therefore
> > + * depending on the pin, it requires to read a different register
> > + */
> > + if (pin < 5) {
> > + tmp = phy_read_mmd(phydev, 2, LAN8841_GPIO_DATA_SEL1);
> > + offset = pin;
> > + } else {
> > + tmp = phy_read_mmd(phydev, 2, LAN8841_GPIO_DATA_SEL2);
> > + offset = pin - 5;
> > + }
> > + tmp &= ~(LAN8841_GPIO_DATA_SEL_GPIO_DATA_SEL_EVENT_MASK << (3 * offset));
> > + if (pin < 5)
> > + phy_write_mmd(phydev, 2, LAN8841_GPIO_DATA_SEL1, tmp);
> > + else
> > + phy_write_mmd(phydev, 2, LAN8841_GPIO_DATA_SEL2, tmp);
>
> This could be much simpler using phy_modify_mmd().
>
> > +
> > + /* Disable the event */
> > + tmp = phy_read_mmd(phydev, 2, LAN8841_PTP_GENERAL_CONFIG);
> > + if (event == LAN8841_EVENT_A) {
> > + tmp &= ~LAN8841_PTP_GENERAL_CONFIG_LTC_EVENT_POL_A;
> > + tmp &= ~LAN8841_PTP_GENERAL_CONFIG_LTC_EVENT_A_MASK;
> > + } else {
> > + tmp &= ~LAN8841_PTP_GENERAL_CONFIG_LTC_EVENT_POL_A;
> > + tmp &= ~LAN8841_PTP_GENERAL_CONFIG_LTC_EVENT_A_MASK;
> > + }
> > + phy_write_mmd(phydev, 2, LAN8841_PTP_GENERAL_CONFIG, tmp);
>
> Ditto... and the theme seems to continue throughout the rest of this
> patch.
>
> > +static int lan8841_ptp_perout(struct ptp_clock_info *ptp,
> > + struct ptp_clock_request *rq, int on)
> > +{
> > + struct kszphy_ptp_priv *ptp_priv = container_of(ptp, struct kszphy_ptp_priv,
> > + ptp_clock_info);
> > + struct phy_device *phydev = ptp_priv->phydev;
> > + struct timespec64 ts_on, ts_period;
> > + s64 on_nsec, period_nsec;
> > + int pulse_width;
> > + int pin;
> > +
> > + if (rq->perout.flags & ~PTP_PEROUT_DUTY_CYCLE)
> > + return -EOPNOTSUPP;
> > +
> > + pin = ptp_find_pin(ptp_priv->ptp_clock, PTP_PF_PEROUT, rq->perout.index);
> > + if (pin == -1 || pin >= LAN8841_PTP_GPIO_NUM)
> > + return -EINVAL;
> > +
> > + if (!on) {
> > + lan8841_ptp_perout_off(ptp_priv, pin);
> > + lan8841_ptp_remove_event(ptp_priv, LAN8841_EVENT_A, pin);
> > + return 0;
> > + }
> > +
> > + ts_on.tv_sec = rq->perout.on.sec;
> > + ts_on.tv_nsec = rq->perout.on.nsec;
> > + on_nsec = timespec64_to_ns(&ts_on);
> > +
> > + ts_period.tv_sec = rq->perout.period.sec;
> > + ts_period.tv_nsec = rq->perout.period.nsec;
> > + period_nsec = timespec64_to_ns(&ts_period);
> > +
> > + if (period_nsec < 200) {
> > + phydev_warn(phydev,
> > + "perout period too small, minimum is 200 nsec\n");
>
> I'm not sure using the kernel log to print such things is a good idea,
> especially without rate limiting.
I think it would be nice to have these warnings as it would be nice to
know why it fails. I will use pr_warn_ratelimited in the next version.
>
> > @@ -3874,7 +4220,24 @@ static int lan8841_probe(struct phy_device *phydev)
> > priv = phydev->priv;
> > ptp_priv = &priv->ptp_priv;
> >
> > + ptp_priv->pin_config = devm_kmalloc_array(&phydev->mdio.dev,
> > + LAN8841_PTP_GPIO_NUM,
> > + sizeof(*ptp_priv->pin_config),
> > + GFP_KERNEL);
>
> devm_kcalloc() to avoid the memset() below?
Good point. I will do that.
>
> --
> RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
> FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
--
/Horatiu