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

[Andy Shevchenko <andy.shevchenko@gmail.com>]

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@kernel.org> 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.

...

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

> 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