On 2020-09-23 3:54 PM, Andy Shevchenko wrote:
> On Wed, Sep 23, 2020 at 02:25:00PM +0200, Cezary Rojewski wrote:
>> DSP designed for Lynxpoint and Wildcat Point offers no dynamic topology
>> i.e. all pipelines are already defined within firmware and host is
>> relegated to allocing stream for predefined pins. This is represented by
>> 'catpt_topology' member.
>>
...
>> +static u32 catpt_get_channel_map(enum catpt_channel_config config)
>> +{
>> + switch (config) {
>> + case CATPT_CHANNEL_CONFIG_MONO:
>> + return (GENMASK(31, 4) | CATPT_CHANNEL_CENTER);
>
> In all cases outer parentheses are not needed. Also I expected to see DUAL MONO
> case here, rather than below.
>
Ack for the parentheses but what's wrong with the sweet '_STEREO' name?
This function is based on internal equivalent. As I aim to ease any
further debug/development by aligning with Windows equivalent, name
changes for no real reason do not help with that goal.
If I'm missing something about 'DUAL MONO', feel free to correct me.
>> + case CATPT_CHANNEL_CONFIG_STEREO:
>> + return (GENMASK(31, 8) | CATPT_CHANNEL_LEFT
>> + | (CATPT_CHANNEL_RIGHT << 4));
>> +
>> + case CATPT_CHANNEL_CONFIG_2_POINT_1:
>> + return (GENMASK(31, 12) | CATPT_CHANNEL_LEFT
>> + | (CATPT_CHANNEL_RIGHT << 4)
>> + | (CATPT_CHANNEL_LFE << 8));
>> +
>> + case CATPT_CHANNEL_CONFIG_3_POINT_0:
>> + return (GENMASK(31, 12) | CATPT_CHANNEL_LEFT
>> + | (CATPT_CHANNEL_CENTER << 4)
>> + | (CATPT_CHANNEL_RIGHT << 8));
>> +
>> + case CATPT_CHANNEL_CONFIG_3_POINT_1:
>> + return (GENMASK(31, 16) | CATPT_CHANNEL_LEFT
>> + | (CATPT_CHANNEL_CENTER << 4)
>> + | (CATPT_CHANNEL_RIGHT << 8)
>> + | (CATPT_CHANNEL_LFE << 12));
>> +
>> + case CATPT_CHANNEL_CONFIG_QUATRO:
>> + return (GENMASK(31, 16) | CATPT_CHANNEL_LEFT
>> + | (CATPT_CHANNEL_RIGHT << 4)
>> + | (CATPT_CHANNEL_LEFT_SURROUND << 8)
>> + | (CATPT_CHANNEL_RIGHT_SURROUND << 12));
>> +
>> + case CATPT_CHANNEL_CONFIG_4_POINT_0:
>> + return (GENMASK(31, 16) | CATPT_CHANNEL_LEFT
>> + | (CATPT_CHANNEL_CENTER << 4)
>> + | (CATPT_CHANNEL_RIGHT << 8)
>> + | (CATPT_CHANNEL_CENTER_SURROUND << 12));
>> +
>> + case CATPT_CHANNEL_CONFIG_5_POINT_0:
>> + return (GENMASK(31, 20) | CATPT_CHANNEL_LEFT
>> + | (CATPT_CHANNEL_CENTER << 4)
>> + | (CATPT_CHANNEL_RIGHT << 8)
>> + | (CATPT_CHANNEL_LEFT_SURROUND << 12)
>> + | (CATPT_CHANNEL_RIGHT_SURROUND << 16));
>> +
>> + case CATPT_CHANNEL_CONFIG_5_POINT_1:
>> + return (GENMASK(31, 24) | CATPT_CHANNEL_CENTER
>> + | (CATPT_CHANNEL_LEFT << 4)
>> + | (CATPT_CHANNEL_RIGHT << 8)
>> + | (CATPT_CHANNEL_LEFT_SURROUND << 12)
>> + | (CATPT_CHANNEL_RIGHT_SURROUND << 16)
>> + | (CATPT_CHANNEL_LFE << 20));
>> +
>> + case CATPT_CHANNEL_CONFIG_DUAL_MONO:
>> + return (GENMASK(31, 8) | CATPT_CHANNEL_LEFT
>> + | (CATPT_CHANNEL_LEFT << 4));
>> +
>> + default:
>> + return U32_MAX;
>> + }
>> +}
>> +
...
>> +#define DSP_VOLUME_MAX S32_MAX /* 0db */
>
>> +static const u32 volume_map[] = {
>> + DSP_VOLUME_MAX >> 30,
>> + DSP_VOLUME_MAX >> 29,
>> + DSP_VOLUME_MAX >> 28,
>> + DSP_VOLUME_MAX >> 27,
>> + DSP_VOLUME_MAX >> 26,
>> + DSP_VOLUME_MAX >> 25,
>> + DSP_VOLUME_MAX >> 24,
>> + DSP_VOLUME_MAX >> 23,
>> + DSP_VOLUME_MAX >> 22,
>> + DSP_VOLUME_MAX >> 21,
>> + DSP_VOLUME_MAX >> 20,
>> + DSP_VOLUME_MAX >> 19,
>> + DSP_VOLUME_MAX >> 18,
>> + DSP_VOLUME_MAX >> 17,
>> + DSP_VOLUME_MAX >> 16,
>> + DSP_VOLUME_MAX >> 15,
>> + DSP_VOLUME_MAX >> 14,
>> + DSP_VOLUME_MAX >> 13,
>> + DSP_VOLUME_MAX >> 12,
>> + DSP_VOLUME_MAX >> 11,
>> + DSP_VOLUME_MAX >> 10,
>> + DSP_VOLUME_MAX >> 9,
>> + DSP_VOLUME_MAX >> 8,
>> + DSP_VOLUME_MAX >> 7,
>> + DSP_VOLUME_MAX >> 6,
>> + DSP_VOLUME_MAX >> 5,
>> + DSP_VOLUME_MAX >> 4,
>> + DSP_VOLUME_MAX >> 3,
>> + DSP_VOLUME_MAX >> 2,
>> + DSP_VOLUME_MAX >> 1,
>> + DSP_VOLUME_MAX >> 0,
>> +};
>
> Why not to get rid of this table completely?
>
I don't see anything wrong with this lookup table. If you insist I can
get rid of it - that's the last piece of /haswell/ that survived the
"cleanup" afterall..
>> +static u32 ctlvol_to_dspvol(u32 value)
>> +{
>> + if (value >= ARRAY_SIZE(volume_map))
>> + value = 0;
>> + return volume_map[value];
>
> if (value > 30)
> value = 0;
> return DSP_VOLUME_MAX >> (30 - value);
>
I suppose 'DSP_VOLUME_STEP_MAX 30' is to be defined right next to
DSP_VOLUME_MAX.
As I said in earlier responses, please note size of this table is
helpful when assigning kcontrol info in catpt_volume_info(). Macro will
take its place then.
>> +}
>> +
>> +static u32 dspvol_to_ctlvol(u32 volume)
>> +{
>> + int i;
>> +
>> + for (i = 0; i < ARRAY_SIZE(volume_map); i++)
>> + if (volume_map[i] >= volume)
>> + return i;
>> + return i - 1;
>
> fls() ?
>
Well, fls(DSP_VOLUME_MAX) yields 31 which is inappropriate for
kcontrol with value range: 0-30.
Guess you meant: __fls(). Following is the implementation (accounting
for edge cases):
if (volume > DSP_VOLUME_MAX)
return DSP_VOLUME_STEP_MAX;
return volume ? __fls(volume) : 0;
Thanks,
Czarek
