On 6/17/19 3:01 PM, Lukasz Majewski wrote:
Hi Marek,
On 6/17/19 10:37 AM, Lukasz Majewski wrote:
Hi Marek,
On 6/16/19 12:34 AM, Lukasz Majewski wrote:
This commit
This is not a commit, it's a change. It only becomes a commit when applied.
adds support for DM in the mxs_gpio.c driver when DM_GPIO is enabled in Kconfig.
But this also adds support for DT probing, which is orthogonal to DM support, yet it's not documented in the commit message.
Ok. Will rewrite the commit message to reflect the changes in the commit.
Signed-off-by: Lukasz Majewski lukma@denx.de
Changes in v3:
- Set more apropriate tags
Changes in v2:
- Use #if !CONFIG_IS_ENABLED(DM_GPIO) instead of plain #ifdef
CONFIG_DM_GPIO
arch/arm/include/asm/arch-mxs/gpio.h | 28 +++++++ drivers/gpio/mxs_gpio.c | 149 +++++++++++++++++++++++++++++++++++ 2 files changed, 177 insertions(+)
diff --git a/arch/arm/include/asm/arch-mxs/gpio.h b/arch/arm/include/asm/arch-mxs/gpio.h index 34fa421945..0c152e25e2 100644 --- a/arch/arm/include/asm/arch-mxs/gpio.h +++ b/arch/arm/include/asm/arch-mxs/gpio.h @@ -15,4 +15,32 @@ void mxs_gpio_init(void); inline void mxs_gpio_init(void) {} #endif
+#if defined(CONFIG_MX28) && CONFIG_IS_ENABLED(DM_GPIO) +/*
- According to i.MX28 Reference Manual:
- 'i.MX28 Applications Processor Reference Manual, Rev. 1, 2010'
- The i.MX28 has following number of GPIOs available:
- Bank 0: 0-28 -> 29 PINS
- Bank 1: 0-31 -> 32 PINS
- Bank 2: 0-27 -> 28 PINS
- Bank 3: 0-30 -> 31 PINS
- Bank 4: 0-20 -> 21 PINS
- */
+#define IMX28_GPIO_BANK0_PIN_NR 29 +#define IMX28_GPIO_BANK1_PIN_NR 32 +#define IMX28_GPIO_BANK2_PIN_NR 28 +#define IMX28_GPIO_BANK3_PIN_NR 31 +#define IMX28_GPIO_BANK4_PIN_NR 21 +#define IMX28_GPIO_BANK_NR 5
Please make a complete conversion, not partial one. You want to use gpio-ranges in DT and then parse the size of each GPIO bank from those gpio-ranges , not hard-code it into the driver.
I would have used the gpio-ranges, but the original Linux code [1] (imx28.dtsi) doesn't define them.
You can add them to imx28-u-boot.dtsi ,
No, IMHO this is not the best solution. My point is:
- i.MX28 driver in Linux is stable and it works (without gpio-ranges).
I do not have any interest in fixing code which is already working. If that were new driver then no issue to use gpio-ranges from the outset. Also if Linux kernel driver would support it - then also no problems with adding it.
Linux is continuously improving, so is U-Boot, code is being constantly rewritten and improved. So this isn't really a convincing argument.
- The proposed code is small - only 24 LOC and doesn't require any
extra parsing of DTS (including pinctrl driver's properties).
Why shall I make the driver more verbose if I can avoid it?
It also adds churn into the driver which does not have to be there. In fact, DT is a hardware description, it should describe hardware and it has facilities to do so -- gpio-ranges . Will you keep adding more and more new macros into the code with every new/old iteration of this GPIO block ?
If you were to put that information into DT, where it belongs, the driver would be much simple(r) and wouldn't grow unnecessarily.
- It is easily reusable in SPL.
And with gpio-ranges it's not ?
and submit patch for mainline Linux, it's easy.
Submitting patches to Linux is easy - but to have them already accepted and pulled is not :-).
Linux GPIO maintainer is real friendly.
Also, the dts files which include [1] don't extend original gpio nodes to have one.
As it is not available in the Linux kernel, I don't see any good reason to add the gpio-ranges to U-Boot. The same approach, as presented here, is used in zynq_gpio.c file (which also uses DM/DTS).
Adding per u-boot property - like 'u-boot,mx28-gpio-ranges' is also less appealing than 24 lines of code, which can be then easily re-used with OF_PLATDATA (without DM/DTS suport) in SPL (u-boot.sb to be precise).
It is well possible the zynq DTs predate the gpio-ranges .
No, it is not.
Read the documentation for it at [2] . It even explicitly states it's used for interaction between pincontrol and gpio controllers.
In those cases where both support it. The i.MX28 Linux GPIO driver is NOT supporting gpio-ranges.
See above -- add support for it and it'd even simplify the driver.
[2] https://elixir.bootlin.com/linux/latest/source/Documentation/devicetree/bind...
+struct mxs_gpio_platdata {
- u32 gpio_nr_of_bank_pins[IMX28_GPIO_BANK_NR];
- u32 gpio_base_nr[IMX28_GPIO_BANK_NR];
- u32 ngpio;
+};
+extern const struct mxs_gpio_platdata mxs_gpio_def; +#define IMX_GPIO_NR(port, index) (mxs_gpio_def.gpio_base_nr[(port)] + (index))
So this should be completely unnecessary when using the DM GPIO, this was only needed for non-DM-GPIO .
This is a compatibility layer - for some reason ALL iMX ports define it
- i.e. imx8, imx7 - which shall use DM/DTS for gpio from the outset.
With the in-board code the dm_gpio_* set of functions shall (and will) be used (as done in opos6ul.c file).
Then use them and drop this.
I will use the new dm_gpio_* API where applicable. However, to be in sync with other iMX ports the IMX_GPIO_NR() shall be also defined.
Are there any users which will actually have to use the old stuff ? If not, just don't add it.
[...]