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

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

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