linux-kernel - Re: [PATCH 3/4] mfd: tps65910: Add device-tree support

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: <20120418090133.GC3021@opensource.wolfsonmicro.com>
Date:	Wed, 18 Apr 2012 10:01:34 +0100
From:	Mark Brown <broonie@...nsource.wolfsonmicro.com>
To:	Rhyland Klein <rklein@...dia.com>
Cc:	Liam Girdwood <lrg@...com>,
	Grant Likely <grant.likely@...retlab.ca>,
	Rob Herring <rob.herring@...xeda.com>,
	Samuel Ortiz <sameo@...ux.intel.com>,
	devicetree-discuss@...ts.ozlabs.org, linux-kernel@...r.kernel.org,
	linux-tegra@...r.kernel.org
Subject: Re: [PATCH 3/4] mfd: tps65910: Add device-tree support

On Tue, Apr 17, 2012 at 06:00:28PM -0700, Rhyland Klein wrote:
> Add device tree based initialization support for TI's tps65910 pmic.

Actually, now I look at the larger patch this probably wants to be split
up by driver and possibly split further within that.

> +	board_data = tps65910->board_data;
> +	if (board_data->use_dt_for_init_data && tps65910->dev->of_node)
> +		ret = tps65910_gpio_parse_dt(tps65910->dev, board_data);
> +

This is a really odd idiom - normally the pattern for device tree
support is to just go and try to use the device tree data if it's there
and there's no need for any flag to say if it should be used.

> +	if (pdata->irq_base <= 0)
> +		pdata->irq_base = irq_alloc_descs(-1, 0, tps65910->irq_num, -1);
> +
> +	if (pdata->irq_base <= 0) {
> +		dev_err(tps65910->dev, "Failed to allocate irq descs: %d\n",
> +			pdata->irq_base);
> +		return pdata->irq_base;
> +	}
> +
> +	tps65910->irq_mask = 0xFFFFFF;
> +
> +	mutex_init(&tps65910->irq_lock);
> +	tps65910->chip_irq = irq;
> +	tps65910->irq_base = pdata->irq_base;

While this is needed for DT support it can be done separately and would
probably be better split out into a separate patch.

> +	/* Pass of data to child devices */
> +	for (idx = 0; idx < ARRAY_SIZE(tps65910s); idx++) {
> +		tps65910s[idx].platform_data = pmic_plat_data;
> +		tps65910s[idx].pdata_size = sizeof(*pmic_plat_data);
> +	}

Why is this needed - can't the DT parsing just be moved where it's used?

> +	for_each_child_of_node(regulators, child) {
> +		struct regulator_init_data *init_data;
> +
> +		init_data = of_get_regulator_init_data(&pdev->dev, child);
> +		if (!init_data) {
> +			dev_err(&pdev->dev,
> +				"failed to parse DT for regulator %s\n",
> +				child->name);
> +			return -EINVAL;
> +		}
> +
> +		for (idx = 0; idx < pmic->num_regulators; idx++) {

Hrm, this iteration over a group of regulators to find the relevant
node by name is going to be a fairly common pattern (there's already
at least one driver doing this IIRC) - we should really factor it out
into common code.  Please consider doing this when you resubmit.

> +			if (!strcasecmp(info[idx].name, child->name)) {
> +				if (all_data[idx]) {
> +					dev_err(&pdev->dev,
> +						"Duplicate Regulator Node %s\n",

Please fix the capitalisation in the error message.

> +		/* Check to see if we iterated without finding its name */
> +		if (idx == pmic->num_regulators) {
> +			dev_err(&pdev->dev,
> +				"Unknown regulator node found [%s]\n",
> +				child->name);
> +			return -EINVAL;
> +		}

It'd seem more robust to only print the warning and not return the
error, that way we don't completely fail the device initialisation if
there's data we don't understand.

I'm also not seeing a change here that passes the DT node to
regulator_register() - you should be doing that, it's needed so
consumers can bind to the regulator.

Download attachment "signature.asc" of type "application/pgp-signature" (837 bytes)