RE: [PATCH v8 06/14] ASoC: Intel: catpt: PCM operations

["Rojewski, Cezary" <cezary.rojewski@intel.com>]

On 2020-09-24 4:22 PM, Andy Shevchenko wrote:
> On Wed, Sep 23, 2020 at 06:23:22PM +0000, Rojewski, Cezary wrote:
>> 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.
> 
> Nothing wrong with stereo. What I'm talking about is to move DUAL MONO from below to here because it looks more logical to have sequence by increasing amount of channels in use.
> 

_DUAL_MONO is so far below that even I - who is very familiar with the
catpt's code : ) - forgotten about it. Relocating below _MONO as
suggested.

>>>> +	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..
> 
> Because it's simply not needed. I specifically asked about gaps, now I don't
> see any possible justification why to keep the table. What are the benefits?
> 

volume_map no longer with us in v9, thanks.

>>>> +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.
> 
> Yes.
> 
>> 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;
> 
> No, I meant fls() due to defined respond to 0 value.
> But maybe __fls() works better in this case.
> 

__fls() is the method of choice in v9 for reasons above.

Thanks,
Czarek