Re: [bug report] pinctrl: amd: Use unicode for debugfs output

[Kent Gibson <warthog618@xxxxxxxxx>]

On Mon, Aug 08, 2022 at 07:33:07AM -0500, Mario Limonciello wrote:
> On 8/8/22 05:28, Dan Carpenter wrote:
> > Hello Mario Limonciello,
> > 
> > The patch e8129a076a50: "pinctrl: amd: Use unicode for debugfs
> > output" from Jul 22, 2022, leads to the following Smatch static
> > checker warning:
> > 
> > drivers/pinctrl/pinctrl-amd.c:249 amd_gpio_dbg_show() warn: format string contains non-ascii character '\x8c'
> > drivers/pinctrl/pinctrl-amd.c:288 amd_gpio_dbg_show() warn: format string contains non-ascii character '\x8e'
> > drivers/pinctrl/pinctrl-amd.c:294 amd_gpio_dbg_show() warn: format string contains non-ascii character '\x85'
> > drivers/pinctrl/pinctrl-amd.c:300 amd_gpio_dbg_show() warn: format string contains non-ascii character '\x85'
> > drivers/pinctrl/pinctrl-amd.c:306 amd_gpio_dbg_show() warn: format string contains non-ascii character '\x85'
> > drivers/pinctrl/pinctrl-amd.c:320 amd_gpio_dbg_show() warn: format string contains non-ascii character '\x86'
> > drivers/pinctrl/pinctrl-amd.c:326 amd_gpio_dbg_show() warn: format string contains non-ascii character '\x86'
> > drivers/pinctrl/pinctrl-amd.c:326 amd_gpio_dbg_show() warn: format string contains non-ascii character '\xe2'
> > drivers/pinctrl/pinctrl-amd.c:370 amd_gpio_dbg_show() warn: format string contains non-ascii character '\xe2'
> > 
> > I didn't add this Smatch check and I don't know the rules for this so
> > when someone adds something that basically looks sane, I don't report
> > it.
> > 
> 
> All of those are expected to me.  If there are rules against this type of
> change then we should (unfortunately) revert that patch and the follow on
> patch that fixed an unused variable.
> 
> > 
> > drivers/pinctrl/pinctrl-amd.c
> >      247                 seq_printf(s, "GPIO bank%d\n", bank);
> >      248                 for (; i < pin_num; i++) {
> >      249                         seq_printf(s, "📌%d\t", i);
> >                                                 ^
> > In Gnome this looks like a thumbtack.
> 
> Right, it's replacing "Pin".

Umm, #?

> 
> > 
> > ...
> > 
> >      279
> >      280                         if (pin_reg & BIT(INTERRUPT_MASK_OFF))
> >      281                                 interrupt_mask = "-";
> >      282                         else
> >      283                                 interrupt_mask = "+";
> >      284                         seq_printf(s, "int %s (🎭 %s)| active-%s| %s-🔫| ",
> > 
> > Gnome emojis seem difficult to read.  Theatre masks and a water gun?
> 
> "Mask" and "Trigger"
> 

🙈 and 💥?

If you would consider seperate symbols for masked and unmasked, rather
than appending "-" or "+", then 😷 and 😛.

> > 
> >      285                                    interrupt_enable,
> >      286                                    interrupt_mask,
> >      287                                    active_level,
> > --> 288                                    level_trig);
> >      289
> >      290                         if (pin_reg & BIT(WAKE_CNTRL_OFF_S0I3))
> >      291                                 wake_cntrl0 = "+";
> >      292                         else
> >      293                                 wake_cntrl0 = "∅";
> >      294                         seq_printf(s, "S0i3 🌅 %s| ", wake_cntrl0);
> > 
> > 
> > Sunrise emoji?
> 
> "S0i3 Wakeup"
> 

Somehow ⏰ makes more sense here to me.
And if you were to use separate symbols then 😴 for wake disabled?

> > 
> > ...
> > 
> >      369                         snprintf(debounce_value, sizeof(debounce_value), "%u", time * unit);
> >      370                         seq_printf(s, "debounce %s (⏰ %sus)| ", debounce_enable, debounce_value);
> > 
> > Alarm clock.
> 
> "Debounce time"
> 

🕑 or ⏲ or nothing?

Sorry - couldn't resist chipping in - poor old dan.

Cheers,
Kent.


> > 
> >      371                         seq_printf(s, " 0x%x\n", pin_reg);
> >      372                 }
> >      373         }
> >      374 }
> > 
> > regards,
> > dan carpenter
> 
> Yeah all of those emoji are the ones I intended.  Details above.  The point
> of this patch was to shrink the length of the line in debugfs output into
> something more manageable that it didn't need to be imported into a CSV
> processor to look at the data.  It can be used with something like "less"
> now.
> 
> If you (or anyone else) has a better proposal for any of those symbols I'm
> happy to make a change.  My goal remains to keep the lines ultra short
> though.