linux-kernel - Re: [PATCH 1/4] MFD: twl4030: add twl4030

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20091021231311.GF17796@sortiz.org>
Date:	Thu, 22 Oct 2009 01:13:11 +0200
From:	Samuel Ortiz <sameo@...ux.intel.com>
To:	Peter Ujfalusi <peter.ujfalusi@...ia.com>
Cc:	alsa-devel@...a-project.org, linux-kernel@...r.kernel.org,
	linux-omap@...r.kernel.org, tony@...mide.com,
	broonie@...nsource.wolfsonmicro.com
Subject: Re: [PATCH 1/4] MFD: twl4030: add twl4030_codec MFD as a new child
 to the core

Hi Peter,

On Mon, Oct 19, 2009 at 03:42:17PM +0300, Peter Ujfalusi wrote:
> New MFD child to twl4030 MFD device.
> This MFD device will be used by the drivers, which needs resources
> from the twl4030 codec like audio and vibra.
> 
> The platform specific configuration data is passed along to the
> child drivers (audio, vibra).
Some comments on your code:

> +config TWL4030_CODEC
> +	bool "Support codec part of the TWL4030 family chips"
> +	depends on TWL4030_CORE
> +	help
> +	  Say yes here if you want to use the codec resources on the
> +	  TWL4030 family chips. The codec in TWL4030 provides the audio
> +	  functionality and also has the vibrator controls.
> +
> +	  This driver provides MFD device to be used for drivers, which needs
> +	  access to the codec part, the board-specific data is passed from this
> +	  driver to the childs as platform data with the board specific
> +	  configuration.
> +
As Mark noticed already, you dont really want users to explicitely select this
obscure mfd driver to get their audio and vibre driver selectable. It should
be the other way around, and I think you already agreed with that.


> +static struct device *
> +twl4030_codec_new_child(const char *name, void *pdata, unsigned pdata_len)
> +{
> +	struct platform_device	*pdev;
> +	int			status = 0;
> +
> +	pdev = platform_device_alloc(name, -1);
> +	if (!pdev) {
> +		dev_dbg(&twl4030_codec_dev->dev, "can't alloc dev (%s)\n",
> +									name);
> +		return ERR_PTR(-ENOMEM);
> +	}
> +	pdev->dev.parent = &twl4030_codec_dev->dev;
> +
> +	if (pdata) {
> +		status = platform_device_add_data(pdev, pdata, pdata_len);
> +		if (status < 0) {
> +			dev_dbg(&pdev->dev, "can't add platform_data\n");
> +			goto err;
> +		}
> +	}
> +	status = platform_device_add(pdev);
> +	if (status < 0) {
> +		dev_dbg(&pdev->dev, "Adding platform device failed\n");
> +		goto err;
> +	}
> +
> +	return &pdev->dev;
> +err:
> +	platform_device_put(pdev);
> +	dev_err(&twl4030_codec_dev->dev, "can't add %s dev\n", name);
> +	return ERR_PTR(status);
> +}
This could really use the mfd-core API, and avoid duplicating code.
You just would have to define a couple cells, and call mfd_add_devices on
them.


> +static int __devexit twl4030_codec_remove(struct platform_device *pdev)
> +{
> +	struct twl4030_codec *codec = platform_get_drvdata(pdev);
> +
> +	platform_set_drvdata(pdev, NULL);
> +	kfree(codec);
> +	twl4030_codec_dev = NULL;
> +
> +	return 0;
> +}
I think you're missing a platform_device_unregister() here (or an
mfd_remove_devices() if you're going to switch to the mfd-core API)

Cheers,
Samuel.

-- 
Intel Open Source Technology Centre
http://oss.intel.com/
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/