On Mon, Sep 02, 2024 at 09:43:36PM +0200, Benjamin Bara wrote:
> Hi Dave!
>
> On Mon, 2 Sept 2024 at 20:00, Dave Stevenson
> <dave.stevenson@xxxxxxxxxxxxxxx> wrote:
> > On Mon, 2 Sept 2024 at 16:58, <bbara93@xxxxxxxxx> wrote:
> > >
> > > From: Benjamin Bara <benjamin.bara@xxxxxxxxxxx>
> > >
> > > For now, the driver activates the first mode (1080p) as current active
> > > mode in probe(). This e.g. means that one cannot set VBLANK below 45
> > > (vmax_min - height), although theoretically the minimum is 30 (720p
> > > mode). Define the absolute possible/supported ranges to have them
> > > available later.
> >
> > Currently the driver will set the ranges for VBLANK and HBLANK
> > whenever the mode changes.
> >
> > How is it helpful to fake these numbers? Seeing as they aren't
> > reflecting anything useful, they may as well all be 0.
> >
> > > Signed-off-by: Benjamin Bara <benjamin.bara@xxxxxxxxxxx>
> > > ---
> > > Changes since v2:
> > > - new
> > > ---
> > > drivers/media/i2c/imx290.c | 36 ++++++++++++++++++++++++++++++++----
> > > 1 file changed, 32 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/drivers/media/i2c/imx290.c b/drivers/media/i2c/imx290.c
> > > index 1c97f9650eb4..466492bab600 100644
> > > --- a/drivers/media/i2c/imx290.c
> > > +++ b/drivers/media/i2c/imx290.c
> > > @@ -499,6 +499,10 @@ static const struct imx290_clk_cfg imx290_720p_clock_config[] = {
> > > };
> > >
> > > /* Mode configs */
> > > +#define WIDTH_720P 1280
> > > +#define HEIGHT_720P 720
That's pure obfuscation :-) Just use the values directly.
> > > +#define MINIMUM_WIDTH WIDTH_720P
> > > +#define MINIMUM_HEIGHT HEIGHT_720P
Driver-specific macros should have a driver-specific prefix.
> > > static const struct imx290_mode imx290_modes_2lanes[] = {
> > > {
> > > .width = 1920,
> > > @@ -512,8 +516,8 @@ static const struct imx290_mode imx290_modes_2lanes[] = {
> > > .clk_cfg = imx290_1080p_clock_config,
> > > },
> > > {
> > > - .width = 1280,
> > > - .height = 720,
> > > + .width = WIDTH_720P,
> > > + .height = HEIGHT_720P,
> > > .hmax_min = 3300,
> > > .vmax_min = 750,
> > > .link_freq_index = FREQ_INDEX_720P,
> > > @@ -537,8 +541,8 @@ static const struct imx290_mode imx290_modes_4lanes[] = {
> > > .clk_cfg = imx290_1080p_clock_config,
> > > },
> > > {
> > > - .width = 1280,
> > > - .height = 720,
> > > + .width = WIDTH_720P,
> > > + .height = HEIGHT_720P,
> > > .hmax_min = 3300,
> > > .vmax_min = 750,
> > > .link_freq_index = FREQ_INDEX_720P,
> > > @@ -846,6 +850,30 @@ static const char * const imx290_test_pattern_menu[] = {
> > > "000/555h Toggle Pattern",
> > > };
> > >
> > > +/* absolute supported control ranges */
> > > +#define HBLANK_MAX (IMX290_HMAX_MAX - MINIMUM_WIDTH)
> > > +#define VBLANK_MAX (IMX290_VMAX_MAX - MINIMUM_HEIGHT)
Here too.
> > > +static unsigned int imx290_get_blank_min(const struct imx290 *imx290, bool v)
> > > +{
> >
> > This function is never used in this patch. I'm surprised the compiler
> > doesn't throw an error on a static function not being used.
> > You first use it in patch 4 "Introduce initial "off" mode & link freq"
> >
> > > + const struct imx290_mode *modes = imx290_modes_ptr(imx290);
> > > + unsigned int min = UINT_MAX;
> > > + int i;
> > > +
> > > + for (i = 0; i < imx290_modes_num(imx290); i++) {
> > > + unsigned int tmp;
> > > +
> > > + if (v)
> > > + tmp = modes[i].hmax_min - modes[i].width;
> >
> > if (v)
> > return h
> >
> > With the complete series my sensor comes up with controls defined as
> > vertical_blanking 0x009e0901 (int) : min=280 max=261423 step=1
> > default=280 value=280
> > horizontal_blanking 0x009e0902 (int) : min=30 max=64255 step=1
> > default=30 value=30
> >
> > Set the mode to 1080p and I get
> > vertical_blanking 0x009e0901 (int) : min=45 max=261063 step=1
> > default=45 value=1169
> > horizontal_blanking 0x009e0902 (int) : min=280 max=63615 step=1
> > default=280 value=280
>
> The idea here is to have VBLANK=30 available in the initial "after
> probe" state of the sensor. VBLANK=30 is a valid value for 720p mode,
> but it cannot be set after probe, because the driver (not the user)
> decided that 1080 mode is active. The idea is to relax the ranges while
> the mode is not set. Once the mode is known, the values are tightened
> to the real mode-dependent values.
>
> Kind regards
> Benjamin
>
> > Dave
> >
> > > + else
> > > + tmp = modes[i].vmax_min - modes[i].height;
> > > +
> > > + if (tmp < min)
> > > + min = tmp;
> > > + }
> > > +
> > > + return min;
> > > +}
> > > +
> > > static void imx290_ctrl_update(struct imx290 *imx290,
> > > const struct imx290_mode *mode)
> > > {
--
Regards,
Laurent Pinchart
