Re: [PATCH 3/3] Add a codec driver for SI476X MFD

[Mark Brown]

On Thu, Sep 13, 2012 at 03:40:13PM -0700, Andrey Smirnov wrote:
 This commit add a sound codec driver for Silicon Laboratories 476x
 series of AM/FM radio chips.

*Always* CC both maintainers and lists on patches.  There's a few
problems here, though none of them terribly substantial.

   select SND_SOC_UDA1380 if I2C
   select SND_SOC_WL1273 if MFD_WL1273_CORE
 + select SND_SOC_SI476X if MFD_SI476X_CORE
   select SND_SOC_WM1250_EV1 if I2C


Keep the Makefile and Kconfig sorted.

 +struct si476x_codec {
 + struct si476x_core *core;
 +};

control_data.

 +static int si476x_codec_set_daudio_params(struct snd_soc_codec *codec,
 +   int width, int rate)
 +{

Just inline this into the one user.

 + int err;
 + u16 digital_io_output_format = \
 + snd_soc_read(codec,
 +  SI476X_DIGITAL_IO_OUTPUT_FORMAT);

Throughout the driver you're randomly using \ for no visible reason -
don't do that.

 + if ((rate  32000) || (rate  48000)) {
 + dev_dbg(codec-dev, Rate: %d is not supported\n, rate);

dev_err.

 + digital_io_output_format = SI476X_DIGITAL_IO_OUTPUT_WIDTH_MASK;
 + digital_io_output_format |= (width  11) | (width  8);
 +
 + return snd_soc_write(codec, SI476X_DIGITAL_IO_OUTPUT_FORMAT,
 +  digital_io_output_format);

snd_soc_update_bits().

 +static int si476x_codec_volume_get(struct snd_kcontrol *kcontrol,
 +struct snd_ctl_elem_value *ucontrol)
 +{
 + struct snd_soc_codec *codec = snd_kcontrol_chip(kcontrol);
 +
 + ucontrol-value.integer.value[0] =
 + snd_soc_read(codec, SI476X_AUDIO_VOLUME);
 + return 0;
 +}

Why are you open coding this?  Looks like a standard register...

 +static int si476x_codec_digital_mute(struct snd_soc_dai *codec_dai, int mute)
 +{
 + if (mute)
 + snd_soc_write(codec_dai-codec, SI476X_AUDIO_MUTE, 0x3);
 +
 + return 0;
 +}

This will never disable the mute once it's been activated; are you sure
this code has been tested?

 + switch (params_format(params)) {
 + case SNDRV_PCM_FORMAT_S8:
 + width = SI476X_PCM_FORMAT_S8;
 + case SNDRV_PCM_FORMAT_S16_LE:

Missing break;

 +static int si476x_codec_probe(struct snd_soc_codec *codec)
 +{
 + struct si476x_core **core = codec-dev-platform_data;
 + struct si476x_codec *si476x;
 +
 + if (!core) {
 + dev_err(codec-dev, Platform data is missing.\n);
 + return -EINVAL;
 + }

You should use dev-parent to find the parent rather than use platform
data like this.

 + si476x = kzalloc(sizeof(*si476x), GFP_KERNEL);
 + if (si476x == NULL) {

devm_kzalloc(), and this should be in the device level probe (as should
the previous check for the core).  Though as the struct ought to be
empty now this'll presumably go away.

 +static int __init si476x_init(void)
 +{
 + return platform_driver_register(si476x_platform_driver);
 +}
 +module_init(si476x_init);

module_platform_driver()
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 3/3] Add a codec driver for SI476X MFD

[Andrey Smirnov]

This commit add a sound codec driver for Silicon Laboratories 476x
series of AM/FM radio chips.

Signed-off-by: Andrey Smirnov andrey.smir...@convergeddevices.net
---
 sound/soc/codecs/Kconfig  |4 +
 sound/soc/codecs/Makefile |2 +
 sound/soc/codecs/si476x.c |  346 +
 3 files changed, 352 insertions(+)
 create mode 100644 sound/soc/codecs/si476x.c

diff --git a/sound/soc/codecs/Kconfig b/sound/soc/codecs/Kconfig
index 9f8e859..71ab390 100644
--- a/sound/soc/codecs/Kconfig
+++ b/sound/soc/codecs/Kconfig
@@ -70,6 +70,7 @@ config SND_SOC_ALL_CODECS
select SND_SOC_UDA134X
select SND_SOC_UDA1380 if I2C
select SND_SOC_WL1273 if MFD_WL1273_CORE
+   select SND_SOC_SI476X if MFD_SI476X_CORE
select SND_SOC_WM1250_EV1 if I2C
select SND_SOC_WM2000 if I2C
select SND_SOC_WM2200 if I2C
@@ -326,6 +327,9 @@ config SND_SOC_UDA1380
 config SND_SOC_WL1273
tristate
 
+config SND_SOC_SI476X
+   tristate
+
 config SND_SOC_WM1250_EV1
tristate
 
diff --git a/sound/soc/codecs/Makefile b/sound/soc/codecs/Makefile
index 34148bb..aecf09b 100644
--- a/sound/soc/codecs/Makefile
+++ b/sound/soc/codecs/Makefile
@@ -61,6 +61,7 @@ snd-soc-twl6040-objs := twl6040.o
 snd-soc-uda134x-objs := uda134x.o
 snd-soc-uda1380-objs := uda1380.o
 snd-soc-wl1273-objs := wl1273.o
+snd-soc-si476x-objs := si476x.o
 snd-soc-wm1250-ev1-objs := wm1250-ev1.o
 snd-soc-wm2000-objs := wm2000.o
 snd-soc-wm2200-objs := wm2200.o
@@ -177,6 +178,7 @@ obj-$(CONFIG_SND_SOC_TWL6040)   += snd-soc-twl6040.o
 obj-$(CONFIG_SND_SOC_UDA134X)  += snd-soc-uda134x.o
 obj-$(CONFIG_SND_SOC_UDA1380)  += snd-soc-uda1380.o
 obj-$(CONFIG_SND_SOC_WL1273)   += snd-soc-wl1273.o
+obj-$(CONFIG_SND_SOC_SI476X)   += snd-soc-si476x.o
 obj-$(CONFIG_SND_SOC_WM1250_EV1) += snd-soc-wm1250-ev1.o
 obj-$(CONFIG_SND_SOC_WM2000)   += snd-soc-wm2000.o
 obj-$(CONFIG_SND_SOC_WM2200)   += snd-soc-wm2200.o
diff --git a/sound/soc/codecs/si476x.c b/sound/soc/codecs/si476x.c
new file mode 100644
index 000..beea2ca
--- /dev/null
+++ b/sound/soc/codecs/si476x.c
@@ -0,0 +1,346 @@
+#include linux/module.h
+#include linux/slab.h
+#include sound/pcm.h
+#include sound/pcm_params.h
+#include sound/soc.h
+#include sound/initval.h
+
+#include linux/mfd/si476x-core.h
+
+#define SI476X_AUDIO_VOLUME0x0300
+#define SI476X_AUDIO_MUTE  0x0301
+#define SI476X_DIGITAL_IO_OUTPUT_FORMAT0x0203
+#define SI476X_DIGITAL_IO_OUTPUT_SAMPLE_RATE   0x0202
+
+#define SI476X_DIGITAL_IO_OUTPUT_WIDTH_MASK~((0b111  11) | (0b111  8))
+#define SI476X_DIGITAL_IO_OUTPUT_FORMAT_MASK   ~(0b110)
+
+
+/* codec private data */
+struct si476x_codec {
+   struct si476x_core *core;
+};
+
+static unsigned int si476x_codec_read(struct snd_soc_codec *codec,
+ unsigned int reg)
+{
+   int err;
+   struct si476x_codec *si476x = snd_soc_codec_get_drvdata(codec);
+   struct si476x_core  *core   = si476x-core;
+
+   si476x_core_lock(core);
+   err = si476x_core_cmd_get_property(core, reg);
+   si476x_core_unlock(core);
+
+   return err;
+}
+
+static int si476x_codec_write(struct snd_soc_codec *codec,
+ unsigned int reg, unsigned int val)
+{
+   int err;
+   struct si476x_codec *si476x = snd_soc_codec_get_drvdata(codec);
+   struct si476x_core  *core   = si476x-core;
+
+   si476x_core_lock(core);
+   err = si476x_core_cmd_set_property(core, reg, val);
+   si476x_core_unlock(core);
+
+   return err;
+}
+
+
+
+static int si476x_codec_set_daudio_params(struct snd_soc_codec *codec,
+ int width, int rate)
+{
+   int err;
+   u16 digital_io_output_format = \
+   snd_soc_read(codec,
+SI476X_DIGITAL_IO_OUTPUT_FORMAT);
+
+   if ((rate  32000) || (rate  48000)) {
+   dev_dbg(codec-dev, Rate: %d is not supported\n, rate);
+   return -EINVAL;
+   }
+
+   err = snd_soc_write(codec, SI476X_DIGITAL_IO_OUTPUT_SAMPLE_RATE,
+   rate);
+   if (err  0) {
+   dev_err(codec-dev, Failed to set sample rate\n);
+   return err;
+   }
+
+   digital_io_output_format = SI476X_DIGITAL_IO_OUTPUT_WIDTH_MASK;
+   digital_io_output_format |= (width  11) | (width  8);
+
+   return snd_soc_write(codec, SI476X_DIGITAL_IO_OUTPUT_FORMAT,
+digital_io_output_format);
+}
+
+static int si476x_codec_volume_get(struct snd_kcontrol *kcontrol,
+  struct snd_ctl_elem_value *ucontrol)
+{
+   struct snd_soc_codec *codec = snd_kcontrol_chip(kcontrol);
+
+   ucontrol-value.integer.value[0] =
+   snd_soc_read(codec, SI476X_AUDIO_VOLUME);
+   return 0;
+}
+
+static int si476x_codec_volume_put(struct snd_kcontrol *kcontrol,
+