From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752511AbbCZSMG (ORCPT ); Thu, 26 Mar 2015 14:12:06 -0400 Received: from mezzanine.sirena.org.uk ([106.187.55.193]:40329 "EHLO mezzanine.sirena.org.uk" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751342AbbCZSMC (ORCPT ); Thu, 26 Mar 2015 14:12:02 -0400 Date: Thu, 26 Mar 2015 11:11:06 -0700 From: Mark Brown To: Yakir Yang Cc: Liam Girdwood , djkurtz@chromium.org, dianders@chromium.org, linux-rockchip@lists.infradead.org, David Airlie , Philipp Zabel , Russell King , Andy Yan , Greg Kroah-Hartman , Fabio Estevam , dri-devel@lists.freedesktop.org, Jaroslav Kysela , Takashi Iwai , Lars-Peter Clausen , Brian Austin , Bard Liao , Oder Chiou , Max Filippov , Axel Lin , Arnd Bergmann , Jyri Sarha , Sean Cross , Ben Zhang , linux-kernel@vger.kernel.org, alsa-devel@alsa-project.org, mmind00@googlemail.com, marcheu@chromium.org, mark.yao@rock-chips.com Message-ID: <20150326181106.GY3572@sirena.org.uk> References: <1425175834-24661-1-git-send-email-ykk@rock-chips.com> <1425178760-2655-1-git-send-email-ykk@rock-chips.com> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="rmLdANEyqdgxlbgR" Content-Disposition: inline In-Reply-To: <1425178760-2655-1-git-send-email-ykk@rock-chips.com> X-Cookie: Better dead than mellow. User-Agent: Mutt/1.5.23 (2014-03-12) X-SA-Exim-Connect-IP: 12.51.221.141 X-SA-Exim-Mail-From: broonie@sirena.org.uk Subject: Re: [PATCH v4 13/15] ASoC: codec/dw-hdmi-audio: add codec driver for dw hdmi audio X-SA-Exim-Version: 4.2.1 (built Mon, 26 Dec 2011 16:24:06 +0000) X-SA-Exim-Scanned: Yes (on mezzanine.sirena.org.uk) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org --rmLdANEyqdgxlbgR Content-Type: text/plain; charset=us-ascii Content-Disposition: inline On Sat, Feb 28, 2015 at 09:59:20PM -0500, Yakir Yang wrote: > codec driver creat an standard alsa device, than config audio > and report jack status through some callback interfaces that > dw_hdmi driver support. Looking at this it's not althogether clear to me how specific this is to the Designware hardware - it looks like it's all callbacks into the main driver doing pretty generic things apart from the fact that we request an interrupt here (but then use it to do another callback into the driver). Please also try to only CC relevant people on mails - you've got a *very* large list of people there and for a lot of them it's hard to understand why you've copied them. Copying people adds to the amount of mail they need to read so it's good to try to stay relevant. > + if (jack_status != hdmi->jack_status) { > + snd_soc_jack_report(&hdmi->jack, jack_status, > + SND_JACK_LINEOUT); We may need a new jack type here, or perhaps we ought to just be reporting the jack status via extcon? > + hdmi->jack_status = jack_status; > + > + dev_info(hdmi->dev, "jack report [%d]\n", hdmi->jack_status); Please remove this and all the other prints, it's far too noisy. > +/* we don't want this irq mark with IRQF_ONESHOT flags, > + * so we build an irq_default_primary_handler here */ > +static irqreturn_t snd_dw_hdmi_hardirq(int irq, void *dev_id) > +{ > + return IRQ_WAKE_THREAD; > +} Why do we not want to use IRQF_ONESHOT? > +static int dw_hdmi_audio_remove(struct platform_device *pdev) > +{ > + struct snd_dw_hdmi *hdmi = platform_get_drvdata(pdev); > + > + snd_soc_unregister_codec(&pdev->dev); > + devm_free_irq(&pdev->dev, hdmi->data.irq, hdmi); > + devm_kfree(&pdev->dev, hdmi); Explicitly freeing devm_ things seems to be missing the point a bit... > +static const struct of_device_id dw_hdmi_audio_ids[] = { > + { .compatible = DRV_NAME, }, > + { } > +}; Your driver name didn't have a vendor prefix, this is broken - you should probably just remove DRV_NAME and use the string directly in the few places it's used. It's also not clear to me that this is a separate device from the parent device and should therefore appear separately in DT at all. > +static struct platform_driver dw_hdmi_audio_driver = { > + .driver = { > + .name = DRV_NAME, > + .owner = THIS_MODULE, No need to assign owner any more. --rmLdANEyqdgxlbgR Content-Type: application/pgp-signature; name="signature.asc" Content-Description: Digital signature -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 iQEcBAEBCAAGBQJVFEu5AAoJECTWi3JdVIfQV4EH/1Tcb/z86vDPZ2gkqXExXi4K zflEkPBVZg7PSRJ7HvS0DyFt4hRZiKqNPSHP1MEaECOUgX5BHtpwD61vtb3ZRgUD e/ES2KixzMu2Aq4rdRJhCTbvzJQeo/XkmTHI0LzIi692YgMPSjpIvcU9f4EEfDmY E0QJdyw56TbT8cKPbrO8/z107au7cTvEbnyowYHeg3O8lfnXeGnb97bHfv38hnVi Gs6ZRw01qu0CXyWAgxlNiZ3Hto2T1X/lVsNs9caFjkNyuE7O34c7RPJhSTplWJJL JhNt+Sma3FXrNiwZzwqnXYzyVq8QB9Brs+3ymMyMToxORvXucO4uzAjyNAodQ/g= =Ofzc -----END PGP SIGNATURE----- --rmLdANEyqdgxlbgR-- From mboxrd@z Thu Jan 1 00:00:00 1970 From: Mark Brown Subject: Re: [PATCH v4 13/15] ASoC: codec/dw-hdmi-audio: add codec driver for dw hdmi audio Date: Thu, 26 Mar 2015 11:11:06 -0700 Message-ID: <20150326181106.GY3572@sirena.org.uk> References: <1425175834-24661-1-git-send-email-ykk@rock-chips.com> <1425178760-2655-1-git-send-email-ykk@rock-chips.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============7966825947444972214==" Return-path: In-Reply-To: <1425178760-2655-1-git-send-email-ykk-TNX95d0MmH7DzftRWevZcw@public.gmane.org> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: "Linux-rockchip" Errors-To: linux-rockchip-bounces+glpar-linux-rockchip=m.gmane.org-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org To: Yakir Yang Cc: alsa-devel-K7yf7f+aM1XWsZ/bQMPhNw@public.gmane.org, David Airlie , djkurtz-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org, dianders-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org, Max Filippov , Bard Liao , mmind00-gM/Ye1E23mwN+BqQ9rBEUg@public.gmane.org, Lars-Peter Clausen , Axel Lin , linux-rockchip-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org, Takashi Iwai , Russell King , Sean Cross , Oder Chiou , Arnd Bergmann , Jyri Sarha , Ben Zhang , dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org, marcheu-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org, Jaroslav Kysela , mark.yao-TNX95d0MmH7DzftRWevZcw@public.gmane.org, Fabio Estevam , Brian Austin , Greg Kroah-Hartman , Liam Girdwood , linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, Philipp Zabel List-Id: alsa-devel@alsa-project.org --===============7966825947444972214== Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="rmLdANEyqdgxlbgR" Content-Disposition: inline --rmLdANEyqdgxlbgR Content-Type: text/plain; charset=us-ascii Content-Disposition: inline On Sat, Feb 28, 2015 at 09:59:20PM -0500, Yakir Yang wrote: > codec driver creat an standard alsa device, than config audio > and report jack status through some callback interfaces that > dw_hdmi driver support. Looking at this it's not althogether clear to me how specific this is to the Designware hardware - it looks like it's all callbacks into the main driver doing pretty generic things apart from the fact that we request an interrupt here (but then use it to do another callback into the driver). Please also try to only CC relevant people on mails - you've got a *very* large list of people there and for a lot of them it's hard to understand why you've copied them. Copying people adds to the amount of mail they need to read so it's good to try to stay relevant. > + if (jack_status != hdmi->jack_status) { > + snd_soc_jack_report(&hdmi->jack, jack_status, > + SND_JACK_LINEOUT); We may need a new jack type here, or perhaps we ought to just be reporting the jack status via extcon? > + hdmi->jack_status = jack_status; > + > + dev_info(hdmi->dev, "jack report [%d]\n", hdmi->jack_status); Please remove this and all the other prints, it's far too noisy. > +/* we don't want this irq mark with IRQF_ONESHOT flags, > + * so we build an irq_default_primary_handler here */ > +static irqreturn_t snd_dw_hdmi_hardirq(int irq, void *dev_id) > +{ > + return IRQ_WAKE_THREAD; > +} Why do we not want to use IRQF_ONESHOT? > +static int dw_hdmi_audio_remove(struct platform_device *pdev) > +{ > + struct snd_dw_hdmi *hdmi = platform_get_drvdata(pdev); > + > + snd_soc_unregister_codec(&pdev->dev); > + devm_free_irq(&pdev->dev, hdmi->data.irq, hdmi); > + devm_kfree(&pdev->dev, hdmi); Explicitly freeing devm_ things seems to be missing the point a bit... > +static const struct of_device_id dw_hdmi_audio_ids[] = { > + { .compatible = DRV_NAME, }, > + { } > +}; Your driver name didn't have a vendor prefix, this is broken - you should probably just remove DRV_NAME and use the string directly in the few places it's used. It's also not clear to me that this is a separate device from the parent device and should therefore appear separately in DT at all. > +static struct platform_driver dw_hdmi_audio_driver = { > + .driver = { > + .name = DRV_NAME, > + .owner = THIS_MODULE, No need to assign owner any more. --rmLdANEyqdgxlbgR Content-Type: application/pgp-signature; name="signature.asc" Content-Description: Digital signature -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 iQEcBAEBCAAGBQJVFEu5AAoJECTWi3JdVIfQV4EH/1Tcb/z86vDPZ2gkqXExXi4K zflEkPBVZg7PSRJ7HvS0DyFt4hRZiKqNPSHP1MEaECOUgX5BHtpwD61vtb3ZRgUD e/ES2KixzMu2Aq4rdRJhCTbvzJQeo/XkmTHI0LzIi692YgMPSjpIvcU9f4EEfDmY E0QJdyw56TbT8cKPbrO8/z107au7cTvEbnyowYHeg3O8lfnXeGnb97bHfv38hnVi Gs6ZRw01qu0CXyWAgxlNiZ3Hto2T1X/lVsNs9caFjkNyuE7O34c7RPJhSTplWJJL JhNt+Sma3FXrNiwZzwqnXYzyVq8QB9Brs+3ymMyMToxORvXucO4uzAjyNAodQ/g= =Ofzc -----END PGP SIGNATURE----- --rmLdANEyqdgxlbgR-- --===============7966825947444972214== Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Content-Disposition: inline _______________________________________________ Linux-rockchip mailing list Linux-rockchip-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org http://lists.infradead.org/mailman/listinfo/linux-rockchip --===============7966825947444972214==--