Re: [RFC v2 2/3] pinctrl: Implementation of the generic scmi-pinctrl driver

[Cristian Marussi]

On Fri, May 12, 2023 at 12:18:03PM +0000, Oleksii Moisieiev wrote:
> Hello Cristian,
> 
> On Fri, May 12, 2023 at 10:04:41AM +0100, Cristian Marussi wrote:
> > On Thu, May 11, 2023 at 01:15:46PM +0000, Oleksii Moisieiev wrote:
> > > Hello Andy,
> > > 
> > > On 05.05.23 23:35, andy.shevchenko@xxxxxxxxx wrote:
> > > > Wed, Apr 26, 2023 at 01:26:37PM +0000, Oleksii Moisieiev kirjoitti:
> > > >> scmi-pinctrl driver implements pinctrl driver interface and using
> > > >> SCMI protocol to redirect messages from pinctrl subsystem SDK to
> > > >> SCP firmware, which does the changes in HW.
> > > >>
> > > >> This setup expects SCP firmware (or similar system, such as ATF)
> > > >> to be installed on the platform, which implements pinctrl driver
> > > >> for the specific platform.
> > > >>
> > > >> SCMI-Pinctrl driver should be configured from the device-tree and uses
> > > >> generic device-tree mappings for the configuration.
> > > > 
> > > > ...
> > > > 
> > > >> +#include <linux/device.h>
> > > >> +#include <linux/err.h>
> > > > 
> > > >> +#include <linux/of.h>
> > > > 
> > > > I do not see any user of this header. Do you?
> > > > 
> > > Yes, thanks. Removing
> > > 
> > > >> +#include <linux/module.h>
> > > >> +#include <linux/seq_file.h>
> > > >> +
> > > >> +#include <linux/pinctrl/machine.h>
> > > >> +#include <linux/pinctrl/pinconf.h>
> > > >> +#include <linux/pinctrl/pinconf-generic.h>
> > > >> +#include <linux/pinctrl/pinctrl.h>
> > > >> +#include <linux/pinctrl/pinmux.h>
> > > > 
> > > >> +#include <linux/scmi_protocol.h>
> > > >> +#include <linux/slab.h>
> > > > 
> > > > Please, move these two to the upper group of the generic headers.
> > > > 
> > > Thanks, fixed.
> > > 
> > > >> +struct scmi_pinctrl_funcs {
> > > >> +	unsigned int num_groups;
> > > >> +	const char **groups;
> > > >> +};
> > > > 
> > > > Please, use struct pinfunction.
> > > >
> > > I can't use pincfunction here because it has the following groups 
> > > definition:
> > > const char * const *groups;
> > > 
> > > Which is meant to be constantly allocated.
> > > So I when I try to gather list of groups in 
> > > pinctrl_scmi_get_function_groups I will receive compilation error.
> > > 
> > 
> > Maybe this is a further signal that we should re-evaluate the benefits of
> > the lazy allocations you now perform during protocol initialization
> > instead of querying and allocating statically all the info structs about
> > existing resources.
> > 
> > Not saying that is necessarily bad, I understood your points about reducing
> > the number of SCMI queries during boot and let pinctrl subsystem trigger only
> > the strictly needed one, just saying maybe good to reason a bit more about this
> > once V3 is posted. (i.e. I could bother you more :P ..)
> > 
> > Thanks,
> > Cristian
> > 
> > P.S. [off-topic]: remember to use get_maintainer.pl as advised elsewhere
> > to include proper maintainers (and their bots)
> 
> That's a good point to think about. Actually, functions are the only
> thing that should be cached on pinctrl side. And we need it specifically
> because groups in each function are presented by names, not selectors.
> Maybe It's better to move this caching to pinctrl scmi driver. But, from
> the other side - storing group names for each function is Linux Kernel
> specific implementation and we probably don't want to add some specific
> case to the Generic protocol driver.
> 
> I think I would leave it as in V3 so we can continue discussion.
> 

Sure, let's review/rediscuss this on top V3.

Thanks,
Cristian