Re: [PATCH v8 3/9] platform: cznic: turris-omnia-mcu: Add support for MCU connected GPIOs

[Andy Shevchenko <andy.shevchenko@xxxxxxxxx>]

On Sun, May 5, 2024 at 11:18 AM Marek Behún <kabel@xxxxxxxxxx> wrote:
> On Fri, May 03, 2024 at 09:51:17PM +0300, Andy Shevchenko wrote:
> > On Fri, May 03, 2024 at 10:28:17AM +0200, Marek Behún wrote:
> > > On Fri, May 03, 2024 at 07:05:34AM +0300, Andy Shevchenko wrote:
> > > > On Tue, Apr 30, 2024 at 2:51 PM Marek Behún <kabel@xxxxxxxxxx> wrote:

...

> > > > > +static const char * const omnia_mcu_gpio_templates[64] = {
> > > > > +       /* GPIOs with value read from the 16-bit wide status */
> > > > > +       [4]  = "gpio%u.MiniPCIe0 Card Detect",
> > > > > +       [5]  = "gpio%u.MiniPCIe0 mSATA Indicator",
> > > > > +       [6]  = "gpio%u.Front USB3 port over-current",
> > > > > +       [7]  = "gpio%u.Rear USB3 port over-current",
> > > > > +       [8]  = "gpio%u.Front USB3 port power",
> > > > > +       [9]  = "gpio%u.Rear USB3 port power",
> > > > > +       [12] = "gpio%u.Front Button",
> > > > > +
> > > > > +       /* GPIOs with value read from the 32-bit wide extended status */
> > > > > +       [16] = "gpio%u.SFP nDET",
> > > > > +       [28] = "gpio%u.MiniPCIe0 LED",
> > > > > +       [29] = "gpio%u.MiniPCIe1 LED",
> > > > > +       [30] = "gpio%u.MiniPCIe2 LED",
> > > > > +       [31] = "gpio%u.MiniPCIe0 PAN LED",
> > > > > +       [32] = "gpio%u.MiniPCIe1 PAN LED",
> > > > > +       [33] = "gpio%u.MiniPCIe2 PAN LED",
> > > > > +       [34] = "gpio%u.WAN PHY LED0",
> > > > > +       [35] = "gpio%u.WAN PHY LED1",
> > > > > +       [36] = "gpio%u.LAN switch p0 LED0",
> > > > > +       [37] = "gpio%u.LAN switch p0 LED1",
> > > > > +       [38] = "gpio%u.LAN switch p1 LED0",
> > > > > +       [39] = "gpio%u.LAN switch p1 LED1",
> > > > > +       [40] = "gpio%u.LAN switch p2 LED0",
> > > > > +       [41] = "gpio%u.LAN switch p2 LED1",
> > > > > +       [42] = "gpio%u.LAN switch p3 LED0",
> > > > > +       [43] = "gpio%u.LAN switch p3 LED1",
> > > > > +       [44] = "gpio%u.LAN switch p4 LED0",
> > > > > +       [45] = "gpio%u.LAN switch p4 LED1",
> > > > > +       [46] = "gpio%u.LAN switch p5 LED0",
> > > > > +       [47] = "gpio%u.LAN switch p5 LED1",
> > > > > +
> > > > > +       /* GPIOs with value read from the 16-bit wide extended control status */
> > > > > +       [48] = "gpio%u.eMMC nRESET",
> > > > > +       [49] = "gpio%u.LAN switch nRESET",
> > > > > +       [50] = "gpio%u.WAN PHY nRESET",
> > > > > +       [51] = "gpio%u.MiniPCIe0 nPERST",
> > > > > +       [52] = "gpio%u.MiniPCIe1 nPERST",
> > > > > +       [53] = "gpio%u.MiniPCIe2 nPERST",
> > > > > +       [54] = "gpio%u.WAN PHY SFP mux",
> > > > > +};
> > > >
> > > > You may reduce the memory footprint of these just by doing "gpio%u."
> > > > part(s) outside. Here compiler won't compress this (as in the case of
> > > > repetitive string literals),
> > >
> > > Are you saying that I should dynamically create another array just to
> > > pass it to the gpiochip's names pointer?
> >
> > I have looked into this again and now I'm puzzled how you tested this.
> > To me it seems that those gpio%u will go as a fixed string to the user space,
> > there is no %u --> number substitution happens. Moreover, this data anyway
> > is redundant. Userspace and debugfs all have line numbers being printed.
> >
>
> It gets substituted in drivers/gpio/gpiolib-sysfs.c, function
> gpiod_export():
>
>   dev = device_create_with_groups(&gpio_class, &gdev->dev,
>                                   MKDEV(0, 0), data, gpio_groups,
>                                   ioname ? ioname : "gpio%u",
>                                   desc_to_gpio(desc));
>
> The ioname variable contains the string.
>
> This is then visible in sysfs:
>
>   $ cd /sys/class/gpio
>   $ echo 560 >export
>   $ ls
>   ...
>   gpio560.eMMC nRESET
>   ...

Interesting. But before giving my conclusion on this, what is the
output of /sys/kernel/debug/gpio and `gpioinfo` (the latter from
libgpiod)?

-- 
With Best Regards,
Andy Shevchenko