On Mon, Jul 29, 2013 at 10:59:26PM +0100, Stephen Warren wrote:
> (Also Cc'ing the DT binding maintainers and hence quoting this in full)
Cheers.
>
> On 07/27/2013 07:23 AM, Marc Dietrich wrote:
> > Hi,
> >
> > this is an RFC for an mfd device introduced here [1]. I didn't got much
> > response, except from Stephen, but that bothered me enough already ;-)
> >
> > I'm going describe the hw again shortly, for a more complete description, see
> > [1]. What we have here is an EC which communicates to the host cpu (NVIDIA
> > Tegra) via I2C. The unusual part is that the EC is I2C master while the host
> > cpu is the slave. The EC has several ports to connect peripherals like
> > keyboard, mouse, ... and also to control some system states (suspend, low bat,
> > power key, ...). Ok, so much for the hardware description, now the binding. I
> > think I integrated all comments made by Stephen in [2].
> >
> > i2c-slave@7000c500 {
> > compatible = "nvidia,tegra20-i2c-slave", "simple-bus";
>
> Thinking more about this, I don't think simple-bus is appropriate here,
> for the same reasons that i2c-master bindings don't specify simple-bus here.
>
> "simple-bus" is usually used in a case where the node in question is
> pretty much just a simple aggregation of some child nodes, and the child
> nodes are the same "type" of node as the node that is the simple-bus.
> For example, when grouping a variety of memory-mapped peripherals
> together to provide some structure to a larger memory-mapped bus,
> perhaps with a "ranges" property etc. to do some remapping. In this
> case, the child nodes are basically independent of the simple-bus node,
> just relying on it for a bit of DT parsing, but not at run-time.
I'd expect any simple-bus node to be completely ignorable - if it
*requires* any configuration to child nodes to be usable, or child node
require any information from the bus (other than some addresss
translation via the ranges property), then the node is clearly not
compatible with the simple-bus concept.
If you need to instantiate child nodes as Linux platform devices, this
should be done in the driver for the bus. Relying on simple-bus to do
this is a hack.
>
> In this case, the child nodes are a different type of object, and we
> really need the driver for the i2c-slave node to know how to handle the
> child nodes; we can't just go an instantiate them
> directly/independently, and plug the devices into the bus that contains
> the i2c-slave.
+1
>
> One question I have here: There is a single I2C HW module on Tegra that
> can act either as a master or a slave. I don't think it can do both at
> once. Should we:
Is that configurable at run-time, or an integration decision?
>
> a) Have a different compatible property for each use-case
>
> b) Use the same compatible property (after all, the same HW module is
> there in either case), yet have some kind of flag which tells the driver
> whether to act as a master or slave. This flag could be standardized and
> used by any binding for a "dual role" I2C controller.
>
> I think it's reasonable to go with a separate compatible value in this
> case, but I wonder if anyone wants to argue against it?
I believe a separate compatible string would make sense.
Thanks,
Mark.
>
> > reg = <0x7000c500 0x100>;
> > interrupts = <0 92 0x04>;
> > #address-cells = <1>;
> > #size-cells = <0>;
> > clock-frequency = <80000>;
> > clocks = <&tegra_car 67>, <&tegra_car 124>;
> > clock-names = "div-clk", "fast-clk";
> >
> > nvec@87 {
> > compatible = "nvidia,nvec", "simple-bus";
>
> Similarly here, I would drop simple-bus.
>
> > reg = <0x87>;
> > request-gpios = <&gpio 170 0>; /* gpio PV2 */
>
> Nit: We should s/170/TEGRA_GPIO(V, 2)/ in the final *.dts file, and then
> we can drop that comment.
>
> > keyboard {
> > compatible = "nvidia,nvec-keyboard";
> > ...
> > <insert key mappings here>
> > ...
> > };
> >
> > ps2 {
> > compatible = "nvidia,nvec-ps2";
> > packet-size = 6; /* for the ac100 tp */
> > };
> >
> > events {
> > compatible = "nvidia,nvec-events";
> > ...
> > <add some system events here,
> > e.g. lid switch, power button>
> > ...
> > };
> >
> > charger: ac {
> > compatible = "nvidia,nvec-ac";
> > };
> >
> > battery {
> > compatible = "nvidia,nvec-battery";
> > charger = <&charger>;
> > };
> >
> > oem {
> > compatible = "compal,nvec-paz00";
> > /* ac100 specific extentions to the protocol */
> > };
> > };
> > };
>
> To me, this seems like a reasonable binding.
>
> > For multi-slave capable SoCs, there can be more than one master connected to
> > the i2c slave controller of the host cpu. Therefore I added "simple-bus"
> > property here. Does "simple-bus" imply bus addresses?
>
> This binding doesn't describe the external masters, so I'm not sure how
> the presence or lack of them affects the binding at all. I argue against
> using simple-bus above; the bus is not "simple".
>
> > The EC registers with
> > the slave controller by supplying the protocol (e.g. smbus or i2c) and an
> > optional gpio.
>
> I don't think "the EC registers ...", but rather "the driver for the EC
> protocol registers ..."
I've lost some context here, but I hope this isn't a binding describing
driver behaviour... :)
Thanks,
Mark.
--
To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
