Skip to contentSkip to conversation

media: i2c: ds90ub960: Convert IC specific variables to flags

PATCH Superseded Alexander Shiyan 2 participants 6 messages Aug 30, 2024 – Sep 27, 2024

Subsystems (2)
Media (V4L/DVB) TI fpd-link drivers

v1 of 3 Superseded Latest is v3 →

lore
Share & tools
lore

Alexander Shiyan

author
#0 lore raw

PATCH 1/3 media: i2c: ds90ub960: Convert IC specific variables to flags

This patch converts chip-specific variables into generic flags that can be used to easily add support for other chip types.

Signed-off-by Alexander Shiyan <[email protected]>
---
 drivers/media/i2c/ds90ub960.c | 48 ++++++++++++++++++++++++-----------
 1 file changed, 33 insertions(+), 15 deletions(-)
drivers/media/i2c/ds90ub960.c +33 -15
index ffe5f25f8647..e9f9abf439ee 100644
@@ -402,12 +402,18 @@
 #define UB960_MAX_EQ_LEVEL  14
 #define UB960_NUM_EQ_LEVELS (UB960_MAX_EQ_LEVEL - UB960_MIN_EQ_LEVEL + 1)
 
+enum chip_type {
+	UB960,
+	UB9702,
+};
+
+#define FLAGS_HAS_FPDLINK4			BIT(0)
+
 struct ub960_hw_data {
-	const char *model;
+	enum chip_type chip_type;
 	u8 num_rxports;
 	u8 num_txports;
-	bool is_ub9702;
-	bool is_fpdlink4;
+	u32 flags;
 };
 
 enum ub960_rxport_mode {

@@ -1654,7 +1660,7 @@ static int ub960_rxport_add_serializer(struct ub960_data *priv, u8 nport)
 
 	ser_pdata->port = nport;
 	ser_pdata->atr = priv->atr;
-	if (priv->hw_data->is_ub9702)
+	if (priv->hw_data->chip_type == UB9702)
 		ser_pdata->bc_rate = ub960_calc_bc_clk_rate_ub9702(priv, rxport);
 	else
 		ser_pdata->bc_rate = ub960_calc_bc_clk_rate_ub960(priv, rxport);

@@ -1785,7 +1791,7 @@ static int ub960_init_tx_ports(struct ub960_data *priv)
 
 	ub960_write(priv, UB960_SR_CSI_PLL_CTL, speed_select);
 
-	if (priv->hw_data->is_ub9702) {
+	if (priv->hw_data->chip_type == UB9702) {
 		ub960_write(priv, UB960_SR_CSI_PLL_DIV, pll_div);
 
 		switch (priv->tx_data_rate) {

@@ -2140,7 +2146,7 @@ static int ub960_init_rx_ports(struct ub960_data *priv)
 		if (!rxport)
 			continue;
 
-		if (priv->hw_data->is_ub9702)
+		if (priv->hw_data->chip_type == UB9702)
 			ub960_init_rx_port_ub9702(priv, rxport);
 		else
 			ub960_init_rx_port_ub960(priv, rxport);

@@ -2509,7 +2515,7 @@ static int ub960_configure_ports_for_streaming(struct ub960_data *priv,
 
 		case RXPORT_MODE_CSI2_SYNC:
 		case RXPORT_MODE_CSI2_NONSYNC:
-			if (!priv->hw_data->is_ub9702) {
+			if (priv->hw_data->chip_type != UB9702) {
 				/* Map all VCs from this port to the same VC */
 				ub960_rxport_write(priv, nport, UB960_RR_CSI_VC_MAP,
 						   (vc << UB960_RR_CSI_VC_MAP_SHIFT(3)) |

@@ -3217,7 +3223,8 @@ ub960_parse_dt_rxport_link_properties(struct ub960_data *priv,
 		return -EINVAL;
 	}
 
-	if (!priv->hw_data->is_fpdlink4 && cdr_mode == RXPORT_CDR_FPD4) {
+	if (!(priv->hw_data->flags & FLAGS_HAS_FPDLINK4) &&
+	    (cdr_mode == RXPORT_CDR_FPD4)) {
 		dev_err(dev, "rx%u: FPD-Link 4 CDR not supported\n", nport);
 		return -EINVAL;
 	}

@@ -3796,6 +3803,7 @@ static int ub960_get_hw_resources(struct ub960_data *priv)
 static int ub960_enable_core_hw(struct ub960_data *priv)
 {
 	struct device *dev = &priv->client->dev;
+	const char *model;
 	u8 rev_mask;
 	int ret;
 	u8 dev_sts;

@@ -3830,8 +3838,19 @@ static int ub960_enable_core_hw(struct ub960_data *priv)
 		goto err_pd_gpio;
 	}
 
-	dev_dbg(dev, "Found %s (rev/mask %#04x)\n", priv->hw_data->model,
-		rev_mask);
+	switch (priv->hw_data->chip_type) {
+	case UB960:
+		model = "UB960";
+		break;
+	case UB9702:
+		model = "UB9702";
+		break;
+	default:
+		model = "Unknown";
+		break;
+	};
+
+	dev_dbg(dev, "Found %s (rev/mask %#04x)\n", model, rev_mask);
 
 	ret = ub960_read(priv, UB960_SR_DEVICE_STS, &dev_sts);
 	if (ret)

@@ -3851,7 +3870,7 @@ static int ub960_enable_core_hw(struct ub960_data *priv)
 		goto err_pd_gpio;
 
 	/* release GPIO lock */
-	if (priv->hw_data->is_ub9702) {
+	if (priv->hw_data->chip_type == UB9702) {
 		ret = ub960_update_bits(priv, UB960_SR_RESET,
 					UB960_SR_RESET_GPIO_LOCK_RELEASE,
 					UB960_SR_RESET_GPIO_LOCK_RELEASE);

@@ -4013,17 +4032,16 @@ static void ub960_remove(struct i2c_client *client)
 }
 
 static const struct ub960_hw_data ds90ub960_hw = {
-	.model = "ub960",
+	.chip_type = UB960,
 	.num_rxports = 4,
 	.num_txports = 2,
 };
 
 static const struct ub960_hw_data ds90ub9702_hw = {
-	.model = "ub9702",
+	.chip_type = UB9702,
 	.num_rxports = 4,
 	.num_txports = 2,
-	.is_ub9702 = true,
-	.is_fpdlink4 = true,
+	.flags = FLAGS_HAS_FPDLINK4,
 };
 
 static const struct i2c_device_id ub960_id[] = {
Signature · 1 line
2.39.1
author
#1 lore raw

PATCH 2/3 media: i2c: ds90ub960: Add DS90UB954 support

Add support for TI DS90UB954 FPD-Link III Deserializer.

Signed-off-by Alexander Shiyan <[email protected]>
---
 drivers/media/i2c/Kconfig     |  2 +-
 drivers/media/i2c/ds90ub960.c | 52 +++++++++++++++++++++++++++++------
 2 files changed, 44 insertions(+), 10 deletions(-)
drivers/media/i2c/Kconfig +1 -1
index 8ba096b8ebca..18766898280b 100644
@@ -1604,7 +1604,7 @@ config VIDEO_DS90UB960
 	select V4L2_FWNODE
 	select VIDEO_V4L2_SUBDEV_API
 	help
-	  Device driver for the Texas Instruments DS90UB960
+	  Device driver for the Texas Instruments DS90UB954/DS90UB960
 	  FPD-Link III Deserializer and DS90UB9702 FPD-Link IV Deserializer.
 
 config VIDEO_MAX96714
drivers/media/i2c/ds90ub960.c +43 -9

116 lines not on this page · raw message

Signature · 1 line
2.39.1
maintainer
#2 lore raw

Re: [PATCH 2/3] media: i2c: ds90ub960: Add DS90UB954 support

Hi,

Quoted 40 lines · Alexander Shiyan

Add support for TI DS90UB954 FPD-Link III Deserializer.

Signed-off-by Alexander Shiyan <[email protected]>
---
  drivers/media/i2c/Kconfig     |  2 +-
  drivers/media/i2c/ds90ub960.c | 52 +++++++++++++++++++++++++++++------
  2 files changed, 44 insertions(+), 10 deletions(-)
drivers/media/i2c/Kconfig +1 -1
index 8ba096b8ebca..18766898280b 100644
@@ -1604,7 +1604,7 @@ config VIDEO_DS90UB960
  	select V4L2_FWNODE
  	select VIDEO_V4L2_SUBDEV_API
  	help
-	  Device driver for the Texas Instruments DS90UB960
+	  Device driver for the Texas Instruments DS90UB954/DS90UB960
  	  FPD-Link III Deserializer and DS90UB9702 FPD-Link IV Deserializer.
 
  config VIDEO_MAX96714
drivers/media/i2c/ds90ub960.c +1 -3
index e9f9abf439ee..9edc7e8ceebd 100644
@@ -403,6 +403,7 @@
  #define UB960_NUM_EQ_LEVELS (UB960_MAX_EQ_LEVEL - UB960_MIN_EQ_LEVEL + 1)
 
  enum chip_type {
+	UB954,
  	UB960,
  	UB9702,
  };

@@ -1154,10 +1155,17 @@ static int ub960_parse_dt_txport(struct ub960_data *priv,
  	priv->tx_link_freq[0] = vep.link_frequencies[0];
  	priv->tx_data_rate = priv->tx_link_freq[0] * 2;
 
-	if (priv->tx_data_rate != MHZ(1600) &&
-	    priv->tx_data_rate != MHZ(1200) &&
-	    priv->tx_data_rate != MHZ(800) &&

drivers/media/i2c/ds90ub960.c static int ub960_parse_dt_txport(struct ub960_data *priv, · line 1166

Quoted 10 lines
drivers/media/i2c/ds90ub960.c +9 -1
-	    priv->tx_data_rate != MHZ(400)) {
+	switch (priv->tx_data_rate) {
+	case MHZ(1600):
+	case MHZ(800):
+	case MHZ(400):
+		break;
+	case MHZ(1200):
+		/* UB954 does not support 1.2 Gbps */
+		if (priv->hw_data->chip_type != UB954)
+			break;

Here, and in a few other places, don't check for model != UB954, but rather check for model == UB954. Otherwise if we add a new chip model these won't necessarily go right.

drivers/media/i2c/ds90ub960.c static void ub960_rxport_config_eq(struct ub960_data *priv, unsigned int nport) · line 1431

Quoted 12 lines
drivers/media/i2c/ds90ub960.c +3 -1
+		fallthrough;
+	default:
  		dev_err(dev, "tx%u: invalid 'link-frequencies' value\n", nport);
  		ret = -EINVAL;
  		goto err_free_vep;

@@ -1419,7 +1427,7 @@ static void ub960_rxport_config_eq(struct ub960_data *priv, unsigned int nport)
 
  	if (priv->strobe.manual)
  		ub960_rxport_set_strobe_pos(priv, nport, rxport->eq.strobe_pos);
-	else
+	else if (priv->hw_data->chip_type != UB954)
  		ub960_rxport_set_strobe_pos(priv, nport, 0);

This looks odd. Manually set strobe pos is ok, but not the default?

What is the reason for this if?

drivers/media/i2c/ds90ub960.c static int ub960_enable_core_hw(struct ub960_data *priv) · line 3818

Quoted 8 lines
drivers/media/i2c/ds90ub960.c +1 -1
  	if (rxport->eq.manual_eq) {

@@ -3807,7 +3815,7 @@ static int ub960_enable_core_hw(struct ub960_data *priv)
  	u8 rev_mask;
  	int ret;
  	u8 dev_sts;
-	u8 refclk_freq;
+	u8 refclk_freq[2];

Instead of an array, I think the code will be clearer if you just add a new variable (refclk_freq_new?).

Quoted 17 lines
drivers/media/i2c/ds90ub960.c +3 -0
  	ret = regulator_enable(priv->vddio);
  	if (ret)

@@ -3839,6 +3847,9 @@ static int ub960_enable_core_hw(struct ub960_data *priv)
  	}
 
  	switch (priv->hw_data->chip_type) {
+	case UB954:
+		model = "UB954";
+		break;
  	case UB960:
  		model = "UB960";
  		break;

@@ -3856,12 +3867,26 @@ static int ub960_enable_core_hw(struct ub960_data *priv)
  	if (ret)
  		goto err_pd_gpio;

drivers/media/i2c/ds90ub960.c static int ub960_enable_core_hw(struct ub960_data *priv) · line 3878

Quoted 10 lines
drivers/media/i2c/ds90ub960.c +6 -1
-	ret = ub960_read(priv, UB960_XR_REFCLK_FREQ, &refclk_freq);
+	ret = ub960_read(priv, UB960_XR_REFCLK_FREQ, &refclk_freq[0]);
  	if (ret)
  		goto err_pd_gpio;
 
+	/* From DS90UB954-Q1 datasheet:
+	 * "REFCLK_FREQ measurement is not synchronized. Value in this register
+	 * should read twice and only considered valid if
+	 * REFCLK_FREQ is unchanged between reads."
+	*/

The coding style says the multiline comments are like:

/*
* Foo
*/

drivers/media/i2c/ds90ub960.c static int ub960_enable_core_hw(struct ub960_data *priv) · line 3886

Quoted 8 lines
drivers/media/i2c/ds90ub960.c +8 -0
+	while (priv->hw_data->chip_type == UB954) {
+		ret = ub960_read(priv, UB960_XR_REFCLK_FREQ, &refclk_freq[1]);
+		if (ret)
+			goto err_pd_gpio;
+		if (refclk_freq[0] == refclk_freq[1])
+			break;
+		refclk_freq[0] = refclk_freq[1];
+	};

This is potentially an infinite loop, which is not a good idea. Also, don't loop with "while (priv->hw_data->chip_type == UB954)"... Just use an if for the chip_type, and loop with a proper condition.

Quoted 8 lines
drivers/media/i2c/ds90ub960.c +2 -1
+
  	dev_dbg(dev, "refclk valid %u freq %u MHz (clk fw freq %lu MHz)\n",
-		!!(dev_sts & BIT(4)), refclk_freq,
+		!!(dev_sts & BIT(4)), refclk_freq[0],
  		clk_get_rate(priv->refclk) / 1000000);
 
  	/* Disable all RX ports by default */

drivers/media/i2c/ds90ub960.c static int ub960_probe(struct i2c_client *client) · line 3952

Quoted 6 lines
drivers/media/i2c/ds90ub960.c +2 -1
@@ -3923,7 +3948,8 @@ static int ub960_probe(struct i2c_client *client)
  	 */
  	priv->reg_current.indirect_target = 0xff;
  	priv->reg_current.rxport = 0xff;
-	priv->reg_current.txport = 0xff;
+	/* Avoid using UB960_SR_CSI_PORT_SEL register for single TX channel */
+	priv->reg_current.txport = priv->hw_data->num_txports > 1 ? 0xff : 0x00;

No, don't do this. Just do a proper check in ub960_txport_select() and skip the reg write there.

Tomi

Quoted 31 lines
drivers/media/i2c/ds90ub960.c +8 -0
  	ret = ub960_get_hw_resources(priv);
  	if (ret)

@@ -4031,6 +4057,12 @@ static void ub960_remove(struct i2c_client *client)
  	mutex_destroy(&priv->reg_lock);
  }
 
+static const struct ub960_hw_data ds90ub954_hw = {
+	.chip_type = UB954,
+	.num_rxports = 2,
+	.num_txports = 1,
+};
+
  static const struct ub960_hw_data ds90ub960_hw = {
  	.chip_type = UB960,
  	.num_rxports = 4,

@@ -4045,6 +4077,7 @@ static const struct ub960_hw_data ds90ub9702_hw = {
  };
 
  static const struct i2c_device_id ub960_id[] = {
+	{ "ds90ub954-q1", (kernel_ulong_t)&ds90ub954_hw },
  	{ "ds90ub960-q1", (kernel_ulong_t)&ds90ub960_hw },
  	{ "ds90ub9702-q1", (kernel_ulong_t)&ds90ub9702_hw },
  	{}

@@ -4052,6 +4085,7 @@ static const struct i2c_device_id ub960_id[] = {
  MODULE_DEVICE_TABLE(i2c, ub960_id);
 
  static const struct of_device_id ub960_dt_ids[] = {
+	{ .compatible = "ti,ds90ub954-q1", .data = &ds90ub954_hw },
  	{ .compatible = "ti,ds90ub960-q1", .data = &ds90ub960_hw },
  	{ .compatible = "ti,ds90ub9702-q1", .data = &ds90ub9702_hw },
  	{}
author
#3 lore raw

Re: [PATCH 2/3] media: i2c: ds90ub960: Add DS90UB954 support

Hello.

вт, 10 сент. 2024 г. в 11:40, Tomi Valkeinen <[email protected]>:

Quoted 2 lines · Alexander Shiyan
Quoted 1 line

Add support for TI DS90UB954 FPD-Link III Deserializer.

...

Quoted 10 lines

drivers/media/i2c/ds90ub960.c static void ub960_rxport_config_eq(struct ub960_data *priv, unsigned int nport) · line 1431

Quoted 7 lines
drivers/media/i2c/ds90ub960.c +1 -1
@@ -1419,7 +1427,7 @@ static void ub960_rxport_config_eq(struct ub960_data *priv, unsigned int nport)
 
      if (priv->strobe.manual)
              ub960_rxport_set_strobe_pos(priv, nport, rxport->eq.strobe_pos);
-     else
+     else if (priv->hw_data->chip_type != UB954)
              ub960_rxport_set_strobe_pos(priv, nport, 0);

This looks odd. Manually set strobe pos is ok, but not the default? What is the reason for this if?

In fact, these registers are described as reserved in the datasheet. (We are speaking about indirect page 1).
Here is an excerpt from datasheet UB960:
Indirect Access Register Select:
Selects target for register access
0000: Pattern Generator and CSI-2 Timing (PATGEN_AND_CSI-2) Registers
xxxx: RESERVED

In UB954 datasheet this area is described as "FPD-Link III Channel 0 Reserved Registers: Test and Debug registers".

I tested the UB954 and when writing to this area an error occurs and the chip no longer responds.
When disabling ub960_rxport_set_strobe_pos() everything works as expected.

maintainer
#4 lore raw

Re: [PATCH 2/3] media: i2c: ds90ub960: Add DS90UB954 support

Hi,

Quoted 17 lines · Alexander Shiyan

Hello.

вт, 10 сент. 2024 г. в 11:40, Tomi Valkeinen <[email protected]>:

Quoted 2 lines · Alexander Shiyan
Quoted 1 line

Add support for TI DS90UB954 FPD-Link III Deserializer.

...

Quoted 10 lines

drivers/media/i2c/ds90ub960.c static void ub960_rxport_config_eq(struct ub960_data *priv, unsigned int nport) · line 1431

Quoted 7 lines
drivers/media/i2c/ds90ub960.c +1 -1
@@ -1419,7 +1427,7 @@ static void ub960_rxport_config_eq(struct ub960_data *priv, unsigned int nport)
 
       if (priv->strobe.manual)
               ub960_rxport_set_strobe_pos(priv, nport, rxport->eq.strobe_pos);
-     else
+     else if (priv->hw_data->chip_type != UB954)
               ub960_rxport_set_strobe_pos(priv, nport, 0);

This looks odd. Manually set strobe pos is ok, but not the default? What is the reason for this if?

Quoted 1 line

In fact, these registers are described as reserved in the datasheet.

Yes, but my point was that in your patch you disable the call to ub960_rxport_set_strobe_pos() if manual strobe is not set, but still allow it if manual strobe is set.

Quoted 9 lines

(We are speaking about indirect page 1).
Here is an excerpt from datasheet UB960:
Indirect Access Register Select:
Selects target for register access
0000: Pattern Generator and CSI-2 Timing (PATGEN_AND_CSI-2) Registers
xxxx: RESERVED

In UB954 datasheet this area is described as "FPD-Link III Channel 0 Reserved Registers: Test and Debug registers".

They're marked reserved in the UB960 datasheet too.

Quoted 3 lines

I tested the UB954 and when writing to this area an error occurs and the chip no longer responds.
When disabling ub960_rxport_set_strobe_pos() everything works as expected.

"Margin Analysis Program (MAP) and strobe positions for DS90UB954-Q1 and DS90UB960-Q1" describes the registers:

https://www.ti.com/lit/pdf/snla301

It's been a while since I worked on this, but I remember having some trouble fitting the docs to what actually worked. And there was some small diff between UB954 and UB960, if I'm not mistaken.

You might be able to sort it out with the doc above, but if not or if you're not interested, I'm fine with marking the relevant code for UB960 only. But be careful to disable the code in all the code paths.

Tomi

maintainer
#5 lore raw
In reply to Alexander Shiyan

Hi,

Quoted 21 lines · Alexander Shiyan

This patch converts chip-specific variables into generic flags that can be used to easily add support for other chip types.

Signed-off-by Alexander Shiyan <[email protected]>
---
  drivers/media/i2c/ds90ub960.c | 48 ++++++++++++++++++++++++-----------
  1 file changed, 33 insertions(+), 15 deletions(-)
drivers/media/i2c/ds90ub960.c +5 -0
index ffe5f25f8647..e9f9abf439ee 100644
@@ -402,12 +402,18 @@
  #define UB960_MAX_EQ_LEVEL  14
  #define UB960_NUM_EQ_LEVELS (UB960_MAX_EQ_LEVEL - UB960_MIN_EQ_LEVEL + 1)
 
+enum chip_type {
+	UB960,
+	UB9702,
+};
+

drivers/media/i2c/ds90ub960.c line 416

Quoted 10 lines
drivers/media/i2c/ds90ub960.c +4 -3
+#define FLAGS_HAS_FPDLINK4			BIT(0)
+
  struct ub960_hw_data {
-	const char *model;
+	enum chip_type chip_type;
  	u8 num_rxports;
  	u8 num_txports;
-	bool is_ub9702;
-	bool is_fpdlink4;
+	u32 flags;

Let's drop the is_fpdlink4/FLAGS_HAS_FPDLINK4. It's only used in one place, and just checking for the model == UB9702 is fine.

While you're at this, can you go through the code for ifs/selects for chip_type, and change the code so that we don't have plain "else" blocks there. I.e. in each chip_type test we would be explicitly testing for matching chip types, and _not_ do things like:

if (ub960)

	xyz-for-ub960;
else
	must-be-ub9702;

Also, with the above change, and adding UB954, I wonder if it would make the code simpler if we added a "chip family" property. The UB960 & UB954 are clearly part of the same family, whereas UB9702 (and other FPD-Link 4 desers which the driver doesn't support) are another. As we don't have a good name for them, maybe just FAMILY_FPD3 and FAMILY_FPD4.

Tomi

Quoted 109 lines
drivers/media/i2c/ds90ub960.c +24 -12
  };
 
  enum ub960_rxport_mode {

@@ -1654,7 +1660,7 @@ static int ub960_rxport_add_serializer(struct ub960_data *priv, u8 nport)
 
  	ser_pdata->port = nport;
  	ser_pdata->atr = priv->atr;
-	if (priv->hw_data->is_ub9702)
+	if (priv->hw_data->chip_type == UB9702)
  		ser_pdata->bc_rate = ub960_calc_bc_clk_rate_ub9702(priv, rxport);
  	else
  		ser_pdata->bc_rate = ub960_calc_bc_clk_rate_ub960(priv, rxport);

@@ -1785,7 +1791,7 @@ static int ub960_init_tx_ports(struct ub960_data *priv)
 
  	ub960_write(priv, UB960_SR_CSI_PLL_CTL, speed_select);
 
-	if (priv->hw_data->is_ub9702) {
+	if (priv->hw_data->chip_type == UB9702) {
  		ub960_write(priv, UB960_SR_CSI_PLL_DIV, pll_div);
 
  		switch (priv->tx_data_rate) {

@@ -2140,7 +2146,7 @@ static int ub960_init_rx_ports(struct ub960_data *priv)
  		if (!rxport)
  			continue;
 
-		if (priv->hw_data->is_ub9702)
+		if (priv->hw_data->chip_type == UB9702)
  			ub960_init_rx_port_ub9702(priv, rxport);
  		else
  			ub960_init_rx_port_ub960(priv, rxport);

@@ -2509,7 +2515,7 @@ static int ub960_configure_ports_for_streaming(struct ub960_data *priv,
 
  		case RXPORT_MODE_CSI2_SYNC:
  		case RXPORT_MODE_CSI2_NONSYNC:
-			if (!priv->hw_data->is_ub9702) {
+			if (priv->hw_data->chip_type != UB9702) {
  				/* Map all VCs from this port to the same VC */
  				ub960_rxport_write(priv, nport, UB960_RR_CSI_VC_MAP,
  						   (vc << UB960_RR_CSI_VC_MAP_SHIFT(3)) |

@@ -3217,7 +3223,8 @@ ub960_parse_dt_rxport_link_properties(struct ub960_data *priv,
  		return -EINVAL;
  	}
 
-	if (!priv->hw_data->is_fpdlink4 && cdr_mode == RXPORT_CDR_FPD4) {
+	if (!(priv->hw_data->flags & FLAGS_HAS_FPDLINK4) &&
+	    (cdr_mode == RXPORT_CDR_FPD4)) {
  		dev_err(dev, "rx%u: FPD-Link 4 CDR not supported\n", nport);
  		return -EINVAL;
  	}

@@ -3796,6 +3803,7 @@ static int ub960_get_hw_resources(struct ub960_data *priv)
  static int ub960_enable_core_hw(struct ub960_data *priv)
  {
  	struct device *dev = &priv->client->dev;
+	const char *model;
  	u8 rev_mask;
  	int ret;
  	u8 dev_sts;

@@ -3830,8 +3838,19 @@ static int ub960_enable_core_hw(struct ub960_data *priv)
  		goto err_pd_gpio;
  	}
 
-	dev_dbg(dev, "Found %s (rev/mask %#04x)\n", priv->hw_data->model,
-		rev_mask);
+	switch (priv->hw_data->chip_type) {
+	case UB960:
+		model = "UB960";
+		break;
+	case UB9702:
+		model = "UB9702";
+		break;
+	default:
+		model = "Unknown";
+		break;
+	};
+
+	dev_dbg(dev, "Found %s (rev/mask %#04x)\n", model, rev_mask);
 
  	ret = ub960_read(priv, UB960_SR_DEVICE_STS, &dev_sts);
  	if (ret)

@@ -3851,7 +3870,7 @@ static int ub960_enable_core_hw(struct ub960_data *priv)
  		goto err_pd_gpio;
 
  	/* release GPIO lock */
-	if (priv->hw_data->is_ub9702) {
+	if (priv->hw_data->chip_type == UB9702) {
  		ret = ub960_update_bits(priv, UB960_SR_RESET,
  					UB960_SR_RESET_GPIO_LOCK_RELEASE,
  					UB960_SR_RESET_GPIO_LOCK_RELEASE);

@@ -4013,17 +4032,16 @@ static void ub960_remove(struct i2c_client *client)
  }
 
  static const struct ub960_hw_data ds90ub960_hw = {
-	.model = "ub960",
+	.chip_type = UB960,
  	.num_rxports = 4,
  	.num_txports = 2,
  };
 
  static const struct ub960_hw_data ds90ub9702_hw = {
-	.model = "ub9702",
+	.chip_type = UB9702,
  	.num_rxports = 4,
  	.num_txports = 2,
-	.is_ub9702 = true,
-	.is_fpdlink4 = true,
+	.flags = FLAGS_HAS_FPDLINK4,
  };
 
  static const struct i2c_device_id ub960_id[] = {