lkml.org 
[lkml]   [2020]   [Apr]   [27]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    SubjectRe: [PATCH v2 2/3] arm64: dts: rockchip: rk3399-roc-pc: Fix MMC numbering for LED triggers
    From
    Date
    On 4/27/20 12:33 PM, Chen-Yu Tsai wrote:
    > On Mon, Apr 27, 2020 at 6:09 PM Johan Jonker <jbx6244@gmail.com> wrote:
    >>
    >> On 4/27/20 11:17 AM, Chen-Yu Tsai wrote:
    >>> On Mon, Apr 27, 2020 at 4:57 PM Johan Jonker <jbx6244@gmail.com> wrote:
    >>>>
    >>>> Hi Chen-Yu,
    >>>>
    >>>>> From: Chen-Yu Tsai <wens@csie.org>
    >>>>>
    >>>>> With SDIO now enabled, the numbering of the existing MMC host controllers
    >>>>> gets incremented by 1, as the SDIO host is the first one.
    >>>>>
    >>>>> Increment the numbering of the MMC LED triggers to match.
    >>>>>
    >>>>> Fixes: cf3c5397835f ("arm64: dts: rockchip: Enable sdio0 and uart0 on rk3399-roc-pc-mezzanine")
    >>>>> Signed-off-by: Chen-Yu Tsai <wens@csie.org>
    >>>>> ---
    >>>>> arch/arm64/boot/dts/rockchip/rk3399-roc-pc-mezzanine.dts | 8 ++++++++
    >>>>> arch/arm64/boot/dts/rockchip/rk3399-roc-pc.dtsi | 4 ++--
    >>>>> 2 files changed, 10 insertions(+), 2 deletions(-)
    >>>>>
    >>>>> diff --git a/arch/arm64/boot/dts/rockchip/rk3399-roc-pc-mezzanine.dts b/arch/arm64/boot/dts/rockchip/rk3399-roc-pc-mezzanine.dts
    >>>>> index 2acb3d500fb9..f0686fc276be 100644
    >>>>> --- a/arch/arm64/boot/dts/rockchip/rk3399-roc-pc-mezzanine.dts
    >>>>> +++ b/arch/arm64/boot/dts/rockchip/rk3399-roc-pc-mezzanine.dts
    >>>>> @@ -38,6 +38,10 @@ vcc3v3_pcie: vcc3v3-pcie {
    >>>>> };
    >>>>> };
    >>>>>
    >>>>> +&diy_led {
    >>
    >>>>> + linux,default-trigger = "mmc2";
    >>
    >> If you decide to use a free form trigger that is not a 'standard' one,
    >> then it becomes a user property.
    >> User defined properties should not go in a generic dts.
    >> It's up to the user what function he/she gives to that led!
    >
    > The original (in the base .dtsi file for this series of boards) trigger
    > is already a non-standard, i.e. not listed in the bindings, one.
    >
    > Now I would very much like to get rid of user specific stuff, and I
    > also mentioned that in the previous round of discussions. No one said
    > anything.
    >
    >>>>> +};
    >>>>> +
    >>>>> &pcie_phy {
    >>>>> status = "okay";
    >>>>> };
    >>>>> @@ -91,3 +95,7 @@ &uart0 {
    >>>>> pinctrl-0 = <&uart0_xfer &uart0_cts &uart0_rts>;
    >>>>> status = "okay";
    >>>>> };
    >>>>> +
    >>>>> +&yellow_led {
    >>>>> + linux,default-trigger = "mmc1";
    >>>>> +};
    >>>>> diff --git a/arch/arm64/boot/dts/rockchip/rk3399-roc-pc.dtsi b/arch/arm64/boot/dts/rockchip/rk3399-roc-pc.dtsi
    >>>>> index 9f225e9c3d54..bc060ac7972d 100644
    >>>>> --- a/arch/arm64/boot/dts/rockchip/rk3399-roc-pc.dtsi
    >>>>> +++ b/arch/arm64/boot/dts/rockchip/rk3399-roc-pc.dtsi
    >>>>> @@ -70,14 +70,14 @@ work-led {
    >>>>> linux,default-trigger = "heartbeat";
    >>>>> };
    >>>>>
    >>>>
    >>>>> - diy-led {
    >>>>> + diy_led: diy-led {
    >>>>
    >>>> This changes an existing nodename into something that is still not the
    >>
    >> Correction:
    >> It takes an existing nodename and adds a label.
    >
    > OK.
    >
    >>>> preferred way. In the current Rockchip dts there are nodenames like
    >>>> 'work', 'yellow' that causing warnings with the command:
    >>>
    >>> This doesn't change the node name at all. It only adds a label.
    >>> If it doesn't pass the check now, it didn't pass the check before.
    >>>
    >>> I just realized that the footnote I added before is gone because I
    >>> regenerated the patches. The original footnote was something along
    >>> the lines of:
    >>>
    >>> I opted to not change the node names nor the labels as the discussion
    >>> had not concluded. The other reason being that people may have scripts
    >>> or device tree overlays depending on the existing node names.
    >>>
    >>> Previously I asked the following but got no response:
    >>>
    >>> Is changing this after it has been in some kernel releases OK? Wouldn't
    >>> it be considered a break of sysfs ABI?
    >>>
    >>> Also, is there some guideline on how to name the labels? For sunxi we've
    >>> been doing "${vendor}:${color}:${function}" since forever.
    >>>
    >>> As far as I can tell, the hardware vendor [1] has no specific uses for
    >>> these two (red and yellow) LEDs designed in. And their GPIO lines are
    >>> simply labeled "DIY" (for the red one) and "YELLOW". So I'm not sure
    >>> if putting "our" interpretations and the default-trigger into the
    >>> label is wise.
    >>>
    >>> For reference, the green one has its GPIO line labeled "WORK", and their
    >>> intention from [1] is to have it as some sort of power / activity indicator.
    >>> Hence it is named / labeled "work".
    >>>
    >>> As for the node names, I think we can keep it as is for now. It's not
    >>> the preferred form, but there's really no need to change it either.
    >>> And some overlay or script might actually expect that name.
    >>>
    >>>> make -k ARCH=arm dtbs_check
    >>>>
    >>>> Could you give a generic guide line/example, so all these changes are
    >>>> treated the same way? As if the naming follows the preferred 'led-0' line.
    >>>
    >>> I'm not sure what you are asking for.
    >>
    >> Your nodename just happend to contain 'led' to pass the regex.
    >> There are many other names in use.
    >
    > Right. So if it passes, what's the problem?
    >
    >> 'If' the DT maintainer (=Heiko) decides the get rid of all the warnings
    >> for led nodes then that change would require all nodename to be handled
    >> to same (=preferred way):
    >>
    >> Change this:
    >>
    >> diy_led: diy-led
    >> yellow_led: yellow-led
    >>
    >> Into something like:
    >>
    >> led_0: led-0
    >> led_1: led-1
    >
    > As I already said, if the maintainers want to clean this up, I am happy to
    > provide patches towards this. That is not the case right now. Furthermore,
    > that cleanup is not directly related to what I'm trying to fix in this
    > patch, which is, from the original submitter's point of view, incorrect
    > triggers are used when the mezzanine board is added.
    >
    > Also, DT labels "led_0" and "led_1" are useless. They have no relation to
    > what is used in the schematics, which are "work", "diy", and "yellow". The
    > board itself doesn't have anything silk-screened on, so on that end the only
    > thing to go with is the color.
    >
    > So for fixing up the LED node names, we'd probably want the following:
    >
    > diy_led: led-0
    > yellow_led: led-1
    > work_led: led-2

    That doesn't look pretty either.
    Would like to hear the maintainers view on how to handle other cases
    without 'led' like for example 'blue' for mk808.

    >
    > Is that what you're asking for?
    >
    >
    > ChenYu
    >
    >>>
    >>> ChenYu
    >>>
    >>>>> label = "red:diy";
    >>>>> gpios = <&gpio0 RK_PB5 GPIO_ACTIVE_HIGH>;
    >>>>> default-state = "off";
    >>>>> linux,default-trigger = "mmc1";
    >>>>> };
    >>>>>
    >>>>> - yellow-led {
    >>>>> + yellow_led: yellow-led {
    >>>>> label = "yellow:yellow-led";
    >>>>> gpios = <&gpio0 RK_PA2 GPIO_ACTIVE_HIGH>;
    >>>>> default-state = "off";
    >>>>> --
    >>>>> 2.26.0
    >>>>
    >>

    \
     
     \ /
      Last update: 2020-04-27 12:57    [W:5.708 / U:0.012 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site