This series fixes various small issues in the drivers, and adds a few things (a couple of pixel formats and a debugging feature).
It also takes a few steps in adding more i2c read/write error handlings to the drivers, but covers only the easy places.
Adding error handling to all reads/writes needs more thinking, perhaps adding a "ret" parameter to the calls, similar to the cci_* functions, or perhaps adding helpers for writing multiple registers from a given table. Also, in some places rolling back from an error will require work.
Signed-off-by: Tomi Valkeinen tomi.valkeinen@ideasonboard.com --- Changes in v4: - Add Jai's Rb - Use HZ_PER_MHZ in MHZ() macro - Use num_rxports when setting up the DEBUG_I2C_RX_ID - Add Reported-by's to patches that add error handling. Note: The patches don't close the issue, so I use "Link:" instead of "Closes:" as directed in Documentation/process/5.Posting.rst. However, checkpatch seems to want "Closes", so it warns about these. - Link to v3: https://lore.kernel.org/r/20241204-ub9xx-fixes-v3-0-a933c109b323@ideasonboar...
Changes in v3: - Include bitfield.h for FIELD_PREP() - Cc stable for relevant fixes - Link to v2: https://lore.kernel.org/r/20241108-ub9xx-fixes-v2-0-c7db3b2ad89f@ideasonboar...
Changes in v2: - Address comments from Andy - Add two new patches: - media: i2c: ds90ub960: Fix shadowing of local variables - media: i2c: ds90ub960: Use HZ_PER_MHZ - Link to v1: https://lore.kernel.org/r/20241004-ub9xx-fixes-v1-0-e30a4633c786@ideasonboar...
--- Tomi Valkeinen (15): media: i2c: ds90ub9x3: Fix extra fwnode_handle_put() media: i2c: ds90ub960: Fix UB9702 refclk register access media: i2c: ds90ub960: Fix use of non-existing registers on UB9702 media: i2c: ds90ub960: Fix logging SP & EQ status only for UB9702 media: i2c: ds90ub960: Fix UB9702 VC map media: i2c: ds90ub960: Use HZ_PER_MHZ media: i2c: ds90ub960: Add support for I2C_RX_ID media: i2c: ds90ub960: Add RGB24, RAW8 and RAW10 formats media: i2c: ds90ub953: Clear CRC errors in ub953_log_status() media: i2c: ds90ub960: Drop unused indirect block define media: i2c: ds90ub960: Reduce sleep in ub960_rxport_wait_locks() media: i2c: ds90ub960: Handle errors in ub960_log_status_ub960_sp_eq() media: i2c: ds90ub913: Add error handling to ub913_hw_init() media: i2c: ds90ub953: Add error handling for i2c reads/writes media: i2c: ds90ub960: Fix shadowing of local variables
drivers/media/i2c/ds90ub913.c | 26 ++++-- drivers/media/i2c/ds90ub953.c | 56 +++++++++---- drivers/media/i2c/ds90ub960.c | 188 ++++++++++++++++++++++++++++-------------- 3 files changed, 188 insertions(+), 82 deletions(-) --- base-commit: adc218676eef25575469234709c2d87185ca223a change-id: 20241004-ub9xx-fixes-bba80dc48627
Best regards,
The ub913 and ub953 drivers call fwnode_handle_put(priv->sd.fwnode) as part of their remove process, and if the driver is removed multiple times, eventually leads to put "overflow", possibly causing memory corruption or crash.
The fwnode_handle_put() is a leftover from commit 905f88ccebb1 ("media: i2c: ds90ub9x3: Fix sub-device matching"), which changed the code related to the sd.fwnode, but missed removing these fwnode_handle_put() calls.
Cc: stable@vger.kernel.org Fixes: 905f88ccebb1 ("media: i2c: ds90ub9x3: Fix sub-device matching") Reviewed-by: Jai Luthra jai.luthra@ideasonboard.com Signed-off-by: Tomi Valkeinen tomi.valkeinen@ideasonboard.com --- drivers/media/i2c/ds90ub913.c | 1 - drivers/media/i2c/ds90ub953.c | 1 - 2 files changed, 2 deletions(-)
diff --git a/drivers/media/i2c/ds90ub913.c b/drivers/media/i2c/ds90ub913.c index 8eed4a200fd8..b5375d736629 100644 --- a/drivers/media/i2c/ds90ub913.c +++ b/drivers/media/i2c/ds90ub913.c @@ -793,7 +793,6 @@ static void ub913_subdev_uninit(struct ub913_data *priv) v4l2_async_unregister_subdev(&priv->sd); ub913_v4l2_nf_unregister(priv); v4l2_subdev_cleanup(&priv->sd); - fwnode_handle_put(priv->sd.fwnode); media_entity_cleanup(&priv->sd.entity); }
diff --git a/drivers/media/i2c/ds90ub953.c b/drivers/media/i2c/ds90ub953.c index 16f88db14981..10daecf6f457 100644 --- a/drivers/media/i2c/ds90ub953.c +++ b/drivers/media/i2c/ds90ub953.c @@ -1291,7 +1291,6 @@ static void ub953_subdev_uninit(struct ub953_data *priv) v4l2_async_unregister_subdev(&priv->sd); ub953_v4l2_notifier_unregister(priv); v4l2_subdev_cleanup(&priv->sd); - fwnode_handle_put(priv->sd.fwnode); media_entity_cleanup(&priv->sd.entity); }
UB9702 has the refclk freq register at a different offset than UB960, but the code uses the UB960's offset for both chips. Fix this.
The refclk freq is only used for a debug print, so there's no functional change here.
Cc: stable@vger.kernel.org Fixes: afe267f2d368 ("media: i2c: add DS90UB960 driver") Reviewed-by: Jai Luthra jai.luthra@ideasonboard.com Signed-off-by: Tomi Valkeinen tomi.valkeinen@ideasonboard.com --- drivers/media/i2c/ds90ub960.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/drivers/media/i2c/ds90ub960.c b/drivers/media/i2c/ds90ub960.c index ffe5f25f8647..b1e848678218 100644 --- a/drivers/media/i2c/ds90ub960.c +++ b/drivers/media/i2c/ds90ub960.c @@ -352,6 +352,8 @@
#define UB960_SR_I2C_RX_ID(n) (0xf8 + (n)) /* < UB960_FPD_RX_NPORTS */
+#define UB9702_SR_REFCLK_FREQ 0x3d + /* Indirect register blocks */ #define UB960_IND_TARGET_PAT_GEN 0x00 #define UB960_IND_TARGET_RX_ANA(n) (0x01 + (n)) @@ -3837,7 +3839,10 @@ static int ub960_enable_core_hw(struct ub960_data *priv) if (ret) goto err_pd_gpio;
- ret = ub960_read(priv, UB960_XR_REFCLK_FREQ, &refclk_freq); + if (priv->hw_data->is_ub9702) + ret = ub960_read(priv, UB9702_SR_REFCLK_FREQ, &refclk_freq); + else + ret = ub960_read(priv, UB960_XR_REFCLK_FREQ, &refclk_freq); if (ret) goto err_pd_gpio;
UB9702 doesn't have the registers for SP and EQ. Adjust the code in ub960_rxport_wait_locks() to not use those registers for UB9702. As these values are only used for a debug print here, there's no functional change.
Cc: stable@vger.kernel.org Fixes: afe267f2d368 ("media: i2c: add DS90UB960 driver") Reviewed-by: Jai Luthra jai.luthra@ideasonboard.com Signed-off-by: Tomi Valkeinen tomi.valkeinen@ideasonboard.com --- drivers/media/i2c/ds90ub960.c | 24 ++++++++++++++++-------- 1 file changed, 16 insertions(+), 8 deletions(-)
diff --git a/drivers/media/i2c/ds90ub960.c b/drivers/media/i2c/ds90ub960.c index b1e848678218..24198b803eff 100644 --- a/drivers/media/i2c/ds90ub960.c +++ b/drivers/media/i2c/ds90ub960.c @@ -1577,16 +1577,24 @@ static int ub960_rxport_wait_locks(struct ub960_data *priv,
ub960_rxport_read16(priv, nport, UB960_RR_RX_FREQ_HIGH, &v);
- ret = ub960_rxport_get_strobe_pos(priv, nport, &strobe_pos); - if (ret) - return ret; + if (priv->hw_data->is_ub9702) { + dev_dbg(dev, "\trx%u: locked, freq %llu Hz\n", + nport, (v * 1000000ULL) >> 8); + } else { + ret = ub960_rxport_get_strobe_pos(priv, nport, + &strobe_pos); + if (ret) + return ret;
- ret = ub960_rxport_get_eq_level(priv, nport, &eq_level); - if (ret) - return ret; + ret = ub960_rxport_get_eq_level(priv, nport, &eq_level); + if (ret) + return ret;
- dev_dbg(dev, "\trx%u: locked, SP: %d, EQ: %u, freq %llu Hz\n", - nport, strobe_pos, eq_level, (v * 1000000ULL) >> 8); + dev_dbg(dev, + "\trx%u: locked, SP: %d, EQ: %u, freq %llu Hz\n", + nport, strobe_pos, eq_level, + (v * 1000000ULL) >> 8); + } }
return 0;
UB9702 does not have SP and EQ registers, but the driver uses them in log_status(). Fix this by separating the SP and EQ related log_status() work into a separate function (for clarity) and calling that function only for UB960.
Cc: stable@vger.kernel.org Fixes: afe267f2d368 ("media: i2c: add DS90UB960 driver") Reviewed-by: Jai Luthra jai.luthra@ideasonboard.com Signed-off-by: Tomi Valkeinen tomi.valkeinen@ideasonboard.com --- drivers/media/i2c/ds90ub960.c | 90 ++++++++++++++++++++++++------------------- 1 file changed, 50 insertions(+), 40 deletions(-)
diff --git a/drivers/media/i2c/ds90ub960.c b/drivers/media/i2c/ds90ub960.c index 24198b803eff..94c8acf171b4 100644 --- a/drivers/media/i2c/ds90ub960.c +++ b/drivers/media/i2c/ds90ub960.c @@ -2950,6 +2950,54 @@ static const struct v4l2_subdev_pad_ops ub960_pad_ops = { .set_fmt = ub960_set_fmt, };
+static void ub960_log_status_ub960_sp_eq(struct ub960_data *priv, + unsigned int nport) +{ + struct device *dev = &priv->client->dev; + u8 eq_level; + s8 strobe_pos; + u8 v = 0; + + /* Strobe */ + + ub960_read(priv, UB960_XR_AEQ_CTL1, &v); + + dev_info(dev, "\t%s strobe\n", + (v & UB960_XR_AEQ_CTL1_AEQ_SFILTER_EN) ? "Adaptive" : + "Manual"); + + if (v & UB960_XR_AEQ_CTL1_AEQ_SFILTER_EN) { + ub960_read(priv, UB960_XR_SFILTER_CFG, &v); + + dev_info(dev, "\tStrobe range [%d, %d]\n", + ((v >> UB960_XR_SFILTER_CFG_SFILTER_MIN_SHIFT) & 0xf) - 7, + ((v >> UB960_XR_SFILTER_CFG_SFILTER_MAX_SHIFT) & 0xf) - 7); + } + + ub960_rxport_get_strobe_pos(priv, nport, &strobe_pos); + + dev_info(dev, "\tStrobe pos %d\n", strobe_pos); + + /* EQ */ + + ub960_rxport_read(priv, nport, UB960_RR_AEQ_BYPASS, &v); + + dev_info(dev, "\t%s EQ\n", + (v & UB960_RR_AEQ_BYPASS_ENABLE) ? "Manual" : + "Adaptive"); + + if (!(v & UB960_RR_AEQ_BYPASS_ENABLE)) { + ub960_rxport_read(priv, nport, UB960_RR_AEQ_MIN_MAX, &v); + + dev_info(dev, "\tEQ range [%u, %u]\n", + (v >> UB960_RR_AEQ_MIN_MAX_AEQ_FLOOR_SHIFT) & 0xf, + (v >> UB960_RR_AEQ_MIN_MAX_AEQ_MAX_SHIFT) & 0xf); + } + + if (ub960_rxport_get_eq_level(priv, nport, &eq_level) == 0) + dev_info(dev, "\tEQ level %u\n", eq_level); +} + static int ub960_log_status(struct v4l2_subdev *sd) { struct ub960_data *priv = sd_to_ub960(sd); @@ -2997,8 +3045,6 @@ static int ub960_log_status(struct v4l2_subdev *sd)
for (nport = 0; nport < priv->hw_data->num_rxports; nport++) { struct ub960_rxport *rxport = priv->rxports[nport]; - u8 eq_level; - s8 strobe_pos; unsigned int i;
dev_info(dev, "RX %u\n", nport); @@ -3034,44 +3080,8 @@ static int ub960_log_status(struct v4l2_subdev *sd) ub960_rxport_read(priv, nport, UB960_RR_CSI_ERR_COUNTER, &v); dev_info(dev, "\tcsi_err_counter %u\n", v);
- /* Strobe */ - - ub960_read(priv, UB960_XR_AEQ_CTL1, &v); - - dev_info(dev, "\t%s strobe\n", - (v & UB960_XR_AEQ_CTL1_AEQ_SFILTER_EN) ? "Adaptive" : - "Manual"); - - if (v & UB960_XR_AEQ_CTL1_AEQ_SFILTER_EN) { - ub960_read(priv, UB960_XR_SFILTER_CFG, &v); - - dev_info(dev, "\tStrobe range [%d, %d]\n", - ((v >> UB960_XR_SFILTER_CFG_SFILTER_MIN_SHIFT) & 0xf) - 7, - ((v >> UB960_XR_SFILTER_CFG_SFILTER_MAX_SHIFT) & 0xf) - 7); - } - - ub960_rxport_get_strobe_pos(priv, nport, &strobe_pos); - - dev_info(dev, "\tStrobe pos %d\n", strobe_pos); - - /* EQ */ - - ub960_rxport_read(priv, nport, UB960_RR_AEQ_BYPASS, &v); - - dev_info(dev, "\t%s EQ\n", - (v & UB960_RR_AEQ_BYPASS_ENABLE) ? "Manual" : - "Adaptive"); - - if (!(v & UB960_RR_AEQ_BYPASS_ENABLE)) { - ub960_rxport_read(priv, nport, UB960_RR_AEQ_MIN_MAX, &v); - - dev_info(dev, "\tEQ range [%u, %u]\n", - (v >> UB960_RR_AEQ_MIN_MAX_AEQ_FLOOR_SHIFT) & 0xf, - (v >> UB960_RR_AEQ_MIN_MAX_AEQ_MAX_SHIFT) & 0xf); - } - - if (ub960_rxport_get_eq_level(priv, nport, &eq_level) == 0) - dev_info(dev, "\tEQ level %u\n", eq_level); + if (!priv->hw_data->is_ub9702) + ub960_log_status_ub960_sp_eq(priv, nport);
/* GPIOs */ for (i = 0; i < UB960_NUM_BC_GPIOS; i++) {
Huomenta,
On Fri, Dec 06, 2024 at 10:26:40AM +0200, Tomi Valkeinen wrote:
UB9702 does not have SP and EQ registers, but the driver uses them in log_status(). Fix this by separating the SP and EQ related log_status() work into a separate function (for clarity) and calling that function only for UB960.
Cc: stable@vger.kernel.org Fixes: afe267f2d368 ("media: i2c: add DS90UB960 driver") Reviewed-by: Jai Luthra jai.luthra@ideasonboard.com Signed-off-by: Tomi Valkeinen tomi.valkeinen@ideasonboard.com
drivers/media/i2c/ds90ub960.c | 90 ++++++++++++++++++++++++------------------- 1 file changed, 50 insertions(+), 40 deletions(-)
diff --git a/drivers/media/i2c/ds90ub960.c b/drivers/media/i2c/ds90ub960.c index 24198b803eff..94c8acf171b4 100644 --- a/drivers/media/i2c/ds90ub960.c +++ b/drivers/media/i2c/ds90ub960.c @@ -2950,6 +2950,54 @@ static const struct v4l2_subdev_pad_ops ub960_pad_ops = { .set_fmt = ub960_set_fmt, }; +static void ub960_log_status_ub960_sp_eq(struct ub960_data *priv,
unsigned int nport)+{
- struct device *dev = &priv->client->dev;
- u8 eq_level;
- s8 strobe_pos;
- u8 v = 0;
- /* Strobe */
- ub960_read(priv, UB960_XR_AEQ_CTL1, &v);
How about adding __must_check to the ub960_read()?
- dev_info(dev, "\t%s strobe\n",
(v & UB960_XR_AEQ_CTL1_AEQ_SFILTER_EN) ? "Adaptive" :"Manual");- if (v & UB960_XR_AEQ_CTL1_AEQ_SFILTER_EN) {
ub960_read(priv, UB960_XR_SFILTER_CFG, &v);dev_info(dev, "\tStrobe range [%d, %d]\n",((v >> UB960_XR_SFILTER_CFG_SFILTER_MIN_SHIFT) & 0xf) - 7,((v >> UB960_XR_SFILTER_CFG_SFILTER_MAX_SHIFT) & 0xf) - 7);- }
- ub960_rxport_get_strobe_pos(priv, nport, &strobe_pos);
- dev_info(dev, "\tStrobe pos %d\n", strobe_pos);
- /* EQ */
- ub960_rxport_read(priv, nport, UB960_RR_AEQ_BYPASS, &v);
- dev_info(dev, "\t%s EQ\n",
(v & UB960_RR_AEQ_BYPASS_ENABLE) ? "Manual" :"Adaptive");- if (!(v & UB960_RR_AEQ_BYPASS_ENABLE)) {
ub960_rxport_read(priv, nport, UB960_RR_AEQ_MIN_MAX, &v);dev_info(dev, "\tEQ range [%u, %u]\n",(v >> UB960_RR_AEQ_MIN_MAX_AEQ_FLOOR_SHIFT) & 0xf,(v >> UB960_RR_AEQ_MIN_MAX_AEQ_MAX_SHIFT) & 0xf);- }
- if (ub960_rxport_get_eq_level(priv, nport, &eq_level) == 0)
dev_info(dev, "\tEQ level %u\n", eq_level);+}
static int ub960_log_status(struct v4l2_subdev *sd) { struct ub960_data *priv = sd_to_ub960(sd); @@ -2997,8 +3045,6 @@ static int ub960_log_status(struct v4l2_subdev *sd) for (nport = 0; nport < priv->hw_data->num_rxports; nport++) { struct ub960_rxport *rxport = priv->rxports[nport];
u8 eq_level;s8 strobe_pos;dev_info(dev, "RX %u\n", nport); @@ -3034,44 +3080,8 @@ static int ub960_log_status(struct v4l2_subdev *sd) ub960_rxport_read(priv, nport, UB960_RR_CSI_ERR_COUNTER, &v); dev_info(dev, "\tcsi_err_counter %u\n", v);
/* Strobe */ub960_read(priv, UB960_XR_AEQ_CTL1, &v);dev_info(dev, "\t%s strobe\n",(v & UB960_XR_AEQ_CTL1_AEQ_SFILTER_EN) ? "Adaptive" :"Manual");if (v & UB960_XR_AEQ_CTL1_AEQ_SFILTER_EN) {ub960_read(priv, UB960_XR_SFILTER_CFG, &v);dev_info(dev, "\tStrobe range [%d, %d]\n",((v >> UB960_XR_SFILTER_CFG_SFILTER_MIN_SHIFT) & 0xf) - 7,((v >> UB960_XR_SFILTER_CFG_SFILTER_MAX_SHIFT) & 0xf) - 7);}ub960_rxport_get_strobe_pos(priv, nport, &strobe_pos);dev_info(dev, "\tStrobe pos %d\n", strobe_pos);/* EQ */ub960_rxport_read(priv, nport, UB960_RR_AEQ_BYPASS, &v);dev_info(dev, "\t%s EQ\n",(v & UB960_RR_AEQ_BYPASS_ENABLE) ? "Manual" :"Adaptive");if (!(v & UB960_RR_AEQ_BYPASS_ENABLE)) {ub960_rxport_read(priv, nport, UB960_RR_AEQ_MIN_MAX, &v);dev_info(dev, "\tEQ range [%u, %u]\n",(v >> UB960_RR_AEQ_MIN_MAX_AEQ_FLOOR_SHIFT) & 0xf,(v >> UB960_RR_AEQ_MIN_MAX_AEQ_MAX_SHIFT) & 0xf);}if (ub960_rxport_get_eq_level(priv, nport, &eq_level) == 0)dev_info(dev, "\tEQ level %u\n", eq_level);
if (!priv->hw_data->is_ub9702)ub960_log_status_ub960_sp_eq(priv, nport);/* GPIOs */ for (i = 0; i < UB960_NUM_BC_GPIOS; i++) {
Hi,
On 09/12/2024 11:11, Sakari Ailus wrote:
Huomenta,
On Fri, Dec 06, 2024 at 10:26:40AM +0200, Tomi Valkeinen wrote:
UB9702 does not have SP and EQ registers, but the driver uses them in log_status(). Fix this by separating the SP and EQ related log_status() work into a separate function (for clarity) and calling that function only for UB960.
Cc: stable@vger.kernel.org Fixes: afe267f2d368 ("media: i2c: add DS90UB960 driver") Reviewed-by: Jai Luthra jai.luthra@ideasonboard.com Signed-off-by: Tomi Valkeinen tomi.valkeinen@ideasonboard.com
drivers/media/i2c/ds90ub960.c | 90 ++++++++++++++++++++++++------------------- 1 file changed, 50 insertions(+), 40 deletions(-)
diff --git a/drivers/media/i2c/ds90ub960.c b/drivers/media/i2c/ds90ub960.c index 24198b803eff..94c8acf171b4 100644 --- a/drivers/media/i2c/ds90ub960.c +++ b/drivers/media/i2c/ds90ub960.c @@ -2950,6 +2950,54 @@ static const struct v4l2_subdev_pad_ops ub960_pad_ops = { .set_fmt = ub960_set_fmt, }; +static void ub960_log_status_ub960_sp_eq(struct ub960_data *priv,
unsigned int nport)+{
- struct device *dev = &priv->client->dev;
- u8 eq_level;
- s8 strobe_pos;
- u8 v = 0;
- /* Strobe */
- ub960_read(priv, UB960_XR_AEQ_CTL1, &v);
How about adding __must_check to the ub960_read()?
Yes, that's a good idea.
We also have a patch in works that'll add error handling to all the i2c reads and writes (and some other ub960 improvements), on top of this series.
Tomi
Hi,
On 09/12/2024 11:11, Sakari Ailus wrote:
Huomenta,
On Fri, Dec 06, 2024 at 10:26:40AM +0200, Tomi Valkeinen wrote:
UB9702 does not have SP and EQ registers, but the driver uses them in log_status(). Fix this by separating the SP and EQ related log_status() work into a separate function (for clarity) and calling that function only for UB960.
Cc: stable@vger.kernel.org Fixes: afe267f2d368 ("media: i2c: add DS90UB960 driver") Reviewed-by: Jai Luthra jai.luthra@ideasonboard.com Signed-off-by: Tomi Valkeinen tomi.valkeinen@ideasonboard.com
drivers/media/i2c/ds90ub960.c | 90 ++++++++++++++++++++++++------------------- 1 file changed, 50 insertions(+), 40 deletions(-)
diff --git a/drivers/media/i2c/ds90ub960.c b/drivers/media/i2c/ds90ub960.c index 24198b803eff..94c8acf171b4 100644 --- a/drivers/media/i2c/ds90ub960.c +++ b/drivers/media/i2c/ds90ub960.c @@ -2950,6 +2950,54 @@ static const struct v4l2_subdev_pad_ops ub960_pad_ops = { .set_fmt = ub960_set_fmt, }; +static void ub960_log_status_ub960_sp_eq(struct ub960_data *priv,
unsigned int nport)+{
- struct device *dev = &priv->client->dev;
- u8 eq_level;
- s8 strobe_pos;
- u8 v = 0;
- /* Strobe */
- ub960_read(priv, UB960_XR_AEQ_CTL1, &v);
How about adding __must_check to the ub960_read()?
Actually, this is just moving code around (behind an if), so I'd rather not add more to this patch, especially as this is a fix.
We'll add the error handling separately on top.
Tomi
- dev_info(dev, "\t%s strobe\n",
(v & UB960_XR_AEQ_CTL1_AEQ_SFILTER_EN) ? "Adaptive" :"Manual");- if (v & UB960_XR_AEQ_CTL1_AEQ_SFILTER_EN) {
ub960_read(priv, UB960_XR_SFILTER_CFG, &v);dev_info(dev, "\tStrobe range [%d, %d]\n",((v >> UB960_XR_SFILTER_CFG_SFILTER_MIN_SHIFT) & 0xf) - 7,((v >> UB960_XR_SFILTER_CFG_SFILTER_MAX_SHIFT) & 0xf) - 7);- }
- ub960_rxport_get_strobe_pos(priv, nport, &strobe_pos);
- dev_info(dev, "\tStrobe pos %d\n", strobe_pos);
- /* EQ */
- ub960_rxport_read(priv, nport, UB960_RR_AEQ_BYPASS, &v);
- dev_info(dev, "\t%s EQ\n",
(v & UB960_RR_AEQ_BYPASS_ENABLE) ? "Manual" :"Adaptive");- if (!(v & UB960_RR_AEQ_BYPASS_ENABLE)) {
ub960_rxport_read(priv, nport, UB960_RR_AEQ_MIN_MAX, &v);dev_info(dev, "\tEQ range [%u, %u]\n",(v >> UB960_RR_AEQ_MIN_MAX_AEQ_FLOOR_SHIFT) & 0xf,(v >> UB960_RR_AEQ_MIN_MAX_AEQ_MAX_SHIFT) & 0xf);- }
- if (ub960_rxport_get_eq_level(priv, nport, &eq_level) == 0)
dev_info(dev, "\tEQ level %u\n", eq_level);+}
- static int ub960_log_status(struct v4l2_subdev *sd) { struct ub960_data *priv = sd_to_ub960(sd);
@@ -2997,8 +3045,6 @@ static int ub960_log_status(struct v4l2_subdev *sd) for (nport = 0; nport < priv->hw_data->num_rxports; nport++) { struct ub960_rxport *rxport = priv->rxports[nport];
u8 eq_level;s8 strobe_pos;dev_info(dev, "RX %u\n", nport); @@ -3034,44 +3080,8 @@ static int ub960_log_status(struct v4l2_subdev *sd) ub960_rxport_read(priv, nport, UB960_RR_CSI_ERR_COUNTER, &v); dev_info(dev, "\tcsi_err_counter %u\n", v);
/* Strobe */ub960_read(priv, UB960_XR_AEQ_CTL1, &v);dev_info(dev, "\t%s strobe\n",(v & UB960_XR_AEQ_CTL1_AEQ_SFILTER_EN) ? "Adaptive" :"Manual");if (v & UB960_XR_AEQ_CTL1_AEQ_SFILTER_EN) {ub960_read(priv, UB960_XR_SFILTER_CFG, &v);dev_info(dev, "\tStrobe range [%d, %d]\n",((v >> UB960_XR_SFILTER_CFG_SFILTER_MIN_SHIFT) & 0xf) - 7,((v >> UB960_XR_SFILTER_CFG_SFILTER_MAX_SHIFT) & 0xf) - 7);}ub960_rxport_get_strobe_pos(priv, nport, &strobe_pos);dev_info(dev, "\tStrobe pos %d\n", strobe_pos);/* EQ */ub960_rxport_read(priv, nport, UB960_RR_AEQ_BYPASS, &v);dev_info(dev, "\t%s EQ\n",(v & UB960_RR_AEQ_BYPASS_ENABLE) ? "Manual" :"Adaptive");if (!(v & UB960_RR_AEQ_BYPASS_ENABLE)) {ub960_rxport_read(priv, nport, UB960_RR_AEQ_MIN_MAX, &v);dev_info(dev, "\tEQ range [%u, %u]\n",(v >> UB960_RR_AEQ_MIN_MAX_AEQ_FLOOR_SHIFT) & 0xf,(v >> UB960_RR_AEQ_MIN_MAX_AEQ_MAX_SHIFT) & 0xf);}if (ub960_rxport_get_eq_level(priv, nport, &eq_level) == 0)dev_info(dev, "\tEQ level %u\n", eq_level);
if (!priv->hw_data->is_ub9702)ub960_log_status_ub960_sp_eq(priv, nport);/* GPIOs */ for (i = 0; i < UB960_NUM_BC_GPIOS; i++) {
Huomenta,
On Tue, Dec 10, 2024 at 09:38:30AM +0200, Tomi Valkeinen wrote:
Hi,
On 09/12/2024 11:11, Sakari Ailus wrote:
Huomenta,
On Fri, Dec 06, 2024 at 10:26:40AM +0200, Tomi Valkeinen wrote:
UB9702 does not have SP and EQ registers, but the driver uses them in log_status(). Fix this by separating the SP and EQ related log_status() work into a separate function (for clarity) and calling that function only for UB960.
Cc: stable@vger.kernel.org Fixes: afe267f2d368 ("media: i2c: add DS90UB960 driver") Reviewed-by: Jai Luthra jai.luthra@ideasonboard.com Signed-off-by: Tomi Valkeinen tomi.valkeinen@ideasonboard.com
drivers/media/i2c/ds90ub960.c | 90 ++++++++++++++++++++++++------------------- 1 file changed, 50 insertions(+), 40 deletions(-)
diff --git a/drivers/media/i2c/ds90ub960.c b/drivers/media/i2c/ds90ub960.c index 24198b803eff..94c8acf171b4 100644 --- a/drivers/media/i2c/ds90ub960.c +++ b/drivers/media/i2c/ds90ub960.c @@ -2950,6 +2950,54 @@ static const struct v4l2_subdev_pad_ops ub960_pad_ops = { .set_fmt = ub960_set_fmt, }; +static void ub960_log_status_ub960_sp_eq(struct ub960_data *priv,
unsigned int nport)+{
- struct device *dev = &priv->client->dev;
- u8 eq_level;
- s8 strobe_pos;
- u8 v = 0;
- /* Strobe */
- ub960_read(priv, UB960_XR_AEQ_CTL1, &v);
How about adding __must_check to the ub960_read()?
Actually, this is just moving code around (behind an if), so I'd rather not add more to this patch, especially as this is a fix.
We'll add the error handling separately on top.
Works for me.
The driver uses a static CSI-2 virtual channel mapping where all virtual channels from an RX port are mapped to a virtual channel number matching the RX port number.
The UB960 and UB9702 have different registers for the purpose, and the UB9702 version is not correct. Each of the VC_ID_MAP registers do not contain a single mapping, as the driver currently thinks, but two.
This can cause received VCs other than 0 to be mapped in a wrong way.
Fix this by writing both mappings to each register.
Cc: stable@vger.kernel.org Fixes: afe267f2d368 ("media: i2c: add DS90UB960 driver") Reviewed-by: Jai Luthra jai.luthra@ideasonboard.com Signed-off-by: Tomi Valkeinen tomi.valkeinen@ideasonboard.com --- drivers/media/i2c/ds90ub960.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/media/i2c/ds90ub960.c b/drivers/media/i2c/ds90ub960.c index 94c8acf171b4..bfffa14e2049 100644 --- a/drivers/media/i2c/ds90ub960.c +++ b/drivers/media/i2c/ds90ub960.c @@ -2533,7 +2533,7 @@ static int ub960_configure_ports_for_streaming(struct ub960_data *priv, for (i = 0; i < 8; i++) ub960_rxport_write(priv, nport, UB960_RR_VC_ID_MAP(i), - nport); + (nport << 4) | nport); }
break;
On Fri, Dec 06, 2024 at 10:26:36AM +0200, Tomi Valkeinen wrote:
This series fixes various small issues in the drivers, and adds a few things (a couple of pixel formats and a debugging feature).
It also takes a few steps in adding more i2c read/write error handlings to the drivers, but covers only the easy places.
Adding error handling to all reads/writes needs more thinking, perhaps adding a "ret" parameter to the calls, similar to the cci_* functions, or perhaps adding helpers for writing multiple registers from a given table. Also, in some places rolling back from an error will require work.
FWIW, Reviewed-by: Andy Shevchenko andriy.shevchenko@linux.intel.com
linux-stable-mirror@lists.linaro.org
-
Andy Shevchenko -
Sakari Ailus -
Tomi Valkeinen