Re: [alsa-devel] [PATCH 06/17] soundwire: cadence_master: use firmware defaults for frame shape

[Pierre-Louis Bossart]

> > > > > +static u32 cdns_set_initial_frame_shape(int n_rows, int n_cols)
> > > > > +{
> > > > > +ÂÂÂ u32 val;
> > > > > +ÂÂÂ int c;
> > > > > +ÂÂÂ int r;
> > > > > +
> > > > > +ÂÂÂ r = sdw_find_row_index(n_rows);
> > > > > +ÂÂÂ c = sdw_find_col_index(n_cols) & CDNS_MCP_FRAME_SHAPE_COL_MASK;
> > > > > +
> > > > > +ÂÂÂ val = (r << CDNS_MCP_FRAME_SHAPE_ROW_OFFSET) | c;
> > > > > +
> > > > > +ÂÂÂ return val;
> > > > > +}
> > > > > +
> > > >
> > > > Guess this have been said already, but this function could be
> > > > simplified - unless you really want to keep explicit variable
> > > > declaration. Both "c" and "r" declarations could be merged into
> > > > single line while "val" is not needed at all.
> > > >
> > > > One more thing - is AND bitwise op really needed for cols
> > > > explicitly? We know all col values upfront - these are static and
> > > > declared in global table nearby. Static declaration takes care of
> > > > "initial range-check". Is another one necessary?
> > > >
> > > > Moreover, this is a _get_ and certainly not a _set_ type of
> > > > function. I'd even consider renaming it to: "cdns_get_frame_shape"
> > > > as this is neither a _set_ nor an explicit initial frame shape
> > > > setter.
> > > >
> > > > It might be even helpful to split two usages:
> > > >
> > > > #define sdw_frame_shape(col_idx, row_idx) \
> > > >  ÂÂÂÂÂ((row_idx << CDNS_MCP_FRAME_SHAPE_ROW_OFFSET) | col_idx)
> > > >
> > > > u32 cdns_get_frame_shape(u16 rows, u16 cols)
> > > > {
> > > >  ÂÂÂÂÂu16 c, r;
> > > >
> > > >  ÂÂÂÂÂr = sdw_find_row_index(rows);
> > > >  ÂÂÂÂÂc = sdw_find_col_index(cols);
> > > >
> > > >  ÂÂÂÂÂreturn sdw_frame_shape(c, r);
> > > > }
> > > >
> > > > The above may even be simplified into one-liner.
> > >
> > > This is a function used once on startup, there is no real need to
> > > simplify further. The separate variables help add debug traces as needed
> > > and keep the code readable while showing how the values are encoded into
> > > a register.
> >
> > Eh, I've thought it's gonna be exposed to userspace (via uapi) so it can be
> > fetched by tests or tools.
>
> Uapi? I dont see anything in this or other series posted, did I miss
> something? Also I am not sure I like the idea of exposing these to
> userland!

Vinod, that was never the intent, and Cezary agreed, see following line

> > In such case - if there is a single usage only - guess function is fine as
> > is.