Subject: Re: Da903x regulator driver. Bug?
From: Liam Girdwood <lrg@kernel.org>
To: Jonathan Cameron <Jonathan.Cameron@gmail.com>
Cc: Jonathan Cameron <jic23@cam.ac.uk>, LKML <linux-kernel@vger.kernel.org>,
       eric miao <eric.y.miao@gmail.com>,
       Liam Girdwood <lg@opensource.wolfsonmicro.com>,
       Mark Brown <broonie@sirena.org.uk>
In-Reply-To: <490204A3.6010505@gmail.com>
References: <4901EE0F.5000508@cam.ac.uk>  <490204A3.6010505@gmail.com>
Content-Type: text/plain
Date: Fri, 24 Oct 2008 19:24:25 +0100
Message-Id: <1224872665.28382.73.camel@dell-desktop.example.com>
Mime-Version: 1.0
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 2768
Lines: 93

On Fri, 2008-10-24 at 18:23 +0100, Jonathan Cameron wrote:
> Jonathan Cameron wrote:
> > Firstly, is lkml the right place to ask questions about regulator drivers?

Yes.

> >
> > Secondly, though I can't track down any examples, I'm guessing the following
> > is a valid board config for the da903x reg etc.
> >

I'm not sure if this is a valid config for this board. Eric will
probably know for sure. 

> > static struct regulator_init_data stargate2_ld8_init_data = {
> > 	.supply_regulator_dev = NULL,
> > 	.constraints = {
> > 		.name = "vdd_mica",
> > 		.min_uV = 1800000,
> > 		.max_uV = 1900000,
> > 		.valid_modes_mask = REGULATOR_CHANGE_VOLTAGE,
> > 	},
> > };
> >
> > /* playing with this ld0 as it only goes to an external connector */
> > static struct da903x_subdev_info stargate2_da9030_subdevs[] = {
> > 	{
> > 		.name = "da903x-regulator",
> > 		.id = DA9030_ID_LDO8,
> > 		.platform_data = &stargate2_ld8_init_data,
> > 	},
> > };
> >
> > static struct da903x_platform_data stargate2_da9030_pdata = {
> > 	.num_subdevs = ARRAY_SIZE(stargate2_da9030_subdevs),
> > 	.subdevs = stargate2_da9030_subdevs,
> > };
> > static struct i2c_board_info __initdata stargate2_pwr_i2c_board_info [] = {
> > 	{
> > 		.type = "da9030",
> > 		.addr = 0x49,
> > 		.platform_data = &stargate2_da9030_pdata,
> > 		.irq = gpio_to_irq(1),
> > 	},
> > };
> >
> > // and relevant registration code.
> >
> >
> > Now if this is now things are expected to be, there is a bug in
> > regulators/da903x.c in da903x_regulator_probe
> >
> > rdev = regulator_register(&ri->desc, pdev->dev.parent, ri);
> >
> > should be
> >
> > rdev = regulator_register(&ri->desc, &pdev->dev, ri);

wm8350 and wm8400 (other mfd regulators) both register using the bottom
case. 

> >
> >   
> Unfortunately this fix causes other issues as now the i2c_client
> is 2 layers down rather than one requiring quite a few changes
> to   
>  struct device *da9034_dev = rdev_get_dev(rdev)->parent->parent;
> from
>     struct device *da9034_dev = rdev_get_dev(rdev)->parent;
> 
> So either a change to the regulator framework is needed to
> allow mfd's or these extra ->parent lines need to go in in lots
> of places.
> 
> Which do people prefer?
> 

Could you fix in a similar method to the wm8350/wm8400. 

I would also move the da903x_regulator_info lookup into each regulator
function, rather than at probe(). This would free up the registration
private data. da903x_regulator_info is an array so we should be able to
use regulator->id as the index.  

Liam


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