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

[Vladimir Oltean <olteanv@gmail.com>]

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@nxp.com>
> ---
> 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
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index f877e5aaf2c7..58f410b666e7 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -26781,6 +26781,13 @@ T:	git git://linuxtv.org/mhadli/v4l-dvb-davinci_devices.git
>  F:	drivers/media/platform/ti/davinci/
>  F:	include/media/davinci/
>  
> +TI DS125DF111 RETIMER PHY DRIVER
> +M:	Ioana Ciornei <ioana.ciornei@nxp.com>
> +L:	linux-phy@lists.infradead.org (moderated for non-subscribers)
> +S:	Maintained
> +F:	Documentation/devicetree/bindings/phy/ti,ds125df111.yaml
> +F:	drivers/phy/ti/phy-ds125df111.c
> +
>  TI ENHANCED CAPTURE (eCAP) DRIVER
>  M:	Vignesh Raghavendra <vigneshr@ti.com>
>  R:	Julien Panis <jpanis@baylibre.com>
> diff --git a/drivers/phy/ti/Kconfig b/drivers/phy/ti/Kconfig
> index b40f28019131..475e80fcd52d 100644
> --- a/drivers/phy/ti/Kconfig
> +++ b/drivers/phy/ti/Kconfig
> @@ -111,3 +111,13 @@ config PHY_TI_GMII_SEL
>  	help
>  	  This driver supports configuring of the TI CPSW Port mode depending on
>  	  the Ethernet PHY connected to the CPSW Port.
> +
> +config PHY_TI_DS125DF111
> +	tristate "DS125DF111 2-Channel Retimer Driver"
> +	depends on OF && I2C
> +	select GENERIC_PHY
> +	help
> +	  Enable this to add support for configuration and runtime management
> +	  of the TI DS125DF111 Multi-Protocol 2-Channel Retimer.
> +	  The retimer is modeled as a Generic PHY and supports both 10G and 1G
> +	  link speeds.
> diff --git a/drivers/phy/ti/Makefile b/drivers/phy/ti/Makefile
> index dcba2571c9bd..e68445ddd848 100644
> --- a/drivers/phy/ti/Makefile
> +++ b/drivers/phy/ti/Makefile
> @@ -9,3 +9,4 @@ obj-$(CONFIG_TWL4030_USB)		+= phy-twl4030-usb.o
>  obj-$(CONFIG_PHY_AM654_SERDES)		+= phy-am654-serdes.o
>  obj-$(CONFIG_PHY_TI_GMII_SEL)		+= phy-gmii-sel.o
>  obj-$(CONFIG_PHY_J721E_WIZ)		+= phy-j721e-wiz.o
> +obj-$(CONFIG_PHY_TI_DS125DF111)		+= phy-ds125df111.o
> diff --git a/drivers/phy/ti/phy-ds125df111.c b/drivers/phy/ti/phy-ds125df111.c
> new file mode 100644
> index 000000000000..084a94e655c2
> --- /dev/null
> +++ b/drivers/phy/ti/phy-ds125df111.c
> @@ -0,0 +1,252 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/* Copyright 2026 NXP */
> +
> +#include <linux/bitfield.h>
> +#include <linux/bits.h>
> +#include <linux/i2c.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/phy.h>
> +#include <linux/phy/phy.h>
> +#include <linux/slab.h>
> +
> +#define DS125DF111_NUM_CH			2
> +#define DS125DF111_NUM_VCO_GROUP_REG		5
> +
> +#define DS125DF111_CH_SELECT			0xff
> +#define DS125DF111_CH_SELECT_TARGET_MASK	GENMASK(3, 0)
> +#define DS125DF111_CH_SELECT_EN			BIT(2)
> +
> +#define DS125DF111_CH_CTRL			0x00
> +#define DS125DF111_CH_CTRL_RESET		BIT(2) /* self clearing */
> +
> +#define DS125DF111_VCO_GROUP_BASE		0x60
> +
> +#define DS125DF111_RATIOS			0x2F
> +#define DS125DF111_RATIOS_RATE_MASK		GENMASK(7, 6)
> +#define DS125DF111_RATIOS_SUBRATE_MASK		GENMASK(5, 4)
> +
> +struct ds125df111_ch {
> +	struct phy *phy;
> +	struct ds125df111_priv *priv;
> +	int idx;
> +};
> +
> +struct ds125df111_priv {
> +	struct ds125df111_ch ch[DS125DF111_NUM_CH];
> +	struct i2c_client *client;
> +	struct mutex mutex; /* protects access to shared registers */
> +};
> +
> +enum ds125df111_mode {
> +	FREQ_1G,
> +	FREQ_10G,
> +};
> +
> +static const struct ds125df111_config {
> +	u8 vco_group[DS125DF111_NUM_VCO_GROUP_REG];
> +	u8 rate;
> +	u8 subrate;
> +} ds125df111_cfg[] = {
> +	[FREQ_1G] = {
> +		/* VCO group #0 = 10GHz, VCO group #1 = 10GHz */
> +		.vco_group = {0x00, 0xB2, 0x00, 0xB2, 0xCC},
> +		/* By using the following combination of rate and subrate we
> +		 * select divide ratios of 1, 2, 4, 8 on both groups
> +		 */
> +		.rate = 0x1,
> +		.subrate = 0x2,
> +	},
> +
> +	[FREQ_10G] = {
> +		/* VCO group #0 = 10.3125GHz, VCO group #1 = 10.3125GHz */
> +		.vco_group = {0x90, 0xB3, 0x90, 0xB3, 0xCD},
> +		/* By using the following combination of rate and subrate we
> +		 * select divide ratios of 1 on both groups
> +		 */
> +		.rate = 0x1,
> +		.subrate = 0x3,
> +	},
> +};
> +
> +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?

> +
> +	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).

> +		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.

> +
> +	/* 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.

> +	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).

> +}
> +
> +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.

> +	}
> +
> +	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().

> +	.owner		= THIS_MODULE,
> +};
> +
> +static struct phy *ds125df111_xlate(struct device *dev,
> +				    const struct of_phandle_args *args)
> +{
> +	struct ds125df111_priv *priv = dev_get_drvdata(dev);
> +	u32 idx;
> +
> +	if (args->args_count != 1)
> +		return ERR_PTR(-EINVAL);
> +
> +	idx = args->args[0];
> +	if (idx >= DS125DF111_NUM_CH) {
> +		dev_err(dev, "Maximum number of channels is %d\n",
> +			DS125DF111_NUM_CH);
> +		return ERR_PTR(-EINVAL);
> +	}
> +
> +	return priv->ch[idx].phy;
> +}
> +
> +static int ds125df111_probe(struct i2c_client *client)
> +{
> +	struct device *dev = &client->dev;
> +	struct phy_provider *provider;
> +	struct ds125df111_priv *priv;
> +	int i, err;
> +
> +	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> +	if (!priv)
> +		return -ENOMEM;
> +	priv->client = client;
> +	err = devm_mutex_init(dev, &priv->mutex);
> +	if (err)
> +		return err;
> +
> +	i2c_set_clientdata(client, priv);
> +
> +	for (i = 0; i < DS125DF111_NUM_CH; i++) {
> +		struct ds125df111_ch *ch = &priv->ch[i];
> +		struct phy *phy;
> +
> +		phy = devm_phy_create(dev, NULL, &ds125df111_ops);
> +		if (IS_ERR(phy))
> +			return PTR_ERR(phy);
> +
> +		ch->idx = i;
> +		ch->priv = priv;
> +		ch->phy = phy;
> +
> +		phy_set_drvdata(phy, ch);
> +	}
> +
> +	provider = devm_of_phy_provider_register(dev, ds125df111_xlate);
> +
> +	return PTR_ERR_OR_ZERO(provider);
> +}
> +
> +static const struct of_device_id ds125df111_dt_ids[] = {
> +	{ .compatible = "ti,ds125df111", },
> +	{},

Unnecessary comma after sentinel entry.

> +};
> +MODULE_DEVICE_TABLE(of, ds125df111_dt_ids);
> +
> +static struct i2c_driver ds125df111_driver = {
> +	.driver = {
> +		.name = "ds125df111",
> +		.of_match_table = ds125df111_dt_ids,
> +	},
> +	.probe = ds125df111_probe,
> +};
> +module_i2c_driver(ds125df111_driver);
> +
> +MODULE_AUTHOR("Ioana Ciornei <ioana.ciornei@nxp.com>");
> +MODULE_DESCRIPTION("TI DS125DF111 Retimer driver");
> +MODULE_LICENSE("GPL");
> -- 
> 2.25.1
> 
>