Linux-ARM-Kernel Archive on lore.kernel.org
 help / color / Atom feed
From: Linus Walleij <linus.walleij@linaro.org>
To: Stefan Wahren <wahrenst@gmx.net>
Cc: Mark Rutland <mark.rutland@arm.com>,
	Ulf Hansson <ulf.hansson@linaro.org>,
	Florian Fainelli <f.fainelli@gmail.com>,
	Scott Branden <sbranden@broadcom.com>,
	Stephen Boyd <sboyd@kernel.org>, Ray Jui <rjui@broadcom.com>,
	Michael Turquette <mturquette@baylibre.com>,
	Al Cooper <alcooperx@gmail.com>,
	Adrian Hunter <adrian.hunter@intel.com>,
	Eric Anholt <eric@anholt.net>, Rob Herring <robh+dt@kernel.org>,
	bcm-kernel-feedback-list <bcm-kernel-feedback-list@broadcom.com>,
	Mark Brown <broonie@kernel.org>,
	linux-rpi-kernel <linux-rpi-kernel@lists.infradead.org>,
	Nicolas Saenz Julienne <nsaenzjulienne@suse.de>,
	Linux ARM <linux-arm-kernel@lists.infradead.org>
Subject: Re: [PATCH RFC 10/18] pinctrl: bcm2835: bcm7211: Add support for 7211 pull-up functionality
Date: Sat, 3 Aug 2019 00:05:30 +0200
Message-ID: <CACRpkdaN-8e9vNn_Q=6TBwUZnfDwaha3Lad-z9ycLykpPusmTQ@mail.gmail.com> (raw)
In-Reply-To: <1563393026-17118-11-git-send-email-wahrenst@gmx.net>

Hi Stefan!

Thanks for your patch!

On Wed, Jul 17, 2019 at 9:50 PM Stefan Wahren <wahrenst@gmx.net> wrote:

> From: Al Cooper <alcooperx@gmail.com>
>
> The 7211 has a new way of selecting the pull-up/pull-down setting
> for a GPIO pin. The registers used for the bcm2837, GP_PUD and
> GP_PUDCLKn0, are no longer connected. A new set of registers,
> GP_GPIO_PUP_PDN_CNTRL_REGx must be used. This commit will add
> a new compatible string "brcm,bcm7211-gpio" and the kernel
> driver will use it to select which method is used to select
> pull-up/pull-down.
>
> Signed-off-by: Al Cooper <alcooperx@gmail.com>
> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>

(...)
>  /* argument: bcm2835_pinconf_pull */
>  #define BCM2835_PINCONF_PARAM_PULL     (PIN_CONFIG_END + 1)

Why this derivative of PULL?

What do you need that these standard configs and their parameters
doesn't already cover:

 * @PIN_CONFIG_BIAS_PULL_DOWN: the pin will be pulled down (usually with high
 *      impedance to GROUND). If the argument is != 0 pull-down is enabled,
 *      if it is 0, pull-down is total, i.e. the pin is connected to GROUND.
(...)
 * @PIN_CONFIG_BIAS_PULL_UP: the pin will be pulled up (usually with high
 *      impedance to VDD). If the argument is != 0 pull-up is enabled,
 *      if it is 0, pull-up is total, i.e. the pin is connected to VDD.

The argument isn't really defined but at one time I thought it should be
in ohms, but proprietary values are possible I guess.

 +static int bcm7211_pinconf_set(struct pinctrl_dev *pctldev,
> +                              unsigned int pin, unsigned long *configs,
> +                              unsigned int num_configs)
> +{
(...)
> +       for (i = 0; i < num_configs; i++) {
> +               param = pinconf_to_config_param(configs[i]);
> +               if (param != BCM2835_PINCONF_PARAM_PULL)
> +                       return -EINVAL;

This seems unnecessary, like some "hello I am here with magic value" instead
of just using the standard pull up/down configs and some ohm or custom
value.

> +               arg = pinconf_to_config_argument(configs[i]);
> +
> +               /* convert to 7211 value */
> +               switch (arg) {
> +               case PIN_CONFIG_BIAS_DISABLE:
> +                       arg = BCM7211_PINCONFIG_PULL_NONE;
> +                       break;
> +               case PIN_CONFIG_BIAS_PULL_DOWN:
> +                       arg = BCM7211_PINCONFIG_PULL_DOWN;
> +                       break;
> +               case PIN_CONFIG_BIAS_PULL_UP:
> +                       arg = BCM7211_PINCONFIG_PULL_UP;
> +                       break;
> +               }

This switch is fine, but lose the arg translation and just use the arg
as-is, I don't see the point.

The interpretation is anyway clear from:

> +               .compatible = "brcm,bcm7211-gpio",
> +               .data = &bcm7211_pinconf_ops,

This compatible string, and the variant of SoC is what you should
store in your state container instead of custom arguments, like

> +       match = of_match_node(bcm2835_pinctrl_match, pdev->dev.of_node);
> +       if (match) {
> +               bcm2835_pinctrl_desc.confops =
> +                       (const struct pinconf_ops *)match->data;
> +       }

Instead of storing the confops in the .data store just an enum for the SoC
and make the code adapt to which SoC it is. Storing different ops may make
sense but it seems to just be adding kludges here in the argument to
work around that later on the driver doesn't know which SoC it is
running on.

Yours,
Linus Walleij

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

  reply index

Thread overview: 32+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-07-17 19:50 [PATCH RFC 00/18] ARM: Add minimal Raspberry Pi 4 support Stefan Wahren
2019-07-17 19:50 ` [PATCH RFC 01/18] ARM: bcm283x: Reduce register ranges for UART, SPI and I2C Stefan Wahren
2019-07-17 19:50 ` [PATCH RFC 02/18] ARM: bcm2835: DMA can only address 1GB Stefan Wahren
2019-07-17 19:50 ` [PATCH RFC 03/18] ARM: dts: bcm283x: Move BCM2835/6/7 specific to bcm2835-common.dtsi Stefan Wahren
2019-07-17 19:50 ` [PATCH RFC 04/18] ARM: dts: bcm283x: Define MMC interfaces at board level Stefan Wahren
2019-07-17 19:50 ` [PATCH RFC 05/18] ARM: dts: bcm283x: Define memory " Stefan Wahren
2019-07-17 19:50 ` [PATCH RFC 06/18] dt-bindings: bcm2835-cprman: Add bcm2838 support Stefan Wahren
2019-07-17 19:50 ` [PATCH RFC 07/18] clk: bcm2835: Add BCM2838_CLOCK_EMMC2 support Stefan Wahren
2019-07-18  8:47   ` Matthias Brugger
2019-07-18 16:51     ` Florian Fainelli
2019-07-18 18:37       ` Eric Anholt
2019-07-19 16:40         ` Stefan Wahren
2019-07-19 19:49           ` Eric Anholt
2019-07-17 19:50 ` [PATCH RFC 08/18] dt-bindings: sdhci-iproc: Add brcm,bcm2838-emmc2 Stefan Wahren
2019-07-17 19:50 ` [PATCH RFC 09/18] mmc: sdhci-iproc: Add support for emmc2 of the BCM2838 Stefan Wahren
2019-07-18  8:34   ` Matthias Brugger
2019-07-18 16:40     ` Florian Fainelli
2019-07-18 16:48       ` Matthias Brugger
2019-07-18 16:52         ` Florian Fainelli
2019-07-18 17:46         ` Stefan Wahren
2019-07-18 18:09         ` Stefan Wahren
2019-07-18 18:19           ` Andrei Gherzan
2019-07-17 19:50 ` [PATCH RFC 10/18] pinctrl: bcm2835: bcm7211: Add support for 7211 pull-up functionality Stefan Wahren
2019-08-02 22:05   ` Linus Walleij [this message]
2019-07-17 19:50 ` [PATCH RFC 11/18] pinctrl: bcm2835: Fix BCM7211 pinconf handling Stefan Wahren
2019-08-02 22:06   ` Linus Walleij
2019-07-17 19:50 ` [PATCH RFC 12/18] dt-bindings: pinctrl: bcm2835: Add brcm, bcm2838 compatible Stefan Wahren
2019-07-17 19:50 ` [PATCH RFC 13/18] pinctrl: bcm2835: Add BCM2838 support Stefan Wahren
2019-08-02 22:08   ` Linus Walleij
2019-07-18 18:45 ` [PATCH RFC 00/18] ARM: Add minimal Raspberry Pi 4 support Eric Anholt
2019-07-18 18:58   ` Florian Fainelli
2019-07-19 13:18   ` Nicolas Saenz Julienne

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to='CACRpkdaN-8e9vNn_Q=6TBwUZnfDwaha3Lad-z9ycLykpPusmTQ@mail.gmail.com' \
    --to=linus.walleij@linaro.org \
    --cc=adrian.hunter@intel.com \
    --cc=alcooperx@gmail.com \
    --cc=bcm-kernel-feedback-list@broadcom.com \
    --cc=broonie@kernel.org \
    --cc=eric@anholt.net \
    --cc=f.fainelli@gmail.com \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-rpi-kernel@lists.infradead.org \
    --cc=mark.rutland@arm.com \
    --cc=mturquette@baylibre.com \
    --cc=nsaenzjulienne@suse.de \
    --cc=rjui@broadcom.com \
    --cc=robh+dt@kernel.org \
    --cc=sboyd@kernel.org \
    --cc=sbranden@broadcom.com \
    --cc=ulf.hansson@linaro.org \
    --cc=wahrenst@gmx.net \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-ARM-Kernel Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-arm-kernel/0 linux-arm-kernel/git/0.git
	git clone --mirror https://lore.kernel.org/linux-arm-kernel/1 linux-arm-kernel/git/1.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-arm-kernel linux-arm-kernel/ https://lore.kernel.org/linux-arm-kernel \
		linux-arm-kernel@lists.infradead.org infradead-linux-arm-kernel@archiver.kernel.org
	public-inbox-index linux-arm-kernel

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.infradead.lists.linux-arm-kernel


AGPL code for this site: git clone https://public-inbox.org/ public-inbox