[1/3] media: i2c: ds90ub960: Convert IC specific variables to flags
Commit Message
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
Hi,
On 30/08/2024 10:00, Alexander Shiyan wrote:
> 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(-)
>
> 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;
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
> };
>
> 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[] = {
@@ -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[] = {