Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <04ed13f1-671f-7416-61d0-0bf452ae862e@canonical.com>
Date:   Tue, 15 Mar 2022 11:49:59 +0100
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.5.0
Subject: Re: [PATCH v2 4/8] pinctrl: mvebu: pinctrl driver for 98DX2530 SoC
Content-Language: en-US
To:     Chris Packham <chris.packham@alliedtelesis.co.nz>,
        huziji@marvell.com, ulf.hansson@linaro.org, robh+dt@kernel.org,
        davem@davemloft.net, kuba@kernel.org, linus.walleij@linaro.org,
        catalin.marinas@arm.com, will@kernel.org, andrew@lunn.ch,
        gregory.clement@bootlin.com, sebastian.hesselbarth@gmail.com,
        adrian.hunter@intel.com, thomas.petazzoni@bootlin.com,
        kostap@marvell.com, robert.marko@sartura.hr
Cc:     linux-mmc@vger.kernel.org, devicetree@vger.kernel.org,
        linux-kernel@vger.kernel.org, netdev@vger.kernel.org,
        linux-gpio@vger.kernel.org, linux-arm-kernel@lists.infradead.org
References: <20220314213143.2404162-1-chris.packham@alliedtelesis.co.nz>
 <20220314213143.2404162-5-chris.packham@alliedtelesis.co.nz>
From:   Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com>
In-Reply-To: <20220314213143.2404162-5-chris.packham@alliedtelesis.co.nz>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 14/03/2022 22:31, Chris Packham wrote:
> This pinctrl driver supports the 98DX25xx and 98DX35xx family of chips
> from Marvell. It is based on the Marvell SDK with additions for various
> (non-gpio) pin configurations based on the datasheet.
> 
> Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
> ---
> 
> Notes:
>     Changes in v2:
>     - Make pinctrl a child of a syscon node like the armada-7k-pinctrl
> 
>  drivers/pinctrl/mvebu/Kconfig       |   4 +
>  drivers/pinctrl/mvebu/Makefile      |   1 +
>  drivers/pinctrl/mvebu/pinctrl-ac5.c | 226 ++++++++++++++++++++++++++++
>  3 files changed, 231 insertions(+)
>  create mode 100644 drivers/pinctrl/mvebu/pinctrl-ac5.c
> 
> diff --git a/drivers/pinctrl/mvebu/Kconfig b/drivers/pinctrl/mvebu/Kconfig
> index 0d12894d3ee1..aa5883f09d7b 100644
> --- a/drivers/pinctrl/mvebu/Kconfig
> +++ b/drivers/pinctrl/mvebu/Kconfig
> @@ -45,6 +45,10 @@ config PINCTRL_ORION
>  	bool
>  	select PINCTRL_MVEBU
>  
> +config PINCTRL_AC5
> +	bool
> +	select PINCTRL_MVEBU
> +
>  config PINCTRL_ARMADA_37XX
>  	bool
>  	select GENERIC_PINCONF
> diff --git a/drivers/pinctrl/mvebu/Makefile b/drivers/pinctrl/mvebu/Makefile
> index cd082dca4482..23458ab17c53 100644
> --- a/drivers/pinctrl/mvebu/Makefile
> +++ b/drivers/pinctrl/mvebu/Makefile
> @@ -11,3 +11,4 @@ obj-$(CONFIG_PINCTRL_ARMADA_CP110) += pinctrl-armada-cp110.o
>  obj-$(CONFIG_PINCTRL_ARMADA_XP)  += pinctrl-armada-xp.o
>  obj-$(CONFIG_PINCTRL_ARMADA_37XX)  += pinctrl-armada-37xx.o
>  obj-$(CONFIG_PINCTRL_ORION)  += pinctrl-orion.o
> +obj-$(CONFIG_PINCTRL_AC5) += pinctrl-ac5.o
> diff --git a/drivers/pinctrl/mvebu/pinctrl-ac5.c b/drivers/pinctrl/mvebu/pinctrl-ac5.c
> new file mode 100644
> index 000000000000..8bc0bbff7c1b
> --- /dev/null
> +++ b/drivers/pinctrl/mvebu/pinctrl-ac5.c
> @@ -0,0 +1,226 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Marvell ac5 pinctrl driver based on mvebu pinctrl core
> + *
> + * Copyright (C) 2021 Marvell
> + *
> + * Noam Liron <lnoam@marvell.com>
> + */
> +
> +#include <linux/err.h>
> +#include <linux/init.h>
> +#include <linux/io.h>
> +#include <linux/platform_device.h>
> +#include <linux/of.h>
> +#include <linux/of_device.h>
> +#include <linux/pinctrl/pinctrl.h>
> +
> +#include "pinctrl-mvebu.h"
> +
> +static struct mvebu_mpp_mode ac5_mpp_modes[] = {
> +	MPP_MODE(0,
> +		 MPP_FUNCTION(0, "gpio",  NULL),
> +		 MPP_FUNCTION(1, "sdio",  "d0"),
> +		 MPP_FUNCTION(2, "nand",  "io4")),
> +	MPP_MODE(1,
> +		 MPP_FUNCTION(0, "gpio",  NULL),
> +		 MPP_FUNCTION(1, "sdio",  "d1"),
> +		 MPP_FUNCTION(2, "nand",  "io3")),
> +	MPP_MODE(2,
> +		 MPP_FUNCTION(0, "gpio",  NULL),
> +		 MPP_FUNCTION(1, "sdio",  "d2"),
> +		 MPP_FUNCTION(2, "nand",  "io2")),
> +	MPP_MODE(3,
> +		 MPP_FUNCTION(0, "gpio",  NULL),
> +		 MPP_FUNCTION(1, "sdio",  "d3"),
> +		 MPP_FUNCTION(2, "nand",  "io7")),
> +	MPP_MODE(4,
> +		 MPP_FUNCTION(0, "gpio",  NULL),
> +		 MPP_FUNCTION(1, "sdio",  "d4"),
> +		 MPP_FUNCTION(2, "nand",  "io6"),
> +		 MPP_FUNCTION(3, "uart3", "txd"),
> +		 MPP_FUNCTION(4, "uart2", "txd")),
> +	MPP_MODE(5,
> +		 MPP_FUNCTION(0, "gpio",  NULL),
> +		 MPP_FUNCTION(1, "sdio",  "d5"),
> +		 MPP_FUNCTION(2, "nand",  "io5"),
> +		 MPP_FUNCTION(3, "uart3", "rxd"),
> +		 MPP_FUNCTION(4, "uart2", "rxd")),
> +	MPP_MODE(6,
> +		 MPP_FUNCTION(0, "gpio",  NULL),
> +		 MPP_FUNCTION(1, "sdio",  "d6"),
> +		 MPP_FUNCTION(2, "nand",  "io0"),
> +		 MPP_FUNCTION(3, "i2c1",  "sck")),
> +	MPP_MODE(7,
> +		 MPP_FUNCTION(0, "gpio",  NULL),
> +		 MPP_FUNCTION(1, "sdio",  "d7"),
> +		 MPP_FUNCTION(2, "nand",  "io1"),
> +		 MPP_FUNCTION(3, "i2c1",  "sda")),
> +	MPP_MODE(8,
> +		 MPP_FUNCTION(0, "gpio",  NULL),
> +		 MPP_FUNCTION(1, "sdio",  "clk"),
> +		 MPP_FUNCTION(2, "nand",  "wen")),
> +	MPP_MODE(9,
> +		 MPP_FUNCTION(0, "gpio",  NULL),
> +		 MPP_FUNCTION(1, "sdio",  "cmd"),
> +		 MPP_FUNCTION(2, "nand",  "ale")),
> +	MPP_MODE(10,
> +		 MPP_FUNCTION(0, "gpio",  NULL),
> +		 MPP_FUNCTION(1, "sdio",  "ds"),
> +		 MPP_FUNCTION(2, "nand",  "cle")),
> +	MPP_MODE(11,
> +		 MPP_FUNCTION(0, "gpio",  NULL),
> +		 MPP_FUNCTION(1, "sdio",  "rst"),
> +		 MPP_FUNCTION(2, "nand",  "cen")),
> +	MPP_MODE(12,
> +		 MPP_FUNCTION(0, "gpio",  NULL),
> +		 MPP_FUNCTION(1, "spi0",  "clk")),
> +	MPP_MODE(13,
> +		 MPP_FUNCTION(0, "gpio",  NULL),
> +		 MPP_FUNCTION(1, "spi0",  "csn")),
> +	MPP_MODE(14,
> +		 MPP_FUNCTION(0, "gpio",  NULL),
> +		 MPP_FUNCTION(1, "spi0",  "mosi")),
> +	MPP_MODE(15,
> +		 MPP_FUNCTION(0, "gpio",  NULL),
> +		 MPP_FUNCTION(1, "spi0",  "miso")),
> +	MPP_MODE(16,
> +		 MPP_FUNCTION(0, "gpio",  NULL),
> +		 MPP_FUNCTION(1, "spi0",  "wpn"),
> +		 MPP_FUNCTION(2, "nand",  "ren"),
> +		 MPP_FUNCTION(3, "uart1", "txd")),
> +	MPP_MODE(17,
> +		 MPP_FUNCTION(0, "gpio",  NULL),
> +		 MPP_FUNCTION(1, "spi0",  "hold"),
> +		 MPP_FUNCTION(2, "nand",  "rb"),
> +		 MPP_FUNCTION(3, "uart1", "rxd")),
> +	MPP_MODE(18,
> +		 MPP_FUNCTION(0, "gpio",  NULL),
> +		 MPP_FUNCTION(2, "uart2", "rxd")),
> +	MPP_MODE(19,
> +		 MPP_FUNCTION(0, "gpio",  NULL),
> +		 MPP_FUNCTION(2, "uart2", "txd")),
> +	MPP_MODE(20,
> +		 MPP_FUNCTION(0, "gpio",  NULL),
> +		 MPP_FUNCTION(2, "i2c1",  "sck"),
> +		 MPP_FUNCTION(3, "spi1",  "clk"),
> +		 MPP_FUNCTION(4, "uart3", "txd")),
> +	MPP_MODE(21,
> +		 MPP_FUNCTION(0, "gpio",  NULL),
> +		 MPP_FUNCTION(2, "i2c1",  "sda"),
> +		 MPP_FUNCTION(3, "spi1",  "csn"),
> +		 MPP_FUNCTION(4, "uart3", "rxd")),
> +	MPP_MODE(22,
> +		 MPP_FUNCTION(0, "gpio",  NULL),
> +		 MPP_FUNCTION(3, "spi1",  "mosi")),
> +	MPP_MODE(23,
> +		 MPP_FUNCTION(0, "gpio",  NULL),
> +		 MPP_FUNCTION(3, "spi1",  "miso")),
> +	MPP_MODE(24,
> +		 MPP_FUNCTION(0, "gpio",  NULL),
> +		 MPP_FUNCTION(2, "uart2", "txd")),
> +	MPP_MODE(25,
> +		 MPP_FUNCTION(0, "gpio",  NULL),
> +		 MPP_FUNCTION(2, "uart2", "rxd")),
> +	MPP_MODE(26,
> +		 MPP_FUNCTION(0, "gpio",  NULL),
> +		 MPP_FUNCTION(1, "i2c0",  "sck"),
> +		 MPP_FUNCTION(3, "uart3", "txd")),
> +	MPP_MODE(27,
> +		 MPP_FUNCTION(0, "gpio",  NULL),
> +		 MPP_FUNCTION(1, "i2c0",  "sda"),
> +		 MPP_FUNCTION(3, "uart3", "rxd")),
> +	MPP_MODE(28,
> +		 MPP_FUNCTION(0, "gpio",  NULL),
> +		 MPP_FUNCTION(3, "uart3", "txd")),
> +	MPP_MODE(29,
> +		 MPP_FUNCTION(0, "gpio",  NULL),
> +		 MPP_FUNCTION(3, "uart3", "rxd")),
> +	MPP_MODE(30,
> +		 MPP_FUNCTION(0, "gpio",  NULL)),
> +	MPP_MODE(31,
> +		 MPP_FUNCTION(0, "gpio",  NULL)),
> +	MPP_MODE(32,
> +		 MPP_FUNCTION(0, "gpio",  NULL),
> +		 MPP_FUNCTION(1, "uart0", "txd")),
> +	MPP_MODE(33,
> +		 MPP_FUNCTION(0, "gpio",  NULL),
> +		 MPP_FUNCTION(1, "uart0", "rxd")),
> +	MPP_MODE(34,
> +		 MPP_FUNCTION(0, "gpio",  NULL),
> +		 MPP_FUNCTION(2, "uart3", "rxd")),
> +	MPP_MODE(35,
> +		 MPP_FUNCTION(0, "gpio",  NULL),
> +		 MPP_FUNCTION(2, "uart3", "txd")),
> +	MPP_MODE(36,
> +		 MPP_FUNCTION(0, "gpio",  NULL)),
> +	MPP_MODE(37,
> +		 MPP_FUNCTION(0, "gpio",  NULL)),
> +	MPP_MODE(38,
> +		 MPP_FUNCTION(0, "gpio",  NULL)),
> +	MPP_MODE(39,
> +		 MPP_FUNCTION(0, "gpio",  NULL)),
> +	MPP_MODE(40,
> +		 MPP_FUNCTION(0, "gpio",  NULL)),
> +	MPP_MODE(41,
> +		 MPP_FUNCTION(0, "gpio",  NULL),
> +		 MPP_FUNCTION(4, "uart2", "txd"),
> +		 MPP_FUNCTION(5, "i2c1",  "sck")),
> +	MPP_MODE(42,
> +		 MPP_FUNCTION(0, "gpio",  NULL),
> +		 MPP_FUNCTION(4, "uart2", "rxd"),
> +		 MPP_FUNCTION(5, "i2c1",  "sda")),
> +	MPP_MODE(43,
> +		 MPP_FUNCTION(0, "gpio",  NULL)),
> +	MPP_MODE(44,
> +		 MPP_FUNCTION(0, "gpio",  NULL)),
> +	MPP_MODE(45,
> +		 MPP_FUNCTION(0, "gpio",  NULL)),
> +};
> +
> +static struct mvebu_pinctrl_soc_info ac5_pinctrl_info;

You should not have static/file-scope variables, especially that it is
not actually used in that way.

> +
> +static const struct of_device_id ac5_pinctrl_of_match[] = {
> +	{
> +		.compatible = "marvell,ac5-pinctrl",
> +	},
> +	{ },
> +};
> +
> +static const struct mvebu_mpp_ctrl ac5_mpp_controls[] = {
> +	MPP_FUNC_CTRL(0, 45, NULL, mvebu_regmap_mpp_ctrl), };
> +
> +static struct pinctrl_gpio_range ac5_mpp_gpio_ranges[] = {
> +	MPP_GPIO_RANGE(0,   0,  0, 46), };
> +
> +static int ac5_pinctrl_probe(struct platform_device *pdev)
> +{
> +	struct mvebu_pinctrl_soc_info *soc = &ac5_pinctrl_info;
> +	const struct of_device_id *match =
> +		of_match_device(ac5_pinctrl_of_match, &pdev->dev);

Why is this needed? Unusual, dead-code.

> +
> +	if (!match || !pdev->dev.parent)
> +		return -ENODEV;
> +
> +	soc->variant = 0; /* no variants for ac5 */
> +	soc->controls = ac5_mpp_controls;
> +	soc->ncontrols = ARRAY_SIZE(ac5_mpp_controls);
> +	soc->gpioranges = ac5_mpp_gpio_ranges;
> +	soc->ngpioranges = ARRAY_SIZE(ac5_mpp_gpio_ranges);
> +	soc->modes = ac5_mpp_modes;
> +	soc->nmodes = ac5_mpp_controls[0].npins;
> +
> +	pdev->dev.platform_data = soc;
> +
> +	return mvebu_pinctrl_simple_regmap_probe(pdev, pdev->dev.parent, 0);
> +}
> +
> +static struct platform_driver ac5_pinctrl_driver = {
> +	.driver = {
> +		.name = "ac5-pinctrl",
> +		.of_match_table = of_match_ptr(ac5_pinctrl_of_match),

of_match_ptr() does not look correct for OF-only platform. This should
complain in W=1 compile tests on !OF config.

Best regards,
Krzysztof