Re: [PATCH v2] pinctrl: single: add big endian support

[Rustam Adilov <adilov@disroot.org>]

On 2026-10-11 20:14, Linus Walleij wrote:
> On Sat, Oct 10, 2026 at 7:57 PM Rustam Adilov <adilov@disroot.org> wrote:
> 
>> Add support for big endian pin controllers by taking into account
>> a big-endian device tree property.
>>
>> This is needed on big endian MIPS platforms like RTL930X, RTL931X
>> and RTL9607C who use pinctrl-single, because when SWAP_IO_SPACE is
>> enabled, the mmio accessors in pinctrl-single perform a byte swap.
>>
>> Signed-off-by: Rustam Adilov <adilov@disroot.org>
> 
> I'm not so sure about expanding the usage of pinctrl-single, we have
> said that this is used in the cases when you want to stash all the
> hardware data into the device tree (such as which groups and functions
> that exist) and it seems mostly done to save memory for the kernel
> so you don't need to compile in that data for all systems.
> 
> The argument that this is "simple" if you just have some hardware
> data in a file is moot these days IMO, because you can now just
> give that data to an LLM and it will spit out any group and function
> definitions you want.
> 
> So I'd like to hear some of the reasoning about why you want to
> do this thing, we have other drivers doing this but without using
> pinctrl-single.

Um, what? I don't understand how that is related to anything with my
commit message.

In MIPS context, SWAP_IO_SPACE is only about [1].

[1] - https://elixir.bootlin.com/linux/v7.2.9/source/arch/mips/include/asm/mach-generic/mangle-port.h#L28

> One of the driver maintainers have to ACK it too.
> 
>> +static unsigned int pcs_readw_be(void __iomem *reg)
>> +{
>> +       return ioread16be(reg);
>> +}
>> +
>> +static unsigned int pcs_readl_be(void __iomem *reg)
>> +{
>> +       return ioread32be(reg);
>> +}
>> +
>> +static void pcs_writew_be(unsigned int val, void __iomem *reg)
>> +{
>> +       iowrite16be(val, reg);
>> +}
>> +
>> +static void pcs_writel_be(unsigned int val, void __iomem *reg)
>> +{
>> +       iowrite32be(val, reg);
>> +}
> 
> Why this indirection? Can't you just assign the standard
> accessors directly?

To mirror the normal pcs_read/write? And yes we can just as easily
assign them to io functions directly.

>>         case 16:
>> -               pcs->read = pcs_readw;
>> -               pcs->write = pcs_writew;
>> +               if (byte_be) {
>> +                       pcs->read = pcs_readw_be;
>> +                       pcs->write = pcs_writew_be;
>> +               } else {
>> +                       pcs->read = pcs_readw;
>> +                       pcs->write = pcs_writew;
> 
> Maybe they should all do that actually, can't we rewrite
> this so we just:
> 
> if (byte_be) {
>   pcs->read = ioread16be;
>   pcs->write = iowrite16be;
> } else {
>   pcs->read = ioread16;
>   pcs->write = iowrite16;
> 
> I don't see how the wrappers help and I hope the compiler
> just optimize them away anyway.

A little out of scope but i am cool with a extending this patch to
a 2 part series to do that.

Best,
Rustam