Date:   Wed, 22 Feb 2023 11:16:20 +0200
From:   Sakari Ailus <sakari.ailus@iki.fi>
To:     Shravan.Chippa@microchip.com
Cc:     paul.j.murphy@intel.com, daniele.alessandrelli@intel.com,
        mchehab@kernel.org, krzysztof.kozlowski+dt@linaro.org,
        shawnguo@kernel.org, s.hauer@pengutronix.de, festevam@gmail.com,
        kernel@pengutronix.de, linux-imx@nxp.com,
        linux-media@vger.kernel.org, linux-kernel@vger.kernel.org,
        devicetree@vger.kernel.org, linux-arm-kernel@lists.infradead.org
Subject: Re: [PATCH RESEND v10 5/5] media: i2c: imx334: update pixel and link
 frequency
Message-ID: <Y/XdZC7Yc8TjPvj4@valkosipuli.retiisi.eu>
References: <20230121033713.3535351-1-shravan.chippa@microchip.com>
 <20230121033713.3535351-6-shravan.chippa@microchip.com>
 <Y88SG5ndXjQ9AjZg@valkosipuli.retiisi.eu>
 <PH0PR11MB56119C93EB4A19FEFC03778F81C99@PH0PR11MB5611.namprd11.prod.outlook.com>
 <Y9D5eG8PP/qYPk40@valkosipuli.retiisi.eu>
 <PH0PR11MB5611880BB2141A73B2D67D7181CC9@PH0PR11MB5611.namprd11.prod.outlook.com>
 <PH0PR11MB561174CA61F9C858577F726781DA9@PH0PR11MB5611.namprd11.prod.outlook.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <PH0PR11MB561174CA61F9C858577F726781DA9@PH0PR11MB5611.namprd11.prod.outlook.com>
Precedence: bulk

Hi Shravan,

On Mon, Feb 06, 2023 at 04:43:42AM +0000, Shravan.Chippa@microchip.com wrote:
> Hi Sakari,
> 
> 
> > -----Original Message-----
> > From: shravan Chippa - I35088
> > Sent: 27 January 2023 05:40 AM
> > To: Sakari Ailus <sakari.ailus@iki.fi>
> > Cc: paul.j.murphy@intel.com; daniele.alessandrelli@intel.com;
> > mchehab@kernel.org; krzysztof.kozlowski+dt@linaro.org;
> > shawnguo@kernel.org; s.hauer@pengutronix.de; festevam@gmail.com;
> > kernel@pengutronix.de; linux-imx@nxp.com; linux-media@vger.kernel.org;
> > linux-kernel@vger.kernel.org; devicetree@vger.kernel.org; linux-arm-
> > kernel@lists.infradead.org
> > Subject: RE: [PATCH RESEND v10 5/5] media: i2c: imx334: update pixel and
> > link frequency
> > 
> > Hi Sakari,
> > 
> > > -----Original Message-----
> > > From: Sakari Ailus <sakari.ailus@iki.fi>
> > > Sent: 25 January 2023 03:12 PM
> > > To: shravan Chippa - I35088 <Shravan.Chippa@microchip.com>
> > > Cc: paul.j.murphy@intel.com; daniele.alessandrelli@intel.com;
> > > mchehab@kernel.org; krzysztof.kozlowski+dt@linaro.org;
> > > shawnguo@kernel.org; s.hauer@pengutronix.de; festevam@gmail.com;
> > > kernel@pengutronix.de; linux-imx@nxp.com; linux-
> > media@vger.kernel.org;
> > > linux-kernel@vger.kernel.org; devicetree@vger.kernel.org; linux-arm-
> > > kernel@lists.infradead.org
> > > Subject: Re: [PATCH RESEND v10 5/5] media: i2c: imx334: update pixel
> > > and link frequency
> > >
> > > EXTERNAL EMAIL: Do not click links or open attachments unless you know
> > > the content is safe
> > >
> > > Hi Shravan,
> > >
> > > On Tue, Jan 24, 2023 at 05:34:02AM +0000, Shravan.Chippa@microchip.com
> > > wrote:
> > > > Hi Sakari,
> > > >
> > > > > -----Original Message-----
> > > > > From: Sakari Ailus <sakari.ailus@iki.fi>
> > > > > Sent: 24 January 2023 04:33 AM
> > > > > To: shravan Chippa - I35088 <Shravan.Chippa@microchip.com>
> > > > > Cc: paul.j.murphy@intel.com; daniele.alessandrelli@intel.com;
> > > > > mchehab@kernel.org; krzysztof.kozlowski+dt@linaro.org;
> > > > > shawnguo@kernel.org; s.hauer@pengutronix.de;
> > festevam@gmail.com;
> > > > > kernel@pengutronix.de; linux-imx@nxp.com;
> > > > > linux-media@vger.kernel.org; linux-kernel@vger.kernel.org;
> > > > > devicetree@vger.kernel.org; linux-arm- kernel@lists.infradead.org
> > > > > Subject: Re: [PATCH RESEND v10 5/5] media: i2c: imx334: update
> > > > > pixel and link frequency
> > > > >
> > > > > EXTERNAL EMAIL: Do not click links or open attachments unless you
> > > > > know the content is safe
> > > > >
> > > > > Hi Shravan,
> > > > >
> > > > > On Sat, Jan 21, 2023 at 09:07:13AM +0530, shravan kumar wrote:
> > > > > > From: Shravan Chippa <shravan.chippa@microchip.com>
> > > > > >
> > > > > > Update pixel_rate and link frequency for 1920x1080@30 while
> > > > > > changing mode.
> > > > > >
> > > > > > Add dummy ctrl cases for pixel_rate and link frequency to avoid
> > > > > > error while changing the modes dynamically.
> > > > > >
> > > > > > Add support to handle multiple link frequencies.
> > > > > >
> > > > > > Suggested-by: Sakari Ailus <sakari.ailus@iki.fi>
> > > > > > Signed-off-by: Shravan Chippa <shravan.chippa@microchip.com>
> > > > > > ---
> > > > > >  drivers/media/i2c/imx334.c | 41
> > > > > > ++++++++++++++++++++++++++++----------
> > > > > >  1 file changed, 30 insertions(+), 11 deletions(-)
> > > > > >
> > > > > > diff --git a/drivers/media/i2c/imx334.c
> > > > > > b/drivers/media/i2c/imx334.c index 309c706114d2..62b104eaa437
> > > > > > 100644
> > > > > > --- a/drivers/media/i2c/imx334.c
> > > > > > +++ b/drivers/media/i2c/imx334.c
> > > > > > @@ -49,7 +49,8 @@
> > > > > >  #define IMX334_INCLK_RATE    24000000
> > > > > >
> > > > > >  /* CSI2 HW configuration */
> > > > > > -#define IMX334_LINK_FREQ     891000000
> > > > > > +#define IMX334_LINK_FREQ_891M        891000000
> > > > > > +#define IMX334_LINK_FREQ_445M        445500000
> > > > > >  #define IMX334_NUM_DATA_LANES        4
> > > > > >
> > > > > >  #define IMX334_REG_MIN               0x00
> > > > > > @@ -139,12 +140,14 @@ struct imx334 {
> > > > > >       u32 vblank;
> > > > > >       const struct imx334_mode *cur_mode;
> > > > > >       struct mutex mutex;
> > > > > > +     unsigned long menu_skip_mask;
> > > > > >       u32 cur_code;
> > > > > >       bool streaming;
> > > > > >  };
> > > > > >
> > > > > >  static const s64 link_freq[] = {
> > > > > > -     IMX334_LINK_FREQ,
> > > > > > +     IMX334_LINK_FREQ_891M,
> > > > > > +     IMX334_LINK_FREQ_445M,
> > > > > >  };
> > > > > >
> > > > > >  /* Sensor mode registers for 1920x1080@30fps */ @@ -468,7
> > > > > > +471,7 @@ static const struct imx334_mode supported_modes[] = {
> > > > > >               .vblank_min = 45,
> > > > > >               .vblank_max = 132840,
> > > > > >               .pclk = 297000000,
> > > > > > -             .link_freq_idx = 0,
> > > > > > +             .link_freq_idx = 1,
> > > > > >               .reg_list = {
> > > > > >                       .num_of_regs = ARRAY_SIZE(mode_1920x1080_regs),
> > > > > >                       .regs = mode_1920x1080_regs, @@ -598,6
> > > > > > +601,11 @@ static int imx334_update_controls(struct imx334
> > > > > > +*imx334,
> > > > > >       if (ret)
> > > > > >               return ret;
> > > > > >
> > > > > > +     ret = __v4l2_ctrl_modify_range(imx334->pclk_ctrl, mode->pclk,
> > > > > > +                                    mode->pclk, 1, mode->pclk);
> > > > > > +     if (ret)
> > > > > > +             return ret;
> > > > > > +
> > > > > >       ret = __v4l2_ctrl_modify_range(imx334->hblank_ctrl, mode-
> > >hblank,
> > > > > >                                      mode->hblank, 1, mode->hblank);
> > > > > >       if (ret)
> > > > > > @@ -698,6 +706,8 @@ static int imx334_set_ctrl(struct v4l2_ctrl *ctrl)
> > > > > >               pm_runtime_put(imx334->dev);
> > > > > >
> > > > > >               break;
> > > > > > +     case V4L2_CID_PIXEL_RATE:
> > > > > > +     case V4L2_CID_LINK_FREQ:
> > > > > >       case V4L2_CID_HBLANK:
> > > > > >               ret = 0;
> > > > > >               break;
> > > > > > @@ -1047,7 +1057,7 @@ static int imx334_parse_hw_config(struct
> > > > > > imx334
> > > > > *imx334)
> > > > > >       struct fwnode_handle *ep;
> > > > > >       unsigned long rate;
> > > > > >       int ret;
> > > > > > -     int i;
> > > > > > +     int i, j;
> > > > >
> > > > > unsigned int would be nicer.
> > > > I will change.
> > > > >
> > > > > >
> > > > > >       if (!fwnode)
> > > > > >               return -ENXIO;
> > > > > > @@ -1097,11 +1107,20 @@ static int imx334_parse_hw_config(struct
> > > > > > imx334
> > > > > *imx334)
> > > > > >               goto done_endpoint_free;
> > > > > >       }
> > > > > >
> > > > > > -     for (i = 0; i < bus_cfg.nr_of_link_frequencies; i++)
> > > > > > -             if (bus_cfg.link_frequencies[i] == IMX334_LINK_FREQ)
> > > > > > +     for (i = 0; i < bus_cfg.nr_of_link_frequencies; i++) {
> > > > > > +             for (j = 0; j < ARRAY_SIZE(link_freq); j++) {
> > > > > > +                     if (bus_cfg.link_frequencies[i] == link_freq[j]) {
> > > > > > +                             set_bit(j,
> > > > > > + &imx334->menu_skip_mask);
> > > > >
> > > > > Is there a guarantee that you'll only be using the modes with the
> > > > > listed frequencies? I don't see one but I might have missed it.
> > > >
> > > > If I understand it correctly, the question here is, the listed
> > > > freqeunceis and modes are one to one mapped? Then yes.
> > >
> > > I don't see this being checked in imx334_set_pad_format(), for instance.
> > >
> > > If a frequency isn't in DT, the driver isn't supposed to be using it either.
> > 
> > Yes, there is no check.
> > 
> > But, if a frequency is not in DT, the driver will not add in menu items.
> > So, the function imx334_set_pad_format() -> imx334_update_controls()
> > fails, if we set the frequencies which are not there in the DT or menu items.
> > 
> 
> Are you ok with the above explanation or any changes you are expecting?
> Please do let me know if there are any changes needed.
> I am planning to send the next version.

There doesn't seem to be anything that would prevent selecting a format
with a wrong link frequency in these patches. Could you address that, or
point me to where this is done? The control can't be changed by the user,
but that's not enough.

-- 
Kind regards,

Sakari Ailus