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

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