Date: Fri, 28 Jun 2024 17:34:02 +0800
From: Huqiang Qin <huqiang.qin@...ogic.com>
To: Rob Herring <robh@...nel.org>
Cc: Linus Walleij <linus.walleij@...aro.org>,
 Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley
 <conor+dt@...nel.org>, Neil Armstrong <neil.armstrong@...aro.org>,
 Kevin Hilman <khilman@...libre.com>, Jerome Brunet <jbrunet@...libre.com>,
 Martin Blumenstingl <martin.blumenstingl@...glemail.com>,
 Bartosz Golaszewski <brgl@...ev.pl>, linux-gpio@...r.kernel.org,
 devicetree@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
 linux-amlogic@...ts.infradead.org, Xianwei Zhao <xianwei.zhao@...ogic.com>,
 linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/3] dt-bindings: pinctrl: Add support for Amlogic A4 SoCs

Hi Rob,

Thanks for your reply!

On 2024/6/14 1:08, Rob Herring wrote:
...
>> diff --git a/include/dt-bindings/gpio/amlogic,a4-periphs-pinctrl.h b/include/dt-bindings/gpio/amlogic,a4-periphs-pinctrl.h
>> new file mode 100644
>> index 000000000000..dfabca4b4790
>> --- /dev/null
>> +++ b/include/dt-bindings/gpio/amlogic,a4-periphs-pinctrl.h
>> @@ -0,0 +1,93 @@
>> +/* SPDX-License-Identifier: (GPL-2.0-only OR MIT) */
>> +/*
>> + * Copyright (c) 2024 Amlogic, Inc. All rights reserved.
>> + * Author: Xianwei Zhao <xianwei.zhao@...ogic.com>
>> + */
>> +
>> +#ifndef _DT_BINDINGS_AMLOGIC_A4_PERIPHS_PINCTRL_H
>> +#define _DT_BINDINGS_AMLOGIC_A4_PERIPHS_PINCTRL_H
>> +
>> +/* GPIOE */
>> +#define GPIOE_0				0
>> +#define GPIOE_1				1
>> +
>> +/* GPIOD */
>> +#define GPIOD_0				2
>> +#define GPIOD_1				3
>> +#define GPIOD_2				4
>> +#define GPIOD_3				5
>> +#define GPIOD_4				6
>> +#define GPIOD_5				7
>> +#define GPIOD_6				8
>> +#define GPIOD_7				9
>> +#define GPIOD_8				10
>> +#define GPIOD_9				11
>> +#define GPIOD_10			12
>> +#define GPIOD_11			13
>> +#define GPIOD_12			14
>> +#define GPIOD_13			15
>> +#define GPIOD_14			16
>> +#define GPIOD_15			17
> 
> I'm not really much of a fan of using defines for GPIOs, but if you do, 
> wouldn't be better to split banks and lines up rather than a global 
> number space. See ASPEED_GPIO() or tegra header.

We have always wanted to make the GPIO code look beautiful and concise.

However, since we put all the banks together at the beginning, this also
means that the automatic numbering of gpiolib will become continuous,
while ASPEED_GPIO() will make the numbering discontinuous.

In pinctrl-meson.c, we did not assign a value to pc->chip.of_xlate,
so this will use the default translation function of_gpio_simple_xlate()

of_gpio_simple_xlate() performs a linear conversion on the parameters
passed in by DT. In this way, in pinctrl-meson.c, we can easily use
meson_calc_reg_and_bit() to get the register corresponding to the pin.

However, when we refer to the ASPEED_GPIO() approach, we have to implement
our own of_xlate to convert the macro-defined numbers (discontinuous) into
continuous numbers of gpiolib.

And we need to define each range on DT, similar to:
	periphs_pinctrl: pinctrl@...0 {
		compatible = "amlogic,a4-periphs-pinctrl";
		...
		gpio: bank@...0 {
			...
			/*  gpio_offset  pin_offset  npins  */
			gpio-ranges = <&periphs_pinctrl 0  AMLOGIC_GPIO(GPIOE, 0) 2 >,	/*  0 ~ 1 */
				      <&periphs_pinctrl 2  AMLOGIC_GPIO(GPIOD, 0) 16>,	/*  2 ~ 17 */
				      <&periphs_pinctrl 18 AMLOGIC_GPIO(GPIOB, 0) 14>,	/* 18 ~ 31 */
				      <&periphs_pinctrl 32 AMLOGIC_GPIO(GPIOX, 0) 18>,	/* 32 ~ 49 */
				      <&periphs_pinctrl 50 AMLOGIC_GPIO(GPIOT, 0) 23>;	/* 50 ~ 72 */
		};
	};
Moreover, we also need to convert the following groups of functions:
	pc->chip.get_direction = meson_gpio_get_direction;
	pc->chip.direction_input = meson_gpio_direction_input;
	pc->chip.direction_output = meson_gpio_direction_output;
	pc->chip.get = meson_gpio_get;
	pc->chip.set = meson_gpio_set;



Of course there is another way to do it. DT can be written like this:
	periphs_pinctrl: pinctrl@...0 {
		compatible = "amlogic,a4-periphs-pinctrl";
		...
		gpioe: bank@0 {
			...
		};
		gpiod: bank@1 {
			...
		};
		gpiob: bank@2 {
			...
		};
		gpiox: bank@3 {
			...
		};
		gpiot: bank@4 {
			...
		};
	};

But this still requires a lot of modifications to pinctrl-meson.c,
because pinctrl-meson.c was originally designed for one gpiochip:
	static int meson_pinctrl_parse_dt(struct meson_pinctrl *pc)
	{
		...
		chips = gpiochip_node_count(pc->dev);
		if (!chips) {
				dev_err(pc->dev, "no gpio node found\n");
				return -EINVAL;
		}
		if (chips > 1) {
				dev_err(pc->dev, "multiple gpio nodes\n");
				return -EINVAL;
		}
		...
	}


The above are the issues I know and have considered. Because it involves
a lot of modifications, and we also need to ensure compatibility with
other platforms, we do not want to do this.

There are many historical reasons why Amlogic's Pinctrl/GPIO driver is so long.
I have been considering whether to refactor Amlogic's Pinctrl driver for
a long time, but I don't know whether the community will accept my doing so.
If Amlogic's driver can be refactored, the driver will become universal.
To support new platforms, we only need to add the pinctrl node to the DT
of the new platform, without having to add a c file and h file and compatible
every time.

If I can refactor Amlogic's Pinctrl, I will provide a new version of
Pinctrl driver within this year at the latest. If not, we can't think
of a better way to make this patch better. We can only do the next best thing
and remove amlogic,a4-aobus-pinctrl.h and amlogic,a4-periphs-pinctrl.h
from the patch. Although this will not affect the function of Pinctrl,
it will not be as easy to use as before.

For example:
	When there is a macro definition:   gpios = <&gpio GPIOX_15 GPIO_ACTIVE_LOW>;
	When there is no macro definition:  gpios = <&gpio 47 GPIO_ACTIVE_LOW>;


If there is any better way to do it, please guide me.

Thank you so much！

:)

Best regards,
Huqiang

Powered by blists - more mailing lists