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