Add the Lantiq PEF2256 audio support

The loop searching for a matching device based on its compatible string is aborted when a matching disabled device is found. This abort avoid to add devices as soon as one disabled device is found.

Continue searching for an other device instead of aborting on the first disabled one fixes the issue.

Fixes 22380b65dc70 ("mfd: mfd-core: Ensure disabled devices are ignored without error")

Signed-off-by Herve Codina <[email protected]>

---
 drivers/mfd/mfd-core.c | 18 +++++++++++++-----
 1 file changed, 13 insertions(+), 5 deletions(-)

drivers/mfd/mfd-core.c +1 -0

index 16d1861e9682..7c47b50b358d 100644
@@ -176,6 +176,7 @@ static int mfd_add_device(struct device *parent, int id,
 	struct platform_device *pdev;
 	struct device_node *np = NULL;
 	struct mfd_of_node_entry *of_entry, *tmp;
+	bool not_available;
 	int ret = -ENOMEM;
 	int platform_id;
 	int r;

drivers/mfd/mfd-core.c +10 -3

-					ret = 0;
-					goto fail_alias;
+					not_available = true;
+					continue;
 				}
 
 				ret = mfd_match_of_node_to_dev(pdev, np, cell);

@@ -227,10 +228,17 @@ static int mfd_add_device(struct device *parent, int id,
 				if (ret)
 					goto fail_alias;
 
-				break;
+				goto match;
 			}
 		}
 
+		if (not_available) {
+			/* Ignore 'disabled' devices error free */
+			ret = 0;
+			goto fail_alias;
+		}
+
+match:
 		if (!pdev->dev.of_node)
 			pr_warn("%s: Failed to locate of_node [id: %d]\n",
 				cell->name, platform_id);

Signature · 1 line

2.39.2

Quoted 29 lines · Herve Codina

The loop searching for a matching device based on its compatible string is aborted when a matching disabled device is found. This abort avoid to add devices as soon as one disabled device is found.

Continue searching for an other device instead of aborting on the first disabled one fixes the issue.

Fixes 22380b65dc70 ("mfd: mfd-core: Ensure disabled devices are ignored without error")

Signed-off-by Herve Codina <[email protected]>

---
 drivers/mfd/mfd-core.c | 18 +++++++++++++-----
 1 file changed, 13 insertions(+), 5 deletions(-)

drivers/mfd/mfd-core.c +1 -0

index 16d1861e9682..7c47b50b358d 100644
@@ -176,6 +176,7 @@ static int mfd_add_device(struct device *parent, int id,
 	struct platform_device *pdev;
 	struct device_node *np = NULL;
 	struct mfd_of_node_entry *of_entry, *tmp;
+	bool not_available;
 	int ret = -ENOMEM;
 	int platform_id;
 	int r;

@@ -211,13 +212,13 @@ static int mfd_add_device(struct device *parent, int id,
 		goto fail_res;
 

drivers/mfd/mfd-core.c static int mfd_add_device(struct device *parent, int id, · line 215

Quoted 2 lines

drivers/mfd/mfd-core.c +1 -0

 	if (IS_ENABLED(CONFIG_OF) && parent->of_node && cell->of_compatible) {
+		not_available = false;  

Why not do:

		bool disabled = false;

... here instead?

drivers/mfd/mfd-core.c static int mfd_add_device(struct device *parent, int id, · line 218

Quoted 7 lines

drivers/mfd/mfd-core.c +1 -2

 		for_each_child_of_node(parent->of_node, np) {
 			if (of_device_is_compatible(np, cell->of_compatible)) {
-				/* Ignore 'disabled' devices error free */
+				/* Skip 'disabled' devices */
 				if (!of_device_is_available(np)) {
-					of_node_put(np);  

Why are you removing the put?

Quoted 17 lines

drivers/mfd/mfd-core.c +3 -3

-					ret = 0;
-					goto fail_alias;
+					not_available = true;
+					continue;
 				}
 
 				ret = mfd_match_of_node_to_dev(pdev, np, cell);

@@ -227,10 +228,17 @@ static int mfd_add_device(struct device *parent, int id,
 				if (ret)
 					goto fail_alias;
 
-				break;
+				goto match;
 			}
 		}
 

Quoted 13 lines

drivers/mfd/mfd-core.c +7 -0

+		if (not_available) {
+			/* Ignore 'disabled' devices error free */
+			ret = 0;
+			goto fail_alias;
+		}
+
+match:
 		if (!pdev->dev.of_node)
 			pr_warn("%s: Failed to locate of_node [id: %d]\n",
 				cell->name, platform_id);

Signature · 1 line

2.39.2

Quoted 29 lines · Herve Codina

The loop searching for a matching device based on its compatible string is aborted when a matching disabled device is found. This abort avoid to add devices as soon as one disabled device is found.

Continue searching for an other device instead of aborting on the first disabled one fixes the issue.

Fixes 22380b65dc70 ("mfd: mfd-core: Ensure disabled devices are ignored without error")

Signed-off-by Herve Codina <[email protected]>

---
 drivers/mfd/mfd-core.c | 18 +++++++++++++-----
 1 file changed, 13 insertions(+), 5 deletions(-)

drivers/mfd/mfd-core.c +1 -0

index 16d1861e9682..7c47b50b358d 100644
@@ -176,6 +176,7 @@ static int mfd_add_device(struct device *parent, int id,
 	struct platform_device *pdev;
 	struct device_node *np = NULL;
 	struct mfd_of_node_entry *of_entry, *tmp;
+	bool not_available;
 	int ret = -ENOMEM;
 	int platform_id;
 	int r;

@@ -211,13 +212,13 @@ static int mfd_add_device(struct device *parent, int id,
 		goto fail_res;
 

drivers/mfd/mfd-core.c static int mfd_add_device(struct device *parent, int id, · line 215

Quoted 2 lines

drivers/mfd/mfd-core.c +1 -0

 	if (IS_ENABLED(CONFIG_OF) && parent->of_node && cell->of_compatible) {
+		not_available = false;  

Why not do:

		bool disabled = false;

... here instead?

drivers/mfd/mfd-core.c static int mfd_add_device(struct device *parent, int id, · line 218

Quoted 7 lines

drivers/mfd/mfd-core.c +1 -2

 		for_each_child_of_node(parent->of_node, np) {
 			if (of_device_is_compatible(np, cell->of_compatible)) {
-				/* Ignore 'disabled' devices error free */
+				/* Skip 'disabled' devices */
 				if (!of_device_is_available(np)) {
-					of_node_put(np);  

Why are you removing the put?

Quoted 17 lines

drivers/mfd/mfd-core.c +3 -3

-					ret = 0;
-					goto fail_alias;
+					not_available = true;
+					continue;
 				}
 
 				ret = mfd_match_of_node_to_dev(pdev, np, cell);

@@ -227,10 +228,17 @@ static int mfd_add_device(struct device *parent, int id,
 				if (ret)
 					goto fail_alias;
 
-				break;
+				goto match;
 			}
 		}
 

Quoted 13 lines

drivers/mfd/mfd-core.c +7 -0

+		if (not_available) {
+			/* Ignore 'disabled' devices error free */
+			ret = 0;
+			goto fail_alias;
+		}
+
+match:
 		if (!pdev->dev.of_node)
 			pr_warn("%s: Failed to locate of_node [id: %d]\n",
 				cell->name, platform_id);

Signature · 1 line

2.39.2

The Lantiq PEF2256 is a framer and line interface component designed to fulfill all required interfacing between an analog E1/T1/J1 line and the digital PCM system highway/H.100 bus.

Signed-off-by Herve Codina <[email protected]>

---
 drivers/mfd/Kconfig         |  16 +
 drivers/mfd/Makefile        |   1 +
 drivers/mfd/pef2256-regs.h  | 250 ++++++++++
 drivers/mfd/pef2256.c       | 950 ++++++++++++++++++++++++++++++++++++

 include/linux/mfd/pef2256.h |  52 ++
 5 files changed, 1269 insertions(+)
 create mode 100644 drivers/mfd/pef2256-regs.h
 create mode 100644 drivers/mfd/pef2256.c
 create mode 100644 include/linux/mfd/pef2256.h

drivers/mfd/pef2256.c new file +17 -0

+	if (!count)
+		return 0;
+
+	audio_devs = kcalloc(count, sizeof(*audio_devs), GFP_KERNEL);
+	if (!audio_devs)
+		return -ENOMEM;
+
+	for (i = 0; i < count; i++) {
+		(audio_devs + i)->name = "lantiq-pef2256-codec";
+		(audio_devs + i)->of_compatible = compatible;
+		(audio_devs + i)->id = i;
+	}
+
+	ret = mfd_add_devices(pef2256->dev, 0, audio_devs, count, NULL, 0, NULL);
+	kfree(audio_devs);
+	return ret;
+}

Quoted 10 lines · Herve Codina

The Lantiq PEF2256 is a framer and line interface component designed to fulfill all required interfacing between an analog E1/T1/J1 line and the digital PCM system highway/H.100 bus.

Signed-off-by Herve Codina <[email protected]>

---
 drivers/mfd/Kconfig         |  16 +
 drivers/mfd/Makefile        |   1 +
 drivers/mfd/pef2256-regs.h  | 250 ++++++++++
 drivers/mfd/pef2256.c       | 950 ++++++++++++++++++++++++++++++++++++  

95% of this driver needs to be moved somewhere else.

What is a Framer? Perhaps sound/ is a good candidate?

Quoted 6 lines

 include/linux/mfd/pef2256.h |  52 ++
 5 files changed, 1269 insertions(+)
 create mode 100644 drivers/mfd/pef2256-regs.h
 create mode 100644 drivers/mfd/pef2256.c
 create mode 100644 include/linux/mfd/pef2256.h  

[...]

drivers/mfd/pef2256.c line 728

Quoted 13 lines

drivers/mfd/pef2256.c new file +13 -0

+static int pef2256_add_audio_devices(struct pef2256 *pef2256)
+{
+	const char *compatible = "lantiq,pef2256-codec";
+	struct mfd_cell *audio_devs;
+	struct device_node *np;
+	unsigned int count = 0;
+	unsigned int i;
+	int ret;
+
+	for_each_available_child_of_node(pef2256->dev->of_node, np) {
+		if (of_device_is_compatible(np, compatible))
+			count++;
+	}  

Converting Device Tree nodes into MFD cells to register with the Platform Device API is not a reasonable use-case of MFD.

Have the CODEC driver match on "lantiq,pef2256-codec" and let it instantiate itself.

Your first version using of_platform_populate() was closer to the mark.

Quoted 10 lines

drivers/mfd/pef2256.c new file +9 -0

+	if (!count)
+		return 0;
+
+	audio_devs = kcalloc(count, sizeof(*audio_devs), GFP_KERNEL);
+	if (!audio_devs)
+		return -ENOMEM;
+
+	for (i = 0; i < count; i++) {
+		(audio_devs + i)->name = "lantiq-pef2256-codec";

drivers/mfd/pef2256.c line 746

Quoted 8 lines

drivers/mfd/pef2256.c new file +8 -0

+		(audio_devs + i)->of_compatible = compatible;
+		(audio_devs + i)->id = i;
+	}
+
+	ret = mfd_add_devices(pef2256->dev, 0, audio_devs, count, NULL, 0, NULL);
+	kfree(audio_devs);
+	return ret;
+}  

Quoted 13 lines · On Thu, 20 Apr 2023 13:39:46 +0100 Lee Jones <[email protected]> wrote:

Quoted 10 lines · Herve Codina

The Lantiq PEF2256 is a framer and line interface component designed to fulfill all required interfacing between an analog E1/T1/J1 line and the digital PCM system highway/H.100 bus.

Signed-off-by Herve Codina <[email protected]>

---
 drivers/mfd/Kconfig         |  16 +
 drivers/mfd/Makefile        |   1 +
 drivers/mfd/pef2256-regs.h  | 250 ++++++++++
 drivers/mfd/pef2256.c       | 950 ++++++++++++++++++++++++++++++++++++  

Quoted 3 lines

95% of this driver needs to be moved somewhere else.

What is a Framer? Perhaps sound/ is a good candidate?

The pef2256 framer is a device that transfers data to/from a TDM (time-slots data) from/to quite old telecommunication lines (E1 in my case). Several subsystem can set/get data to/from the TDM. Each device using their own time-slots set.

On my use-case, I have some audio consumer and a not yet upstreamed HDLC consumer. Both of them uses the framer to know the E1 link state. The framer needs to be initialized 'globally' and not by a specific consumer as several consumers can use the framer.

Quoted 25 lines

Quoted 5 lines

 include/linux/mfd/pef2256.h |  52 ++
 5 files changed, 1269 insertions(+)
 create mode 100644 drivers/mfd/pef2256-regs.h
 create mode 100644 drivers/mfd/pef2256.c
 create mode 100644 include/linux/mfd/pef2256.h  

[...]

drivers/mfd/pef2256.c line 728

Quoted 13 lines

drivers/mfd/pef2256.c new file +13 -0

+static int pef2256_add_audio_devices(struct pef2256 *pef2256)
+{
+	const char *compatible = "lantiq,pef2256-codec";
+	struct mfd_cell *audio_devs;
+	struct device_node *np;
+	unsigned int count = 0;
+	unsigned int i;
+	int ret;
+
+	for_each_available_child_of_node(pef2256->dev->of_node, np) {
+		if (of_device_is_compatible(np, compatible))
+			count++;
+	}  

Converting Device Tree nodes into MFD cells to register with the Platform Device API is not a reasonable use-case of MFD.

Quoted 2 lines

Have the CODEC driver match on "lantiq,pef2256-codec" and let it instantiate itself.

As the framer is going to used by several subsystem, I cannot instantiate it in the specific ASoC subsystem.

Quoted 2 lines

Your first version using of_platform_populate() was closer to the mark.

The issue was that I need MFD cells for the pinctrl part.

Quoted 17 lines

Quoted 17 lines

drivers/mfd/pef2256.c new file +17 -0

+	if (!count)
+		return 0;
+
+	audio_devs = kcalloc(count, sizeof(*audio_devs), GFP_KERNEL);
+	if (!audio_devs)
+		return -ENOMEM;
+
+	for (i = 0; i < count; i++) {
+		(audio_devs + i)->name = "lantiq-pef2256-codec";
+		(audio_devs + i)->of_compatible = compatible;
+		(audio_devs + i)->id = i;
+	}
+
+	ret = mfd_add_devices(pef2256->dev, 0, audio_devs, count, NULL, 0, NULL);
+	kfree(audio_devs);
+	return ret;
+}  

Quoted 25 lines · Herve Codina

Quoted 13 lines · On Thu, 20 Apr 2023 13:39:46 +0100 Lee Jones <[email protected]> wrote:

Quoted 10 lines · Herve Codina

The Lantiq PEF2256 is a framer and line interface component designed to fulfill all required interfacing between an analog E1/T1/J1 line and the digital PCM system highway/H.100 bus.

Signed-off-by Herve Codina <[email protected]>

---
 drivers/mfd/Kconfig         |  16 +
 drivers/mfd/Makefile        |   1 +
 drivers/mfd/pef2256-regs.h  | 250 ++++++++++
 drivers/mfd/pef2256.c       | 950 ++++++++++++++++++++++++++++++++++++    

Quoted 3 lines

95% of this driver needs to be moved somewhere else.

What is a Framer? Perhaps sound/ is a good candidate?

The pef2256 framer is a device that transfers data to/from a TDM (time-slots data) from/to quite old telecommunication lines (E1 in my case). Several subsystem can set/get data to/from the TDM. Each device using their own time-slots set.

Quoted 4 lines

On my use-case, I have some audio consumer and a not yet upstreamed HDLC consumer. Both of them uses the framer to know the E1 link state. The framer needs to be initialized 'globally' and not by a specific consumer as several consumers can use the framer.

I can't think of a good place for this.

If all else fails, it's drivers/misc

Quoted 32 lines

Quoted 25 lines

Quoted 5 lines

 include/linux/mfd/pef2256.h |  52 ++
 5 files changed, 1269 insertions(+)
 create mode 100644 drivers/mfd/pef2256-regs.h
 create mode 100644 drivers/mfd/pef2256.c
 create mode 100644 include/linux/mfd/pef2256.h    

[...]

drivers/mfd/pef2256.c line 728

Quoted 13 lines

drivers/mfd/pef2256.c new file +13 -0

+static int pef2256_add_audio_devices(struct pef2256 *pef2256)
+{
+	const char *compatible = "lantiq,pef2256-codec";
+	struct mfd_cell *audio_devs;
+	struct device_node *np;
+	unsigned int count = 0;
+	unsigned int i;
+	int ret;
+
+	for_each_available_child_of_node(pef2256->dev->of_node, np) {
+		if (of_device_is_compatible(np, compatible))
+			count++;
+	}    

Converting Device Tree nodes into MFD cells to register with the Platform Device API is not a reasonable use-case of MFD.

Quoted 2 lines

Have the CODEC driver match on "lantiq,pef2256-codec" and let it instantiate itself.

As the framer is going to used by several subsystem, I cannot instantiate it in the specific ASoC subsystem.

Quoted 1 line

Quoted 3 lines

Quoted 1 line

Your first version using of_platform_populate() was closer to the mark.

The issue was that I need MFD cells for the pinctrl part.

Why can't it be represented in DT?

Hi Lee, Krzysztof,

Quoted 68 lines · On Thu, 20 Apr 2023 14:47:03 +0100 Lee Jones <[email protected]> wrote:

Quoted 25 lines · Herve Codina

Quoted 13 lines · On Thu, 20 Apr 2023 13:39:46 +0100 Lee Jones <[email protected]> wrote:

Quoted 10 lines · Herve Codina

The Lantiq PEF2256 is a framer and line interface component designed to fulfill all required interfacing between an analog E1/T1/J1 line and the digital PCM system highway/H.100 bus.

Signed-off-by Herve Codina <[email protected]>

---
 drivers/mfd/Kconfig         |  16 +
 drivers/mfd/Makefile        |   1 +
 drivers/mfd/pef2256-regs.h  | 250 ++++++++++
 drivers/mfd/pef2256.c       | 950 ++++++++++++++++++++++++++++++++++++    

Quoted 3 lines

95% of this driver needs to be moved somewhere else.

What is a Framer? Perhaps sound/ is a good candidate?

The pef2256 framer is a device that transfers data to/from a TDM (time-slots data) from/to quite old telecommunication lines (E1 in my case). Several subsystem can set/get data to/from the TDM. Each device using their own time-slots set.

Quoted 4 lines

On my use-case, I have some audio consumer and a not yet upstreamed HDLC consumer. Both of them uses the framer to know the E1 link state. The framer needs to be initialized 'globally' and not by a specific consumer as several consumers can use the framer.

I can't think of a good place for this.

If all else fails, it's drivers/misc

Quoted 32 lines

Quoted 25 lines

Quoted 5 lines

 include/linux/mfd/pef2256.h |  52 ++
 5 files changed, 1269 insertions(+)
 create mode 100644 drivers/mfd/pef2256-regs.h
 create mode 100644 drivers/mfd/pef2256.c
 create mode 100644 include/linux/mfd/pef2256.h    

[...]

drivers/mfd/pef2256.c line 728

Quoted 13 lines

drivers/mfd/pef2256.c new file +13 -0

+static int pef2256_add_audio_devices(struct pef2256 *pef2256)
+{
+	const char *compatible = "lantiq,pef2256-codec";
+	struct mfd_cell *audio_devs;
+	struct device_node *np;
+	unsigned int count = 0;
+	unsigned int i;
+	int ret;
+
+	for_each_available_child_of_node(pef2256->dev->of_node, np) {
+		if (of_device_is_compatible(np, compatible))
+			count++;
+	}    

Converting Device Tree nodes into MFD cells to register with the Platform Device API is not a reasonable use-case of MFD.

Quoted 2 lines

Have the CODEC driver match on "lantiq,pef2256-codec" and let it instantiate itself.

As the framer is going to used by several subsystem, I cannot instantiate it in the specific ASoC subsystem.

Quoted 1 line

Quoted 5 lines

Quoted 3 lines

Quoted 1 line

Your first version using of_platform_populate() was closer to the mark.

The issue was that I need MFD cells for the pinctrl part.

Why can't it be represented in DT?

The pinctrl part has no specific compatible string.
Not sure that a compatible string for pinctrl can be accepted as there is only one pinctrl subnode and no specific reg for this subnode.

The DT looks like this:

    framer@2000000 {
      compatible = "lantiq,pef2256";
      reg = <0x2000000 0x100>;
      ...
      pinctrl {
        pef2256_rpa_sypr: rpa-pins {
          pins = "RPA";
          function = "SYPR";
        };
      };

      pef2256_codec0: codec-0 {
        compatible = "lantiq,pef2256-codec";
        #sound-dai-cells = <0>;
        sound-name-prefix = "PEF2256_0";
      };
    };

Krzysztof, is it acceptable to have a compatible string in the pinctrl node ?

In this case, it will looks like this:

    framer@2000000 {
      compatible = "lantiq,pef2256";
      reg = <0x2000000 0x100>;
      ...
      pinctrl {
        compatible = "lantiq,pef2256-pinctrl";
        pef2256_rpa_sypr: rpa-pins {
          pins = "RPA";
          function = "SYPR";
        };
      };

      pef2256_codec0: codec-0 {
        compatible = "lantiq,pef2256-codec";
        #sound-dai-cells = <0>;
        sound-name-prefix = "PEF2256_0";
      };
    };

Best regards,
Hervé

Quoted 103 lines · Herve Codina

Hi Lee, Krzysztof,

Quoted 68 lines · On Thu, 20 Apr 2023 14:47:03 +0100 Lee Jones <[email protected]> wrote:

Quoted 25 lines · Herve Codina

Quoted 13 lines · On Thu, 20 Apr 2023 13:39:46 +0100 Lee Jones <[email protected]> wrote:

Quoted 10 lines · Herve Codina

The Lantiq PEF2256 is a framer and line interface component designed to fulfill all required interfacing between an analog E1/T1/J1 line and the digital PCM system highway/H.100 bus.

Signed-off-by Herve Codina <[email protected]>

---
 drivers/mfd/Kconfig         |  16 +
 drivers/mfd/Makefile        |   1 +
 drivers/mfd/pef2256-regs.h  | 250 ++++++++++
 drivers/mfd/pef2256.c       | 950 ++++++++++++++++++++++++++++++++++++    

Quoted 3 lines

95% of this driver needs to be moved somewhere else.

What is a Framer? Perhaps sound/ is a good candidate?

The pef2256 framer is a device that transfers data to/from a TDM (time-slots data) from/to quite old telecommunication lines (E1 in my case). Several subsystem can set/get data to/from the TDM. Each device using their own time-slots set.

Quoted 4 lines

On my use-case, I have some audio consumer and a not yet upstreamed HDLC consumer. Both of them uses the framer to know the E1 link state. The framer needs to be initialized 'globally' and not by a specific consumer as several consumers can use the framer.

I can't think of a good place for this.

If all else fails, it's drivers/misc

Quoted 32 lines

Quoted 25 lines

Quoted 5 lines

 include/linux/mfd/pef2256.h |  52 ++
 5 files changed, 1269 insertions(+)
 create mode 100644 drivers/mfd/pef2256-regs.h
 create mode 100644 drivers/mfd/pef2256.c
 create mode 100644 include/linux/mfd/pef2256.h    

[...]

drivers/mfd/pef2256.c line 728

Quoted 13 lines

drivers/mfd/pef2256.c new file +13 -0

+static int pef2256_add_audio_devices(struct pef2256 *pef2256)
+{
+	const char *compatible = "lantiq,pef2256-codec";
+	struct mfd_cell *audio_devs;
+	struct device_node *np;
+	unsigned int count = 0;
+	unsigned int i;
+	int ret;
+
+	for_each_available_child_of_node(pef2256->dev->of_node, np) {
+		if (of_device_is_compatible(np, compatible))
+			count++;
+	}    

Converting Device Tree nodes into MFD cells to register with the Platform Device API is not a reasonable use-case of MFD.

Quoted 2 lines

Have the CODEC driver match on "lantiq,pef2256-codec" and let it instantiate itself.

As the framer is going to used by several subsystem, I cannot instantiate it in the specific ASoC subsystem.

Quoted 1 line

Quoted 5 lines

Quoted 3 lines

Quoted 1 line

Your first version using of_platform_populate() was closer to the mark.

The issue was that I need MFD cells for the pinctrl part.

Why can't it be represented in DT?

The pinctrl part has no specific compatible string.
Not sure that a compatible string for pinctrl can be accepted as there is only one pinctrl subnode and no specific reg for this subnode.

The DT looks like this:

    framer@2000000 {
      compatible = "lantiq,pef2256";
      reg = <0x2000000 0x100>;
      ...
      pinctrl {
        pef2256_rpa_sypr: rpa-pins {
          pins = "RPA";
          function = "SYPR";
        };
      };

      pef2256_codec0: codec-0 {
        compatible = "lantiq,pef2256-codec";
        #sound-dai-cells = <0>;
        sound-name-prefix = "PEF2256_0";
      };
    };

Quoted 1 line

Krzysztof, is it acceptable to have a compatible string in the pinctrl node ?

Why wouldn't it be?

$ git grep ".compatible" -- drivers/pinctrl/

Quoted 7 lines

In this case, it will looks like this:

    framer@2000000 {
      compatible = "lantiq,pef2256";
      reg = <0x2000000 0x100>;
      ...
      pinctrl {
        compatible = "lantiq,pef2256-pinctrl";

Quoted 105 lines · Lee Jones

Quoted 103 lines · Herve Codina

Hi Lee, Krzysztof,

Quoted 68 lines · On Thu, 20 Apr 2023 14:47:03 +0100 Lee Jones <[email protected]> wrote:

Quoted 25 lines · Herve Codina

Quoted 13 lines · On Thu, 20 Apr 2023 13:39:46 +0100 Lee Jones <[email protected]> wrote:

Quoted 10 lines · Herve Codina

The Lantiq PEF2256 is a framer and line interface component designed to fulfill all required interfacing between an analog E1/T1/J1 line and the digital PCM system highway/H.100 bus.

Signed-off-by Herve Codina <[email protected]>

---
 drivers/mfd/Kconfig         |  16 +
 drivers/mfd/Makefile        |   1 +
 drivers/mfd/pef2256-regs.h  | 250 ++++++++++
 drivers/mfd/pef2256.c       | 950 ++++++++++++++++++++++++++++++++++++    

Quoted 3 lines

95% of this driver needs to be moved somewhere else.

What is a Framer? Perhaps sound/ is a good candidate?

The pef2256 framer is a device that transfers data to/from a TDM (time-slots data) from/to quite old telecommunication lines (E1 in my case). Several subsystem can set/get data to/from the TDM. Each device using their own time-slots set.

Quoted 4 lines

On my use-case, I have some audio consumer and a not yet upstreamed HDLC consumer. Both of them uses the framer to know the E1 link state. The framer needs to be initialized 'globally' and not by a specific consumer as several consumers can use the framer.

I can't think of a good place for this.

If all else fails, it's drivers/misc

Quoted 32 lines

Quoted 25 lines

Quoted 5 lines

 include/linux/mfd/pef2256.h |  52 ++
 5 files changed, 1269 insertions(+)
 create mode 100644 drivers/mfd/pef2256-regs.h
 create mode 100644 drivers/mfd/pef2256.c
 create mode 100644 include/linux/mfd/pef2256.h    

[...]

Review

Quoted 13 lines

(quoted diff) +13 -0

+static int pef2256_add_audio_devices(struct pef2256 *pef2256)
+{
+	const char *compatible = "lantiq,pef2256-codec";
+	struct mfd_cell *audio_devs;
+	struct device_node *np;
+	unsigned int count = 0;
+	unsigned int i;
+	int ret;
+
+	for_each_available_child_of_node(pef2256->dev->of_node, np) {
+		if (of_device_is_compatible(np, compatible))
+			count++;
+	}    

Converting Device Tree nodes into MFD cells to register with the Platform Device API is not a reasonable use-case of MFD.

Quoted 2 lines

Have the CODEC driver match on "lantiq,pef2256-codec" and let it instantiate itself.

As the framer is going to used by several subsystem, I cannot instantiate it in the specific ASoC subsystem.

Quoted 1 line

Quoted 5 lines

Quoted 3 lines

Quoted 1 line

Your first version using of_platform_populate() was closer to the mark.

The issue was that I need MFD cells for the pinctrl part.

Why can't it be represented in DT?

The pinctrl part has no specific compatible string.
Not sure that a compatible string for pinctrl can be accepted as there is only one pinctrl subnode and no specific reg for this subnode.

The DT looks like this:

    framer@2000000 {
      compatible = "lantiq,pef2256";
      reg = <0x2000000 0x100>;
      ...
      pinctrl {
        pef2256_rpa_sypr: rpa-pins {
          pins = "RPA";
          function = "SYPR";
        };
      };

      pef2256_codec0: codec-0 {
        compatible = "lantiq,pef2256-codec";
        #sound-dai-cells = <0>;
        sound-name-prefix = "PEF2256_0";
      };
    };

Quoted 13 lines

Quoted 1 line

Krzysztof, is it acceptable to have a compatible string in the pinctrl node ?

Why wouldn't it be?

$ git grep ".compatible" -- drivers/pinctrl/

Quoted 7 lines

In this case, it will looks like this:

    framer@2000000 {
      compatible = "lantiq,pef2256";
      reg = <0x2000000 0x100>;
      ...
      pinctrl {
        compatible = "lantiq,pef2256-pinctrl";

If you do not have any resources, there is no point in having separate compatible for separate device node.

Anyway this discussions should not be about DT. How Linux drivers are implementing DT is not really a guide how to write DT. Since these series were brought there were some DT decisions made based how you want to write the driver. No, please don't. I also do not see any problems in handling more-or-less complex driver structures without poking the DT. We have already many such device families.

Quoted 76 lines

Quoted 75 lines

Quoted 62 lines

Quoted 32 lines

Quoted 32 lines

Quoted 25 lines

Quoted 5 lines

 include/linux/mfd/pef2256.h |  52 ++
 5 files changed, 1269 insertions(+)
 create mode 100644 drivers/mfd/pef2256-regs.h
 create mode 100644 drivers/mfd/pef2256.c
 create mode 100644 include/linux/mfd/pef2256.h    

[...]

Review

Quoted 13 lines

(quoted diff) +13 -0

+static int pef2256_add_audio_devices(struct pef2256 *pef2256)
+{
+	const char *compatible = "lantiq,pef2256-codec";
+	struct mfd_cell *audio_devs;
+	struct device_node *np;
+	unsigned int count = 0;
+	unsigned int i;
+	int ret;
+
+	for_each_available_child_of_node(pef2256->dev->of_node, np) {
+		if (of_device_is_compatible(np, compatible))
+			count++;
+	}    

Converting Device Tree nodes into MFD cells to register with the Platform Device API is not a reasonable use-case of MFD.

Quoted 2 lines

Have the CODEC driver match on "lantiq,pef2256-codec" and let it instantiate itself.

As the framer is going to used by several subsystem, I cannot instantiate it in the specific ASoC subsystem.

Quoted 1 line

Quoted 5 lines

Quoted 3 lines

Quoted 1 line

Your first version using of_platform_populate() was closer to the mark.

The issue was that I need MFD cells for the pinctrl part.

Why can't it be represented in DT?

The pinctrl part has no specific compatible string.
Not sure that a compatible string for pinctrl can be accepted as there is only one pinctrl subnode and no specific reg for this subnode.

The DT looks like this:

    framer@2000000 {
      compatible = "lantiq,pef2256";
      reg = <0x2000000 0x100>;
      ...
      pinctrl {
        pef2256_rpa_sypr: rpa-pins {
          pins = "RPA";
          function = "SYPR";
        };
      };

      pef2256_codec0: codec-0 {
        compatible = "lantiq,pef2256-codec";
        #sound-dai-cells = <0>;
        sound-name-prefix = "PEF2256_0";
      };
    };

Quoted 1 line

Krzysztof, is it acceptable to have a compatible string in the pinctrl node ?

Why wouldn't it be?

$ git grep ".compatible" -- drivers/pinctrl/

Quoted 7 lines

In this case, it will looks like this:

    framer@2000000 {
      compatible = "lantiq,pef2256";
      reg = <0x2000000 0x100>;
      ...
      pinctrl {
        compatible = "lantiq,pef2256-pinctrl";

Quoted 2 lines

If you do not have any resources, there is no point in having separate compatible for separate device node.

That's a new rule. Is that documented somewhere? I'm sure we already have device nodes for devices whom only operate on shared resources.

Quoted 25 lines · Herve Codina

Quoted 13 lines · On Thu, 20 Apr 2023 13:39:46 +0100 Lee Jones <[email protected]> wrote:

Quoted 10 lines · Herve Codina

The Lantiq PEF2256 is a framer and line interface component designed to fulfill all required interfacing between an analog E1/T1/J1 line and the digital PCM system highway/H.100 bus.

Signed-off-by Herve Codina <[email protected]>

---
 drivers/mfd/Kconfig         |  16 +
 drivers/mfd/Makefile        |   1 +
 drivers/mfd/pef2256-regs.h  | 250 ++++++++++
 drivers/mfd/pef2256.c       | 950 ++++++++++++++++++++++++++++++++++++    

Quoted 3 lines

95% of this driver needs to be moved somewhere else.

What is a Framer? Perhaps sound/ is a good candidate?

The pef2256 framer is a device that transfers data to/from a TDM (time-slots data) from/to quite old telecommunication lines (E1 in my case). Several subsystem can set/get data to/from the TDM. Each device using their own time-slots set.

Quoted 4 lines

On my use-case, I have some audio consumer and a not yet upstreamed HDLC consumer. Both of them uses the framer to know the E1 link state. The framer needs to be initialized 'globally' and not by a specific consumer as several consumers can use the framer.

I can't think of a good place for this.

If all else fails, it's drivers/misc

Quoted 32 lines

Quoted 25 lines

Quoted 5 lines

 include/linux/mfd/pef2256.h |  52 ++
 5 files changed, 1269 insertions(+)
 create mode 100644 drivers/mfd/pef2256-regs.h
 create mode 100644 drivers/mfd/pef2256.c
 create mode 100644 include/linux/mfd/pef2256.h    

[...]

drivers/mfd/pef2256.c line 728

Quoted 13 lines

drivers/mfd/pef2256.c new file +13 -0

+static int pef2256_add_audio_devices(struct pef2256 *pef2256)
+{
+	const char *compatible = "lantiq,pef2256-codec";
+	struct mfd_cell *audio_devs;
+	struct device_node *np;
+	unsigned int count = 0;
+	unsigned int i;
+	int ret;
+
+	for_each_available_child_of_node(pef2256->dev->of_node, np) {
+		if (of_device_is_compatible(np, compatible))
+			count++;
+	}    

Converting Device Tree nodes into MFD cells to register with the Platform Device API is not a reasonable use-case of MFD.

Quoted 2 lines

Have the CODEC driver match on "lantiq,pef2256-codec" and let it instantiate itself.

As the framer is going to used by several subsystem, I cannot instantiate it in the specific ASoC subsystem.

Quoted 1 line

Quoted 3 lines

Quoted 1 line

Your first version using of_platform_populate() was closer to the mark.

The issue was that I need MFD cells for the pinctrl part.

Why can't it be represented in DT?

Quoted 10 lines · Herve Codina

The Lantiq PEF2256 is a framer and line interface component designed to fulfill all required interfacing between an analog E1/T1/J1 line and the digital PCM system highway/H.100 bus.

Signed-off-by Herve Codina <[email protected]>

---
 drivers/mfd/Kconfig         |  16 +
 drivers/mfd/Makefile        |   1 +
 drivers/mfd/pef2256-regs.h  | 250 ++++++++++
 drivers/mfd/pef2256.c       | 950 ++++++++++++++++++++++++++++++++++++  

95% of this driver needs to be moved somewhere else.

What is a Framer? Perhaps sound/ is a good candidate?

Quoted 6 lines

 include/linux/mfd/pef2256.h |  52 ++
 5 files changed, 1269 insertions(+)
 create mode 100644 drivers/mfd/pef2256-regs.h
 create mode 100644 drivers/mfd/pef2256.c
 create mode 100644 include/linux/mfd/pef2256.h  

[...]

drivers/mfd/pef2256.c line 728

Quoted 13 lines

drivers/mfd/pef2256.c new file +13 -0

+static int pef2256_add_audio_devices(struct pef2256 *pef2256)
+{
+	const char *compatible = "lantiq,pef2256-codec";
+	struct mfd_cell *audio_devs;
+	struct device_node *np;
+	unsigned int count = 0;
+	unsigned int i;
+	int ret;
+
+	for_each_available_child_of_node(pef2256->dev->of_node, np) {
+		if (of_device_is_compatible(np, compatible))
+			count++;
+	}  

Converting Device Tree nodes into MFD cells to register with the Platform Device API is not a reasonable use-case of MFD.

Have the CODEC driver match on "lantiq,pef2256-codec" and let it instantiate itself.

Your first version using of_platform_populate() was closer to the mark.

Quoted 10 lines

drivers/mfd/pef2256.c new file +9 -0

+	if (!count)
+		return 0;
+
+	audio_devs = kcalloc(count, sizeof(*audio_devs), GFP_KERNEL);
+	if (!audio_devs)
+		return -ENOMEM;
+
+	for (i = 0; i < count; i++) {
+		(audio_devs + i)->name = "lantiq-pef2256-codec";

drivers/mfd/pef2256.c line 746

Quoted 8 lines

drivers/mfd/pef2256.c new file +8 -0

+		(audio_devs + i)->of_compatible = compatible;
+		(audio_devs + i)->id = i;
+	}
+
+	ret = mfd_add_devices(pef2256->dev, 0, audio_devs, count, NULL, 0, NULL);
+	kfree(audio_devs);
+	return ret;
+}