Re: [PATCH] regulator: Support different config and dev of_nodes in regulator_register

[Bjorn Andersson]

On Thu 05 Feb 11:27 PST 2015, Mark Brown wrote:

> On Thu, Feb 05, 2015 at 10:37:12AM -0800, Tim Bird wrote:
> > On 02/05/2015 09:43 AM, Mark Brown wrote:
> 
> > >> Sorry - what is the "Linux MFD structure"?
> 
> > > The way we split things up into subsystems via drivers/mfd.  Our set of
> > > subsystems is neither fixed nor authorative.
> 
> > I'm not doing anything in drivers/mfd?  Should I be?
> > The charger driver is currently in drivers/power, but should it be
> > moved to drivers/mfd if it's going to expose regulators as
> > well as power supplies?  I'm sorry, but I'm not following
> > your point here.  I associated this regulator with the charger
> 
> Possibly not.  My reply was explaining the sorts of breakage that
> allowing the DT node to be overridden is usually intended to support,
> the sort of thoughtless bindings for MFDs that I'm describing is one
> typical example.
> 

The charging block have 1 bit that serves as a vbus-switch, when the USB
code detects that we're in host mode and should supply power on VBUS
we need to enable the switch.

This is why we suggested Tim to implement this as a regulator from the
charger driver itself.

> > So you're saying I should have a "regulators" child node of the charger
> > node, and then define the chg_otg and boost regulators under that, each
> > with it's own compatible string, so that the DT code can instantiate
> 
> No, absolutely not - you should not need to put compatible strings for
> individual regulators within a single device in the device tree.  Please
> take a look at how other devices do this - there are plenty of bindings
> for existing devices in tree with matching code in the kernel.
> 

As Tim mentioned, we have (with different naming):

charger {
	compatible = "awesome,charger";
	vbus: vbus {
		vbus-supply = <&upstream_switch>;
	};
};

vbus is not a separate device in any form, it's just one aspect of the
block exposed through a different subsystem (than power). The vbus node
groups regulator properties related to the vbus regulator (and gives us
a possibility to reference it).

Your statement and the code indicates that it is okay to register a
regulator with of_node being different from the device's of_node itself.


However this only works for the non-supply regulator properties - and
this is where Tim's patch is trying to sort out.

I interpreted your first answer as it being incorrect usage to have
dev->of_node != config->of_node, but I'm not so sure anymore.

If it is invalid then we should drop config->of_node and use
dev->of_node in regulator_register, if it's valid we should have all the
standard regulator properties read from the same of_node (i.e. fix
supply processing in regulator_register).

(I have some style comments on the original patch, but would like to
know if we're going the wrong way here or not)

> > Or is this instantiation something I do manually in the charger probe
> > routine?  (That's what I'm doing now, but open coding each regulator
> > individually.)
> 
> Given the way you're talking separately about there being a charger
> driver and a regulator driver here it sounds like you should be creating
> a MFD.  The MFD subsystem exists to provide a way of mapping a single
> physical device into multiple kernel subsystems which sounds like it
> will be what you're trying to do here.
> 

It's a charger block that contains a switch for powering the VBUS line,
it's one hardware block.

> > Can you recommend a driver to look at that does (properly) what
> > you're describing?
> 
> Most of drivers/mfd is PMICs doing this, anything recently added should
> be reasonable to look at.  Most of the Maxim or Wolfson devices for
> example.

Correct, but those are separate hardware blocks (although same chip),
that's not what we have here.

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