On Thu, Jan 21, 2021 at 01:18:58PM +0200, Andy Shevchenko wrote:

On Thu, Jan 21, 2021 at 7:18 AM Drew Fustini [off-list ref] wrote:

quoted

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.

quoted

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.

quoted

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.

...

quoted

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.

quoted

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?

...

quoted

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

quoted

+       if (*ppos != 0)
+               return -EINVAL;

quoted

+       if (cnt == 0)
+               return 0;

Has it ever happened here?

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

quoted

+       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?)

quoted

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

quoted

+               err = -EINVAL;
+               goto err_freebuf;
+       }

quoted

+       sfile = file->private_data;
+       pctldev = sfile->private;

These can be applied directly in the definition block above.

I'll clean that up.

quoted

+       ops = pctldev->desc->pmxops;
+       ret = ops->set_mux(pctldev, fsel, gsel);

quoted

+       if (ret != 0) {

if (ret)

quoted

+               pr_warn("%s(): set_mux() failed: %d", __func__, ret);

As above.

quoted

+               err = -EINVAL;
+               goto err_freebuf;
+       }

quoted

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

quoted

+}
+

...

quoted

+       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