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");- }
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");- }
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.