From mboxrd@z Thu Jan 1 00:00:00 1970 From: Marek Vasut Date: Mon, 17 Jun 2019 15:34:22 +0200 Subject: [U-Boot] [PATCH v3 3/5] ARM: dm: mxs: gpio: Add support for DM/DTS in the mxs_gpio.c driver (DM_GPIO) In-Reply-To: <20190617150137.7e6500c5@jawa> References: <20190615223442.12246-1-lukma@denx.de> <20190615223442.12246-4-lukma@denx.de> <1c07bf95-8a43-6a5b-3e22-31cd4cd0b811@denx.de> <20190617103737.0bee7dc3@jawa> <20190617150137.7e6500c5@jawa> Message-ID: <26174e32-2b03-f8b4-a7a3-124f9fbdf90d@denx.de> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de 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 >>>>> >>>>> --- >>>>> >>>>> 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: > > 1. 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. > 2. 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. > 3. 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/bindings/gpio/gpio.txt#L239 >> >>>>> +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. [...] -- Best regards, Marek Vasut