From: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
To: Yemike Abhilash Chandra <y-abhilashchandra@ti.com>
Cc: hansg@kernel.org, mehdi.djait@linux.intel.com,
ribalda@chromium.org, git@apitzsch.eu,
vladimir.zapolskiy@linaro.org, benjamin.mugnier@foss.st.com,
dongcheng.yan@intel.com, u-kumar1@ti.com, jai.luthra@linux.dev,
linux-media@vger.kernel.org, devicetree@vger.kernel.org,
linux-kernel@vger.kernel.org, mchehab@kernel.org,
robh@kernel.org, krzk+dt@kernel.org, conor+dt@kernel.org,
hverkuil@xs4all.nl, sakari.ailus@linux.intel.com,
laurent.pinchart@ideasonboard.com
Subject: Re: [PATCH V2 2/4] media: i2c: ds90ub960: Use enums for chip type and chip family
Date: Fri, 5 Dec 2025 12:46:18 +0200 [thread overview]
Message-ID: <3fb1b12c-2f54-4bdf-8a33-a42b4852d651@ideasonboard.com> (raw)
In-Reply-To: <20251202102208.80713-3-y-abhilashchandra@ti.com>
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
next prev parent reply other threads:[~2025-12-05 10:46 UTC|newest]
Thread overview: 13+ messages / expand[flat|nested] mbox.gz Atom feed top
2025-12-02 10:22 [PATCH V2 0/4] Add support for DS90UB954-Q1 Yemike Abhilash Chandra
2025-12-02 10:22 ` [PATCH V2 1/4] media: dt-bindings: ti,ds90ub960: Refactor port definitions Yemike Abhilash Chandra
2025-12-05 15:11 ` Rob Herring
2025-12-10 9:25 ` Yemike Abhilash Chandra
2025-12-02 10:22 ` [PATCH V2 2/4] media: i2c: ds90ub960: Use enums for chip type and chip family Yemike Abhilash Chandra
2025-12-05 10:46 ` Tomi Valkeinen [this message]
2025-12-18 10:24 ` Yemike Abhilash Chandra
2025-12-02 10:22 ` [PATCH V2 3/4] media: dt-bindings: ti,ds90ub960: Add support for DS90UB954-Q1 Yemike Abhilash Chandra
2025-12-05 15:17 ` Rob Herring
2025-12-10 9:33 ` Yemike Abhilash Chandra
2025-12-02 10:22 ` [PATCH V2 4/4] media: i2c: ds90ub960: " Yemike Abhilash Chandra
2025-12-05 11:10 ` Tomi Valkeinen
2025-12-18 10:39 ` Yemike Abhilash Chandra
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=3fb1b12c-2f54-4bdf-8a33-a42b4852d651@ideasonboard.com \
--to=tomi.valkeinen@ideasonboard.com \
--cc=benjamin.mugnier@foss.st.com \
--cc=conor+dt@kernel.org \
--cc=devicetree@vger.kernel.org \
--cc=dongcheng.yan@intel.com \
--cc=git@apitzsch.eu \
--cc=hansg@kernel.org \
--cc=hverkuil@xs4all.nl \
--cc=jai.luthra@linux.dev \
--cc=krzk+dt@kernel.org \
--cc=laurent.pinchart@ideasonboard.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-media@vger.kernel.org \
--cc=mchehab@kernel.org \
--cc=mehdi.djait@linux.intel.com \
--cc=ribalda@chromium.org \
--cc=robh@kernel.org \
--cc=sakari.ailus@linux.intel.com \
--cc=u-kumar1@ti.com \
--cc=vladimir.zapolskiy@linaro.org \
--cc=y-abhilashchandra@ti.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
all inboxes | Powered by JetHome®