[PATCH v3 0/3] am335x: set pinmux registers from pins debug file

[PATCH v3 0/3] am335x: set pinmux registers from pins debug file

[Dario Binacchi]

The patch was born from the need to change the slew rate of the LCD pins
of a custom AM335x board during EMC tests. The AM335x, as described in a
note in section 9.1 of its reference manual [1], is unable to write
pinmux registers from user space. The series now makes it possible to
write these registers from the pins debug file.

[1] https://www.ti.com/lit/ug/spruh73q/spruh73q.pdf


Changes in v3:
- Use strncpy_from_user() instead of copy_from_user().
- Do not shadow the error code returned by kstrtouint().
- Change pin_dbg_set() interface (char *buf --> unsigned int val).
- Describe pin_dbg_set().
- Remove CONFIG_DEV_MEM dependency.
- Change pcs_pin_dbg_set() interface (char *buf -> unsigned int val).

Changes in v2:
- Remove CONFIG_SOC_AM33XX dependency.

Dario Binacchi (3):
  docs/pinctrl: update `pins' description under debugfs
  pinctrl: core: configure pinmux from pins debug file
  pinctrl: single: set pinmux from pins debug file

 Documentation/driver-api/pin-control.rst |  3 ++
 drivers/pinctrl/core.c                   | 63 +++++++++++++++++++++++-
 drivers/pinctrl/pinctrl-single.c         | 13 +++++
 include/linux/pinctrl/pinctrl.h          |  4 ++
 4 files changed, 81 insertions(+), 2 deletions(-)

-- 
2.17.1

[PATCH v3 0/3] am335x: set pinmux registers from pins debug file

[Dario Binacchi]

The patch was born from the need to change the slew rate of the LCD pins
of a custom AM335x board during EMC tests. The AM335x, as described in a
note in section 9.1 of its reference manual [1], is unable to write
pinmux registers from user space. The series now makes it possible to
write these registers from the pins debug file.

[1] https://www.ti.com/lit/ug/spruh73q/spruh73q.pdf


Changes in v3:
- Use strncpy_from_user() instead of copy_from_user().
- Do not shadow the error code returned by kstrtouint().
- Change pin_dbg_set() interface (char *buf --> unsigned int val).
- Describe pin_dbg_set().
- Remove CONFIG_DEV_MEM dependency.
- Change pcs_pin_dbg_set() interface (char *buf -> unsigned int val).

Changes in v2:
- Remove CONFIG_SOC_AM33XX dependency.

Dario Binacchi (3):
  docs/pinctrl: update `pins' description under debugfs
  pinctrl: core: configure pinmux from pins debug file
  pinctrl: single: set pinmux from pins debug file

 Documentation/driver-api/pin-control.rst |  3 ++
 drivers/pinctrl/core.c                   | 63 +++++++++++++++++++++++-
 drivers/pinctrl/pinctrl-single.c         | 13 +++++
 include/linux/pinctrl/pinctrl.h          |  4 ++
 4 files changed, 81 insertions(+), 2 deletions(-)

-- 
2.17.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v3 1/3] docs/pinctrl: update `pins' description under debugfs

[Dario Binacchi]

Optionally the `pins' file created inside the `/sys/kernel/debug/pinctrl'
subdirectory of each pin controller device now allows you to set the
mux register of a pin.

Signed-off-by: Dario Binacchi <dariobin@libero.it>
---

(no changes since v1)

 Documentation/driver-api/pin-control.rst | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/Documentation/driver-api/pin-control.rst b/Documentation/driver-api/pin-control.rst
index e2474425fb0c..3d8192fa45a3 100644
--- a/Documentation/driver-api/pin-control.rst
+++ b/Documentation/driver-api/pin-control.rst
@@ -1447,6 +1447,9 @@ controller device containing these files:
 
 - ``pins``: prints a line for each pin registered on the pin controller. The
   pinctrl driver may add additional information such as register contents.
+  Optionally writes the mux register for the selected pin.
+  e.g. to write the value 0x73 in the mux register of pin 19:
+  echo 19 0x73 >/sys/kernel/debug/pinctrl/pins
 
 - ``gpio-ranges``: print ranges that map gpio lines to pins on the controller
 
-- 
2.17.1

[PATCH v3 2/3] pinctrl: core: configure pinmux from pins debug file

[Dario Binacchi]

The MPUs of some architectures (e.g AM335x) must be in privileged
operating mode to write on the pinmux registers. In such cases, where
writes will not work from user space, now it can be done from the pins
debug file if the platform driver exports the pin_dbg_set() helper among
the registered operations.

Signed-off-by: Dario Binacchi <dariobin@libero.it>
---

Changes in v3:
- Use strncpy_from_user() instead of copy_from_user().
- Do not shadow the error code returned by kstrtouint().
- Change pin_dbg_set() interface (char *buf --> unsigned int val).
- Describe pin_dbg_set().

 drivers/pinctrl/core.c          | 63 +++++++++++++++++++++++++++++++--
 include/linux/pinctrl/pinctrl.h |  4 +++
 2 files changed, 65 insertions(+), 2 deletions(-)

diff --git a/drivers/pinctrl/core.c b/drivers/pinctrl/core.c
index a4ac87c8b4f8..ab832044a0c3 100644
--- a/drivers/pinctrl/core.c
+++ b/drivers/pinctrl/core.c
@@ -1620,6 +1620,53 @@ EXPORT_SYMBOL_GPL(pinctrl_pm_select_idle_state);
 
 #ifdef CONFIG_DEBUG_FS
 
+static ssize_t pinctrl_pins_write(struct file *file,
+				  const char __user *user_buf, size_t count,
+				  loff_t *ppos)
+{
+	struct seq_file	*s = file->private_data;
+	struct pinctrl_dev *pctldev = s->private;
+	const struct pinctrl_ops *ops = pctldev->desc->pctlops;
+	char buf[32];
+	char *c = &buf[0];
+	char *token;
+	int ret;
+	unsigned int i, pin, val;
+
+	if (!ops->pin_dbg_set)
+		return -EFAULT;
+
+	ret = strncpy_from_user(buf, user_buf, sizeof(buf));
+	if (ret == 0 || ret == sizeof(buf))
+		ret = -ERANGE;
+
+	if (ret < 0)
+		return ret;
+
+	token = strsep(&c, " ");
+	ret = kstrtouint(token, 0, &pin);
+	if (ret)
+		return ret;
+
+	token = strsep(&c, " ");
+	ret = kstrtouint(token, 0, &val);
+	if (ret)
+		return ret;
+
+	for (i = 0; i < pctldev->desc->npins; i++) {
+		if (pin != pctldev->desc->pins[i].number)
+			continue;
+
+		ret = ops->pin_dbg_set(pctldev, pin, val);
+		if (ret)
+			return ret;
+
+		return count;
+	}
+
+	return -EINVAL;
+}
+
 static int pinctrl_pins_show(struct seq_file *s, void *what)
 {
 	struct pinctrl_dev *pctldev = s->private;
@@ -1677,7 +1724,11 @@ static int pinctrl_pins_show(struct seq_file *s, void *what)
 
 	return 0;
 }
-DEFINE_SHOW_ATTRIBUTE(pinctrl_pins);
+
+static int pinctrl_pins_open(struct inode *inode, struct file *file)
+{
+	return single_open(file, pinctrl_pins_show, inode->i_private);
+}
 
 static int pinctrl_groups_show(struct seq_file *s, void *what)
 {
@@ -1886,6 +1937,14 @@ static int pinctrl_show(struct seq_file *s, void *what)
 }
 DEFINE_SHOW_ATTRIBUTE(pinctrl);
 
+static const struct file_operations pinctrl_pins_fops = {
+	.open		= pinctrl_pins_open,
+	.read		= seq_read,
+	.write		= pinctrl_pins_write,
+	.llseek		= seq_lseek,
+	.release	= single_release,
+};
+
 static struct dentry *debugfs_root;
 
 static void pinctrl_init_device_debugfs(struct pinctrl_dev *pctldev)
@@ -1915,7 +1974,7 @@ static void pinctrl_init_device_debugfs(struct pinctrl_dev *pctldev)
 			dev_name(pctldev->dev));
 		return;
 	}
-	debugfs_create_file("pins", 0444,
+	debugfs_create_file("pins", 0644,
 			    device_root, pctldev, &pinctrl_pins_fops);
 	debugfs_create_file("pingroups", 0444,
 			    device_root, pctldev, &pinctrl_groups_fops);
diff --git a/include/linux/pinctrl/pinctrl.h b/include/linux/pinctrl/pinctrl.h
index 70b45d28e7a9..7ae8aca3dfa5 100644
--- a/include/linux/pinctrl/pinctrl.h
+++ b/include/linux/pinctrl/pinctrl.h
@@ -75,6 +75,8 @@ struct pinctrl_gpio_range {
  *	group selector @pins, and the size of the array in @num_pins
  * @pin_dbg_show: optional debugfs display hook that will provide per-device
  *	info for a certain pin in debugfs
+ * @pin_dbg_set: optional debugfs set hook that will write per-device pinmux
+ *	register for a certain pin in debugfs
  * @dt_node_to_map: parse a device tree "pin configuration node", and create
  *	mapping table entries for it. These are returned through the @map and
  *	@num_maps output parameters. This function is optional, and may be
@@ -95,6 +97,8 @@ struct pinctrl_ops {
 			       unsigned *num_pins);
 	void (*pin_dbg_show) (struct pinctrl_dev *pctldev, struct seq_file *s,
 			  unsigned offset);
+	int (*pin_dbg_set) (struct pinctrl_dev *pctldev, unsigned int offset,
+			    unsigned int val);
 	int (*dt_node_to_map) (struct pinctrl_dev *pctldev,
 			       struct device_node *np_config,
 			       struct pinctrl_map **map, unsigned *num_maps);
-- 
2.17.1

[PATCH v3 3/3] pinctrl: single: set pinmux from pins debug file

[Dario Binacchi]

As described in section 9.1 of the TI reference manual for AM335x [1],
"For writing to the control module registers, the MPU will need to be in
privileged mode of operation and writes will not work from user mode".
By adding the pin_dbg_set helper to pcs_pinctrl_ops it will be possible
to write these registers from the pins debug:

cd /sys/kernel/debug/pinctrl/44e10800.pinmux-pinctrl-single/
echo <pin-number> <reg-value> >pins

[1] https://www.ti.com/lit/ug/spruh73q/spruh73q.pdf

Signed-off-by: Dario Binacchi <dariobin@libero.it>

---

Changes in v3:
- Remove CONFIG_DEV_MEM dependency.
- Change pcs_pin_dbg_set() interface (char *buf -> unsigned int val).

Changes in v2:
- Remove CONFIG_SOC_AM33XX dependency.

 drivers/pinctrl/pinctrl-single.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/drivers/pinctrl/pinctrl-single.c b/drivers/pinctrl/pinctrl-single.c
index 2c9c9835f375..1b75236563cf 100644
--- a/drivers/pinctrl/pinctrl-single.c
+++ b/drivers/pinctrl/pinctrl-single.c
@@ -313,6 +313,18 @@ static void pcs_pin_dbg_show(struct pinctrl_dev *pctldev,
 	seq_printf(s, "%zx %08x %s ", pa, val, DRIVER_NAME);
 }
 
+static int pcs_pin_dbg_set(struct pinctrl_dev *pctldev, unsigned int pin,
+			   unsigned int val)
+{
+	struct pcs_device *pcs;
+	unsigned int mux_bytes;
+
+	pcs = pinctrl_dev_get_drvdata(pctldev);
+	mux_bytes = pcs->width / BITS_PER_BYTE;
+	pcs->write(val, pcs->base + pin * mux_bytes);
+	return 0;
+}
+
 static void pcs_dt_free_map(struct pinctrl_dev *pctldev,
 				struct pinctrl_map *map, unsigned num_maps)
 {
@@ -331,6 +343,7 @@ static const struct pinctrl_ops pcs_pinctrl_ops = {
 	.get_group_name = pinctrl_generic_get_group_name,
 	.get_group_pins = pinctrl_generic_get_group_pins,
 	.pin_dbg_show = pcs_pin_dbg_show,
+	.pin_dbg_set = pcs_pin_dbg_set,
 	.dt_node_to_map = pcs_dt_node_to_map,
 	.dt_free_map = pcs_dt_free_map,
 };
-- 
2.17.1

[PATCH v3 3/3] pinctrl: single: set pinmux from pins debug file

[Dario Binacchi]

As described in section 9.1 of the TI reference manual for AM335x [1],
"For writing to the control module registers, the MPU will need to be in
privileged mode of operation and writes will not work from user mode".
By adding the pin_dbg_set helper to pcs_pinctrl_ops it will be possible
to write these registers from the pins debug:

cd /sys/kernel/debug/pinctrl/44e10800.pinmux-pinctrl-single/
echo <pin-number> <reg-value> >pins

[1] https://www.ti.com/lit/ug/spruh73q/spruh73q.pdf

Signed-off-by: Dario Binacchi <dariobin@libero.it>

---

Changes in v3:
- Remove CONFIG_DEV_MEM dependency.
- Change pcs_pin_dbg_set() interface (char *buf -> unsigned int val).

Changes in v2:
- Remove CONFIG_SOC_AM33XX dependency.

 drivers/pinctrl/pinctrl-single.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/drivers/pinctrl/pinctrl-single.c b/drivers/pinctrl/pinctrl-single.c
index 2c9c9835f375..1b75236563cf 100644
--- a/drivers/pinctrl/pinctrl-single.c
+++ b/drivers/pinctrl/pinctrl-single.c
@@ -313,6 +313,18 @@ static void pcs_pin_dbg_show(struct pinctrl_dev *pctldev,
 	seq_printf(s, "%zx %08x %s ", pa, val, DRIVER_NAME);
 }
 
+static int pcs_pin_dbg_set(struct pinctrl_dev *pctldev, unsigned int pin,
+			   unsigned int val)
+{
+	struct pcs_device *pcs;
+	unsigned int mux_bytes;
+
+	pcs = pinctrl_dev_get_drvdata(pctldev);
+	mux_bytes = pcs->width / BITS_PER_BYTE;
+	pcs->write(val, pcs->base + pin * mux_bytes);
+	return 0;
+}
+
 static void pcs_dt_free_map(struct pinctrl_dev *pctldev,
 				struct pinctrl_map *map, unsigned num_maps)
 {
@@ -331,6 +343,7 @@ static const struct pinctrl_ops pcs_pinctrl_ops = {
 	.get_group_name = pinctrl_generic_get_group_name,
 	.get_group_pins = pinctrl_generic_get_group_pins,
 	.pin_dbg_show = pcs_pin_dbg_show,
+	.pin_dbg_set = pcs_pin_dbg_set,
 	.dt_node_to_map = pcs_dt_node_to_map,
 	.dt_free_map = pcs_dt_free_map,
 };
-- 
2.17.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v3 2/3] pinctrl: core: configure pinmux from pins debug file

[Vladimir Zapolskiy]

Hello Dario,

On 5/20/21 11:27 PM, Dario Binacchi wrote:
> The MPUs of some architectures (e.g AM335x) must be in privileged
> operating mode to write on the pinmux registers. In such cases, where
> writes will not work from user space, now it can be done from the pins

user space has no connection to the problem you're trying to solve.

Please provide a reasonable rationale for adding a new interface, thank
you in advance.

> debug file if the platform driver exports the pin_dbg_set() helper among
> the registered operations.
> 
> Signed-off-by: Dario Binacchi <dariobin@libero.it>

I strongly object against this new interface.

As Andy've already mentioned you have to operate with defined pin groups
and functions, and so far you create an interface with an option to
disasterous misusage, it shall be avoided, because there are better
options.

What's the issue with a regular declaration of pin groups and functions
on your SoC? When it's done, you can operate on this level of abstraction,
there is absolutely no need to add the proposed low-level debug interface.

--
Best wishes,
Vladimir

Re: [PATCH v3 3/3] pinctrl: single: set pinmux from pins debug file

[Vladimir Zapolskiy]

Hello Dario,

On 5/20/21 11:27 PM, Dario Binacchi wrote:
> As described in section 9.1 of the TI reference manual for AM335x [1],
> "For writing to the control module registers, the MPU will need to be in
> privileged mode of operation and writes will not work from user mode".
> By adding the pin_dbg_set helper to pcs_pinctrl_ops it will be possible
> to write these registers from the pins debug:

this makes little sense, because it should be possible to write to the
registers without introduction of this interface, pin group/function
is the right level of abstraction for your particular task.

> cd /sys/kernel/debug/pinctrl/44e10800.pinmux-pinctrl-single/
> echo <pin-number> <reg-value> >pins
> 
> [1] https://www.ti.com/lit/ug/spruh73q/spruh73q.pdf
> 
> Signed-off-by: Dario Binacchi <dariobin@libero.it>

I strongly object against this new interface to be added.

--
Best wishes,
Vladimir

Re: [PATCH v3 3/3] pinctrl: single: set pinmux from pins debug file

[Vladimir Zapolskiy]

Hello Dario,

On 5/20/21 11:27 PM, Dario Binacchi wrote:
> As described in section 9.1 of the TI reference manual for AM335x [1],
> "For writing to the control module registers, the MPU will need to be in
> privileged mode of operation and writes will not work from user mode".
> By adding the pin_dbg_set helper to pcs_pinctrl_ops it will be possible
> to write these registers from the pins debug:

this makes little sense, because it should be possible to write to the
registers without introduction of this interface, pin group/function
is the right level of abstraction for your particular task.

> cd /sys/kernel/debug/pinctrl/44e10800.pinmux-pinctrl-single/
> echo <pin-number> <reg-value> >pins
> 
> [1] https://www.ti.com/lit/ug/spruh73q/spruh73q.pdf
> 
> Signed-off-by: Dario Binacchi <dariobin@libero.it>

I strongly object against this new interface to be added.

--
Best wishes,
Vladimir

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v3 2/3] pinctrl: core: configure pinmux from pins debug file

[Dario Binacchi]

Hi Vladimir,

> Il 21/05/2021 08:44 Vladimir Zapolskiy <vz@mleia.com> ha scritto:
> 
>  
> Hello Dario,
> 
> On 5/20/21 11:27 PM, Dario Binacchi wrote:
> > The MPUs of some architectures (e.g AM335x) must be in privileged
> > operating mode to write on the pinmux registers. In such cases, where
> > writes will not work from user space, now it can be done from the pins
> 
> user space has no connection to the problem you're trying to solve.
> 
> Please provide a reasonable rationale for adding a new interface, thank
> you in advance.
> 
> > debug file if the platform driver exports the pin_dbg_set() helper among
> > the registered operations.
> > 
> > Signed-off-by: Dario Binacchi <dariobin@libero.it>
> 
> I strongly object against this new interface.
> 
> As Andy've already mentioned you have to operate with defined pin groups
> and functions, and so far you create an interface with an option to
> disasterous misusage, it shall be avoided, because there are better
> options.
> 
> What's the issue with a regular declaration of pin groups and functions
> on your SoC? When it's done, you can operate on this level of abstraction,
> there is absolutely no need to add the proposed low-level debug interface.
> 

I quote Drew's words: 

"I think it could be helpful to be able to set the conf_<module>_<pin>  
registers directly through debugfs as I can imagine situations where it would 
be useful for testing. It is a bit dangerous as the person using it has to be 
careful not to change the wrong bits, but they would need to have debugfs mounted 
and permissions to write to it."

"Bits 6:3 are related to what this subsystem would refer to as pin conf
such as slew, input enable and bias. Thus it might make sense to expose
something like a select-pinconf file to activate pin conf settings from
userspace."
 
From the emails exchanged I seem to have understood that there is no way to 
reconfigure slew rate, pull up / down and other properties on the fly. 
In the kernel version 4.1.6 that I am using on my custom board, I have fixed
the commit f07512e615dd ("pinctrl/pinconfig: add debug interface"). However,
this feature was later removed (https://lore.kernel.org/patchwork/patch/1033755/).
The patches I've submitted implement some sort of devmem for pinmux. It too can 
be used in a dangerous way, but it exists and it is used.

Anyway, the implementation may be wrong but it does highlight a feature that 
can be useful in testing or prototyping boards but is not present in the kernel.
Can we then find a solution that is right for everyone?

Thanks and regards, 
Dario

> --
> Best wishes,
> Vladimir

Re: [PATCH v3 2/3] pinctrl: core: configure pinmux from pins debug file

[Vladimir Zapolskiy]

Hi Dario,

On 5/24/21 8:28 PM, Dario Binacchi wrote:
> Hi Vladimir,
> 
>> Il 21/05/2021 08:44 Vladimir Zapolskiy <vz@mleia.com> ha scritto:
>>
>>   
>> Hello Dario,
>>
>> On 5/20/21 11:27 PM, Dario Binacchi wrote:
>>> The MPUs of some architectures (e.g AM335x) must be in privileged
>>> operating mode to write on the pinmux registers. In such cases, where
>>> writes will not work from user space, now it can be done from the pins
>>
>> user space has no connection to the problem you're trying to solve.
>>
>> Please provide a reasonable rationale for adding a new interface, thank
>> you in advance.
>>
>>> debug file if the platform driver exports the pin_dbg_set() helper among
>>> the registered operations.
>>>
>>> Signed-off-by: Dario Binacchi <dariobin@libero.it>
>>
>> I strongly object against this new interface.
>>
>> As Andy've already mentioned you have to operate with defined pin groups
>> and functions, and so far you create an interface with an option to
>> disasterous misusage, it shall be avoided, because there are better
>> options.
>>
>> What's the issue with a regular declaration of pin groups and functions
>> on your SoC? When it's done, you can operate on this level of abstraction,
>> there is absolutely no need to add the proposed low-level debug interface.
>>
> 
> I quote Drew's words:
> 
> "I think it could be helpful to be able to set the conf_<module>_<pin>
> registers directly through debugfs as I can imagine situations where it would
> be useful for testing. It is a bit dangerous as the person using it has to be
> careful not to change the wrong bits, but they would need to have debugfs mounted
> and permissions to write to it."
> 
> "Bits 6:3 are related to what this subsystem would refer to as pin conf
> such as slew, input enable and bias. Thus it might make sense to expose
> something like a select-pinconf file to activate pin conf settings from
> userspace."

This is already present, please define all wanted configurations of pin
groups and pin group functions, then switch them in runtime. I see no
need of a coarse grained interface here...

>  From the emails exchanged I seem to have understood that there is no way to
> reconfigure slew rate, pull up / down and other properties on the fly.

I think you still can do all the tasks mentioned above on the recent kernel,
why not?

I am not closely familiar with TI AM335x pinmux/pinconf controller, and if
needed I can look at the datasheet, but I can imagine that there are pins,
pin groups, and pin group functions controls. Board specific configuration
of pinmux is given in DTS, you can modify it with DT overlays for instance,
and selection of pin group functions in runtime is already possible for
users in runtime. What is missing from the picture, and why do you insist
on re-introduction of a much worse interface?

> In the kernel version 4.1.6 that I am using on my custom board, I have fixed
> the commit f07512e615dd ("pinctrl/pinconfig: add debug interface"). However,
> this feature was later removed (https://lore.kernel.org/patchwork/patch/1033755/).

Exactly, the feature is not needed.

> The patches I've submitted implement some sort of devmem for pinmux. It too can
> be used in a dangerous way, but it exists and it is used.

My objection is to giving a "red button" to users, even to users of debugfs.

If it's possible to keep the "dangerous goods" on developers' side only,
then this shall be preferable, I believe. And fortunately there is such
a mechanism.

> Anyway, the implementation may be wrong but it does highlight a feature that
> can be useful in testing or prototyping boards but is not present in the kernel.
> Can we then find a solution that is right for everyone?

Please see the method above.

In my understanding the problem you are trying to solve shall be defined
much more precised. Can you please elaborate on this part thoroughly?

I still can not grasp a too generic explanation from you, writing of
totally arbitrary data to controller registers looks senseless to me...

Assume you are giving a handle to users to write arbitrary data to arbitrary
I/O mem region, will it solve the problem for you? Of course yes.

But does it sound like a good and acceptable solution? Of course no.
Why? You need a better and more fine grained interface, namely write only
to pinmux I/O mem. Great, that's provided by your change, however another
important condition is still missing, a user shall write only valid data.
Thus a higher level of abstraction is wanted:

* writing data to I/O mem -- not good enough,
* writing data to pinmux/pinconf I/O mem -- better, but not good enough,
* writing *valid* data to pinmux/pinconf I/O mem -- that's right.

The validity of data is defined by a developer, the abstraction name
has been mentioned multiple times, it's pin groups and pin group functions.

--
Best wishes,
Vladimir

Re: [PATCH v3 0/3] am335x: set pinmux registers from pins debug file

[Linus Walleij]

On Thu, May 20, 2021 at 10:27 PM Dario Binacchi <dariobin@libero.it> wrote:

> The patch was born from the need to change the slew rate of the LCD pins
> of a custom AM335x board during EMC tests. The AM335x, as described in a
> note in section 9.1 of its reference manual [1], is unable to write
> pinmux registers from user space. The series now makes it possible to
> write these registers from the pins debug file.

I am going to hold this series back pending more discussion on the ABI
and whether it is useful.

Yours,
Linus Walleij

Re: [PATCH v3 0/3] am335x: set pinmux registers from pins debug file

[Linus Walleij]

On Thu, May 20, 2021 at 10:27 PM Dario Binacchi <dariobin@libero.it> wrote:

> The patch was born from the need to change the slew rate of the LCD pins
> of a custom AM335x board during EMC tests. The AM335x, as described in a
> note in section 9.1 of its reference manual [1], is unable to write
> pinmux registers from user space. The series now makes it possible to
> write these registers from the pins debug file.

I am going to hold this series back pending more discussion on the ABI
and whether it is useful.

Yours,
Linus Walleij

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v3 2/3] pinctrl: core: configure pinmux from pins debug file

[Tony Lindgren]

Hi,

* Vladimir Zapolskiy <vz@mleia.com> [210524 18:52]:
> Thus a higher level of abstraction is wanted:
> 
> * writing data to I/O mem -- not good enough,
> * writing data to pinmux/pinconf I/O mem -- better, but not good enough,
> * writing *valid* data to pinmux/pinconf I/O mem -- that's right.
> 
> The validity of data is defined by a developer, the abstraction name
> has been mentioned multiple times, it's pin groups and pin group functions.

Sounds like the planned interface should only allow changing between
already defined pingroups, and only if the pins are unclaimed. That
would leave out the issues tinkering with raw register data.

And it probably should also allow adding and removing new pingroups out
of the already defined pin functions that are unclaimed.

I also think such interface should be sysfs and not debugfs eventually.

Regards,

Tony

Re: [PATCH v3 2/3] pinctrl: core: configure pinmux from pins debug file

[Dario Binacchi]

Hi Vladimir,

> Il 24/05/2021 20:52 Vladimir Zapolskiy <vz@mleia.com> ha scritto:
> 
>  
> Hi Dario,
> 
> On 5/24/21 8:28 PM, Dario Binacchi wrote:
> > Hi Vladimir,
> > 
> >> Il 21/05/2021 08:44 Vladimir Zapolskiy <vz@mleia.com> ha scritto:
> >>
> >>   
> >> Hello Dario,
> >>
> >> On 5/20/21 11:27 PM, Dario Binacchi wrote:
> >>> The MPUs of some architectures (e.g AM335x) must be in privileged
> >>> operating mode to write on the pinmux registers. In such cases, where
> >>> writes will not work from user space, now it can be done from the pins
> >>
> >> user space has no connection to the problem you're trying to solve.
> >>
> >> Please provide a reasonable rationale for adding a new interface, thank
> >> you in advance.
> >>
> >>> debug file if the platform driver exports the pin_dbg_set() helper among
> >>> the registered operations.
> >>>
> >>> Signed-off-by: Dario Binacchi <dariobin@libero.it>
> >>
> >> I strongly object against this new interface.
> >>
> >> As Andy've already mentioned you have to operate with defined pin groups
> >> and functions, and so far you create an interface with an option to
> >> disasterous misusage, it shall be avoided, because there are better
> >> options.
> >>
> >> What's the issue with a regular declaration of pin groups and functions
> >> on your SoC? When it's done, you can operate on this level of abstraction,
> >> there is absolutely no need to add the proposed low-level debug interface.
> >>
> > 
> > I quote Drew's words:
> > 
> > "I think it could be helpful to be able to set the conf_<module>_<pin>
> > registers directly through debugfs as I can imagine situations where it would
> > be useful for testing. It is a bit dangerous as the person using it has to be
> > careful not to change the wrong bits, but they would need to have debugfs mounted
> > and permissions to write to it."
> > 
> > "Bits 6:3 are related to what this subsystem would refer to as pin conf
> > such as slew, input enable and bias. Thus it might make sense to expose
> > something like a select-pinconf file to activate pin conf settings from
> > userspace."
> 
> This is already present, please define all wanted configurations of pin
> groups and pin group functions, then switch them in runtime. I see no
> need of a coarse grained interface here...
> 
> >  From the emails exchanged I seem to have understood that there is no way to
> > reconfigure slew rate, pull up / down and other properties on the fly.
> 
> I think you still can do all the tasks mentioned above on the recent kernel,
> why not?
> 
> I am not closely familiar with TI AM335x pinmux/pinconf controller, and if
> needed I can look at the datasheet, but I can imagine that there are pins,
> pin groups, and pin group functions controls. Board specific configuration
> of pinmux is given in DTS, you can modify it with DT overlays for instance,
> and selection of pin group functions in runtime is already possible for
> users in runtime. What is missing from the picture, and why do you insist
> on re-introduction of a much worse interface?
> 
> > In the kernel version 4.1.6 that I am using on my custom board, I have fixed
> > the commit f07512e615dd ("pinctrl/pinconfig: add debug interface"). However,
> > this feature was later removed (https://lore.kernel.org/patchwork/patch/1033755/).
> 
> Exactly, the feature is not needed.
> 
> > The patches I've submitted implement some sort of devmem for pinmux. It too can
> > be used in a dangerous way, but it exists and it is used.
> 
> My objection is to giving a "red button" to users, even to users of debugfs.
> 
> If it's possible to keep the "dangerous goods" on developers' side only,
> then this shall be preferable, I believe. And fortunately there is such
> a mechanism.
> 
> > Anyway, the implementation may be wrong but it does highlight a feature that
> > can be useful in testing or prototyping boards but is not present in the kernel.
> > Can we then find a solution that is right for everyone?
> 
> Please see the method above.
> 
> In my understanding the problem you are trying to solve shall be defined
> much more precised. Can you please elaborate on this part thoroughly?
> 
> I still can not grasp a too generic explanation from you, writing of
> totally arbitrary data to controller registers looks senseless to me...
> 
> Assume you are giving a handle to users to write arbitrary data to arbitrary
> I/O mem region, will it solve the problem for you? Of course yes.
> 
> But does it sound like a good and acceptable solution? Of course no.
> Why? You need a better and more fine grained interface, namely write only
> to pinmux I/O mem. Great, that's provided by your change, however another
> important condition is still missing, a user shall write only valid data.
> Thus a higher level of abstraction is wanted:
> 
> * writing data to I/O mem -- not good enough,
> * writing data to pinmux/pinconf I/O mem -- better, but not good enough,
> * writing *valid* data to pinmux/pinconf I/O mem -- that's right.

So, why not start from the feature you removed?
It wrote valid data to pinconf I/O mem.

Thanks and regards,
Dario

> 
> The validity of data is defined by a developer, the abstraction name
> has been mentioned multiple times, it's pin groups and pin group functions.
> 
> --
> Best wishes,
> Vladimir

Re: [PATCH v3 2/3] pinctrl: core: configure pinmux from pins debug file

[Vladimir Zapolskiy]

Hi Dario,

On 5/27/21 10:23 PM, Dario Binacchi wrote:
> Hi Vladimir,
> 
>> Il 24/05/2021 20:52 Vladimir Zapolskiy <vz@mleia.com> ha scritto:
>>
>>   
>> Hi Dario,
>>
>> On 5/24/21 8:28 PM, Dario Binacchi wrote:
>>> Hi Vladimir,
>>>
>>>> Il 21/05/2021 08:44 Vladimir Zapolskiy <vz@mleia.com> ha scritto:
>>>>
>>>>    
>>>> Hello Dario,
>>>>
>>>> On 5/20/21 11:27 PM, Dario Binacchi wrote:
>>>>> The MPUs of some architectures (e.g AM335x) must be in privileged
>>>>> operating mode to write on the pinmux registers. In such cases, where
>>>>> writes will not work from user space, now it can be done from the pins
>>>>
>>>> user space has no connection to the problem you're trying to solve.
>>>>
>>>> Please provide a reasonable rationale for adding a new interface, thank
>>>> you in advance.
>>>>
>>>>> debug file if the platform driver exports the pin_dbg_set() helper among
>>>>> the registered operations.
>>>>>
>>>>> Signed-off-by: Dario Binacchi <dariobin@libero.it>
>>>>
>>>> I strongly object against this new interface.
>>>>
>>>> As Andy've already mentioned you have to operate with defined pin groups
>>>> and functions, and so far you create an interface with an option to
>>>> disasterous misusage, it shall be avoided, because there are better
>>>> options.
>>>>
>>>> What's the issue with a regular declaration of pin groups and functions
>>>> on your SoC? When it's done, you can operate on this level of abstraction,
>>>> there is absolutely no need to add the proposed low-level debug interface.
>>>>
>>>
>>> I quote Drew's words:
>>>
>>> "I think it could be helpful to be able to set the conf_<module>_<pin>
>>> registers directly through debugfs as I can imagine situations where it would
>>> be useful for testing. It is a bit dangerous as the person using it has to be
>>> careful not to change the wrong bits, but they would need to have debugfs mounted
>>> and permissions to write to it."
>>>
>>> "Bits 6:3 are related to what this subsystem would refer to as pin conf
>>> such as slew, input enable and bias. Thus it might make sense to expose
>>> something like a select-pinconf file to activate pin conf settings from
>>> userspace."
>>
>> This is already present, please define all wanted configurations of pin
>> groups and pin group functions, then switch them in runtime. I see no
>> need of a coarse grained interface here...
>>
>>>   From the emails exchanged I seem to have understood that there is no way to
>>> reconfigure slew rate, pull up / down and other properties on the fly.
>>
>> I think you still can do all the tasks mentioned above on the recent kernel,
>> why not?
>>
>> I am not closely familiar with TI AM335x pinmux/pinconf controller, and if
>> needed I can look at the datasheet, but I can imagine that there are pins,
>> pin groups, and pin group functions controls. Board specific configuration
>> of pinmux is given in DTS, you can modify it with DT overlays for instance,
>> and selection of pin group functions in runtime is already possible for
>> users in runtime. What is missing from the picture, and why do you insist
>> on re-introduction of a much worse interface?
>>
>>> In the kernel version 4.1.6 that I am using on my custom board, I have fixed
>>> the commit f07512e615dd ("pinctrl/pinconfig: add debug interface"). However,
>>> this feature was later removed (https://lore.kernel.org/patchwork/patch/1033755/).
>>
>> Exactly, the feature is not needed.
>>
>>> The patches I've submitted implement some sort of devmem for pinmux. It too can
>>> be used in a dangerous way, but it exists and it is used.
>>
>> My objection is to giving a "red button" to users, even to users of debugfs.
>>
>> If it's possible to keep the "dangerous goods" on developers' side only,
>> then this shall be preferable, I believe. And fortunately there is such
>> a mechanism.
>>
>>> Anyway, the implementation may be wrong but it does highlight a feature that
>>> can be useful in testing or prototyping boards but is not present in the kernel.
>>> Can we then find a solution that is right for everyone?
>>
>> Please see the method above.
>>
>> In my understanding the problem you are trying to solve shall be defined
>> much more precised. Can you please elaborate on this part thoroughly?
>>
>> I still can not grasp a too generic explanation from you, writing of
>> totally arbitrary data to controller registers looks senseless to me...
>>
>> Assume you are giving a handle to users to write arbitrary data to arbitrary
>> I/O mem region, will it solve the problem for you? Of course yes.
>>
>> But does it sound like a good and acceptable solution? Of course no.
>> Why? You need a better and more fine grained interface, namely write only
>> to pinmux I/O mem. Great, that's provided by your change, however another
>> important condition is still missing, a user shall write only valid data.
>> Thus a higher level of abstraction is wanted:
>>
>> * writing data to I/O mem -- not good enough,
>> * writing data to pinmux/pinconf I/O mem -- better, but not good enough,
>> * writing *valid* data to pinmux/pinconf I/O mem -- that's right.
> 
> So, why not start from the feature you removed?
> It wrote valid data to pinconf I/O mem.

Nope. The interface you've introduced allows to write invalid data, and
the choice between writing valid data and writing invalid data is given
to a user. Please remove this choice completely, technically it's doable.

> 
>>
>> The validity of data is defined by a developer, the abstraction name
>> has been mentioned multiple times, it's pin groups and pin group functions.

Unfortunately you continue to cling to the broken interface, while I see no
comments from you about asked to consider pin groups and pin group functions.

--
Best wishes,
Vladimir

Re: [PATCH v3 2/3] pinctrl: core: configure pinmux from pins debug file

[Dario Binacchi]

Hi Vladimir,

> Il 27/05/2021 21:57 Vladimir Zapolskiy <vz@mleia.com> ha scritto:
> 
>  
> Hi Dario,
> 
> On 5/27/21 10:23 PM, Dario Binacchi wrote:
> > Hi Vladimir,
> > 
> >> Il 24/05/2021 20:52 Vladimir Zapolskiy <vz@mleia.com> ha scritto:
> >>
> >>   
> >> Hi Dario,
> >>
> >> On 5/24/21 8:28 PM, Dario Binacchi wrote:
> >>> Hi Vladimir,
> >>>
> >>>> Il 21/05/2021 08:44 Vladimir Zapolskiy <vz@mleia.com> ha scritto:
> >>>>
> >>>>    
> >>>> Hello Dario,
> >>>>
> >>>> On 5/20/21 11:27 PM, Dario Binacchi wrote:
> >>>>> The MPUs of some architectures (e.g AM335x) must be in privileged
> >>>>> operating mode to write on the pinmux registers. In such cases, where
> >>>>> writes will not work from user space, now it can be done from the pins
> >>>>
> >>>> user space has no connection to the problem you're trying to solve.
> >>>>
> >>>> Please provide a reasonable rationale for adding a new interface, thank
> >>>> you in advance.
> >>>>
> >>>>> debug file if the platform driver exports the pin_dbg_set() helper among
> >>>>> the registered operations.
> >>>>>
> >>>>> Signed-off-by: Dario Binacchi <dariobin@libero.it>
> >>>>
> >>>> I strongly object against this new interface.
> >>>>
> >>>> As Andy've already mentioned you have to operate with defined pin groups
> >>>> and functions, and so far you create an interface with an option to
> >>>> disasterous misusage, it shall be avoided, because there are better
> >>>> options.
> >>>>
> >>>> What's the issue with a regular declaration of pin groups and functions
> >>>> on your SoC? When it's done, you can operate on this level of abstraction,
> >>>> there is absolutely no need to add the proposed low-level debug interface.
> >>>>
> >>>
> >>> I quote Drew's words:
> >>>
> >>> "I think it could be helpful to be able to set the conf_<module>_<pin>
> >>> registers directly through debugfs as I can imagine situations where it would
> >>> be useful for testing. It is a bit dangerous as the person using it has to be
> >>> careful not to change the wrong bits, but they would need to have debugfs mounted
> >>> and permissions to write to it."
> >>>
> >>> "Bits 6:3 are related to what this subsystem would refer to as pin conf
> >>> such as slew, input enable and bias. Thus it might make sense to expose
> >>> something like a select-pinconf file to activate pin conf settings from
> >>> userspace."
> >>
> >> This is already present, please define all wanted configurations of pin
> >> groups and pin group functions, then switch them in runtime. I see no
> >> need of a coarse grained interface here...
> >>
> >>>   From the emails exchanged I seem to have understood that there is no way to
> >>> reconfigure slew rate, pull up / down and other properties on the fly.
> >>
> >> I think you still can do all the tasks mentioned above on the recent kernel,
> >> why not?
> >>
> >> I am not closely familiar with TI AM335x pinmux/pinconf controller, and if
> >> needed I can look at the datasheet, but I can imagine that there are pins,
> >> pin groups, and pin group functions controls. Board specific configuration
> >> of pinmux is given in DTS, you can modify it with DT overlays for instance,
> >> and selection of pin group functions in runtime is already possible for
> >> users in runtime. What is missing from the picture, and why do you insist
> >> on re-introduction of a much worse interface?
> >>
> >>> In the kernel version 4.1.6 that I am using on my custom board, I have fixed
> >>> the commit f07512e615dd ("pinctrl/pinconfig: add debug interface"). However,
> >>> this feature was later removed (https://lore.kernel.org/patchwork/patch/1033755/).
> >>
> >> Exactly, the feature is not needed.
> >>
> >>> The patches I've submitted implement some sort of devmem for pinmux. It too can
> >>> be used in a dangerous way, but it exists and it is used.
> >>
> >> My objection is to giving a "red button" to users, even to users of debugfs.
> >>
> >> If it's possible to keep the "dangerous goods" on developers' side only,
> >> then this shall be preferable, I believe. And fortunately there is such
> >> a mechanism.
> >>
> >>> Anyway, the implementation may be wrong but it does highlight a feature that
> >>> can be useful in testing or prototyping boards but is not present in the kernel.
> >>> Can we then find a solution that is right for everyone?
> >>
> >> Please see the method above.
> >>
> >> In my understanding the problem you are trying to solve shall be defined
> >> much more precised. Can you please elaborate on this part thoroughly?
> >>
> >> I still can not grasp a too generic explanation from you, writing of
> >> totally arbitrary data to controller registers looks senseless to me...
> >>
> >> Assume you are giving a handle to users to write arbitrary data to arbitrary
> >> I/O mem region, will it solve the problem for you? Of course yes.
> >>
> >> But does it sound like a good and acceptable solution? Of course no.
> >> Why? You need a better and more fine grained interface, namely write only
> >> to pinmux I/O mem. Great, that's provided by your change, however another
> >> important condition is still missing, a user shall write only valid data.
> >> Thus a higher level of abstraction is wanted:
> >>
> >> * writing data to I/O mem -- not good enough,
> >> * writing data to pinmux/pinconf I/O mem -- better, but not good enough,
> >> * writing *valid* data to pinmux/pinconf I/O mem -- that's right.
> > 
> > So, why not start from the feature you removed?
> > It wrote valid data to pinconf I/O mem.
> 
> Nope. The interface you've introduced allows to write invalid data, and
> the choice between writing valid data and writing invalid data is given
> to a user. Please remove this choice completely, technically it's doable.

I was not pointing to my patch, but to patch f07512e615dd ("pinctrl / pinconfig: add debug interface" 
(which has been removed). If I am not missing something it wrote data to the 
pinconf I/O mem in a more controlled way. Could we use this patch as a starting 
point for a new implementation that uses pin groups and pin group functions ?

> 
> > 
> >>
> >> The validity of data is defined by a developer, the abstraction name
> >> has been mentioned multiple times, it's pin groups and pin group functions.
> 
> Unfortunately you continue to cling to the broken interface, while I see no
> comments from you about asked to consider pin groups and pin group functions.

Could you kindly explain to me, with some practical examples, what kind of interface
would you implement ?

Thanks and regards,
Dario

> 
> --
> Best wishes,
> Vladimir

Re: [PATCH v3 2/3] pinctrl: core: configure pinmux from pins debug file

[Vladimir Zapolskiy]

Hi Dario,

On 5/27/21 11:33 PM, Dario Binacchi wrote:
> Hi Vladimir,
> 
>> Il 27/05/2021 21:57 Vladimir Zapolskiy <vz@mleia.com> ha scritto:
>>
>>   
>> Hi Dario,
>>
>> On 5/27/21 10:23 PM, Dario Binacchi wrote:
>>> Hi Vladimir,
>>>
>>>> Il 24/05/2021 20:52 Vladimir Zapolskiy <vz@mleia.com> ha scritto:
>>>>
>>>>    
>>>> Hi Dario,
>>>>
>>>> On 5/24/21 8:28 PM, Dario Binacchi wrote:
>>>>> Hi Vladimir,
>>>>>
>>>>>> Il 21/05/2021 08:44 Vladimir Zapolskiy <vz@mleia.com> ha scritto:
>>>>>>
>>>>>>     
>>>>>> Hello Dario,
>>>>>>
>>>>>> On 5/20/21 11:27 PM, Dario Binacchi wrote:
>>>>>>> The MPUs of some architectures (e.g AM335x) must be in privileged
>>>>>>> operating mode to write on the pinmux registers. In such cases, where
>>>>>>> writes will not work from user space, now it can be done from the pins
>>>>>>
>>>>>> user space has no connection to the problem you're trying to solve.
>>>>>>
>>>>>> Please provide a reasonable rationale for adding a new interface, thank
>>>>>> you in advance.
>>>>>>
>>>>>>> debug file if the platform driver exports the pin_dbg_set() helper among
>>>>>>> the registered operations.
>>>>>>>
>>>>>>> Signed-off-by: Dario Binacchi <dariobin@libero.it>
>>>>>>
>>>>>> I strongly object against this new interface.
>>>>>>
>>>>>> As Andy've already mentioned you have to operate with defined pin groups
>>>>>> and functions, and so far you create an interface with an option to
>>>>>> disasterous misusage, it shall be avoided, because there are better
>>>>>> options.
>>>>>>
>>>>>> What's the issue with a regular declaration of pin groups and functions
>>>>>> on your SoC? When it's done, you can operate on this level of abstraction,
>>>>>> there is absolutely no need to add the proposed low-level debug interface.
>>>>>>
>>>>>
>>>>> I quote Drew's words:
>>>>>
>>>>> "I think it could be helpful to be able to set the conf_<module>_<pin>
>>>>> registers directly through debugfs as I can imagine situations where it would
>>>>> be useful for testing. It is a bit dangerous as the person using it has to be
>>>>> careful not to change the wrong bits, but they would need to have debugfs mounted
>>>>> and permissions to write to it."
>>>>>
>>>>> "Bits 6:3 are related to what this subsystem would refer to as pin conf
>>>>> such as slew, input enable and bias. Thus it might make sense to expose
>>>>> something like a select-pinconf file to activate pin conf settings from
>>>>> userspace."
>>>>
>>>> This is already present, please define all wanted configurations of pin
>>>> groups and pin group functions, then switch them in runtime. I see no
>>>> need of a coarse grained interface here...
>>>>
>>>>>    From the emails exchanged I seem to have understood that there is no way to
>>>>> reconfigure slew rate, pull up / down and other properties on the fly.
>>>>
>>>> I think you still can do all the tasks mentioned above on the recent kernel,
>>>> why not?
>>>>
>>>> I am not closely familiar with TI AM335x pinmux/pinconf controller, and if
>>>> needed I can look at the datasheet, but I can imagine that there are pins,
>>>> pin groups, and pin group functions controls. Board specific configuration
>>>> of pinmux is given in DTS, you can modify it with DT overlays for instance,
>>>> and selection of pin group functions in runtime is already possible for
>>>> users in runtime. What is missing from the picture, and why do you insist
>>>> on re-introduction of a much worse interface?
>>>>
>>>>> In the kernel version 4.1.6 that I am using on my custom board, I have fixed
>>>>> the commit f07512e615dd ("pinctrl/pinconfig: add debug interface"). However,
>>>>> this feature was later removed (https://lore.kernel.org/patchwork/patch/1033755/).
>>>>
>>>> Exactly, the feature is not needed.
>>>>
>>>>> The patches I've submitted implement some sort of devmem for pinmux. It too can
>>>>> be used in a dangerous way, but it exists and it is used.
>>>>
>>>> My objection is to giving a "red button" to users, even to users of debugfs.
>>>>
>>>> If it's possible to keep the "dangerous goods" on developers' side only,
>>>> then this shall be preferable, I believe. And fortunately there is such
>>>> a mechanism.
>>>>
>>>>> Anyway, the implementation may be wrong but it does highlight a feature that
>>>>> can be useful in testing or prototyping boards but is not present in the kernel.
>>>>> Can we then find a solution that is right for everyone?
>>>>
>>>> Please see the method above.
>>>>
>>>> In my understanding the problem you are trying to solve shall be defined
>>>> much more precised. Can you please elaborate on this part thoroughly?
>>>>
>>>> I still can not grasp a too generic explanation from you, writing of
>>>> totally arbitrary data to controller registers looks senseless to me...
>>>>
>>>> Assume you are giving a handle to users to write arbitrary data to arbitrary
>>>> I/O mem region, will it solve the problem for you? Of course yes.
>>>>
>>>> But does it sound like a good and acceptable solution? Of course no.
>>>> Why? You need a better and more fine grained interface, namely write only
>>>> to pinmux I/O mem. Great, that's provided by your change, however another
>>>> important condition is still missing, a user shall write only valid data.
>>>> Thus a higher level of abstraction is wanted:
>>>>
>>>> * writing data to I/O mem -- not good enough,
>>>> * writing data to pinmux/pinconf I/O mem -- better, but not good enough,
>>>> * writing *valid* data to pinmux/pinconf I/O mem -- that's right.
>>>
>>> So, why not start from the feature you removed?
>>> It wrote valid data to pinconf I/O mem.
>>
>> Nope. The interface you've introduced allows to write invalid data, and
>> the choice between writing valid data and writing invalid data is given
>> to a user. Please remove this choice completely, technically it's doable.
> 
> I was not pointing to my patch, but to patch f07512e615dd ("pinctrl / pinconfig: add debug interface"
> (which has been removed). If I am not missing something it wrote data to the
> pinconf I/O mem in a more controlled way. 

Right, but still it's not good enough, the reasons are given in the message
of the commit, which reverts the change. And again and again, it is not based
on the most reasonable user-visible abstractions as pin groups and functions.

> Could we use this patch as a starting point for a new implementation that
> uses pin groups and pin group functions ?

Well, you can include this patch into your development process as one of
the initial stages, but it makes no sense to resurrect it in the vanilla.

>>
>>>
>>>>
>>>> The validity of data is defined by a developer, the abstraction name
>>>> has been mentioned multiple times, it's pin groups and pin group functions.
>>
>> Unfortunately you continue to cling to the broken interface, while I see no
>> comments from you about asked to consider pin groups and pin group functions.
> 
> Could you kindly explain to me, with some practical examples, what kind of interface
> would you implement ?
> 

So far I'm quite happy with device tree overlays, for me it gives a solution
to a problem which you're trying to address, but that's my guess only, because
I still don't quite understand what do you seek for... Changing pin slew rates
in runtime or something else? Why can't your keep this strange function out of
vanilla or just make it specific to the AM335x controller?

Any reasonable pinmux/pinctrl "debug" interface anyway should be based on
board device trees/firmware, that's the right place to describe SoC/board
specific valid configurations of the controller.

--
Best wishes,
Vladimir

Re: [PATCH v3 2/3] pinctrl: core: configure pinmux from pins debug file

[Linus Walleij]

On Thu, May 27, 2021 at 10:33 PM Dario Binacchi <dariobin@libero.it> wrote:
> > Il 27/05/2021 21:57 Vladimir Zapolskiy <vz@mleia.com> ha scritto:

> > Unfortunately you continue to cling to the broken interface, while I see no
> > comments from you about asked to consider pin groups and pin group functions.
>
> Could you kindly explain to me, with some practical examples, what kind of interface
> would you implement ?

I am not fully understanding this discussion.

I get the feeling that this is caused by the early architectural decisions with
pinctrl-single to put all configuration of pin groups and functions per pin into
the device tree.

Tony specifically wanted this because what he gets from TI are some raw
ASIC data dumps from the designers, that he could make a script to process
into device tree rather than into .h files, and get this out of the kernel.
(As I remember it, Tony correct me if I'm wrong.)

This makes it hard to align some concepts of the pin control subsystem such
as functions and groups because pinctrl-single assume a 1-to-1 mapping
between one pin and one group, which in turn has a 1-to-many mapping
to functions.

Is the patch trying to debugfs around this somehow?

If this hack is only needed for pinctrl-single.c then it should be placed in
that driver, so Tony can review it and maintain it as applicable in that
driver's context only, not in the pinctrl core as it appears the general
applicability for other drivers is not there.

Would this really be useful for other drivers than pinctrl-single.c?

Yours,
Linus Walleij

Re: [PATCH v3 2/3] pinctrl: core: configure pinmux from pins debug file

[Tony Lindgren]

* Linus Walleij <linus.walleij@linaro.org> [210528 09:08]:
> On Thu, May 27, 2021 at 10:33 PM Dario Binacchi <dariobin@libero.it> wrote:
> > > Il 27/05/2021 21:57 Vladimir Zapolskiy <vz@mleia.com> ha scritto:
> 
> > > Unfortunately you continue to cling to the broken interface, while I see no
> > > comments from you about asked to consider pin groups and pin group functions.
> >
> > Could you kindly explain to me, with some practical examples, what kind of interface
> > would you implement ?
> 
> I am not fully understanding this discussion.
> 
> I get the feeling that this is caused by the early architectural decisions with
> pinctrl-single to put all configuration of pin groups and functions per pin into
> the device tree.
>
> Tony specifically wanted this because what he gets from TI are some raw
> ASIC data dumps from the designers, that he could make a script to process
> into device tree rather than into .h files, and get this out of the kernel.
> (As I remember it, Tony correct me if I'm wrong.)

Yeah the idea was to avoid stuffing even more SoC specific data into the
kernel and rather use devicetree data only for the booted SoC.
 
> This makes it hard to align some concepts of the pin control subsystem such
> as functions and groups because pinctrl-single assume a 1-to-1 mapping
> between one pin and one group, which in turn has a 1-to-many mapping
> to functions.
> 
> Is the patch trying to debugfs around this somehow?
> 
> If this hack is only needed for pinctrl-single.c then it should be placed in
> that driver, so Tony can review it and maintain it as applicable in that
> driver's context only, not in the pinctrl core as it appears the general
> applicability for other drivers is not there.
> 
> Would this really be useful for other drivers than pinctrl-single.c?

I'd rather go with a generic interface. I think it should work if we only
allow enabling and disabling of unclaimed pingroups from sysfs. And then
we can also allow creating new pingroups for unclaimed pins if needed.

Regards,

Tony

Re: [PATCH v3 2/3] pinctrl: core: configure pinmux from pins debug file

[Dario Binacchi]

Hi,

> Il 02/06/2021 07:03 Tony Lindgren <tony@atomide.com> ha scritto:
> 
>  
> * Linus Walleij <linus.walleij@linaro.org> [210528 09:08]:
> > On Thu, May 27, 2021 at 10:33 PM Dario Binacchi <dariobin@libero.it> wrote:
> > > > Il 27/05/2021 21:57 Vladimir Zapolskiy <vz@mleia.com> ha scritto:
> > 
> > > > Unfortunately you continue to cling to the broken interface, while I see no
> > > > comments from you about asked to consider pin groups and pin group functions.
> > >
> > > Could you kindly explain to me, with some practical examples, what kind of interface
> > > would you implement ?
> > 
> > I am not fully understanding this discussion.
> > 
> > I get the feeling that this is caused by the early architectural decisions with
> > pinctrl-single to put all configuration of pin groups and functions per pin into
> > the device tree.
> >
> > Tony specifically wanted this because what he gets from TI are some raw
> > ASIC data dumps from the designers, that he could make a script to process
> > into device tree rather than into .h files, and get this out of the kernel.
> > (As I remember it, Tony correct me if I'm wrong.)
> 
> Yeah the idea was to avoid stuffing even more SoC specific data into the
> kernel and rather use devicetree data only for the booted SoC.
>  
> > This makes it hard to align some concepts of the pin control subsystem such
> > as functions and groups because pinctrl-single assume a 1-to-1 mapping
> > between one pin and one group, which in turn has a 1-to-many mapping
> > to functions.
> > 
> > Is the patch trying to debugfs around this somehow?
> > 
> > If this hack is only needed for pinctrl-single.c then it should be placed in
> > that driver, so Tony can review it and maintain it as applicable in that
> > driver's context only, not in the pinctrl core as it appears the general
> > applicability for other drivers is not there.
> > 
> > Would this really be useful for other drivers than pinctrl-single.c?
> 
> I'd rather go with a generic interface. I think it should work if we only
> allow enabling and disabling of unclaimed pingroups from sysfs. And then
> we can also allow creating new pingroups for unclaimed pins if needed.
> 

Could you kindly explain to me, with some practical examples, what kind of interface
would you implement ? Or something similar to start from that is already in the Kernel?

Thanks and regards,
Dario

> Regards,
> 
> Tony