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

[Pierre-Louis Bossart]

On 8/6/19 10:27 AM, Cezary Rojewski wrote:
> On 2019-08-06 02:55, Pierre-Louis Bossart wrote:
> > diff --git a/drivers/soundwire/cadence_master.c 
> > b/drivers/soundwire/cadence_master.c
> > index 5d9729b4d634..89c55e4bb72c 100644
> > --- a/drivers/soundwire/cadence_master.c
> > +++ b/drivers/soundwire/cadence_master.c
> > @@ -48,6 +48,8 @@
> > Â #define CDNS_MCP_SSPSTATÂÂÂÂÂÂÂÂÂÂÂ 0xC
> > Â #define CDNS_MCP_FRAME_SHAPEÂÂÂÂÂÂÂÂÂÂÂ 0x10
> > Â #define CDNS_MCP_FRAME_SHAPE_INITÂÂÂÂÂÂÂ 0x14
> > +#define CDNS_MCP_FRAME_SHAPE_COL_MASKÂÂÂÂÂÂÂ GENMASK(2, 0)
> > +#define CDNS_MCP_FRAME_SHAPE_ROW_OFFSETÂÂÂÂÂÂÂ 3
> > Â #define CDNS_MCP_CONFIG_UPDATEÂÂÂÂÂÂÂÂÂÂÂ 0x18
> > Â #define CDNS_MCP_CONFIG_UPDATE_BITÂÂÂÂÂÂÂ BIT(0)
> > @@ -175,7 +177,6 @@
> > Â /* Driver defaults */
> > Â #define CDNS_DEFAULT_CLK_DIVIDERÂÂÂÂÂÂÂ 0
> > -#define CDNS_DEFAULT_FRAME_SHAPEÂÂÂÂÂÂÂ 0x30
> > Â #define CDNS_DEFAULT_SSP_INTERVALÂÂÂÂÂÂÂ 0x18
> > Â #define CDNS_TX_TIMEOUTÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ 2000
> > @@ -901,6 +902,20 @@ int sdw_cdns_pdi_init(struct sdw_cdns *cdns,
> > Â }
> > Â EXPORT_SYMBOL(sdw_cdns_pdi_init);
> > +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.