Hi Laurent!
On Mon, 2 Sept 2024 at 21:58, Laurent Pinchart
<laurent.pinchart@xxxxxxxxxxxxxxxx> wrote:
> On Mon, Sep 02, 2024 at 05:57:29PM +0200, bbara93@xxxxxxxxx wrote:
> > From: Benjamin Bara <benjamin.bara@xxxxxxxxxxx>
> >
> > To be compliant to the V4L2 API, the driver currently "randomly" decides
> > on one of the two supported modes which also implies a link frequency.
> >
> > Add a new mode and frequency which symbolize that the sensor is not in
> > use. This can be used as a default value during probe() and enables us
> > to avoid communication with the sensor.
>
> I really doin't like this change. I would like to instead move away from
> modes and make the driver freely configurable.
Which controls do you want to have freely configurable? At least on the
imx290 the exposure limits depend on the blanking, and the blanking
limits depend on the format. As the format is defined by the mode on
imx290, I think this will be quite hard with the current controls.
> Furthermore, the concept of an initial unconfigured state isn't valid
> in V4L2. The driver must fully initialize the whole device state at
> probe time.
I understand that and it makes sense to me. But given the dependencies
from above and the fact that the format is currently part of this
"state", it makes the "freely configurable" intention even harder :-(
Kind regards
Benjamin
> > Signed-off-by: Benjamin Bara <benjamin.bara@xxxxxxxxxxx>
> > ---
> > Changes since v2:
> > - new
> > ---
> > drivers/media/i2c/imx290.c | 29 +++++++++++++++++++++++------
> > 1 file changed, 23 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/media/i2c/imx290.c b/drivers/media/i2c/imx290.c
> > index 6812e7cb9e23..ece4d66001f5 100644
> > --- a/drivers/media/i2c/imx290.c
> > +++ b/drivers/media/i2c/imx290.c
> > @@ -425,14 +425,17 @@ static const struct imx290_csi_cfg imx290_csi_297mhz = {
> > /* supported link frequencies */
> > #define FREQ_INDEX_1080P 0
> > #define FREQ_INDEX_720P 1
> > +#define FREQ_INDEX_OFF 2
> > static const s64 imx290_link_freq_2lanes[] = {
> > [FREQ_INDEX_1080P] = 445500000,
> > [FREQ_INDEX_720P] = 297000000,
> > + [FREQ_INDEX_OFF] = 0,
> > };
> >
> > static const s64 imx290_link_freq_4lanes[] = {
> > [FREQ_INDEX_1080P] = 222750000,
> > [FREQ_INDEX_720P] = 148500000,
> > + [FREQ_INDEX_OFF] = 0,
> > };
> >
> > /*
> > @@ -552,6 +555,10 @@ static const struct imx290_mode imx290_modes_4lanes[] = {
> > },
> > };
> >
> > +static const struct imx290_mode imx290_mode_off = {
> > + .link_freq_index = FREQ_INDEX_OFF,
> > +};
> > +
> > static inline const struct imx290_mode *imx290_modes_ptr(const struct imx290 *imx290)
> > {
> > if (imx290->nlanes == 2)
> > @@ -876,10 +883,19 @@ static unsigned int imx290_get_blank_min(const struct imx290 *imx290, bool v)
> > static void imx290_ctrl_update(struct imx290 *imx290,
> > const struct imx290_mode *mode)
> > {
> > - unsigned int hblank_min = mode->hmax_min - mode->width;
> > - unsigned int hblank_max = IMX290_HMAX_MAX - mode->width;
> > - unsigned int vblank_min = mode->vmax_min - mode->height;
> > - unsigned int vblank_max = IMX290_VMAX_MAX - mode->height;
> > + unsigned int hblank_min, hblank_max, vblank_min, vblank_max;
> > +
> > + if (mode == &imx290_mode_off) {
> > + hblank_min = imx290_get_blank_min(imx290, false);
> > + hblank_max = HBLANK_MAX;
> > + vblank_min = imx290_get_blank_min(imx290, true);
> > + vblank_max = VBLANK_MAX;
> > + } else {
> > + hblank_min = mode->hmax_min - mode->width;
> > + hblank_max = IMX290_HMAX_MAX - mode->width;
> > + vblank_min = mode->vmax_min - mode->height;
> > + vblank_max = IMX290_VMAX_MAX - mode->height;
> > + }
> >
> > __v4l2_ctrl_s_ctrl(imx290->link_freq, mode->link_freq_index);
> >
> > @@ -932,7 +948,8 @@ static int imx290_ctrl_init(struct imx290 *imx290)
> > imx290->link_freq =
> > v4l2_ctrl_new_int_menu(&imx290->ctrls, &imx290_ctrl_ops,
> > V4L2_CID_LINK_FREQ,
> > - imx290_link_freqs_num(imx290) - 1, 0,
> > + imx290_link_freqs_num(imx290) - 1,
> > + FREQ_INDEX_OFF,
> > imx290_link_freqs_ptr(imx290));
> > if (imx290->link_freq)
> > imx290->link_freq->flags |= V4L2_CTRL_FLAG_READ_ONLY;
> > @@ -1278,7 +1295,7 @@ static int imx290_subdev_init(struct imx290 *imx290)
> > struct v4l2_subdev_state *state;
> > int ret;
> >
> > - imx290->current_mode = &imx290_modes_ptr(imx290)[0];
> > + imx290->current_mode = &imx290_mode_off;
> >
> > v4l2_i2c_subdev_init(&imx290->sd, client, &imx290_subdev_ops);
> > imx290->sd.internal_ops = &imx290_internal_ops;
> >
>
> --
> Regards,
>
> Laurent Pinchart
