Re: [Freedreno] [PATCH v3 12/13] drm/msm/dsi: Add support for DSC configuration

[Vinod Koul <vkoul@kernel.org>]

On 22-03-22, 19:59, Marijn Suijten wrote:
> On 2022-03-22 22:46:50, Vinod Koul wrote:
> > On 17-02-22, 16:11, Marijn Suijten wrote:
> > > Hi Vinod,
> > > 
> > > Thanks for taking time to go through this review, please find some
> > > clarifications below.
> > > 
> > > On 2022-02-17 16:44:04, Vinod Koul wrote:
> > > > Hi Marijn,
> > > > 
> > > > On 11-12-21, 01:03, Marijn Suijten wrote:
> > > > 
> > > > > > +static int dsi_dsc_update_pic_dim(struct msm_display_dsc_config *dsc,
> > > > > > +				  int pic_width, int pic_height)
> > > > > 
> > > > > This function - adopted from downstream - does not seem to perform a
> > > > > whole lot, especially without the modulo checks against the slice size.
> > > > > Perhaps it can be inlined?
> > > > 
> > > > Most of the code here is :)
> > > > 
> > > > This was split from downstream code to check and update dimension. We
> > > > can inline this, or should we leave that to compiler. I am not a very
> > > > big fan of inlining...
> > > 
> > > It doesn't seem beneficial to code readability to have this function,
> > > which is only called just once and also has the same struct members read
> > > in a `DBG()` directly, abstracted away to a function.  Not really
> > > concerned about generated code/performance FWIW.
> > > 
> > > Also note that the caller isn't checking the `-EINVAL` result...
> > 
> > I have made this void inline.
> 
> Perhaps there is a misunderstanding here: with inlining I am referring
> to the process of transplanting the _function body_ to the only
> call-site, not adding the `inline` keyword nor changing this to `void`.
> 
> The checks that make this function return `-EINVAL` seem valid, so the
> caller should check it instead of removing the return?

Okay somehow I misunderstood then, let me see how the code looks in this
case then
-- 
~Vinod