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

[Drew Fustini <drew@xxxxxxxxxxxxxxx>]

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:
> >
> > This RFC is a change in approach from my previous RFC patch [1]. It adds
> > "pinnux-set" to debugfs. A function and group on the pin control device
> > will be activated when 2 integers "<function-selector> <group-selector>"
> > are written to the file. The debugfs write operation pinmux_set_write()
> > handles this by calling ops->set_mux() with fsel and gsel.
> 
> s/ops//

Ok, thanks.
> 
> > 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.

> 
> The following is better to include in documentation and remove from
> the commit message.
> 
> > The following is an example on the PocketBeagle [2] which has the AM3358
> > SoC and binds to pinctrl-single. I added this to the device tree [3] to
> > represent two of the pins on the expansion header as an example: P1.36
> > and P2.01. Both of these header pins are designed to be set to PWM mode
> > by default [4] but can now be set back to gpio mode through pinmux-set.
> 
> ...
> 
> > The following shows the pin functions registered for the pin controller:
> >
> > root@beaglebone:/sys/kernel/debug/pinctrl/44e10800.pinmux-pinctrl-single# cat pinmux-functions
> 
> 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.

> > function: pinmux_P1_36_default_pin, groups = [ pinmux_P1_36_default_pin ]
> > function: pinmux_P2_01_default_pin, groups = [ pinmux_P2_01_default_pin ]
> > function: pinmux_P1_36_gpio_pin, groups = [ pinmux_P1_36_gpio_pin ]
> > function: pinmux_P1_36_gpio_pu_pin, groups = [ pinmux_P1_36_gpio_pu_pin ]
> > function: pinmux_P1_36_gpio_pd_pin, groups = [ pinmux_P1_36_gpio_pd_pin ]
> > function: pinmux_P1_36_gpio_input_pin, groups = [ pinmux_P1_36_gpio_input_pin ]
> > function: pinmux_P1_36_pwm_pin, groups = [ pinmux_P1_36_pwm_pin ]
> > function: pinmux_P2_01_gpio_pin, groups = [ pinmux_P2_01_gpio_pin ]
> > function: pinmux_P2_01_gpio_pu_pin, groups = [ pinmux_P2_01_gpio_pu_pin ]
> > function: pinmux_P2_01_gpio_pd_pin, groups = [ pinmux_P2_01_gpio_pd_pin ]
> > function: pinmux_P2_01_gpio_input_pin, groups = [ pinmux_P2_01_gpio_input_pin ]
> > function: pinmux_P2_01_pwm_pin, groups = [ pinmux_P2_01_pwm_pin ]
> > function: pinmux-uart0-pins, groups = [ pinmux-uart0-pins ]
> > function: pinmux-mmc0-pins, groups = [ pinmux-mmc0-pins ]
> > function: pinmux-i2c0-pins, groups = [ pinmux-i2c0-pins ]
> >
> > Activate the pinmux_P1_36_gpio_pin function (fsel 2):
> >
> > root@beaglebone:/sys/kernel/debug/pinctrl/44e10800.pinmux-pinctrl-single# echo '2 2' > pinmux-set
> >
> > Extra debug output that I added shows that pinctrl-single's set_mux()
> > has set the register correctly for gpio mode:
> >
> > pinmux core: DEBUG pinmux_set_write(): returned 0
> > pinmux core: DEBUG pinmux_set_write(): buf=[2 2]
> > pinmux core: DEBUG pinmux_set_write(): sscanf(2,2)
> > pinmux core: DEBUG pinmux_set_write(): call ops->set_mux(fsel=2, gsel=2)
> > pinctrl-single 44e10800.pinmux: DEBUG pcs_set_mux(): call pinmux_generic_get_function() on fselector=2
> > pinctrl-single 44e10800.pinmux: enabling (null) function2
> > pinctrl-single 44e10800.pinmux: DEBUG pcs_set_mux(): func->nvals=1
> > pinctrl-single 44e10800.pinmux: DEBUG pcs_set_mux(): offset=0x190 old_val=0x21 val=0x2f
> >
> > Activate the pinmux_P1_36_pwm_pin function (fsel 6):
> >
> > root@beaglebone:/sys/kernel/debug/pinctrl/44e10800.pinmux-pinctrl-single# echo '6 6' > pinmux-set
> >
> > pinctrl-single set_mux() is able to set register correctly for pwm mode:
> >
> > pinmux core: DEBUG pinmux_set_write(): returned 0
> > pinmux core: DEBUG pinmux_set_write(): buf=[6 6]
> > pinmux core: DEBUG pinmux_set_write(): sscanf(6,6)
> > pinmux core: DEBUG pinmux_set_write(): call ops->set_mux(fsel=6, gsel=6)
> > pinctrl-single 44e10800.pinmux: DEBUG pcs_set_mux(): call pinmux_generic_get_function() on fselector=6
> > pinctrl-single 44e10800.pinmux: enabling (null) function6
> > pinctrl-single 44e10800.pinmux: DEBUG pcs_set_mux(): func->nvals=1
> > pinctrl-single 44e10800.pinmux: DEBUG pcs_set_mux(): offset=0x190 old_val=0x2f val=0x21
> 
> 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?

> 
> ...
> 
> > +static ssize_t pinmux_set_write(struct file *file, const char __user *user_buf,
> > +                                  size_t cnt, loff_t *ppos)
> > +{
> > +       int err;
> > +       int fsel;
> > +       int gsel;
> > +       int ret;
> > +       char *buf;
> > +       struct seq_file *sfile;
> > +       struct pinctrl_dev *pctldev;
> > +       const struct pinmux_ops *ops;
> 
> Reversed xmas tree order please, and you may group some of them, like
> 
>    int fsel, gsel;
> 

Ok, understood.

> > +       if (*ppos != 0)
> > +               return -EINVAL;
> 
> > +       if (cnt == 0)
> > +               return 0;
> 
> Has it ever happened here?

Good point, I guess there is no reason for userspace to write 0 bytes.

> > +       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?)
> 
> > +       ret = sscanf(buf, "%d %d", &fsel, &gsel);
> > +       if (ret != 2) {
> > +               pr_warn("%s: sscanf() expects '<fsel> <gsel>'", __func__);
> 
> No __func__ and instead use dev_err() (it is strange you are using
> warn level for errors).
> 

Ok, that makes sense. I used warn because I wasn't sure if bad format in
a write to a debugfs file rises to the level of error.

> > +               err = -EINVAL;
> > +               goto err_freebuf;
> > +       }
> 
> > +       sfile = file->private_data;
> > +       pctldev = sfile->private;
> 
> These can be applied directly in the definition block above.

I'll clean that up.

> 
> > +       ops = pctldev->desc->pmxops;
> > +       ret = ops->set_mux(pctldev, fsel, gsel);
> 
> > +       if (ret != 0) {
> 
> if (ret)
> 
> > +               pr_warn("%s(): set_mux() failed: %d", __func__, ret);
> 
> As above.
> 
> > +               err = -EINVAL;
> > +               goto err_freebuf;
> > +       }
> 
> > +       kfree(buf);
> > +       return cnt;
> > +
> > +err_freebuf:
> > +       kfree(buf);
> > +       return err;
> 
> 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.

> 
> > +}
> > +
> 
> ...
> 
> > +       debugfs_create_file("pinmux-set", S_IFREG | S_IWUSR,
> > +                           devroot, pctldev, &pinmux_set_ops);
> 
> I would rather call it 'pinmux-select'.

I think that makes sense, too.
> 
> Overall since it's a debugfs I do not much care about interfaces and
> particular implementation details, but in general looks good to me,
> thanks for doing this!

Thanks for the review.  I'll get a v2 posted.

-Drew