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

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