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

[Andy Shevchenko <andy.shevchenko@xxxxxxxxx>]

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

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

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.

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

...

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

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

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

Has it ever happened here?

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

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

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

These can be applied directly in the definition block above.

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

> +}
> +

...

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

I would rather call it 'pinmux-select'.

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!

-- 
With Best Regards,
Andy Shevchenko