| Message ID | 20241206-ub9xx-fixes-v4-4-466786eec7cc@ideasonboard.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | media: i2c: ds90ub9xx: Misc fixes and improvements | expand |
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; > 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++) { >
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; >> 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 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.
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++) {
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(-)