Re: [RFC PATCH v2 4/6] ASoC: fsl_asrc_m2m: Add memory to memory function

[Pierre-Louis Bossart]

On 8/20/24 09:42, Jaroslav Kysela wrote:
> On 20. 08. 24 9:37, Shengjiu Wang wrote:
>> On Tue, Aug 20, 2024 at 2:59 PM Pierre-Louis Bossart
>>  wrote:
>>>
>>>
>>>
>>> On 8/20/24 04:53, Shengjiu Wang wrote:
>>>> On Mon, Aug 19, 2024 at 3:42 PM Pierre-Louis Bossart
>>>>  wrote:
>>>>>
>>>>>
>>>>>
>>>>> On 8/16/24 12:42, Shengjiu Wang wrote:
>>>>>> Implement the ASRC memory to memory function using
>>>>>> the compress framework, user can use this function with
>>>>>> compress ioctl interface.
>>>>>>
>>>>>> Define below private metadata key value for output
>>>>>> format, output rate and ratio modifier configuration.
>>>>>> ASRC_OUTPUT_FORMAT 0x8001
>>>>>> ASRC_OUTPUT_RATE   0x8002
>>>>>> ASRC_RATIO_MOD 0x8003
>>>>>
>>>>> Can the output format/rate change at run-time?
>>>>
>>>> Seldom I think.
>>>>
>>>>>
>>>>> If no, then these parameters should be moved somewhere else - e.g.
>>>>> hw_params or something.
>>>>
>>>> This means I will do some changes in compress_params.h, add
>>>> output format and output rate definition, follow Jaroslav's example
>>>> right?
>>>
>>> yes, having parameters for the PCM case would make sense.
>>>
>>>>> I am still not very clear on the expanding the SET_METADATA ioctl to
>>>>> deal with the ratio changes. This isn't linked to the control layer as
>>>>> suggested before, and there's no precedent of calling it multiple
>>>>> times
>>>>> during streaming.
>>>>
>>>> Which control layer? if you means the snd_kcontrol_new?  it is bound
>>>> with sound card,  but in my case,  I need to the control bind with
>>>> the snd_compr_stream to support multi streams/instances.
>>>
>>> I can only quote Jaroslav's previous answer:
>>>
>>> "
>>> This argument is not valid. The controls are bound to the card, but the
>>> element identifiers have already iface (interface), device and subdevice
>>> numbers. We are using controls for PCM devices for example. The binding
>>> is straight.
>>>
>>> Just add SNDRV_CTL_ELEM_IFACE_COMPRESS define and specify the compress
>>> device number in the 'struct snd_ctl_elem_id'.
>>> "
>>
>> I don't think it is doable,  or at least I don't know how to do it.
>>
>> My case is just one card/one device/one subdevice.  can't use it to
>> distinguish multi streams.
> 
> I already wrote that the compress core code should be extended to
> support subdevices like other ALSA APIs. I'll work on it. For now, just
> add support for one converter.

I am not sure I get the benefits of subdevices in this context.

Can we not use different devices already, one per hardware ASRC instance?

Put differently, what would be the difference between a card with N
compressed devices or a card with 1 compressed device and N subdevices?

Re: [RFC PATCH v2 4/6] ASoC: fsl_asrc_m2m: Add memory to memory function

[Pierre-Louis Bossart]

On 8/20/24 04:53, Shengjiu Wang wrote:
> On Mon, Aug 19, 2024 at 3:42 PM Pierre-Louis Bossart
>  wrote:
>>
>>
>>
>> On 8/16/24 12:42, Shengjiu Wang wrote:
>>> Implement the ASRC memory to memory function using
>>> the compress framework, user can use this function with
>>> compress ioctl interface.
>>>
>>> Define below private metadata key value for output
>>> format, output rate and ratio modifier configuration.
>>> ASRC_OUTPUT_FORMAT 0x8001
>>> ASRC_OUTPUT_RATE   0x8002
>>> ASRC_RATIO_MOD 0x8003
>>
>> Can the output format/rate change at run-time?
> 
> Seldom I think.
> 
>>
>> If no, then these parameters should be moved somewhere else - e.g.
>> hw_params or something.
> 
> This means I will do some changes in compress_params.h, add
> output format and output rate definition, follow Jaroslav's example
> right?

yes, having parameters for the PCM case would make sense.

>> I am still not very clear on the expanding the SET_METADATA ioctl to
>> deal with the ratio changes. This isn't linked to the control layer as
>> suggested before, and there's no precedent of calling it multiple times
>> during streaming.
> 
> Which control layer? if you means the snd_kcontrol_new?  it is bound
> with sound card,  but in my case,  I need to the control bind with
> the snd_compr_stream to support multi streams/instances.

I can only quote Jaroslav's previous answer:

"
This argument is not valid. The controls are bound to the card, but the
element identifiers have already iface (interface), device and subdevice
numbers. We are using controls for PCM devices for example. The binding
is straight.

Just add SNDRV_CTL_ELEM_IFACE_COMPRESS define and specify the compress
device number in the 'struct snd_ctl_elem_id'.
"

>> I also wonder how it was tested since tinycompress does not support this?
> 
> I wrote a unit test to test these ASRC M2M functions.

This should be shared IMHO, usually when we add/extend a new interface
it's best to have a userspace test program that can be used by others.

>>> +static int fsl_asrc_m2m_fill_codec_caps(struct fsl_asrc *asrc,
>>> + struct snd_compr_codec_caps *codec)
>>> +{
>>> + struct fsl_asrc_m2m_cap cap;
>>> + __u32 rates[MAX_NUM_BITRATES];
>>> + snd_pcm_format_t k;
>>> + int i = 0, j = 0;
>>> + int ret;
>>> +
>>> + ret = asrc->m2m_get_cap(&cap);
>>> + if (ret)
>>> + return -EINVAL;
>>> +
>>> + if (cap.rate_in & SNDRV_PCM_RATE_5512)
>>> + rates[i++] = snd_pcm_rate_bit_to_rate(SNDRV_PCM_RATE_5512);
>>
>> this doesn't sound compatible with the patch2 definitions?
>>
>> cap->rate_in = SNDRV_PCM_RATE_8000_768000;
> 
> This ASRC M2M driver is designed for two kinds of hw ASRC modules.
> 
> one cap is : cap->rate_in = SNDRV_PCM_RATE_8000_192000 | SNDRV_PCM_RATE_5512;
> another is : cap->rate_in = SNDRV_PCM_RATE_8000_768000;
> they are in patch2 and patch3
> 
> 
>>
>>> + if (cap.rate_in & SNDRV_PCM_RATE_8000)
>>> + rates[i++] = snd_pcm_rate_bit_to_rate(SNDRV_PCM_RATE_8000);
>>> + if (cap.rate_in & SNDRV_PCM_RATE_11025)
>>> + rates[i++] = snd_pcm_rate_bit_to_rate(SNDRV_PCM_RATE_11025);
>>> + if (cap.rate_in & SNDRV_PCM_RATE_16000)
>>> + rates[i++] = snd_pcm_rate_bit_to_rate(SNDRV_PCM_RATE_16000);
>>> + if (cap.rate_in & SNDRV_PCM_RATE_22050)
>>> + rates[i++] = snd_pcm_rate_bit_to_rate(SNDRV_PCM_RATE_22050);
>>
>> missing 24 kHz
> 
> There is no SNDRV_PCM_RATE_24000 in ALSA.

Right, but that doesn't mean 24kHz cannot be supported. We use
constraints in those cases. see quote from Takashi found with a 2s
Google search

https://mailman.alsa-project.org/pipermail/alsa-devel/2013-November/069356.html

"
CONTINUOUS means that any rate between the specified min and max is
fine, if no min or max is specified any rate is fine. KNOT means there
are rates supported other than the standard rates defines by ALSA, but
the other rates are enumerable. You'd typically specify them by
explicitly listing them all and use a list constraint or you'd use one
of the ratio constraints.
"

>>> + if (cap.rate_in & SNDRV_PCM_RATE_32000)
>>> + rates[i++] = snd_pcm_rate_bit_to_rate(SNDRV_PCM_RATE_32000);
>>> + if (cap.rate_in & SNDRV_PCM_RATE_44100)
>>> + rates[i++] = snd_pcm_rate_bit_to_rate(SNDRV_PCM_RATE_44100);
>>> + if (cap.rate_in & SNDRV_PCM_RATE_48000)
>>> + rates[i++] = snd_pcm_rate_bit_to_rate(SNDRV_PCM_RATE_48000);
>>
>> missing 64kHz
> 
> Yes, will add it.

Re: [RFC PATCH v2 4/6] ASoC: fsl_asrc_m2m: Add memory to memory function

[Pierre-Louis Bossart]

On 8/16/24 12:42, Shengjiu Wang wrote:
> Implement the ASRC memory to memory function using
> the compress framework, user can use this function with
> compress ioctl interface.
> 
> Define below private metadata key value for output
> format, output rate and ratio modifier configuration.
> ASRC_OUTPUT_FORMAT 0x8001
> ASRC_OUTPUT_RATE   0x8002
> ASRC_RATIO_MOD 0x8003

Can the output format/rate change at run-time?

If no, then these parameters should be moved somewhere else - e.g.
hw_params or something.

I am still not very clear on the expanding the SET_METADATA ioctl to
deal with the ratio changes. This isn't linked to the control layer as
suggested before, and there's no precedent of calling it multiple times
during streaming.

I also wonder how it was tested since tinycompress does not support this?


> +static int fsl_asrc_m2m_fill_codec_caps(struct fsl_asrc *asrc,
> + struct snd_compr_codec_caps *codec)
> +{
> + struct fsl_asrc_m2m_cap cap;
> + __u32 rates[MAX_NUM_BITRATES];
> + snd_pcm_format_t k;
> + int i = 0, j = 0;
> + int ret;
> +
> + ret = asrc->m2m_get_cap(&cap);
> + if (ret)
> + return -EINVAL;
> +
> + if (cap.rate_in & SNDRV_PCM_RATE_5512)
> + rates[i++] = snd_pcm_rate_bit_to_rate(SNDRV_PCM_RATE_5512);

this doesn't sound compatible with the patch2 definitions?

cap->rate_in = SNDRV_PCM_RATE_8000_768000;

> + if (cap.rate_in & SNDRV_PCM_RATE_8000)
> + rates[i++] = snd_pcm_rate_bit_to_rate(SNDRV_PCM_RATE_8000);
> + if (cap.rate_in & SNDRV_PCM_RATE_11025)
> + rates[i++] = snd_pcm_rate_bit_to_rate(SNDRV_PCM_RATE_11025);
> + if (cap.rate_in & SNDRV_PCM_RATE_16000)
> + rates[i++] = snd_pcm_rate_bit_to_rate(SNDRV_PCM_RATE_16000);
> + if (cap.rate_in & SNDRV_PCM_RATE_22050)
> + rates[i++] = snd_pcm_rate_bit_to_rate(SNDRV_PCM_RATE_22050);

missing 24 kHz

> + if (cap.rate_in & SNDRV_PCM_RATE_32000)
> + rates[i++] = snd_pcm_rate_bit_to_rate(SNDRV_PCM_RATE_32000);
> + if (cap.rate_in & SNDRV_PCM_RATE_44100)
> + rates[i++] = snd_pcm_rate_bit_to_rate(SNDRV_PCM_RATE_44100);
> + if (cap.rate_in & SNDRV_PCM_RATE_48000)
> + rates[i++] = snd_pcm_rate_bit_to_rate(SNDRV_PCM_RATE_48000);

missing 64kHz

> + if (cap.rate_in & SNDRV_PCM_RATE_88200)
> + rates[i++] = snd_pcm_rate_bit_to_rate(SNDRV_PCM_RATE_88200);
> + if (cap.rate_in & SNDRV_PCM_RATE_96000)
> + rates[i++] = snd_pcm_rate_bit_to_rate(SNDRV_PCM_RATE_96000);
> + if (cap.rate_in & SNDRV_PCM_RATE_176400)
> + rates[i++] = snd_pcm_rate_bit_to_rate(SNDRV_PCM_RATE_176400);
> + if (cap.rate_in & SNDRV_PCM_RATE_192000)
> + rates[i++] = snd_pcm_rate_bit_to_rate(SNDRV_PCM_RATE_192000);
> + if (cap.rate_in & SNDRV_PCM_RATE_352800)
> + rates[i++] = snd_pcm_rate_bit_to_rate(SNDRV_PCM_RATE_352800);
> + if (cap.rate_in & SNDRV_PCM_RATE_384000)
> + rates[i++] = snd_pcm_rate_bit_to_rate(SNDRV_PCM_RATE_384000);
> + if (cap.rate_in & SNDRV_PCM_RATE_705600)
> + rates[i++] = snd_pcm_rate_bit_to_rate(SNDRV_PCM_RATE_705600);
> + if (cap.rate_in & SNDRV_PCM_RATE_768000)
> + rates[i++] = snd_pcm_rate_bit_to_rate(SNDRV_PCM_RATE_768000);
> +
> + pcm_for_each_format(k) {
> + if (pcm_format_to_bits(k) & cap.fmt_in) {
> + codec->descriptor[j].max_ch = cap.chan_max;
> + memcpy(codec->descriptor[j].sample_rates, rates, i * 
> sizeof(__u32));
> + codec->descriptor[j].num_sample_rates = i;
> + codec->descriptor[j].formats = k;
> + j++;
> + }
> + }
> +
> + codec->codec = SND_AUDIOCODEC_PCM;
> + codec->num_descriptors = j;
> + return 0;

Re: [RFC PATCH 1/6] ALSA: compress: add Sample Rate Converter codec support

[Pierre-Louis Bossart]

On 8/14/24 13:12, Shengjiu Wang wrote:
> On Wed, Aug 14, 2024 at 5:40 PM Pierre-Louis Bossart
>  wrote:
>>
>>
>>> Yes, to go further, I think we can use SND_AUDIOCODEC_PCM, then
>>> the SRC type will be dropped.
>>
>> sounds good.
>>
>>> But my understanding of the control means the .set_metadata() API, right?
>>> As I said, the output rate, output format, and ratio modifier are applied to
>>> the instances of ASRC,  which is the snd_compr_stream in driver.
>>> so only the .set_metadata() API can be used for these purposes.
>>
>> Humm, this is more controversial.
>>
>> The term 'metadata' really referred to known information present in
>> headers or additional ID3 tags and not in the compressed file itself.
>> The .set_metadata was assumed to be called ONCE before decoding.
>>
>> But here you have a need to update the ratio modifier on a regular basis
>> to compensate for the drift. This isn't what this specific callback was
>> designed for. We could change and allow this callback to be used
>> multiple times, but then this could create problems for existing
>> implementations which cannot deal with modified metadata on the fly.
> 
> .set_metadata can be called multi times now, no need to change currently.

Not really, this set_metadata() callback is used only for gapless
transitions between tracks, see fcplay.c in tinycompress.

And now I am really confused because tinycompress uses an IOCTL directly:

metadata.key = SNDRV_COMPRESS_ENCODER_PADDING;
metadata.value[0] = mdata->encoder_padding;
if (ioctl(compress->fd, SNDRV_COMPRESS_SET_METADATA, &metadata))

Whereas you want to use the ops callback directly from the control layer?

What would present a userspace program from using the ioctl directly
then? In that case, why do we need the control? I must be missing something.


>> And then there's the problem of defining a 'key' for the metadata. the
>> definition of the key is a u32, so there's plenty of space for different
>> implementations, but a collision is possible. We'd need an agreement on
>> how to allocate keys to different solutions without changing the header
>> file for every implementation.
> 
> Can we define a private space for each case?   For example the key larger
> than 0x8000 is private, each driver can define it by themself?

that would be a possibility indeed - provided that the opens above are
straightened out.

>> It sounds like we'd need a 'runtime params' callback - unless there's a
>> better trick to tie the control and compress layers?

Re: [RFC PATCH 1/6] ALSA: compress: add Sample Rate Converter codec support

[Pierre-Louis Bossart]

> Yes, to go further, I think we can use SND_AUDIOCODEC_PCM, then
> the SRC type will be dropped.

sounds good.

> But my understanding of the control means the .set_metadata() API, right?
> As I said, the output rate, output format, and ratio modifier are applied to
> the instances of ASRC,  which is the snd_compr_stream in driver.
> so only the .set_metadata() API can be used for these purposes.

Humm, this is more controversial.

The term 'metadata' really referred to known information present in
headers or additional ID3 tags and not in the compressed file itself.
The .set_metadata was assumed to be called ONCE before decoding.

But here you have a need to update the ratio modifier on a regular basis
to compensate for the drift. This isn't what this specific callback was
designed for. We could change and allow this callback to be used
multiple times, but then this could create problems for existing
implementations which cannot deal with modified metadata on the fly.

And then there's the problem of defining a 'key' for the metadata. the
definition of the key is a u32, so there's plenty of space for different
implementations, but a collision is possible. We'd need an agreement on
how to allocate keys to different solutions without changing the header
file for every implementation.

It sounds like we'd need a 'runtime params' callback - unless there's a
better trick to tie the control and compress layers?

Re: [RFC PATCH 1/6] ALSA: compress: add Sample Rate Converter codec support

[Pierre-Louis Bossart]

On 8/12/24 15:31, Jaroslav Kysela wrote:
> On 12. 08. 24 12:24, Shengjiu Wang wrote:
>> On Fri, Aug 9, 2024 at 10:01 PM Jaroslav Kysela  wrote:
>>>
>>> On 09. 08. 24 14:52, Pierre-Louis Bossart wrote:
>>>
>>>>> And metadata
>>>>> ioctl can be called many times which can meet the ratio modifier
>>>>> requirement (ratio may be drift on the fly)
>>>>
>>>> Interesting, that's yet another way of handling the drift with
>>>> userspace
>>>> modifying the ratio dynamically. That's different to what I've seen
>>>> before.
>>>
>>> Note that the "timing" is managed by the user space with this scheme.
>>>
>>>>> And compress API uses codec as the unit for capability query and
>>>>> parameter setting,  So I think need to define "SND_AUDIOCODEC_SRC'
>>>>> and 'struct snd_dec_src',  for the 'snd_dec_src' just defined output
>>>>> format and output rate, channels definition just reuse the
>>>>> snd_codec.ch_in.
>>>>
>>>> The capability query is an interesting point as well, it's not clear
>>>> how
>>>> to expose to userspace what this specific implementation can do, while
>>>> at the same time *requiring* userpace to update the ratio dynamically.
>>>> For something like this to work, userspace needs to have pre-existing
>>>> information on how the SRC works.
>>>
>>> Yes, it's about abstraction. The user space wants to push data, read
>>> data back
>>> converted to the target rate and eventually modify the drift using a
>>> control
>>> managing clocks using own way. We can eventually assume, that if this
>>> control
>>> does not exist, the drift cannot be controlled. Also, nice thing is
>>> that the
>>> control has min and max values (range), so driver can specify the
>>> drift range,
>>> too.
>>>
>>> And again, look to "PCM Rate Shift 10" control implementation in
>>> sound/drivers/aloop.c. It would be nice to have the base offset for the
>>> shift/drift/pitch value standardized.
>>
>> Thanks.
>>
>> But the ASRC driver I implemented is different, I just register one sound
>> card, one device/subdevice.  but the ASRC hardware support 4 instances
>> together, so user can open the card device 4 times to create 4 instances
>> then the controls can only bind with compress streams.
> 
> It's just a reason to add the subdevice code for the compress offload
> layer like we have in other APIs for overall consistency. I'll try to
> work on this.

I thought this was supported already? I remember there was a request to
enable more than one compressed stream for enhanced cross-fade support
with different formats? That isn't supported with the single-device +
PARTIAL_DRAIN method.

Vinod?

>> I think I can remove the 'SNDRV_COMPRESS_SRC_RATIO_MOD',
> 
> Yes.
> 
>> Only define a private type for driver,  which means only the ASRC driver
>> and its user application know the type.
> 
> The control API should be used for this IMHO.

Agree, this would be a 'clean' split where the compress API is used for
the data parts and the control parts used otherwise to alter the ratio
or whatever else is needed.

>> For the change in 'include/uapi/sound/compress_params.h",  should I
>> keep them,  is there any other suggestion for them?

You can add the SRC type but if you use a control for the parameters you
don't need to add anything for the encoder options, do you?

Re: [RFC PATCH 1/6] ALSA: compress: add Sample Rate Converter codec support

[Pierre-Louis Bossart]

>>> And metadata
>>> ioctl can be called many times which can meet the ratio modifier
>>> requirement (ratio may be drift on the fly)
>>
>> Interesting, that's yet another way of handling the drift with userspace
>> modifying the ratio dynamically. That's different to what I've seen
>> before.
> 
> Note that the "timing" is managed by the user space with this scheme.
> 
>>> And compress API uses codec as the unit for capability query and
>>> parameter setting,  So I think need to define "SND_AUDIOCODEC_SRC'
>>> and 'struct snd_dec_src',  for the 'snd_dec_src' just defined output
>>> format and output rate, channels definition just reuse the
>>> snd_codec.ch_in.
>>
>> The capability query is an interesting point as well, it's not clear how
>> to expose to userspace what this specific implementation can do, while
>> at the same time *requiring* userpace to update the ratio dynamically.
>> For something like this to work, userspace needs to have pre-existing
>> information on how the SRC works.
> 
> Yes, it's about abstraction. The user space wants to push data, read
> data back converted to the target rate and eventually modify the drift
> using a control managing clocks using own way. We can eventually assume,
> that if this control does not exist, the drift cannot be controlled.
> Also, nice thing is that the control has min and max values (range), so
> driver can specify the drift range, too.

This mode of operation would be fine, but if you add the SRC as part of
the processing allowed in a compressed stream, it might be used in a
'regular' real-time pipeline and the arguments  and implementation could
be very different.

In other words, this SRC support looks like an extension of the compress
API for a very narrow usage restricted to the memory-to-memory case
only. I worry a bit about the next steps... Are there going to be other
types of PCM processing like this, and if yes, how would we know if they
are intended to be used for the 'regular' compress API or for the
memory-to-memory scheme?

Re: [RFC PATCH 1/6] ALSA: compress: add Sample Rate Converter codec support

[Pierre-Louis Bossart]

> Why I use the metadata ioctl is because the ALSA controls are binding
> to the sound card.  What I want is the controls can be bound to
> snd_compr_stream, because the ASRC compress sound card can
> support multi instances ( the ASRC can support multi conversion in
> parallel).   The ALSA controls can't be used for this case,  the only
> choice in current compress API is metadata ioctl. 

I don't know if there is really a technical limitation for this, this is
for Jaroslav to comment. I am not sure why it would be a problem to e.g.
have a volume control prior to an encoder or after a decoder.

> And metadata
> ioctl can be called many times which can meet the ratio modifier
> requirement (ratio may be drift on the fly)

Interesting, that's yet another way of handling the drift with userspace
modifying the ratio dynamically. That's different to what I've seen before.

> And compress API uses codec as the unit for capability query and
> parameter setting,  So I think need to define "SND_AUDIOCODEC_SRC'
> and 'struct snd_dec_src',  for the 'snd_dec_src' just defined output
> format and output rate, channels definition just reuse the snd_codec.ch_in.

The capability query is an interesting point as well, it's not clear how
to expose to userspace what this specific implementation can do, while
at the same time *requiring* userpace to update the ratio dynamically.
For something like this to work, userspace needs to have pre-existing
information on how the SRC works.

Re: [RFC PATCH 1/6] ALSA: compress: add Sample Rate Converter codec support

[Pierre-Louis Bossart]

 Then there's the issue of parameters, we chose to only add parameters
 for standard encoders/decoders. Post-processing is highly specific and
 the parameter definitions varies from one implementation to another -
 and usually parameters are handled in an opaque way with binary
 controls. This is best handled with a UUID that needs to be known only
 to applications and low-level firmware/hardware, the kernel code should
 not have to be modified for each and every processing and to add new
 parameters. It just does not scale and it's unmaintainable.

 At the very least if you really want to use this compress API,
 extend it
 to use a non-descript "UUID-defined" type and an opaque set of
 parameters with this UUID passed in a header.
>>>
>>> We don't need to use UUID-defined scheme for simple (A)SRC
>>> implementation. As I noted, the specific runtime controls may use
>>> existing ALSA control API.
>>
>> "Simple (A)SRC" is an oxymoron. There are multiple ways to define the
>> performance, and how the drift estimator is handled. There's nothing
>> simple if you look under the hood. The SOF implementation has for
>> example those parameters:
>>
>> uint32_t source_rate;   /**< Define fixed source rate or */
>>     /**< use 0 to indicate need to get */
>>     /**< the rate from stream */
>> uint32_t sink_rate; /**< Define fixed sink rate or */
>>     /**< use 0 to indicate need to get */
>>     /**< the rate from stream */
>> uint32_t asynchronous_mode; /**< synchronous 0, asynchronous 1 */
>>     /**< When 1 the ASRC tracks and */
>>     /**< compensates for drift. */
>> uint32_t operation_mode;    /**< push 0, pull 1, In push mode the */
>>     /**< ASRC consumes a defined number */
>>     /**< of frames at input, with varying */
>>     /**< number of frames at output. */
>>     /**< In pull mode the ASRC outputs */
>>     /**< a defined number of frames while */
>>     /**< number of input frames varies. */
>>
>> They are clearly different from what is suggested above with a 'ratio-
>> mod'.
> 
> I don't think so. The proposed (A)SRC for compress-accel is just one
> case for the above configs where the input is known and output is
> controlled by the requested rate. The I/O mechanism is abstracted enough
> in this case and the driver/hardware/firmware must follow it.

ASRC is usually added when the nominal rates are known but the clock
sources differ and the drift needs to be estimated at run-time and the
coefficients or interpolation modified dynamically

If the ratio is known exactly and there's no clock drift, then it's a
different problem where the filter coefficients are constant.

>> Same if you have a 'simple EQ'. there are dozens of ways to implement
>> the functionality with FIR, IIR or a combination of the two, and
>> multiple bands.
>>
>> The point is that you have to think upfront about a generic way to pass
>> parameters. We didn't have to do it for encoders/decoders because we
>> only catered to well-documented standard solutions only. By choosing to
>> support PCM processing, a new can of worms is now open.
>>
>> I repeat: please do not make the mistake of listing all processing with
>> an enum and a new structure for parameters every time someone needs a
>> specific transform in their pipeline. We made that mistake with SOF and
>> had to backtrack rather quickly. The only way to scale is an identifier
>> that is NOT included in the kernel code but is known to higher and
>> lower-levels only.
> 
> There are two ways - black box (UUID - as you suggested) - or well
> defined purpose (abstraction). For your example 'simple EQ', the
> parameters should be the band (frequency range) volume values. It's
> abstract and the real filters (resp. implementation) used behind may
> depend on the hardware/driver capabilities.

Indeed there is a possibility that the parameters are high-level, but
that would require firmware or hardware to be able to generate actual
coefficients from those parameters. That usually requires some advanced
math which isn't necessarily obvious to implement with fixed-point hardware.

> From my view, the really special cases may be handled as black box, but
> others like (A)SRC should follow some well-defined abstraction IMHO to
> not force user space to handle all special cases.

I am not against the high-level abstractions, e.g. along the lines of
what Android defined:
https://developer.android.com/reference/android/media/audiofx/AudioEffect

That's not sufficient however, we also need to make sure there's an
ability to provide pre-computed coefficients in an opaque manner for
processing that doesn't fit in the well-defined cases. In practice there
are very few 3rd party IP that fits in well-defined cases, everyone has
secret-sauce parameters and options.

Re: [RFC PATCH 1/6] ALSA: compress: add Sample Rate Converter codec support

[Pierre-Louis Bossart]

 files changed, 10 insertions(+), 1 deletion(-)
>
> diff --git a/include/uapi/sound/compress_offload.h b/include/uapi/
> sound/compress_offload.h
> index 98772b0cbcb7..8b2b72f94e26 100644
> --- a/include/uapi/sound/compress_offload.h
> +++ b/include/uapi/sound/compress_offload.h
> @@ -112,10 +112,12 @@ struct snd_compr_codec_caps {
>    * end of the track
>    * @SNDRV_COMPRESS_ENCODER_DELAY: no of samples inserted by the
> encoder at the
>    * beginning of the track
> + * @SNDRV_COMPRESS_SRC_RATIO_MOD: Resampling Ratio Modifier for
> sample rate converter
>    */
>   enum sndrv_compress_encoder {
>    SNDRV_COMPRESS_ENCODER_PADDING = 1,
>    SNDRV_COMPRESS_ENCODER_DELAY = 2,
> + SNDRV_COMPRESS_SRC_RATIO_MOD = 3,
>   };

 this sounds wrong to me. The sample rate converter is not an "encoder",
 and the properties for padding/delay are totally specific to an encoder
 function.
>>>
>>> There is only decoder and encoder definition for compress,  I know
>>> it is difficult to add SRC to encoder or decoder classification,
>>> SRC is a Post Processing.  I hope you can have a recommandation
>>
>> I don't. I think we're blurring layers in a really odd way.
>>
>> The main reason why the compress API was added is to remove the
>> byte-to-time conversions. But that's clearly not useful for a
>> post-processing of PCM data, where the bitrate is constant. It really
>> feels like we're adding this memory-to-memory API to the compress
>> framework because we don't have anything else available, not because it
>> makes sense to do so.
> 
> It makes sense to offload decoder/encoder tasks as batch processing for
> standard compress stream and return back result (PCM stream or encoded
> stream) to user space. So it makes sense to use the compress interface
> (parameters handshake) for it. Let's talk about the proper SRC extension.
> 
> For SRC and dynamic rate modification. I would just create an ALSA
> control for this. We are already using the "PCM Rate Shift 10"
> control in the sound/drivers/aloop.c for this purpose (something like
> pitch in MIDI) for example. There is no requirement to add this function
> through metadata ioctls. As bonus, this control can be monitored with
> multiple tasks.

this wouldn't work when the rate is estimated in firmware/hardware,
which is precisely what the 'asynchronous' part of ASRC does.


>> Then there's the issue of parameters, we chose to only add parameters
>> for standard encoders/decoders. Post-processing is highly specific and
>> the parameter definitions varies from one implementation to another -
>> and usually parameters are handled in an opaque way with binary
>> controls. This is best handled with a UUID that needs to be known only
>> to applications and low-level firmware/hardware, the kernel code should
>> not have to be modified for each and every processing and to add new
>> parameters. It just does not scale and it's unmaintainable.
>>
>> At the very least if you really want to use this compress API, extend it
>> to use a non-descript "UUID-defined" type and an opaque set of
>> parameters with this UUID passed in a header.
> 
> We don't need to use UUID-defined scheme for simple (A)SRC
> implementation. As I noted, the specific runtime controls may use
> existing ALSA control API.

"Simple (A)SRC" is an oxymoron. There are multiple ways to define the
performance, and how the drift estimator is handled. There's nothing
simple if you look under the hood. The SOF implementation has for
example those parameters:

uint32_t source_rate;   /**< Define fixed source rate or */
/**< use 0 to indicate need to get */
/**< the rate from stream */
uint32_t sink_rate; /**< Define fixed sink rate or */
/**< use 0 to indicate need to get */
/**< the rate from stream */
uint32_t asynchronous_mode; /**< synchronous 0, asynchronous 1 */
/**< When 1 the ASRC tracks and */
/**< compensates for drift. */
uint32_t operation_mode;/**< push 0, pull 1, In push mode the */
/**< ASRC consumes a defined number */
/**< of frames at input, with varying */
/**< number of frames at output. */
/**< In pull mode the ASRC outputs */
/**< a defined number of frames while */
/**< number of input frames varies. */

They are clearly different from what is suggested above with a 'ratio-mod'.

Same if you have a 'simple EQ'. there are dozens of ways to implement
the functionality with FIR, IIR or a combination of the two, and
multiple bands.

The point is that you have to think upfront about a gener

Re: [RFC PATCH 1/6] ALSA: compress: add Sample Rate Converter codec support

[Pierre-Louis Bossart]

On 8/8/24 11:17, Shengjiu Wang wrote:
> On Tue, Aug 6, 2024 at 7:25 PM Pierre-Louis Bossart
>  wrote:
>>
>>
>>
>> On 8/6/24 12:26, Shengjiu Wang wrote:
>>> Add Sample Rate Converter(SRC) codec support, define the output
>>> format and rate for SRC.
>>>
>>> Signed-off-by: Shengjiu Wang 
>>> ---
>>>  include/uapi/sound/compress_offload.h | 2 ++
>>>  include/uapi/sound/compress_params.h  | 9 -
>>>  2 files changed, 10 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/include/uapi/sound/compress_offload.h 
>>> b/include/uapi/sound/compress_offload.h
>>> index 98772b0cbcb7..8b2b72f94e26 100644
>>> --- a/include/uapi/sound/compress_offload.h
>>> +++ b/include/uapi/sound/compress_offload.h
>>> @@ -112,10 +112,12 @@ struct snd_compr_codec_caps {
>>>   * end of the track
>>>   * @SNDRV_COMPRESS_ENCODER_DELAY: no of samples inserted by the encoder at 
>>> the
>>>   * beginning of the track
>>> + * @SNDRV_COMPRESS_SRC_RATIO_MOD: Resampling Ratio Modifier for sample 
>>> rate converter
>>>   */
>>>  enum sndrv_compress_encoder {
>>>   SNDRV_COMPRESS_ENCODER_PADDING = 1,
>>>   SNDRV_COMPRESS_ENCODER_DELAY = 2,
>>> + SNDRV_COMPRESS_SRC_RATIO_MOD = 3,
>>>  };
>>
>> this sounds wrong to me. The sample rate converter is not an "encoder",
>> and the properties for padding/delay are totally specific to an encoder
>> function.
> 
> There is only decoder and encoder definition for compress,  I know
> it is difficult to add SRC to encoder or decoder classification,
> SRC is a Post Processing.  I hope you can have a recommandation

I don't. I think we're blurring layers in a really odd way.

The main reason why the compress API was added is to remove the
byte-to-time conversions. But that's clearly not useful for a
post-processing of PCM data, where the bitrate is constant. It really
feels like we're adding this memory-to-memory API to the compress
framework because we don't have anything else available, not because it
makes sense to do so.

Then there's the issue of parameters, we chose to only add parameters
for standard encoders/decoders. Post-processing is highly specific and
the parameter definitions varies from one implementation to another -
and usually parameters are handled in an opaque way with binary
controls. This is best handled with a UUID that needs to be known only
to applications and low-level firmware/hardware, the kernel code should
not have to be modified for each and every processing and to add new
parameters. It just does not scale and it's unmaintainable.

At the very least if you really want to use this compress API, extend it
to use a non-descript "UUID-defined" type and an opaque set of
parameters with this UUID passed in a header.

Re: [RFC PATCH 1/6] ALSA: compress: add Sample Rate Converter codec support

[Pierre-Louis Bossart]

On 8/6/24 12:26, Shengjiu Wang wrote:
> Add Sample Rate Converter(SRC) codec support, define the output
> format and rate for SRC.
> 
> Signed-off-by: Shengjiu Wang 
> ---
>  include/uapi/sound/compress_offload.h | 2 ++
>  include/uapi/sound/compress_params.h  | 9 -
>  2 files changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/include/uapi/sound/compress_offload.h 
> b/include/uapi/sound/compress_offload.h
> index 98772b0cbcb7..8b2b72f94e26 100644
> --- a/include/uapi/sound/compress_offload.h
> +++ b/include/uapi/sound/compress_offload.h
> @@ -112,10 +112,12 @@ struct snd_compr_codec_caps {
>   * end of the track
>   * @SNDRV_COMPRESS_ENCODER_DELAY: no of samples inserted by the encoder at 
> the
>   * beginning of the track
> + * @SNDRV_COMPRESS_SRC_RATIO_MOD: Resampling Ratio Modifier for sample rate 
> converter
>   */
>  enum sndrv_compress_encoder {
>   SNDRV_COMPRESS_ENCODER_PADDING = 1,
>   SNDRV_COMPRESS_ENCODER_DELAY = 2,
> + SNDRV_COMPRESS_SRC_RATIO_MOD = 3,
>  };

this sounds wrong to me. The sample rate converter is not an "encoder",
and the properties for padding/delay are totally specific to an encoder
function.

The other point is that I am not sure how standard this ratio_mod
parameter is. This could be totally specific to a specific
implementation, and another ASRC might have different parameters.

>  
>  /**
> diff --git a/include/uapi/sound/compress_params.h 
> b/include/uapi/sound/compress_params.h
> index ddc77322d571..0843773ea6b4 100644
> --- a/include/uapi/sound/compress_params.h
> +++ b/include/uapi/sound/compress_params.h
> @@ -43,7 +43,8 @@
>  #define SND_AUDIOCODEC_BESPOKE   ((__u32) 0x000E)
>  #define SND_AUDIOCODEC_ALAC  ((__u32) 0x000F)
>  #define SND_AUDIOCODEC_APE   ((__u32) 0x0010)
> -#define SND_AUDIOCODEC_MAX   SND_AUDIOCODEC_APE
> +#define SND_AUDIOCODEC_SRC   ((__u32) 0x0011)
> +#define SND_AUDIOCODEC_MAX   SND_AUDIOCODEC_SRC

I am not sure this is wise to change such definitions?
>  
>  /*
>   * Profile and modes are listed with bit masks. This allows for a
> @@ -324,6 +325,11 @@ struct snd_dec_ape {
>   __u32 seek_table_present;
>  } __attribute__((packed, aligned(4)));
>  
> +struct snd_dec_src {
> + __u32 format_out;
> + __u32 rate_out;
> +} __attribute__((packed, aligned(4)));

Again I am not sure how standard those parameters are, and even if they
were if their representation is reusable.

And the compressed API has a good split between encoders and decoders, I
am not sure how an SRC can be classified as either.

>  union snd_codec_options {
>   struct snd_enc_wma wma;
>   struct snd_enc_vorbis vorbis;
> @@ -334,6 +340,7 @@ union snd_codec_options {
>   struct snd_dec_wma wma_d;
>   struct snd_dec_alac alac_d;
>   struct snd_dec_ape ape_d;
> + struct snd_dec_src src;
>  } __attribute__((packed, aligned(4)));
>  
>  /** struct snd_codec_desc - description of codec capabilities

Re: [PATCH v15 00/16] Add audio support in v4l2 framework

[Pierre-Louis Bossart]

On 5/9/24 06:13, Jaroslav Kysela wrote:
> On 09. 05. 24 12:44, Shengjiu Wang wrote:
 mem2mem is just like the decoder in the compress pipeline. which is
 one of the components in the pipeline.
>>>
>>> I was thinking of loopback with endpoints using compress streams,
>>> without physical endpoint, something like:
>>>
>>> compress playback (to feed data from userspace) -> DSP (processing) ->
>>> compress capture (send data back to userspace)
>>>
>>> Unless I'm missing something, you should be able to process data as fast
>>> as you can feed it and consume it in such case.
>>>
>>
>> Actually in the beginning I tried this,  but it did not work well.
>> ALSA needs time control for playback and capture, playback and capture
>> needs to synchronize.  Usually the playback and capture pipeline is
>> independent in ALSA design,  but in this case, the playback and capture
>> should synchronize, they are not independent.
> 
> The core compress API core no strict timing constraints. You can
> eventually0 have two half-duplex compress devices, if you like to have
> really independent mechanism. If something is missing in API, you can
> extend this API (like to inform the user space that it's a
> producer/consumer processing without any relation to the real time). I
> like this idea.

The compress API was never intended to be used this way. It was meant to
send compressed data to a DSP for rendering, and keep the host processor
in a low-power state while the DSP local buffer was drained. There was
no intent to do a loop back to the host, because that keeps the host in
a high-power state and probably negates the power savings due to a DSP.

The other problem with the loopback is that the compress stuff is
usually a "Front-End" in ASoC/DPCM parlance, and we don't have a good
way to do a loopback between Front-Ends. The entire framework is based
on FEs being connected to BEs.

One problem that I can see for ASRC is that it's not clear when the data
will be completely processed on the "capture" stream when you stop the
"playback" stream. There's a non-zero risk of having a truncated output
or waiting for data that will never be generated.

In other words, it might be possible to reuse/extend the compress API
for a 'coprocessor' approach without any rendering to traditional
interfaces, but it's uncharted territory.

[PATCH 4/5] ASoC: fsl: fsl-utils: remove useless assignment

[Pierre-Louis Bossart]

cppcheck warning:

sound/soc/fsl/fsl_utils.c:127:10: style: Variable 'ret' is assigned a
value that is never used. [unreadVariable]
 int ret = 0;
 ^

Signed-off-by: Pierre-Louis Bossart 
Reviewed-by: Bard Liao 
Reviewed-by: Kai Vehmanen 
Reviewed-by: Chao Song 
---
 sound/soc/fsl/fsl_utils.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/sound/soc/fsl/fsl_utils.c b/sound/soc/fsl/fsl_utils.c
index d0fc430f7033d..a5ab27c2f711c 100644
--- a/sound/soc/fsl/fsl_utils.c
+++ b/sound/soc/fsl/fsl_utils.c
@@ -124,7 +124,7 @@ void fsl_asoc_reparent_pll_clocks(struct device *dev, 
struct clk *clk,
 {
struct clk *p, *pll = NULL, *npll = NULL;
bool reparent = false;
-   int ret = 0;
+   int ret;
 
if (!clk || !pll8k_clk || !pll11k_clk)
return;
-- 
2.34.1

[PATCH 07/11] ASoC: fsl: fsl_sai: use pm_runtime_resume_and_get()

[Pierre-Louis Bossart]

Simplify the flow.

Signed-off-by: Pierre-Louis Bossart 
Reviewed-by: Bard Liao 
Reviewed-by: Kai Vehmanen 
Reviewed-by: Ranjani Sridharan 
---
 sound/soc/fsl/fsl_sai.c | 6 ++
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/sound/soc/fsl/fsl_sai.c b/sound/soc/fsl/fsl_sai.c
index 4f5bd9597c746..b6407d4d3e09d 100644
--- a/sound/soc/fsl/fsl_sai.c
+++ b/sound/soc/fsl/fsl_sai.c
@@ -1141,11 +1141,9 @@ static int fsl_sai_probe(struct platform_device *pdev)
goto err_pm_disable;
}
 
-   ret = pm_runtime_get_sync(dev);
-   if (ret < 0) {
-   pm_runtime_put_noidle(dev);
+   ret = pm_runtime_resume_and_get(dev);
+   if (ret < 0)
goto err_pm_get_sync;
-   }
 
/* Get sai version */
ret = fsl_sai_check_version(dev);
-- 
2.34.1

[RFC PATCH v3 10/13] ASoC: fsl: asrc_dma: protect for_each_dpcm_be() loops

[Pierre-Louis Bossart]

Follow the locking model used within soc-pcm.c

Signed-off-by: Pierre-Louis Bossart 
---
 sound/soc/fsl/fsl_asrc_dma.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/sound/soc/fsl/fsl_asrc_dma.c b/sound/soc/fsl/fsl_asrc_dma.c
index cd9b36ec0ecb..b67097503836 100644
--- a/sound/soc/fsl/fsl_asrc_dma.c
+++ b/sound/soc/fsl/fsl_asrc_dma.c
@@ -151,6 +151,7 @@ static int fsl_asrc_dma_hw_params(struct snd_soc_component 
*component,
int ret, width;
 
/* Fetch the Back-End dma_data from DPCM */
+   snd_soc_dpcm_fe_lock_irq(rtd, stream);
for_each_dpcm_be(rtd, stream, dpcm) {
struct snd_soc_pcm_runtime *be = dpcm->be;
struct snd_pcm_substream *substream_be;
@@ -164,6 +165,7 @@ static int fsl_asrc_dma_hw_params(struct snd_soc_component 
*component,
dev_be = dai->dev;
break;
}
+   snd_soc_dpcm_fe_unlock_irq(rtd, stream);
 
if (!dma_params_be) {
dev_err(dev, "failed to get the substream of Back-End\n");
-- 
2.25.1

[PATCH 8/9] ASoC: fsl: mpc8610: remove useless assignment

[Pierre-Louis Bossart]

cppcheck warning:

sound/soc/fsl/mpc8610_hpcd.c:333:6: style: Redundant initialization
for 'ret'. The initialized value is overwritten before it is
read. [redundantInitialization]
 ret = fsl_asoc_get_dma_channel(np, "fsl,playback-dma",
 ^
sound/soc/fsl/mpc8610_hpcd.c:193:10: note: ret is initialized
 int ret = -ENODEV;
 ^
sound/soc/fsl/mpc8610_hpcd.c:333:6: note: ret is overwritten
 ret = fsl_asoc_get_dma_channel(np, "fsl,playback-dma",
 ^

Signed-off-by: Pierre-Louis Bossart 
---
 sound/soc/fsl/mpc8610_hpcd.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/sound/soc/fsl/mpc8610_hpcd.c b/sound/soc/fsl/mpc8610_hpcd.c
index eccc833390d4..58b9ca3c4da0 100644
--- a/sound/soc/fsl/mpc8610_hpcd.c
+++ b/sound/soc/fsl/mpc8610_hpcd.c
@@ -190,7 +190,7 @@ static int mpc8610_hpcd_probe(struct platform_device *pdev)
struct device_node *codec_np = NULL;
struct mpc8610_hpcd_data *machine_data;
struct snd_soc_dai_link_component *comp;
-   int ret = -ENODEV;
+   int ret;
const char *sprop;
const u32 *iprop;
 
-- 
2.25.1

[PATCH 6/9] ASoC: fsl: imx-hdmi: remove unused structure members

[Pierre-Louis Bossart]

cppcheck warning:

sound/soc/fsl/imx-hdmi.c:21:16: style: struct member
'cpu_priv::sysclk_freq' is never used. [unusedStructMember]
 unsigned long sysclk_freq[2];
   ^

Additional checks show the sysclk_dir member is also not used.

Signed-off-by: Pierre-Louis Bossart 
---
 sound/soc/fsl/imx-hdmi.c | 4 
 1 file changed, 4 deletions(-)

diff --git a/sound/soc/fsl/imx-hdmi.c b/sound/soc/fsl/imx-hdmi.c
index dbbb7618351c..1ebcb9a2336b 100644
--- a/sound/soc/fsl/imx-hdmi.c
+++ b/sound/soc/fsl/imx-hdmi.c
@@ -10,16 +10,12 @@
 
 /**
  * struct cpu_priv - CPU private data
- * @sysclk_freq: SYSCLK rates for set_sysclk()
- * @sysclk_dir: SYSCLK directions for set_sysclk()
  * @sysclk_id: SYSCLK ids for set_sysclk()
  * @slot_width: Slot width of each frame
  *
  * Note: [1] for tx and [0] for rx
  */
 struct cpu_priv {
-   unsigned long sysclk_freq[2];
-   u32 sysclk_dir[2];
u32 sysclk_id[2];
u32 slot_width;
 };
-- 
2.25.1

[PATCH 5/9] ASoC: fsl: fsl_ssi: remove unnecessary tests

[Pierre-Louis Bossart]

cppcheck warnings:

sound/soc/fsl/fsl_ssi.c:767:34: style: Condition 'div2' is always
false [knownConditionTrueFalse]
 stccr = SSI_SxCCR_PM(pm + 1) | (div2 ? SSI_SxCCR_DIV2 : 0) |
 ^
sound/soc/fsl/fsl_ssi.c:722:9: note: Assignment 'div2=0', assigned value is 0
 div2 = 0;
^
sound/soc/fsl/fsl_ssi.c:767:34: note: Condition 'div2' is always false
 stccr = SSI_SxCCR_PM(pm + 1) | (div2 ? SSI_SxCCR_DIV2 : 0) |
 ^
sound/soc/fsl/fsl_ssi.c:768:4: style: Condition 'psr' is always false
[knownConditionTrueFalse]
  (psr ? SSI_SxCCR_PSR : 0);
   ^
sound/soc/fsl/fsl_ssi.c:721:8: note: Assignment 'psr=0', assigned
value is 0
 psr = 0;
   ^
sound/soc/fsl/fsl_ssi.c:768:4: note: Condition 'psr' is always false
  (psr ? SSI_SxCCR_PSR : 0);
   ^

Upon further analysis, the variables 'div2' and 'psr' are set to zero
and never modified. All the tests can be removed.

Signed-off-by: Pierre-Louis Bossart 
---
 sound/soc/fsl/fsl_ssi.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/sound/soc/fsl/fsl_ssi.c b/sound/soc/fsl/fsl_ssi.c
index 57811743c294..c57d0428c0a3 100644
--- a/sound/soc/fsl/fsl_ssi.c
+++ b/sound/soc/fsl/fsl_ssi.c
@@ -747,7 +747,7 @@ static int fsl_ssi_set_bclk(struct snd_pcm_substream 
*substream,
sub *= 10;
do_div(sub, freq);
 
-   if (sub < savesub && !(i == 0 && psr == 0 && div2 == 0)) {
+   if (sub < savesub && !(i == 0)) {
baudrate = tmprate;
savesub = sub;
pm = i;
@@ -764,8 +764,7 @@ static int fsl_ssi_set_bclk(struct snd_pcm_substream 
*substream,
return -EINVAL;
}
 
-   stccr = SSI_SxCCR_PM(pm + 1) | (div2 ? SSI_SxCCR_DIV2 : 0) |
-   (psr ? SSI_SxCCR_PSR : 0);
+   stccr = SSI_SxCCR_PM(pm + 1);
mask = SSI_SxCCR_PM_MASK | SSI_SxCCR_DIV2 | SSI_SxCCR_PSR;
 
/* STCCR is used for RX in synchronous mode */
-- 
2.25.1

[PATCH 4/9] ASoC: fsl: fsl_esai: clarify expression

[Pierre-Louis Bossart]

cppcheck warning:

sound/soc/fsl/fsl_esai.c:307:16: style: Clarify calculation precedence
for '%' and '?'. [clarifyCalculation]
clk_id % 2 ? "extal" : "fsys");
       ^

Signed-off-by: Pierre-Louis Bossart 
---
 sound/soc/fsl/fsl_esai.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/sound/soc/fsl/fsl_esai.c b/sound/soc/fsl/fsl_esai.c
index 08056fa0a0fa..41b154417b92 100644
--- a/sound/soc/fsl/fsl_esai.c
+++ b/sound/soc/fsl/fsl_esai.c
@@ -304,7 +304,7 @@ static int fsl_esai_set_dai_sysclk(struct snd_soc_dai *dai, 
int clk_id,
 
if (IS_ERR(clksrc)) {
dev_err(dai->dev, "no assigned %s clock\n",
-   clk_id % 2 ? "extal" : "fsys");
+   (clk_id % 2) ? "extal" : "fsys");
return PTR_ERR(clksrc);
}
clk_rate = clk_get_rate(clksrc);
-- 
2.25.1

[PATCH 3/9] ASoC: fsl: fsl_easrc: remove useless assignments

[Pierre-Louis Bossart]

cppcheck warnings:

sound/soc/fsl/fsl_easrc.c:751:53: style: Variable 'st2_mem_alloc' is
assigned a value that is never used. [unreadVariable]
 int st1_chanxexp, st1_mem_alloc = 0, st2_mem_alloc = 0;
^
sound/soc/fsl/fsl_easrc.c:1331:11: style: Variable 'size' is assigned
a value that is never used. [unreadVariable]
 int size = 0;
  ^

Signed-off-by: Pierre-Louis Bossart 
---
 sound/soc/fsl/fsl_easrc.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/sound/soc/fsl/fsl_easrc.c b/sound/soc/fsl/fsl_easrc.c
index 636a702f37a6..725a5d3aaa02 100644
--- a/sound/soc/fsl/fsl_easrc.c
+++ b/sound/soc/fsl/fsl_easrc.c
@@ -710,7 +710,7 @@ static int fsl_easrc_max_ch_for_slot(struct fsl_asrc_pair 
*ctx,
 struct fsl_easrc_slot *slot)
 {
struct fsl_easrc_ctx_priv *ctx_priv = ctx->private;
-   int st1_mem_alloc = 0, st2_mem_alloc = 0;
+   int st1_mem_alloc = 0, st2_mem_alloc;
int pf_mem_alloc = 0;
int max_channels = 8 - slot->num_channel;
int channels = 0;
@@ -748,7 +748,7 @@ static int fsl_easrc_config_one_slot(struct fsl_asrc_pair 
*ctx,
 {
struct fsl_asrc *easrc = ctx->asrc;
struct fsl_easrc_ctx_priv *ctx_priv = ctx->private;
-   int st1_chanxexp, st1_mem_alloc = 0, st2_mem_alloc = 0;
+   int st1_chanxexp, st1_mem_alloc = 0, st2_mem_alloc;
unsigned int reg0, reg1, reg2, reg3;
unsigned int addr;
 
@@ -1328,7 +1328,7 @@ static int fsl_easrc_stop_context(struct fsl_asrc_pair 
*ctx)
 {
struct fsl_asrc *easrc = ctx->asrc;
int val, i;
-   int size = 0;
+   int size;
int retry = 200;
 
regmap_read(easrc->regmap, REG_EASRC_CC(ctx->index), &val);
-- 
2.25.1

[PATCH 2/9] ASoC: fsl: fsl_dma: remove unused variable

[Pierre-Louis Bossart]

cppcheck warning:

sound/soc/fsl/fsl_dma.c:411:10: style: Variable 'channel' is assigned
a value that is never used. [unreadVariable]

 channel = substream->stream == SNDRV_PCM_STREAM_PLAYBACK ? 0 : 1;
 ^

Removing this line shows the variable isn't needed any longer so
remove declaration as well.

Signed-off-by: Pierre-Louis Bossart 
---
 sound/soc/fsl/fsl_dma.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/sound/soc/fsl/fsl_dma.c b/sound/soc/fsl/fsl_dma.c
index e0c39c5f4854..84bd8a5356eb 100644
--- a/sound/soc/fsl/fsl_dma.c
+++ b/sound/soc/fsl/fsl_dma.c
@@ -392,7 +392,6 @@ static int fsl_dma_open(struct snd_soc_component *component,
dma_addr_t ld_buf_phys;
u64 temp_link;  /* Pointer to next link descriptor */
u32 mr;
-   unsigned int channel;
int ret = 0;
unsigned int i;
 
@@ -408,8 +407,6 @@ static int fsl_dma_open(struct snd_soc_component *component,
return ret;
}
 
-   channel = substream->stream == SNDRV_PCM_STREAM_PLAYBACK ? 0 : 1;
-
if (dma->assigned) {
dev_err(dev, "dma channel already assigned\n");
return -EBUSY;
-- 
2.25.1

[PATCH 1/9] ASoC: fsl: fsl_asrc: remove useless assignment

[Pierre-Louis Bossart]

cppcheck warning:

sound/soc/fsl/fsl_asrc.c:613:8: style: Variable 'i' is assigned a
value that is never used. [unreadVariable]
 int i = 0, j = 0;
   ^

The same issue occurs for the 'j' variable.

Signed-off-by: Pierre-Louis Bossart 
---
 sound/soc/fsl/fsl_asrc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/sound/soc/fsl/fsl_asrc.c b/sound/soc/fsl/fsl_asrc.c
index c325c984d165..63d236ef5c4d 100644
--- a/sound/soc/fsl/fsl_asrc.c
+++ b/sound/soc/fsl/fsl_asrc.c
@@ -610,7 +610,7 @@ static void fsl_asrc_select_clk(struct fsl_asrc_priv 
*asrc_priv,
struct asrc_config *config = pair_priv->config;
int rate[2], select_clk[2]; /* Array size 2 means IN and OUT */
int clk_rate, clk_index;
-   int i = 0, j = 0;
+   int i, j;
 
rate[IN] = in_rate;
rate[OUT] = out_rate;
-- 
2.25.1

[PATCH v2 6/6] ASoC: fsl: fsl_esai: fix kernel-doc

[Pierre-Louis Bossart]

Fix W=1 warnings. Fix kernel-doc syntax and add missing parameters.

Acked-by: Nicolin Chen 
Signed-off-by: Pierre-Louis Bossart 
---
 sound/soc/fsl/fsl_esai.c | 32 ++--
 1 file changed, 18 insertions(+), 14 deletions(-)

diff --git a/sound/soc/fsl/fsl_esai.c b/sound/soc/fsl/fsl_esai.c
index cbcb70d6f8c8..b8fbd7ba94af 100644
--- a/sound/soc/fsl/fsl_esai.c
+++ b/sound/soc/fsl/fsl_esai.c
@@ -22,8 +22,7 @@
SNDRV_PCM_FMTBIT_S24_LE)
 
 /**
- * fsl_esai_soc_data: soc specific data
- *
+ * struct fsl_esai_soc_data - soc specific data
  * @imx: for imx platform
  * @reset_at_xrun: flags for enable reset operaton
  */
@@ -33,8 +32,7 @@ struct fsl_esai_soc_data {
 };
 
 /**
- * fsl_esai: ESAI private data
- *
+ * struct fsl_esai - ESAI private data
  * @dma_params_rx: DMA parameters for receive channel
  * @dma_params_tx: DMA parameters for transmit channel
  * @pdev: platform device pointer
@@ -49,6 +47,8 @@ struct fsl_esai_soc_data {
  * @fifo_depth: depth of tx/rx FIFO
  * @slot_width: width of each DAI slot
  * @slots: number of slots
+ * @tx_mask: slot mask for TX
+ * @rx_mask: slot mask for RX
  * @channels: channel num for tx or rx
  * @hck_rate: clock rate of desired HCKx clock
  * @sck_rate: clock rate of desired SCKx clock
@@ -157,13 +157,15 @@ static irqreturn_t esai_isr(int irq, void *devid)
 }
 
 /**
- * This function is used to calculate the divisors of psr, pm, fp and it is
- * supposed to be called in set_dai_sysclk() and set_bclk().
+ * fsl_esai_divisor_cal - This function is used to calculate the
+ * divisors of psr, pm, fp and it is supposed to be called in
+ * set_dai_sysclk() and set_bclk().
  *
+ * @dai: pointer to DAI
+ * @tx: current setting is for playback or capture
  * @ratio: desired overall ratio for the paticipating dividers
  * @usefp: for HCK setting, there is no need to set fp divider
  * @fp: bypass other dividers by setting fp directly if fp != 0
- * @tx: current setting is for playback or capture
  */
 static int fsl_esai_divisor_cal(struct snd_soc_dai *dai, bool tx, u32 ratio,
bool usefp, u32 fp)
@@ -250,13 +252,12 @@ static int fsl_esai_divisor_cal(struct snd_soc_dai *dai, 
bool tx, u32 ratio,
 }
 
 /**
- * This function mainly configures the clock frequency of MCLK (HCKT/HCKR)
- *
- * @Parameters:
- * clk_id: The clock source of HCKT/HCKR
+ * fsl_esai_set_dai_sysclk - configure the clock frequency of MCLK (HCKT/HCKR)
+ * @dai: pointer to DAI
+ * @clk_id: The clock source of HCKT/HCKR
  *   (Input from outside; output from inside, FSYS or EXTAL)
- * freq: The required clock rate of HCKT/HCKR
- * dir: The clock direction of HCKT/HCKR
+ * @freq: The required clock rate of HCKT/HCKR
+ * @dir: The clock direction of HCKT/HCKR
  *
  * Note: If the direction is input, we do not care about clk_id.
  */
@@ -358,7 +359,10 @@ static int fsl_esai_set_dai_sysclk(struct snd_soc_dai 
*dai, int clk_id,
 }
 
 /**
- * This function configures the related dividers according to the bclk rate
+ * fsl_esai_set_bclk - configure the related dividers according to the bclk 
rate
+ * @dai: pointer to DAI
+ * @tx: direction boolean
+ * @freq: bclk freq
  */
 static int fsl_esai_set_bclk(struct snd_soc_dai *dai, bool tx, u32 freq)
 {
-- 
2.25.1

[PATCH v2 5/6] ASoC: fsl: fsl_asrc: fix kernel-doc

[Pierre-Louis Bossart]

Fix W=1 warnings. fix kernel doc and describe arguments.

Acked-by: Nicolin Chen 
Signed-off-by: Pierre-Louis Bossart 
---
 sound/soc/fsl/fsl_asrc.c | 57 +++-
 1 file changed, 39 insertions(+), 18 deletions(-)

diff --git a/sound/soc/fsl/fsl_asrc.c b/sound/soc/fsl/fsl_asrc.c
index 462ce9f9ab48..02c81d2e34ad 100644
--- a/sound/soc/fsl/fsl_asrc.c
+++ b/sound/soc/fsl/fsl_asrc.c
@@ -37,7 +37,7 @@ static struct snd_pcm_hw_constraint_list 
fsl_asrc_rate_constraints = {
.list = supported_asrc_rate,
 };
 
-/**
+/*
  * The following tables map the relationship between asrc_inclk/asrc_outclk in
  * fsl_asrc.h and the registers of ASRCSR
  */
@@ -68,7 +68,7 @@ static unsigned char output_clk_map_imx53[ASRC_CLK_MAP_LEN] = 
{
0x7, 0x7, 0x7, 0x7, 0x7, 0x7, 0x7, 0x7, 0x7, 0x7, 0x7, 0x7, 0x7, 0x7, 
0x7, 0x7,
 };
 
-/**
+/*
  * i.MX8QM/i.MX8QXP uses the same map for input and output.
  * clk_map_imx8qm[0] is for i.MX8QM asrc0
  * clk_map_imx8qm[1] is for i.MX8QM asrc1
@@ -102,16 +102,17 @@ static unsigned char clk_map_imx8qxp[2][ASRC_CLK_MAP_LEN] 
= {
 };
 
 /**
- * Select the pre-processing and post-processing options
+ * fsl_asrc_sel_proc - Select the pre-processing and post-processing options
+ * @inrate: input sample rate
+ * @outrate: output sample rate
+ * @pre_proc: return value for pre-processing option
+ * @post_proc: return value for post-processing option
+ *
  * Make sure to exclude following unsupported cases before
  * calling this function:
  * 1) inrate > 8.125 * outrate
  * 2) inrate > 16.125 * outrate
  *
- * inrate: input sample rate
- * outrate: output sample rate
- * pre_proc: return value for pre-processing option
- * post_proc: return value for post-processing option
  */
 static void fsl_asrc_sel_proc(int inrate, int outrate,
 int *pre_proc, int *post_proc)
@@ -148,7 +149,9 @@ static void fsl_asrc_sel_proc(int inrate, int outrate,
 }
 
 /**
- * Request ASRC pair
+ * fsl_asrc_request_pair - Request ASRC pair
+ * @channels: number of channels
+ * @pair: pointer to pair
  *
  * It assigns pair by the order of A->C->B because allocation of pair B,
  * within range [ANCA, ANCA+ANCB-1], depends on the channels of pair A
@@ -193,7 +196,8 @@ static int fsl_asrc_request_pair(int channels, struct 
fsl_asrc_pair *pair)
 }
 
 /**
- * Release ASRC pair
+ * fsl_asrc_release_pair - Release ASRC pair
+ * @pair: pair to release
  *
  * It clears the resource from asrc and releases the occupied channels.
  */
@@ -217,7 +221,10 @@ static void fsl_asrc_release_pair(struct fsl_asrc_pair 
*pair)
 }
 
 /**
- * Configure input and output thresholds
+ * fsl_asrc_set_watermarks- configure input and output thresholds
+ * @pair: pointer to pair
+ * @in: input threshold
+ * @out: output threshold
  */
 static void fsl_asrc_set_watermarks(struct fsl_asrc_pair *pair, u32 in, u32 
out)
 {
@@ -234,7 +241,9 @@ static void fsl_asrc_set_watermarks(struct fsl_asrc_pair 
*pair, u32 in, u32 out)
 }
 
 /**
- * Calculate the total divisor between asrck clock rate and sample rate
+ * fsl_asrc_cal_asrck_divisor - Calculate the total divisor between asrck 
clock rate and sample rate
+ * @pair: pointer to pair
+ * @div: divider
  *
  * It follows the formula clk_rate = samplerate * (2 ^ prescaler) * divider
  */
@@ -250,7 +259,10 @@ static u32 fsl_asrc_cal_asrck_divisor(struct fsl_asrc_pair 
*pair, u32 div)
 }
 
 /**
- * Calculate and set the ratio for Ideal Ratio mode only
+ * fsl_asrc_set_ideal_ratio - Calculate and set the ratio for Ideal Ratio mode 
only
+ * @pair: pointer to pair
+ * @inrate: input rate
+ * @outrate: output rate
  *
  * The ratio is a 32-bit fixed point value with 26 fractional bits.
  */
@@ -293,7 +305,9 @@ static int fsl_asrc_set_ideal_ratio(struct fsl_asrc_pair 
*pair,
 }
 
 /**
- * Configure the assigned ASRC pair
+ * fsl_asrc_config_pair - Configure the assigned ASRC pair
+ * @pair: pointer to pair
+ * @use_ideal_rate: boolean configuration
  *
  * It configures those ASRC registers according to a configuration instance
  * of struct asrc_config which includes in/output sample rate, width, channel
@@ -508,7 +522,8 @@ static int fsl_asrc_config_pair(struct fsl_asrc_pair *pair, 
bool use_ideal_rate)
 }
 
 /**
- * Start the assigned ASRC pair
+ * fsl_asrc_start_pair - Start the assigned ASRC pair
+ * @pair: pointer to pair
  *
  * It enables the assigned pair and makes it stopped at the stall level.
  */
@@ -539,7 +554,8 @@ static void fsl_asrc_start_pair(struct fsl_asrc_pair *pair)
 }
 
 /**
- * Stop the assigned ASRC pair
+ * fsl_asrc_stop_pair - Stop the assigned ASRC pair
+ * @pair: pointer to pair
  */
 static void fsl_asrc_stop_pair(struct fsl_asrc_pair *pair)
 {
@@ -552,7 +568,9 @@ static void fsl_asrc_stop_pair(struct fsl_asrc_pair *pair)
 }
 
 /**
- * Get DMA channel according to the pair and direction.
+ * fsl_asrc_get_dma_channel- Get DMA channel according to the pair and 
direction.
+ * @pair: pointer to pair
+

[PATCH v2 4/6] ASoC: fsl: fsl_spdif: fix kernel-doc

[Pierre-Louis Bossart]

Fix W=1 warnings. kernel-doc syntax was not followed and missing parameter

Acked-by: Nicolin Chen 
Signed-off-by: Pierre-Louis Bossart 
---
 sound/soc/fsl/fsl_spdif.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/sound/soc/fsl/fsl_spdif.c b/sound/soc/fsl/fsl_spdif.c
index 5b2689ae63d4..9fb95c6ee7ba 100644
--- a/sound/soc/fsl/fsl_spdif.c
+++ b/sound/soc/fsl/fsl_spdif.c
@@ -81,8 +81,8 @@ struct spdif_mixer_control {
 };
 
 /**
- * fsl_spdif_priv: Freescale SPDIF private data
- *
+ * struct fsl_spdif_priv - Freescale SPDIF private data
+ * @soc: SPDIF soc data
  * @fsl_spdif_control: SPDIF control data
  * @cpu_dai_drv: cpu dai driver
  * @pdev: platform device pointer
@@ -100,6 +100,7 @@ struct spdif_mixer_control {
  * @spbaclk: SPBA clock (optional, depending on SoC design)
  * @dma_params_tx: DMA parameters for transmit channel
  * @dma_params_rx: DMA parameters for receive channel
+ * @regcache_srpc: regcache for SRPC
  */
 struct fsl_spdif_priv {
const struct fsl_spdif_soc_data *soc;
-- 
2.25.1

[PATCH v2 2/6] ASoC: fsl: fsl_ssi: fix kernel-doc

[Pierre-Louis Bossart]

Fix W=1 warnings. The kernel-doc support is partial, add more
descriptions and follow proper syntax

Acked-by: Nicolin Chen 
Signed-off-by: Pierre-Louis Bossart 
---
 sound/soc/fsl/fsl_ssi.c | 70 ++---
 1 file changed, 44 insertions(+), 26 deletions(-)

diff --git a/sound/soc/fsl/fsl_ssi.c b/sound/soc/fsl/fsl_ssi.c
index 1a2fa7f18142..7ec80b240563 100644
--- a/sound/soc/fsl/fsl_ssi.c
+++ b/sound/soc/fsl/fsl_ssi.c
@@ -203,12 +203,10 @@ struct fsl_ssi_soc_data {
 };
 
 /**
- * fsl_ssi: per-SSI private data
- *
+ * struct fsl_ssi - per-SSI private data
  * @regs: Pointer to the regmap registers
  * @irq: IRQ of this SSI
  * @cpu_dai_drv: CPU DAI driver for this device
- *
  * @dai_fmt: DAI configuration this device is currently used with
  * @streams: Mask of current active streams: BIT(TX) and BIT(RX)
  * @i2s_net: I2S and Network mode configurations of SCR register
@@ -221,38 +219,29 @@ struct fsl_ssi_soc_data {
  * @slot_width: Width of each DAI slot
  * @slots: Number of slots
  * @regvals: Specific RX/TX register settings
- *
  * @clk: Clock source to access register
  * @baudclk: Clock source to generate bit and frame-sync clocks
  * @baudclk_streams: Active streams that are using baudclk
- *
  * @regcache_sfcsr: Cache sfcsr register value during suspend and resume
  * @regcache_sacnt: Cache sacnt register value during suspend and resume
- *
  * @dma_params_tx: DMA transmit parameters
  * @dma_params_rx: DMA receive parameters
  * @ssi_phys: physical address of the SSI registers
- *
  * @fiq_params: FIQ stream filtering parameters
- *
  * @card_pdev: Platform_device pointer to register a sound card for PowerPC or
  * to register a CODEC platform device for AC97
  * @card_name: Platform_device name to register a sound card for PowerPC or
  * to register a CODEC platform device for AC97
  * @card_idx: The index of SSI to register a sound card for PowerPC or
  *to register a CODEC platform device for AC97
- *
  * @dbg_stats: Debugging statistics
- *
  * @soc: SoC specific data
  * @dev: Pointer to &pdev->dev
- *
  * @fifo_watermark: The FIFO watermark setting. Notifies DMA when there are
  *  @fifo_watermark or fewer words in TX fifo or
  *  @fifo_watermark or more empty words in RX fifo.
  * @dma_maxburst: Max number of words to transfer in one go. So far,
  *this is always the same as fifo_watermark.
- *
  * @ac97_reg_lock: Mutex lock to serialize AC97 register access operations
  */
 struct fsl_ssi {
@@ -374,7 +363,9 @@ static bool fsl_ssi_is_i2s_cbm_cfs(struct fsl_ssi *ssi)
 }
 
 /**
- * Interrupt handler to gather states
+ * fsl_ssi_irq - Interrupt handler to gather states
+ * @irq: irq number
+ * @dev_id: context
  */
 static irqreturn_t fsl_ssi_isr(int irq, void *dev_id)
 {
@@ -395,7 +386,10 @@ static irqreturn_t fsl_ssi_isr(int irq, void *dev_id)
 }
 
 /**
- * Set SCR, SIER, STCR and SRCR registers with cached values in regvals
+ * fsl_ssi_config_enable - Set SCR, SIER, STCR and SRCR registers with
+ * cached values in regvals
+ * @ssi: SSI context
+ * @tx: direction
  *
  * Notes:
  * 1) For offline_config SoCs, enable all necessary bits of both streams
@@ -474,7 +468,7 @@ static void fsl_ssi_config_enable(struct fsl_ssi *ssi, bool 
tx)
ssi->streams |= BIT(dir);
 }
 
-/**
+/*
  * Exclude bits that are used by the opposite stream
  *
  * When both streams are active, disabling some bits for the current stream
@@ -495,7 +489,10 @@ static void fsl_ssi_config_enable(struct fsl_ssi *ssi, 
bool tx)
((vals) & _ssi_xor_shared_bits(vals, avals, aactive))
 
 /**
- * Unset SCR, SIER, STCR and SRCR registers with cached values in regvals
+ * fsl_ssi_config_disable - Unset SCR, SIER, STCR and SRCR registers
+ * with cached values in regvals
+ * @ssi: SSI context
+ * @tx: direction
  *
  * Notes:
  * 1) For offline_config SoCs, to avoid online reconfigurations, disable all
@@ -577,7 +574,9 @@ static void fsl_ssi_tx_ac97_saccst_setup(struct fsl_ssi 
*ssi)
 }
 
 /**
- * Cache critical bits of SIER, SRCR, STCR and SCR to later set them safely
+ * fsl_ssi_setup_regvals - Cache critical bits of SIER, SRCR, STCR and
+ * SCR to later set them safely
+ * @ssi: SSI context
  */
 static void fsl_ssi_setup_regvals(struct fsl_ssi *ssi)
 {
@@ -661,9 +660,12 @@ static void fsl_ssi_shutdown(struct snd_pcm_substream 
*substream,
 }
 
 /**
- * Configure Digital Audio Interface bit clock
+ * fsl_ssi_set_bclk - Configure Digital Audio Interface bit clock
+ * @substream: ASoC substream
+ * @dai: pointer to DAI
+ * @hw_params: pointers to hw_params
  *
- * Note: This function can be only called when using SSI as DAI master
+ * Notes: This function can be only called when using SSI as DAI master
  *
  * Quick instruction for parameters:
  * freq: Output BCLK frequency = samplerate * slots * slot_width
@@ -782,7 +784,10 @@ static int fsl_ssi_set_bclk(struct snd_pcm_substre

[PATCH v2 3/6] ASoC: fsl: fsl-asoc-card: fix kernel-doc

[Pierre-Louis Bossart]

Fix W=1 warnings. Kernel-doc syntax was not properly used.

Acked-by: Nicolin Chen 
Signed-off-by: Pierre-Louis Bossart 
---
 sound/soc/fsl/fsl-asoc-card.c | 21 +
 1 file changed, 9 insertions(+), 12 deletions(-)

diff --git a/sound/soc/fsl/fsl-asoc-card.c b/sound/soc/fsl/fsl-asoc-card.c
index 57ea1b072326..faac6ce9a82c 100644
--- a/sound/soc/fsl/fsl-asoc-card.c
+++ b/sound/soc/fsl/fsl-asoc-card.c
@@ -33,8 +33,7 @@
 #define DAI_FMT_BASE (SND_SOC_DAIFMT_I2S | SND_SOC_DAIFMT_NB_NF)
 
 /**
- * CODEC private data
- *
+ * struct codec_priv - CODEC private data
  * @mclk_freq: Clock rate of MCLK
  * @mclk_id: MCLK (or main clock) id for set_sysclk()
  * @fll_id: FLL (or secordary clock) id for set_sysclk()
@@ -48,11 +47,10 @@ struct codec_priv {
 };
 
 /**
- * CPU private data
- *
- * @sysclk_freq[2]: SYSCLK rates for set_sysclk()
- * @sysclk_dir[2]: SYSCLK directions for set_sysclk()
- * @sysclk_id[2]: SYSCLK ids for set_sysclk()
+ * struct cpu_priv - CPU private data
+ * @sysclk_freq: SYSCLK rates for set_sysclk()
+ * @sysclk_dir: SYSCLK directions for set_sysclk()
+ * @sysclk_id: SYSCLK ids for set_sysclk()
  * @slot_width: Slot width of each frame
  *
  * Note: [1] for tx and [0] for rx
@@ -65,9 +63,8 @@ struct cpu_priv {
 };
 
 /**
- * Freescale Generic ASOC card private data
- *
- * @dai_link[3]: DAI link structure including normal one and DPCM link
+ * struct fsl_asoc_card_priv - Freescale Generic ASOC card private data
+ * @dai_link: DAI link structure including normal one and DPCM link
  * @pdev: platform device pointer
  * @codec_priv: CODEC private data
  * @cpu_priv: CPU private data
@@ -94,8 +91,8 @@ struct fsl_asoc_card_priv {
char name[32];
 };
 
-/**
- * This dapm route map exsits for DPCM link only.
+/*
+ * This dapm route map exits for DPCM link only.
  * The other routes shall go through Device Tree.
  *
  * Note: keep all ASRC routes in the second half
-- 
2.25.1

[PATCH v2 1/6] ASoC: fsl: fsl_ssi_dbg: remove spurious kernel-doc comment start

[Pierre-Louis Bossart]

Fix W=1 warnings. There is no kernel-doc here.

Acked-by: Nicolin Chen 
Signed-off-by: Pierre-Louis Bossart 
---
 sound/soc/fsl/fsl_ssi_dbg.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/sound/soc/fsl/fsl_ssi_dbg.c b/sound/soc/fsl/fsl_ssi_dbg.c
index 2a20ee23dc52..2c46c55f0a88 100644
--- a/sound/soc/fsl/fsl_ssi_dbg.c
+++ b/sound/soc/fsl/fsl_ssi_dbg.c
@@ -78,7 +78,7 @@ void fsl_ssi_dbg_isr(struct fsl_ssi_dbg *dbg, u32 sisr)
dbg->stats.tfe0++;
 }
 
-/**
+/*
  * Show the statistics of a flag only if its interrupt is enabled
  *
  * Compilers will optimize it to a no-op if the interrupt is disabled
@@ -90,7 +90,7 @@ void fsl_ssi_dbg_isr(struct fsl_ssi_dbg *dbg, u32 sisr)
} while (0)
 
 
-/**
+/*
  * Display the statistics for the current SSI device
  *
  * To avoid confusion, only show those counts that are enabled
-- 
2.25.1

Re: [PATCH 6/6] ASoC: fsl: fsl_esai: fix kernel-doc

[Pierre-Louis Bossart]

On 7/2/20 1:55 PM, Nicolin Chen wrote:

On Thu, Jul 02, 2020 at 12:22:27PM -0500, Pierre-Louis Bossart wrote:

Fix W=1 warnings. Fix kernel-doc syntax and add missing parameters.

Signed-off-by: Pierre-Louis Bossart 



+ * fsl_esai_set_dai_sysclk - This function mainly configures the clock 
frequency of MCLK (HCKT/HCKR)


Can drop "This function mainly"


  /**
- * This function configures the related dividers according to the bclk rate
+ * fsl_esai_set_bclk - This function configures the related dividers according 
to the bclk rate


Here too -- dropping "This function"


will fix both, thanks for reviewing the edits.



Otherwise,
Acked-by: Nicolin Chen 

Thanks!

Re: [PATCH 3/6] ASoC: fsl: fsl-asoc-card: fix kernel-doc

[Pierre-Louis Bossart]

On 7/2/20 1:47 PM, Nicolin Chen wrote:

On Thu, Jul 02, 2020 at 12:22:24PM -0500, Pierre-Louis Bossart wrote:

Fix W=1 warnings. Kernel-doc syntax was not properly used.

Signed-off-by: Pierre-Louis Bossart 
---
  sound/soc/fsl/fsl-asoc-card.c | 21 +
  1 file changed, 9 insertions(+), 12 deletions(-)

diff --git a/sound/soc/fsl/fsl-asoc-card.c b/sound/soc/fsl/fsl-asoc-card.c



  /**
- * Freescale Generic ASOC card private data
+ * struct fsl_asoc_card_priv - struct Freescale Generic ASOC card private data


Just a nit, can you drop the "struct" before "Freescale"?
Other parts of your changes don't really add this word.


will do, thanks for spotting this.


Otherwise,
Acked-by: Nicolin Chen 

[PATCH 6/6] ASoC: fsl: fsl_esai: fix kernel-doc

[Pierre-Louis Bossart]

Fix W=1 warnings. Fix kernel-doc syntax and add missing parameters.

Signed-off-by: Pierre-Louis Bossart 
---
 sound/soc/fsl/fsl_esai.c | 32 ++--
 1 file changed, 18 insertions(+), 14 deletions(-)

diff --git a/sound/soc/fsl/fsl_esai.c b/sound/soc/fsl/fsl_esai.c
index cbcb70d6f8c8..a1db69061b4b 100644
--- a/sound/soc/fsl/fsl_esai.c
+++ b/sound/soc/fsl/fsl_esai.c
@@ -22,8 +22,7 @@
SNDRV_PCM_FMTBIT_S24_LE)
 
 /**
- * fsl_esai_soc_data: soc specific data
- *
+ * struct fsl_esai_soc_data - soc specific data
  * @imx: for imx platform
  * @reset_at_xrun: flags for enable reset operaton
  */
@@ -33,8 +32,7 @@ struct fsl_esai_soc_data {
 };
 
 /**
- * fsl_esai: ESAI private data
- *
+ * struct fsl_esai - ESAI private data
  * @dma_params_rx: DMA parameters for receive channel
  * @dma_params_tx: DMA parameters for transmit channel
  * @pdev: platform device pointer
@@ -49,6 +47,8 @@ struct fsl_esai_soc_data {
  * @fifo_depth: depth of tx/rx FIFO
  * @slot_width: width of each DAI slot
  * @slots: number of slots
+ * @tx_mask: slot mask for TX
+ * @rx_mask: slot mask for RX
  * @channels: channel num for tx or rx
  * @hck_rate: clock rate of desired HCKx clock
  * @sck_rate: clock rate of desired SCKx clock
@@ -157,13 +157,15 @@ static irqreturn_t esai_isr(int irq, void *devid)
 }
 
 /**
- * This function is used to calculate the divisors of psr, pm, fp and it is
- * supposed to be called in set_dai_sysclk() and set_bclk().
+ * fsl_esai_divisor_cal - This function is used to calculate the
+ * divisors of psr, pm, fp and it is supposed to be called in
+ * set_dai_sysclk() and set_bclk().
  *
+ * @dai: pointer to DAI
+ * @tx: current setting is for playback or capture
  * @ratio: desired overall ratio for the paticipating dividers
  * @usefp: for HCK setting, there is no need to set fp divider
  * @fp: bypass other dividers by setting fp directly if fp != 0
- * @tx: current setting is for playback or capture
  */
 static int fsl_esai_divisor_cal(struct snd_soc_dai *dai, bool tx, u32 ratio,
bool usefp, u32 fp)
@@ -250,13 +252,12 @@ static int fsl_esai_divisor_cal(struct snd_soc_dai *dai, 
bool tx, u32 ratio,
 }
 
 /**
- * This function mainly configures the clock frequency of MCLK (HCKT/HCKR)
- *
- * @Parameters:
- * clk_id: The clock source of HCKT/HCKR
+ * fsl_esai_set_dai_sysclk - This function mainly configures the clock 
frequency of MCLK (HCKT/HCKR)
+ * @dai: pointer to DAI
+ * @clk_id: The clock source of HCKT/HCKR
  *   (Input from outside; output from inside, FSYS or EXTAL)
- * freq: The required clock rate of HCKT/HCKR
- * dir: The clock direction of HCKT/HCKR
+ * @freq: The required clock rate of HCKT/HCKR
+ * @dir: The clock direction of HCKT/HCKR
  *
  * Note: If the direction is input, we do not care about clk_id.
  */
@@ -358,7 +359,10 @@ static int fsl_esai_set_dai_sysclk(struct snd_soc_dai 
*dai, int clk_id,
 }
 
 /**
- * This function configures the related dividers according to the bclk rate
+ * fsl_esai_set_bclk - This function configures the related dividers according 
to the bclk rate
+ * @dai: pointer to DAI
+ * @tx: direction boolean
+ * @freq: bclk freq
  */
 static int fsl_esai_set_bclk(struct snd_soc_dai *dai, bool tx, u32 freq)
 {
-- 
2.25.1

[PATCH 4/6] ASoC: fsl: fsl_spdif: fix kernel-doc

[Pierre-Louis Bossart]

Fix W=1 warnings. kernel-doc syntax was not followed and missing parameter

Signed-off-by: Pierre-Louis Bossart 
---
 sound/soc/fsl/fsl_spdif.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/sound/soc/fsl/fsl_spdif.c b/sound/soc/fsl/fsl_spdif.c
index 5b2689ae63d4..9fb95c6ee7ba 100644
--- a/sound/soc/fsl/fsl_spdif.c
+++ b/sound/soc/fsl/fsl_spdif.c
@@ -81,8 +81,8 @@ struct spdif_mixer_control {
 };
 
 /**
- * fsl_spdif_priv: Freescale SPDIF private data
- *
+ * struct fsl_spdif_priv - Freescale SPDIF private data
+ * @soc: SPDIF soc data
  * @fsl_spdif_control: SPDIF control data
  * @cpu_dai_drv: cpu dai driver
  * @pdev: platform device pointer
@@ -100,6 +100,7 @@ struct spdif_mixer_control {
  * @spbaclk: SPBA clock (optional, depending on SoC design)
  * @dma_params_tx: DMA parameters for transmit channel
  * @dma_params_rx: DMA parameters for receive channel
+ * @regcache_srpc: regcache for SRPC
  */
 struct fsl_spdif_priv {
const struct fsl_spdif_soc_data *soc;
-- 
2.25.1

[PATCH 5/6] ASoC: fsl: fsl_asrc: fix kernel-doc

[Pierre-Louis Bossart]

Fix W=1 warnings. fix kernel doc and describe arguments.

Signed-off-by: Pierre-Louis Bossart 
---
 sound/soc/fsl/fsl_asrc.c | 57 +++-
 1 file changed, 39 insertions(+), 18 deletions(-)

diff --git a/sound/soc/fsl/fsl_asrc.c b/sound/soc/fsl/fsl_asrc.c
index 462ce9f9ab48..02c81d2e34ad 100644
--- a/sound/soc/fsl/fsl_asrc.c
+++ b/sound/soc/fsl/fsl_asrc.c
@@ -37,7 +37,7 @@ static struct snd_pcm_hw_constraint_list 
fsl_asrc_rate_constraints = {
.list = supported_asrc_rate,
 };
 
-/**
+/*
  * The following tables map the relationship between asrc_inclk/asrc_outclk in
  * fsl_asrc.h and the registers of ASRCSR
  */
@@ -68,7 +68,7 @@ static unsigned char output_clk_map_imx53[ASRC_CLK_MAP_LEN] = 
{
0x7, 0x7, 0x7, 0x7, 0x7, 0x7, 0x7, 0x7, 0x7, 0x7, 0x7, 0x7, 0x7, 0x7, 
0x7, 0x7,
 };
 
-/**
+/*
  * i.MX8QM/i.MX8QXP uses the same map for input and output.
  * clk_map_imx8qm[0] is for i.MX8QM asrc0
  * clk_map_imx8qm[1] is for i.MX8QM asrc1
@@ -102,16 +102,17 @@ static unsigned char clk_map_imx8qxp[2][ASRC_CLK_MAP_LEN] 
= {
 };
 
 /**
- * Select the pre-processing and post-processing options
+ * fsl_asrc_sel_proc - Select the pre-processing and post-processing options
+ * @inrate: input sample rate
+ * @outrate: output sample rate
+ * @pre_proc: return value for pre-processing option
+ * @post_proc: return value for post-processing option
+ *
  * Make sure to exclude following unsupported cases before
  * calling this function:
  * 1) inrate > 8.125 * outrate
  * 2) inrate > 16.125 * outrate
  *
- * inrate: input sample rate
- * outrate: output sample rate
- * pre_proc: return value for pre-processing option
- * post_proc: return value for post-processing option
  */
 static void fsl_asrc_sel_proc(int inrate, int outrate,
 int *pre_proc, int *post_proc)
@@ -148,7 +149,9 @@ static void fsl_asrc_sel_proc(int inrate, int outrate,
 }
 
 /**
- * Request ASRC pair
+ * fsl_asrc_request_pair - Request ASRC pair
+ * @channels: number of channels
+ * @pair: pointer to pair
  *
  * It assigns pair by the order of A->C->B because allocation of pair B,
  * within range [ANCA, ANCA+ANCB-1], depends on the channels of pair A
@@ -193,7 +196,8 @@ static int fsl_asrc_request_pair(int channels, struct 
fsl_asrc_pair *pair)
 }
 
 /**
- * Release ASRC pair
+ * fsl_asrc_release_pair - Release ASRC pair
+ * @pair: pair to release
  *
  * It clears the resource from asrc and releases the occupied channels.
  */
@@ -217,7 +221,10 @@ static void fsl_asrc_release_pair(struct fsl_asrc_pair 
*pair)
 }
 
 /**
- * Configure input and output thresholds
+ * fsl_asrc_set_watermarks- configure input and output thresholds
+ * @pair: pointer to pair
+ * @in: input threshold
+ * @out: output threshold
  */
 static void fsl_asrc_set_watermarks(struct fsl_asrc_pair *pair, u32 in, u32 
out)
 {
@@ -234,7 +241,9 @@ static void fsl_asrc_set_watermarks(struct fsl_asrc_pair 
*pair, u32 in, u32 out)
 }
 
 /**
- * Calculate the total divisor between asrck clock rate and sample rate
+ * fsl_asrc_cal_asrck_divisor - Calculate the total divisor between asrck 
clock rate and sample rate
+ * @pair: pointer to pair
+ * @div: divider
  *
  * It follows the formula clk_rate = samplerate * (2 ^ prescaler) * divider
  */
@@ -250,7 +259,10 @@ static u32 fsl_asrc_cal_asrck_divisor(struct fsl_asrc_pair 
*pair, u32 div)
 }
 
 /**
- * Calculate and set the ratio for Ideal Ratio mode only
+ * fsl_asrc_set_ideal_ratio - Calculate and set the ratio for Ideal Ratio mode 
only
+ * @pair: pointer to pair
+ * @inrate: input rate
+ * @outrate: output rate
  *
  * The ratio is a 32-bit fixed point value with 26 fractional bits.
  */
@@ -293,7 +305,9 @@ static int fsl_asrc_set_ideal_ratio(struct fsl_asrc_pair 
*pair,
 }
 
 /**
- * Configure the assigned ASRC pair
+ * fsl_asrc_config_pair - Configure the assigned ASRC pair
+ * @pair: pointer to pair
+ * @use_ideal_rate: boolean configuration
  *
  * It configures those ASRC registers according to a configuration instance
  * of struct asrc_config which includes in/output sample rate, width, channel
@@ -508,7 +522,8 @@ static int fsl_asrc_config_pair(struct fsl_asrc_pair *pair, 
bool use_ideal_rate)
 }
 
 /**
- * Start the assigned ASRC pair
+ * fsl_asrc_start_pair - Start the assigned ASRC pair
+ * @pair: pointer to pair
  *
  * It enables the assigned pair and makes it stopped at the stall level.
  */
@@ -539,7 +554,8 @@ static void fsl_asrc_start_pair(struct fsl_asrc_pair *pair)
 }
 
 /**
- * Stop the assigned ASRC pair
+ * fsl_asrc_stop_pair - Stop the assigned ASRC pair
+ * @pair: pointer to pair
  */
 static void fsl_asrc_stop_pair(struct fsl_asrc_pair *pair)
 {
@@ -552,7 +568,9 @@ static void fsl_asrc_stop_pair(struct fsl_asrc_pair *pair)
 }
 
 /**
- * Get DMA channel according to the pair and direction.
+ * fsl_asrc_get_dma_channel- Get DMA channel according to the pair and 
direction.
+ * @pair: pointer to pair
+ * @dir: DMA direction
  */
 s

[PATCH 3/6] ASoC: fsl: fsl-asoc-card: fix kernel-doc

[Pierre-Louis Bossart]

Fix W=1 warnings. Kernel-doc syntax was not properly used.

Signed-off-by: Pierre-Louis Bossart 
---
 sound/soc/fsl/fsl-asoc-card.c | 21 +
 1 file changed, 9 insertions(+), 12 deletions(-)

diff --git a/sound/soc/fsl/fsl-asoc-card.c b/sound/soc/fsl/fsl-asoc-card.c
index 57ea1b072326..91220b96e043 100644
--- a/sound/soc/fsl/fsl-asoc-card.c
+++ b/sound/soc/fsl/fsl-asoc-card.c
@@ -33,8 +33,7 @@
 #define DAI_FMT_BASE (SND_SOC_DAIFMT_I2S | SND_SOC_DAIFMT_NB_NF)
 
 /**
- * CODEC private data
- *
+ * struct codec_priv - CODEC private data
  * @mclk_freq: Clock rate of MCLK
  * @mclk_id: MCLK (or main clock) id for set_sysclk()
  * @fll_id: FLL (or secordary clock) id for set_sysclk()
@@ -48,11 +47,10 @@ struct codec_priv {
 };
 
 /**
- * CPU private data
- *
- * @sysclk_freq[2]: SYSCLK rates for set_sysclk()
- * @sysclk_dir[2]: SYSCLK directions for set_sysclk()
- * @sysclk_id[2]: SYSCLK ids for set_sysclk()
+ * struct cpu_priv - CPU private data
+ * @sysclk_freq: SYSCLK rates for set_sysclk()
+ * @sysclk_dir: SYSCLK directions for set_sysclk()
+ * @sysclk_id: SYSCLK ids for set_sysclk()
  * @slot_width: Slot width of each frame
  *
  * Note: [1] for tx and [0] for rx
@@ -65,9 +63,8 @@ struct cpu_priv {
 };
 
 /**
- * Freescale Generic ASOC card private data
- *
- * @dai_link[3]: DAI link structure including normal one and DPCM link
+ * struct fsl_asoc_card_priv - struct Freescale Generic ASOC card private data
+ * @dai_link: DAI link structure including normal one and DPCM link
  * @pdev: platform device pointer
  * @codec_priv: CODEC private data
  * @cpu_priv: CPU private data
@@ -94,8 +91,8 @@ struct fsl_asoc_card_priv {
char name[32];
 };
 
-/**
- * This dapm route map exsits for DPCM link only.
+/*
+ * This dapm route map exits for DPCM link only.
  * The other routes shall go through Device Tree.
  *
  * Note: keep all ASRC routes in the second half
-- 
2.25.1

[PATCH 1/6] ASoC: fsl: fsl_ssi_dbg: remove spurious kernel-doc comment start

[Pierre-Louis Bossart]

Fix W=1 warnings. There is no kernel-doc here.

Signed-off-by: Pierre-Louis Bossart 
---
 sound/soc/fsl/fsl_ssi_dbg.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/sound/soc/fsl/fsl_ssi_dbg.c b/sound/soc/fsl/fsl_ssi_dbg.c
index 2a20ee23dc52..2c46c55f0a88 100644
--- a/sound/soc/fsl/fsl_ssi_dbg.c
+++ b/sound/soc/fsl/fsl_ssi_dbg.c
@@ -78,7 +78,7 @@ void fsl_ssi_dbg_isr(struct fsl_ssi_dbg *dbg, u32 sisr)
dbg->stats.tfe0++;
 }
 
-/**
+/*
  * Show the statistics of a flag only if its interrupt is enabled
  *
  * Compilers will optimize it to a no-op if the interrupt is disabled
@@ -90,7 +90,7 @@ void fsl_ssi_dbg_isr(struct fsl_ssi_dbg *dbg, u32 sisr)
} while (0)
 
 
-/**
+/*
  * Display the statistics for the current SSI device
  *
  * To avoid confusion, only show those counts that are enabled
-- 
2.25.1

[PATCH 2/6] ASoC: fsl: fsl_ssi: fix kernel-doc

[Pierre-Louis Bossart]

Fix W=1 warnings. The kernel-doc support is partial, add more
descriptions and follow proper syntax

Signed-off-by: Pierre-Louis Bossart 
---
 sound/soc/fsl/fsl_ssi.c | 70 ++---
 1 file changed, 44 insertions(+), 26 deletions(-)

diff --git a/sound/soc/fsl/fsl_ssi.c b/sound/soc/fsl/fsl_ssi.c
index 1a2fa7f18142..7ec80b240563 100644
--- a/sound/soc/fsl/fsl_ssi.c
+++ b/sound/soc/fsl/fsl_ssi.c
@@ -203,12 +203,10 @@ struct fsl_ssi_soc_data {
 };
 
 /**
- * fsl_ssi: per-SSI private data
- *
+ * struct fsl_ssi - per-SSI private data
  * @regs: Pointer to the regmap registers
  * @irq: IRQ of this SSI
  * @cpu_dai_drv: CPU DAI driver for this device
- *
  * @dai_fmt: DAI configuration this device is currently used with
  * @streams: Mask of current active streams: BIT(TX) and BIT(RX)
  * @i2s_net: I2S and Network mode configurations of SCR register
@@ -221,38 +219,29 @@ struct fsl_ssi_soc_data {
  * @slot_width: Width of each DAI slot
  * @slots: Number of slots
  * @regvals: Specific RX/TX register settings
- *
  * @clk: Clock source to access register
  * @baudclk: Clock source to generate bit and frame-sync clocks
  * @baudclk_streams: Active streams that are using baudclk
- *
  * @regcache_sfcsr: Cache sfcsr register value during suspend and resume
  * @regcache_sacnt: Cache sacnt register value during suspend and resume
- *
  * @dma_params_tx: DMA transmit parameters
  * @dma_params_rx: DMA receive parameters
  * @ssi_phys: physical address of the SSI registers
- *
  * @fiq_params: FIQ stream filtering parameters
- *
  * @card_pdev: Platform_device pointer to register a sound card for PowerPC or
  * to register a CODEC platform device for AC97
  * @card_name: Platform_device name to register a sound card for PowerPC or
  * to register a CODEC platform device for AC97
  * @card_idx: The index of SSI to register a sound card for PowerPC or
  *to register a CODEC platform device for AC97
- *
  * @dbg_stats: Debugging statistics
- *
  * @soc: SoC specific data
  * @dev: Pointer to &pdev->dev
- *
  * @fifo_watermark: The FIFO watermark setting. Notifies DMA when there are
  *  @fifo_watermark or fewer words in TX fifo or
  *  @fifo_watermark or more empty words in RX fifo.
  * @dma_maxburst: Max number of words to transfer in one go. So far,
  *this is always the same as fifo_watermark.
- *
  * @ac97_reg_lock: Mutex lock to serialize AC97 register access operations
  */
 struct fsl_ssi {
@@ -374,7 +363,9 @@ static bool fsl_ssi_is_i2s_cbm_cfs(struct fsl_ssi *ssi)
 }
 
 /**
- * Interrupt handler to gather states
+ * fsl_ssi_irq - Interrupt handler to gather states
+ * @irq: irq number
+ * @dev_id: context
  */
 static irqreturn_t fsl_ssi_isr(int irq, void *dev_id)
 {
@@ -395,7 +386,10 @@ static irqreturn_t fsl_ssi_isr(int irq, void *dev_id)
 }
 
 /**
- * Set SCR, SIER, STCR and SRCR registers with cached values in regvals
+ * fsl_ssi_config_enable - Set SCR, SIER, STCR and SRCR registers with
+ * cached values in regvals
+ * @ssi: SSI context
+ * @tx: direction
  *
  * Notes:
  * 1) For offline_config SoCs, enable all necessary bits of both streams
@@ -474,7 +468,7 @@ static void fsl_ssi_config_enable(struct fsl_ssi *ssi, bool 
tx)
ssi->streams |= BIT(dir);
 }
 
-/**
+/*
  * Exclude bits that are used by the opposite stream
  *
  * When both streams are active, disabling some bits for the current stream
@@ -495,7 +489,10 @@ static void fsl_ssi_config_enable(struct fsl_ssi *ssi, 
bool tx)
((vals) & _ssi_xor_shared_bits(vals, avals, aactive))
 
 /**
- * Unset SCR, SIER, STCR and SRCR registers with cached values in regvals
+ * fsl_ssi_config_disable - Unset SCR, SIER, STCR and SRCR registers
+ * with cached values in regvals
+ * @ssi: SSI context
+ * @tx: direction
  *
  * Notes:
  * 1) For offline_config SoCs, to avoid online reconfigurations, disable all
@@ -577,7 +574,9 @@ static void fsl_ssi_tx_ac97_saccst_setup(struct fsl_ssi 
*ssi)
 }
 
 /**
- * Cache critical bits of SIER, SRCR, STCR and SCR to later set them safely
+ * fsl_ssi_setup_regvals - Cache critical bits of SIER, SRCR, STCR and
+ * SCR to later set them safely
+ * @ssi: SSI context
  */
 static void fsl_ssi_setup_regvals(struct fsl_ssi *ssi)
 {
@@ -661,9 +660,12 @@ static void fsl_ssi_shutdown(struct snd_pcm_substream 
*substream,
 }
 
 /**
- * Configure Digital Audio Interface bit clock
+ * fsl_ssi_set_bclk - Configure Digital Audio Interface bit clock
+ * @substream: ASoC substream
+ * @dai: pointer to DAI
+ * @hw_params: pointers to hw_params
  *
- * Note: This function can be only called when using SSI as DAI master
+ * Notes: This function can be only called when using SSI as DAI master
  *
  * Quick instruction for parameters:
  * freq: Output BCLK frequency = samplerate * slots * slot_width
@@ -782,7 +784,10 @@ static int fsl_ssi_set_bclk(struct snd_pcm_substream 
*substream,
 }
 
 /**
- * Confi

Re: [alsa-devel] Build regressions/improvements in v4.20-rc1 (sound/pci/hda/patch_ca0132.c)

[Pierre-Louis Bossart]

*** ERRORS ***

   + /kisskb/src/sound/pci/hda/patch_ca0132.c: error: implicit declaration of 
function 'pci_iomap' [-Werror=implicit-function-declaration]:  => 8799:3

sh4-all{mod,yes}config

Looks like d9b84a15892c0233 ("ALSA: hda: Fix implicit definition of
pci_iomap() on SH")
is not sufficient?

Different problem.  This is about "select":

config SND_SOC_ALL_CODECS
 tristate "Build all ASoC CODEC drivers"

That enables (sets):
 select SND_SOC_HDAC_HDA
which selects SND_HDA even though CONFIG_PCI is not enabled.

After SND_HDA is selected (above), the Kconfig symbols in
sound/pci/hda/Kconfig are available for enabling, so
SND_HDA_CODEC_CA0132 is enabled but will not build.

Thanks for looking into this!


One simple solution (but possibly too naive) is:

---
  sound/soc/codecs/Kconfig |2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

--- lnx-420-rc1.orig/sound/soc/codecs/Kconfig
+++ lnx-420-rc1/sound/soc/codecs/Kconfig
@@ -82,7 +82,7 @@ config SND_SOC_ALL_CODECS
 select SND_SOC_ES7241
 select SND_SOC_GTM601
 select SND_SOC_HDAC_HDMI
-   select SND_SOC_HDAC_HDA
+   select SND_SOC_HDAC_HDA if PCI
 select SND_SOC_ICS43432
 select SND_SOC_INNO_RK3036
 select SND_SOC_ISABELLE if I2C

I guess that will work. There are already plenty of "select foo if bar" lines.
However, looking at what else can enable SND_HDA, I think it should be

 select SND_SOC_HDAC_HDA if SND_PCI || ARCH_TEGRA


This codec can only be used by the Skylake driver (and the upcoming SOF 
one). For Tegra this module will never be used unless they follow the 
same path of enabling ASoC to deal with the HDaudio codecs instead of 
the legacy.


Likewise HDAC_HDMI will only work on Intel platforms for now.



That still leaves the issue that pci_iomap() on SH should be an empty stub if
PCI is not available, like on other architectures.


I thought Mark Brown provided a fix to SH maintainers?