Re: [PATCH] Alsa modularisations and support for tsc2101 1/7

Daniel Petrini
2006-02-21T12:14:14Z

Komal,

Thanks for the comments,

On 2/20/06, Komal Shah <komal_shah802003@yahoo.com> wrote:
> Daniel,
>
> > @@ -0,0 +1,714 @@
> > +/*
> > + * sound/arm/omap-alsa.c
> > + *
> > + * Alsa Driver for AIC23 codec on OSK5912 platform board
>
> Now *not* just for AIC23. Time to update this line.

Sure

> > + *
> > + * History:
> > + *
> > + * 2005-07-29 INdT Kernel Team - Alsa driver for omap osk.
> > Creation of new
> > + * file omap-aic23.c
> > + *
> > + * 2005-12-18 Dirk Behme - Added L/R Channel Interchange fix
> > as proposed by Ajaya Babu
> > + *
> > + * 2006-02-07 Daniel Petrini - Codec specific function separated.
> > File renamed to
> > + * omap-alsa.c
> > + * 2006-02-10 Mika Laitio - Codec specific function pointers are set
> > in the codec.
> > + * Default samplerate queried from the codec.
> > + */
>
> I am happy to see this big development and cleaned up driver interface,
> but I think we can trim down this History as I believe GIT is being
> really used for this purpose. Only thing we need to take care is give
> better comment at patch submit time :)

Very good suggestion. We will keep in mind to do it from now on.

> > +MODULE_ALIAS("omap_alsa_mcbsp.1");
>
> Do we really need to add "_mcbsp" in that string? It looks confusing
> while reading.

Matter of taste? Isn't better to have all this information?

> > +#define DMA_BUF_SIZE 1024 * 8
> > +
>
> Brackets around 1024 * 8 would be better.

ok.

> > +static int audio_dma_request(struct audio_stream *s,
> > + void (*callback) (void *))
> > +{
> > + int err;
> > + ADEBUG();
> > +
> > + err = omap_request_sound_dma(s->dma_dev, s->id, s, &s->lch);
> > + if (err < 0)
> > + printk(KERN_ERR "unable to grab audio dma 0x%x\n",
> > + s->dma_dev);
>
> s/unable/Unable will look good :)

ok

> > +
> > +/*
> > + * This function should calculate the current position of the dma
> > in the
> > + * buffer. It will help alsa middle layer to continue update the
> > buffer.
> > + * Its correctness is crucial for good functioning.
> > + */
> > +static u_int audio_get_dma_pos(struct audio_stream *s)
> > +{
> > + snd_pcm_substream_t *substream = s->stream;
> > + snd_pcm_runtime_t *runtime = substream->runtime;
> > + unsigned int offset;
> > + unsigned long flags;
> > + dma_addr_t count;
> > + ADEBUG();
> > +
> > + /* this must be called w/ interrupts locked as requested in dma.c
> > */
> > + spin_lock_irqsave(&s->dma_lock, flags);
> > +
> > + /* For the current period let's see where we are */
> > + count = omap_get_dma_src_addr_counter(s->lch[s->dma_q_head]);
> > +
> > + spin_unlock_irqrestore(&s->dma_lock, flags);
> > +
> > + /* Now, the position related to the end of that period */
> > + offset = bytes_to_frames(runtime, s->offset) -
> > bytes_to_frames(runtime, count);
> > +
> > + if (offset >= runtime->buffer_size || offset < 0)
> > + offset = 0;
>
> I am not sure but will this "unsigned int offset" will be ever become <
> 0 here ?

Indeed, offset should not go to below zero but just for the sake that
strange things can happen, we should change its type to int to make
this test properly.

> > +
> > +/*
> > + * This is called when dma IRQ occurs at the end of each transmited
> > block
> > + */
> > +void audio_dma_callback(void *data)
> > +{
> > + struct audio_stream *s = data;
> > + ADEBUG();
> > +
> > + /*
> > + * If we are getting a callback for an active stream then we inform
> > + * the PCM middle layer we've finished a period
> > + */
> > + if (s->active)
> > + snd_pcm_period_elapsed(s->stream);
> > +
> > + spin_lock(&s->dma_lock);
> > + if (s->periods > 0) {
> > + s->periods--;
> > + }
>
> {} not need around if.

ok, and for the next space, brackets ones too.

>
> > +
> > +/*
> > + * Alsa section
> > + * PCM settings and callbacks
> > + */
> > +
> > +static int snd_omap_alsa_trigger(snd_pcm_substream_t * substream,
> > int cmd)
>
> Extra empty line after function comments. Same review comment applies
> to all other places in the file. You may like to remove that.
>
> > +
> > +static int snd_card_omap_alsa_open(snd_pcm_substream_t * substream)
> > +{
> > + struct snd_card_omap_codec *chip =
> > + snd_pcm_substream_chip(substream);
> > + snd_pcm_runtime_t *runtime = substream->runtime;
> > + int stream_id = substream->pstr->stream;
> > + int err;
> > +
> > + ADEBUG();
> > + chip->s[stream_id].stream = substream;
> > + alsa_codec_config->codec_clock_on();
> > + if (stream_id == SNDRV_PCM_STREAM_PLAYBACK) {
> > + runtime->hw = snd_omap_alsa_playback;
> > + }
>
> {} not needed after if and else below.
>
> > + else {
> > + runtime->hw = snd_omap_alsa_capture;
> > + }
> > + if ((err = snd_pcm_hw_constraint_integer(runtime,
> > + SNDRV_PCM_HW_PARAM_PERIODS)) < 0) {
> > + return err;
> > + }
> > + if ((err = snd_pcm_hw_constraint_list(runtime, 0,
> > + SNDRV_PCM_HW_PARAM_RATE,
> > + &hw_constraints_rates)) < 0) {
> > + return err;
> > + }
>
> Ditto. Also adding one empty line between ifs will look better.
>
> > +
> > + /* sets up initial buffer with continuous allocation */
> > + snd_pcm_lib_preallocate_pages_for_all(pcm,
> > + SNDRV_DMA_TYPE_CONTINUOUS,
> > + snd_dma_continuous_data
> > + (GFP_KERNEL),
> > + 128 * 1024, 128 * 1024);
>
> Brackets around GFP_KERNEL may not be needed.
>
> > +
> > + snd_pcm_set_ops(pcm, SNDRV_PCM_STREAM_PLAYBACK,
> > + &snd_card_omap_alsa_playback_ops);
> > + snd_pcm_set_ops(pcm, SNDRV_PCM_STREAM_CAPTURE,
> > + &snd_card_omap_alsa_capture_ops);
> > + pcm->private_data = omap_alsa;
> > + pcm->info_flags = 0;
> > + strcpy(pcm->name, "omap aic23 pcm");
>
> aic23?

ok

> > +
> > +
> > +#ifdef CONFIG_PM
> > +
> > +static int snd_omap_alsa_suspend(snd_card_t * card, pm_message_t
> > state)
>
> Empty line after CONFIG_PM may not be needed.
>
> > +
> > +#else
> > +#define snd_omap_alsa_suspend NULL
> > +#define snd_omap_alsa_resume NULL
> > +#define omap_alsa_suspend NULL
> > +#define omap_alsa_resume NULL
> > +
> > +#endif /* CONFIG_PM */
>
> May be snd_omap_alsa_suspend(x) do {} while(0) style would be better.

Some files looked as models has used this manner. Mainly the PXA ones..

> > +
> > +/*
> > + */
> > +void snd_omap_alsa_free(snd_card_t * card)
> > +{
> > + struct snd_card_omap_codec *chip = card->private_data;
> > + ADEBUG();
> > +
> > + /*
> > + * Turn off codec after it is done.
> > + * Can't do it immediately, since it may still have
> > + * buffered data.
> > + */
> > + set_current_state(TASK_INTERRUPTIBLE);
> > + schedule_timeout(2);
>
> We have now API to combine this two functions. There one long name if I
> remember.

Indeed, you mean schedule_timeout_interruptible . I will change. That
is one issue of not being in the main line :-(

> > +
> > + if (alsa_codec_config && alsa_codec_config->codec_configure_dev) {
> > + alsa_codec_config->codec_configure_dev();
> > + }
>
> As mentioned earlier...{} not needed, also few more in code below.
>
> > +
> > +static int snd_omap_alsa_remove(struct platform_device *pdev)
> > +{
> > + snd_card_t *card = platform_get_drvdata(pdev);
> > + struct snd_card_omap_codec *chip = card->private_data;
> > +
> > + snd_card_free(card);
> > +
> > + alsa_codec = NULL;
> > + card->private_data = NULL;
> > + kfree(chip);
> > +
> > + platform_set_drvdata(pdev, NULL);
> > +
> > + return 0;
> > +
> > +}
>
> Extra empty lines after return 0.
>
> > +
> > +static struct platform_driver omap_alsa_driver = {
> > + .probe = snd_omap_alsa_probe,
> > + .remove = snd_omap_alsa_remove,
> > + .suspend = omap_alsa_suspend,
> > + .resume = omap_alsa_resume,
>
> May be keeping consistent naming will be good for suspend and resume.

Allright.

> > +
> > +/*
> > + * Alsa card structure for aic23
> > + */
>
> aic23?
>
> > +
> > +/*
> > + * Defines codec specific functions pointers that can be used from
> > the
> > + * common omap-alse base driver for all omap codecs. (tsc2101 and
> > aic23)
> > + */
> > +void define_codec_functions(struct omap_alsa_codec_config
> > *codec_config);
> > +inline void aic23_configure(void);
> > +void aic23_set_samplerate(long rate);
> > +void aic23_clock_setup(void);
> > +int aic23_clock_on(void);
> > +int aic23_clock_off(void);
> > +int aic23_get_default_samplerate(void);
> > +
> > +inline void tsc2101_configure(void);
> > +void tsc2101_set_samplerate(long rate);
> > +void tsc2101_clock_setup(void);
> > +int tsc2101_clock_on(void);
> > +int tsc2101_clock_off(void);
> > +int tsc2101_get_default_samplerate(void);
>
> I think above functions should not be mentioned in omap-alsa.h, as it
> meant to be independant of any codec information. You may want to put
> them in specific codec header files, anyway they are going to called
> through function pointers I believe.

They are called through function pointers... And actually, this
prototypes can go to specific codec files, their only need is in the
board file.

> ---Komal Shah
> http://komalshah.blogspot.com/

Thanks for that,
Daniel
--
INdT - Manaus - Brazil