Hi Laurent,
Thank you for your comments.
On Fri, Sep 26, 2014 at 01:44:03PM +0300, Laurent Pinchart wrote:
> Hi Sakari,
>
> Thank you for the patch.
>
> On Wednesday 17 September 2014 23:45:41 Sakari Ailus wrote:
> > Decrease the link frequency to the next lower if the user chooses a media
> > bus code (BPP) cannot be achieved using the selected link frequency.
> >
> > Signed-off-by: Sakari Ailus <sakari.ailus@xxxxxxxxxxxxxxx>
> > ---
> > drivers/media/i2c/smiapp/smiapp-core.c | 20 ++++++++++++++++++--
> > 1 file changed, 18 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/media/i2c/smiapp/smiapp-core.c
> > b/drivers/media/i2c/smiapp/smiapp-core.c index 537ca92..ce2c34d 100644
> > --- a/drivers/media/i2c/smiapp/smiapp-core.c
> > +++ b/drivers/media/i2c/smiapp/smiapp-core.c
> > @@ -286,11 +286,27 @@ static int smiapp_pll_update(struct smiapp_sensor
> > *sensor)
> >
> > pll->binning_horizontal = sensor->binning_horizontal;
> > pll->binning_vertical = sensor->binning_vertical;
> > - pll->link_freq =
> > - sensor->link_freq->qmenu_int[sensor->link_freq->val];
> > pll->scale_m = sensor->scale_m;
> > pll->bits_per_pixel = sensor->csi_format->compressed;
> >
> > + if (!test_bit(sensor->link_freq->val,
> > + &sensor->valid_link_freqs[
> > + sensor->csi_format->compressed
> > + - SMIAPP_COMPRESSED_BASE])) {
> > + /*
> > + * Setting the link frequency will perform PLL
> > + * re-calculation already, so skip that.
> > + */
> > + return __v4l2_ctrl_s_ctrl(
> > + sensor->link_freq,
> > + __ffs(sensor->valid_link_freqs[
> > + sensor->csi_format->compressed
> > + - SMIAPP_COMPRESSED_BASE]));
>
> I have an uneasy feeling about this, as smiapp_pll_update is called from the
> link freq s_ctrl handler. Have you double-checked the recursion bounds ?
We haven't actually done any PLL tree calculation yet here. The condition
will evaluate true in a case when the user chooses a format which isn't
available on a given link frequency, or chooses a link frequency which isn't
available for a given format.
The condition will be false the next time the function is called since we've
just chosen a valid combination of the two.
But now that you brought the topic up, I think the link frequency selection
should just probably return -EBUSY if the selected link frquency cannot be
used. Also __ffs() should be __fls() instead in order to still come up with
the highest link freqency.
--
Kind regards,
Sakari Ailus
e-mail: sakari.ailus@xxxxxx XMPP: sailus@xxxxxxxxxxxxxx
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
