linux-kernel - Re: [PATCH 3/5] ASoC: RX-51: Add DT support to sound driver

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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20140415123449.GC12304@sirena.org.uk>
Date:	Tue, 15 Apr 2014 13:34:49 +0100
From:	Mark Brown <broonie@...nel.org>
To:	Sebastian Reichel <sre@...nel.org>
Cc:	Sebastian Reichel <sre@...g0.de>,
	Peter Ujfalusi <peter.ujfalusi@...com>,
	Pawel Moll <pawel.moll@....com>,
	Mark Rutland <mark.rutland@....com>,
	Ian Campbell <ijc+devicetree@...lion.org.uk>,
	Kumar Gala <galak@...eaurora.org>,
	Tony Lindgren <tony@...mide.com>,
	Liam Girdwood <lgirdwood@...il.com>,
	Jarkko Nikula <jarkko.nikula@...mer.com>,
	devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
	linux-omap@...r.kernel.org, alsa-devel@...a-project.org
Subject: Re: [PATCH 3/5] ASoC: RX-51: Add DT support to sound driver

On Sat, Apr 05, 2014 at 11:35:51PM +0200, Sebastian Reichel wrote:
> This patch adds device tree support to the Nokia N900 audio driver.
> It also removes GPIO defines and gets them from platform data /
> device tree, since some GPIO numbers may be different with DT boot.

This binding looks mostly fine, a few code things though which may
influence the binding.  The main thing is that you're doing a lot of
changes here which aren't really related to adding the binding which
aren't mentioned and make it harder to review - as well as making the
change larger one of the things that's done in review is to check that
the change did what it was described as doing.  At least some of the
time there isn't even any code overlap.

> @@ -290,6 +286,9 @@ static const struct snd_kcontrol_new aic34_rx51_controlsb[] = {
>  static int rx51_aic34_init(struct snd_soc_pcm_runtime *rtd)
>  {
>  	struct snd_soc_codec *codec = rtd->codec;
> +	struct snd_soc_card *card = codec->card;
> +	struct rx51_audio_pdata *pdata = snd_soc_card_get_drvdata(card);
> +
>  	struct snd_soc_dapm_context *dapm = &codec->dapm;
>  	int err;
>  

Random blank line added here.

> @@ -301,8 +300,10 @@ static int rx51_aic34_init(struct snd_soc_pcm_runtime *rtd)
>  	/* Add RX-51 specific controls */
>  	err = snd_soc_add_card_controls(rtd->card, aic34_rx51_controls,
>  				   ARRAY_SIZE(aic34_rx51_controls));
> -	if (err < 0)
> +	if (err < 0) {
> +		dev_err(card->dev, "Failed to add RX-51 specific controls\n");
>  		return err;
> +	}
>  
>  	/* Add RX-51 specific widgets */
>  	snd_soc_dapm_new_controls(dapm, aic34_dapm_widgets,

This is nothing to do with DT support, separate patch please.  There's
quite a few other things like this.  You're also not printing any error
codes in the messages.

> @@ -312,25 +313,39 @@ static int rx51_aic34_init(struct snd_soc_pcm_runtime *rtd)
>  	snd_soc_dapm_add_routes(dapm, audio_map, ARRAY_SIZE(audio_map));
>  
>  	err = tpa6130a2_add_controls(codec);
> -	if (err < 0)
> +	if (err < 0) {
> +		dev_err(card->dev, "Failed to add TPA6130A2 controls\n");
>  		return err;
> +	}

If this is converted to DT and you've added aux CODEC support as part of
that then why are we still manually adding the controls for the
headphone amp?  I would have expected this to be registered as an aux
CODEC.  This seems likely to feed through into the binding for
referencing the components.

> +	err = omap_mcbsp_st_add_controls(rtd, 2);
> +	if (err < 0) {
> +		dev_err(card->dev, "Failed to add MCBSP controls\n");
>  		return err;
> +	}

Refactoring this as a separate patch would also help.

> -	err = gpio_request_one(RX51_ECI_SW_GPIO,
> +	if (err) {
> +		dev_err(card->dev, "could not setup tvout_sel\n");
> +		goto error;
> +	}
> +	err = devm_gpio_request_one(card->dev, pdata->eci_sw_gpio,
>  			       GPIOF_DIR_OUT | GPIOF_INIT_HIGH, "eci_sw");

Again, moving this refactoring into a separate patch will help a lot
with review.

Download attachment "signature.asc" of type "application/pgp-signature" (837 bytes)