lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <8e39a894-e921-51e6-c9bf-b007a08b10fb@gmail.com>
Date:   Tue, 19 Sep 2023 12:35:45 +0400
From:   Ivan Orlov <ivan.orlov0322@...il.com>
To:     Takashi Iwai <tiwai@...e.de>
Cc:     perex@...ex.cz, tiwai@...e.com, corbet@....net,
        alsa-devel@...a-project.org, linux-doc@...r.kernel.org,
        linux-kernel@...r.kernel.org, gregkh@...uxfoundation.org
Subject: Re: [PATCH v2 2/2] ALSA: Add new driver for Marian M2 sound card

On 9/19/23 11:56, Takashi Iwai wrote:
> 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

Hi Takashi,

Thank you for the review! I will send the next version after removing 
all indirections and cleaning the code again.

--
Kind regards,
Ivan Orlov

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ