[v2,4/8] pinctrl: mvebu: pinctrl driver for 98DX2530 SoC

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

On Tue, Mar 15, 2022 at 10:31:39AM +1300, 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>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

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

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

The Marvell family of SoC which this embedded SoC borrows HW blocks
from can boot using ACPI. I doubt anybody would boot this particularly
SoC using ACPI, but the drivers Chris copied probably do build !OF for
when ACPI is in us.

     Andrew

On 15/03/2022 15:33, Andrew Lunn wrote:
>>> +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.
> 
> The Marvell family of SoC which this embedded SoC borrows HW blocks
> from can boot using ACPI. I doubt anybody would boot this particularly
> SoC using ACPI, but the drivers Chris copied probably do build !OF for
> when ACPI is in us.

What I wanted to say - current setting should cause warnings. Therefore
choose:
1. For ACPI && !OF this should be still without of_match_ptr, to allow
ACPI matching by OF (PRP0001).
2. For !OF with of_match_ptr() (weird setup... how to match then?) the
ac5_pinctrl_of_match should be marked as maybe unused.


Best regards,
Krzysztof