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

[Mark Brown <broonie-yzvPICuk2AATkU/dhu1WVueM+bqZidxxQQ4Iyu8u01E@public.gmane.org>]

[-- Attachment #1: Type: text/plain, Size: 2871 bytes --]

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.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]