* [PATCH v2 0/4] Add drive-strength in Meson pinctrl driver @ 2019-04-18 12:47 Guillaume La Roque 2019-04-18 12:47 ` [PATCH v2 1/4] dt-bindings: pinctrl: add a 'drive-strength-uA' property Guillaume La Roque ` (4 more replies) 0 siblings, 5 replies; 18+ messages in thread From: Guillaume La Roque @ 2019-04-18 12:47 UTC (permalink / raw) To: linus.walleij, robh+dt, mark.rutland, khilman Cc: linux-gpio, linux-amlogic, linux-kernel, devicetree The purpose of this patchset is to add drive-strength support in meson pinconf driver. This is a new feature that was added on the g12a. It is critical for us to support this since many functions are failing with default pad drive-strength. The value achievable by the SoC are 0.5mA, 2.5mA, 3mA and 4mA and the DT property 'drive-strength' is expressed in mA. So this patch add another generic property "drive-strength-uA". The change to do so would be minimal and could be benefit to other platforms later on. Cheers Guillaume Changes since v1: - fix missing break - implement new pinctrl generic property "drive-strength-uA" [1] https://lkml.kernel.org/r/20190314163725.7918-1-jbrunet@baylibre.com Guillaume La Roque (4): dt-bindings: pinctrl: add a 'drive-strength-uA' property pinctrl: generic: add new 'drive-strength-uA' property support dt-bindings: pinctrl: meson: Add drive-strength-uA property pinctrl: meson: add support of drive-strength-uA .../bindings/pinctrl/meson,pinctrl.txt | 3 + .../bindings/pinctrl/pinctrl-bindings.txt | 3 + drivers/pinctrl/meson/pinctrl-meson-g12a.c | 36 ++-- drivers/pinctrl/meson/pinctrl-meson.c | 166 ++++++++++++++---- drivers/pinctrl/meson/pinctrl-meson.h | 20 ++- drivers/pinctrl/pinconf-generic.c | 2 + include/linux/pinctrl/pinconf-generic.h | 3 + 7 files changed, 174 insertions(+), 59 deletions(-) -- 2.17.1 _______________________________________________ linux-amlogic mailing list linux-amlogic@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-amlogic ^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH v2 1/4] dt-bindings: pinctrl: add a 'drive-strength-uA' property 2019-04-18 12:47 [PATCH v2 0/4] Add drive-strength in Meson pinctrl driver Guillaume La Roque @ 2019-04-18 12:47 ` Guillaume La Roque 2019-04-23 11:13 ` Linus Walleij ` (3 more replies) 2019-04-18 12:47 ` [PATCH v2 2/4] pinctrl: generic: add new 'drive-strength-uA' property support Guillaume La Roque ` (3 subsequent siblings) 4 siblings, 4 replies; 18+ messages in thread From: Guillaume La Roque @ 2019-04-18 12:47 UTC (permalink / raw) To: linus.walleij, robh+dt, mark.rutland, khilman Cc: linux-gpio, linux-amlogic, linux-kernel, devicetree This property allow drive-strength parameter in uA instead of mA. Signed-off-by: Guillaume La Roque <glaroque@baylibre.com> --- Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt | 3 +++ 1 file changed, 3 insertions(+) diff --git a/Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt b/Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt index cef2b5855d60..fc7018459aa2 100644 --- a/Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt +++ b/Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt @@ -258,6 +258,7 @@ drive-push-pull - drive actively high and low drive-open-drain - drive with open drain drive-open-source - drive with open source drive-strength - sink or source at most X mA +drive-strength-uA - sink or source at most X uA input-enable - enable input on pin (no effect on output, such as enabling an input buffer) input-disable - disable input on pin (no effect on output, such as @@ -326,6 +327,8 @@ arguments are described below. - drive-strength takes as argument the target strength in mA. +- drive-strength-uA takes as argument the target strength in uA. + - input-debounce takes the debounce time in usec as argument or 0 to disable debouncing -- 2.17.1 _______________________________________________ linux-amlogic mailing list linux-amlogic@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-amlogic ^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH v2 1/4] dt-bindings: pinctrl: add a 'drive-strength-uA' property 2019-04-18 12:47 ` [PATCH v2 1/4] dt-bindings: pinctrl: add a 'drive-strength-uA' property Guillaume La Roque @ 2019-04-23 11:13 ` Linus Walleij 2019-04-27 19:19 ` Martin Blumenstingl ` (2 subsequent siblings) 3 siblings, 0 replies; 18+ messages in thread From: Linus Walleij @ 2019-04-23 11:13 UTC (permalink / raw) To: Guillaume La Roque Cc: Mark Rutland, open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, Kevin Hilman, linux-kernel, open list:GPIO SUBSYSTEM, Rob Herring, open list:ARM/Amlogic Meson... On Thu, Apr 18, 2019 at 2:48 PM Guillaume La Roque <glaroque@baylibre.com> wrote: > This property allow drive-strength parameter in uA instead of mA. > > Signed-off-by: Guillaume La Roque <glaroque@baylibre.com> I'm fine with this but I need the DT maintainers to ACK it explicitly. Yours, Linus Walleij _______________________________________________ linux-amlogic mailing list linux-amlogic@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-amlogic ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v2 1/4] dt-bindings: pinctrl: add a 'drive-strength-uA' property 2019-04-18 12:47 ` [PATCH v2 1/4] dt-bindings: pinctrl: add a 'drive-strength-uA' property Guillaume La Roque 2019-04-23 11:13 ` Linus Walleij @ 2019-04-27 19:19 ` Martin Blumenstingl 2019-04-29 19:19 ` Kevin Hilman 2019-04-30 15:12 ` Rob Herring 3 siblings, 0 replies; 18+ messages in thread From: Martin Blumenstingl @ 2019-04-27 19:19 UTC (permalink / raw) To: Guillaume La Roque Cc: mark.rutland, devicetree, khilman, linus.walleij, linux-kernel, linux-gpio, robh+dt, linux-amlogic On Thu, Apr 18, 2019 at 2:48 PM Guillaume La Roque <glaroque@baylibre.com> wrote: > > This property allow drive-strength parameter in uA instead of mA. > > Signed-off-by: Guillaume La Roque <glaroque@baylibre.com> personally I'm happy with this if the DT maintainers give their ACK. based on that: Acked-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com> _______________________________________________ linux-amlogic mailing list linux-amlogic@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-amlogic ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v2 1/4] dt-bindings: pinctrl: add a 'drive-strength-uA' property 2019-04-18 12:47 ` [PATCH v2 1/4] dt-bindings: pinctrl: add a 'drive-strength-uA' property Guillaume La Roque 2019-04-23 11:13 ` Linus Walleij 2019-04-27 19:19 ` Martin Blumenstingl @ 2019-04-29 19:19 ` Kevin Hilman 2019-04-30 15:12 ` Rob Herring 3 siblings, 0 replies; 18+ messages in thread From: Kevin Hilman @ 2019-04-29 19:19 UTC (permalink / raw) To: Guillaume La Roque, linus.walleij, robh+dt, mark.rutland Cc: linux-gpio, linux-amlogic, linux-kernel, devicetree Rob, Guillaume La Roque <glaroque@baylibre.com> writes: > This property allow drive-strength parameter in uA instead of mA. > > Signed-off-by: Guillaume La Roque <glaroque@baylibre.com> > --- > Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt b/Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt > index cef2b5855d60..fc7018459aa2 100644 > --- a/Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt > +++ b/Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt > @@ -258,6 +258,7 @@ drive-push-pull - drive actively high and low > drive-open-drain - drive with open drain > drive-open-source - drive with open source > drive-strength - sink or source at most X mA > +drive-strength-uA - sink or source at most X uA > input-enable - enable input on pin (no effect on output, such as > enabling an input buffer) > input-disable - disable input on pin (no effect on output, such as > @@ -326,6 +327,8 @@ arguments are described below. > > - drive-strength takes as argument the target strength in mA. > > +- drive-strength-uA takes as argument the target strength in uA. > + > - input-debounce takes the debounce time in usec as argument > or 0 to disable debouncing Can we get your input on this? Linus W. is OK with this[1], but wants opinion/approval from DT maintainers first. Thanks, Kevin [1] https://lore.kernel.org/lkml/CACRpkdZ2dPzrtJQkxmN7V=f6+qYZAvrF+b0J77cN9hoRAgFqrw@mail.gmail.com/T/#u _______________________________________________ linux-amlogic mailing list linux-amlogic@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-amlogic ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v2 1/4] dt-bindings: pinctrl: add a 'drive-strength-uA' property 2019-04-18 12:47 ` [PATCH v2 1/4] dt-bindings: pinctrl: add a 'drive-strength-uA' property Guillaume La Roque ` (2 preceding siblings ...) 2019-04-29 19:19 ` Kevin Hilman @ 2019-04-30 15:12 ` Rob Herring 2019-04-30 15:24 ` guillaume La Roque 3 siblings, 1 reply; 18+ messages in thread From: Rob Herring @ 2019-04-30 15:12 UTC (permalink / raw) To: Guillaume La Roque Cc: mark.rutland, devicetree, khilman, linus.walleij, linux-kernel, linux-gpio, linux-amlogic On Thu, Apr 18, 2019 at 02:47:55PM +0200, Guillaume La Roque wrote: > This property allow drive-strength parameter in uA instead of mA. > > Signed-off-by: Guillaume La Roque <glaroque@baylibre.com> > --- > Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt b/Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt > index cef2b5855d60..fc7018459aa2 100644 > --- a/Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt > +++ b/Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt > @@ -258,6 +258,7 @@ drive-push-pull - drive actively high and low > drive-open-drain - drive with open drain > drive-open-source - drive with open source > drive-strength - sink or source at most X mA > +drive-strength-uA - sink or source at most X uA > input-enable - enable input on pin (no effect on output, such as > enabling an input buffer) > input-disable - disable input on pin (no effect on output, such as > @@ -326,6 +327,8 @@ arguments are described below. > > - drive-strength takes as argument the target strength in mA. > > +- drive-strength-uA takes as argument the target strength in uA. > + We have standard unit suffixes defined in bindings/property-units.txt. Use them please. Rob _______________________________________________ linux-amlogic mailing list linux-amlogic@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-amlogic ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v2 1/4] dt-bindings: pinctrl: add a 'drive-strength-uA' property 2019-04-30 15:12 ` Rob Herring @ 2019-04-30 15:24 ` guillaume La Roque 0 siblings, 0 replies; 18+ messages in thread From: guillaume La Roque @ 2019-04-30 15:24 UTC (permalink / raw) To: Rob Herring Cc: mark.rutland, devicetree, khilman, linus.walleij, linux-kernel, linux-gpio, linux-amlogic Hi Rob, On 4/30/19 5:12 PM, Rob Herring wrote: > On Thu, Apr 18, 2019 at 02:47:55PM +0200, Guillaume La Roque wrote: >> This property allow drive-strength parameter in uA instead of mA. >> >> Signed-off-by: Guillaume La Roque <glaroque@baylibre.com> >> --- >> Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt | 3 +++ >> 1 file changed, 3 insertions(+) >> >> diff --git a/Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt b/Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt >> index cef2b5855d60..fc7018459aa2 100644 >> --- a/Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt >> +++ b/Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt >> @@ -258,6 +258,7 @@ drive-push-pull - drive actively high and low >> drive-open-drain - drive with open drain >> drive-open-source - drive with open source >> drive-strength - sink or source at most X mA >> +drive-strength-uA - sink or source at most X uA >> input-enable - enable input on pin (no effect on output, such as >> enabling an input buffer) >> input-disable - disable input on pin (no effect on output, such as >> @@ -326,6 +327,8 @@ arguments are described below. >> >> - drive-strength takes as argument the target strength in mA. >> >> +- drive-strength-uA takes as argument the target strength in uA. >> + > We have standard unit suffixes defined in bindings/property-units.txt. > Use them please. thanks for your feedback and sorry i don't see this doc. According to it i will update patch series with drive-strength-microamp > Rob Guillaume _______________________________________________ linux-amlogic mailing list linux-amlogic@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-amlogic ^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH v2 2/4] pinctrl: generic: add new 'drive-strength-uA' property support 2019-04-18 12:47 [PATCH v2 0/4] Add drive-strength in Meson pinctrl driver Guillaume La Roque 2019-04-18 12:47 ` [PATCH v2 1/4] dt-bindings: pinctrl: add a 'drive-strength-uA' property Guillaume La Roque @ 2019-04-18 12:47 ` Guillaume La Roque 2019-04-18 12:47 ` [PATCH v2 3/4] dt-bindings: pinctrl: meson: Add drive-strength-uA property Guillaume La Roque ` (2 subsequent siblings) 4 siblings, 0 replies; 18+ messages in thread From: Guillaume La Roque @ 2019-04-18 12:47 UTC (permalink / raw) To: linus.walleij, robh+dt, mark.rutland, khilman Cc: linux-gpio, linux-amlogic, linux-kernel, devicetree Add drive-strength-uA property support to allow drive strength in uA Signed-off-by: Guillaume La Roque <glaroque@baylibre.com> --- drivers/pinctrl/pinconf-generic.c | 2 ++ include/linux/pinctrl/pinconf-generic.h | 3 +++ 2 files changed, 5 insertions(+) diff --git a/drivers/pinctrl/pinconf-generic.c b/drivers/pinctrl/pinconf-generic.c index b4f7f8a458ea..87393e9452b4 100644 --- a/drivers/pinctrl/pinconf-generic.c +++ b/drivers/pinctrl/pinconf-generic.c @@ -39,6 +39,7 @@ static const struct pin_config_item conf_items[] = { PCONFDUMP(PIN_CONFIG_DRIVE_OPEN_SOURCE, "output drive open source", NULL, false), PCONFDUMP(PIN_CONFIG_DRIVE_PUSH_PULL, "output drive push pull", NULL, false), PCONFDUMP(PIN_CONFIG_DRIVE_STRENGTH, "output drive strength", "mA", true), + PCONFDUMP(PIN_CONFIG_DRIVE_STRENGTH_UA, "output drive strength", "uA", true), PCONFDUMP(PIN_CONFIG_INPUT_DEBOUNCE, "input debounce", "usec", true), PCONFDUMP(PIN_CONFIG_INPUT_ENABLE, "input enabled", NULL, false), PCONFDUMP(PIN_CONFIG_INPUT_SCHMITT, "input schmitt trigger", NULL, false), @@ -167,6 +168,7 @@ static const struct pinconf_generic_params dt_params[] = { { "drive-open-source", PIN_CONFIG_DRIVE_OPEN_SOURCE, 0 }, { "drive-push-pull", PIN_CONFIG_DRIVE_PUSH_PULL, 0 }, { "drive-strength", PIN_CONFIG_DRIVE_STRENGTH, 0 }, + { "drive-strength-uA", PIN_CONFIG_DRIVE_STRENGTH_UA, 0 }, { "input-debounce", PIN_CONFIG_INPUT_DEBOUNCE, 0 }, { "input-disable", PIN_CONFIG_INPUT_ENABLE, 0 }, { "input-enable", PIN_CONFIG_INPUT_ENABLE, 1 }, diff --git a/include/linux/pinctrl/pinconf-generic.h b/include/linux/pinctrl/pinconf-generic.h index 6c0680641108..72d06d6a3099 100644 --- a/include/linux/pinctrl/pinconf-generic.h +++ b/include/linux/pinctrl/pinconf-generic.h @@ -55,6 +55,8 @@ * push-pull mode, the argument is ignored. * @PIN_CONFIG_DRIVE_STRENGTH: the pin will sink or source at most the current * passed as argument. The argument is in mA. + * @PIN_CONFIG_DRIVE_STRENGTH_UA: the pin will sink or source at most the current + * passed as argument. The argument is in uA. * @PIN_CONFIG_INPUT_DEBOUNCE: this will configure the pin to debounce mode, * which means it will wait for signals to settle when reading inputs. The * argument gives the debounce time in usecs. Setting the @@ -112,6 +114,7 @@ enum pin_config_param { PIN_CONFIG_DRIVE_OPEN_SOURCE, PIN_CONFIG_DRIVE_PUSH_PULL, PIN_CONFIG_DRIVE_STRENGTH, + PIN_CONFIG_DRIVE_STRENGTH_UA, PIN_CONFIG_INPUT_DEBOUNCE, PIN_CONFIG_INPUT_ENABLE, PIN_CONFIG_INPUT_SCHMITT, -- 2.17.1 _______________________________________________ linux-amlogic mailing list linux-amlogic@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-amlogic ^ permalink raw reply related [flat|nested] 18+ messages in thread
* [PATCH v2 3/4] dt-bindings: pinctrl: meson: Add drive-strength-uA property 2019-04-18 12:47 [PATCH v2 0/4] Add drive-strength in Meson pinctrl driver Guillaume La Roque 2019-04-18 12:47 ` [PATCH v2 1/4] dt-bindings: pinctrl: add a 'drive-strength-uA' property Guillaume La Roque 2019-04-18 12:47 ` [PATCH v2 2/4] pinctrl: generic: add new 'drive-strength-uA' property support Guillaume La Roque @ 2019-04-18 12:47 ` Guillaume La Roque 2019-04-27 19:21 ` Martin Blumenstingl 2019-04-18 12:47 ` [PATCH v2 4/4] pinctrl: meson: add support of drive-strength-uA Guillaume La Roque 2019-04-23 11:13 ` [PATCH v2 0/4] Add drive-strength in Meson pinctrl driver Linus Walleij 4 siblings, 1 reply; 18+ messages in thread From: Guillaume La Roque @ 2019-04-18 12:47 UTC (permalink / raw) To: linus.walleij, robh+dt, mark.rutland, khilman Cc: linux-gpio, linux-amlogic, linux-kernel, devicetree Add optional drive-strength-uA property Signed-off-by: Guillaume La Roque <glaroque@baylibre.com> --- Documentation/devicetree/bindings/pinctrl/meson,pinctrl.txt | 3 +++ 1 file changed, 3 insertions(+) diff --git a/Documentation/devicetree/bindings/pinctrl/meson,pinctrl.txt b/Documentation/devicetree/bindings/pinctrl/meson,pinctrl.txt index a47dd990a8d3..b3e4be696ddc 100644 --- a/Documentation/devicetree/bindings/pinctrl/meson,pinctrl.txt +++ b/Documentation/devicetree/bindings/pinctrl/meson,pinctrl.txt @@ -51,6 +51,9 @@ Configuration nodes support the generic properties "bias-disable", "bias-pull-up" and "bias-pull-down", described in file pinctrl-bindings.txt +Optional properties : + - drive-strength-uA: Drive strength for the specified pins in uA. + === Example === pinctrl: pinctrl@c1109880 { -- 2.17.1 _______________________________________________ linux-amlogic mailing list linux-amlogic@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-amlogic ^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH v2 3/4] dt-bindings: pinctrl: meson: Add drive-strength-uA property 2019-04-18 12:47 ` [PATCH v2 3/4] dt-bindings: pinctrl: meson: Add drive-strength-uA property Guillaume La Roque @ 2019-04-27 19:21 ` Martin Blumenstingl 2019-04-30 6:38 ` guillaume La Roque 0 siblings, 1 reply; 18+ messages in thread From: Martin Blumenstingl @ 2019-04-27 19:21 UTC (permalink / raw) To: Guillaume La Roque Cc: mark.rutland, devicetree, khilman, linus.walleij, linux-kernel, linux-gpio, robh+dt, linux-amlogic Hi Guillaume, On Thu, Apr 18, 2019 at 2:48 PM Guillaume La Roque <glaroque@baylibre.com> wrote: > > Add optional drive-strength-uA property > > Signed-off-by: Guillaume La Roque <glaroque@baylibre.com> > --- > Documentation/devicetree/bindings/pinctrl/meson,pinctrl.txt | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/Documentation/devicetree/bindings/pinctrl/meson,pinctrl.txt b/Documentation/devicetree/bindings/pinctrl/meson,pinctrl.txt > index a47dd990a8d3..b3e4be696ddc 100644 > --- a/Documentation/devicetree/bindings/pinctrl/meson,pinctrl.txt > +++ b/Documentation/devicetree/bindings/pinctrl/meson,pinctrl.txt > @@ -51,6 +51,9 @@ Configuration nodes support the generic properties "bias-disable", > "bias-pull-up" and "bias-pull-down", described in file > pinctrl-bindings.txt > > +Optional properties : > + - drive-strength-uA: Drive strength for the specified pins in uA. if you have to re-send this series for whatever reason then please mention that drive-strength-uA is only valid for G12A and newer otherwise: Reviewed-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com> _______________________________________________ linux-amlogic mailing list linux-amlogic@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-amlogic ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v2 3/4] dt-bindings: pinctrl: meson: Add drive-strength-uA property 2019-04-27 19:21 ` Martin Blumenstingl @ 2019-04-30 6:38 ` guillaume La Roque 0 siblings, 0 replies; 18+ messages in thread From: guillaume La Roque @ 2019-04-30 6:38 UTC (permalink / raw) To: Martin Blumenstingl Cc: mark.rutland, devicetree, khilman, linus.walleij, linux-kernel, linux-gpio, robh+dt, linux-amlogic Hi Martin, On 4/27/19 9:21 PM, Martin Blumenstingl wrote: > Hi Guillaume, > > On Thu, Apr 18, 2019 at 2:48 PM Guillaume La Roque > <glaroque@baylibre.com> wrote: >> Add optional drive-strength-uA property >> >> Signed-off-by: Guillaume La Roque <glaroque@baylibre.com> >> --- >> Documentation/devicetree/bindings/pinctrl/meson,pinctrl.txt | 3 +++ >> 1 file changed, 3 insertions(+) >> >> diff --git a/Documentation/devicetree/bindings/pinctrl/meson,pinctrl.txt b/Documentation/devicetree/bindings/pinctrl/meson,pinctrl.txt >> index a47dd990a8d3..b3e4be696ddc 100644 >> --- a/Documentation/devicetree/bindings/pinctrl/meson,pinctrl.txt >> +++ b/Documentation/devicetree/bindings/pinctrl/meson,pinctrl.txt >> @@ -51,6 +51,9 @@ Configuration nodes support the generic properties "bias-disable", >> "bias-pull-up" and "bias-pull-down", described in file >> pinctrl-bindings.txt >> >> +Optional properties : >> + - drive-strength-uA: Drive strength for the specified pins in uA. > if you have to re-send this series for whatever reason then please > mention that drive-strength-uA is only valid for G12A and newer thanks for your review, i will do if i send new series. > otherwise: > Reviewed-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com> _______________________________________________ linux-amlogic mailing list linux-amlogic@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-amlogic ^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH v2 4/4] pinctrl: meson: add support of drive-strength-uA 2019-04-18 12:47 [PATCH v2 0/4] Add drive-strength in Meson pinctrl driver Guillaume La Roque ` (2 preceding siblings ...) 2019-04-18 12:47 ` [PATCH v2 3/4] dt-bindings: pinctrl: meson: Add drive-strength-uA property Guillaume La Roque @ 2019-04-18 12:47 ` Guillaume La Roque 2019-04-27 19:44 ` Martin Blumenstingl 2019-04-23 11:13 ` [PATCH v2 0/4] Add drive-strength in Meson pinctrl driver Linus Walleij 4 siblings, 1 reply; 18+ messages in thread From: Guillaume La Roque @ 2019-04-18 12:47 UTC (permalink / raw) To: linus.walleij, robh+dt, mark.rutland, khilman Cc: linux-gpio, linux-amlogic, linux-kernel, devicetree drive-strength-uA is a new feature needed for G12A SoC. the default DS setting after boot is usually 500uA and it is not enough for many functions. We need to be able to set the drive strength to reliably enable things like MMC, I2C, etc ... Signed-off-by: Guillaume La Roque <glaroque@baylibre.com> --- drivers/pinctrl/meson/pinctrl-meson-g12a.c | 36 ++--- drivers/pinctrl/meson/pinctrl-meson.c | 166 ++++++++++++++++----- drivers/pinctrl/meson/pinctrl-meson.h | 20 ++- 3 files changed, 163 insertions(+), 59 deletions(-) diff --git a/drivers/pinctrl/meson/pinctrl-meson-g12a.c b/drivers/pinctrl/meson/pinctrl-meson-g12a.c index d494492e98e9..3475cd7bd2af 100644 --- a/drivers/pinctrl/meson/pinctrl-meson-g12a.c +++ b/drivers/pinctrl/meson/pinctrl-meson-g12a.c @@ -1304,28 +1304,28 @@ static struct meson_pmx_func meson_g12a_aobus_functions[] = { }; static struct meson_bank meson_g12a_periphs_banks[] = { - /* name first last irq pullen pull dir out in */ - BANK("Z", GPIOZ_0, GPIOZ_15, 12, 27, - 4, 0, 4, 0, 12, 0, 13, 0, 14, 0), - BANK("H", GPIOH_0, GPIOH_8, 28, 36, - 3, 0, 3, 0, 9, 0, 10, 0, 11, 0), - BANK("BOOT", BOOT_0, BOOT_15, 37, 52, - 0, 0, 0, 0, 0, 0, 1, 0, 2, 0), - BANK("C", GPIOC_0, GPIOC_7, 53, 60, - 1, 0, 1, 0, 3, 0, 4, 0, 5, 0), - BANK("A", GPIOA_0, GPIOA_15, 61, 76, - 5, 0, 5, 0, 16, 0, 17, 0, 18, 0), - BANK("X", GPIOX_0, GPIOX_19, 77, 96, - 2, 0, 2, 0, 6, 0, 7, 0, 8, 0), + /* name first last irq pullen pull dir out in ds */ + BANK_DS("Z", GPIOZ_0, GPIOZ_15, 12, 27, + 4, 0, 4, 0, 12, 0, 13, 0, 14, 0, 5, 0), + BANK_DS("H", GPIOH_0, GPIOH_8, 28, 36, + 3, 0, 3, 0, 9, 0, 10, 0, 11, 0, 4, 0), + BANK_DS("BOOT", BOOT_0, BOOT_15, 37, 52, + 0, 0, 0, 0, 0, 0, 1, 0, 2, 0, 0, 0), + BANK_DS("C", GPIOC_0, GPIOC_7, 53, 60, + 1, 0, 1, 0, 3, 0, 4, 0, 5, 0, 1, 0), + BANK_DS("A", GPIOA_0, GPIOA_15, 61, 76, + 5, 0, 5, 0, 16, 0, 17, 0, 18, 0, 6, 0), + BANK_DS("X", GPIOX_0, GPIOX_19, 77, 96, + 2, 0, 2, 0, 6, 0, 7, 0, 8, 0, 2, 0), }; static struct meson_bank meson_g12a_aobus_banks[] = { - /* name first last irq pullen pull dir out in */ - BANK("AO", GPIOAO_0, GPIOAO_11, 0, 11, - 3, 0, 2, 0, 0, 0, 4, 0, 1, 0), + /* name first last irq pullen pull dir out in ds */ + BANK_DS("AO", GPIOAO_0, GPIOAO_11, 0, 11, 3, 0, 2, 0, 0, 0, 4, 0, 1, 0, + 0, 0), /* GPIOE actually located in the AO bank */ - BANK("E", GPIOE_0, GPIOE_2, 97, 99, - 3, 16, 2, 16, 0, 16, 4, 16, 1, 16), + BANK_DS("E", GPIOE_0, GPIOE_2, 97, 99, 3, 16, 2, 16, 0, 16, 4, 16, 1, + 16, 1, 0), }; static struct meson_pmx_bank meson_g12a_periphs_pmx_banks[] = { diff --git a/drivers/pinctrl/meson/pinctrl-meson.c b/drivers/pinctrl/meson/pinctrl-meson.c index 96a4a72708e4..5108e5aa6514 100644 --- a/drivers/pinctrl/meson/pinctrl-meson.c +++ b/drivers/pinctrl/meson/pinctrl-meson.c @@ -174,62 +174,106 @@ int meson_pmx_get_groups(struct pinctrl_dev *pcdev, unsigned selector, return 0; } -static int meson_pinconf_set(struct pinctrl_dev *pcdev, unsigned int pin, - unsigned long *configs, unsigned num_configs) +static int meson_pinconf_set_bias(struct meson_pinctrl *pc, unsigned int pin, + enum pin_config_param conf) +{ + struct meson_bank *bank; + unsigned int reg, bit, val = 0; + int ret; + + ret = meson_get_bank(pc, pin, &bank); + if (ret) + return ret; + + meson_calc_reg_and_bit(bank, pin, REG_PULLEN, ®, &bit); + + if (conf == PIN_CONFIG_BIAS_DISABLE) { + ret = regmap_update_bits(pc->reg_pullen, reg, BIT(bit), 0); + if (ret) + return ret; + } else { + meson_calc_reg_and_bit(bank, pin, REG_PULL, ®, &bit); + if (conf == PIN_CONFIG_BIAS_PULL_UP) + val = BIT(bit); + + ret = regmap_update_bits(pc->reg_pull, reg, BIT(bit), val); + if (ret) + return ret; + + meson_calc_reg_and_bit(bank, pin, REG_PULLEN, ®, &bit); + ret = regmap_update_bits(pc->reg_pullen, reg, BIT(bit), + BIT(bit)); + if (ret) + return ret; + } + + return 0; +} + +static int meson_pinconf_set_drive_strength(struct meson_pinctrl *pc, + unsigned int pin, u16 arg) { - struct meson_pinctrl *pc = pinctrl_dev_get_drvdata(pcdev); struct meson_bank *bank; - enum pin_config_param param; unsigned int reg, bit; - int i, ret; + unsigned int ds_val; + int ret; + + if (!pc->reg_ds) { + dev_err(pc->dev, "drive-strength not supported\n"); + return -ENOTSUPP; + } ret = meson_get_bank(pc, pin, &bank); if (ret) return ret; + meson_calc_reg_and_bit(bank, pin, REG_DS, ®, &bit); + bit = bit << 1; + + if (arg <= 500) { + ds_val = MESON_PINCONF_DRV_500UA; + } else if (arg <= 2500) { + ds_val = MESON_PINCONF_DRV_2500UA; + } else if (arg <= 3000) { + ds_val = MESON_PINCONF_DRV_3000UA; + } else if (arg <= 4000) { + ds_val = MESON_PINCONF_DRV_4000UA; + } else { + dev_warn_once(pc->dev, + "pin %u: invalid drive-strength : %d , default to 4mA\n", + pin, arg); + ds_val = MESON_PINCONF_DRV_4000UA; + } + + ret = regmap_update_bits(pc->reg_ds, reg, 0x3 << bit, ds_val << bit); + if (ret) + return ret; + + return 0; +} + +static int meson_pinconf_set(struct pinctrl_dev *pcdev, unsigned int pin, + unsigned long *configs, unsigned num_configs) +{ + struct meson_pinctrl *pc = pinctrl_dev_get_drvdata(pcdev); + enum pin_config_param param; + unsigned int arg; + int i, ret; + for (i = 0; i < num_configs; i++) { param = pinconf_to_config_param(configs[i]); switch (param) { case PIN_CONFIG_BIAS_DISABLE: - dev_dbg(pc->dev, "pin %u: disable bias\n", pin); - - meson_calc_reg_and_bit(bank, pin, REG_PULLEN, ®, - &bit); - ret = regmap_update_bits(pc->reg_pullen, reg, - BIT(bit), 0); - if (ret) - return ret; - break; case PIN_CONFIG_BIAS_PULL_UP: - dev_dbg(pc->dev, "pin %u: enable pull-up\n", pin); - - meson_calc_reg_and_bit(bank, pin, REG_PULLEN, - ®, &bit); - ret = regmap_update_bits(pc->reg_pullen, reg, - BIT(bit), BIT(bit)); - if (ret) - return ret; - - meson_calc_reg_and_bit(bank, pin, REG_PULL, ®, &bit); - ret = regmap_update_bits(pc->reg_pull, reg, - BIT(bit), BIT(bit)); - if (ret) - return ret; - break; case PIN_CONFIG_BIAS_PULL_DOWN: - dev_dbg(pc->dev, "pin %u: enable pull-down\n", pin); - - meson_calc_reg_and_bit(bank, pin, REG_PULLEN, - ®, &bit); - ret = regmap_update_bits(pc->reg_pullen, reg, - BIT(bit), BIT(bit)); + ret = meson_pinconf_set_bias(pc, pin, param); if (ret) return ret; - - meson_calc_reg_and_bit(bank, pin, REG_PULL, ®, &bit); - ret = regmap_update_bits(pc->reg_pull, reg, - BIT(bit), 0); + break; + case PIN_CONFIG_DRIVE_STRENGTH_UA: + arg = pinconf_to_config_argument(configs[i]); + ret = meson_pinconf_set_drive_strength(pc, pin, arg); if (ret) return ret; break; @@ -275,12 +319,51 @@ static int meson_pinconf_get_pull(struct meson_pinctrl *pc, unsigned int pin) return conf; } +static int meson_pinconf_get_drive_strength(struct meson_pinctrl *pc, + unsigned int pin, u16 *arg) +{ + struct meson_bank *bank; + unsigned int reg, bit; + unsigned int val; + int ret; + + ret = meson_get_bank(pc, pin, &bank); + if (ret) + return ret; + + meson_calc_reg_and_bit(bank, pin, REG_DS, ®, &bit); + + ret = regmap_read(pc->reg_ds, reg, &val); + if (ret) + return ret; + + switch ((val >> bit) & 0x3) { + case MESON_PINCONF_DRV_500UA: + *arg = 500; + break; + case MESON_PINCONF_DRV_2500UA: + *arg = 2500; + break; + case MESON_PINCONF_DRV_3000UA: + *arg = 3000; + break; + case MESON_PINCONF_DRV_4000UA: + *arg = 4000; + break; + default: + return -EINVAL; + } + + return 0; +} + static int meson_pinconf_get(struct pinctrl_dev *pcdev, unsigned int pin, unsigned long *config) { struct meson_pinctrl *pc = pinctrl_dev_get_drvdata(pcdev); enum pin_config_param param = pinconf_to_config_param(*config); u16 arg; + int ret; switch (param) { case PIN_CONFIG_BIAS_DISABLE: @@ -291,6 +374,11 @@ static int meson_pinconf_get(struct pinctrl_dev *pcdev, unsigned int pin, else return -EINVAL; break; + case PIN_CONFIG_DRIVE_STRENGTH_UA: + ret = meson_pinconf_get_drive_strength(pc, pin, &arg); + if (ret) + return ret; + break; default: return -ENOTSUPP; } diff --git a/drivers/pinctrl/meson/pinctrl-meson.h b/drivers/pinctrl/meson/pinctrl-meson.h index 5eaab925f427..1a88103dcb9b 100644 --- a/drivers/pinctrl/meson/pinctrl-meson.h +++ b/drivers/pinctrl/meson/pinctrl-meson.h @@ -71,9 +71,20 @@ enum meson_reg_type { REG_DIR, REG_OUT, REG_IN, + REG_DS, NUM_REG, }; +/** + * enum meson_pinconf_drv - value of drive-strength supported + */ +enum meson_pinconf_drv { + MESON_PINCONF_DRV_500UA, + MESON_PINCONF_DRV_2500UA, + MESON_PINCONF_DRV_3000UA, + MESON_PINCONF_DRV_4000UA, +}; + /** * struct meson bank * @@ -132,7 +143,8 @@ struct meson_pinctrl { .num_groups = ARRAY_SIZE(fn ## _groups), \ } -#define BANK(n, f, l, fi, li, per, peb, pr, pb, dr, db, or, ob, ir, ib) \ +#define BANK_DS(n, f, l, fi, li, per, peb, pr, pb, dr, db, or, ob, ir, ib, \ + dsr, dsb) \ { \ .name = n, \ .first = f, \ @@ -145,8 +157,12 @@ struct meson_pinctrl { [REG_DIR] = { dr, db }, \ [REG_OUT] = { or, ob }, \ [REG_IN] = { ir, ib }, \ + [REG_DS] = { dsr, dsb }, \ }, \ - } + } + +#define BANK(n, f, l, fi, li, per, peb, pr, pb, dr, db, or, ob, ir, ib) \ + BANK_DS(n, f, l, fi, li, per, peb, pr, pb, dr, db, or, ob, ir, ib, 0, 0) #define MESON_PIN(x) PINCTRL_PIN(x, #x) -- 2.17.1 _______________________________________________ linux-amlogic mailing list linux-amlogic@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-amlogic ^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH v2 4/4] pinctrl: meson: add support of drive-strength-uA 2019-04-18 12:47 ` [PATCH v2 4/4] pinctrl: meson: add support of drive-strength-uA Guillaume La Roque @ 2019-04-27 19:44 ` Martin Blumenstingl 2019-04-30 7:20 ` guillaume La Roque 0 siblings, 1 reply; 18+ messages in thread From: Martin Blumenstingl @ 2019-04-27 19:44 UTC (permalink / raw) To: Guillaume La Roque Cc: mark.rutland, devicetree, khilman, linus.walleij, linux-kernel, linux-gpio, robh+dt, linux-amlogic Hi Guillaume, On Thu, Apr 18, 2019 at 2:48 PM Guillaume La Roque <glaroque@baylibre.com> wrote: > > drive-strength-uA is a new feature needed for G12A SoC. > the default DS setting after boot is usually 500uA and it is not enough for > many functions. We need to be able to set the drive strength to reliably > enable things like MMC, I2C, etc ... > > Signed-off-by: Guillaume La Roque <glaroque@baylibre.com> I gave this a go on Meson8m2 (meaning I applied all four patches from this series and booted the result on my board): [Meson8m2 doesn't support drive strength and still boots without any crashes or obvious regressions] Tested-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com> > --- > drivers/pinctrl/meson/pinctrl-meson-g12a.c | 36 ++--- > drivers/pinctrl/meson/pinctrl-meson.c | 166 ++++++++++++++++----- > drivers/pinctrl/meson/pinctrl-meson.h | 20 ++- personally I would have split this into two separate patches: - one for the generic pinctrl-meson part which adds drive-strength-uA support - another patch for enabling this on G12A if nobody else wants you to split this then it's fine for me as well > 3 files changed, 163 insertions(+), 59 deletions(-) > > diff --git a/drivers/pinctrl/meson/pinctrl-meson-g12a.c b/drivers/pinctrl/meson/pinctrl-meson-g12a.c > index d494492e98e9..3475cd7bd2af 100644 > --- a/drivers/pinctrl/meson/pinctrl-meson-g12a.c > +++ b/drivers/pinctrl/meson/pinctrl-meson-g12a.c > @@ -1304,28 +1304,28 @@ static struct meson_pmx_func meson_g12a_aobus_functions[] = { > }; > > static struct meson_bank meson_g12a_periphs_banks[] = { > - /* name first last irq pullen pull dir out in */ > - BANK("Z", GPIOZ_0, GPIOZ_15, 12, 27, > - 4, 0, 4, 0, 12, 0, 13, 0, 14, 0), > - BANK("H", GPIOH_0, GPIOH_8, 28, 36, > - 3, 0, 3, 0, 9, 0, 10, 0, 11, 0), > - BANK("BOOT", BOOT_0, BOOT_15, 37, 52, > - 0, 0, 0, 0, 0, 0, 1, 0, 2, 0), > - BANK("C", GPIOC_0, GPIOC_7, 53, 60, > - 1, 0, 1, 0, 3, 0, 4, 0, 5, 0), > - BANK("A", GPIOA_0, GPIOA_15, 61, 76, > - 5, 0, 5, 0, 16, 0, 17, 0, 18, 0), > - BANK("X", GPIOX_0, GPIOX_19, 77, 96, > - 2, 0, 2, 0, 6, 0, 7, 0, 8, 0), > + /* name first last irq pullen pull dir out in ds */ > + BANK_DS("Z", GPIOZ_0, GPIOZ_15, 12, 27, > + 4, 0, 4, 0, 12, 0, 13, 0, 14, 0, 5, 0), > + BANK_DS("H", GPIOH_0, GPIOH_8, 28, 36, > + 3, 0, 3, 0, 9, 0, 10, 0, 11, 0, 4, 0), > + BANK_DS("BOOT", BOOT_0, BOOT_15, 37, 52, > + 0, 0, 0, 0, 0, 0, 1, 0, 2, 0, 0, 0), > + BANK_DS("C", GPIOC_0, GPIOC_7, 53, 60, > + 1, 0, 1, 0, 3, 0, 4, 0, 5, 0, 1, 0), > + BANK_DS("A", GPIOA_0, GPIOA_15, 61, 76, > + 5, 0, 5, 0, 16, 0, 17, 0, 18, 0, 6, 0), > + BANK_DS("X", GPIOX_0, GPIOX_19, 77, 96, > + 2, 0, 2, 0, 6, 0, 7, 0, 8, 0, 2, 0), > }; > > static struct meson_bank meson_g12a_aobus_banks[] = { > - /* name first last irq pullen pull dir out in */ > - BANK("AO", GPIOAO_0, GPIOAO_11, 0, 11, > - 3, 0, 2, 0, 0, 0, 4, 0, 1, 0), > + /* name first last irq pullen pull dir out in ds */ > + BANK_DS("AO", GPIOAO_0, GPIOAO_11, 0, 11, 3, 0, 2, 0, 0, 0, 4, 0, 1, 0, > + 0, 0), > /* GPIOE actually located in the AO bank */ > - BANK("E", GPIOE_0, GPIOE_2, 97, 99, > - 3, 16, 2, 16, 0, 16, 4, 16, 1, 16), > + BANK_DS("E", GPIOE_0, GPIOE_2, 97, 99, 3, 16, 2, 16, 0, 16, 4, 16, 1, > + 16, 1, 0), > }; these definitions are really hard to read, but it's been like this even before your patch > static struct meson_pmx_bank meson_g12a_periphs_pmx_banks[] = { > diff --git a/drivers/pinctrl/meson/pinctrl-meson.c b/drivers/pinctrl/meson/pinctrl-meson.c > index 96a4a72708e4..5108e5aa6514 100644 > --- a/drivers/pinctrl/meson/pinctrl-meson.c > +++ b/drivers/pinctrl/meson/pinctrl-meson.c > @@ -174,62 +174,106 @@ int meson_pmx_get_groups(struct pinctrl_dev *pcdev, unsigned selector, > return 0; > } > > -static int meson_pinconf_set(struct pinctrl_dev *pcdev, unsigned int pin, > - unsigned long *configs, unsigned num_configs) > +static int meson_pinconf_set_bias(struct meson_pinctrl *pc, unsigned int pin, > + enum pin_config_param conf) can you please confirm that I understood the purpose of this correctly: I think you introduce this to make setting the bias consistent with how you set the drive-strength. if so then it would be great to have a separate patch which describes that it's only a code-style change and a functional no-op additionally the function arguments are not consistent with meson_pinconf_get_drive_strength(): - here you pass the pinctrl subsystem specific parameters (enum pin_config_param conf) - in meson_pinconf_get_drive_strength the conversion for pinctrl subsystem specific values (pinconf_to_config_argument) is part of meson_pinconf_set I'm wondering whether two separate functions (meson_pinconf_disable_bias and meson_pinconf_enable_bias) would make things easier to read. I haven't tried whether this would really make things better, so I'd like to hear your opinion on this Guillaume! [...] > +static int meson_pinconf_set_drive_strength(struct meson_pinctrl *pc, > + unsigned int pin, u16 arg) > { > - struct meson_pinctrl *pc = pinctrl_dev_get_drvdata(pcdev); > struct meson_bank *bank; > - enum pin_config_param param; > unsigned int reg, bit; > - int i, ret; > + unsigned int ds_val; > + int ret; > + > + if (!pc->reg_ds) { > + dev_err(pc->dev, "drive-strength not supported\n"); > + return -ENOTSUPP; in meson_pinconf_set() we don't complain (with a dev_err) for this case. I'm not sure what the best-practice is for the pinctrl subsystem, maybe Linus can comment on this > + } > > ret = meson_get_bank(pc, pin, &bank); > if (ret) > return ret; > > + meson_calc_reg_and_bit(bank, pin, REG_DS, ®, &bit); > + bit = bit << 1; > + > + if (arg <= 500) { > + ds_val = MESON_PINCONF_DRV_500UA; > + } else if (arg <= 2500) { > + ds_val = MESON_PINCONF_DRV_2500UA; > + } else if (arg <= 3000) { > + ds_val = MESON_PINCONF_DRV_3000UA; > + } else if (arg <= 4000) { > + ds_val = MESON_PINCONF_DRV_4000UA; > + } else { > + dev_warn_once(pc->dev, > + "pin %u: invalid drive-strength : %d , default to 4mA\n", > + pin, arg); > + ds_val = MESON_PINCONF_DRV_4000UA; why not return -EINVAL here? (my assumption is that the pinctrl subsystem would like to have -EINVAL instead of drivers doing fallbacks if the values are out-of-range, but I'm not 100% sure about this) [...] > +static int meson_pinconf_get_drive_strength(struct meson_pinctrl *pc, > + unsigned int pin, u16 *arg) > +{ > + struct meson_bank *bank; > + unsigned int reg, bit; > + unsigned int val; > + int ret; > + do you need to return -ENOTSUPP here if pc->reg_ds is NULL, similar to what you already have in meson_pinconf_set_drive_strength()? Regards Martin _______________________________________________ linux-amlogic mailing list linux-amlogic@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-amlogic ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v2 4/4] pinctrl: meson: add support of drive-strength-uA 2019-04-27 19:44 ` Martin Blumenstingl @ 2019-04-30 7:20 ` guillaume La Roque 2019-04-30 20:28 ` Martin Blumenstingl 0 siblings, 1 reply; 18+ messages in thread From: guillaume La Roque @ 2019-04-30 7:20 UTC (permalink / raw) To: Martin Blumenstingl Cc: mark.rutland, devicetree, khilman, linus.walleij, linux-kernel, linux-gpio, robh+dt, linux-amlogic Hi Martin, thanks for your feedback. On 4/27/19 9:44 PM, Martin Blumenstingl wrote: > Hi Guillaume, > > On Thu, Apr 18, 2019 at 2:48 PM Guillaume La Roque > <glaroque@baylibre.com> wrote: >> drive-strength-uA is a new feature needed for G12A SoC. >> the default DS setting after boot is usually 500uA and it is not enough for >> many functions. We need to be able to set the drive strength to reliably >> enable things like MMC, I2C, etc ... >> >> Signed-off-by: Guillaume La Roque <glaroque@baylibre.com> > I gave this a go on Meson8m2 (meaning I applied all four patches from > this series and booted the result on my board): > [Meson8m2 doesn't support drive strength and still boots without any > crashes or obvious regressions] > Tested-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com> > >> --- >> drivers/pinctrl/meson/pinctrl-meson-g12a.c | 36 ++--- >> drivers/pinctrl/meson/pinctrl-meson.c | 166 ++++++++++++++++----- >> drivers/pinctrl/meson/pinctrl-meson.h | 20 ++- > personally I would have split this into two separate patches: > - one for the generic pinctrl-meson part which adds drive-strength-uA support > - another patch for enabling this on G12A > > if nobody else wants you to split this then it's fine for me as well why not if i send new series i will do. > >> 3 files changed, 163 insertions(+), 59 deletions(-) >> >> diff --git a/drivers/pinctrl/meson/pinctrl-meson-g12a.c b/drivers/pinctrl/meson/pinctrl-meson-g12a.c >> index d494492e98e9..3475cd7bd2af 100644 >> --- a/drivers/pinctrl/meson/pinctrl-meson-g12a.c >> +++ b/drivers/pinctrl/meson/pinctrl-meson-g12a.c >> @@ -1304,28 +1304,28 @@ static struct meson_pmx_func meson_g12a_aobus_functions[] = { >> }; >> >> static struct meson_bank meson_g12a_periphs_banks[] = { >> - /* name first last irq pullen pull dir out in */ >> - BANK("Z", GPIOZ_0, GPIOZ_15, 12, 27, >> - 4, 0, 4, 0, 12, 0, 13, 0, 14, 0), >> - BANK("H", GPIOH_0, GPIOH_8, 28, 36, >> - 3, 0, 3, 0, 9, 0, 10, 0, 11, 0), >> - BANK("BOOT", BOOT_0, BOOT_15, 37, 52, >> - 0, 0, 0, 0, 0, 0, 1, 0, 2, 0), >> - BANK("C", GPIOC_0, GPIOC_7, 53, 60, >> - 1, 0, 1, 0, 3, 0, 4, 0, 5, 0), >> - BANK("A", GPIOA_0, GPIOA_15, 61, 76, >> - 5, 0, 5, 0, 16, 0, 17, 0, 18, 0), >> - BANK("X", GPIOX_0, GPIOX_19, 77, 96, >> - 2, 0, 2, 0, 6, 0, 7, 0, 8, 0), >> + /* name first last irq pullen pull dir out in ds */ >> + BANK_DS("Z", GPIOZ_0, GPIOZ_15, 12, 27, >> + 4, 0, 4, 0, 12, 0, 13, 0, 14, 0, 5, 0), >> + BANK_DS("H", GPIOH_0, GPIOH_8, 28, 36, >> + 3, 0, 3, 0, 9, 0, 10, 0, 11, 0, 4, 0), >> + BANK_DS("BOOT", BOOT_0, BOOT_15, 37, 52, >> + 0, 0, 0, 0, 0, 0, 1, 0, 2, 0, 0, 0), >> + BANK_DS("C", GPIOC_0, GPIOC_7, 53, 60, >> + 1, 0, 1, 0, 3, 0, 4, 0, 5, 0, 1, 0), >> + BANK_DS("A", GPIOA_0, GPIOA_15, 61, 76, >> + 5, 0, 5, 0, 16, 0, 17, 0, 18, 0, 6, 0), >> + BANK_DS("X", GPIOX_0, GPIOX_19, 77, 96, >> + 2, 0, 2, 0, 6, 0, 7, 0, 8, 0, 2, 0), >> }; >> >> static struct meson_bank meson_g12a_aobus_banks[] = { >> - /* name first last irq pullen pull dir out in */ >> - BANK("AO", GPIOAO_0, GPIOAO_11, 0, 11, >> - 3, 0, 2, 0, 0, 0, 4, 0, 1, 0), >> + /* name first last irq pullen pull dir out in ds */ >> + BANK_DS("AO", GPIOAO_0, GPIOAO_11, 0, 11, 3, 0, 2, 0, 0, 0, 4, 0, 1, 0, >> + 0, 0), >> /* GPIOE actually located in the AO bank */ >> - BANK("E", GPIOE_0, GPIOE_2, 97, 99, >> - 3, 16, 2, 16, 0, 16, 4, 16, 1, 16), >> + BANK_DS("E", GPIOE_0, GPIOE_2, 97, 99, 3, 16, 2, 16, 0, 16, 4, 16, 1, >> + 16, 1, 0), >> }; > these definitions are really hard to read, but it's been like this > even before your patch > >> static struct meson_pmx_bank meson_g12a_periphs_pmx_banks[] = { >> diff --git a/drivers/pinctrl/meson/pinctrl-meson.c b/drivers/pinctrl/meson/pinctrl-meson.c >> index 96a4a72708e4..5108e5aa6514 100644 >> --- a/drivers/pinctrl/meson/pinctrl-meson.c >> +++ b/drivers/pinctrl/meson/pinctrl-meson.c >> @@ -174,62 +174,106 @@ int meson_pmx_get_groups(struct pinctrl_dev *pcdev, unsigned selector, >> return 0; >> } >> >> -static int meson_pinconf_set(struct pinctrl_dev *pcdev, unsigned int pin, >> - unsigned long *configs, unsigned num_configs) >> +static int meson_pinconf_set_bias(struct meson_pinctrl *pc, unsigned int pin, >> + enum pin_config_param conf) > can you please confirm that I understood the purpose of this correctly: > I think you introduce this to make setting the bias consistent with > how you set the drive-strength. > if so then it would be great to have a separate patch which describes > that it's only a code-style change and a functional no-op you are right, i will separate this changes. > > additionally the function arguments are not consistent with > meson_pinconf_get_drive_strength(): > - here you pass the pinctrl subsystem specific parameters (enum > pin_config_param conf) > - in meson_pinconf_get_drive_strength the conversion for pinctrl > subsystem specific values (pinconf_to_config_argument) is part of > meson_pinconf_set for param i'm not sure i understand what you want, if you talk about difference between set_bias and set_drive arg , it's difficult to align it. if it's about diff between get_bias and get_drive i think i can return drive stength value instead of using an u16 arg input param. > I'm wondering whether two separate functions > (meson_pinconf_disable_bias and meson_pinconf_enable_bias) would make > things easier to read. I haven't tried whether this would really make > things better, so I'd like to hear your opinion on this Guillaume! no special opinion on this, if you think it's better for understanding i can separate set_bias function. > > [...] >> +static int meson_pinconf_set_drive_strength(struct meson_pinctrl *pc, >> + unsigned int pin, u16 arg) >> { >> - struct meson_pinctrl *pc = pinctrl_dev_get_drvdata(pcdev); >> struct meson_bank *bank; >> - enum pin_config_param param; >> unsigned int reg, bit; >> - int i, ret; >> + unsigned int ds_val; >> + int ret; >> + >> + if (!pc->reg_ds) { >> + dev_err(pc->dev, "drive-strength not supported\n"); >> + return -ENOTSUPP; > in meson_pinconf_set() we don't complain (with a dev_err) for this case. > I'm not sure what the best-practice is for the pinctrl subsystem, > maybe Linus can comment on this > this check is to be sure it's possible to set drive stength, if no register bank is setting in DT but drive-stength properties are setting on pins i need to generate an error because something is wrong. >> + } >> >> ret = meson_get_bank(pc, pin, &bank); >> if (ret) >> return ret; >> >> + meson_calc_reg_and_bit(bank, pin, REG_DS, ®, &bit); >> + bit = bit << 1; >> + >> + if (arg <= 500) { >> + ds_val = MESON_PINCONF_DRV_500UA; >> + } else if (arg <= 2500) { >> + ds_val = MESON_PINCONF_DRV_2500UA; >> + } else if (arg <= 3000) { >> + ds_val = MESON_PINCONF_DRV_3000UA; >> + } else if (arg <= 4000) { >> + ds_val = MESON_PINCONF_DRV_4000UA; >> + } else { >> + dev_warn_once(pc->dev, >> + "pin %u: invalid drive-strength : %d , default to 4mA\n", >> + pin, arg); >> + ds_val = MESON_PINCONF_DRV_4000UA; > why not return -EINVAL here? (my assumption is that the pinctrl > subsystem would like to have -EINVAL instead of drivers doing > fallbacks if the values are out-of-range, but I'm not 100% sure about > this) > > [...] i choose to set a default value instead of generating an error, in this case it's only if you ask a value upper than 4000uA so it's not really a risk to set 4000uA by default. >> +static int meson_pinconf_get_drive_strength(struct meson_pinctrl *pc, >> + unsigned int pin, u16 *arg) >> +{ >> + struct meson_bank *bank; >> + unsigned int reg, bit; >> + unsigned int val; >> + int ret; >> + > do you need to return -ENOTSUPP here if pc->reg_ds is NULL, similar to > what you already have in meson_pinconf_set_drive_strength()? depending of linux comment on your feedback on meson_pinconf_set_drive_strength i will update this part . > > > Regards > Martin Regards Guillaume _______________________________________________ linux-amlogic mailing list linux-amlogic@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-amlogic ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v2 4/4] pinctrl: meson: add support of drive-strength-uA 2019-04-30 7:20 ` guillaume La Roque @ 2019-04-30 20:28 ` Martin Blumenstingl 2019-05-06 13:16 ` guillaume La Roque 0 siblings, 1 reply; 18+ messages in thread From: Martin Blumenstingl @ 2019-04-30 20:28 UTC (permalink / raw) To: guillaume La Roque Cc: mark.rutland, devicetree, khilman, linus.walleij, linux-kernel, linux-gpio, robh+dt, linux-amlogic Hi Guillaume, On Tue, Apr 30, 2019 at 9:20 AM guillaume La Roque <glaroque@baylibre.com> wrote: > > Hi Martin, > > > thanks for your feedback. > > > On 4/27/19 9:44 PM, Martin Blumenstingl wrote: > > Hi Guillaume, > > > > On Thu, Apr 18, 2019 at 2:48 PM Guillaume La Roque > > <glaroque@baylibre.com> wrote: > >> drive-strength-uA is a new feature needed for G12A SoC. > >> the default DS setting after boot is usually 500uA and it is not enough for > >> many functions. We need to be able to set the drive strength to reliably > >> enable things like MMC, I2C, etc ... > >> > >> Signed-off-by: Guillaume La Roque <glaroque@baylibre.com> > > I gave this a go on Meson8m2 (meaning I applied all four patches from > > this series and booted the result on my board): > > [Meson8m2 doesn't support drive strength and still boots without any > > crashes or obvious regressions] > > Tested-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com> > > > >> --- > >> drivers/pinctrl/meson/pinctrl-meson-g12a.c | 36 ++--- > >> drivers/pinctrl/meson/pinctrl-meson.c | 166 ++++++++++++++++----- > >> drivers/pinctrl/meson/pinctrl-meson.h | 20 ++- > > personally I would have split this into two separate patches: > > - one for the generic pinctrl-meson part which adds drive-strength-uA support > > - another patch for enabling this on G12A > > > > if nobody else wants you to split this then it's fine for me as well > > > why not if i send new series i will do. great, thank you [...] > > additionally the function arguments are not consistent with > > meson_pinconf_get_drive_strength(): > > - here you pass the pinctrl subsystem specific parameters (enum > > pin_config_param conf) > > - in meson_pinconf_get_drive_strength the conversion for pinctrl > > subsystem specific values (pinconf_to_config_argument) is part of > > meson_pinconf_set > > > for param i'm not sure i understand what you want, if you talk about difference between set_bias and set_drive arg , it's difficult to align it. > > if it's about diff between get_bias and get_drive i think i can return drive stength value instead of using an u16 arg input param. let me use an example to better explain what I mean. meson_pinconf_set(struct pinctrl_dev *pcdev, unsigned int pin, unsigned long *configs, unsigned num_configs) -> this uses parameters from the pinctrl subsystem only (struct pinctrl_dev, configs/pinconf_to_config_argument) meson_pinconf_get_pull(struct meson_pinctrl *pc, unsigned int pin) meson_pinconf_set_drive_strength(struct meson_pinctrl *pc, unsigned int pin, u16 drive_strength_ua) -> these use hardware-specific parameters only (struct meson_pinctrl, [drive strength in uA]) meson_pinconf_set_bias(struct meson_pinctrl *pc, unsigned int pin, enum pin_config_param conf) -> this mixes hardware-specific parameters (struct meson_pinctrl) with parameters from the pinctrl subsystem (enum pin_config_param conf) > > I'm wondering whether two separate functions > > (meson_pinconf_disable_bias and meson_pinconf_enable_bias) would make > > things easier to read. I haven't tried whether this would really make > > things better, so I'd like to hear your opinion on this Guillaume! > > > no special opinion on this, if you think it's better for understanding i can separate set_bias function. this goes with the mixed parameters from my previous comment: if we stick to the way these "private" functions are defined the "set bias" function parameters should be hardware/driver specific (in other words: don't use enum pin_config_param conf) so my initial idea was to keep the switch/case for enum pin_config_param in meson_pinconf_set and to have these function declarations: - meson_pinconf_disable_bias(struct meson_pinctrl *pc, unsigned int pin) - meson_pinconf_enable_bias(struct meson_pinctrl *pc, unsigned int pin, bool pull_up) and to make it clear: I haven't tested whether this *really* looks better when fully implemented. let me know what you think - I'm happy with anything that others will understand in the end (to make easy to catch potential breakage in a code-review ;)) > > > > > [...] > >> +static int meson_pinconf_set_drive_strength(struct meson_pinctrl *pc, > >> + unsigned int pin, u16 arg) > >> { > >> - struct meson_pinctrl *pc = pinctrl_dev_get_drvdata(pcdev); > >> struct meson_bank *bank; > >> - enum pin_config_param param; > >> unsigned int reg, bit; > >> - int i, ret; > >> + unsigned int ds_val; > >> + int ret; > >> + > >> + if (!pc->reg_ds) { > >> + dev_err(pc->dev, "drive-strength not supported\n"); > >> + return -ENOTSUPP; > > in meson_pinconf_set() we don't complain (with a dev_err) for this case. > > I'm not sure what the best-practice is for the pinctrl subsystem, > > maybe Linus can comment on this > > > > this check is to be sure it's possible to set drive stength, > > if no register bank is setting in DT but drive-stength properties are setting on pins i need to generate an error > > because something is wrong. OK, I see, there are two different use-cases: - meson_pinconf_set returns -ENOTSUPP in the "default" case if the pin_config_param is not supported but no error message - we don't differentiate between SoCs PIN_CONFIG_DRIVE_STRENGTH_UA is always delegated from meson_pinconf_set() to meson_pinconf_set_drive_strength(). you also return -ENOTSUPP and print an error message what I meant with my original comment is that there are two different "-ENOTSUPP" cases but only one prints an error. I don't know if there are any rules in the pinctrl subsystem how these cases should be implemented, maybe Linus W. can give his feedback on this topic. > > >> + } > >> > >> ret = meson_get_bank(pc, pin, &bank); > >> if (ret) > >> return ret; > >> > >> + meson_calc_reg_and_bit(bank, pin, REG_DS, ®, &bit); > >> + bit = bit << 1; > >> + > >> + if (arg <= 500) { > >> + ds_val = MESON_PINCONF_DRV_500UA; > >> + } else if (arg <= 2500) { > >> + ds_val = MESON_PINCONF_DRV_2500UA; > >> + } else if (arg <= 3000) { > >> + ds_val = MESON_PINCONF_DRV_3000UA; > >> + } else if (arg <= 4000) { > >> + ds_val = MESON_PINCONF_DRV_4000UA; > >> + } else { > >> + dev_warn_once(pc->dev, > >> + "pin %u: invalid drive-strength : %d , default to 4mA\n", > >> + pin, arg); > >> + ds_val = MESON_PINCONF_DRV_4000UA; > > why not return -EINVAL here? (my assumption is that the pinctrl > > subsystem would like to have -EINVAL instead of drivers doing > > fallbacks if the values are out-of-range, but I'm not 100% sure about > > this) > > > > [...] > > > i choose to set a default value instead of generating an error, > > in this case it's only if you ask a value upper than 4000uA so it's not really a risk to set 4000uA by default. in that case I'm fine with it if Linus W. is happy as well (I'm not sure if there are any rules about "fallback values" in the pinctrl subsystem) > > >> +static int meson_pinconf_get_drive_strength(struct meson_pinctrl *pc, > >> + unsigned int pin, u16 *arg) > >> +{ > >> + struct meson_bank *bank; > >> + unsigned int reg, bit; > >> + unsigned int val; > >> + int ret; > >> + > > do you need to return -ENOTSUPP here if pc->reg_ds is NULL, similar to > > what you already have in meson_pinconf_set_drive_strength()? > > depending of linux comment on your feedback on meson_pinconf_set_drive_strength great, thank you! Martin _______________________________________________ linux-amlogic mailing list linux-amlogic@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-amlogic ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v2 4/4] pinctrl: meson: add support of drive-strength-uA 2019-04-30 20:28 ` Martin Blumenstingl @ 2019-05-06 13:16 ` guillaume La Roque 0 siblings, 0 replies; 18+ messages in thread From: guillaume La Roque @ 2019-05-06 13:16 UTC (permalink / raw) To: Martin Blumenstingl Cc: mark.rutland, devicetree, khilman, linus.walleij, linux-kernel, linux-gpio, robh+dt, linux-amlogic hi Martin, On 4/30/19 10:28 PM, Martin Blumenstingl wrote: > Hi Guillaume, > > On Tue, Apr 30, 2019 at 9:20 AM guillaume La Roque > <glaroque@baylibre.com> wrote: >> Hi Martin, >> >> >> thanks for your feedback. >> >> >> On 4/27/19 9:44 PM, Martin Blumenstingl wrote: >>> Hi Guillaume, >>> >>> On Thu, Apr 18, 2019 at 2:48 PM Guillaume La Roque >>> <glaroque@baylibre.com> wrote: >>>> drive-strength-uA is a new feature needed for G12A SoC. >>>> the default DS setting after boot is usually 500uA and it is not enough for >>>> many functions. We need to be able to set the drive strength to reliably >>>> enable things like MMC, I2C, etc ... >>>> >>>> Signed-off-by: Guillaume La Roque <glaroque@baylibre.com> >>> I gave this a go on Meson8m2 (meaning I applied all four patches from >>> this series and booted the result on my board): >>> [Meson8m2 doesn't support drive strength and still boots without any >>> crashes or obvious regressions] >>> Tested-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com> >>> >>>> --- >>>> drivers/pinctrl/meson/pinctrl-meson-g12a.c | 36 ++--- >>>> drivers/pinctrl/meson/pinctrl-meson.c | 166 ++++++++++++++++----- >>>> drivers/pinctrl/meson/pinctrl-meson.h | 20 ++- >>> personally I would have split this into two separate patches: >>> - one for the generic pinctrl-meson part which adds drive-strength-uA support >>> - another patch for enabling this on G12A >>> >>> if nobody else wants you to split this then it's fine for me as well >> >> why not if i send new series i will do. > great, thank you > > [...] >>> additionally the function arguments are not consistent with >>> meson_pinconf_get_drive_strength(): >>> - here you pass the pinctrl subsystem specific parameters (enum >>> pin_config_param conf) >>> - in meson_pinconf_get_drive_strength the conversion for pinctrl >>> subsystem specific values (pinconf_to_config_argument) is part of >>> meson_pinconf_set >> >> for param i'm not sure i understand what you want, if you talk about difference between set_bias and set_drive arg , it's difficult to align it. >> >> if it's about diff between get_bias and get_drive i think i can return drive stength value instead of using an u16 arg input param. > let me use an example to better explain what I mean. > > meson_pinconf_set(struct pinctrl_dev *pcdev, unsigned int pin, > unsigned long *configs, unsigned num_configs) > -> this uses parameters from the pinctrl subsystem only (struct > pinctrl_dev, configs/pinconf_to_config_argument) > > meson_pinconf_get_pull(struct meson_pinctrl *pc, unsigned int pin) > meson_pinconf_set_drive_strength(struct meson_pinctrl *pc, unsigned > int pin, u16 drive_strength_ua) > -> these use hardware-specific parameters only (struct meson_pinctrl, > [drive strength in uA]) > > meson_pinconf_set_bias(struct meson_pinctrl *pc, unsigned int pin, > enum pin_config_param conf) > -> this mixes hardware-specific parameters (struct meson_pinctrl) with > parameters from the pinctrl subsystem (enum pin_config_param conf) ok i see, it fix in next series. > >>> I'm wondering whether two separate functions >>> (meson_pinconf_disable_bias and meson_pinconf_enable_bias) would make >>> things easier to read. I haven't tried whether this would really make >>> things better, so I'd like to hear your opinion on this Guillaume! >> >> no special opinion on this, if you think it's better for understanding i can separate set_bias function. > this goes with the mixed parameters from my previous comment: if we > stick to the way these "private" functions are defined the "set bias" > function parameters should be hardware/driver specific (in other > words: don't use enum pin_config_param conf) > so my initial idea was to keep the switch/case for enum > pin_config_param in meson_pinconf_set and to have these function > declarations: > - meson_pinconf_disable_bias(struct meson_pinctrl *pc, unsigned int pin) > - meson_pinconf_enable_bias(struct meson_pinctrl *pc, unsigned int > pin, bool pull_up) > > and to make it clear: I haven't tested whether this *really* looks > better when fully implemented. > let me know what you think - I'm happy with anything that others will > understand in the end (to make easy to catch potential breakage in a > code-review ;)) what i would like to say is it ok for me, it's better to review. i will do one patch for enable/disable bias function, one for adding drive-strength function and one for adding G12A part. >>> [...] >>>> +static int meson_pinconf_set_drive_strength(struct meson_pinctrl *pc, >>>> + unsigned int pin, u16 arg) >>>> { >>>> - struct meson_pinctrl *pc = pinctrl_dev_get_drvdata(pcdev); >>>> struct meson_bank *bank; >>>> - enum pin_config_param param; >>>> unsigned int reg, bit; >>>> - int i, ret; >>>> + unsigned int ds_val; >>>> + int ret; >>>> + >>>> + if (!pc->reg_ds) { >>>> + dev_err(pc->dev, "drive-strength not supported\n"); >>>> + return -ENOTSUPP; >>> in meson_pinconf_set() we don't complain (with a dev_err) for this case. >>> I'm not sure what the best-practice is for the pinctrl subsystem, >>> maybe Linus can comment on this >>> >> this check is to be sure it's possible to set drive stength, >> >> if no register bank is setting in DT but drive-stength properties are setting on pins i need to generate an error >> >> because something is wrong. > OK, I see, there are two different use-cases: > - meson_pinconf_set returns -ENOTSUPP in the "default" case if the > pin_config_param is not supported but no error message > - we don't differentiate between SoCs PIN_CONFIG_DRIVE_STRENGTH_UA is > always delegated from meson_pinconf_set() to > meson_pinconf_set_drive_strength(). you also return -ENOTSUPP and > print an error message > > what I meant with my original comment is that there are two different > "-ENOTSUPP" cases but only one prints an error. > I don't know if there are any rules in the pinctrl subsystem how these > cases should be implemented, maybe Linus W. can give his feedback on > this topic. i just add error message for drive strength because it's an optional feature, people can forget to set register part and just set pin part so they can see why it's not working. >>>> + } >>>> >>>> ret = meson_get_bank(pc, pin, &bank); >>>> if (ret) >>>> return ret; >>>> >>>> + meson_calc_reg_and_bit(bank, pin, REG_DS, ®, &bit); >>>> + bit = bit << 1; >>>> + >>>> + if (arg <= 500) { >>>> + ds_val = MESON_PINCONF_DRV_500UA; >>>> + } else if (arg <= 2500) { >>>> + ds_val = MESON_PINCONF_DRV_2500UA; >>>> + } else if (arg <= 3000) { >>>> + ds_val = MESON_PINCONF_DRV_3000UA; >>>> + } else if (arg <= 4000) { >>>> + ds_val = MESON_PINCONF_DRV_4000UA; >>>> + } else { >>>> + dev_warn_once(pc->dev, >>>> + "pin %u: invalid drive-strength : %d , default to 4mA\n", >>>> + pin, arg); >>>> + ds_val = MESON_PINCONF_DRV_4000UA; >>> why not return -EINVAL here? (my assumption is that the pinctrl >>> subsystem would like to have -EINVAL instead of drivers doing >>> fallbacks if the values are out-of-range, but I'm not 100% sure about >>> this) >>> >>> [...] >> >> i choose to set a default value instead of generating an error, >> >> in this case it's only if you ask a value upper than 4000uA so it's not really a risk to set 4000uA by default. > in that case I'm fine with it if Linus W. is happy as well (I'm not > sure if there are any rules about "fallback values" in the pinctrl > subsystem) > >>>> +static int meson_pinconf_get_drive_strength(struct meson_pinctrl *pc, >>>> + unsigned int pin, u16 *arg) >>>> +{ >>>> + struct meson_bank *bank; >>>> + unsigned int reg, bit; >>>> + unsigned int val; >>>> + int ret; >>>> + >>> do you need to return -ENOTSUPP here if pc->reg_ds is NULL, similar to >>> what you already have in meson_pinconf_set_drive_strength()? >> depending of linux comment on your feedback on meson_pinconf_set_drive_strength > great, thank you! > > > Martin Guillaume _______________________________________________ linux-amlogic mailing list linux-amlogic@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-amlogic ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v2 0/4] Add drive-strength in Meson pinctrl driver 2019-04-18 12:47 [PATCH v2 0/4] Add drive-strength in Meson pinctrl driver Guillaume La Roque ` (3 preceding siblings ...) 2019-04-18 12:47 ` [PATCH v2 4/4] pinctrl: meson: add support of drive-strength-uA Guillaume La Roque @ 2019-04-23 11:13 ` Linus Walleij 2019-04-27 19:46 ` Martin Blumenstingl 4 siblings, 1 reply; 18+ messages in thread From: Linus Walleij @ 2019-04-23 11:13 UTC (permalink / raw) To: Guillaume La Roque, Martin Blumenstingl Cc: Mark Rutland, open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, Kevin Hilman, linux-kernel, open list:GPIO SUBSYSTEM, Rob Herring, open list:ARM/Amlogic Meson... On Thu, Apr 18, 2019 at 2:48 PM Guillaume La Roque <glaroque@baylibre.com> wrote: > The purpose of this patchset is to add drive-strength support in meson pinconf > driver. This is a new feature that was added on the g12a. It is critical for us > to support this since many functions are failing with default pad drive-strength. > > The value achievable by the SoC are 0.5mA, 2.5mA, 3mA and 4mA and the DT property > 'drive-strength' is expressed in mA. > So this patch add another generic property "drive-strength-uA". The change to do so > would be minimal and could be benefit to other platforms later on. > > Cheers > Guillaume This needs to be reviewed by Martin Blumenstigl, Martin can you have a look at this patch set? If you don't have the patches I think Guillaume can resend them. Yours, Linus Walleij _______________________________________________ linux-amlogic mailing list linux-amlogic@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-amlogic ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v2 0/4] Add drive-strength in Meson pinctrl driver 2019-04-23 11:13 ` [PATCH v2 0/4] Add drive-strength in Meson pinctrl driver Linus Walleij @ 2019-04-27 19:46 ` Martin Blumenstingl 0 siblings, 0 replies; 18+ messages in thread From: Martin Blumenstingl @ 2019-04-27 19:46 UTC (permalink / raw) To: Linus Walleij Cc: Mark Rutland, open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, Kevin Hilman, linux-kernel, open list:GPIO SUBSYSTEM, Rob Herring, Guillaume La Roque, open list:ARM/Amlogic Meson... Hi Linus, On Tue, Apr 23, 2019 at 1:13 PM Linus Walleij <linus.walleij@linaro.org> wrote: > > On Thu, Apr 18, 2019 at 2:48 PM Guillaume La Roque > <glaroque@baylibre.com> wrote: > > > The purpose of this patchset is to add drive-strength support in meson pinconf > > driver. This is a new feature that was added on the g12a. It is critical for us > > to support this since many functions are failing with default pad drive-strength. > > > > The value achievable by the SoC are 0.5mA, 2.5mA, 3mA and 4mA and the DT property > > 'drive-strength' is expressed in mA. > > So this patch add another generic property "drive-strength-uA". The change to do so > > would be minimal and could be benefit to other platforms later on. > > > > Cheers > > Guillaume > > This needs to be reviewed by Martin Blumenstigl, Martin can you have a look > at this patch set? If you don't have the patches I think Guillaume can > resend them. thank you for the reminder - I just sent my feedback. can you please also have a look at patch 4/4? there are two cases where I'm not sure what the best practice of the pinctrl subsystem is Regards Martin _______________________________________________ linux-amlogic mailing list linux-amlogic@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-amlogic ^ permalink raw reply [flat|nested] 18+ messages in thread
end of thread, other threads:[~2019-05-06 13:16 UTC | newest] Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-04-18 12:47 [PATCH v2 0/4] Add drive-strength in Meson pinctrl driver Guillaume La Roque 2019-04-18 12:47 ` [PATCH v2 1/4] dt-bindings: pinctrl: add a 'drive-strength-uA' property Guillaume La Roque 2019-04-23 11:13 ` Linus Walleij 2019-04-27 19:19 ` Martin Blumenstingl 2019-04-29 19:19 ` Kevin Hilman 2019-04-30 15:12 ` Rob Herring 2019-04-30 15:24 ` guillaume La Roque 2019-04-18 12:47 ` [PATCH v2 2/4] pinctrl: generic: add new 'drive-strength-uA' property support Guillaume La Roque 2019-04-18 12:47 ` [PATCH v2 3/4] dt-bindings: pinctrl: meson: Add drive-strength-uA property Guillaume La Roque 2019-04-27 19:21 ` Martin Blumenstingl 2019-04-30 6:38 ` guillaume La Roque 2019-04-18 12:47 ` [PATCH v2 4/4] pinctrl: meson: add support of drive-strength-uA Guillaume La Roque 2019-04-27 19:44 ` Martin Blumenstingl 2019-04-30 7:20 ` guillaume La Roque 2019-04-30 20:28 ` Martin Blumenstingl 2019-05-06 13:16 ` guillaume La Roque 2019-04-23 11:13 ` [PATCH v2 0/4] Add drive-strength in Meson pinctrl driver Linus Walleij 2019-04-27 19:46 ` Martin Blumenstingl
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).