Re: [RFC PATCH] pinctrl: pinmux: Add pinmux-set debugfs file

[Andy Shevchenko <andy.shevchenko@xxxxxxxxx>]

On Fri, Jan 22, 2021 at 1:26 AM Drew Fustini <drew@xxxxxxxxxxxxxxx> wrote:
> On Thu, Jan 21, 2021 at 01:18:58PM +0200, Andy Shevchenko wrote:
> > On Thu, Jan 21, 2021 at 7:18 AM Drew Fustini <drew@xxxxxxxxxxxxxxx> wrote:

...

> > > RFC question: should pinmux-set take function name and group name
> > > instead of the selector numbers?
> >
> > I would prefer names and integers (but from user p.o.v. names are
> > easier to understand, while numbers are good for scripting).
>
> I don't actually see any example of looking up the function name in the
> existing pinctrl code. There is pin_function_tree in struct pinctrl_dev.
> pinmux_generic_get_function_name() does radix_tree_lookup() with the
> selector integer as the key, but there is no corresponding "get function
> selector by name" function.
>
> I think I would need to go through all the nodes in the radix tree to
> find the name that matches. Although, I am just learning now about the
> radix implementation in Linux so there might be a simpler way that I am
> missing.

I probably have to revive my work towards gluing ACPI with pin control
where AFAIR I have created some kind of radix / rbtree for something
(not sure it's exactly what you need here, so consider this just as a
side note).

...

> > The following is better to include in documentation and remove from
> > the commit message.

> > Shorter is better, what about simply
> >
> > # cat /sys/kernel/debug/pinctrl/44e10800.pinmux-pinctrl-single/pinmux-functions
> > ?
> >
> > Btw  in reST format you may create a nice citation of this. And yes,
> > this should also go to the documentation.
>
> Good point, I'll shorten the example lines in v2.

Even better to tell that we operate on the level of mount point of
debugfs and use

 # cat pinctrl/44e10800.pinmux-pinctrl-single/pinmux-functions

> > This and above is still part of documentation, and not a commit message thingy.
>
> Is something I should add to Documentation/driver-api/pinctl.rst in a
> seperate patch?

Not sure, I think more about as a part of this very path you change
code and documentation. But usually it's a preference of the certain
subsystem.

...

> > > +       if (cnt == 0)
> > > +               return 0;
> >
> > Has it ever happened here?
>
> Good point, I guess there is no reason for userspace to write 0 bytes.

My point is that this check is done somewhere in the guts of kernfs.
When in doubt I recommend to look around in the kernel and check most
recent code with similar code pieces.

...

> > > +       buf = memdup_user_nul(user_buf, cnt);
> > > +       if (IS_ERR(buf))
> > > +               return PTR_ERR(buf);
> > > +
> > > +       if (buf[cnt - 1] == '\n')
> > > +               buf[cnt - 1] = '\0';
> >
> > Shouldn't you rather use strndup_from_user() (or how is it called?)

Any comments?

...

> > Can be simply
> >
> >  err_freebuf:
> >         kfree(buf);
> >         return err ?: cnt;
>
> Thanks, I didn't really like the duplication but was having trouble
> thinking of a cleaner way to write it.  That is good to know it is ok to
> use the ternary operator in a return statement.

Again, depends on certain subsystem maintainer's preferences.


> > > +       debugfs_create_file("pinmux-set", S_IFREG | S_IWUSR,
> > > +                           devroot, pctldev, &pinmux_set_ops);

One more thing, as a preparatory patch please move from S_I* to plain
octal numbers as it's preferable.


-- 
With Best Regards,
Andy Shevchenko