[PATCH v3 2/4] firmware: arm_scmi: Add SCMI v3.2 pincontrol protocol basic support
Oleksii Moisieiev
Oleksii_Moisieiev at epam.com
Thu Jul 6 07:09:38 PDT 2023
Hi Cristian,
Sorry for late answer.
Please see below.
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://lore.kernel.org/linux-arm-kernel/5bf0e975-d314-171f-b6a8-c1c1c7198cd3@amd.com/
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.
> > Signed-off-by: Oleksii Moisieiev <oleksii_moisieiev at epam.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.
> > +M: Oleksii Moisieiev <oleksii_moisieiev at epam.com>
> > +L: linux-arm-kernel at lists.infradead.org
> > +S: Maintained
> > +F: drivers/firmware/arm_scmi/pinctrl.c
> > +
> > SYSTEM RESET/SHUTDOWN DRIVERS
> > M: Sebastian Reichel <sre at kernel.org>
> > L: linux-pm at vger.kernel.org
> > diff --git a/drivers/firmware/arm_scmi/Makefile b/drivers/firmware/arm_scmi/Makefile
> > index b31d78fa66cc..603430ec0bfe 100644
> > --- a/drivers/firmware/arm_scmi/Makefile
> > +++ b/drivers/firmware/arm_scmi/Makefile
> > @@ -10,7 +10,7 @@ scmi-transport-$(CONFIG_ARM_SCMI_TRANSPORT_SMC) += smc.o
> > scmi-transport-$(CONFIG_ARM_SCMI_HAVE_MSG) += msg.o
> > scmi-transport-$(CONFIG_ARM_SCMI_TRANSPORT_VIRTIO) += virtio.o
> > scmi-transport-$(CONFIG_ARM_SCMI_TRANSPORT_OPTEE) += optee.o
> > -scmi-protocols-y = base.o clock.o perf.o power.o reset.o sensors.o system.o voltage.o powercap.o
> > +scmi-protocols-y = base.o clock.o perf.o power.o reset.o sensors.o system.o voltage.o powercap.o pinctrl.o
> > scmi-module-objs := $(scmi-driver-y) $(scmi-protocols-y) $(scmi-transport-y)
> >
> > obj-$(CONFIG_ARM_SCMI_PROTOCOL) += scmi-core.o
> > diff --git a/drivers/firmware/arm_scmi/driver.c b/drivers/firmware/arm_scmi/driver.c
> > index 5be931a07c84..a9fd337b9596 100644
> > --- a/drivers/firmware/arm_scmi/driver.c
> > +++ b/drivers/firmware/arm_scmi/driver.c
> > @@ -3025,6 +3025,7 @@ static int __init scmi_driver_init(void)
> > scmi_voltage_register();
> > scmi_system_register();
> > scmi_powercap_register();
> > + scmi_pinctrl_register();
> >
> > return platform_driver_register(&scmi_driver);
> > }
> > @@ -3042,6 +3043,7 @@ static void __exit scmi_driver_exit(void)
> > scmi_voltage_unregister();
> > scmi_system_unregister();
> > scmi_powercap_unregister();
> > + scmi_pinctrl_unregister();
> >
> > scmi_transports_exit();
> >
> > diff --git a/drivers/firmware/arm_scmi/pinctrl.c b/drivers/firmware/arm_scmi/pinctrl.c
> > new file mode 100644
> > index 000000000000..fc0fcc26dfb6
> > --- /dev/null
> > +++ b/drivers/firmware/arm_scmi/pinctrl.c
> > @@ -0,0 +1,836 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * System Control and Management Interface (SCMI) Pinctrl Protocol
> > + *
> > + * Copyright (C) 2023 EPAM
> > + */
> > +
> > +#include <linux/module.h>
> > +#include <linux/scmi_protocol.h>
> > +#include <linux/slab.h>
> > +
> > +#include "protocols.h"
> > +
> > +#define REG_TYPE_BITS GENMASK(9, 8)
> > +#define REG_CONFIG GENMASK(7, 0)
> > +
> > +#define GET_GROUPS_NR(x) le32_get_bits((x), GENMASK(31, 16))
> > +#define GET_PINS_NR(x) le32_get_bits((x), GENMASK(15, 0))
> > +#define GET_FUNCTIONS_NR(x) le32_get_bits((x), GENMASK(15, 0))
> > +
> > +#define EXT_NAME_FLAG(x) le32_get_bits((x), BIT(31))
> > +#define NUM_ELEMS(x) le32_get_bits((x), GENMASK(15, 0))
> > +
> > +#define REMAINING(x) le32_get_bits((x), GENMASK(31, 16))
> > +#define RETURNED(x) le32_get_bits((x), GENMASK(11, 0))
> > +
> > +enum scmi_pinctrl_protocol_cmd {
> > + PINCTRL_ATTRIBUTES = 0x3,
> > + PINCTRL_LIST_ASSOCIATIONS = 0x4,
> > + PINCTRL_CONFIG_GET = 0x5,
> > + PINCTRL_CONFIG_SET = 0x6,
> > + PINCTRL_FUNCTION_SELECT = 0x7,
> > + PINCTRL_REQUEST = 0x8,
> > + PINCTRL_RELEASE = 0x9,
> > + PINCTRL_NAME_GET = 0xa,
> > + PINCTRL_SET_PERMISSIONS = 0xb
> > +};
> > +
> > +struct scmi_msg_conf_set {
> > + __le32 identifier;
> > + __le32 attributes;
> > + __le32 config_value;
> > +};
> > +
> > +struct scmi_msg_conf_get {
> > + __le32 identifier;
> > + __le32 attributes;
> > +};
> > +
> > +struct scmi_msg_pinctrl_protocol_attributes {
> > + __le32 attributes_low;
> > + __le32 attributes_high;
> > +};
> > +
> > +struct scmi_msg_pinctrl_attributes {
> > + __le32 identifier;
> > + __le32 flags;
> > +};
> > +
> > +struct scmi_resp_pinctrl_attributes {
> > + __le32 attributes;
> > + u8 name[SCMI_SHORT_NAME_MAX_SIZE];
> > +};
> > +
> > +struct scmi_msg_pinctrl_list_assoc {
> > + __le32 identifier;
> > + __le32 flags;
> > + __le32 index;
> > +};
> > +
> > +struct scmi_resp_pinctrl_list_assoc {
> > + __le32 flags;
> > + __le16 array[];
> > +};
> > +
> > +struct scmi_msg_func_set {
> > + __le32 identifier;
> > + __le32 function_id;
> > + __le32 flags;
> > +};
> > +
> > +struct scmi_msg_request {
> > + __le32 identifier;
> > + __le32 flags;
> > +};
> > +
> > +struct scmi_group_info {
> > + bool present;
> > + char name[SCMI_MAX_STR_SIZE];
> > + unsigned int *group_pins;
> > + unsigned int nr_pins;
> > +};
> > +
> > +struct scmi_function_info {
> > + bool present;
> > + char name[SCMI_MAX_STR_SIZE];
> > + unsigned int *groups;
> > + unsigned int nr_groups;
> > +};
> > +
>
> 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.
> > +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?
> > +
> > + switch (type) {
> > + case PIN_TYPE:
> > + return pi->nr_pins;
> > + case GROUP_TYPE:
> > + return pi->nr_groups;
> > + case FUNCTION_TYPE:
> > + return pi->nr_functions;
> > + default:
> > + return -EINVAL;
> > + }
> > +}
> > +
> > +static int scmi_pinctrl_validate_id(const struct scmi_protocol_handle *ph,
> > + u32 identifier,
> > + enum scmi_pinctrl_selector_type type)
> > +{
> > + int value;
> > +
> > + value = scmi_pinctrl_get_count(ph, type);
> > + if (value < 0)
> > + return value;
> > +
> > + if (identifier >= value)
> > + return -EINVAL;
> > +
> > + return 0;
> > +}
> > +
> > +static int scmi_pinctrl_attributes(const struct scmi_protocol_handle *ph,
> > + enum scmi_pinctrl_selector_type type,
> > + u32 selector, char *name,
> > + unsigned int *n_elems)
> > +{
> > + int ret;
> > + struct scmi_xfer *t;
> > + struct scmi_msg_pinctrl_attributes *tx;
> > + struct scmi_resp_pinctrl_attributes *rx;
> > +
> > + if (!name)
> > + return -EINVAL;
> > +
> > + ret = scmi_pinctrl_validate_id(ph, selector, type);
> > + if (ret)
> > + return ret;
> > +
> > + ret = ph->xops->xfer_get_init(ph, PINCTRL_ATTRIBUTES, sizeof(*tx),
> > + sizeof(*rx), &t);
> > + if (ret)
> > + return ret;
> > +
> > + tx = t->tx.buf;
> > + rx = t->rx.buf;
> > + tx->identifier = cpu_to_le32(selector);
> > + tx->flags = cpu_to_le32(type);
> > +
> > + ret = ph->xops->do_xfer(ph, t);
> > + if (!ret) {
> > + if (n_elems)
> > + *n_elems = NUM_ELEMS(rx->attributes);
> > +
> > + strscpy(name, rx->name, SCMI_SHORT_NAME_MAX_SIZE);
> > + }
> > +
> > + ph->xops->xfer_put(ph, t);
> > +
> > + /*
> > + * If supported overwrite short name with the extended one;
> > + * on error just carry on and use already provided short name.
> > + */
> > + if (!ret && EXT_NAME_FLAG(rx->attributes))
> > + ph->hops->extended_name_get(ph, PINCTRL_NAME_GET, selector,
> > + (u32 *)&type, name,
> > + SCMI_MAX_STR_SIZE);
> > + return ret;
> > +}
> > +
>
> [snip]
>
> > +
> > +static int scmi_pinctrl_get_group_info(const struct scmi_protocol_handle *ph,
> > + u32 selector,
> > + struct scmi_group_info *group)
> > +{
> > + int ret;
> > +
> > + if (!group)
> > + return -EINVAL;
> > +
> > + ret = scmi_pinctrl_attributes(ph, GROUP_TYPE, selector,
> > + group->name,
> > + &group->nr_pins);
> > + if (ret)
> > + return ret;
> > +
> > + if (!group->nr_pins) {
> > + dev_err(ph->dev, "Group %d has 0 elements", selector);
> > + return -ENODATA;
> > + }
> > +
> > + group->group_pins = devm_kmalloc_array(ph->dev, group->nr_pins,
> > + sizeof(*group->group_pins),
> > + GFP_KERNEL);
> > + if (!group->group_pins)
> > + return -ENOMEM;
>
> This is a lazy-allocation happening outside of the protocol init path so you
> are rigthly tracking it manually here and in protocol_deinit, BUT you
> should not use devm_ helpers, it is fuorviating even though harmless;
> just plain kmalloc/kcalloc/kfree will do. (As I said oin the reply to
> Andy I miss this previously, apologies)
>
Thank you and Andy for the observations. I will refactor.
> > +
> > + ret = scmi_pinctrl_list_associations(ph, selector, GROUP_TYPE,
> > + group->nr_pins, group->group_pins);
> > + if (ret) {
> > + devm_kfree(ph->dev, group->group_pins);
>
> kfree
>
Ok.
> > + return ret;
> > + }
> > +
> > + group->present = true;
> > + return 0;
> > +}
> > +
> > +static int scmi_pinctrl_get_group_name(const struct scmi_protocol_handle *ph,
> > + u32 selector, const char **name)
> > +{
> > + struct scmi_pinctrl_info *pi;
> > +
> > + if (!name)
> > + return -EINVAL;
> > +
> > + pi = ph->get_priv(ph);
> > + if (!pi)
>
> Ditto.
>
> > + return -EINVAL;
> > +
> > + if (selector > pi->nr_groups)
> > + return -EINVAL;
>
> selector >= ?
>
Right. Thanks.
> > +
> > + if (!pi->groups[selector].present) {
> > + int ret;
> > +
> > + ret = scmi_pinctrl_get_group_info(ph, selector,
> > + &pi->groups[selector]);
> > + if (ret)
> > + return ret;
> > + }
> > +
> > + *name = pi->groups[selector].name;
> > +
> > + return 0;
> > +}
> > +
> > +static int scmi_pinctrl_get_group_pins(const struct scmi_protocol_handle *ph,
> > + u32 selector, const unsigned int **pins,
> > + unsigned int *nr_pins)
> > +{
> > + struct scmi_pinctrl_info *pi;
> > +
> > + if (!pins || !nr_pins)
> > + return -EINVAL;
> > +
> > + pi = ph->get_priv(ph);
> > + if (!pi)
> > + return -EINVAL;
>
> Ditto.
>
> > +
> > + if (selector > pi->nr_groups)
> > + return -EINVAL;
> > +
>
> selector >= ?
>
Yes, thanks.
> > + if (!pi->groups[selector].present) {
> > + int ret;
> > +
> > + ret = scmi_pinctrl_get_group_info(ph, selector,
> > + &pi->groups[selector]);
> > + if (ret)
> > + return ret;
> > + }
> > +
> > + *pins = pi->groups[selector].group_pins;
> > + *nr_pins = pi->groups[selector].nr_pins;
> > +
> > + return 0;
> > +}
> > +
> > +static int scmi_pinctrl_get_function_info(const struct scmi_protocol_handle *ph,
> > + u32 selector,
> > + struct scmi_function_info *func)
> > +{
> > + int ret;
> > +
> > + if (!func)
> > + return -EINVAL;
> > +
> > + ret = scmi_pinctrl_attributes(ph, FUNCTION_TYPE, selector,
> > + func->name,
> > + &func->nr_groups);
> > + if (ret)
> > + return ret;
> > +
> > + if (!func->nr_groups) {
> > + dev_err(ph->dev, "Function %d has 0 elements", selector);
> > + return -ENODATA;
> > + }
> > +
> > + func->groups = devm_kmalloc_array(ph->dev, func->nr_groups,
> > + sizeof(*func->groups),
> > + GFP_KERNEL);
> > + if (!func->groups)
> > + return -ENOMEM;
>
> Same as above...lazy allocation properly tracked BUT do not use devm_
> variants.
>
Ok.
> > +
> > + ret = scmi_pinctrl_list_associations(ph, selector, FUNCTION_TYPE,
> > + func->nr_groups, func->groups);
> > + if (ret) {
> > + devm_kfree(ph->dev, func->groups);
> > + return ret;
>
> kfree
>
> > + }
> > +
> > + func->present = true;
> > + return 0;
> > +}
> > +
> > +static int scmi_pinctrl_get_function_name(const struct scmi_protocol_handle *ph,
> > + u32 selector, const char **name)
> > +{
> > + struct scmi_pinctrl_info *pi;
> > +
> > + if (!name)
> > + return -EINVAL;
> > +
> > + pi = ph->get_priv(ph);
> > + if (!pi)
> > + return -EINVAL;
>
> Ditto.
>
> > +
> > + if (selector > pi->nr_functions)
> > + return -EINVAL;
>
> selector >= ?
>
Ok. thanks.
> > +
> > + if (!pi->functions[selector].present) {
> > + int ret;
> > +
> > + ret = scmi_pinctrl_get_function_info(ph, selector,
> > + &pi->functions[selector]);
> > + if (ret)
> > + return ret;
> > + }
> > +
> > + *name = pi->functions[selector].name;
> > + return 0;
> > +}
> > +
> > +static int scmi_pinctrl_get_function_groups(const struct scmi_protocol_handle *ph,
> > + u32 selector,
> > + unsigned int *nr_groups,
> > + const unsigned int **groups)
> > +{
> > + struct scmi_pinctrl_info *pi;
> > +
> > + if (!groups || !nr_groups)
> > + return -EINVAL;
> > +
> > + pi = ph->get_priv(ph);
> > + if (!pi)
> > + return -EINVAL;
>
> Ditto.
>
> > +
> > + if (selector > pi->nr_functions)
> > + return -EINVAL;
>
> selector >= ?
>
Ok. thanks.
> > +
> > + if (!pi->functions[selector].present) {
> > + int ret;
> > +
> > + ret = scmi_pinctrl_get_function_info(ph, selector,
> > + &pi->functions[selector]);
> > + if (ret)
> > + return ret;
> > + }
> > +
> > + *groups = pi->functions[selector].groups;
> > + *nr_groups = pi->functions[selector].nr_groups;
> > +
> > + return 0;
> > +}
> > +
> > +static int scmi_pinctrl_set_mux(const struct scmi_protocol_handle *ph,
> > + u32 selector, u32 group)
> > +{
> > + return scmi_pinctrl_function_select(ph, group, GROUP_TYPE,
> > + selector);
> > +}
> > +
> > +static int scmi_pinctrl_get_pin_info(const struct scmi_protocol_handle *ph,
> > + u32 selector, struct scmi_pin_info *pin)
> > +{
> > + int ret;
> > + struct scmi_pinctrl_info *pi;
> > +
> > + if (!pin)
> > + return -EINVAL;
> > +
> > + pi = ph->get_priv(ph);
> > + if (!pi)
> > + return -EINVAL;
>
> Ditto.
>
> > +
> > + ret = scmi_pinctrl_attributes(ph, PIN_TYPE, selector,
> > + pin->name, NULL);
> > + if (ret)
> > + return ret;
> > +
> > + pin->present = true;
> > + return 0;
> > +}
> > +
> > +static int scmi_pinctrl_get_pin_name(const struct scmi_protocol_handle *ph,
> > + u32 selector, const char **name)
> > +{
> > + struct scmi_pinctrl_info *pi;
> > +
> > + if (!name)
> > + return -EINVAL;
> > +
> > + pi = ph->get_priv(ph);
> > + if (!pi)
> > + return -EINVAL;
>
> Ditto.
>
> > +
> > + if (selector > pi->nr_pins)
> > + return -EINVAL;
>
> selector >= ?
>
Ok. thanks.
> > +
> > + if (!pi->pins[selector].present) {
> > + int ret;
> > +
> > + ret = scmi_pinctrl_get_pin_info(ph, selector,
> > + &pi->pins[selector]);
> > + if (ret)
> > + return ret;
> > + }
> > +
> > + *name = pi->pins[selector].name;
> > +
> > + return 0;
> > +}
> > +
> > +static int scmi_pinctrl_get_name(const struct scmi_protocol_handle *ph,
> > + u32 selector,
> > + enum scmi_pinctrl_selector_type type,
> > + const char **name)
> > +{
> > + switch (type) {
> > + case PIN_TYPE:
> > + return scmi_pinctrl_get_pin_name(ph, selector, name);
> > + case GROUP_TYPE:
> > + return scmi_pinctrl_get_group_name(ph, selector, name);
> > + case FUNCTION_TYPE:
> > + return scmi_pinctrl_get_function_name(ph, selector, name);
> > + default:
> > + return -EINVAL;
> > + }
> > +}
> > +
> > +static const struct scmi_pinctrl_proto_ops pinctrl_proto_ops = {
> > + .get_count = scmi_pinctrl_get_count,
> > + .get_name = scmi_pinctrl_get_name,
> > + .get_group_pins = scmi_pinctrl_get_group_pins,
> > + .get_function_groups = scmi_pinctrl_get_function_groups,
> > + .set_mux = scmi_pinctrl_set_mux,
> > + .get_config = scmi_pinctrl_get_config,
> > + .set_config = scmi_pinctrl_set_config,
> > + .request_pin = scmi_pinctrl_request_pin,
> > + .free_pin = scmi_pinctrl_free_pin
> > +};
> > +
> > +static int scmi_pinctrl_protocol_init(const struct scmi_protocol_handle *ph)
> > +{
> > + int ret;
> > + u32 version;
> > + struct scmi_pinctrl_info *pinfo;
> > +
> > + ret = ph->xops->version_get(ph, &version);
> > + if (ret)
> > + return ret;
> > +
> > + dev_dbg(ph->dev, "Pinctrl Version %d.%d\n",
> > + PROTOCOL_REV_MAJOR(version), PROTOCOL_REV_MINOR(version));
> > +
> > + pinfo = devm_kzalloc(ph->dev, sizeof(*pinfo), GFP_KERNEL);
> > + if (!pinfo)
> > + return -ENOMEM;
> > +
> > + ret = scmi_pinctrl_attributes_get(ph, pinfo);
> > + if (ret)
> > + return ret;
> > +
> > + pinfo->pins = devm_kcalloc(ph->dev, pinfo->nr_pins,
> > + sizeof(*pinfo->pins),
> > + GFP_KERNEL);
> > + if (!pinfo->pins)
> > + return -ENOMEM;
> > +
> > + pinfo->groups = devm_kcalloc(ph->dev, pinfo->nr_groups,
> > + sizeof(*pinfo->groups),
> > + GFP_KERNEL);
> > + if (!pinfo->groups)
> > + return -ENOMEM;
> > +
> > + pinfo->functions = devm_kcalloc(ph->dev, pinfo->nr_functions,
> > + sizeof(*pinfo->functions),
> > + GFP_KERNEL);
> > + if (!pinfo->functions)
> > + return -ENOMEM;
> > +
> > + pinfo->version = version;
> > +
> > + return ph->set_priv(ph, pinfo);
> > +}
> > +
> > +static int scmi_pinctrl_protocol_deinit(const struct scmi_protocol_handle *ph)
> > +{
> > + int i;
> > + struct scmi_pinctrl_info *pi;
> > +
> > + pi = ph->get_priv(ph);
> > + if (!pi)
> > + return -EINVAL;
>
> Ditto. You never get even here if protocol init had not succesfully
> completed.
>
> > +
> > + for (i = 0; i < pi->nr_groups; i++)
> > + if (pi->groups[i].present) {
> > + devm_kfree(ph->dev, pi->groups[i].group_pins);
>
> kfree..you are managing these.
>
> > + pi->groups[i].present = false;
> > + }
> > +
> > + for (i = 0; i < pi->nr_functions; i++)
> > + if (pi->functions[i].present) {
> > + devm_kfree(ph->dev, pi->functions[i].groups);
>
> kfree..you are managing these.
>
> > + pi->functions[i].present = false;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static const struct scmi_protocol scmi_pinctrl = {
> > + .id = SCMI_PROTOCOL_PINCTRL,
> > + .owner = THIS_MODULE,
> > + .instance_init = &scmi_pinctrl_protocol_init,
> > + .instance_deinit = &scmi_pinctrl_protocol_deinit,
> > + .ops = &pinctrl_proto_ops,
> > +};
> > +
> > +DEFINE_SCMI_PROTOCOL_REGISTER_UNREGISTER(pinctrl, scmi_pinctrl)
> > diff --git a/drivers/firmware/arm_scmi/protocols.h b/drivers/firmware/arm_scmi/protocols.h
> > index b3c6314bb4b8..674f949354f9 100644
> > --- a/drivers/firmware/arm_scmi/protocols.h
> > +++ b/drivers/firmware/arm_scmi/protocols.h
> > @@ -346,5 +346,6 @@ DECLARE_SCMI_REGISTER_UNREGISTER(sensors);
> > DECLARE_SCMI_REGISTER_UNREGISTER(voltage);
> > DECLARE_SCMI_REGISTER_UNREGISTER(system);
> > DECLARE_SCMI_REGISTER_UNREGISTER(powercap);
> > +DECLARE_SCMI_REGISTER_UNREGISTER(pinctrl);
> >
> > #endif /* _SCMI_PROTOCOLS_H */
> > diff --git a/include/linux/scmi_protocol.h b/include/linux/scmi_protocol.h
> > index 0ce5746a4470..97631783a5a4 100644
> > --- a/include/linux/scmi_protocol.h
> > +++ b/include/linux/scmi_protocol.h
> > @@ -735,6 +735,52 @@ struct scmi_notify_ops {
> > struct notifier_block *nb);
> > };
> >
> > +enum scmi_pinctrl_selector_type {
> > + PIN_TYPE = 0,
> > + GROUP_TYPE,
> > + FUNCTION_TYPE
> > +};
> > +
> > +/**
> > + * struct scmi_pinctrl_proto_ops - represents the various operations provided
> > + * by SCMI Pinctrl Protocol
> > + *
> > + * @get_count: returns count of the registered elements in given type
> > + * @get_name: returns name by index of given type
> > + * @get_group_pins: returns the set of pins, assigned to the specified group
> > + * @get_function_groups: returns the set of groups, assigned to the specified
> > + * function
> > + * @set_mux: set muxing function for groups of pins
> > + * @get_config: returns configuration parameter for pin or group
> > + * @set_config: sets the configuration parameter for pin or group
> > + * @request_pin: aquire pin before selecting mux setting
> > + * @free_pin: frees pin, acquired by request_pin call
> > + */
> > +struct scmi_pinctrl_proto_ops {
> > + int (*get_count)(const struct scmi_protocol_handle *ph,
> > + enum scmi_pinctrl_selector_type type);
> > + int (*get_name)(const struct scmi_protocol_handle *ph,
> > + u32 selector,
> > + enum scmi_pinctrl_selector_type type,
> > + const char **name);
> > + int (*get_group_pins)(const struct scmi_protocol_handle *ph,
> > + u32 selector,
> > + const unsigned int **pins, unsigned int *nr_pins);
> > + int (*get_function_groups)(const struct scmi_protocol_handle *ph,
> > + u32 selector, unsigned int *nr_groups,
> > + const unsigned int **groups);
> > + int (*set_mux)(const struct scmi_protocol_handle *ph, u32 selector,
> > + u32 group);
> > + int (*get_config)(const struct scmi_protocol_handle *ph, u32 selector,
> > + enum scmi_pinctrl_selector_type type,
> > + u8 config_type, unsigned long *config_value);
> > + int (*set_config)(const struct scmi_protocol_handle *ph, u32 selector,
> > + enum scmi_pinctrl_selector_type type,
> > + u8 config_type, unsigned long config_value);
> > + int (*request_pin)(const struct scmi_protocol_handle *ph, u32 pin);
> > + int (*free_pin)(const struct scmi_protocol_handle *ph, u32 pin);
> > +};
> > +
>
> Can you move all of these before .notify_ops where all others protocol
> ops lives ? ... and rename all pinctrl_ops to match the ".<object>_<verb>"
> pattern like other ops (i.e. count_get/name_get ... instead get_count/get_name)
>
I will do this.
> > /**
> > * struct scmi_handle - Handle returned to ARM SCMI clients for usage.
> > *
> > @@ -783,6 +829,7 @@ enum scmi_std_protocol {
> > SCMI_PROTOCOL_RESET = 0x16,
> > SCMI_PROTOCOL_VOLTAGE = 0x17,
> > SCMI_PROTOCOL_POWERCAP = 0x18,
> > + SCMI_PROTOCOL_PINCTRL = 0x19,
> > };
> >
>
> Thanks,
> Cristian
Thanks,
Oleksii.
More information about the linux-arm-kernel
mailing list