Re: [PATCH 02/10] dt-bindings: pinctrl: Add devicetree bindings for DM816x SoC Pinctrl

[Krzysztof Kozlowski <krzk@kernel.org>]

On 20/10/2026 12:18, Ettore Chimenti wrote:
> Hi Krzyszrof,
> 
> On Mon Oct 19, 2026 at 8:46 AM CEST, Krzysztof Kozlowski wrote:
>> On 16/10/2026 17:07, Ettore Chimenti wrote:
>>> Hi Krzyszrof,
>>>
>>> On Fri Oct 16, 2026 at 4:51 PM CEST, Krzysztof Kozlowski wrote:
>>>> On 16/10/2026 16:41, Ettore Chimenti wrote:
>>>>> Add devicetree bindings for TI DM816x SoC pinctrl configuration.
>>>>> Using same definition as OMAP2/3 except pullup/down enable macros, which
>>>>> are inverted in this SoC.
>>>>>
>>>>> Signed-off-by: Ettore Chimenti <ettore.chimenti@linaro.org>
>>>>> ---
>>>>>  include/dt-bindings/pinctrl/dm816x.h | 20 ++++++++++++++++++++
>>>>>  1 file changed, 20 insertions(+)
>>>>>
>>>>> diff --git a/include/dt-bindings/pinctrl/dm816x.h b/include/dt-bindings/pinctrl/dm816x.h
>>>>> new file mode 100644
>>>>> index 000000000000..15aa15bca9bd
>>>>> --- /dev/null
>>>>> +++ b/include/dt-bindings/pinctrl/dm816x.h
>>>>> @@ -0,0 +1,20 @@
>>>>> +/* SPDX-License-Identifier: GPL-2.0-only */
>>>>> +/*
>>>>> + * This header provides constants specific to DM816X pinctrl bindings.
>>>>> + */
>>>>> +
>>>>> +#ifndef _DT_BINDINGS_PINCTRL_DM816X_H
>>>>> +#define _DT_BINDINGS_PINCTRL_DM816X_H
>>>>> +
>>>>> +#include <dt-bindings/pinctrl/omap.h>
>>>>
>>>> I do not see this being used here, IWYU.
>>>>
>>>>> +
>>>>> +#undef PULL_UP
>>>>> +#undef PULL_ENA
>>>>
>>>> Why are you undefining other macros? That's not a easy to follow code.
>>>
>>> Do you prefer to duplicate `omap.h` defines? Only the defines above do not
>>> apply to DM816x.
>>
>> Not sure what you need to duplicate, but if you add header with bindings
>> for this device, then you should not include headers for other devices.
> 
> I basically used dm814x.h as a template which does that, is that not
> correct?
> 
>>
>>>
>>>>
>>>>
>>>>> +
>>>>> +/* DM81xx mux bit defines */
>>>>> +#define PULL_ENA		(0 << 3)  /* Pullup enabled  => 0 */
>>>>> +#define PULL_DIS		(1 << 3)  /* Pullup disabled => 1 */
>>>>> +#define PULL_DN			(0 << 4)
>>>>> +#define PULL_UP			(1 << 4)
>>>>
>>>> I do not see this used by drivers, so why do we need one more header?
>>>> What part of SW are you binding here?
>>>>
>>>
>>> I added those (DN/UP) because they are not defined in `omap.h`. It may be
>>> convenient for future board upstreaming. I can drop those if undesired.
>>
>> Just add the header to proper place - DTS. These are just not looking as
>> bindings.
> 
> Sorry, does dm814x.h not look as bindings? I'm a bit confused.

I asked  which part of SW are you binding. There was no answer, but
question. So if you cannot point which part of software you bind, then
we can assume it is not a binding. I did not do any extensive look,
because I assume that contributor will bring the argument in favor of
their patch, not me.


> FWIW, dm8148-evm.dts is not including the header, dm814x.dtsi is.
> Following 
> 
> Basically PULL_ENA define in omap.h is wrong for DM8168 SoCs. 
> 
> For a better solution, you would prefer:
> - Keep omap.h in dm816x.dtsi
> - Add a variant like DM816X_PULL_{ENA,DIS} in dm816x.h
> - Replace defines in dm8168-evm.dts
> - Add the header in dm8168-evm.dts
> 
> or:
> 
> - Keep omap.h in dm816x.dtsi
> - Add a variant like DM816X_PULL_{ENA,DIS} in omap.h
> - Replace defines in dm8168-evm.dts

I gave you one idea, but feel free to implement it in other ways aligned
with that platform maintainer.


Best regards,
Krzysztof