Re: [PATCH v2 phy-next 2/2] phy: ti: add PHY driver for TI DS125DF111 Dual-Channel Retimer

[Ioana Ciornei]

On Fri, May 15, 2026 at 03:24:18PM +0300, Vladimir Oltean wrote:
> On Fri, May 15, 2026 at 02:01:45PM +0300, Ioana Ciornei wrote:
> > Add a generic PHY driver for the TI DS125DF111 Multi-Protocol
> > Dual-Channel Retimer. The driver currently supports only 10G and 1G link
> > speeds but it can easily extended to also cover other usecases.
> > 
> > Since the available datasheet (https://www.ti.com/lit/gpn/DS125DF111)
> > does not name the registers, the name for the macros were determined by
> > their usage pattern.
> > 
> > A PHY device is created for each of the two channels present on the
> > retimer. This allows for independent configuration of the two channels.
> > This capability is especially important on retimers which have more than
> > 2 channels that can be, depending on the board design, connected in
> > multiple different ways to the SerDes lanes.
> > 
> > Signed-off-by: Ioana Ciornei <ioana.ciornei@xxxxxxx>
> > ---
> > Changes in v2:
> > - Explicitly include all the needed headers
> > - Change ds125df111_xlate() so that it returns an error if args_count is
> > not exactly 1
> > - Add a MAINTAINERS entry
> > ---
> >  MAINTAINERS                     |   7 +
> >  drivers/phy/ti/Kconfig          |  10 ++
> >  drivers/phy/ti/Makefile         |   1 +
> >  drivers/phy/ti/phy-ds125df111.c | 252 ++++++++++++++++++++++++++++++++
> >  4 files changed, 270 insertions(+)
> >  create mode 100644 drivers/phy/ti/phy-ds125df111.c
> > 

(...)

> > +static int ds125df111_configure(struct phy *phy,
> > +				const struct ds125df111_config *cfg)
> > +{
> > +	struct ds125df111_ch *ch = phy_get_drvdata(phy);
> > +	struct ds125df111_priv *priv = ch->priv;
> > +	struct i2c_client *i2c = priv->client;
> > +	struct device *dev = &phy->dev;
> > +	u8 val;
> > +	int err, i;
> 
> Not mandatory, but if the rest of the file uses reverse Christmas tree
> variable ordering, could you stick to that here as well?

Ok, sure.

> 
> > +
> > +	mutex_lock(&priv->mutex);
> > +
> > +	/* Make sure that any subsequent read/write operation will be directed
> > +	 * only to the registers of the selected channel
> > +	 */
> > +	err = i2c_smbus_read_byte_data(i2c, DS125DF111_CH_SELECT);
> > +	if (err < 0) {
> > +		dev_err(dev, "Unable to select channel\n");
> 
> Here and everywhere else: could you please print a symbolic description
> of the error? %pe, ERR_PTR(err).

Nice, I didn't know about the %pe.

> 
> > +		goto out;
> > +	}
> > +	val = (u8)err;
> > +	val &= ~DS125DF111_CH_SELECT_TARGET_MASK;
> > +	val |= DS125DF111_CH_SELECT_EN | ch->idx;
> > +	err = i2c_smbus_write_byte_data(i2c, DS125DF111_CH_SELECT, val);
> > +	if (err < 0) {
> > +		dev_err(dev, "Unable to select channel\n");
> > +		goto out;
> > +	}
> > +
> > +	/* Reset Channel Registers */
> > +	err = i2c_smbus_read_byte_data(i2c, DS125DF111_CH_CTRL);
> > +	if (err < 0) {
> > +		dev_err(dev, "Error resetting channel configuration\n");
> > +		goto out;
> > +	}
> > +	val = (u8)err;
> > +	val |= DS125DF111_CH_CTRL_RESET;
> > +	err = i2c_smbus_write_byte_data(i2c, DS125DF111_CH_CTRL, val);
> > +	if (err < 0) {
> > +		dev_err(dev, "Error resetting channel configuration\n");
> > +		goto out;
> > +	}
> 
> Did you consider simplifying this function using a ds125df111_rmw() helper?
> All configuration accesses except the VCO group frequencies are
> read-modify-write.
> 

I will look into a _rmw helper and see if it helps.

> > +
> > +	/* Program the VCO group frequencies */
> > +	for (i = 0; i < DS125DF111_NUM_VCO_GROUP_REG; i++) {
> > +		err = i2c_smbus_write_byte_data(i2c,
> > +						DS125DF111_VCO_GROUP_BASE + i,
> > +						cfg->vco_group[i]);
> > +		if (err < 0) {
> > +			dev_err(dev, "Error programming VCO group frequencies\n");
> > +			goto out;
> > +		}
> > +	}
> > +
> > +	/* Set the Divide Ratios for the VCO Groups*/
> 
> Space between Groups and */
> Also, Divide Ratios, Groups, Channel Registers are not proper nouns,
> they don't need to be capitalized.

Ok, will fix.

> 
> > +	err = i2c_smbus_read_byte_data(i2c, DS125DF111_RATIOS);
> > +	if (err < 0) {
> > +		dev_err(dev, "Error programming the divide ratios\n");
> > +		goto out;
> > +	}
> > +	val = (u8)err;
> > +	val &= ~(DS125DF111_RATIOS_RATE_MASK | DS125DF111_RATIOS_SUBRATE_MASK);
> > +	val |= FIELD_PREP(DS125DF111_RATIOS_RATE_MASK, cfg->rate) |
> > +		FIELD_PREP(DS125DF111_RATIOS_SUBRATE_MASK, cfg->subrate);
> > +	err = i2c_smbus_write_byte_data(i2c, DS125DF111_RATIOS, val);
> > +	if (err < 0) {
> > +		dev_err(dev, "Error programming the divide ratios\n");
> > +		goto out;
> > +	}
> > +
> > +	mutex_unlock(&priv->mutex);
> > +
> > +	return 0;
> > +
> > +out:
> > +	mutex_unlock(&priv->mutex);
> > +
> > +	return err;
> 
> You don't need a separate code path for the 'out' label, it can be
> common with the normal exit path (err will be 0).

Ok.

> 
> > +}
> > +
> > +static int ds125df111_set_mode(struct phy *phy, enum phy_mode mode, int submode)
> > +{
> > +	const struct ds125df111_config *cfg;
> > +
> > +	if (mode != PHY_MODE_ETHERNET)
> > +		return -EOPNOTSUPP;
> 
> Please use a different error code like -EINVAL. Let -EOPNOTSUPP mean
> that the function is not implemented (when calling phy_set_mode_ext()).
> 
> > +
> > +	switch (submode) {
> > +	case PHY_INTERFACE_MODE_10GBASER:
> > +		cfg = &ds125df111_cfg[FREQ_10G];
> > +		break;
> > +	case PHY_INTERFACE_MODE_1000BASEX:
> > +	case PHY_INTERFACE_MODE_SGMII:
> > +		cfg = &ds125df111_cfg[FREQ_1G];
> > +		break;
> > +	default:
> > +		return -EOPNOTSUPP;
> 
> Same here.

Will change in both instances.

> 
> > +	}
> > +
> > +	return ds125df111_configure(phy, cfg);
> > +}
> > +
> > +static const struct phy_ops ds125df111_ops = {
> > +	.set_mode	= ds125df111_set_mode,
> 
> Can you please implement .validate() as well? It will be made mandatory
> in the future for those who implement .set_mode().
> 

Sure. Will implement the .validate() callback in v3.

(...)

> > +	.owner		= THIS_MODULE,
> > +};
> > +
> > +static const struct of_device_id ds125df111_dt_ids[] =
> > +	{ .compatible = "ti,ds125df111", },
> > +	{},
> 
> Unnecessary comma after sentinel entry.

I will remove it.