Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.36 as permitted sender) client-ip=23.128.96.36;
Date:   Tue, 19 Sep 2023 09:56:00 +0200
Message-ID: <87y1h2y4tr.wl-tiwai@suse.de>
From:   Takashi Iwai <tiwai@suse.de>
To:     Ivan Orlov <ivan.orlov0322@gmail.com>
Cc:     perex@perex.cz, tiwai@suse.com, corbet@lwn.net,
        alsa-devel@alsa-project.org, linux-doc@vger.kernel.org,
        linux-kernel@vger.kernel.org, gregkh@linuxfoundation.org
Subject: Re: [PATCH v2 2/2] ALSA: Add new driver for Marian M2 sound card
In-Reply-To: <20230918181044.7257-2-ivan.orlov0322@gmail.com>
References: <20230918181044.7257-1-ivan.orlov0322@gmail.com>
        <20230918181044.7257-2-ivan.orlov0322@gmail.com>
User-Agent: Wanderlust/2.15.9 (Almost Unreal) Emacs/27.2 Mule/6.0
MIME-Version: 1.0 (generated by SEMI-EPG 1.14.7 - "Harue")
Content-Type: text/plain; charset=US-ASCII
Precedence: bulk

On Mon, 18 Sep 2023 20:10:44 +0200,
Ivan Orlov wrote:
> 
> +#include <sound/core.h>
> +#include <sound/control.h>
> +#include <sound/pcm.h>
> +#include <sound/pcm_params.h>
> +#include <sound/core.h>
> +#include <sound/pcm.h>
> +#include <sound/initval.h>
> +#include <sound/info.h>
> +#include <linux/delay.h>
> +#include <linux/module.h>
> +#include <linux/pci.h>
> +#include <linux/interrupt.h>

We usually include linux/* at first, followed by sound/*.


> +#define DEBUG

Any need to define this for the production system?


> +struct marian_card_descriptor;
> +struct marian_card;
> +
> +struct marian_card_descriptor {
> +	char *name;
> +	char *port_names;
> +	unsigned int speedmode_max;
> +	unsigned int ch_in;
> +	unsigned int ch_out;
> +	unsigned int midi_in;
> +	unsigned int midi_out;
> +	unsigned int serial_in;
> +	unsigned int serial_out;
> +	unsigned int wck_in;
> +	unsigned int wck_out;
> +
> +	unsigned int dma_bufsize;
> +
> +	void (*hw_constraints_func)(struct marian_card *marian,
> +				    struct snd_pcm_substream *substream,
> +				    struct snd_pcm_hw_params *params);
> +	/* custom function to set up ALSA controls */
> +	void (*create_controls)(struct marian_card *marian);
> +	/* init is called after probing the card */
> +	int (*init_card)(struct marian_card *marian);
> +	void (*free_card)(struct marian_card *marian);
> +	/* prepare is called when ALSA is opening the card */
> +	void (*prepare)(struct marian_card *marian);
> +	void (*set_speedmode)(struct marian_card *marian, unsigned int speedmode);
> +	void (*proc_status)(struct marian_card *marian, struct snd_info_buffer *buffer);
> +	void (*proc_ports)(struct marian_card *marian, struct snd_info_buffer *buffer,
> +			   unsigned int type);
> +
> +	struct snd_pcm_hardware info_playback;
> +	struct snd_pcm_hardware info_capture;

Do we need this kind of abstraction inside the driver?
As far as I see, the driver supports only a single model, hence there
is no real merit of abstracted / indirect function calls.

So I stop reading at this point.


thanks,

Takashi