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-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/