[v3,4/6] pinctrl: scmi: export pinctrl_scmi_get_pins

Commit Message

Peng Fan (OSS) April 28, 2024, 5:07 a.m. UTC

From: Peng Fan <[e-mail]>

Add pinctrl-scmi.h to include the function prototype and 'struct
scmi_pinctrl' to export pinctrl_scmi_get_pins, so other drivers
could use it.

Signed-off-by: Peng Fan <[e-mail]>
---
 drivers/pinctrl/pinctrl-scmi.c | 17 +++--------------
 drivers/pinctrl/pinctrl-scmi.h | 29 +++++++++++++++++++++++++++++
 2 files changed, 32 insertions(+), 14 deletions(-)

Comments

Cristian Marussi May 1, 2024, 12:36 p.m. UTC | #1

On Sun, Apr 28, 2024 at 01:07:50PM +0800, Peng Fan (OSS) wrote:
> From: Peng Fan <[e-mail]>
> 
> Add pinctrl-scmi.h to include the function prototype and 'struct
> scmi_pinctrl' to export pinctrl_scmi_get_pins, so other drivers
> could use it.
> 

Hi Peng,

so you wrote a new alternative SCMI driver using Pinctrl protocol@0x19
so that you can just parse you custom DT bindings and then use the SCMI
pinctrl_ops to set the OEM extensions to configure your platform...
...since your firmware cannot cope with the all SCMI stack footprint....

... you seemed to have solved the issue of having 2 Pinctrl drivers
coexisting under the Linux Pinctrl subsystem while attached to the same
protocol@19 node with patch 5/6 blocklist (if I get that right..)

I think this approach of a standalone SCMI alternative Pinctrl driver
that handles distinctly NXP OEM extensions and DT-parsing is certainly
more preferable than the original series you posted months ago where
custom NXP stuff were simply stuck on top of the Generic SCMI Pinctrl driver...

...what I still dont understand is why you exported data and structure
from pincttl-scmi.c to use it here; when NXP pinctrl is active the
standard Linux generic Pinctrl driver wont be alive, so not probed, so
no data can be shared, the only thing I can imagine is that you are
just trying to avoid duplicating a dozen lines from the logic of
scmi_pinctrl_get_pins() into your new NXP driver.

In this way, though, you are creating a dependency between 2 drivers,
that are not even allowed to cohexist at runtime really (due to the
blocklist trick).

Am I missing something ?

If not, I think it will be much better to just rewrite that few lines of
scmi_pincrtrl_pins_get trivial logic into your NXP driver and keep the 2
drivers fully distinct at all times.

Thanks,
Cristian

Peng Fan May 1, 2024, 12:42 p.m. UTC | #2

> Subject: Re: [PATCH v3 4/6] pinctrl: scmi: export pinctrl_scmi_get_pins
> 
> On Sun, Apr 28, 2024 at 01:07:50PM +0800, Peng Fan (OSS) wrote:
> > From: Peng Fan <[e-mail]>
> >
> > Add pinctrl-scmi.h to include the function prototype and 'struct
> > scmi_pinctrl' to export pinctrl_scmi_get_pins, so other drivers could
> > use it.
> >
> 
> Hi Peng,
> 
> so you wrote a new alternative SCMI driver using Pinctrl protocol@0x19 so
> that you can just parse you custom DT bindings and then use the SCMI
> pinctrl_ops to set the OEM extensions to configure your platform...
> ...since your firmware cannot cope with the all SCMI stack footprint....
> 
> ... you seemed to have solved the issue of having 2 Pinctrl drivers coexisting
> under the Linux Pinctrl subsystem while attached to the same
> protocol@19 node with patch 5/6 blocklist (if I get that right..)

Yes, right. With blocklist and allowlist, two drivers could coexist.

> 
> I think this approach of a standalone SCMI alternative Pinctrl driver that
> handles distinctly NXP OEM extensions and DT-parsing is certainly more
> preferable than the original series you posted months ago where custom NXP
> stuff were simply stuck on top of the Generic SCMI Pinctrl driver...
> 
> ...what I still dont understand is why you exported data and structure from
> pincttl-scmi.c to use it here; when NXP pinctrl is active the standard Linux
> generic Pinctrl driver wont be alive, so not probed, so no data can be shared,
> the only thing I can imagine is that you are just trying to avoid duplicating a
> dozen lines from the logic of
> scmi_pinctrl_get_pins() into your new NXP driver.

Yes, you are right, I just wanna avoid duplicating scmi_pinctrl_get_pins.

> 
> In this way, though, you are creating a dependency between 2 drivers, that
> are not even allowed to cohexist at runtime really (due to the blocklist trick).
> 
> Am I missing something ?

No, your understanding is correct.

> 
> If not, I think it will be much better to just rewrite that few lines of
> scmi_pincrtrl_pins_get trivial logic into your NXP driver and keep the 2
> drivers fully distinct at all times.

ok. I could write the pinctrl-scmi-imx.c local get pins logic, not using
pinctrl-scmi.c to decouple the two drivers.

Thanks,
Peng
> 
> Thanks,
> Cristian

Patch

diff --git a/drivers/pinctrl/pinctrl-scmi.c b/drivers/pinctrl/pinctrl-scmi.c
index 682ff595c3c7..360b813072df 100644
--- a/drivers/pinctrl/pinctrl-scmi.c
+++ b/drivers/pinctrl/pinctrl-scmi.c
@@ -21,6 +21,7 @@ 
 #include <linux/pinctrl/pinctrl.h>
 #include <linux/pinctrl/pinmux.h>
 
+#include "pinctrl-scmi.h"
 #include "pinctrl-utils.h"
 #include "core.h"
 #include "pinconf.h"
@@ -30,18 +31,6 @@ 
 /* Define num configs, if not large than 4 use stack, else use kcalloc() */
 #define SCMI_NUM_CONFIGS	4
 
-struct scmi_pinctrl {
-	struct device *dev;
-	struct scmi_protocol_handle *ph;
-	struct pinctrl_dev *pctldev;
-	struct pinctrl_desc pctl_desc;
-	struct pinfunction *functions;
-	unsigned int nr_functions;
-	struct pinctrl_pin_desc *pins;
-	unsigned int nr_pins;
-	const struct scmi_pinctrl_proto_ops *ops;
-};
-
 static int pinctrl_scmi_get_groups_count(struct pinctrl_dev *pctldev)
 {
 	struct scmi_pinctrl *pmx = pinctrl_dev_get_drvdata(pctldev);
@@ -468,8 +457,7 @@  static const struct pinconf_ops pinctrl_scmi_pinconf_ops = {
 	.pin_config_config_dbg_show = pinconf_generic_dump_config,
 };
 
-static int pinctrl_scmi_get_pins(struct scmi_pinctrl *pmx,
-				 struct pinctrl_desc *desc)
+int pinctrl_scmi_get_pins(struct scmi_pinctrl *pmx, struct pinctrl_desc *desc)
 {
 	struct pinctrl_pin_desc *pins;
 	unsigned int npins;
@@ -502,6 +490,7 @@  static int pinctrl_scmi_get_pins(struct scmi_pinctrl *pmx,
 
 	return 0;
 }
+EXPORT_SYMBOL(pinctrl_scmi_get_pins);
 
 static int scmi_pinctrl_probe(struct scmi_device *sdev)
 {
diff --git a/drivers/pinctrl/pinctrl-scmi.h b/drivers/pinctrl/pinctrl-scmi.h
new file mode 100644
index 000000000000..ae9e0be7c89e
--- /dev/null
+++ b/drivers/pinctrl/pinctrl-scmi.h
@@ -0,0 +1,29 @@ 
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Copyright 2024 NXP
+ */
+
+#ifndef __DRIVERS_PINCTRL_SCMI_H
+#define __DRIVERS_PINCTRL_SCMI_H
+
+#include <linux/device.h>
+#include <linux/pinctrl/pinctrl.h>
+#include <linux/scmi_protocol.h>
+
+#include "core.h"
+
+struct scmi_pinctrl {
+	struct device *dev;
+	struct scmi_protocol_handle *ph;
+	struct pinctrl_dev *pctldev;
+	struct pinctrl_desc pctl_desc;
+	struct pinfunction *functions;
+	unsigned int nr_functions;
+	struct pinctrl_pin_desc *pins;
+	unsigned int nr_pins;
+	const struct scmi_pinctrl_proto_ops *ops;
+};
+
+int pinctrl_scmi_get_pins(struct scmi_pinctrl *pmx, struct pinctrl_desc *desc);
+
+#endif /* __DRIVERS_PINCTRL_SCMI_H */