From: Mark Brown <broonie@kernel.org> To: Sebastian Reichel <sre@kernel.org> Cc: Sebastian Reichel <sre@ring0.de>, Peter Ujfalusi <peter.ujfalusi@ti.com>, Pawel Moll <pawel.moll@arm.com>, Mark Rutland <mark.rutland@arm.com>, Ian Campbell <ijc+devicetree@hellion.org.uk>, Kumar Gala <galak@codeaurora.org>, Tony Lindgren <tony@atomide.com>, Liam Girdwood <lgirdwood@gmail.com>, Jarkko Nikula <jarkko.nikula@bitmer.com>, devicetree@vger.kernel.org, linux-kernel@vger.kernel.org, linux-omap@vger.kernel.org, alsa-devel@alsa-project.org Subject: Re: [PATCH 3/5] ASoC: RX-51: Add DT support to sound driver Date: Tue, 15 Apr 2014 13:34:49 +0100 [thread overview] Message-ID: <20140415123449.GC12304@sirena.org.uk> (raw) In-Reply-To: <1396733753-9820-4-git-send-email-sre@kernel.org> [-- Attachment #1: Type: text/plain, Size: 3018 bytes --] 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. [-- Attachment #2: Digital signature --] [-- Type: application/pgp-signature, Size: 836 bytes --]
WARNING: multiple messages have this Message-ID (diff)
From: Mark Brown <broonie-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> To: Sebastian Reichel <sre-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> Cc: Sebastian Reichel <sre-GFxCN5SEZAc@public.gmane.org>, Peter Ujfalusi <peter.ujfalusi-l0cyMroinI0@public.gmane.org>, Pawel Moll <pawel.moll-5wv7dgnIgG8@public.gmane.org>, Mark Rutland <mark.rutland-5wv7dgnIgG8@public.gmane.org>, Ian Campbell <ijc+devicetree-KcIKpvwj1kUDXYZnReoRVg@public.gmane.org>, Kumar Gala <galak-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>, Tony Lindgren <tony-4v6yS6AI5VpBDgjK7y7TUQ@public.gmane.org>, Liam Girdwood <lgirdwood-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>, Jarkko Nikula <jarkko.nikula-FVTvWyuFUl3QT0dZR+AlfA@public.gmane.org>, devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, linux-omap-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, alsa-devel-K7yf7f+aM1XWsZ/bQMPhNw@public.gmane.org Subject: Re: [PATCH 3/5] ASoC: RX-51: Add DT support to sound driver Date: Tue, 15 Apr 2014 13:34:49 +0100 [thread overview] Message-ID: <20140415123449.GC12304@sirena.org.uk> (raw) In-Reply-To: <1396733753-9820-4-git-send-email-sre-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> [-- Attachment #1: Type: text/plain, Size: 3018 bytes --] 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. [-- Attachment #2: Digital signature --] [-- Type: application/pgp-signature, Size: 836 bytes --]
next prev parent reply other threads:[~2014-04-15 12:35 UTC|newest] Thread overview: 16+ messages / expand[flat|nested] mbox.gz Atom feed top 2014-04-05 21:35 [PATCH 0/5] DT support for N900 soundcard (rx51-audio) Sebastian Reichel 2014-04-05 21:35 ` [PATCH 1/5] ASoC: omap: rx51: Use devm_snd_soc_register_card Sebastian Reichel 2014-04-15 12:20 ` Mark Brown 2014-04-15 12:20 ` Mark Brown 2014-04-15 13:39 ` [alsa-devel] " Rajeev kumar 2014-04-15 13:39 ` Rajeev kumar 2014-04-05 21:35 ` [PATCH 2/5] ASoC: Allow Aux Codecs to be specified using DT Sebastian Reichel 2014-04-15 12:14 ` Mark Brown 2014-04-15 12:14 ` Mark Brown 2014-04-05 21:35 ` [PATCH 3/5] ASoC: RX-51: Add DT support to sound driver Sebastian Reichel 2014-04-15 12:34 ` Mark Brown [this message] 2014-04-15 12:34 ` Mark Brown 2014-04-05 21:35 ` [PATCH 4/5] ASoC: RX-51: Convert to table based DAPM setup Sebastian Reichel 2014-04-15 12:23 ` Mark Brown 2014-04-05 21:35 ` [PATCH 5/5] ASoC: tlv320aic3x: fix shared reset pin for DT Sebastian Reichel 2014-04-15 12:24 ` Mark Brown
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20140415123449.GC12304@sirena.org.uk \ --to=broonie@kernel.org \ --cc=alsa-devel@alsa-project.org \ --cc=devicetree@vger.kernel.org \ --cc=galak@codeaurora.org \ --cc=ijc+devicetree@hellion.org.uk \ --cc=jarkko.nikula@bitmer.com \ --cc=lgirdwood@gmail.com \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-omap@vger.kernel.org \ --cc=mark.rutland@arm.com \ --cc=pawel.moll@arm.com \ --cc=peter.ujfalusi@ti.com \ --cc=sre@kernel.org \ --cc=sre@ring0.de \ --cc=tony@atomide.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.