| Message ID | 20231206063045.97234-10-aakarsh.jain@samsung.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v5,01/11] dt-bindings: media: s5p-mfc: Add mfcv12 variant | expand |
> -----Original Message----- > From: Hans Verkuil <hverkuil-cisco@xs4all.nl> > Sent: 06 December 2023 18:28 > To: Aakarsh Jain <aakarsh.jain@samsung.com>; linux-arm- > kernel@lists.infradead.org; linux-media@vger.kernel.org; linux- > kernel@vger.kernel.org; devicetree@vger.kernel.org > Cc: m.szyprowski@samsung.com; andrzej.hajda@intel.com; > mchehab@kernel.org; krzysztof.kozlowski+dt@linaro.org; > robh+dt@kernel.org; conor+dt@kernel.org; linux-samsung- > soc@vger.kernel.org; andi@etezian.org; gost.dev@samsung.com; > alim.akhtar@samsung.com; aswani.reddy@samsung.com; > pankaj.dubey@samsung.com; ajaykumar.rs@samsung.com; linux- > fsd@tesla.com; Smitha T Murthy <smithatmurthy@gmail.com> > Subject: Re: [Patch v5 09/11] media: s5p-mfc: Load firmware for each run in > MFCv12. > > On 06/12/2023 07:30, Aakarsh Jain wrote: > > In MFCv12, some section of firmware gets updated at each MFC run. > > Hence we need to reload original firmware for each run at the start. > > > > Cc: linux-fsd@tesla.com > > Signed-off-by: Smitha T Murthy <smithatmurthy@gmail.com> > > Signed-off-by: Aakarsh Jain <aakarsh.jain@samsung.com> > > --- > > drivers/media/platform/samsung/s5p-mfc/s5p_mfc_ctrl.c | 6 ++++-- > > 1 file changed, 4 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/media/platform/samsung/s5p-mfc/s5p_mfc_ctrl.c > > b/drivers/media/platform/samsung/s5p-mfc/s5p_mfc_ctrl.c > > index b49159142c53..24dd40ae71ec 100644 > > --- a/drivers/media/platform/samsung/s5p-mfc/s5p_mfc_ctrl.c > > +++ b/drivers/media/platform/samsung/s5p-mfc/s5p_mfc_ctrl.c > > @@ -51,8 +51,10 @@ int s5p_mfc_load_firmware(struct s5p_mfc_dev > *dev) > > * into kernel. */ > > mfc_debug_enter(); > > > > - if (dev->fw_get_done) > > - return 0; > > + /* Load MFC v12 firmware for each run when MFC runs sequentially > */ > > You had a much longer explanation in your reply to my original question. > > I think it is better if that longer explanation is added here. > okay sure. Will add that explanation here. > Things that are weird and unexpected need good comments, explaining why > it is done, and also what you know and do not know about this. > > E.g. you know through trial and error that it is needed (or perhaps you got > information on this some the fw team), but there might be open questions > that are not yet answered. > > This is all information that you can't get from the source code since it has to > do with the black box firmware. So putting all you know in a comment is the > best way of communicating this to future readers of the source code. > Thanks for the review! > Regards, > > Hans > > > + if (!IS_MFCV12(dev)) > > + if (dev->fw_get_done) > > + return 0; > > > > for (i = MFC_FW_MAX_VERSIONS - 1; i >= 0; i--) { > > if (!dev->variant->fw_name[i])
diff --git a/drivers/media/platform/samsung/s5p-mfc/s5p_mfc_ctrl.c b/drivers/media/platform/samsung/s5p-mfc/s5p_mfc_ctrl.c index b49159142c53..24dd40ae71ec 100644 --- a/drivers/media/platform/samsung/s5p-mfc/s5p_mfc_ctrl.c +++ b/drivers/media/platform/samsung/s5p-mfc/s5p_mfc_ctrl.c @@ -51,8 +51,10 @@ int s5p_mfc_load_firmware(struct s5p_mfc_dev *dev) * into kernel. */ mfc_debug_enter(); - if (dev->fw_get_done) - return 0; + /* Load MFC v12 firmware for each run when MFC runs sequentially */ + if (!IS_MFCV12(dev)) + if (dev->fw_get_done) + return 0; for (i = MFC_FW_MAX_VERSIONS - 1; i >= 0; i--) { if (!dev->variant->fw_name[i])