linux-arm-kernel.lists.infradead.org archive mirror
 help / color / mirror / Atom feed
From: broonie@opensource.wolfsonmicro.com (Mark Brown)
To: linux-arm-kernel@lists.infradead.org
Subject: [PATCH] of: add devicetree API for regulator
Date: Sat, 9 Jul 2011 11:03:25 +0900	[thread overview]
Message-ID: <20110709020323.GH18860@opensource.wolfsonmicro.com> (raw)
In-Reply-To: <1310120428-22700-8-git-send-email-haojian.zhuang@marvell.com>

On Fri, Jul 08, 2011 at 06:20:24PM +0800, Haojian Zhuang wrote:
> Signed-off-by: Haojian Zhuang <haojian.zhuang@marvell.com>

Overall this looks like a reasonable start but I'm having a hard time
seeing how this would actually be used and there's a *lot* of Linux
specifics in here which aren't going to be acceptable for device tree as
device tree is supposed to be platform neutral.

As a general comment blank lines would *really* improve the legibility
of the code.

> ---
>  drivers/of/Kconfig           |    4 +
>  drivers/of/Makefile          |    1 +
>  drivers/of/of_regulator.c    |  166 ++++++++++++++++++++++++++++++++++++++++++

Why is this in drivers/of?

> +	p = of_get_property(of_dev, "voltages", &size);

voltage_range would probably be better.

> +	if (p && size / sizeof(int) == 2) {
> +		constraints->min_uV = be32_to_cpu(*p++);
> +		constraints->max_uV = be32_to_cpu(*p);
> +	}

This and pretty much all of the rest of the code doesn't look like it's
going to complain about parse problems.  That seems wrong, we'll just
silently ignore things we think we should understand.

> +	p = of_get_property(of_dev, "modes-mask", NULL);
> +	if (p)
> +		constraints->valid_modes_mask = be32_to_cpu(*p);

This probably shouldn't be in the OF bindings at all, the modes are a
Linux specific property.  We probably shouldn't be putting random
bitmasks directly in OF, and certainly not Linux defined ones.  This
applies to an awful lot of the rest of the code too.

> +	cp = of_get_property(of_dev, "ops-mask", &size);
> +	tmp = 0;
> +	if (cp && size > 0) {
> +		i = 0;
> +		do {
> +			len = strlen(ops[i]);
> +			if (!strncmp(cp, ops[i], len)) {
> +				constraints->valid_ops_mask |= 1 << i;
> +				/* need to handle '\0' */
> +				cp += len + 1;
> +				size = size - len - 1;
> +				i = 0;
> +			} else
> +				i++;
> +		} while (i < ARRAY_SIZE(ops));
> +		if (size > 0)
> +			printk(KERN_WARNING "Invalid string:%s\n", cp);
> +	}

If there isn't a helper function for this there should be one.

> +	supply = kzalloc(sizeof(struct regulator_consumer_supply) * count,
> +		GFP_KERNEL);
> +	if (supply == NULL)
> +		return -EINVAL;
> +	str = p;
> +	calc_size = size;
> +	i = 0;
> +	while (str && calc_size > 0 && i < count) {
> +		len = strlen(str);
> +		if (len == 0)
> +			break;
> +		supply[i++].supply = str;
> +		calc_size = calc_size - len - 1;
> +		str += len + 1;
> +	}
> +	data->consumer_supplies = supply;
> +	data->num_consumer_supplies = count;

This isn't going to fly, the consumer devices need to be referenced in
the OF tree rather than via their Linux device names.

> +extern int of_regulator_init_data(struct device_node *of_node,
> +				struct regulator_init_data *data);
> +extern void of_regulator_deinit_data(struct regulator_init_data *data);

How exactly is this supposed to be used?  I'd really expect the
regulator core to be doing this transparently for the drivers.

  parent reply	other threads:[~2011-07-09  2:03 UTC|newest]

Thread overview: 29+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2011-07-08 10:20 [PATCH] ARM: mmp: remove SPARSE_IRQ for mmp Haojian Zhuang
2011-07-08 10:20 ` [PATCH] ARM: mmp: remove builtin gpio driver support Haojian Zhuang
2011-07-08 10:20   ` [PATCH] ARM: mmp: parse irq from DT Haojian Zhuang
2011-07-08 10:20     ` [PATCH] ARM: mmp: support OF by default Haojian Zhuang
2011-07-08 10:20       ` [PATCH] tty: serial: support device tree in pxa Haojian Zhuang
2011-07-08 10:20         ` [PATCH] tty: serial: check ops before registering console Haojian Zhuang
2011-07-08 10:20           ` [PATCH] i2c: pxa: create dynamic platform device from device tree Haojian Zhuang
2011-07-08 10:20             ` [PATCH] of: add devicetree API for regulator Haojian Zhuang
2011-07-08 10:20               ` [PATCH] regulator: convert devicetree to platform data on max8649 Haojian Zhuang
2011-07-08 10:20                 ` [PATCH] mfd: convert devicetree to platform data on max8925 Haojian Zhuang
2011-07-08 10:20                   ` [PATCH] mfd: convert devicetree to platform on 88pm860x Haojian Zhuang
2011-07-08 10:20                     ` [PATCH] ARM: mmp: add DTS file Haojian Zhuang
2011-07-10  7:35                       ` Grant Likely
2011-07-11  5:21                         ` Haojian Zhuang
2011-07-10  7:21                     ` [PATCH] mfd: convert devicetree to platform on 88pm860x Grant Likely
2011-07-10  7:20                   ` [PATCH] mfd: convert devicetree to platform data on max8925 Grant Likely
2011-07-10  9:17                     ` Mark Brown
2011-07-10 10:40                       ` Grant Likely
2011-07-08 14:51               ` [PATCH] of: add devicetree API for regulator Grant Likely
2011-07-09  1:14                 ` Mark Brown
2011-07-08 18:32               ` Liam Girdwood
2011-07-09  2:03               ` Mark Brown [this message]
2011-07-10  5:26             ` [PATCH] i2c: pxa: create dynamic platform device from device tree Grant Likely
2011-07-10  5:11         ` [PATCH] tty: serial: support device tree in pxa Grant Likely
2011-07-10  4:34     ` [PATCH] ARM: mmp: parse irq from DT Grant Likely
2011-07-10  4:02   ` [PATCH] ARM: mmp: remove builtin gpio driver support Grant Likely
2011-07-11  5:05     ` Haojian Zhuang
2011-07-11 11:44       ` Eric Miao
2011-07-08 14:46 ` [PATCH] ARM: mmp: remove SPARSE_IRQ for mmp Grant Likely

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20110709020323.GH18860@opensource.wolfsonmicro.com \
    --to=broonie@opensource.wolfsonmicro.com \
    --cc=linux-arm-kernel@lists.infradead.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).