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@xxxxxxx/__;!!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@xxxxxxxx> > > > > --- > > > > 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.
