[RFC v2 3/3] ath79: add support for Mikrotik RouterBoard 912G

[Adrian Schmutzler]

Hi,

> -----Original Message-----
> From: openwrt-devel [mailto:openwrt-devel-bounces at lists.openwrt.org]
> On Behalf Of Sergey Ryazanov
> Sent: Montag, 24. Mai 2021 01:07
> To: Adrian Schmutzler <mail at adrianschmutzler.de>; Denis Kalashnikov
> <denis281089 at gmail.com>
> Cc: OpenWrt Development List <openwrt-devel at lists.openwrt.org>; Gabor
> Juhos <juhosg at openwrt.org>; Koen Vandeputte
> <koen.vandeputte at citymesh.com>
> Subject: Re: [RFC v2 3/3] ath79: add support for Mikrotik RouterBoard 912G
> 
> Hello Adrian and Denis,
> 
> sorry for interfering with your conversation, I would like to discuss the best
> way to document not yet finished functionality. Please find my question and
> proposal below.
> 
> On Sun, May 23, 2021 at 12:01 PM Adrian Schmutzler
> <mail at adrianschmutzler.de> wrote:
> 
> [skipped]
> 
> >> +     beeper {
> >> +             status = "disabled";
> >> +             compatible = "gpio-beeper";
> >> +             gpios = <&ssr 5 GPIO_ACTIVE_HIGH>;
> >> +     };
> >
> > If it's broken, I'd not add it here but just name the correct GPIO number in
> the commit message.
> >
> >> +
> >> +     keys {
> >> +             status = "disabled";
> >
> > Like above: If it's broken, remove it, so nobody enables it accidentally and
> causes harm.
> > (But document how to set it up in the commit message, as you mostly
> > did already ...)
> 
> Factoring out realization details to the commit message is quite unusual, I
> personally assume that the commit message is a place where a committer
> should describe reasons for a particular change. While hard to understand
> things should be commented on in the code themself.
> 
> I agree with Adrian that a not yet finished code should not be committed to
> the master branch. But the device tree has a dualistic nature, on one hand it
> is a place for driver configuration, on the other hand it is a way to document
> board stuff interconnections. So maybe we should combine Denise's
> approach to document even a not yet fully supported component in the DTS
> with Adrian's suggestion to document why a component is not supported yet
> and place the reason to disable DTS node as comment inside the node? E.g.:

My main motivation is preventing too much bloat in the DTS files.
Nevertheless, typically, if stuff is not working it will either not be resolved ever or the solution will deviate from what people were thinking it would be initially (otherwise, they would have solved it back then). Thus, the DTS "implementation" of that part is either irrelevant (first case) or wrong/subject to change (second case) later. In both cases, I don't think it really makes sense to add it to the DTS in the first place.

Hovewer, I'm relatively flexible here. So if you really think it would be necessary to have this broken part of configuration in the DTS, I won't stop you because of that.

Best

Adrian

> 
> -------------------------------- 8< --------------------------- keys {
>     compatible = "gpio-keys-polled";
> 
>     reset {
>         ...
>         gpios = <&gpio 15 GPIO_ACTIVE_LOW>;
>         /*
>          * GPIO line #15 is shared between the reset button on input and
>          * the NAND ALE (via the latch) on output. We are unable to just
>          * enable the reset button. So, this node here is for
>          * documentation purposes only.
>          *
>          * [Here should be a text describing the possible ways to
>          * overcome shared line issues]
>          */
>         status = "disabled";
>     };
> };
> -------------------------------- 8< ---------------------------
> 
> This way we will keep all interconnection documentation in DTS and at the
> same time we will make sure that no sane user enables this node.
> 
> > > +             compatible = "gpio-keys";
> > > +             reset {
> > > +                     label = "reset";
> > > +                     linux,code = <KEY_RESTART>;
> > > +                     gpios = <&gpio 15 GPIO_ACTIVE_LOW>;
> > > +                     debounce-interval = <60>;
> > > +             };
> > > +     };
> 
> --
> Sergey
> 
> _______________________________________________
> openwrt-devel mailing list
> openwrt-devel at lists.openwrt.org
> https://lists.openwrt.org/mailman/listinfo/openwrt-devel
-------------- next part --------------
A non-text attachment was scrubbed...
Name: openpgp-digital-signature.asc
Type: application/pgp-signature
Size: 834 bytes
Desc: not available
URL: <http://lists.openwrt.org/pipermail/openwrt-devel/attachments/20210531/04467c0d/attachment.sig>