Re: [PATCH v6 06/12] mfd: wm97xx-core: core support for wm97xx Codec

On Sat, 02 Sep 2017, Robert Jarzmik wrote:

The WM9705, WM9712 and WM9713 are highly integrated codecs, with an
audio codec, DAC and ADC, GPIO unit and a touchscreen interface.

Historically the support was spread across drivers/input/touchscreen and
sound/soc/codecs. The sharing was done through ac97 bus sharing. This
model will not withstand the new AC97 bus model, where codecs are
discovered on runtime.

Signed-off-by: Robert Jarzmik <robert.jarzmik@free.fr>
Acked-by: Charles Keepax <redacted>
---
Since v3:
 - added a "depends on AC97_BUS_NEW" Kconfig statement
 - added default values for wm9705, wm9712 per Charles's comment
Since v4:
 - added Charles's ack
Since v5:
 - took into account Lee's comments
---
 drivers/mfd/Kconfig        |  14 ++
 drivers/mfd/Makefile       |   1 +
 drivers/mfd/wm97xx-core.c  | 379 +++++++++++++++++++++++++++++++++++++++++++++
 include/linux/mfd/wm97xx.h |  25 +++
 4 files changed, 419 insertions(+)
 create mode 100644 drivers/mfd/wm97xx-core.c
 create mode 100644 include/linux/mfd/wm97xx.h

[...]

+static struct mfd_cell wm9705_cells[] = {
+	{
+		.name = "wm9705-codec",
+	},
+	{
+		.name = "wm97xx-ts",
+	},
+};

Nit: These should be one line entries:

     {	   .name = "wm9705-codec"	},
     { 	   .name = "wm97xx-ts"		},

[...]

+static struct mfd_cell wm9712_cells[] = {
+	{
+		.name = "wm9712-codec",
+	},
+	{
+		.name = "wm97xx-ts",
+	},
+};

As above.

[...]

+static struct mfd_cell wm9713_cells[] = {
+	{
+		.name = "wm9713-codec",
+	},
+	{
+		.name = "wm97xx-ts",
+	},
+};

And again.

+static int wm97xx_ac97_probe(struct ac97_codec_device *adev)
+{
+	struct wm97xx_priv *wm97xx;
+	const struct regmap_config *config;
+	struct wm97xx_platform_data *codec_pdata;
+	struct mfd_cell *cells;
+	int ret = 0, nb_cells, i;
+	struct wm97xx_pdata *pdata = snd_ac97_codec_get_platdata(adev);
+
+	wm97xx = devm_kzalloc(ac97_codec_dev2dev(adev),
+			      sizeof(*wm97xx), GFP_KERNEL);
+	if (!wm97xx)
+		return -ENOMEM;
+
+	wm97xx->dev = ac97_codec_dev2dev(adev);
+	wm97xx->ac97 = snd_ac97_compat_alloc(adev);
+	if (IS_ERR(wm97xx->ac97))
+		return PTR_ERR(wm97xx->ac97);
+
+
+	ac97_set_drvdata(adev, wm97xx);
+	dev_info(wm97xx->dev, "wm97xx core found, id=0x%x\n",
+		 adev->vendor_id);
+
+	codec_pdata = &wm97xx->codec_pdata;
+	codec_pdata->ac97 = wm97xx->ac97;
+	codec_pdata->batt_pdata = pdata->batt_pdata;
+
+	switch (adev->vendor_id) {
+	case WM9705_VENDOR_ID:
+		config = &wm9705_regmap_config;
+		cells = wm9705_cells;
+		nb_cells = ARRAY_SIZE(wm9705_cells);
+		break;
+	case WM9712_VENDOR_ID:
+		config = &wm9712_regmap_config;
+		cells = wm9712_cells;
+		nb_cells = ARRAY_SIZE(wm9712_cells);
+		break;
+	case WM9713_VENDOR_ID:
+		config = &wm9713_regmap_config;
+		cells = wm9713_cells;
+		nb_cells = ARRAY_SIZE(wm9713_cells);
+		break;
+	default:
+		config = NULL;

goto an error label here.

+	}
+
+	for (i = 0; i < nb_cells; i++) {
+		cells[i].platform_data = codec_pdata;
+		cells[i].pdata_size = sizeof(*codec_pdata);
+	}
+
+	if (config) {
+		codec_pdata->regmap =
+			devm_regmap_init_ac97(wm97xx->ac97, config);
+		if (IS_ERR(codec_pdata->regmap))
+			ret = PTR_ERR(codec_pdata->regmap);

Remove the if and do this regardless.

+	} else {
+		ret = -ENODEV;
+	}

remove this.

+	if (!ret)
+		ret = devm_mfd_add_devices(wm97xx->dev, PLATFORM_DEVID_NONE,
+					   cells, nb_cells, NULL, 0, NULL);

Remove the if and do this regardless.

No need to allocate to a variable.  Just return the result directly.

+	if (ret)
+		snd_ac97_compat_release(wm97xx->ac97);

Place this in the error path, under the goto label.

+	return ret;
+}

Once fixed, please apply my:

For my own reference:
  Acked-for-MFD-by: Lee Jones [off-list ref]

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog