Return-Path: <linux-kernel-owner@vger.kernel.org>
Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
        id S1756540AbdGLMaa (ORCPT <rfc822;w@1wt.eu>);
        Wed, 12 Jul 2017 08:30:30 -0400
Received: from mail-it0-f41.google.com ([209.85.214.41]:35885 "EHLO
        mail-it0-f41.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
        with ESMTP id S1755296AbdGLMa1 (ORCPT
        <rfc822;linux-kernel@vger.kernel.org>);
        Wed, 12 Jul 2017 08:30:27 -0400
MIME-Version: 1.0
In-Reply-To: <20170627120626.GA1067@spreadtrum.com>
References: <62a67526907157d41235403bd05af309fd4db16b.1498045588.git.baolin.wang@spreadtrum.com>
 <20170626161348.vrtlpat5eof5aa2c@rob-hp-laptop> <20170627082132.GB32447@spreadtrum.com>
 <20170627120626.GA1067@spreadtrum.com>
From: Linus Walleij <linus.walleij@linaro.org>
Date: Wed, 12 Jul 2017 14:30:25 +0200
Message-ID: <CACRpkdapE2+7jR3J0Qw5Hnvg-19-sR5uCYnvMoHiAbowm8-Uug@mail.gmail.com>
Subject: Re: [PATCH v4 1/3] pinctrl: Add sleep related configuration
To: Rob Herring <robh@kernel.org>,
        Linus Walleij <linus.walleij@linaro.org>,
        Mark Rutland <mark.rutland@arm.com>,
        "linux-gpio@vger.kernel.org" <linux-gpio@vger.kernel.org>,
        "devicetree@vger.kernel.org" <devicetree@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        Mark Brown <broonie@kernel.org>, Baolin Wang <baolin.wang@linaro.org>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org
Content-Length: 3463
Lines: 101

On Tue, Jun 27, 2017 at 2:06 PM, Baolin Wang <baolin.wang@spreadtrum.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?

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.

> 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".

> "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.

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

Yours,
Linus Walleij