Re: [PATCH v3 1/3] dt-bindings: pinctrl: Add support for Amlogic A4 SoCs

[neil.armstrong@linaro.org]

On 21/10/2024 11:56, Krzysztof Kozlowski wrote:
> On 21/10/2024 09:38, neil.armstrong@linaro.org wrote:
>> On 18/10/2024 17:31, Krzysztof Kozlowski wrote:
>>> On 18/10/2024 14:31, Neil Armstrong wrote:
>>>> On 18/10/2024 12:13, Krzysztof Kozlowski wrote:
>>>>> On 18/10/2024 11:20, Jerome Brunet wrote:
>>>>>> On Fri 18 Oct 2024 at 17:01, Xianwei Zhao <xianwei.zhao@amlogic.com> wrote:
>>>>>>
>>>>>>> Hi Jerome,
>>>>>>>       Thanks for your reply.
>>>>>>>
>>>>>>> On 2024/10/18 16:39, Jerome Brunet wrote:
>>>>>>>> [ EXTERNAL EMAIL ]
>>>>>>>> On Fri 18 Oct 2024 at 10:28, Krzysztof Kozlowski <krzk@kernel.org> wrote:
>>>>>>>>
>>>>>>>>> On 18/10/2024 10:10, Xianwei Zhao via B4 Relay wrote:
>>>>>>>>>> From: Xianwei Zhao <xianwei.zhao@amlogic.com>
>>>>>>>>>>
>>>>>>>>>> Add the new compatible name for Amlogic A4 pin controller, and add
>>>>>>>>>> a new dt-binding header file which document the detail pin names.
>>>>>>>> the change does not do what is described here. At least the description
>>>>>>>> needs updating.
>>>>>>>>
>>>>>>>
>>>>>>> Will do.
>>>>>>>
>>>>>>>> So if the pin definition is now in the driver, does it mean that pins have
>>>>>>>> to be referenced in DT directly using the made up numbers that are
>>>>>>>> created in pinctrl-amlogic-a4.c at the beginning of patch #2 ?
>>>>>>>>
>>>>>>>
>>>>>>> Yes.
>>>>>>>
>>>>>>>> If that's case, it does not look very easy a read.
>>>>>>>>
>>>>>>>
>>>>>>> It does happen. The pin definition does not fall under the category of
>>>>>>> binding.
>>>>>>>
>>>>>>> https://lore.kernel.org/all/106f4321-59e8-49b9-bad3-eeb57627c921@amlogic.com/
>>>>>>
>>>>>> So the expectation is that people will write something like:
>>>>>>
>>>>>>     reset-gpios = <&gpio 42 GPIO_ACTIVE_LOW>;
>>>>>>
>>>>>> And others will go in the driver to see that is maps to GPIOX_10 ? the number
>>>>>> being completly made up, with no link to anything HW/Datasheet
>>>>>> whatsoever ?
>>>>>>
>>>>>> This is how things should be done now ?
>>>>>
>>>>> Why would you need to do this? Why it cannot be <&gpio 10
>>>>> GPIO_ACTIVE_LOW>, assuming it is GPIO 10?
>>>>>
>>>>> Bindings have absolutely nothing to do with it. You have GPIO 10, not
>>>>> 42, right?
>>>>
>>>> There's no 1:1 mapping between the number and the pin on Amlogic platforms,
>>>> so either a supplementary gpio phandle cell is needed to encode the gpio pin
>>>> group or some bindings header is needed to map those to well known identifiers.
>>>
>>> So I assume this is not linear mapping (simple offset)? If so, this fits
>>> the binding header with identifiers, but I have impression these were
>>> not really used in earlier versions of this patchset. Instead some offsets:
>>> https://lore.kernel.org/all/20241014-a4_pinctrl-v2-1-3e74a65c285e@amlogic.com/
>>>
>>> and pre-proccessor.
>>>
>>> These looked almost good:
>>> https://lore.kernel.org/all/20240613170816.GA2020944-robh@kernel.org/
>>>
>>> but then 0 -> 0
>>> 1 -> 1
>>> so where is this need for IDs?
>>
>> ???
>>
>> Of courses the first pins maps to linear values...
>>
>>>
>>> See also last comment from Rob in above email.
>>
>> OK so I looked and v2 was in fact correct:
>> https://lore.kernel.org/all/20241014-a4_pinctrl-v2-1-3e74a65c285e@amlogic.com/
>>
>> ====><=================
>> +/* Standard port */
>> +#define GPIOB_START	0
>> +#define GPIOB_NUM	14
>> +
>> +#define GPIOD_START	(GPIOB_START + GPIOB_NUM)
>> +#define GPIOD_NUM	16
>> +
>> +#define GPIOE_START	(GPIOD_START + GPIOD_NUM)
>> +#define GPIOE_NUM	2
>> +
>> +#define GPIOT_START	(GPIOE_START + GPIOE_NUM)
>> +#define GPIOT_NUM	23
>> +
>> +#define GPIOX_START	(GPIOT_START + GPIOT_NUM)
>> +#define GPIOX_NUM	18
>> +
>> +#define PERIPHS_PIN_NUM	(GPIOX_START + GPIOX_NUM)
>> +
>> +/* Aobus port */
>> +#define GPIOAO_START	0
>> +#define GPIOAO_NUM	7
>> +
>> +/* It's a special definition, put at the end, just 1 num */
>> +#define	GPIO_TEST_N	(GPIOAO_START +  GPIOAO_NUM)
>> +#define	AOBUS_PIN_NUM	(GPIO_TEST_N + 1)
>> +
>> +#define AMLOGIC_GPIO(port, offset)	(port##_START + (offset))
>> ====><=================
>>
>> is exactly what rob asked for, and you nacked it.
> 
> No, this is not what was asked, at least according to my understanding.
> Number of GPIOs is not an ABI. Neither is their relationship, where one
> starts and other ends.

I confirm this need some work, but it moved the per-pin define to start
and ranges, so what did rob expect ?

> 
> Maybe I missed something, but I could not find any users of these in the
> DTS. Look:
> 
> https://lore.kernel.org/all/20241014-a4_pinctrl-v2-3-3e74a65c285e@amlogic.com/

So you want consumers before the bindings ? strange argument

> 
> Where is any of above defines?
> 
> Maybe they will be visible in the consumer code, but I did not imagine
> such use. You expect:
> reset-gpios = <&ctrl GPIOAO_START 1>???

No I expect:
reset-gpios = <&ctrl AMLOGIC_GPIO(B, 0) 1>;

but the macro should go along the dts like we did for the reset defines,
so perhaps this is the solution ?

> 
> How this is anyway close to what we have for Aspeed or Tegra? I
> understand that there was no consumer DTS, but you have also cover
> letter which could bring some answers in case reviewer is confused. What
> did cover letter say? Let me quote:
> 
> "Add pinctrl driver support for Amloigc A4 SoC"

Well he didn't expect a such sudden radical change in maintainers
requirements! Neither did I.

Neil

> 
> 
> 
> 
> Best regards,
> Krzysztof
>