linux-kernel - Re: [PATCH v4 1/3] pinctrl: Add sleep related configuration

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <CAMz4kuLFyw4gGbvXFmPozM2J85kkEXf-UGm0FhwxZWSvvBWKWA@mail.gmail.com>
Date:   Thu, 13 Jul 2017 14:35:23 +0800
From:   Baolin Wang <baolin.wang@...aro.org>
To:     Linus Walleij <linus.walleij@...aro.org>
Cc:     Rob Herring <robh@...nel.org>, Mark Rutland <mark.rutland@....com>,
        "linux-gpio@...r.kernel.org" <linux-gpio@...r.kernel.org>,
        "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Mark Brown <broonie@...nel.org>
Subject: Re: [PATCH v4 1/3] pinctrl: Add sleep related configuration

Hi,

On 12 July 2017 at 20:30, Linus Walleij <linus.walleij@...aro.org> wrote:
> On Tue, Jun 27, 2017 at 2:06 PM, Baolin Wang <baolin.wang@...eadtrum.com> wrote:
>
>> If we introduce "sleep-input-enable" config, we can set the pin's config
>> as below:
>>
>> vio_sd0_ms_3: regctrl3 {
>>         pins = "SC9860_RFCTL30", "SC9860_RFCTL31", "SC9860_RFCTL32";
>>         function = "func1";
>>         sprd,sleep-mode = <0x3>;
>>         sleep-input-enable;
>> };
>
> This looks like a "default" mode. Is that correct?

This can be not default. In some situation, user can change the pins
function and other config.

> I.e. do you set up this on probe then do not touch it?
>
> It seems some of the problems come from the insistance to use a single
> node for all configuration. Compare to this nomadik:
>
>                i2c0 {
>                         i2c0_default_mux: i2c0_mux {
>                                 i2c0_default_mux {
>                                         function = "i2c0";
>                                         groups = "i2c0_a_1";
>                                 };
>                         };
>                         i2c0_default_mode: i2c0_default {
>                                 i2c0_default_cfg {
>                                         pins = "GPIO62_D3", "GPIO63_D2";
>                                         input-enable;
>                                 };
>                         };
>                 };
>
> It is easy to imagine:
>
>                i2c0 {
>                         i2c0_default_mux: i2c0_mux {
>                                 i2c0_default_mux {
>                                         function = "i2c0";
>                                         groups = "i2c0_a_1";
>                                 };
>                         };
>                         i2c0_default_mode: i2c0_default {
>                                 i2c0_default_cfg {
>                                         pins = "GPIO62_D3", "GPIO63_D2";
>                                         input-enable;
>                                 };
>                         };
>                         i2c0_default_mode_sleep: i2c0_default_sleep {
>                                 i2c0_default_cfg {
>                                         pins = "GPIO62_D3", "GPIO63_D2";
>                                         sleep-hardware-state;
>                                         input-disable;
>                                 };
>                         };
>                 };
>
> Notice the new bool property "sleep-hardware-state" that just
> indicate that this should be programmed into the registers for
> the sleep state.

That means we should introduce one "sleep-hardware-state" config.

So my instance can change to be :
grp1: regctrl3 {
        pins = "SC9860_RFCTL30", "SC9860_RFCTL31", "SC9860_RFCTL32";
        function = "func1";
        sprd,sleep-mode = <0x3>;

        grp1_sleep_mode: regctrl3_default_sleep {
                 pins = "SC9860_RFCTL30", "SC9860_RFCTL31", "SC9860_RFCTL32";
                 sleep-hardware-state;
                 input-enable;
       }
};

That sounds reasonable and I will try to check if it can work.

>
>> But If we create one extra "sleep-xxx" state for sleep-related configs,
>> it will be like:
>>
>> grp1: regctrl3 {
>>         pins = "SC9860_RFCTL30", "SC9860_RFCTL31";
>>         function = "func1";
>>         sprd,sleep-mode = <0x3>;
>> };
>>
>> sleep-input: input_grp {
>>         pins = "SC9860_RFCTL30", "SC9860_RFCTL31", "SC9860_RFCTL32";
>>         input-enable;
>> };
>>
>> pinctrl-names = "sleep-input";
>> pinctrl-0 = <&sleep-input>;
>>
>> "sleep-input" state will be selected when initializing pinctrl driver,
>
> The state you should use for initial configuration should be called
> just "init".

Yes.

>
>> "grp1"
>> will be selected by user to set other pin configuration.
>
> Like "default"?
>
>> Then we need config "SC9860_RFCTL30" pin in 2 different places, which is
>> more inconvenient for users.
>
> I'm not so sure about that. Having a lot more sleep,* config options
> may be even more inconvenient for users, and especially for the
> community of developers as a whole.

Make sense. Thanks for your suggestion.

>
> Several config nodes on the other hand, we have had in the pin
> control subsystem since day 1.
>
> Yours,
> Linus Walleij



-- 
Baolin.wang
Best Regards