From:   <Shravan.Chippa@microchip.com>
To:     <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
Thread-Topic: [PATCH RESEND v10 5/5] media: i2c: imx334: update pixel and link
 frequency
Thread-Index: AQHZLUnPWr0eVsa9dkWYdFRaw/UsYa6sow+AgABq7FCAAdoPAIACfu1w
Date:   Fri, 27 Jan 2023 00:10:22 +0000
Message-ID: <PH0PR11MB5611880BB2141A73B2D67D7181CC9@PH0PR11MB5611.namprd11.prod.outlook.com>
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>
In-Reply-To: <Y9D5eG8PP/qYPk40@valkosipuli.retiisi.eu>
Accept-Language: en-GB, en-US
Content-Language: en-US
x-ms-exchange-antispam-messagedata-chunkcount: 1
x-ms-exchange-antispam-messagedata-0: =?us-ascii?Q?cXV1g0Z41hTEV9VhbrzM+/1JqD12pF7P89sA31KJIIPb6AyLurmzv/CSaVb4?=
 =?us-ascii?Q?IXQXIxsUr75yRVaQsgZIGp4um7fQXrXBtfjGI4Ur9VYEFA/fWb/8WjBwFF5L?=
 =?us-ascii?Q?HwOcuyRZwF5z09XOL9Rb5GZDFxZTtlA4q9UzuCT3z6ATMwBJARaMnQKZYKpU?=
 =?us-ascii?Q?02MefORLcr320H6FgJV1gg9tiCr0wLpPrR95yWjcVPVkTMfFUTblhefAG5Fj?=
 =?us-ascii?Q?2Rwk0mpnJDZIWaGS+6I26ur8rmqeRUupwVm3NJEN2L0iohk+tlFNPK/khCfU?=
 =?us-ascii?Q?sFVI0Y9AQQkATXKjE/mE0s+I9pt/12rV0IxIg/pB5PiWz7yhUeIHS38Pxdgt?=
 =?us-ascii?Q?7VzM4tze/ERLRMDzExcCZuNAflqDh5E/wvxyrAplN6EyyJbuNC705jutbc9r?=
 =?us-ascii?Q?LFoqbaOUWU7oSTudknlq/42OQGQ3cEjLGlQVjJTc+HbMxqH4Jlr6sG+vvlm/?=
 =?us-ascii?Q?hlKDPBngMrcv4zqmeR3uw0KkbMWaPd0AyeaCY6bdRUMeIIFTVmv9gDsQx6PN?=
 =?us-ascii?Q?Uq7S/lRBiddtcLJ+5xLQ4mvjWkE2PlCDSQ63pJXXxQmvKKhfQE5SB4RnRWgz?=
 =?us-ascii?Q?tx5YWr796fnoOcpkmLHMcZ1z1gBKlLkvjAi1XrbjaDTERcf+BQWV84bwluP9?=
 =?us-ascii?Q?waNFYHH3Mz+O5G45Wg1OnhXoaW0hVgDSe+O8REPaIClx8AAERDlSGogfpNsT?=
 =?us-ascii?Q?hkimwEksiWQn9ZhBlwZZz0cc7/HE9anNebzd14wIRjbZNTRGIBrYO84gXRVv?=
 =?us-ascii?Q?VCEHeB+T60a3MZi5y4VTPq5zV/AzR1KaUTQsTN75h7l8GniEY6jUh7oFppcp?=
 =?us-ascii?Q?cIJ8DHZc90Bl09rXAgjOi13kwcAoYX+TRDitMPCDwAjIjeTchA6XqzV2u3RB?=
 =?us-ascii?Q?/nxsI7lnXocOweN+P298HRKII99+ZWpesYoCXvKBZZdDwQkn9IWgHwNyvSXw?=
 =?us-ascii?Q?pFshE5a31ywjYEERDVX4M3x30CNSZZZJXl4R0sBIekyxYy7hyo+verpBTODp?=
 =?us-ascii?Q?5HTFXCdUTMGXJ8UmJ0Yc5sZ18o4D4qBTn+UiGrgmP67g83KG+jDad9wfaL/y?=
 =?us-ascii?Q?tavMCPJ1FXra39R5Op96fOYY8ZPSDo10Db9KnGuMQu7pc65/S6C1VVqXK+fk?=
 =?us-ascii?Q?UqfrX8tbMOiFvWbUUKOxLBQVURNHWbm/2f8RrzeZS0j1j+6/T69l1DV5DLWc?=
 =?us-ascii?Q?8WUgjWiEfOxFu7K6EhYw0JExJ4bpbmmX6TDvUbV0RcczGxr8NQB+dphwi7qq?=
 =?us-ascii?Q?97zGWvvom3wf/TWz4S2h8XWoazsS1omuCcnqQ0qnJ9Go5LQJn1atLh7vroNr?=
 =?us-ascii?Q?QDJV4k/A97AJGGkdrkVi1d/YwD3tGwqtoyPGb4bZYNm8wblChkpw/BUgymBX?=
 =?us-ascii?Q?WD6xmvPZtqfGLP72UvGQJn+hR1SOyd5VbqhgUelmadAhYMTcp+MyfPsd6Fm1?=
 =?us-ascii?Q?GZ+dMgGYBrB2kXWVUWYYYjsiUxKbUORF5n8EUYustRchLi3RjZc7fx3AdC/u?=
 =?us-ascii?Q?m63RC3KJuz1MaFbOblGVeKTpkWjWPRsFomosfVcIEds2M+F2rX5p5Y72u9By?=
 =?us-ascii?Q?XsNXwRg95RmIlQoKXlh2kOS+1dVPbcS3FFhJhYOnhA/MC4NMWiSjaeoL2zaO?=
 =?us-ascii?Q?Ww=3D=3D?=
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-AuthSource: PH0PR11MB5611.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 6cc7e770-e123-4ed6-8dac-08dafffae207
X-MS-Exchange-CrossTenant-originalarrivaltime: 27 Jan 2023 00:10:22.4774
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 3f4057f3-b418-4d4e-ba84-d55b4e897d88
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: Xs7HwUZQXmhGAK3xSOX4E84IKmZiVhS7sPu4z1IzLLNxclWm152SEOv630IFuA/t147WfFRrFZVps4I2dadoJ5xpn51UTk3tiw5lZN9rb5w=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: MW4PR11MB6982
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

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
>=20
> EXTERNAL EMAIL: Do not click links or open attachments unless you know th=
e
> content is safe
>=20
> Hi Shravan,
>=20
> 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[] =3D {
> > > > -     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[] =3D {
> > > >               .vblank_min =3D 45,
> > > >               .vblank_max =3D 132840,
> > > >               .pclk =3D 297000000,
> > > > -             .link_freq_idx =3D 0,
> > > > +             .link_freq_idx =3D 1,
> > > >               .reg_list =3D {
> > > >                       .num_of_regs =3D ARRAY_SIZE(mode_1920x1080_re=
gs),
> > > >                       .regs =3D mode_1920x1080_regs, @@ -598,6
> > > > +601,11 @@ static int imx334_update_controls(struct imx334 *imx334,
> > > >       if (ret)
> > > >               return ret;
> > > >
> > > > +     ret =3D __v4l2_ctrl_modify_range(imx334->pclk_ctrl, mode->pcl=
k,
> > > > +                                    mode->pclk, 1, mode->pclk);
> > > > +     if (ret)
> > > > +             return ret;
> > > > +
> > > >       ret =3D __v4l2_ctrl_modify_range(imx334->hblank_ctrl, mode->h=
blank,
> > > >                                      mode->hblank, 1, mode->hblank)=
;
> > > >       if (ret)
> > > > @@ -698,6 +706,8 @@ static int imx334_set_ctrl(struct v4l2_ctrl *ct=
rl)
> > > >               pm_runtime_put(imx334->dev);
> > > >
> > > >               break;
> > > > +     case V4L2_CID_PIXEL_RATE:
> > > > +     case V4L2_CID_LINK_FREQ:
> > > >       case V4L2_CID_HBLANK:
> > > >               ret =3D 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 =3D 0; i < bus_cfg.nr_of_link_frequencies; i++)
> > > > -             if (bus_cfg.link_frequencies[i] =3D=3D IMX334_LINK_FR=
EQ)
> > > > +     for (i =3D 0; i < bus_cfg.nr_of_link_frequencies; i++) {
> > > > +             for (j =3D 0; j < ARRAY_SIZE(link_freq); j++) {
> > > > +                     if (bus_cfg.link_frequencies[i] =3D=3D link_f=
req[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.
>=20
> I don't see this being checked in imx334_set_pad_format(), for instance.
>=20
> If a frequency isn't in DT, the driver isn't supposed to be using it eith=
er.

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.


Thanks,
Shravan

>=20
> >
> > Thanks.
> > shravan
> > >
> > > > +                             break;
> > > > +                     }
> > > > +             }
> > > > +
> > > > +             if (j =3D=3D ARRAY_SIZE(link_freq)) {
> > > > +                     ret =3D dev_err_probe(imx334->dev, -EINVAL,
> > > > +                                         "no supported link freq
> > > > + found\n");
> > > >                       goto done_endpoint_free;
> > > > -
> > > > -     ret =3D -EINVAL;
> > > > +             }
> > > > +     }
> > > >
> > > >  done_endpoint_free:
> > > >       v4l2_fwnode_endpoint_free(&bus_cfg);
> > > > @@ -1232,10 +1251,10 @@ static int imx334_init_controls(struct
> > > > imx334
> > > *imx334)
> > > >       imx334->link_freq_ctrl =3D v4l2_ctrl_new_int_menu(ctrl_hdlr,
> > > >                                                       &imx334_ctrl_=
ops,
> > > >                                                       V4L2_CID_LINK=
_FREQ,
> > > > -                                                     ARRAY_SIZE(li=
nk_freq) -
> > > > -                                                     1,
> > > > -                                                     mode->link_fr=
eq_idx,
> > > > +
> > > > + __fls(imx334->menu_skip_mask),
> > > > +
> > > > + __ffs(imx334->menu_skip_mask),
> > > >                                                       link_freq);
> > > > +
> > > >       if (imx334->link_freq_ctrl)
> > > >               imx334->link_freq_ctrl->flags |=3D
> > > > V4L2_CTRL_FLAG_READ_ONLY;
>=20
> --
> Kind regards,
>=20
> Sakari Ailus