Hi Mark, On Sun, 2012-02-12 at 13:20 +0000, Mark Brown wrote:
On Sat, Feb 11, 2012 at 06:23:15PM -0600, Ricardo Neri wrote:
The ASoC HDMI codec is to be relocated under sound/soc/codecs. This patch places the codec under ASoC. A previous patch removed the code from DSS. The purpose of the relocation is to give a more logical organization to OMAP HDMI code.
Looks pretty clean, a few things below but mostly fairly nitpicky.
Thanks for your comments!
sound/soc/codecs/omap-hdmi.c | 422 ++++++++++++++++++++++++++++++++++++++++++
If this is relocating the code shouldn't some old code be being deleted?
Yes, indeed. However, as it is being relocated from DSS, I thought it would be good to split in two patches so that the respective maintainers can apply independently. Here is the patch that removes the code from DSS:
http://www.spinics.net/lists/linux-omap/msg64482.html
- switch (sample_freq) {
- case 32000:
if ((deep_color == 125) && ((pclk == 54054)|| (pclk == 74250)))*n = 8192;else*n = 4096;Use a switch on deep_color too (in all these cases).
+static int hdmi_audio_startup(struct snd_pcm_substream *substream,
struct snd_soc_dai *dai)+{
- if (omapdss_hdmi_get_hdmi_mode() != HDMI_HDMI) {
pr_err("Current video settings do not support audio.\n");dev_ prints please (throughout the driver).
I will correct.
- priv = kzalloc(sizeof(*priv), GFP_KERNEL);
- if (priv == NULL)
return -ENOMEM;Use devm_ functions for this and mapping the io region - that way you don't have to worry about them when cleaning up and doing error handling...
- hdmi_rsrc = platform_get_resource(pdev, IORESOURCE_MEM, 0);
- if (!hdmi_rsrc) {
dev_err(&pdev->dev, "Cannot obtain IORESOURCE_MEM HDMI\n");return -EINVAL;- }
...like here :) You should also move the allocation and mapping to the platform device probe and only register the CODEC once you've got everything else, this is more idiomatic for the Linux driver stack.
I will correct this as well.
+static struct snd_soc_dai_driver hdmi_codec_dai_drv = {
- .name = "hdmi-audio-codec",
Should proably have an omap- in here somewhere.
+static int __init hdmi_codec_init(void) +{
- return platform_driver_register(&hdmi_codec_driver);
+} +module_init(hdmi_codec_init);
module_platform_driver()