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@xxxxxxxxx> 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@xxxxxxxxx>
> >
> > 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