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

[Ioana Ciornei]

On Sun, May 17, 2026 at 10:07:56PM +0530, Vinod Koul wrote:
> On 16-05-26, 09:03, 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 v3:
> > - Use reverse Christmas tree ordering
> > - Print a symbolic description in case of error
> > - Some words do not need to be capitalized
> > - Remove duplicated exit code path
> > - Return -EINVAL in case of unsupported submode received in .set_mode()
> > - Add a .validate() callback
> > - Remove comma after sentinel entry
> > - Add a ds125df111_rmw() helper
> > - Use read_poll_timeout() to wait for channel reset to complete
> > 
> > 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 | 294 ++++++++++++++++++++++++++++++++
> >  4 files changed, 312 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@xxxxxxx>
> > +L:	linux-phy@xxxxxxxxxxxxxxxxxxx (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@xxxxxx>
> >  R:	Julien Panis <jpanis@xxxxxxxxxxxx>
> > 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
> 
> This should be in alphabetical order, so I guess before PHY_TI_G...

Ok, will reorder them.
> 
> > +	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
> 
> Here as well

Sure.

> 
> > diff --git a/drivers/phy/ti/phy-ds125df111.c b/drivers/phy/ti/phy-ds125df111.c
> > new file mode 100644
> > index 000000000000..8788f340e3b0
> > --- /dev/null
> > +++ b/drivers/phy/ti/phy-ds125df111.c
> > @@ -0,0 +1,294 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/* Copyright 2026 NXP */
> > +
> > +#include <linux/bitfield.h>
> > +#include <linux/bits.h>
> > +#include <linux/i2c.h>
> > +#include <linux/iopoll.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_CH_RST_SLEEP_US		10
> > +#define DS125DF111_CH_RST_TIMEOUT_US		10000
> > +
> > +#define DS125DF111_VCO_GROUP_BASE		0x60
> > +
> > +#define DS125DF111_RATIOS			0x2F
> 
> Lower case for the hex values please
> 
> > +#define DS125DF111_RATIOS_RATE_MASK		GENMASK(7, 6)
> > +#define DS125DF111_RATIOS_SUBRATE_MASK		GENMASK(5, 4)
> > +#define DS125DF111_RATIOS_MASK			GENMASK(7, 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_rmw(struct ds125df111_priv *priv, u8 reg, u8 clr, u8 set)
> > +{
> > +	struct i2c_client *i2c = priv->client;
> > +	int err;
> > +	u8 val;
> > +
> > +	err = i2c_smbus_read_byte_data(i2c, reg);
> > +	if (err < 0)
> > +		return err;
> > +
> > +	val = (u8)err;
> > +	val &= ~clr;
> > +	val |= set;
> > +
> > +	err = i2c_smbus_write_byte_data(i2c, reg, val);
> > +	if (err < 0)
> > +		return err;
> > +
> > +	return 0;
> > +}
> > +
> > +static int ds125df111_configure(struct phy *phy,
> > +				const struct ds125df111_config *cfg)
> 
> This should start at preceding line open braces (here and few other
> places)

And it does start after the open braces of the previous line.

The .patch format adds that '+' which messes up with the alignment,
mainly when tabs are used. Once you apply the patch you can see that the
arguments start where they should.