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

[Marek Behún <kabel@xxxxxxxxxx>]

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
  ...


> ...
> 
> > > > +       for_each_set_bit(i, mask, ARRAY_SIZE(omnia_gpios)) {
> > > > +               if (omnia_gpios[i].cmd == CMD_GET_STATUS_WORD)
> > > > +                       __assign_bit(i, bits, sts_bits & omnia_gpios[i].bit);
> > > > +               else if (omnia_gpios[i].cmd == CMD_GET_EXT_STATUS_DWORD)
> > > > +                       __assign_bit(i, bits, ext_sts_bits &
> > > > +                                             omnia_gpios[i].bit);
> > > 
> > > One line?
> > 
> > That would be 81 columns, which I would like to avoid.
> 
> 81?! It's fine! Even documentation allows that for the readability.

OK

> > I can remove the _bits suffix from these variables, though. What do you
> > think?
> 
> Make sense as well.
> 
> > > > +               else if (omnia_gpios[i].cmd == CMD_GET_EXT_CONTROL_STATUS)
> > > > +                       __assign_bit(i, bits, ext_ctl_bits &
> > > > +                                             omnia_gpios[i].bit);
> > > 
> > > Ditto?
> > > 
> > > > +       }
> 
> -- 
> With Best Regards,
> Andy Shevchenko
> 
>