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

[Kent Gibson <warthog618@xxxxxxxxx>]

On Mon, Aug 08, 2022 at 01:21:07PM -0500, Limonciello, Mario wrote:
> On 8/8/2022 11:06, Kent Gibson wrote:
> > 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, #?
> 
> I guess if paired with a heading that is a great suggestion, thanks.
> 
> > 
> > > 
> > > > 
> > > > ...
> > > > 
> > > >       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 😛.
> > 
> Oh duh.
> 
> Separate symbols makes perfect sense for mask/unmasked.
> I'm not sure that 💥 is any better than 🔫 though.
> 

I was looking for something more visually distinctive - it is hard to
tell what the pistol is.  I was also thinking "spark" so ⚡, but again
the collision symbol looked more distinctive to me.  YMMV.

You are after level triggered and edge triggered, right?
So "-💥" 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.
> 
> In this context it feels like a timer though, not sure on this one.
> 

Well it is an alarm, which is why it is inappropriate for the debounce,
as that is more of a filter.

With an alarm clock you will wake, else you will continue to sleep.

> > 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.
> > 
> 
> Thanks for your suggestions.  I'll work out something later this week to
> change to some of them and leave you a Suggested-by tag in the commit.
> 

No problem - just my 2¢, plus it was the highlight of the day 😂.

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