Messages in this thread | | | From | <> | Subject | RE: [PATCH v2 net-next] net: phy: micrel: Adding SQI support for lan8814 phy | Date | Tue, 6 Sep 2022 10:41:33 +0000 |
| |
Hi Andrew,
> -----Original Message----- > From: Andrew Lunn <andrew@lunn.ch> > Sent: Monday, September 5, 2022 6:40 PM > To: Divya Koppera - I30481 <Divya.Koppera@microchip.com> > Cc: hkallweit1@gmail.com; linux@armlinux.org.uk; davem@davemloft.net; > edumazet@google.com; kuba@kernel.org; pabeni@redhat.com; > netdev@vger.kernel.org; linux-kernel@vger.kernel.org; UNGLinuxDriver > <UNGLinuxDriver@microchip.com> > Subject: Re: [PATCH v2 net-next] net: phy: micrel: Adding SQI support for > lan8814 phy > > EXTERNAL EMAIL: Do not click links or open attachments unless you know the > content is safe > > On Mon, Sep 05, 2022 at 03:47:30PM +0530, Divya Koppera wrote: > > Supports SQI(Signal Quality Index) for lan8814 phy, where it has SQI > > index of 0-7 values and this indicator can be used for cable integrity > > diagnostic and investigating other noise sources. It is not supported > > for 10Mbps speed > > > > Signed-off-by: Divya Koppera <Divya.Koppera@microchip.com> > > --- > > v1 -> v2 > > - Given SQI support for all pairs of wires in 1000/100 base-T phy's > > uAPI may run through all instances in future. At present returning > > only first instance as uAPI supports for only 1 pair. > > - SQI is not supported for 10Mbps speed, handled accordingly. > > I would prefer you solve the problem of returning all pairs. >
I will try to improve uAPI.
The other way I can try is the point you mentioned below to write helper with pair number and having function cal with pair 0.
> I'm not sure how useful the current implementation is, especially at > 100Mbps, where pair 0 could actually be the transmit pair. Does it give a > sensible value in that case? >
We do have SQI support for 100Mbps to pair 0 only. For other pairs SQI values are invalid values.
> > +static int lan8814_get_sqi(struct phy_device *phydev) { > > + int ret, val, pair; > > + int sqi_val[4]; > > + > > + if (phydev->speed == SPEED_10) > > + return -EOPNOTSUPP; > > + > > + for (pair = 0; pair < 4; pair++) { > > + val = lanphy_read_page_reg(phydev, 1, LAN8814_DCQ_CTRL); > > + if (val < 0) > > + return val; > > + > > + val &= ~LAN8814_DCQ_CTRL_CHANNEL_MASK; > > + val |= pair; > > + val |= LAN8814_DCQ_CTRL_READ_CAPTURE_; > > + ret = lanphy_write_page_reg(phydev, 1, LAN8814_DCQ_CTRL, val); > > + if (ret < 0) > > + return ret; > > + > > + ret = lanphy_read_page_reg(phydev, 1, LAN8814_DCQ_SQI); > > + if (ret < 0) > > + return ret; > > + > > + sqi_val[pair] = FIELD_GET(LAN8814_DCQ_SQI_VAL_MASK, ret); > > + } > > + > > + return *sqi_val; > > How is this going to work in the future? sqi_val is on the stack. You cannot > return a pointer to it. So this function is going to need modifications. > > If you really want to prepare for a future implementation which could return > all four, i would probably make this a helper which takes a pair number. And > then have a function call it once for pair 0. >
I Will go for resolving this if I couldn't resolve that 4 pair issue in uAPI.
> Andrew
Thanks, Divya
| |