[v3,2/4] firmware: arm_scmi: Add SCMI v3.2 pincontrol protocol basic support

scmi: Introduce pinctrl SCMI protocol driver

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

Signed-off-by: Oleksii Moisieiev <oleksii_moisieiev@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

Tue, Jun 06, 2023 at 04:22:27PM +0000, Oleksii Moisieiev kirjoitti:
> scmi: Introduce pinctrl SCMI protocol driver

Seems like you forgot to remove previous line(s) in the commit message and
the above has to be the Subject here.

> 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

drop include/ part, everybody will understand that. Also mind the grammar
period.

...

> -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

Why not splitting it and make it ordered?

...

Missing headers:

	bitfield.h
	bits.h
	byteorder/
	types.h

> +#include <linux/module.h>
> +#include <linux/scmi_protocol.h>
> +#include <linux/slab.h>

Missing

	asm/unaligned.h

...

> +struct scmi_group_info { 
> +       bool present; 
> +       char name[SCMI_MAX_STR_SIZE]; 
> +       unsigned int *group_pins; 
> +       unsigned int nr_pins; 
> +}; 
 
So, why struct pingroup can't be embeded here?

> +struct scmi_function_info {
> +	bool present;
> +	char name[SCMI_MAX_STR_SIZE];
> +	unsigned int *groups;
> +	unsigned int nr_groups;
> +};

So, why and struct pinfunction can't be embedded here (yes, you would need a
duplication of groups as here they are integers)?

As far as I understand these data structures are not part of any ABI (otherwise
the wrong type(s) / padding might be in use) and hence don't see the impediments
to use them, but would be nice to have a comment on top of each.

...

> +struct scmi_pin_info {
> +	bool present;
> +	char name[SCMI_MAX_STR_SIZE];

Swapping order might help compiler to generate a better code.
Also this applies to the _group_info and _function_info.

> +};

...

> +	ret = ph->xops->do_xfer(ph, t);
> +	if (!ret) {

Can you rather follow the usual pattern, i.e. checking for the errors?

	if (ret)
		goto out_put_xfer;

> +		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);
> +	}

out_put_xfer:

> +	ph->xops->xfer_put(ph, t);
> +	return ret;

...

> +	ret = ph->xops->do_xfer(ph, t);
> +	if (!ret) {

Ditto.

> +		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))

	if (ret)
		return ret;

> +		ph->hops->extended_name_get(ph, PINCTRL_NAME_GET, selector,
> +					    (u32 *)&type, name,
> +					    SCMI_MAX_STR_SIZE);
> +	return ret;

	return 0;

> +}

...

> +	ret = ph->xops->do_xfer(ph, t);
> +	if (!ret)

	if (ret)
		goto out_put_xfer;

(but in this and similar, aka one line, cases the current wouldn't be bad, just
matter of the consistency with the rest of the code)

> +		*config_value = get_unaligned_le32(t->rx.buf);
> +
> +	ph->xops->xfer_put(ph, t);
> +	return ret;

...

> +	ret = ph->xops->xfer_get_init(ph, PINCTRL_RELEASE,
> +				      sizeof(*tx), 0, &t);

This is perfectly one line. Please also check entire code for such an unneeded
wrap.

...

> +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;
> +
> +	ret = scmi_pinctrl_list_associations(ph, selector, GROUP_TYPE,
> +					     group->nr_pins, group->group_pins);
> +	if (ret) {
> +		devm_kfree(ph->dev, group->group_pins);

This is a red flag. Is this function is solely used at the ->probe() stage?
I do not think so. Why then the devm_*() is being used to begin with?

Also what are the object lifetimes for device here and the _group_info
instances?

> +		return ret;
> +	}
> +
> +	group->present = true;
> +	return 0;
> +}

...

> +static int scmi_pinctrl_get_function_info(const struct scmi_protocol_handle *ph,
> +					  u32 selector,
> +					  struct scmi_function_info *func)
> +{

As per above.

> +}

...

> +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

It's not a terminator item, so leave trailing comma. It will reduce the burden
in case of update of this.

> +};

...

> +static int scmi_pinctrl_protocol_init(const struct scmi_protocol_handle *ph)
> +{

> +	pinfo = devm_kzalloc(ph->dev, sizeof(*pinfo), GFP_KERNEL);
> +	if (!pinfo)
> +		return -ENOMEM;

All the same, why devm_*() is in use and what are the object lifetimes?

> +}

...

> +	for (i = 0; i < pi->nr_groups; i++)
> +		if (pi->groups[i].present) {
> +			devm_kfree(ph->dev, pi->groups[i].group_pins);
> +			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);
> +			pi->functions[i].present = false;
> +		}

Missing outer {}, but see above as well.

...

> +static const struct scmi_protocol scmi_pinctrl = {
> +	.id = SCMI_PROTOCOL_PINCTRL,

> +	.owner = THIS_MODULE,

This is not needed if you use a trick from ~15 years back...

> +	.instance_init = &scmi_pinctrl_protocol_init,
> +	.instance_deinit = &scmi_pinctrl_protocol_deinit,
> +	.ops = &pinctrl_proto_ops,
> +};
> +

Redundant blank line.

> +DEFINE_SCMI_PROTOCOL_REGISTER_UNREGISTER(pinctrl, scmi_pinctrl)

...i.e. initializing the owner by this macro.

It might require some update to the above macro and its users.

...


> +enum scmi_pinctrl_selector_type {
> +	PIN_TYPE = 0,
> +	GROUP_TYPE,
> +	FUNCTION_TYPE

Leave trailing comma.

> +};

On Wed, Jun 07, 2023 at 10:10:44AM +0300, andy.shevchenko@gmail.com wrote:
> Tue, Jun 06, 2023 at 04:22:27PM +0000, Oleksii Moisieiev kirjoitti:

Hi Andy,

I'll answer some of your remarks down below, since more generally
related to the SCMI stack.

> > scmi: Introduce pinctrl SCMI protocol driver
> 
> Seems like you forgot to remove previous line(s) in the commit message and
> the above has to be the Subject here.
> 
> > 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
> 
> drop include/ part, everybody will understand that. Also mind the grammar
> period.
> 
> ...
> 
> > -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
> 
> Why not splitting it and make it ordered?
>

Maybe a good idea for a separate cleanup...not sure can fit this series
without causing churn with other in-flight SCMI series...I'll happily wait
for Sudeep to decide.

> ...
> 
> Missing headers:
> 
> 	bitfield.h
> 	bits.h
> 	byteorder/
> 	types.h
> 
> > +#include <linux/module.h>
> > +#include <linux/scmi_protocol.h>
> > +#include <linux/slab.h>
> 
> Missing
> 
> 	asm/unaligned.h
> 
> ...
>

Most if not all of these headers are already included by the

	#include "protocols.h"

above that introduces a lot of common other stuff needed to implement
a new SCMI protocol.

> > +struct scmi_group_info { 
> > +       bool present; 
> > +       char name[SCMI_MAX_STR_SIZE]; 
> > +       unsigned int *group_pins; 
> > +       unsigned int nr_pins; 
> > +}; 
>  
> So, why struct pingroup can't be embeded here?
> 
> > +struct scmi_function_info {
> > +	bool present;
> > +	char name[SCMI_MAX_STR_SIZE];
> > +	unsigned int *groups;
> > +	unsigned int nr_groups;
> > +};
> 
> So, why and struct pinfunction can't be embedded here (yes, you would need a
> duplication of groups as here they are integers)?
> 
> As far as I understand these data structures are not part of any ABI (otherwise
> the wrong type(s) / padding might be in use) and hence don't see the impediments
> to use them, but would be nice to have a comment on top of each.
> 
> ...
> 
> > +struct scmi_pin_info {
> > +	bool present;
> > +	char name[SCMI_MAX_STR_SIZE];
> 
> Swapping order might help compiler to generate a better code.
> Also this applies to the _group_info and _function_info.
> 
> > +};
> 
> ...
> 
> > +	ret = ph->xops->do_xfer(ph, t);
> > +	if (!ret) {
> 
> Can you rather follow the usual pattern, i.e. checking for the errors?
> 

I think Oleksii here followed the (opinable maybe) pattern we have in
the SCMI stack where typically you get/build/send/put an scmi message
(xfer) while doing a few things only if the message was sent
successfully (if !ret ... most of the time): checking for success avoid
a lot of 'goto err:' all around.
 
[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;
> > +
> > +	ret = scmi_pinctrl_list_associations(ph, selector, GROUP_TYPE,
> > +					     group->nr_pins, group->group_pins);
> > +	if (ret) {
> > +		devm_kfree(ph->dev, group->group_pins);
> 
> This is a red flag. Is this function is solely used at the ->probe() stage?
> I do not think so. Why then the devm_*() is being used to begin with?
> 
> Also what are the object lifetimes for device here and the _group_info
> instances?
> 
> > +		return ret;
> > +	}
> > +
> > +	group->present = true;
> > +	return 0;
> > +}
> 
> ...
> 
> > +static int scmi_pinctrl_get_function_info(const struct scmi_protocol_handle *ph,
> > +					  u32 selector,
> > +					  struct scmi_function_info *func)
> > +{
> 
> As per above.
> 
> > +}
> 
> ...
> 
> > +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
> 
> It's not a terminator item, so leave trailing comma. It will reduce the burden
> in case of update of this.
> 
> > +};
> 
> ...
> 
> > +static int scmi_pinctrl_protocol_init(const struct scmi_protocol_handle *ph)
> > +{
> 
> > +	pinfo = devm_kzalloc(ph->dev, sizeof(*pinfo), GFP_KERNEL);
> > +	if (!pinfo)
> > +		return -ENOMEM;
> 
> All the same, why devm_*() is in use and what are the object lifetimes?
> 
> > +}
> 

This bit about alocation and devres deserves an explanation in the context
of the SCMI stack.

So, you can add support for a new SCMI protocol using the below macro

 DEFINE_SCMI_PROTOCOL_REGISTER_UNREGISTER

to register with the core SCMI stack a few things like an
initialization function and the protocol operations you wish this
protocol to expose.

At run-time, once the first user of your new protocol comes up (like
the pinctrl driver later in the series), the core SCMI will take care
to setup and initialize the protocol so that can be used by the SCMI
drivers (like pinctrl-scmi.c) via its exposed proto_operations.
(assuming the protocol has been also found as supported by the fw
serving as the SCMI server)

When the last user of a protocol is gone, similarly, the protocol
will be deinitialized (if anything is needed to be deinit really...)

Basically the core calls upfront the protocol_init function you provided
and passes to it a ph protocol_handle that embeds a number of things
useful for protocol implementations, like as example the xops-> needed
to build and send messages using the core facilities.

Another thing that is embedded in the ph, as you noticed, is the ph->dev
device reference to be optionally used for devres in your protocol: now,
we do NOT have per-protocol devices, so, that device lifetine is NOT bound
strictly to this protocol but to the whole stack... BUT the SCMI core
takes care to open/close a devres group around your protocol_init invocation,
so that you can use devres on your .protocol_init, and be assured that when
your protocol will be de-initialized (since no more used by anyone) all your
devres allocations will be freed.

For this see:

 drivers/firmware/arm_scmi/driver.c::scmi_alloc_init_protocol_instance()

This clearly works ONLY for allocations descending directly from the
.protocol_init() call (when the devres group is open) and it was working
fine till today for all protocols, since all existing protocols
allocated all what they needed during protocol_init....

... Pinctrl is a differenet beast, though, since it could make sense indeed
(even though still under a bit of discussion..) to delay some allocations and
SCMI querying to the platform after the protocol_init stage...i.e. lazy allocate
some resources only later when the pinctrl subsystem will parse the DT and will
ask the pinctrl-scmi driver just for the strictly needed resources.
(so you avoid to query and allocate at boot time a bunch of pin stuff that you
will never use...)

These lazy allocations instead, like the ones in scmi_pinctrl_get_group_info(),
happen outside of the .protocol_init path so they HAVE TO to be explicitly
managed manually without devres; as a consequence the addition of a
dedicated .protocol_deinit() function and the frees on the err path: so
that anything non devres allocated in the protcol devres_group can be
freed properly when the core deinitializes the protocol.

What is WRONG, though, in this patch (and I missed it ... my bad) is that such
explicit manual alloc/dealloc need not and should not be of devres kind but just
plain simple kmalloc_ / kfree.
(even though it is not harmful in this context...the ph->dev never unbounds till
the end of the stack..it is certainkly not needed and confusing)

Hoping not to have bored you to death with all of this SCMI digression... :D

Thanks,
Cristian

On Tue, Jun 06, 2023 at 04:22:27PM +0000, Oleksii Moisieiev wrote:
> scmi: Introduce pinctrl SCMI protocol driver
>

Hi Oleksii,

spurios line above.

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

> Signed-off-by: Oleksii Moisieiev <oleksii_moisieiev@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...

> +M:	Oleksii Moisieiev <oleksii_moisieiev@epam.com>
> +L:	linux-arm-kernel@lists.infradead.org
> +S:	Maintained
> +F:	drivers/firmware/arm_scmi/pinctrl.c
> +
>  SYSTEM RESET/SHUTDOWN DRIVERS
>  M:	Sebastian Reichel <sre@kernel.org>
>  L:	linux-pm@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

> +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.

> +
> +	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.

> +
> +	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)

> +
> +	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

> +		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 >=  ?

> +
> +	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 >=  ?

> +	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.

> +
> +	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 >=  ?

> +
> +	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 >=  ?

> +
> +	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 >=  ?

> +
> +	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)

>  /**
>   * 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

Fri, Jun 30, 2023 at 05:02:12PM +0100, Cristian Marussi kirjoitti:
> On Wed, Jun 07, 2023 at 10:10:44AM +0300, andy.shevchenko@gmail.com wrote:
> > Tue, Jun 06, 2023 at 04:22:27PM +0000, Oleksii Moisieiev kirjoitti:

...

> > > -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
> > 
> > Why not splitting it and make it ordered?
> 
> Maybe a good idea for a separate cleanup...not sure can fit this series
> without causing churn with other in-flight SCMI series...I'll happily wait
> for Sudeep to decide.

Sure.

...

> > Missing headers:
> > 
> > 	bitfield.h
> > 	bits.h
> > 	byteorder/
> > 	types.h
> > 
> > > +#include <linux/module.h>
> > > +#include <linux/scmi_protocol.h>
> > > +#include <linux/slab.h>
> > 
> > Missing
> > 
> > 	asm/unaligned.h
> 
> Most if not all of these headers are already included by the
> 
> 	#include "protocols.h"
> 
> above that introduces a lot of common other stuff needed to implement
> a new SCMI protocol.

OK!

...

> > > +	ret = ph->xops->do_xfer(ph, t);
> > > +	if (!ret) {
> > 
> > Can you rather follow the usual pattern, i.e. checking for the errors?
> 
> I think Oleksii here followed the (opinable maybe) pattern we have in
> the SCMI stack where typically you get/build/send/put an scmi message
> (xfer) while doing a few things only if the message was sent
> successfully (if !ret ... most of the time): checking for success avoid
> a lot of 'goto err:' all around.

If it's
	ret = fpp();
	if (!ret)
		ret = bpp();
	return ret;

I would agree with you, but in some cases it involves more core and that code
doesn't affect ret itself.

> > > +	}

...

> > All the same, why devm_*() is in use and what are the object lifetimes?
> 
> This bit about alocation and devres deserves an explanation in the context
> of the SCMI stack.
> 
> So, you can add support for a new SCMI protocol using the below macro
> 
>  DEFINE_SCMI_PROTOCOL_REGISTER_UNREGISTER
> 
> to register with the core SCMI stack a few things like an
> initialization function and the protocol operations you wish this
> protocol to expose.
> 
> At run-time, once the first user of your new protocol comes up (like
> the pinctrl driver later in the series), the core SCMI will take care
> to setup and initialize the protocol so that can be used by the SCMI
> drivers (like pinctrl-scmi.c) via its exposed proto_operations.
> (assuming the protocol has been also found as supported by the fw
> serving as the SCMI server)
> 
> When the last user of a protocol is gone, similarly, the protocol
> will be deinitialized (if anything is needed to be deinit really...)
> 
> Basically the core calls upfront the protocol_init function you provided
> and passes to it a ph protocol_handle that embeds a number of things
> useful for protocol implementations, like as example the xops-> needed
> to build and send messages using the core facilities.
> 
> Another thing that is embedded in the ph, as you noticed, is the ph->dev
> device reference to be optionally used for devres in your protocol: now,
> we do NOT have per-protocol devices, so, that device lifetine is NOT bound
> strictly to this protocol but to the whole stack... BUT the SCMI core
> takes care to open/close a devres group around your protocol_init invocation,
> so that you can use devres on your .protocol_init, and be assured that when
> your protocol will be de-initialized (since no more used by anyone) all your
> devres allocations will be freed.
> 
> For this see:
> 
>  drivers/firmware/arm_scmi/driver.c::scmi_alloc_init_protocol_instance()
> 
> This clearly works ONLY for allocations descending directly from the
> .protocol_init() call (when the devres group is open) and it was working
> fine till today for all protocols, since all existing protocols
> allocated all what they needed during protocol_init....
> 
> ... Pinctrl is a differenet beast, though, since it could make sense indeed
> (even though still under a bit of discussion..) to delay some allocations and
> SCMI querying to the platform after the protocol_init stage...i.e. lazy allocate
> some resources only later when the pinctrl subsystem will parse the DT and will
> ask the pinctrl-scmi driver just for the strictly needed resources.
> (so you avoid to query and allocate at boot time a bunch of pin stuff that you
> will never use...)
> 
> These lazy allocations instead, like the ones in scmi_pinctrl_get_group_info(),
> happen outside of the .protocol_init path so they HAVE TO to be explicitly
> managed manually without devres; as a consequence the addition of a
> dedicated .protocol_deinit() function and the frees on the err path: so
> that anything non devres allocated in the protcol devres_group can be
> freed properly when the core deinitializes the protocol.
> 
> What is WRONG, though, in this patch (and I missed it ... my bad) is that such
> explicit manual alloc/dealloc need not and should not be of devres kind but just
> plain simple kmalloc_ / kfree.
> (even though it is not harmful in this context...the ph->dev never unbounds till
> the end of the stack..it is certainkly not needed and confusing)
> 
> Hoping not to have bored you to death with all of this SCMI digression... :D

Thank you for a dive into the implementation of the SCMI. Perhaps you can
summarize that into some kernel doc aroung thouse callbacks, so people can
clearly see when it's possible and when it's not to use devm_*() APIs.

On Tue, Jul 04, 2023 at 12:27:40AM +0300, andy.shevchenko@gmail.com wrote:
> Fri, Jun 30, 2023 at 05:02:12PM +0100, Cristian Marussi kirjoitti:
> > On Wed, Jun 07, 2023 at 10:10:44AM +0300, andy.shevchenko@gmail.com wrote:
> > > Tue, Jun 06, 2023 at 04:22:27PM +0000, Oleksii Moisieiev kirjoitti:
> 
> ...
> 

Hi Andy,

> > > > -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
> > > 
> > > Why not splitting it and make it ordered?
> > 
> > Maybe a good idea for a separate cleanup...not sure can fit this series
> > without causing churn with other in-flight SCMI series...I'll happily wait
> > for Sudeep to decide.
 
[snip]
 
> > > > +	}
> 
> ...
> 
> > > All the same, why devm_*() is in use and what are the object lifetimes?
> > 
> > This bit about alocation and devres deserves an explanation in the context
> > of the SCMI stack.
> > 
> > So, you can add support for a new SCMI protocol using the below macro
> > 
> >  DEFINE_SCMI_PROTOCOL_REGISTER_UNREGISTER
> > 
> > to register with the core SCMI stack a few things like an
> > initialization function and the protocol operations you wish this
> > protocol to expose.
> > 
> > At run-time, once the first user of your new protocol comes up (like
> > the pinctrl driver later in the series), the core SCMI will take care
> > to setup and initialize the protocol so that can be used by the SCMI
> > drivers (like pinctrl-scmi.c) via its exposed proto_operations.
> > (assuming the protocol has been also found as supported by the fw
> > serving as the SCMI server)
> > 
> > When the last user of a protocol is gone, similarly, the protocol
> > will be deinitialized (if anything is needed to be deinit really...)
> > 
> > Basically the core calls upfront the protocol_init function you provided
> > and passes to it a ph protocol_handle that embeds a number of things
> > useful for protocol implementations, like as example the xops-> needed
> > to build and send messages using the core facilities.
> > 
> > Another thing that is embedded in the ph, as you noticed, is the ph->dev
> > device reference to be optionally used for devres in your protocol: now,
> > we do NOT have per-protocol devices, so, that device lifetine is NOT bound
> > strictly to this protocol but to the whole stack... BUT the SCMI core
> > takes care to open/close a devres group around your protocol_init invocation,
> > so that you can use devres on your .protocol_init, and be assured that when
> > your protocol will be de-initialized (since no more used by anyone) all your
> > devres allocations will be freed.
> > 
> > For this see:
> > 
> >  drivers/firmware/arm_scmi/driver.c::scmi_alloc_init_protocol_instance()
> > 
> > This clearly works ONLY for allocations descending directly from the
> > .protocol_init() call (when the devres group is open) and it was working
> > fine till today for all protocols, since all existing protocols
> > allocated all what they needed during protocol_init....
> > 
> > ... Pinctrl is a differenet beast, though, since it could make sense indeed
> > (even though still under a bit of discussion..) to delay some allocations and
> > SCMI querying to the platform after the protocol_init stage...i.e. lazy allocate
> > some resources only later when the pinctrl subsystem will parse the DT and will
> > ask the pinctrl-scmi driver just for the strictly needed resources.
> > (so you avoid to query and allocate at boot time a bunch of pin stuff that you
> > will never use...)
> > 
> > These lazy allocations instead, like the ones in scmi_pinctrl_get_group_info(),
> > happen outside of the .protocol_init path so they HAVE TO to be explicitly
> > managed manually without devres; as a consequence the addition of a
> > dedicated .protocol_deinit() function and the frees on the err path: so
> > that anything non devres allocated in the protcol devres_group can be
> > freed properly when the core deinitializes the protocol.
> > 
> > What is WRONG, though, in this patch (and I missed it ... my bad) is that such
> > explicit manual alloc/dealloc need not and should not be of devres kind but just
> > plain simple kmalloc_ / kfree.
> > (even though it is not harmful in this context...the ph->dev never unbounds till
> > the end of the stack..it is certainkly not needed and confusing)
> > 
> > Hoping not to have bored you to death with all of this SCMI digression... :D
> 
> Thank you for a dive into the implementation of the SCMI. Perhaps you can
> summarize that into some kernel doc aroung thouse callbacks, so people can
> clearly see when it's possible and when it's not to use devm_*() APIs.
>

Absolutely, this is definitely missing, it's just that till now I was
the only dealing with this, so docs was overlooked ... and I am really
the first to be in need of this documentation :P

Thanks,
Cristian

P.S.: and apologies for late replies but our mail server seems to
constantly classify you as spam :D

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@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@epam.com>
> > +L:	linux-arm-kernel@lists.infradead.org
> > +S:	Maintained
> > +F:	drivers/firmware/arm_scmi/pinctrl.c
> > +
> >  SYSTEM RESET/SHUTDOWN DRIVERS
> >  M:	Sebastian Reichel <sre@kernel.org>
> >  L:	linux-pm@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.

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://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.
>

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)

> > > Signed-off-by: Oleksii Moisieiev <oleksii_moisieiev@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@epam.com>
> > > +L:	linux-arm-kernel@lists.infradead.org
> > > +S:	Maintained
> > > +F:	drivers/firmware/arm_scmi/pinctrl.c
> > > +
> > >  SYSTEM RESET/SHUTDOWN DRIVERS
> > >  M:	Sebastian Reichel <sre@kernel.org>
> > >  L:	linux-pm@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.
> 

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

Hi Andy,

On Wed, Jun 07, 2023 at 10:10:44AM +0300, andy.shevchenko@gmail.com wrote:
> Tue, Jun 06, 2023 at 04:22:27PM +0000, Oleksii Moisieiev kirjoitti:
> > scmi: Introduce pinctrl SCMI protocol driver
> 
> Seems like you forgot to remove previous line(s) in the commit message and
> the above has to be the Subject here.
> 
> > 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
> 
> drop include/ part, everybody will understand that. Also mind the grammar
> period.
>
Fixed. Thanks,

> ...
> 
> > -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
> 
> Why not splitting it and make it ordered?
> 
> ...
> 
> Missing headers:
> 
> 	bitfield.h
> 	bits.h
> 	byteorder/
> 	types.h
> 
> > +#include <linux/module.h>
> > +#include <linux/scmi_protocol.h>
> > +#include <linux/slab.h>
> 
> Missing
> 
> 	asm/unaligned.h
> 
> ...
> 
> > +struct scmi_group_info { 
> > +       bool present; 
> > +       char name[SCMI_MAX_STR_SIZE]; 
> > +       unsigned int *group_pins; 
> > +       unsigned int nr_pins; 
> > +}; 
>  
> So, why struct pingroup can't be embeded here?
> 
> > +struct scmi_function_info {
> > +	bool present;
> > +	char name[SCMI_MAX_STR_SIZE];
> > +	unsigned int *groups;
> > +	unsigned int nr_groups;
> > +};
> 
> So, why and struct pinfunction can't be embedded here (yes, you would need a
> duplication of groups as here they are integers)?
> 
> As far as I understand these data structures are not part of any ABI (otherwise
> the wrong type(s) / padding might be in use) and hence don't see the impediments
> to use them, but would be nice to have a comment on top of each.
> 

My opinion is that we don't want to make SCMI Pinctrl protocol to be
dependent from Pinctrl subsystem. This will potentially require SCMI
protocol change in case of significant Pinctrl subsystem refactoring.
Another reason is that pincfunction 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 structs are designed to be statically allocated and
can't be used for lazy allocations.

> ...
> 
> > +struct scmi_pin_info {
> > +	bool present;
> > +	char name[SCMI_MAX_STR_SIZE];
> 
> Swapping order might help compiler to generate a better code.
> Also this applies to the _group_info and _function_info.
> 
> > +};
> 
> ...
> 
> > +	ret = ph->xops->do_xfer(ph, t);
> > +	if (!ret) {
> 
> Can you rather follow the usual pattern, i.e. checking for the errors?
> 
> 	if (ret)
> 		goto out_put_xfer;
> 
> > +		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);
> > +	}
> 
> out_put_xfer:
> 
> > +	ph->xops->xfer_put(ph, t);
> > +	return ret;
> 
> ...
> 
> > +	ret = ph->xops->do_xfer(ph, t);
> > +	if (!ret) {
> 
> Ditto.
> 
> > +		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))
> 
> 	if (ret)
> 		return ret;
> 
> > +		ph->hops->extended_name_get(ph, PINCTRL_NAME_GET, selector,
> > +					    (u32 *)&type, name,
> > +					    SCMI_MAX_STR_SIZE);
> > +	return ret;
> 
> 	return 0;
> 
> > +}
> 
> ...
> 
> > +	ret = ph->xops->do_xfer(ph, t);
> > +	if (!ret)
> 
> 	if (ret)
> 		goto out_put_xfer;
> 
> (but in this and similar, aka one line, cases the current wouldn't be bad, just
> matter of the consistency with the rest of the code)
> 
> > +		*config_value = get_unaligned_le32(t->rx.buf);
> > +
> > +	ph->xops->xfer_put(ph, t);
> > +	return ret;
> 
> ...
> 
> > +	ret = ph->xops->xfer_get_init(ph, PINCTRL_RELEASE,
> > +				      sizeof(*tx), 0, &t);
> 
> This is perfectly one line. Please also check entire code for such an unneeded
> wrap.
> 
Thanks, I will go through the code and fix this.
> ...
> 
> > +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;
> > +
> > +	ret = scmi_pinctrl_list_associations(ph, selector, GROUP_TYPE,
> > +					     group->nr_pins, group->group_pins);
> > +	if (ret) {
> > +		devm_kfree(ph->dev, group->group_pins);
> 
> This is a red flag. Is this function is solely used at the ->probe() stage?
> I do not think so. Why then the devm_*() is being used to begin with?
> 
> Also what are the object lifetimes for device here and the _group_info
> instances?
> 
> > +		return ret;
> > +	}
> > +
> > +	group->present = true;
> > +	return 0;
> > +}
> 
> ...
> 
> > +static int scmi_pinctrl_get_function_info(const struct scmi_protocol_handle *ph,
> > +					  u32 selector,
> > +					  struct scmi_function_info *func)
> > +{
> 
> As per above.
> 
> > +}
> 
> ...
> 
> > +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
> 
> It's not a terminator item, so leave trailing comma. It will reduce the burden
> in case of update of this.
> 
Ok, thanks.
> > +};
> 
> ...
> 
> > +static int scmi_pinctrl_protocol_init(const struct scmi_protocol_handle *ph)
> > +{
> 
> > +	pinfo = devm_kzalloc(ph->dev, sizeof(*pinfo), GFP_KERNEL);
> > +	if (!pinfo)
> > +		return -ENOMEM;
> 
> All the same, why devm_*() is in use and what are the object lifetimes?
> 
Ok, thanks. 
> > +}
> 
> ...
> 
> > +	for (i = 0; i < pi->nr_groups; i++)
> > +		if (pi->groups[i].present) {
> > +			devm_kfree(ph->dev, pi->groups[i].group_pins);
> > +			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);
> > +			pi->functions[i].present = false;
> > +		}
> 
> Missing outer {}, but see above as well.
> 

Checked linux kernel code with the following command:
pcregrep -lcrM "for \(.*\)[^{]$\n.*if" .

As I can see - this approach is taking place in kernel code. Although
not as widely as for with outer {}. I will add outer {}.


> ...
> 
> > +static const struct scmi_protocol scmi_pinctrl = {
> > +	.id = SCMI_PROTOCOL_PINCTRL,
> 
> > +	.owner = THIS_MODULE,
> 
> This is not needed if you use a trick from ~15 years back...
> 
> > +	.instance_init = &scmi_pinctrl_protocol_init,
> > +	.instance_deinit = &scmi_pinctrl_protocol_deinit,
> > +	.ops = &pinctrl_proto_ops,
> > +};
> > +
> 
> Redundant blank line.
> 
> > +DEFINE_SCMI_PROTOCOL_REGISTER_UNREGISTER(pinctrl, scmi_pinctrl)
> 
> ...i.e. initializing the owner by this macro.
> 
> It might require some update to the above macro and its users.
> 
> ...
> 
> 
> > +enum scmi_pinctrl_selector_type {
> > +	PIN_TYPE = 0,
> > +	GROUP_TYPE,
> > +	FUNCTION_TYPE
> 
> Leave trailing comma.
> 
> > +};
> 
> -- 
> With Best Regards,
> Andy Shevchenko
> 
> 

Thanks,
Oleksii.

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

[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.