[RFC PATCH linux dev-4.13 2/3] dts: aspeed-g5: Expose VGA scratch registers

[Joel Stanley]

On 19 April 2018 at 13:30, Andrew Jeffery <andrew at aj.id.au> wrote:
> On Thu, 19 Apr 2018, at 12:57, Joel Stanley wrote:
>> On 12 April 2018 at 13:21, Andrew Jeffery <andrew at aj.id.au> wrote:
>> > Signed-off-by: Andrew Jeffery <andrew at aj.id.au>
>> > ---
>> >  arch/arm/boot/dts/aspeed-g5.dtsi | 48 ++++++++++++++++++++++++++++++++++++++++
>> >  1 file changed, 48 insertions(+)
>> >
>> > diff --git a/arch/arm/boot/dts/aspeed-g5.dtsi b/arch/arm/boot/dts/aspeed-g5.dtsi
>> > index 1468b4ad22dc..8321df50c593 100644
>> > --- a/arch/arm/boot/dts/aspeed-g5.dtsi
>> > +++ b/arch/arm/boot/dts/aspeed-g5.dtsi
>> > @@ -158,6 +158,11 @@
>> >
>> >                                 };
>> >
>> > +                               vga_scratch: scratch at 50 {
>> > +                                       compatible = "aspeed,bmc-misc";
>>
>> Convention is to use aspeed,<socname>-<hardware>, so this would be
>> aspeed,ast2500-bmc-misc.
>
> Yeah, this partly comes back to the naming problem you mentioned in the other patch. The functionality is generic, and so probably shouldn't mention aspeed at all, let alone the SoC revision.
>

I disagree. The bindings need to be hardware specific, but the code
can be generic.

We have a preceidnce for this in the reset controller. There's a
controller called reset-simple that contains generic code (with some
quirks for some systems it supports), and a list of compatible
strings.

On the other side of the equation, the bindings themselves are
specific to each SoC.

If we get push back for being generic we can cite them as our inspiration :)

>>
>>
>> > +                                       reg = <0x50 0x20>;
>> > +                               };
>> > +
>> >                                 hwrng at 78 {
>> >                                         compatible = "timeriomem_rng";
>> >                                         reg = <0x78 0x4>;
>> > @@ -1425,3 +1430,46 @@
>> >                 groups = "WDTRST2";
>> >         };
>> >  };
>> > +
>> > +&vga_scratch {
>> > +       vga0 {
>>
>> Do these names come out in the userspace API?
>
> Yes, though you can override the use of the node name by adding a label property inside the node. That way if someone complains about the node naming we don't have to break userspace.

Similar to the leds bindings, sgtm.

>
>>
>> > +               offset = <0x50>;
>>
>> Would reg <> be more devicetree-y than offset = <>?
>
> There's a mix. I was looking through some other syscon-type stuff and some of those drivers used offset. This approach might avoid militant opinions around the relationship between the unit address and the reg property, as you "can't" have multiple nodes with the same unit address. There's some discussion on this in the PECI threads.
>
>>
>>
>> > +               bit-mask = <0xffffffff>;
>> > +               bit-shift = <0>;
>>
>> Should we assume this is the default? Or something else? Having a
>> default for the common case would be nice.
>
> Whilst the nodes I've added in these examples expose an entire register, I think this would actually be a bit of an unusual case. More likely you'll want to expose a single bit from some register, in which case a default for mask (though maybe 0x1?) or shift isn't going to make any sense.

Ok. If you can think of something that makes the bindings less verbose
I'm all for that.