Message-ID: <20230706150631.GA850348@EPUAKYIW0A6A>
Date:   Thu, 6 Jul 2023 15:06:32 +0000
From:   Oleksii Moisieiev <Oleksii_Moisieiev@...m.com>
To:     Cristian Marussi <cristian.marussi@....com>
CC:     "sudeep.holla@....com" <sudeep.holla@....com>,
        Rob Herring <robh+dt@...nel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        Conor Dooley <conor+dt@...nel.org>,
        Linus Walleij <linus.walleij@...aro.org>,
        "linux-arm-kernel@...ts.infradead.org" 
        <linux-arm-kernel@...ts.infradead.org>,
        "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linux-gpio@...r.kernel.org" <linux-gpio@...r.kernel.org>
Subject: Re: [PATCH v3 2/4] firmware: arm_scmi: Add SCMI v3.2 pincontrol
 protocol basic support

Hi Cristian,

On Thu, Jul 06, 2023 at 03:42:34PM +0100, Cristian Marussi wrote:
> On Thu, Jul 06, 2023 at 02:09:38PM +0000, Oleksii Moisieiev wrote:
> > Hi Cristian,
> > 
> 
> Hi Oleksii,
> 
> > Sorry for late answer.
> > Please see below.
> > 
> 
> No worries, not late at all.
> 
> > On Mon, Jul 03, 2023 at 11:16:47AM +0100, Cristian Marussi wrote:
> > > On Tue, Jun 06, 2023 at 04:22:27PM +0000, Oleksii Moisieiev wrote:
> > > > scmi: Introduce pinctrl SCMI protocol driver
> > > >
> > > 
> > > Hi Oleksii,
> > > 
> > > spurios line above.
> > 
> > Yep thanks, I will remove.
> > 
> > > 
> > > > Add basic implementation of the SCMI v3.2 pincontrol protocol
> > > > excluding GPIO support. All pinctrl related callbacks and operations
> > > > are exposed in the include/linux/scmi_protocol.h
> > > > 
> > > 
> > > As Andy said already, you can drop the second sentence here, but I would
> > > ALSO drop the GPIO part in the first sentence, since there is nothing
> > > specific to GPIO in the SCMI spec and this patch is about the SCMI protocol
> > > not the pinctrl driver.
> > >
> > 
> > I've added few words about GPIO because in v2 series Michal Simek asked
> > about it: https://urldefense.com/v3/__https://lore.kernel.org/linux-arm-kernel/5bf0e975-d314-171f-b6a8-c1c1c7198cd3@amd.com/__;!!GF_29dbcQIUBPA!1eit_iJDFpGMDrWcBk1hej0zgnDQilbjCvnU-4h-t8eL2GbpNrvXpdWEo7pttPI8rMae2gJAfCgRrkeiq5Qrr7OeqxXTiQ$ [lore[.]kernel[.]org]
> > So I've decided to mention that there is still no GPIO support in the
> > commit message to avoid this type of questions in future. But I agree
> > that the commit message looks weird and will try to rephrase it.
> >
> 
> Yes I remember that and I understand why you want to mention this, what
> I am saying is that anyway is NOT something related to the SCMI Pinctrl
> spec AFAIU (I may be wrong): I mean GPIO support is something you can
> build on top of Pinctrl SCMI spec and driver NOT something that has
> still to be added to the spec right ? and this patch is about supporting
> the new SCMI protocol, so I certainly agree that can be fine to point
> out that GPIO support is missing, just maybe this is a comment more
> appropriate to be added to the Pinctrl SCMI driver than to the Pinctrl
> SCMI protocol layer...(but maybe the Pinctrl subsys maintainer will
> disagree on this :P)

Yeah, you're right. Just looked into the spec to ensure. I will rework this part.
Pinctrl maintainer will definitely disagree because GPIO is a separate
subsystem.

> 
> > > > Signed-off-by: Oleksii Moisieiev <oleksii_moisieiev@...m.com>
> > > > ---
> > > >  MAINTAINERS                           |   6 +
> > > >  drivers/firmware/arm_scmi/Makefile    |   2 +-
> > > >  drivers/firmware/arm_scmi/driver.c    |   2 +
> > > >  drivers/firmware/arm_scmi/pinctrl.c   | 836 ++++++++++++++++++++++++++
> > > >  drivers/firmware/arm_scmi/protocols.h |   1 +
> > > >  include/linux/scmi_protocol.h         |  47 ++
> > > >  6 files changed, 893 insertions(+), 1 deletion(-)
> > > >  create mode 100644 drivers/firmware/arm_scmi/pinctrl.c
> > > > 
> > > > diff --git a/MAINTAINERS b/MAINTAINERS
> > > > index 0dab9737ec16..297b2512963d 100644
> > > > --- a/MAINTAINERS
> > > > +++ b/MAINTAINERS
> > > > @@ -20522,6 +20522,12 @@ F:	include/linux/sc[mp]i_protocol.h
> > > >  F:	include/trace/events/scmi.h
> > > >  F:	include/uapi/linux/virtio_scmi.h
> > > >  
> > > > +PINCTRL DRIVER FOR SYSTEM CONTROL & POWER/MANAGEMENT INTERFACE (SCPI/SCMI)
> > > 
> > > SCPI is a leftover here I suppose...
> > > 
> > 
> > Thanks. I'll fix it.

[snip]

> > > > +
> > > 
> > > A small note related to Andy remarks about directly embedding here pinctrl
> > > subsystem structures (like pingroup / pinfucntion) that I forgot to say
> > > in my reply to him.
> > > 
> > > These structs above indeed are very similar to the Pinctrl ones but this is
> > > the protocol layer inside SCMI, I would not mix here stuff from the Pinctrl
> > > subsystem which is, at the end the, one of the possible users of this layer
> > > (via the SCMI pinctrl driver) but not necessarily the only one in the
> > > future; moreover Pinctrl subsystem is not even needed at all if you think
> > > about a testing scenario, so I would not build up a dependency here between
> > > SCMI and Pinctrl by using Pinctrl structures...what if these latter change
> > > in the future ?
> > > 
> > > All of this to just say this is fine for me as it is now :D
> > > 
> > I agree with you.
> > What we currently have is that scmi pinctrl protocol is not bound to
> > pinctrl-subsystem so in case of some changes in the pinctrl - no need to
> > change the protocol implementation.
> > Also, as I mentioned in v2: I can't use pincfunction 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.
> > 
> > Pinctrl subsystem was designed to use statically defined
> > pins/groups/functions so we can't use those structures on lazy
> > allocations.
> > 
> 
> Indeed, I forgot that additional reason.
> 
> > 
> > > > +struct scmi_pin_info {
> > > > +	bool present;
> > > > +	char name[SCMI_MAX_STR_SIZE];
> > > > +};
> > > > +
> > > > +struct scmi_pinctrl_info {
> > > > +	u32 version;
> > > > +	int nr_groups;
> > > > +	int nr_functions;
> > > > +	int nr_pins;
> > > > +	struct scmi_group_info *groups;
> > > > +	struct scmi_function_info *functions;
> > > > +	struct scmi_pin_info *pins;
> > > > +};
> > > > +
> > > > +static int scmi_pinctrl_attributes_get(const struct scmi_protocol_handle *ph,
> > > > +				       struct scmi_pinctrl_info *pi)
> > > > +{
> > > > +	int ret;
> > > > +	struct scmi_xfer *t;
> > > > +	struct scmi_msg_pinctrl_protocol_attributes *attr;
> > > > +
> > > > +	if (!pi)
> > > > +		return -EINVAL;
> > > 
> > > You can drop this, cannot happen given the code paths.
> > >
> > 
> > Ok. thanks.
> > 
> > > > +
> > > > +	ret = ph->xops->xfer_get_init(ph, PROTOCOL_ATTRIBUTES,
> > > > +				      0, sizeof(*attr), &t);
> > > > +	if (ret)
> > > > +		return ret;
> > > > +
> > > > +	attr = t->rx.buf;
> > > > +
> > > > +	ret = ph->xops->do_xfer(ph, t);
> > > > +	if (!ret) {
> > > > +		pi->nr_functions = GET_FUNCTIONS_NR(attr->attributes_high);
> > > > +		pi->nr_groups = GET_GROUPS_NR(attr->attributes_low);
> > > > +		pi->nr_pins = GET_PINS_NR(attr->attributes_low);
> > > > +	}
> > > > +
> > > > +	ph->xops->xfer_put(ph, t);
> > > > +	return ret;
> > > > +}
> > > > +
> > > > +static int scmi_pinctrl_get_count(const struct scmi_protocol_handle *ph,
> > > > +				  enum scmi_pinctrl_selector_type type)
> > > > +{
> > > > +	struct scmi_pinctrl_info *pi;
> > > > +
> > > > +	pi = ph->get_priv(ph);
> > > > +	if (!pi)
> > > > +		return -ENODEV;
> > > 
> > > You dont need to check for NULL here and nowhere else.
> > > You set protocol private data with set_priv at the end of protocol init
> > > which is called as soon as a user tries to use this protocol operations,
> > > so it cannot ever be NULL in any of these following ops.
> > > 
> > 
> > And what if I call set_priv(ph, NULL) on init stage?
> > As I can see there is no check for NULL in scmi_set_protocol_priv. So
> > theoretically I'm able to set ph->priv = NULL. Or did I missed some check in
> > SCMI driver? Or maybe priv = NULL is expected scenario and I shouldn't
> > return error here?
> 
> Well, you are right that you could set periv to NULL, but the whole
> point of set_priv/get_priv helpers are to help you protocol-writer to
> store your private data at init for future usage while processing the
> protocol operations that you, protocol-writer, are implementing; the
> idea of all of this 'dancing' around protocol_handle was to ease the
> developement of protocols by exposing a limited, common and well
> controlled interface to use to build/send messages (ph->xops) while
> hiding some internals related to protocol stack init that are handled
> by the core for you.
> 
> The priv data are only set and get optionally by You depending on the
> need of the protocol, so unless you can dynamically set, at runtime, priv
> to NULL or not-NULL depending on the outcome of the init, you should very
> well know at coding time if your priv could possibly be ever NULL or it
> cannot be NULL at all (like in this case it seems to me): so the check
> seemed to me redundant...
> 
> ...clearly, beside trying to help the protocol devel, the SCMI core
> protocol 'framework' cannot prevent you from shooting yourself in the
> foot if you want :P
> 
> Thanks,
> Cristian
>


That's why I was puzzled. Trying to shoot myself in the knee is what I've mostly
tried while written unit tests. Probably just need to write less tests
:).
I'll remove checks.

Best regards,
Oleksii.

Powered by blists - more mailing lists