[v2] Add support for DS90UB954-Q1

[PATCH V2 2/4] media: i2c: ds90ub960: Use enums for chip type and chip family

Posted by Yemike Abhilash Chandra 1 week, 5 days ago

Replace chip-specific boolean flags with chip_type and chip_family enums.
This simplifies the process of adding support for newer devices and also
improves code readability.

Signed-off-by: Yemike Abhilash Chandra <y-abhilashchandra@ti.com>
---
 drivers/media/i2c/ds90ub960.c | 56 ++++++++++++++++++++++++-----------
 1 file changed, 38 insertions(+), 18 deletions(-)

diff --git a/drivers/media/i2c/ds90ub960.c b/drivers/media/i2c/ds90ub960.c
index 5a83218e64ab..45494fcaf095 100644
--- a/drivers/media/i2c/ds90ub960.c
+++ b/drivers/media/i2c/ds90ub960.c
@@ -454,12 +454,21 @@
 #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,
+};
+
+enum chip_family {
+	FAMILY_FPD3,
+	FAMILY_FPD4,
+};
+
 struct ub960_hw_data {
-	const char *model;
+	enum chip_type chip_type;
+	enum chip_family chip_family;
 	u8 num_rxports;
 	u8 num_txports;
-	bool is_ub9702;
-	bool is_fpdlink4;
 };
 
 enum ub960_rxport_mode {
@@ -1933,7 +1942,7 @@ static int ub960_rxport_wait_locks(struct ub960_data *priv,
 		if (ret)
 			return ret;
 
-		if (priv->hw_data->is_ub9702) {
+		if (priv->hw_data->chip_type == UB9702) {
 			dev_dbg(dev, "\trx%u: locked, freq %llu Hz\n",
 				nport, ((u64)v * HZ_PER_MHZ) >> 8);
 		} else {
@@ -2195,7 +2204,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);
@@ -2361,7 +2370,7 @@ static int ub960_init_tx_ports(struct ub960_data *priv)
 {
 	int ret;
 
-	if (priv->hw_data->is_ub9702)
+	if (priv->hw_data->chip_type == UB9702)
 		ret = ub960_init_tx_ports_ub9702(priv);
 	else
 		ret = ub960_init_tx_ports_ub960(priv);
@@ -3633,7 +3642,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)) |
@@ -4259,7 +4268,7 @@ static int ub960_log_status(struct v4l2_subdev *sd)
 
 		dev_info(dev, "\tcsi_err_counter %u\n", v);
 
-		if (!priv->hw_data->is_ub9702) {
+		if (priv->hw_data->chip_type != UB9702) {
 			ret = ub960_log_status_ub960_sp_eq(priv, nport);
 			if (ret)
 				return ret;
@@ -4417,7 +4426,7 @@ 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->chip_family != FAMILY_FPD4 && cdr_mode == RXPORT_CDR_FPD4) {
 		dev_err(dev, "rx%u: FPD-Link 4 CDR not supported\n", nport);
 		return -EINVAL;
 	}
@@ -4976,6 +4985,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;
@@ -5012,14 +5022,24 @@ 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, NULL);
 	if (ret)
 		goto err_pd_gpio;
 
-	if (priv->hw_data->is_ub9702)
+	if (priv->hw_data->chip_type == UB9702)
 		ret = ub960_read(priv, UB9702_SR_REFCLK_FREQ, &refclk_freq,
 				 NULL);
 	else
@@ -5038,7 +5058,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,
@@ -5111,7 +5131,7 @@ static int ub960_probe(struct i2c_client *client)
 	if (ret)
 		goto err_free_ports;
 
-	if (priv->hw_data->is_ub9702)
+	if (priv->hw_data->chip_type == UB9702)
 		ret = ub960_init_rx_ports_ub9702(priv);
 	else
 		ret = ub960_init_rx_ports_ub960(priv);
@@ -5179,17 +5199,17 @@ static void ub960_remove(struct i2c_client *client)
 }
 
 static const struct ub960_hw_data ds90ub960_hw = {
-	.model = "ub960",
+	.chip_type = UB960,
+	.chip_family = FAMILY_FPD3,
 	.num_rxports = 4,
 	.num_txports = 2,
 };
 
 static const struct ub960_hw_data ds90ub9702_hw = {
-	.model = "ub9702",
+	.chip_type = UB9702,
+	.chip_family = FAMILY_FPD4,
 	.num_rxports = 4,
 	.num_txports = 2,
-	.is_ub9702 = true,
-	.is_fpdlink4 = true,
 };
 
 static const struct i2c_device_id ub960_id[] = {
-- 
2.34.1

Re: [PATCH V2 2/4] media: i2c: ds90ub960: Use enums for chip type and chip family

Posted by Tomi Valkeinen 1 week, 2 days ago

Hi,

On 02/12/2025 12:22, Yemike Abhilash Chandra wrote:
> Replace chip-specific boolean flags with chip_type and chip_family enums.
> This simplifies the process of adding support for newer devices and also
> improves code readability.
> 
> Signed-off-by: Yemike Abhilash Chandra <y-abhilashchandra@ti.com>
> ---
>  drivers/media/i2c/ds90ub960.c | 56 ++++++++++++++++++++++++-----------
>  1 file changed, 38 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/media/i2c/ds90ub960.c b/drivers/media/i2c/ds90ub960.c
> index 5a83218e64ab..45494fcaf095 100644
> --- a/drivers/media/i2c/ds90ub960.c
> +++ b/drivers/media/i2c/ds90ub960.c
> @@ -454,12 +454,21 @@
>  #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,
> +};
> +
> +enum chip_family {
> +	FAMILY_FPD3,
> +	FAMILY_FPD4,
> +};
> +
>  struct ub960_hw_data {
> -	const char *model;
> +	enum chip_type chip_type;
> +	enum chip_family chip_family;
>  	u8 num_rxports;
>  	u8 num_txports;
> -	bool is_ub9702;
> -	bool is_fpdlink4;
>  };
>  
>  enum ub960_rxport_mode {
> @@ -1933,7 +1942,7 @@ static int ub960_rxport_wait_locks(struct ub960_data *priv,
>  		if (ret)
>  			return ret;
>  
> -		if (priv->hw_data->is_ub9702) {
> +		if (priv->hw_data->chip_type == UB9702) {
>  			dev_dbg(dev, "\trx%u: locked, freq %llu Hz\n",
>  				nport, ((u64)v * HZ_PER_MHZ) >> 8);
>  		} else {
> @@ -2195,7 +2204,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);
> @@ -2361,7 +2370,7 @@ static int ub960_init_tx_ports(struct ub960_data *priv)
>  {
>  	int ret;
>  
> -	if (priv->hw_data->is_ub9702)
> +	if (priv->hw_data->chip_type == UB9702)
>  		ret = ub960_init_tx_ports_ub9702(priv);
>  	else
>  		ret = ub960_init_tx_ports_ub960(priv);
> @@ -3633,7 +3642,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) {

While the above is correct, I think it's better to do 'if
(what-we-need-here)'. So rather check for UB960.

>  				/* 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)) |
> @@ -4259,7 +4268,7 @@ static int ub960_log_status(struct v4l2_subdev *sd)
>  
>  		dev_info(dev, "\tcsi_err_counter %u\n", v);
>  
> -		if (!priv->hw_data->is_ub9702) {
> +		if (priv->hw_data->chip_type != UB9702) {

Same here.

>  			ret = ub960_log_status_ub960_sp_eq(priv, nport);
>  			if (ret)
>  				return ret;
> @@ -4417,7 +4426,7 @@ 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->chip_family != FAMILY_FPD4 && cdr_mode == RXPORT_CDR_FPD4) {
>  		dev_err(dev, "rx%u: FPD-Link 4 CDR not supported\n", nport);
>  		return -EINVAL;
>  	}
> @@ -4976,6 +4985,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;
> @@ -5012,14 +5022,24 @@ 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, NULL);
>  	if (ret)
>  		goto err_pd_gpio;
>  
> -	if (priv->hw_data->is_ub9702)
> +	if (priv->hw_data->chip_type == UB9702)
>  		ret = ub960_read(priv, UB9702_SR_REFCLK_FREQ, &refclk_freq,
>  				 NULL);
>  	else
> @@ -5038,7 +5058,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,
> @@ -5111,7 +5131,7 @@ static int ub960_probe(struct i2c_client *client)
>  	if (ret)
>  		goto err_free_ports;
>  
> -	if (priv->hw_data->is_ub9702)
> +	if (priv->hw_data->chip_type == UB9702)
>  		ret = ub960_init_rx_ports_ub9702(priv);
>  	else
>  		ret = ub960_init_rx_ports_ub960(priv);
> @@ -5179,17 +5199,17 @@ static void ub960_remove(struct i2c_client *client)
>  }
>  
>  static const struct ub960_hw_data ds90ub960_hw = {
> -	.model = "ub960",
> +	.chip_type = UB960,
> +	.chip_family = FAMILY_FPD3,

I think we can keep the model name here. It's a bit duplicate with the
chip_type, but allows us to drop that switch-case from probe.

>  	.num_rxports = 4,
>  	.num_txports = 2,
>  };
>  
>  static const struct ub960_hw_data ds90ub9702_hw = {
> -	.model = "ub9702",
> +	.chip_type = UB9702,
> +	.chip_family = FAMILY_FPD4,
>  	.num_rxports = 4,
>  	.num_txports = 2,
> -	.is_ub9702 = true,
> -	.is_fpdlink4 = true,
>  };
>  
>  static const struct i2c_device_id ub960_id[] = {

 Tomi

[PATCH V2 1/4] media: dt-bindings: ti,ds90ub960: Refactor port definitions
[PATCH V2 2/4] media: i2c: ds90ub960: Use enums for chip type and chip family
[PATCH V2 3/4] media: dt-bindings: ti,ds90ub960: Add support for DS90UB954-Q1
[PATCH V2 4/4] media: i2c: ds90ub960: Add support for DS90UB954-Q1