diff mbox series

[3/5] pinctrl: add helpers reading pins, groups & functions from DT

Message ID 20211118132152.15722-4-zajec5@gmail.com (mailing list archive)
State New, archived
Headers show
Series pinctrl: allow storing pins, groups & functions in DT | expand

Commit Message

Rafał Miłecki Nov. 18, 2021, 1:21 p.m. UTC 
From: Rafał Miłecki <rafal@milecki.pl>

DT binding allows specifying pins, groups & functions now. That allows
storing them in DT instead of hardcoding in drivers.

Introduce helpers based on CONFIG_GENERIC_PINCONF,
CONFIG_GENERIC_PINCTRL_GROUPS and CONFIG_GENERIC_PINMUX_FUNCTIONS for
parsing that info into pinctrl generic structures.

Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
---
 drivers/pinctrl/core.c   | 89 ++++++++++++++++++++++++++++++++++++++++
 drivers/pinctrl/core.h   |  5 +++
 drivers/pinctrl/pinmux.c | 43 +++++++++++++++++++
 drivers/pinctrl/pinmux.h |  2 +
 4 files changed, 139 insertions(+)

Comments

Andy Shevchenko Nov. 18, 2021, 1:57 p.m. UTC | #1 
On Thu, Nov 18, 2021 at 3:22 PM Rafał Miłecki <zajec5@gmail.com> wrote:

...

> --- a/drivers/pinctrl/pinmux.c
> +++ b/drivers/pinctrl/pinmux.c

> +#include <linux/of.h>

I don't like this. This shows not thought through the design of the series.

What I rather expect is a proper interfacing layer that you fill with
options that can be provided by corresponding underlying
implementation, e.g. DT.

Moreover, before doing this you probably would need to refactor the
pin control core to get rid of DT specifics, i.e. abstract them away
first.
Rafał Miłecki Nov. 18, 2021, 2:17 p.m. UTC | #2 
On 18.11.2021 14:57, Andy Shevchenko wrote:
> On Thu, Nov 18, 2021 at 3:22 PM Rafał Miłecki <zajec5@gmail.com> wrote:
> 
> ...
> 
>> --- a/drivers/pinctrl/pinmux.c
>> +++ b/drivers/pinctrl/pinmux.c
> 
>> +#include <linux/of.h>
> 
> I don't like this. This shows not thought through the design of the series.
> 
> What I rather expect is a proper interfacing layer that you fill with
> options that can be provided by corresponding underlying
> implementation, e.g. DT.
> 
> Moreover, before doing this you probably would need to refactor the
> pin control core to get rid of DT specifics, i.e. abstract them away
> first.

Ouch, it seems like pinctrl got into a tricky state. As I understand it
we need some abstraction layer between DT and pinctrl but noone is
working on it? Does it mean we should consider pinctrl core frozen until
it's refactored?

It's quite inconvenient for me as I'm not sure if I can handle such
heavy pinctrl refactoring while at the same time I'd like to add
those small features to it.

Can you point to an example of extra interfacing layer that could be
used as a reference for what you expect for pinctrl one, please? Some
solution in another Linux's subsystem?
Andy Shevchenko Nov. 18, 2021, 4:22 p.m. UTC | #3 
On Thu, Nov 18, 2021 at 4:17 PM Rafał Miłecki <zajec5@gmail.com> wrote:
> On 18.11.2021 14:57, Andy Shevchenko wrote:
> > On Thu, Nov 18, 2021 at 3:22 PM Rafał Miłecki <zajec5@gmail.com> wrote:

...

> >> +#include <linux/of.h>
> >
> > I don't like this. This shows not thought through the design of the series.
> >
> > What I rather expect is a proper interfacing layer that you fill with
> > options that can be provided by corresponding underlying
> > implementation, e.g. DT.
> >
> > Moreover, before doing this you probably would need to refactor the
> > pin control core to get rid of DT specifics, i.e. abstract them away
> > first.
>
> Ouch, it seems like pinctrl got into a tricky state. As I understand it
> we need some abstraction layer between DT and pinctrl but noone is
> working on it?

Seems so to me.
To be more specific, I'm talking about these:

  struct pinctrl_ops {
    ...
    int (*dt_node_to_map)...
    void (*dt_free_map)...
  }

and this:

  struct pinctrl {
    ...
    struct list_head dt_maps;
 }

In the latter case it is almost related to renaming dt_maps to
something like fw_maps.
In both cases it is about decoupling DT stuff out from the pin control core.

So, with something like pinctrl_fw_ops to be introduced, the DT stuff
moved to drivers/pinctrl/devicetree.c.

> Does it mean we should consider pinctrl core frozen until
> it's refactored?

Solutions which are related to pin control core without keeping non-DT
providers in mind will be NAKed by me, definitely. Of course it can be
overridden by maintainers.

> It's quite inconvenient for me as I'm not sure if I can handle such
> heavy pinctrl refactoring while at the same time I'd like to add
> those small features to it.

Which effectively means "let give somebody else even more burden and problems".

> Can you point to an example of extra interfacing layer that could be
> used as a reference for what you expect for pinctrl one, please? Some
> solution in another Linux's subsystem?

GPIOLIB decoupled this.
Another example (not sure if it's good enough here) is the fwnode API
(see fwnode ops).
Linus Walleij Nov. 21, 2021, 11:53 p.m. UTC | #4 
On Thu, Nov 18, 2021 at 2:58 PM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
> On Thu, Nov 18, 2021 at 3:22 PM Rafał Miłecki <zajec5@gmail.com> wrote:

> > +#include <linux/of.h>
>
> I don't like this. This shows not thought through the design of the series.
>
> What I rather expect is a proper interfacing layer that you fill with
> options that can be provided by corresponding underlying
> implementation, e.g. DT.

I agree, the DT parts should land in pinctrl/devicetree.c in the
end with an opt-in for ACPI & board files. I don't know if we
can use software nodes fully instead of board file-specific
helpers at this point, it seems to have
grown pretty competent.

Yours,
Linus Walleij
diff mbox series

Patch

diff --git a/drivers/pinctrl/core.c b/drivers/pinctrl/core.c
index ffe39336fcac..8f6ed8488313 100644
--- a/drivers/pinctrl/core.c
+++ b/drivers/pinctrl/core.c
@@ -515,8 +515,97 @@  void pinctrl_remove_gpio_range(struct pinctrl_dev *pctldev,
 }
 EXPORT_SYMBOL_GPL(pinctrl_remove_gpio_range);
 
+int pinctrl_generic_get_dt_pins(struct pinctrl_desc *pctldesc,
+				struct device *dev)
+{
+	struct pinctrl_pin_desc *descs;
+	struct device_node *pins;
+	struct device_node *np;
+	int err = 0;
+	int i = 0;
+
+	pins = of_get_child_by_name(dev->of_node, "pins");
+	if (!pins) {
+		dev_err(dev, "failed to find \"pins\" DT node\n");
+		err = -ENOENT;
+		goto err_out;
+	}
+
+	pctldesc->npins = of_get_available_child_count(pins);
+
+	descs = devm_kcalloc(dev, pctldesc->npins, sizeof(*descs), GFP_KERNEL);
+	if (!descs) {
+		err = -ENOMEM;
+		goto err_put_node;
+	}
+
+	for_each_available_child_of_node(pins, np) {
+		if (of_property_read_u32(np, "reg", &descs[i].number)) {
+			dev_err(dev, "missing \"reg\" property in %pOF\n", np);
+			err = -ENOENT;
+			goto err_put_node;
+		}
+
+		if (of_property_read_string(np, "label", &descs[i].name)) {
+			dev_err(dev, "missing \"label\" property in %pOF\n", np);
+			err = -ENOENT;
+			goto err_put_node;
+		}
+
+		i++;
+	}
+
+	pctldesc->pins = descs;
+
+err_put_node:
+	of_node_put(pins);
+err_out:
+	return err;
+}
+EXPORT_SYMBOL_GPL(pinctrl_generic_get_dt_pins);
+
 #ifdef CONFIG_GENERIC_PINCTRL_GROUPS
 
+int pinctrl_generic_get_dt_groups(struct pinctrl_dev *pctldev)
+{
+	struct device *dev = pctldev->dev;
+	struct device_node *groups;
+	struct device_node *np;
+	int err = 0;
+
+	groups = of_get_child_by_name(dev->of_node, "groups");
+	if (!groups) {
+		dev_err(dev, "failed to find \"groups\" DT node\n");
+		err = -ENOENT;
+		goto err_out;
+	}
+
+	for_each_available_child_of_node(groups, np) {
+		int num_pins;
+		u32 *pins;
+
+		num_pins = of_property_count_u32_elems(np, "pins");
+		pins = devm_kmalloc_array(dev, num_pins, sizeof(*pins), GFP_KERNEL);
+		if (!pins) {
+			err = -ENOMEM;
+			goto err_put_node;
+		}
+
+		if (of_property_read_u32_array(np, "pins", pins, num_pins)) {
+			err = -EIO;
+			goto err_put_node;
+		}
+
+		pinctrl_generic_add_group(pctldev, np->name, pins, num_pins, np);
+	}
+
+err_put_node:
+	of_node_put(groups);
+err_out:
+	return err;
+}
+EXPORT_SYMBOL_GPL(pinctrl_generic_get_dt_groups);
+
 /**
  * pinctrl_generic_get_group_count() - returns the number of pin groups
  * @pctldev: pin controller device
diff --git a/drivers/pinctrl/core.h b/drivers/pinctrl/core.h
index 840103c40c14..59661d4d4cc7 100644
--- a/drivers/pinctrl/core.h
+++ b/drivers/pinctrl/core.h
@@ -182,6 +182,9 @@  struct pinctrl_maps {
 	unsigned num_maps;
 };
 
+int pinctrl_generic_get_dt_pins(struct pinctrl_desc *pctldesc,
+				struct device *dev);
+
 #ifdef CONFIG_GENERIC_PINCTRL_GROUPS
 
 /**
@@ -198,6 +201,8 @@  struct group_desc {
 	void *data;
 };
 
+int pinctrl_generic_get_dt_groups(struct pinctrl_dev *pctldev);
+
 int pinctrl_generic_get_group_count(struct pinctrl_dev *pctldev);
 
 const char *pinctrl_generic_get_group_name(struct pinctrl_dev *pctldev,
diff --git a/drivers/pinctrl/pinmux.c b/drivers/pinctrl/pinmux.c
index 6cdbd9ccf2f0..5e34bd3135f5 100644
--- a/drivers/pinctrl/pinmux.c
+++ b/drivers/pinctrl/pinmux.c
@@ -24,6 +24,7 @@ 
 #include <linux/string.h>
 #include <linux/debugfs.h>
 #include <linux/seq_file.h>
+#include <linux/of.h>
 #include <linux/pinctrl/machine.h>
 #include <linux/pinctrl/pinmux.h>
 #include "core.h"
@@ -788,6 +789,48 @@  void pinmux_init_device_debugfs(struct dentry *devroot,
 
 #ifdef CONFIG_GENERIC_PINMUX_FUNCTIONS
 
+int pinmux_generic_get_dt_functions(struct pinctrl_dev *pctldev)
+{
+	struct device *dev = pctldev->dev;
+	struct device_node *functions;
+	struct device_node *np;
+	int err = 0;
+
+	functions = of_get_child_by_name(dev->of_node, "functions");
+	if (!functions) {
+		dev_err(dev, "failed to find \"functions\" DT node\n");
+		err = -ENOENT;
+		goto err_out;
+	}
+
+	for_each_available_child_of_node(functions, np) {
+		int num_groups = of_count_phandle_with_args(np, "groups", NULL);
+		struct of_phandle_iterator it;
+		const char **groups;
+		int ret;
+		int i;
+
+		groups = devm_kmalloc_array(dev, num_groups, sizeof(*groups), GFP_KERNEL);
+		if (!groups) {
+			err = -ENOMEM;
+			goto err_put_node;
+		}
+
+		i = 0;
+		of_for_each_phandle(&it, ret, np, "groups", NULL, 0) {
+			groups[i++] = it.node->name;
+		}
+
+		pinmux_generic_add_function(pctldev, np->name, groups, num_groups, np);
+	}
+
+err_put_node:
+	of_node_put(functions);
+err_out:
+	return err;
+}
+EXPORT_SYMBOL_GPL(pinmux_generic_get_dt_functions);
+
 /**
  * pinmux_generic_get_function_count() - returns number of functions
  * @pctldev: pin controller device
diff --git a/drivers/pinctrl/pinmux.h b/drivers/pinctrl/pinmux.h
index 78c3a31be882..ca69025fce46 100644
--- a/drivers/pinctrl/pinmux.h
+++ b/drivers/pinctrl/pinmux.h
@@ -134,6 +134,8 @@  struct function_desc {
 	void *data;
 };
 
+int pinmux_generic_get_dt_functions(struct pinctrl_dev *pctldev);
+
 int pinmux_generic_get_function_count(struct pinctrl_dev *pctldev);
 
 const char *