[U-Boot] [PATCH v3 4/5] ARM: imx: pinctrl: Add support for i.MX28 mxs pinctrl driver

[Marek Vasut]

On 6/17/19 9:43 AM, Lukasz Majewski wrote:
> Hi Marek,
> 
>> On 6/16/19 12:34 AM, Lukasz Majewski wrote:
>>> The code responsible for setting proper values in the MUX registers
>>> (in the mxs_pinctrl_set_state()) has been ported from Barebox
>>> project (branch: master, SHA1:
>>> eb3b0f7414cd8102844dd16b1c789e445e8947f8, file:
>>> drivers/pinctrl/pinctrl-mxs.c).  
>>
>> The format of a commit, when referenced, is documented here:
>> https://www.kernel.org/doc/html/v4.12/process/submitting-patches.html
>> e.g. e21d2170f366 ("video: remove unnecessary platform_set_drvdata()")
>>
>> Although. maybe you should port this from Linux instead ?
> 
> The code from linux is a bit more convoluted and verbose. I may re-look
> into the kernel version.

Please do

>>> As the pinctrl node in the imx28.dtsi file has gpio pins nodes as
>>> subnodes, it was necessary to use 'dm_scan_fdt_dev()' (as a .bind
>>> method) to also make them 'visible' by the DM's "gpio_mxs" driver.  
>>
>> Look at drivers/pinctrl/renesas/pfc-r7s72100.c r7s72100_pfc_probe() ,
>> I think that one deals with the exact same problem .
> 
> It looks like this is the same problem.
> 
> However, the dm_scan_fdt_dev() is exactly for that, when combined
> with .bind method from DM.

Ah, because you have a compat string for the GPIO controllers, right.
Then please ignore this comment, this is better solution.

>>> Signed-off-by: Lukasz Majewski <lukma at denx.de>
>>>
>>> ---
>>>
>>> Changes in v3:
>>> - Set more apropriate tags
>>>
>>> Changes in v2: None
>>>
>>>  drivers/pinctrl/nxp/Kconfig       |  10 +++
>>>  drivers/pinctrl/nxp/Makefile      |   1 +
>>>  drivers/pinctrl/nxp/pinctrl-mxs.c | 164
>>> ++++++++++++++++++++++++++++++++++++++ 3 files changed, 175
>>> insertions(+) create mode 100644 drivers/pinctrl/nxp/pinctrl-mxs.c
>>>
>>> diff --git a/drivers/pinctrl/nxp/Kconfig
>>> b/drivers/pinctrl/nxp/Kconfig index 61f93be42d..f2e67ca231 100644
>>> --- a/drivers/pinctrl/nxp/Kconfig
>>> +++ b/drivers/pinctrl/nxp/Kconfig
>>> @@ -89,6 +89,16 @@ config PINCTRL_IMX8M
>>>  	  only parses the 'fsl,pins' property and configure related
>>>  	  registers.
>>>  
>>> +config PINCTRL_MXS
>>> +	bool "NXP MXS pinctrl driver"
>>> +	depends on ARCH_MX28 && PINCTRL_FULL
>>> +	help
>>> +	  Say Y here to enable the i.MX mxs pinctrl driver
>>> +
>>> +	  This option provides a simple pinctrl driver for i.MX
>>> mxs SoC
>>> +	  familiy, e.g. i.MX28. This feature depends on device tree
>>> +	  configuration.
>>> +
>>>  config PINCTRL_VYBRID
>>>  	bool "Vybrid (vf610) pinctrl driver"
>>>  	depends on ARCH_VF610 && PINCTRL_FULL
>>> diff --git a/drivers/pinctrl/nxp/Makefile
>>> b/drivers/pinctrl/nxp/Makefile index b340d9448a..b86448aac9 100644
>>> --- a/drivers/pinctrl/nxp/Makefile
>>> +++ b/drivers/pinctrl/nxp/Makefile
>>> @@ -6,4 +6,5 @@ obj-$(CONFIG_PINCTRL_IMX7ULP)		+=
>>> pinctrl-imx7ulp.o obj-$(CONFIG_PINCTRL_IMX_SCU)		+=
>>> pinctrl-scu.o obj-$(CONFIG_PINCTRL_IMX8)		+=
>>> pinctrl-imx8.o obj-$(CONFIG_PINCTRL_IMX8M)		+=
>>> pinctrl-imx8m.o +obj-$(CONFIG_PINCTRL_MXS)		+=
>>> pinctrl-mxs.o obj-$(CONFIG_PINCTRL_VYBRID)		+=
>>> pinctrl-vf610.o diff --git a/drivers/pinctrl/nxp/pinctrl-mxs.c
>>> b/drivers/pinctrl/nxp/pinctrl-mxs.c new file mode 100644
>>> index 0000000000..42b1b7998b
>>> --- /dev/null
>>> +++ b/drivers/pinctrl/nxp/pinctrl-mxs.c
>>> @@ -0,0 +1,164 @@
>>> +// SPDX-License-Identifier: GPL-2.0
>>> +/*
>>> + * Copyright (C) 2019 DENX Software Engineering
>>> + * Lukasz Majewski, DENX Software Engineering, lukma at denx.de
>>> + *
>>> + * This work is based on drivers/pinctrl/pinctrl-mxs.c
>>> + * SHA1: eb3b0f7414cd8102844dd16b1c789e445e8947f8
>>> + * from Barebox project.
>>> + *
>>> + * Author of original Barebox pinctrl-mxs.c:
>>> + * Copyright (c) 2015 Sascha Hauer <s.hauer at pengutronix.de>
>>> + */
>>> +
>>> +#include <common.h>
>>> +#include <linux/io.h>
>>> +#include <linux/err.h>
>>> +#include <dm.h>
>>> +#include <dm/pinctrl.h>
>>> +#include <dm/read.h>
>>> +
>>> +DECLARE_GLOBAL_DATA_PTR;
>>> +
>>> +#define PINID(bank, pin)	((bank) * 32 + (pin))
>>> +#define MUXID_TO_PINID(m)	PINID((m) >> 12 & 0xf, (m) >> 4 &
>>> 0xff) +#define MUXID_TO_MUXSEL(m)	((m) & 0xf)
>>> +#define PINID_TO_BANK(p)	((p) >> 5)
>>> +#define PINID_TO_PIN(p)	((p) % 32)
>>> +
>>> +#define STMP_OFFSET_REG_SET     0x4
>>> +#define STMP_OFFSET_REG_CLR     0x8
>>> +#define STMP_OFFSET_REG_TOG     0xc
>>> +
>>> +struct mxs_pinctrl_priv {
>>> +	void __iomem *base;
>>> +};
>>> +
>>> +static int mxs_pinctrl_set_state(struct udevice *dev, struct
>>> udevice *config) +{
>>> +	int ma_present = 0, vol_present = 0, pull_present = 0;
>>> +	struct mxs_pinctrl_priv *iomux = dev_get_priv(dev);
>>> +	u32 *pin_data, val, ma, vol, pull;
>>> +	int npins, size, i, ret;
>>> +
>>> +	debug("\n%s: set state: %s\n", __func__, config->name);
>>> +
>>> +	size = dev_read_size(config, "fsl,pinmux-ids");
>>> +	if (size < 0)
>>> +		return size;
>>> +
>>> +	if (!size || size % 4) {
>>> +		dev_err(dev, "Invalid fsl,pinmux-ids property in
>>> %s\n",
>>> +			config->name);
>>> +		return -EINVAL;
>>> +	}
>>> +
>>> +	npins = size / 4;
>>> +
>>> +	pin_data = devm_kzalloc(dev, size, 0);  
>>
>> Is this ever free'd() ?
>>
> 
> Ach... right the devm_kfree() shall be used on the exit from the
> function.
> 
>> [...]
>>
> 
> 
> 
> 
> Best regards,
> 
> Lukasz Majewski
> 
> --
> 
> DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
> HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
> Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma at denx.de
> 


-- 
Best regards,
Marek Vasut