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

[Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>]

>>>>> +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.
>