diff mbox series

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

Message ID 20240830070008.9486-1-eagle.alexander923@gmail.com
State New
Headers show
Series [1/3] media: i2c: ds90ub960: Convert IC specific variables to flags | expand

Commit Message

Alexander Shiyan Aug. 30, 2024, 7 a.m. UTC 
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 <eagle.alexander923@gmail.com>
---
 drivers/media/i2c/ds90ub960.c | 48 ++++++++++++++++++++++++-----------
 1 file changed, 33 insertions(+), 15 deletions(-)

Comments

Tomi Valkeinen Sept. 27, 2024, 4:04 p.m. UTC | #1 
Hi,

On 11/09/2024 09:15, Alexander Shiyan wrote:
> Hello.
> 
> вт, 10 сент. 2024 г. в 11:40, Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>:
>> On 30/08/2024 10:00, Alexander Shiyan wrote:
>>> Add support for TI DS90UB954 FPD-Link III Deserializer.
> ...
>>> @@ -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.

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.

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

> 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
diff mbox series

Patch

diff --git a/drivers/media/i2c/ds90ub960.c b/drivers/media/i2c/ds90ub960.c
index ffe5f25f8647..e9f9abf439ee 100644
--- a/drivers/media/i2c/ds90ub960.c
+++ b/drivers/media/i2c/ds90ub960.c
@@ -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[] = {