Re: [PATCH 14/15] drivers/regulators: Enable the ab8500 for Device Tree

[Lee Jones <lee.jones-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>]

On 08/05/12 14:34, Mark Brown wrote:
> On Tue, May 08, 2012 at 01:38:18PM +0100, Lee Jones wrote:
>> On 08/05/12 13:19, Mark Brown wrote:
>
>>> It's been kicking around for review for a little while longer than that
>>> (it was waiting for review while Rhyland was on holiday), and in any
>>> case half the reason for adding infrastructure is to avoid adding
>>> repeated code.
>
>> I'm sorry Mark, but I just don't have the time to read all of the
>> mailing lists in order to keep up with, and in-turn use all of the
>> new features which might make it upstream. I only use what I see in
>> the kernel at time of writing, as I have an entire platform to
>> enable and very little time in which to do it.
>
> If you're going to do this you really need to track -next rather than
> -rc for actively developed subsystems - it's not just that you're not
> using the latest APIs here you're submitting code that won't even
> compile against the current subsystem.

I plan to do that now, but that still wouldn't have helped in this 
instance, as the API you mentioned wasn't in -next at the time.

>> This piece of code plucks pre-defined initialisation values and from
>> the Device Tree and uses them to set-up regulator related registers
>> on the u8500. See 'struct ab8500_regulator_reg_init
>> ab8500_regulator_reg_init' in
>> arch/arm/mach-ux500/board-mop500-regulators.c for reference.
>
> Oh, dear.  It's quite hard to associate this with the code especially
> not without the binding document.

Take a look at: "[PATCH 13/15] ARM: ux500: Add support for ab8500 
regulators into the Device Tree" and compare it with the *.c file and 
I'm sure all will become apparent. It can't be complicated - I wrote it. :D

> Looking at the usage here it looks like most of this stuff shouldn't be
> there even with non-DT stuff, we probably don't want to add DT bindings
> for those bits.All the voltage setting is not at all device specific
> and can be done using the generic regulator bindings, the forcing on or
> off is similarly generic.

All the generic properties _are_ set using the generic bindings. The 
only vendor specific values are the initialisation register values 
referenced above. I'll see what happens when I remove those from DT. I 
have a feeling that the regulators will just fail though.

 > There looks to be a large chunk of mode
> setting too.  We probably need to scrub the existing magic number stuff
> prior to trying to do this.
>
> While looking for the original patch I also noticed that you're not CCing
> the mailing list either...  please always CC the subsystem mailing list
> on patches.

You don't appear to have one. I ran get_maintainer.pl on the patch and 
the only ML it came up with was LKML. If you do have one, you may need 
to update the MAINTAINERS file.

Kind regards,
Lee

-- 
Lee Jones
Linaro ST-Ericsson Landing Team Lead
M: +44 77 88 633 515
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog