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

[Yemike Abhilash Chandra <y-abhilashchandra@ti.com>]

Hi Tomi,
Thanks for the review.

On 05/12/25 16:16, Tomi Valkeinen wrote:
> 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.
> 

I will change this in v3.

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

Understood, I will keep the model name here and drop the switch case.

Thanks and Regards,
Yemike Abhilash Chandra

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