Add the Lantiq PEF2256 audio support

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

---
 .../bindings/mfd/lantiq,pef2256.yaml          | 267 ++++++++++++++++++
 1 file changed, 267 insertions(+)

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         |   17 +
 drivers/mfd/Makefile        |    1 +
 drivers/mfd/pef2256.c       | 1355 +++++++++++++++++++++++++++++++++++
 include/linux/mfd/pef2256.h |   28 +
 4 files changed, 1401 insertions(+)
 create mode 100644 drivers/mfd/pef2256.c
 create mode 100644 include/linux/mfd/pef2256.h

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

My goodness!

It's been a long time since I've seen anything quite like this.

My suggestion to you:

* Split this up into components that fit functional subsystems

* Run checkpatch.pl

* Remove all of the debug prints

* Move all of the defines out to a header file

* Be more verbose in your documentation / comments

* Consider using simple-mfd to probe child devices.

Quoted 10 lines

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

---
 drivers/mfd/Kconfig         |   17 +
 drivers/mfd/Makefile        |    1 +
 drivers/mfd/pef2256.c       | 1355 +++++++++++++++++++++++++++++++++++
 include/linux/mfd/pef2256.h |   28 +
 4 files changed, 1401 insertions(+)
 create mode 100644 drivers/mfd/pef2256.c
 create mode 100644 include/linux/mfd/pef2256.h  

--
Lee Jones [李琼斯]

Hi Lee,

Quoted 9 lines · On Thu, 30 Mar 2023 17:05:10 +0100 Lee Jones <[email protected]> wrote:

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

My goodness!

It's been a long time since I've seen anything quite like this.

Yes, old things but working on recent kernel.

Quoted 1 line

Quoted 3 lines

My suggestion to you:

* Split this up into components that fit functional subsystems

It is done. The audio part is present in ASoC subsystem (path 5 in this series). pinctrl function is implemented in this driver and, as I don't want to share registers, I would prefer to keep this function inside this driver.

Also, I sent a RFC related to HDLC and PHY. In this RFC, the pef2256 is considered as a PHY and handled in the PHY subsystem.
https://lore.kernel.org/linux-kernel/[email protected]/

Quoted 1 line

* Run checkpatch.pl

I did.

Quoted 2 lines

* Remove all of the debug prints

I can do that in the next iteration if really needed.

Quoted 1 line

* Move all of the defines out to a header file

These defines are related to registers. As I don't want to share these registers, is it really necessary to use a header file for them ?

Quoted 1 line

* Be more verbose in your documentation / comments

I can improve the API documentation present in include/mfd/pef2256.h. Do you thing that is necessary ? Only a few devices will use this API.

Quoted 18 lines · Herve Codina

Hi Lee,

Quoted 9 lines · On Thu, 30 Mar 2023 17:05:10 +0100 Lee Jones <[email protected]> wrote:

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

My goodness!

It's been a long time since I've seen anything quite like this.

Yes, old things but working on recent kernel.

Quoted 1 line

Quoted 8 lines

Quoted 3 lines

My suggestion to you:

* Split this up into components that fit functional subsystems

It is done. The audio part is present in ASoC subsystem (path 5 in this series). pinctrl function is implemented in this driver and, as I don't want to share registers, I would prefer to keep this function inside this driver.

The amount of defines and huge functions like pef2256_setup_e1() contradict it.

Even the pef2256_setup_e1() really does not follow Linux coding style - you know the size requirement, right?

pef2256_get_groups_count, struct pinmux_ops and others - this is pinctrl, not MFD! They cannot be in MFD driver.

Quoted 6 lines

Also, I sent a RFC related to HDLC and PHY. In this RFC, the pef2256 is considered as a PHY and handled in the PHY subsystem.
https://lore.kernel.org/linux-kernel/[email protected]/

Quoted 3 lines

Quoted 1 line

* Run checkpatch.pl

I did.

There are tons of weird indentation,e.g.:
+#define PEF2256_2X_PC_XPC_XLT (0x8 << 0)
^^^^ there is only one space after #define

... and other style issues:

CHECK: Please don't use multiple blank lines
CHECK: spaces preferred around that '+' (ctx:VxV)
CHECK: Alignment should match open parenthesis
CHECK: Macro argument reuse '_groups' - possible side-effects?
CHECK: usleep_range is preferred over udelay; see Documentation/timers/timers-howto.rst
CHECK: spaces preferred around that '/' (ctx:VxV)

Quoted 12 lines

Quoted 4 lines

* Remove all of the debug prints

I can do that in the next iteration if really needed.

Quoted 1 line

* Move all of the defines out to a header file

These defines are related to registers. As I don't want to share these registers, is it really necessary to use a header file for them ?

Quoted 5 lines

Quoted 1 line

* Be more verbose in your documentation / comments

I can improve the API documentation present in include/mfd/pef2256.h. Do you thing that is necessary ? Only a few devices will use this API.

Krzysztof

Hi Krzysztof, Lee

Quoted 28 lines · On Fri, 31 Mar 2023 11:13:30 +0200 Krzysztof Kozlowski…

Quoted 18 lines · Herve Codina

Hi Lee,

Quoted 9 lines · On Thu, 30 Mar 2023 17:05:10 +0100 Lee Jones <[email protected]> wrote:

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

My goodness!

It's been a long time since I've seen anything quite like this.

Yes, old things but working on recent kernel.

Quoted 1 line

Quoted 8 lines

Quoted 3 lines

My suggestion to you:

* Split this up into components that fit functional subsystems

It is done. The audio part is present in ASoC subsystem (path 5 in this series). pinctrl function is implemented in this driver and, as I don't want to share registers, I would prefer to keep this function inside this driver.

Quoted 5 lines

The amount of defines and huge functions like pef2256_setup_e1() contradict it.

Even the pef2256_setup_e1() really does not follow Linux coding style - you know the size requirement, right?

I know that pef2256_setup_e1() is quite big and I will look at a way to split it in a consistent way.

Quoted 1 line

Quoted 2 lines

pef2256_get_groups_count, struct pinmux_ops and others - this is pinctrl, not MFD! They cannot be in MFD driver.

Maybe the issue is that MFD was not a good choice.
The "function" provided are not independent of each other.
The "main" driver (pef2256.c) needs to do the setup and handle the interrupt.

The "function" provided are some glues in order to be used in some sub-systems such as audio. Indeed, ASoC needs a codec DAI to be connected to a CPU DAI.

These "functions" need to be started (ie probe()) after the pef2256 setup was done. So a start (probe()) order relationship is needed.

If a MFD driver needs independent children to handle independent functions, the pef2256 does not fit well as a MFD driver.

I switched from misc to MFD just to handle child DT nodes instead of having phandles. Using child DT nodes instead of phandles is really a good thing and need to be kept.

The switch to MFD was probably not the best thing to do.

What do you think if I switched back the pef2256 "main" driver (pef2256.c) to misc ?

Quoted 8 lines

Quoted 8 lines

Also, I sent a RFC related to HDLC and PHY. In this RFC, the pef2256 is considered as a PHY and handled in the PHY subsystem.
https://lore.kernel.org/linux-kernel/[email protected]/

Quoted 7 lines

Quoted 3 lines

Quoted 1 line

* Run checkpatch.pl

I did.

There are tons of weird indentation,e.g.:
+#define PEF2256_2X_PC_XPC_XLT (0x8 << 0)
^^^^ there is only one space after #define

I ran checkpatch.pl, not checkpatch.pl --strict.

The spaces related the #define can be seen on many other drivers.

#define FOO_REG_BAR 0x10
#define FOO_REG_BAR_BIT0 BIT(0)
#define FOO_REG_BAR_BIT4 BIT(4)

The first line is the register offset and the other lines (indented) are the bits description related to this register.

Quoted 43 lines · Herve Codina

Hi Krzysztof, Lee

Quoted 28 lines · On Fri, 31 Mar 2023 11:13:30 +0200 Krzysztof Kozlowski…

Quoted 18 lines · Herve Codina

Hi Lee,

Quoted 9 lines · On Thu, 30 Mar 2023 17:05:10 +0100 Lee Jones <[email protected]> wrote:

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

My goodness!

It's been a long time since I've seen anything quite like this.

Yes, old things but working on recent kernel.

Quoted 1 line

Quoted 8 lines

Quoted 3 lines

My suggestion to you:

* Split this up into components that fit functional subsystems

It is done. The audio part is present in ASoC subsystem (path 5 in this series). pinctrl function is implemented in this driver and, as I don't want to share registers, I would prefer to keep this function inside this driver.

Quoted 5 lines

The amount of defines and huge functions like pef2256_setup_e1() contradict it.

Even the pef2256_setup_e1() really does not follow Linux coding style - you know the size requirement, right?

I know that pef2256_setup_e1() is quite big and I will look at a way to split it in a consistent way.

Quoted 1 line

Quoted 6 lines

Quoted 2 lines

pef2256_get_groups_count, struct pinmux_ops and others - this is pinctrl, not MFD! They cannot be in MFD driver.

Maybe the issue is that MFD was not a good choice.
The "function" provided are not independent of each other.
The "main" driver (pef2256.c) needs to do the setup and handle the interrupt.

Just like all PMICs...

Quoted 2 lines

The "function" provided are some glues in order to be used in some sub-systems such as audio. Indeed, ASoC needs a codec DAI to be connected to a CPU DAI.

Just like in all other cases...

Quoted 2 lines

These "functions" need to be started (ie probe()) after the pef2256 setup was done. So a start (probe()) order relationship is needed.

Just like in all other cases, so I really do not see here anything special.

Quoted 3 lines

If a MFD driver needs independent children to handle independent functions, the pef2256 does not fit well as a MFD driver.

Why? So far everything is exactly the same.

Quoted 4 lines

I switched from misc to MFD just to handle child DT nodes instead of having phandles. Using child DT nodes instead of phandles is really a good thing and need to be kept.

Your DT bindings and nodes are not related to driver design. It does not matter for Devicetree if you put it to misc or MFD.

It does not matter for driver whether you call it in Devicetree foo or bar.

Quoted 1 line

The switch to MFD was probably not the best thing to do.

Maybe, but your existing arguments are not really related...

Quoted 3 lines

What do you think if I switched back the pef2256 "main" driver (pef2256.c) to misc ?

Why? What would it solve? You want to stuff pinctrl driver to misc, to avoid Lee's review? No. Pinctrl goes to pinctrl. Not to misc. Not to MFD.

Quoted 19 lines

Quoted 9 lines

Quoted 9 lines

Also, I sent a RFC related to HDLC and PHY. In this RFC, the pef2256 is considered as a PHY and handled in the PHY subsystem.
https://lore.kernel.org/linux-kernel/[email protected]/

Quoted 7 lines

Quoted 3 lines

Quoted 1 line

* Run checkpatch.pl

I did.

There are tons of weird indentation,e.g.:
+#define PEF2256_2X_PC_XPC_XLT (0x8 << 0)
^^^^ there is only one space after #define

I ran checkpatch.pl, not checkpatch.pl --strict.

Quoted 8 lines

The spaces related the #define can be seen on many other drivers.

#define FOO_REG_BAR 0x10
#define FOO_REG_BAR_BIT0 BIT(0)
#define FOO_REG_BAR_BIT4 BIT(4)

The first line is the register offset and the other lines (indented) are the bits description related to this register.

I don't think we have such convention in general and argument that some drivers do it in some subsystems is never a good argument. If they also misspell things or use Hungarian notation, shall we do the same?

Although maybe it is fine for Lee. I find it unreadable.

git grep '#define \+[A-Z]' | wc -l
73889
git grep '#define [A-Z]' | wc -l
3996054

In MFD there is only one driver doing this.

Most of other cases are net and gpu.

Best regards,
Krzysztof

Quoted 81 lines · On Fri, 31 Mar 2023 15:44:19 +0200 Krzysztof Kozlowski…

Quoted 43 lines · Herve Codina

Hi Krzysztof, Lee

Quoted 28 lines · On Fri, 31 Mar 2023 11:13:30 +0200 Krzysztof Kozlowski…

Quoted 18 lines · Herve Codina

Hi Lee,

Quoted 9 lines · On Thu, 30 Mar 2023 17:05:10 +0100 Lee Jones <[email protected]> wrote:

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

My goodness!

It's been a long time since I've seen anything quite like this.

Yes, old things but working on recent kernel.

Quoted 1 line

Quoted 8 lines

Quoted 3 lines

My suggestion to you:

* Split this up into components that fit functional subsystems

It is done. The audio part is present in ASoC subsystem (path 5 in this series). pinctrl function is implemented in this driver and, as I don't want to share registers, I would prefer to keep this function inside this driver.

Quoted 5 lines

The amount of defines and huge functions like pef2256_setup_e1() contradict it.

Even the pef2256_setup_e1() really does not follow Linux coding style - you know the size requirement, right?

I know that pef2256_setup_e1() is quite big and I will look at a way to split it in a consistent way.

Quoted 1 line

Quoted 6 lines

Quoted 2 lines

pef2256_get_groups_count, struct pinmux_ops and others - this is pinctrl, not MFD! They cannot be in MFD driver.

Maybe the issue is that MFD was not a good choice.
The "function" provided are not independent of each other.
The "main" driver (pef2256.c) needs to do the setup and handle the interrupt.

Just like all PMICs...

Quoted 2 lines

The "function" provided are some glues in order to be used in some sub-systems such as audio. Indeed, ASoC needs a codec DAI to be connected to a CPU DAI.

Just like in all other cases...

Quoted 2 lines

These "functions" need to be started (ie probe()) after the pef2256 setup was done. So a start (probe()) order relationship is needed.

Just like in all other cases, so I really do not see here anything special.

Quoted 3 lines

If a MFD driver needs independent children to handle independent functions, the pef2256 does not fit well as a MFD driver.

Why? So far everything is exactly the same.

Quoted 4 lines

I switched from misc to MFD just to handle child DT nodes instead of having phandles. Using child DT nodes instead of phandles is really a good thing and need to be kept.

Your DT bindings and nodes are not related to driver design. It does not matter for Devicetree if you put it to misc or MFD.

It does not matter for driver whether you call it in Devicetree foo or bar.

Quoted 1 line

The switch to MFD was probably not the best thing to do.

Quoted 8 lines

Maybe, but your existing arguments are not really related...

Quoted 3 lines

What do you think if I switched back the pef2256 "main" driver (pef2256.c) to misc ?

Why? What would it solve? You want to stuff pinctrl driver to misc, to avoid Lee's review? No. Pinctrl goes to pinctrl. Not to misc. Not to MFD.

Just to be clear on that point, I do not want to avoid any reviews and Lee's reviews are fully welcome.

I just misunderstood. I thought the issue was that the pef2256 MFD driver has an internal setup() and that the 'function' provided were not independent (they share the pef2256 API).
My first feeling was that it was expected a kind of 'empty' driver for the pef2256 MFD driver. The pef2256 driver cannot be empty. That's why I talk about misc.

Quoted 93 lines · Herve Codina

Quoted 81 lines · On Fri, 31 Mar 2023 15:44:19 +0200 Krzysztof Kozlowski…

Quoted 43 lines · Herve Codina

Hi Krzysztof, Lee

Quoted 28 lines · On Fri, 31 Mar 2023 11:13:30 +0200 Krzysztof Kozlowski…

Quoted 18 lines · Herve Codina

Hi Lee,

Quoted 9 lines · On Thu, 30 Mar 2023 17:05:10 +0100 Lee Jones <[email protected]> wrote:

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

My goodness!

It's been a long time since I've seen anything quite like this.

Yes, old things but working on recent kernel.

Quoted 1 line

Quoted 8 lines

Quoted 3 lines

My suggestion to you:

* Split this up into components that fit functional subsystems

It is done. The audio part is present in ASoC subsystem (path 5 in this series). pinctrl function is implemented in this driver and, as I don't want to share registers, I would prefer to keep this function inside this driver.

Quoted 5 lines

The amount of defines and huge functions like pef2256_setup_e1() contradict it.

Even the pef2256_setup_e1() really does not follow Linux coding style - you know the size requirement, right?

I know that pef2256_setup_e1() is quite big and I will look at a way to split it in a consistent way.

Quoted 1 line

Quoted 6 lines

Quoted 2 lines

pef2256_get_groups_count, struct pinmux_ops and others - this is pinctrl, not MFD! They cannot be in MFD driver.

Maybe the issue is that MFD was not a good choice.
The "function" provided are not independent of each other.
The "main" driver (pef2256.c) needs to do the setup and handle the interrupt.

Just like all PMICs...

Quoted 2 lines

The "function" provided are some glues in order to be used in some sub-systems such as audio. Indeed, ASoC needs a codec DAI to be connected to a CPU DAI.

Just like in all other cases...

Quoted 2 lines

These "functions" need to be started (ie probe()) after the pef2256 setup was done. So a start (probe()) order relationship is needed.

Just like in all other cases, so I really do not see here anything special.

Quoted 3 lines

If a MFD driver needs independent children to handle independent functions, the pef2256 does not fit well as a MFD driver.

Why? So far everything is exactly the same.

Quoted 4 lines

I switched from misc to MFD just to handle child DT nodes instead of having phandles. Using child DT nodes instead of phandles is really a good thing and need to be kept.

Your DT bindings and nodes are not related to driver design. It does not matter for Devicetree if you put it to misc or MFD.

It does not matter for driver whether you call it in Devicetree foo or bar.

Quoted 1 line

The switch to MFD was probably not the best thing to do.

Quoted 8 lines

Maybe, but your existing arguments are not really related...

Quoted 3 lines

What do you think if I switched back the pef2256 "main" driver (pef2256.c) to misc ?

Why? What would it solve? You want to stuff pinctrl driver to misc, to avoid Lee's review? No. Pinctrl goes to pinctrl. Not to misc. Not to MFD.

Quoted 8 lines

Just to be clear on that point, I do not want to avoid any reviews and Lee's reviews are fully welcome.

I just misunderstood. I thought the issue was that the pef2256 MFD driver has an internal setup() and that the 'function' provided were not independent (they share the pef2256 API).
My first feeling was that it was expected a kind of 'empty' driver for the pef2256 MFD driver. The pef2256 driver cannot be empty. That's why I talk about misc.

It can be empty. That's what simple-mfd is for. :)

I suggest that none of this (besides the child registration, which is achieved by a simple OF API call in this case) has anything to do with MFD. We are not requesting and initialising shared resources and we are not using the MFD API to register children. The pin control functionality clearly needs moving to Pinctrl and the rest, if you cannot find a suitable home for it *may be* suitable for Misc.

Quoted 7 lines

I suggest that none of this (besides the child registration, which is achieved by a simple OF API call in this case) has anything to do with MFD. We are not requesting and initialising shared resources and we are not using the MFD API to register children. The pin control functionality clearly needs moving to Pinctrl and the rest, if you cannot find a suitable home for it *may be* suitable for Misc.

I am confused and I am not really sure to understand where to put my driver.

The core pef2256.c needs to:
1) setup the pef2256
2) add the children

To add the children it calls devm_of_platform_populate() to add the audio parts as several audio children can be available with the same compatible string.

I plan to move the pinctrl part to the pinctrl subsystem. With this done, the core pef2256.c will probably add the children using:
- a mfd_cell for the pinctrl part
- devm_of_platform_populate() for the audio children

The setup (E1 lines and TDM configuration) still needs to be done by the core pef2256.c. Moving this part only to Misc will break the hierarchy. The audio children depends on the core pef2256.c as this one do the setup. Having in the audio children and the part that do the setup in same hierarchy level is not correct. Audio children should be children of the part that do the setup.

So, the structure I have in mind:
- pef2256.c (MFD)
implement and do the setup at probe()
Add the children at probe():
- pef2256-pinctrl (pinctrl) added using mfd_add_devices()
- pef2256-codec (ASoC codec) added using devm_of_platform_populate()

Lee, with this in mind, can the core pef2256.c be a MFD driver ?

Quoted 37 lines · Herve Codina

Quoted 7 lines

I suggest that none of this (besides the child registration, which is achieved by a simple OF API call in this case) has anything to do with MFD. We are not requesting and initialising shared resources and we are not using the MFD API to register children. The pin control functionality clearly needs moving to Pinctrl and the rest, if you cannot find a suitable home for it *may be* suitable for Misc.

I am confused and I am not really sure to understand where to put my driver.

The core pef2256.c needs to:
1) setup the pef2256
2) add the children

To add the children it calls devm_of_platform_populate() to add the audio parts as several audio children can be available with the same compatible string.

I plan to move the pinctrl part to the pinctrl subsystem. With this done, the core pef2256.c will probably add the children using:
- a mfd_cell for the pinctrl part
- devm_of_platform_populate() for the audio children

The setup (E1 lines and TDM configuration) still needs to be done by the core pef2256.c. Moving this part only to Misc will break the hierarchy. The audio children depends on the core pef2256.c as this one do the setup. Having in the audio children and the part that do the setup in same hierarchy level is not correct. Audio children should be children of the part that do the setup.

So, the structure I have in mind:
- pef2256.c (MFD)
implement and do the setup at probe()
Add the children at probe():
- pef2256-pinctrl (pinctrl) added using mfd_add_devices()
- pef2256-codec (ASoC codec) added using devm_of_platform_populate()

Quoted 1 line

Lee, with this in mind, can the core pef2256.c be a MFD driver ?

You do not use MFD here, so why do you want to keep it in MFD? If you disagree, please tell me where is the MFD code in your patch?

Best regards,
Krzysztof

Quoted 11 lines

Quoted 8 lines

So, the structure I have in mind:
- pef2256.c (MFD)
implement and do the setup at probe()
Add the children at probe():
- pef2256-pinctrl (pinctrl) added using mfd_add_devices()
- pef2256-codec (ASoC codec) added using devm_of_platform_populate()

Lee, with this in mind, can the core pef2256.c be a MFD driver ?

You do not use MFD here, so why do you want to keep it in MFD? If you disagree, please tell me where is the MFD code in your patch?

I don't want to absolutely use MFD.
I just want to put my driver somewhere and I don't know the right location between MFD and Misc.

Basically, the driver needs to do (little simplified and error path removed):

  static const struct mfd_cell pef2256_devs[] = {
  	{ .name = "lantiq-pef2256-pinctrl", },
  };

static int pef2256_probe(struct platform_device *pdev)

  {
	struct pef2256 *pef2256;
	void __iomem *iomem;
	int ret;
	int irq;

	pef2256 = devm_kzalloc(&pdev->dev, sizeof(*pef2256), GFP_KERNEL);
	if (!pef2256)
		return -ENOMEM;

	pef2256->dev = &pdev->dev;

	iomem = devm_platform_ioremap_resource(pdev, 0);

	pef2256->regmap = devm_regmap_init_mmio(&pdev->dev, iomem,
						&pef2256_regmap_config);

	pef2256->mclk = devm_clk_get_enabled(&pdev->dev, "mclk");
	pef2256->sclkr = devm_clk_get_enabled(&pdev->dev, "sclkr");
	pef2256->sclkx = devm_clk_get_enabled(&pdev->dev, "sclkx");

	pef2256->reset_gpio = devm_gpiod_get_optional(&pdev->dev, "reset", GPIOD_OUT_LOW);
	if (pef2256->reset_gpio) {
		gpiod_set_value_cansleep(pef2256->reset_gpio, 1);
		udelay(10);
		gpiod_set_value_cansleep(pef2256->reset_gpio, 0);
		udelay(10);
	}

	pef2556_of_parse(pef2256, np);

	irq = platform_get_irq(pdev, 0);
	ret = devm_request_irq(pef2256->dev, irq, pef2256_irq_handler, 0, "pef2256", pef2256);

	platform_set_drvdata(pdev, pef2256);

	mfd_add_devices(pef2256->dev, PLATFORM_DEVID_NONE, pef2256_devs,
	      		ARRAY_SIZE(pef2256_devs), NULL, 0, NULL);

Quoted 57 lines · Herve Codina

Quoted 11 lines

Quoted 8 lines

So, the structure I have in mind:
- pef2256.c (MFD)
implement and do the setup at probe()
Add the children at probe():
- pef2256-pinctrl (pinctrl) added using mfd_add_devices()
- pef2256-codec (ASoC codec) added using devm_of_platform_populate()

Lee, with this in mind, can the core pef2256.c be a MFD driver ?

You do not use MFD here, so why do you want to keep it in MFD? If you disagree, please tell me where is the MFD code in your patch?

I don't want to absolutely use MFD.
I just want to put my driver somewhere and I don't know the right location between MFD and Misc.

Basically, the driver needs to do (little simplified and error path removed):

  static const struct mfd_cell pef2256_devs[] = {
  	{ .name = "lantiq-pef2256-pinctrl", },
  };

static int pef2256_probe(struct platform_device *pdev)

  {
	struct pef2256 *pef2256;
	void __iomem *iomem;
	int ret;
	int irq;

	pef2256 = devm_kzalloc(&pdev->dev, sizeof(*pef2256), GFP_KERNEL);
	if (!pef2256)
		return -ENOMEM;

	pef2256->dev = &pdev->dev;

	iomem = devm_platform_ioremap_resource(pdev, 0);

	pef2256->regmap = devm_regmap_init_mmio(&pdev->dev, iomem,
						&pef2256_regmap_config);

	pef2256->mclk = devm_clk_get_enabled(&pdev->dev, "mclk");
	pef2256->sclkr = devm_clk_get_enabled(&pdev->dev, "sclkr");
	pef2256->sclkx = devm_clk_get_enabled(&pdev->dev, "sclkx");

	pef2256->reset_gpio = devm_gpiod_get_optional(&pdev->dev, "reset", GPIOD_OUT_LOW);
	if (pef2256->reset_gpio) {
		gpiod_set_value_cansleep(pef2256->reset_gpio, 1);
		udelay(10);
		gpiod_set_value_cansleep(pef2256->reset_gpio, 0);
		udelay(10);
	}

	pef2556_of_parse(pef2256, np);

	irq = platform_get_irq(pdev, 0);
	ret = devm_request_irq(pef2256->dev, irq, pef2256_irq_handler, 0, "pef2256", pef2256);

Quoted 4 lines

	platform_set_drvdata(pdev, pef2256);

	mfd_add_devices(pef2256->dev, PLATFORM_DEVID_NONE, pef2256_devs,
	      		ARRAY_SIZE(pef2256_devs), NULL, 0, NULL);

Wait, now you use MFD framework, so the driver is suitable for MFD. Before there was nothing like that in your code.

Quoted 64 lines · Krzysztof Kozlowski

Quoted 61 lines · Herve Codina

Quoted 11 lines

Quoted 8 lines

So, the structure I have in mind:
- pef2256.c (MFD)
implement and do the setup at probe()
Add the children at probe():
- pef2256-pinctrl (pinctrl) added using mfd_add_devices()
- pef2256-codec (ASoC codec) added using devm_of_platform_populate()

Lee, with this in mind, can the core pef2256.c be a MFD driver ?

You do not use MFD here, so why do you want to keep it in MFD? If you disagree, please tell me where is the MFD code in your patch?

I don't want to absolutely use MFD.
I just want to put my driver somewhere and I don't know the right location between MFD and Misc.

Basically, the driver needs to do (little simplified and error path removed):

  static const struct mfd_cell pef2256_devs[] = {
  	{ .name = "lantiq-pef2256-pinctrl", },
  };

static int pef2256_probe(struct platform_device *pdev)

  {
	struct pef2256 *pef2256;
	void __iomem *iomem;
	int ret;
	int irq;

	pef2256 = devm_kzalloc(&pdev->dev, sizeof(*pef2256), GFP_KERNEL);
	if (!pef2256)
		return -ENOMEM;

	pef2256->dev = &pdev->dev;

	iomem = devm_platform_ioremap_resource(pdev, 0);

	pef2256->regmap = devm_regmap_init_mmio(&pdev->dev, iomem,
						&pef2256_regmap_config);

	pef2256->mclk = devm_clk_get_enabled(&pdev->dev, "mclk");
	pef2256->sclkr = devm_clk_get_enabled(&pdev->dev, "sclkr");
	pef2256->sclkx = devm_clk_get_enabled(&pdev->dev, "sclkx");

	pef2256->reset_gpio = devm_gpiod_get_optional(&pdev->dev, "reset", GPIOD_OUT_LOW);
	if (pef2256->reset_gpio) {
		gpiod_set_value_cansleep(pef2256->reset_gpio, 1);
		udelay(10);
		gpiod_set_value_cansleep(pef2256->reset_gpio, 0);
		udelay(10);
	}

	pef2556_of_parse(pef2256, np);

	irq = platform_get_irq(pdev, 0);
	ret = devm_request_irq(pef2256->dev, irq, pef2256_irq_handler, 0, "pef2256", pef2256);

	platform_set_drvdata(pdev, pef2256);

	mfd_add_devices(pef2256->dev, PLATFORM_DEVID_NONE, pef2256_devs,
	      		ARRAY_SIZE(pef2256_devs), NULL, 0, NULL);  

Quoted 2 lines

Wait, now you use MFD framework, so the driver is suitable for MFD. Before there was nothing like that in your code.

Agree, the above is suitable for MFD, since it does all the things I said your last one did not. You *can* also use of_platform_populate() here, since you are *also* requesting and initialising shared resources. You cannot do *both* however.

Hi Lee,

Quoted 66 lines · On Wed, 5 Apr 2023 14:54:50 +0100 Lee Jones <[email protected]> wrote:

Quoted 64 lines · Krzysztof Kozlowski

Quoted 61 lines · Herve Codina

Quoted 11 lines

Quoted 8 lines

So, the structure I have in mind:
- pef2256.c (MFD)
implement and do the setup at probe()
Add the children at probe():
- pef2256-pinctrl (pinctrl) added using mfd_add_devices()
- pef2256-codec (ASoC codec) added using devm_of_platform_populate()

Lee, with this in mind, can the core pef2256.c be a MFD driver ?

You do not use MFD here, so why do you want to keep it in MFD? If you disagree, please tell me where is the MFD code in your patch?

I don't want to absolutely use MFD.
I just want to put my driver somewhere and I don't know the right location between MFD and Misc.

Basically, the driver needs to do (little simplified and error path removed):

  static const struct mfd_cell pef2256_devs[] = {
  	{ .name = "lantiq-pef2256-pinctrl", },
  };

static int pef2256_probe(struct platform_device *pdev)

  {
	struct pef2256 *pef2256;
	void __iomem *iomem;
	int ret;
	int irq;

	pef2256 = devm_kzalloc(&pdev->dev, sizeof(*pef2256), GFP_KERNEL);
	if (!pef2256)
		return -ENOMEM;

	pef2256->dev = &pdev->dev;

	iomem = devm_platform_ioremap_resource(pdev, 0);

	pef2256->regmap = devm_regmap_init_mmio(&pdev->dev, iomem,
						&pef2256_regmap_config);

	pef2256->mclk = devm_clk_get_enabled(&pdev->dev, "mclk");
	pef2256->sclkr = devm_clk_get_enabled(&pdev->dev, "sclkr");
	pef2256->sclkx = devm_clk_get_enabled(&pdev->dev, "sclkx");

	pef2256->reset_gpio = devm_gpiod_get_optional(&pdev->dev, "reset", GPIOD_OUT_LOW);
	if (pef2256->reset_gpio) {
		gpiod_set_value_cansleep(pef2256->reset_gpio, 1);
		udelay(10);
		gpiod_set_value_cansleep(pef2256->reset_gpio, 0);
		udelay(10);
	}

	pef2556_of_parse(pef2256, np);

	irq = platform_get_irq(pdev, 0);
	ret = devm_request_irq(pef2256->dev, irq, pef2256_irq_handler, 0, "pef2256", pef2256);

	platform_set_drvdata(pdev, pef2256);

	mfd_add_devices(pef2256->dev, PLATFORM_DEVID_NONE, pef2256_devs,
	      		ARRAY_SIZE(pef2256_devs), NULL, 0, NULL);    

Quoted 8 lines

Quoted 2 lines

Wait, now you use MFD framework, so the driver is suitable for MFD. Before there was nothing like that in your code.

Agree, the above is suitable for MFD, since it does all the things I said your last one did not. You *can* also use of_platform_populate() here, since you are *also* requesting and initialising shared resources. You cannot do *both* however.

Thanks for having confirmed that this driver can be a MFD driver.

Related to of_platform_populate() / mfd_add_devices(), I wanted to use both because:
- the pinctrl part does not have (and does not need to have) a specific node with a specific compatible property. In order to instantiate the related driver mfd_add_devices() is the only way I know.
- the audio component nodes have a compatible string and several components (ie several nodes) can be present. of_platform_populate() call seems the simple way to instantiate them.

Is there a way to use mfd_add_devices() in this case without the need to count the audio component nodes in order to allocate as much mfd_cell as nodes having a matching compatible property ? Is there an other API to do it ?

Hi Lee,

Quoted 90 lines · On Wed, 5 Apr 2023 18:00:23 +0200 Herve Codina <[email protected]> wrote:

Hi Lee,

Quoted 66 lines · On Wed, 5 Apr 2023 14:54:50 +0100 Lee Jones <[email protected]> wrote:

Quoted 64 lines · Krzysztof Kozlowski

Quoted 61 lines · Herve Codina

Quoted 11 lines

Quoted 8 lines

So, the structure I have in mind:
- pef2256.c (MFD)
implement and do the setup at probe()
Add the children at probe():
- pef2256-pinctrl (pinctrl) added using mfd_add_devices()
- pef2256-codec (ASoC codec) added using devm_of_platform_populate()

Lee, with this in mind, can the core pef2256.c be a MFD driver ?

You do not use MFD here, so why do you want to keep it in MFD? If you disagree, please tell me where is the MFD code in your patch?

I don't want to absolutely use MFD.
I just want to put my driver somewhere and I don't know the right location between MFD and Misc.

Basically, the driver needs to do (little simplified and error path removed):

  static const struct mfd_cell pef2256_devs[] = {
  	{ .name = "lantiq-pef2256-pinctrl", },
  };

static int pef2256_probe(struct platform_device *pdev)

  {
	struct pef2256 *pef2256;
	void __iomem *iomem;
	int ret;
	int irq;

	pef2256 = devm_kzalloc(&pdev->dev, sizeof(*pef2256), GFP_KERNEL);
	if (!pef2256)
		return -ENOMEM;

	pef2256->dev = &pdev->dev;

	iomem = devm_platform_ioremap_resource(pdev, 0);

	pef2256->regmap = devm_regmap_init_mmio(&pdev->dev, iomem,
						&pef2256_regmap_config);

	pef2256->mclk = devm_clk_get_enabled(&pdev->dev, "mclk");
	pef2256->sclkr = devm_clk_get_enabled(&pdev->dev, "sclkr");
	pef2256->sclkx = devm_clk_get_enabled(&pdev->dev, "sclkx");

	pef2256->reset_gpio = devm_gpiod_get_optional(&pdev->dev, "reset", GPIOD_OUT_LOW);
	if (pef2256->reset_gpio) {
		gpiod_set_value_cansleep(pef2256->reset_gpio, 1);
		udelay(10);
		gpiod_set_value_cansleep(pef2256->reset_gpio, 0);
		udelay(10);
	}

	pef2556_of_parse(pef2256, np);

	irq = platform_get_irq(pdev, 0);
	ret = devm_request_irq(pef2256->dev, irq, pef2256_irq_handler, 0, "pef2256", pef2256);

	platform_set_drvdata(pdev, pef2256);

	mfd_add_devices(pef2256->dev, PLATFORM_DEVID_NONE, pef2256_devs,
	      		ARRAY_SIZE(pef2256_devs), NULL, 0, NULL);

Quoted 8 lines

Quoted 2 lines

Wait, now you use MFD framework, so the driver is suitable for MFD. Before there was nothing like that in your code.

Agree, the above is suitable for MFD, since it does all the things I said your last one did not. You *can* also use of_platform_populate() here, since you are *also* requesting and initialising shared resources. You cannot do *both* however.

Thanks for having confirmed that this driver can be a MFD driver.

Related to of_platform_populate() / mfd_add_devices(), I wanted to use both because:
- the pinctrl part does not have (and does not need to have) a specific node with a specific compatible property. In order to instantiate the related driver mfd_add_devices() is the only way I know.
- the audio component nodes have a compatible string and several components (ie several nodes) can be present. of_platform_populate() call seems the simple way to instantiate them.

Quoted 5 lines

Is there a way to use mfd_add_devices() in this case without the need to count the audio component nodes in order to allocate as much mfd_cell as nodes having a matching compatible property ? Is there an other API to do it ?

I looked deeper for using mfd_add_devices() only and found an issue (related to my use-case).

mfd_add_devices() calls mfd_add_device() and in this function we have:
---- 8< ----

	if (IS_ENABLED(CONFIG_OF) && parent->of_node && cell->of_compatible) {
		for_each_child_of_node(parent->of_node, np) {
			if (of_device_is_compatible(np, cell->of_compatible)) {
				/* Ignore 'disabled' devices error free */
				if (!of_device_is_available(np)) {
					of_node_put(np);
					ret = 0;
					goto fail_alias;
				}

				ret = mfd_match_of_node_to_dev(pdev, np, cell);
				if (ret == -EAGAIN)
					continue;
				of_node_put(np);
				if (ret)
					goto fail_alias;

				break;
			}
		}

		if (!pdev->dev.of_node)
			pr_warn("%s: Failed to locate of_node [id: %d]\n",
				cell->name, platform_id);
	}

---- 8< ----

My issue is related to the loop.
The loop is aborted if the device is not available (typically 'Disabled' in the DT node).

My full pef2256 node can have several pef2256 audio subnodes with compatible set to "lantiq,pef2256-codec" and some of them can have 'status = "Disabled";' As soon as one device (with my expected compatible) is found 'Disabled', there is no way to look at other children.

Having 'continue' in case of device not available in the loop to look at the next child will change the behavior of mfd_add_device().
Do you think that looking at next child if we find a 'not available' child can be correct for mfd_add_device() ?
This probably will have impacts on other MFD drivers.

Quoted 142 lines · Herve Codina

Hi Lee,

Quoted 90 lines · On Wed, 5 Apr 2023 18:00:23 +0200 Herve Codina <[email protected]> wrote:

Hi Lee,

Quoted 66 lines · On Wed, 5 Apr 2023 14:54:50 +0100 Lee Jones <[email protected]> wrote:

Quoted 64 lines · Krzysztof Kozlowski

Quoted 61 lines · Herve Codina

Quoted 11 lines

Quoted 8 lines

So, the structure I have in mind:
- pef2256.c (MFD)
implement and do the setup at probe()
Add the children at probe():
- pef2256-pinctrl (pinctrl) added using mfd_add_devices()
- pef2256-codec (ASoC codec) added using devm_of_platform_populate()

Lee, with this in mind, can the core pef2256.c be a MFD driver ?

You do not use MFD here, so why do you want to keep it in MFD? If you disagree, please tell me where is the MFD code in your patch?

I don't want to absolutely use MFD.
I just want to put my driver somewhere and I don't know the right location between MFD and Misc.

Basically, the driver needs to do (little simplified and error path removed):

  static const struct mfd_cell pef2256_devs[] = {
  	{ .name = "lantiq-pef2256-pinctrl", },
  };

static int pef2256_probe(struct platform_device *pdev)

  {
	struct pef2256 *pef2256;
	void __iomem *iomem;
	int ret;
	int irq;

	pef2256 = devm_kzalloc(&pdev->dev, sizeof(*pef2256), GFP_KERNEL);
	if (!pef2256)
		return -ENOMEM;

	pef2256->dev = &pdev->dev;

	iomem = devm_platform_ioremap_resource(pdev, 0);

	pef2256->regmap = devm_regmap_init_mmio(&pdev->dev, iomem,
						&pef2256_regmap_config);

	pef2256->mclk = devm_clk_get_enabled(&pdev->dev, "mclk");
	pef2256->sclkr = devm_clk_get_enabled(&pdev->dev, "sclkr");
	pef2256->sclkx = devm_clk_get_enabled(&pdev->dev, "sclkx");

	pef2256->reset_gpio = devm_gpiod_get_optional(&pdev->dev, "reset", GPIOD_OUT_LOW);
	if (pef2256->reset_gpio) {
		gpiod_set_value_cansleep(pef2256->reset_gpio, 1);
		udelay(10);
		gpiod_set_value_cansleep(pef2256->reset_gpio, 0);
		udelay(10);
	}

	pef2556_of_parse(pef2256, np);

	irq = platform_get_irq(pdev, 0);
	ret = devm_request_irq(pef2256->dev, irq, pef2256_irq_handler, 0, "pef2256", pef2256);

	platform_set_drvdata(pdev, pef2256);

	mfd_add_devices(pef2256->dev, PLATFORM_DEVID_NONE, pef2256_devs,
	      		ARRAY_SIZE(pef2256_devs), NULL, 0, NULL);  

Quoted 8 lines

Quoted 2 lines

Wait, now you use MFD framework, so the driver is suitable for MFD. Before there was nothing like that in your code.

Agree, the above is suitable for MFD, since it does all the things I said your last one did not. You *can* also use of_platform_populate() here, since you are *also* requesting and initialising shared resources. You cannot do *both* however.

Thanks for having confirmed that this driver can be a MFD driver.

Related to of_platform_populate() / mfd_add_devices(), I wanted to use both because:
- the pinctrl part does not have (and does not need to have) a specific node with a specific compatible property. In order to instantiate the related driver mfd_add_devices() is the only way I know.
- the audio component nodes have a compatible string and several components (ie several nodes) can be present. of_platform_populate() call seems the simple way to instantiate them.

Quoted 5 lines

Is there a way to use mfd_add_devices() in this case without the need to count the audio component nodes in order to allocate as much mfd_cell as nodes having a matching compatible property ? Is there an other API to do it ?

I looked deeper for using mfd_add_devices() only and found an issue (related to my use-case).

mfd_add_devices() calls mfd_add_device() and in this function we have:
---- 8< ----

	if (IS_ENABLED(CONFIG_OF) && parent->of_node && cell->of_compatible) {
		for_each_child_of_node(parent->of_node, np) {
			if (of_device_is_compatible(np, cell->of_compatible)) {
				/* Ignore 'disabled' devices error free */
				if (!of_device_is_available(np)) {
					of_node_put(np);
					ret = 0;
					goto fail_alias;
				}

				ret = mfd_match_of_node_to_dev(pdev, np, cell);
				if (ret == -EAGAIN)
					continue;
				of_node_put(np);
				if (ret)
					goto fail_alias;

				break;
			}
		}

		if (!pdev->dev.of_node)
			pr_warn("%s: Failed to locate of_node [id: %d]\n",
				cell->name, platform_id);
	}

---- 8< ----

My issue is related to the loop.
The loop is aborted if the device is not available (typically 'Disabled' in the DT node).

My full pef2256 node can have several pef2256 audio subnodes with compatible set to "lantiq,pef2256-codec" and some of them can have 'status = "Disabled";' As soon as one device (with my expected compatible) is found 'Disabled', there is no way to look at other children.

Quoted 5 lines

Having 'continue' in case of device not available in the loop to look at the next child will change the behavior of mfd_add_device().
Do you think that looking at next child if we find a 'not available' child can be correct for mfd_add_device() ?
This probably will have impacts on other MFD drivers.

Looks like a bug. Encountering one disabled cell should not prevent the others from registering IMHO.

Quoted 142 lines · Herve Codina

Hi Lee,

Quoted 90 lines · On Wed, 5 Apr 2023 18:00:23 +0200 Herve Codina <[email protected]> wrote:

Hi Lee,

Quoted 66 lines · On Wed, 5 Apr 2023 14:54:50 +0100 Lee Jones <[email protected]> wrote:

Quoted 64 lines · Krzysztof Kozlowski

Quoted 61 lines · Herve Codina

Quoted 11 lines

Quoted 8 lines

So, the structure I have in mind:
- pef2256.c (MFD)
implement and do the setup at probe()
Add the children at probe():
- pef2256-pinctrl (pinctrl) added using mfd_add_devices()
- pef2256-codec (ASoC codec) added using devm_of_platform_populate()

Lee, with this in mind, can the core pef2256.c be a MFD driver ?

You do not use MFD here, so why do you want to keep it in MFD? If you disagree, please tell me where is the MFD code in your patch?

I don't want to absolutely use MFD.
I just want to put my driver somewhere and I don't know the right location between MFD and Misc.

Basically, the driver needs to do (little simplified and error path removed):

  static const struct mfd_cell pef2256_devs[] = {
  	{ .name = "lantiq-pef2256-pinctrl", },
  };

static int pef2256_probe(struct platform_device *pdev)

  {
	struct pef2256 *pef2256;
	void __iomem *iomem;
	int ret;
	int irq;

	pef2256 = devm_kzalloc(&pdev->dev, sizeof(*pef2256), GFP_KERNEL);
	if (!pef2256)
		return -ENOMEM;

	pef2256->dev = &pdev->dev;

	iomem = devm_platform_ioremap_resource(pdev, 0);

	pef2256->regmap = devm_regmap_init_mmio(&pdev->dev, iomem,
						&pef2256_regmap_config);

	pef2256->mclk = devm_clk_get_enabled(&pdev->dev, "mclk");
	pef2256->sclkr = devm_clk_get_enabled(&pdev->dev, "sclkr");
	pef2256->sclkx = devm_clk_get_enabled(&pdev->dev, "sclkx");

	pef2256->reset_gpio = devm_gpiod_get_optional(&pdev->dev, "reset", GPIOD_OUT_LOW);
	if (pef2256->reset_gpio) {
		gpiod_set_value_cansleep(pef2256->reset_gpio, 1);
		udelay(10);
		gpiod_set_value_cansleep(pef2256->reset_gpio, 0);
		udelay(10);
	}

	pef2556_of_parse(pef2256, np);

	irq = platform_get_irq(pdev, 0);
	ret = devm_request_irq(pef2256->dev, irq, pef2256_irq_handler, 0, "pef2256", pef2256);

	platform_set_drvdata(pdev, pef2256);

	mfd_add_devices(pef2256->dev, PLATFORM_DEVID_NONE, pef2256_devs,
	      		ARRAY_SIZE(pef2256_devs), NULL, 0, NULL);  

Quoted 8 lines

Quoted 2 lines

Wait, now you use MFD framework, so the driver is suitable for MFD. Before there was nothing like that in your code.

Agree, the above is suitable for MFD, since it does all the things I said your last one did not. You *can* also use of_platform_populate() here, since you are *also* requesting and initialising shared resources. You cannot do *both* however.

Thanks for having confirmed that this driver can be a MFD driver.

Related to of_platform_populate() / mfd_add_devices(), I wanted to use both because:
- the pinctrl part does not have (and does not need to have) a specific node with a specific compatible property. In order to instantiate the related driver mfd_add_devices() is the only way I know.
- the audio component nodes have a compatible string and several components (ie several nodes) can be present. of_platform_populate() call seems the simple way to instantiate them.

Quoted 5 lines

Is there a way to use mfd_add_devices() in this case without the need to count the audio component nodes in order to allocate as much mfd_cell as nodes having a matching compatible property ? Is there an other API to do it ?

I looked deeper for using mfd_add_devices() only and found an issue (related to my use-case).

mfd_add_devices() calls mfd_add_device() and in this function we have:
---- 8< ----

	if (IS_ENABLED(CONFIG_OF) && parent->of_node && cell->of_compatible) {
		for_each_child_of_node(parent->of_node, np) {
			if (of_device_is_compatible(np, cell->of_compatible)) {
				/* Ignore 'disabled' devices error free */
				if (!of_device_is_available(np)) {
					of_node_put(np);
					ret = 0;
					goto fail_alias;
				}

				ret = mfd_match_of_node_to_dev(pdev, np, cell);
				if (ret == -EAGAIN)
					continue;
				of_node_put(np);
				if (ret)
					goto fail_alias;

				break;
			}
		}

		if (!pdev->dev.of_node)
			pr_warn("%s: Failed to locate of_node [id: %d]\n",
				cell->name, platform_id);
	}

---- 8< ----

My issue is related to the loop.
The loop is aborted if the device is not available (typically 'Disabled' in the DT node).

My full pef2256 node can have several pef2256 audio subnodes with compatible set to "lantiq,pef2256-codec" and some of them can have 'status = "Disabled";' As soon as one device (with my expected compatible) is found 'Disabled', there is no way to look at other children.

Quoted 5 lines

Having 'continue' in case of device not available in the loop to look at the next child will change the behavior of mfd_add_device().
Do you think that looking at next child if we find a 'not available' child can be correct for mfd_add_device() ?
This probably will have impacts on other MFD drivers.

Looks like a bug. Encountering one disabled cell should not prevent the others from registering IMHO.

Hi Lee,

Quoted 66 lines · On Wed, 5 Apr 2023 14:54:50 +0100 Lee Jones <[email protected]> wrote:

Quoted 64 lines · Krzysztof Kozlowski

Quoted 61 lines · Herve Codina

Quoted 11 lines

Quoted 8 lines

So, the structure I have in mind:
- pef2256.c (MFD)
implement and do the setup at probe()
Add the children at probe():
- pef2256-pinctrl (pinctrl) added using mfd_add_devices()
- pef2256-codec (ASoC codec) added using devm_of_platform_populate()

Lee, with this in mind, can the core pef2256.c be a MFD driver ?

You do not use MFD here, so why do you want to keep it in MFD? If you disagree, please tell me where is the MFD code in your patch?

I don't want to absolutely use MFD.
I just want to put my driver somewhere and I don't know the right location between MFD and Misc.

Basically, the driver needs to do (little simplified and error path removed):

  static const struct mfd_cell pef2256_devs[] = {
  	{ .name = "lantiq-pef2256-pinctrl", },
  };

static int pef2256_probe(struct platform_device *pdev)

  {
	struct pef2256 *pef2256;
	void __iomem *iomem;
	int ret;
	int irq;

	pef2256 = devm_kzalloc(&pdev->dev, sizeof(*pef2256), GFP_KERNEL);
	if (!pef2256)
		return -ENOMEM;

	pef2256->dev = &pdev->dev;

	iomem = devm_platform_ioremap_resource(pdev, 0);

	pef2256->regmap = devm_regmap_init_mmio(&pdev->dev, iomem,
						&pef2256_regmap_config);

	pef2256->mclk = devm_clk_get_enabled(&pdev->dev, "mclk");
	pef2256->sclkr = devm_clk_get_enabled(&pdev->dev, "sclkr");
	pef2256->sclkx = devm_clk_get_enabled(&pdev->dev, "sclkx");

	pef2256->reset_gpio = devm_gpiod_get_optional(&pdev->dev, "reset", GPIOD_OUT_LOW);
	if (pef2256->reset_gpio) {
		gpiod_set_value_cansleep(pef2256->reset_gpio, 1);
		udelay(10);
		gpiod_set_value_cansleep(pef2256->reset_gpio, 0);
		udelay(10);
	}

	pef2556_of_parse(pef2256, np);

	irq = platform_get_irq(pdev, 0);
	ret = devm_request_irq(pef2256->dev, irq, pef2256_irq_handler, 0, "pef2256", pef2256);

	platform_set_drvdata(pdev, pef2256);

	mfd_add_devices(pef2256->dev, PLATFORM_DEVID_NONE, pef2256_devs,
	      		ARRAY_SIZE(pef2256_devs), NULL, 0, NULL);    

Quoted 8 lines

Quoted 2 lines

Wait, now you use MFD framework, so the driver is suitable for MFD. Before there was nothing like that in your code.

Agree, the above is suitable for MFD, since it does all the things I said your last one did not. You *can* also use of_platform_populate() here, since you are *also* requesting and initialising shared resources. You cannot do *both* however.

Thanks for having confirmed that this driver can be a MFD driver.

Related to of_platform_populate() / mfd_add_devices(), I wanted to use both because:
- the pinctrl part does not have (and does not need to have) a specific node with a specific compatible property. In order to instantiate the related driver mfd_add_devices() is the only way I know.
- the audio component nodes have a compatible string and several components (ie several nodes) can be present. of_platform_populate() call seems the simple way to instantiate them.

Is there a way to use mfd_add_devices() in this case without the need to count the audio component nodes in order to allocate as much mfd_cell as nodes having a matching compatible property ? Is there an other API to do it ?

Quoted 64 lines · Krzysztof Kozlowski

Quoted 61 lines · Herve Codina

Quoted 11 lines

Quoted 8 lines

So, the structure I have in mind:
- pef2256.c (MFD)
implement and do the setup at probe()
Add the children at probe():
- pef2256-pinctrl (pinctrl) added using mfd_add_devices()
- pef2256-codec (ASoC codec) added using devm_of_platform_populate()

Lee, with this in mind, can the core pef2256.c be a MFD driver ?

You do not use MFD here, so why do you want to keep it in MFD? If you disagree, please tell me where is the MFD code in your patch?

I don't want to absolutely use MFD.
I just want to put my driver somewhere and I don't know the right location between MFD and Misc.

Basically, the driver needs to do (little simplified and error path removed):

  static const struct mfd_cell pef2256_devs[] = {
  	{ .name = "lantiq-pef2256-pinctrl", },
  };

static int pef2256_probe(struct platform_device *pdev)

  {
	struct pef2256 *pef2256;
	void __iomem *iomem;
	int ret;
	int irq;

	pef2256 = devm_kzalloc(&pdev->dev, sizeof(*pef2256), GFP_KERNEL);
	if (!pef2256)
		return -ENOMEM;

	pef2256->dev = &pdev->dev;

	iomem = devm_platform_ioremap_resource(pdev, 0);

	pef2256->regmap = devm_regmap_init_mmio(&pdev->dev, iomem,
						&pef2256_regmap_config);

	pef2256->mclk = devm_clk_get_enabled(&pdev->dev, "mclk");
	pef2256->sclkr = devm_clk_get_enabled(&pdev->dev, "sclkr");
	pef2256->sclkx = devm_clk_get_enabled(&pdev->dev, "sclkx");

	pef2256->reset_gpio = devm_gpiod_get_optional(&pdev->dev, "reset", GPIOD_OUT_LOW);
	if (pef2256->reset_gpio) {
		gpiod_set_value_cansleep(pef2256->reset_gpio, 1);
		udelay(10);
		gpiod_set_value_cansleep(pef2256->reset_gpio, 0);
		udelay(10);
	}

	pef2556_of_parse(pef2256, np);

	irq = platform_get_irq(pdev, 0);
	ret = devm_request_irq(pef2256->dev, irq, pef2256_irq_handler, 0, "pef2256", pef2256);

	platform_set_drvdata(pdev, pef2256);

	mfd_add_devices(pef2256->dev, PLATFORM_DEVID_NONE, pef2256_devs,
	      		ARRAY_SIZE(pef2256_devs), NULL, 0, NULL);  

Quoted 2 lines

Wait, now you use MFD framework, so the driver is suitable for MFD. Before there was nothing like that in your code.

Agree, the above is suitable for MFD, since it does all the things I said your last one did not. You *can* also use of_platform_populate() here, since you are *also* requesting and initialising shared resources. You cannot do *both* however.

Quoted 37 lines · Herve Codina

Quoted 7 lines

I suggest that none of this (besides the child registration, which is achieved by a simple OF API call in this case) has anything to do with MFD. We are not requesting and initialising shared resources and we are not using the MFD API to register children. The pin control functionality clearly needs moving to Pinctrl and the rest, if you cannot find a suitable home for it *may be* suitable for Misc.

I am confused and I am not really sure to understand where to put my driver.

The core pef2256.c needs to:
1) setup the pef2256
2) add the children

To add the children it calls devm_of_platform_populate() to add the audio parts as several audio children can be available with the same compatible string.

I plan to move the pinctrl part to the pinctrl subsystem. With this done, the core pef2256.c will probably add the children using:
- a mfd_cell for the pinctrl part
- devm_of_platform_populate() for the audio children

The setup (E1 lines and TDM configuration) still needs to be done by the core pef2256.c. Moving this part only to Misc will break the hierarchy. The audio children depends on the core pef2256.c as this one do the setup. Having in the audio children and the part that do the setup in same hierarchy level is not correct. Audio children should be children of the part that do the setup.

So, the structure I have in mind:
- pef2256.c (MFD)
implement and do the setup at probe()
Add the children at probe():
- pef2256-pinctrl (pinctrl) added using mfd_add_devices()
- pef2256-codec (ASoC codec) added using devm_of_platform_populate()

Quoted 1 line

Lee, with this in mind, can the core pef2256.c be a MFD driver ?

You do not use MFD here, so why do you want to keep it in MFD? If you disagree, please tell me where is the MFD code in your patch?

Best regards,
Krzysztof

Quoted 93 lines · Herve Codina

Quoted 81 lines · On Fri, 31 Mar 2023 15:44:19 +0200 Krzysztof Kozlowski…

Quoted 43 lines · Herve Codina

Hi Krzysztof, Lee

Quoted 28 lines · On Fri, 31 Mar 2023 11:13:30 +0200 Krzysztof Kozlowski…

Quoted 18 lines · Herve Codina

Hi Lee,

Quoted 9 lines · On Thu, 30 Mar 2023 17:05:10 +0100 Lee Jones <[email protected]> wrote:

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

My goodness!

It's been a long time since I've seen anything quite like this.

Yes, old things but working on recent kernel.

Quoted 1 line

Quoted 8 lines

Quoted 3 lines

My suggestion to you:

* Split this up into components that fit functional subsystems

It is done. The audio part is present in ASoC subsystem (path 5 in this series). pinctrl function is implemented in this driver and, as I don't want to share registers, I would prefer to keep this function inside this driver.

Quoted 5 lines

The amount of defines and huge functions like pef2256_setup_e1() contradict it.

Even the pef2256_setup_e1() really does not follow Linux coding style - you know the size requirement, right?

I know that pef2256_setup_e1() is quite big and I will look at a way to split it in a consistent way.

Quoted 1 line

Quoted 6 lines

Quoted 2 lines

pef2256_get_groups_count, struct pinmux_ops and others - this is pinctrl, not MFD! They cannot be in MFD driver.

Maybe the issue is that MFD was not a good choice.
The "function" provided are not independent of each other.
The "main" driver (pef2256.c) needs to do the setup and handle the interrupt.

Just like all PMICs...

Quoted 2 lines

The "function" provided are some glues in order to be used in some sub-systems such as audio. Indeed, ASoC needs a codec DAI to be connected to a CPU DAI.

Just like in all other cases...

Quoted 2 lines

These "functions" need to be started (ie probe()) after the pef2256 setup was done. So a start (probe()) order relationship is needed.

Just like in all other cases, so I really do not see here anything special.

Quoted 3 lines

If a MFD driver needs independent children to handle independent functions, the pef2256 does not fit well as a MFD driver.

Why? So far everything is exactly the same.

Quoted 4 lines

I switched from misc to MFD just to handle child DT nodes instead of having phandles. Using child DT nodes instead of phandles is really a good thing and need to be kept.

Your DT bindings and nodes are not related to driver design. It does not matter for Devicetree if you put it to misc or MFD.

It does not matter for driver whether you call it in Devicetree foo or bar.

Quoted 1 line

The switch to MFD was probably not the best thing to do.

Quoted 8 lines

Maybe, but your existing arguments are not really related...

Quoted 3 lines

What do you think if I switched back the pef2256 "main" driver (pef2256.c) to misc ?

Why? What would it solve? You want to stuff pinctrl driver to misc, to avoid Lee's review? No. Pinctrl goes to pinctrl. Not to misc. Not to MFD.

Quoted 8 lines

Just to be clear on that point, I do not want to avoid any reviews and Lee's reviews are fully welcome.

I just misunderstood. I thought the issue was that the pef2256 MFD driver has an internal setup() and that the 'function' provided were not independent (they share the pef2256 API).
My first feeling was that it was expected a kind of 'empty' driver for the pef2256 MFD driver. The pef2256 driver cannot be empty. That's why I talk about misc.

It can be empty. That's what simple-mfd is for. :)

I suggest that none of this (besides the child registration, which is achieved by a simple OF API call in this case) has anything to do with MFD. We are not requesting and initialising shared resources and we are not using the MFD API to register children. The pin control functionality clearly needs moving to Pinctrl and the rest, if you cannot find a suitable home for it *may be* suitable for Misc.

Quoted 43 lines · Herve Codina

Hi Krzysztof, Lee

Quoted 28 lines · On Fri, 31 Mar 2023 11:13:30 +0200 Krzysztof Kozlowski…

Quoted 18 lines · Herve Codina

Hi Lee,

Quoted 9 lines · On Thu, 30 Mar 2023 17:05:10 +0100 Lee Jones <[email protected]> wrote:

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

My goodness!

It's been a long time since I've seen anything quite like this.

Yes, old things but working on recent kernel.

Quoted 1 line

Quoted 8 lines

Quoted 3 lines

My suggestion to you:

* Split this up into components that fit functional subsystems

It is done. The audio part is present in ASoC subsystem (path 5 in this series). pinctrl function is implemented in this driver and, as I don't want to share registers, I would prefer to keep this function inside this driver.

Quoted 5 lines

The amount of defines and huge functions like pef2256_setup_e1() contradict it.

Even the pef2256_setup_e1() really does not follow Linux coding style - you know the size requirement, right?

I know that pef2256_setup_e1() is quite big and I will look at a way to split it in a consistent way.

Quoted 1 line

Quoted 6 lines

Quoted 2 lines

pef2256_get_groups_count, struct pinmux_ops and others - this is pinctrl, not MFD! They cannot be in MFD driver.

Maybe the issue is that MFD was not a good choice.
The "function" provided are not independent of each other.
The "main" driver (pef2256.c) needs to do the setup and handle the interrupt.

Just like all PMICs...

Quoted 2 lines

The "function" provided are some glues in order to be used in some sub-systems such as audio. Indeed, ASoC needs a codec DAI to be connected to a CPU DAI.

Just like in all other cases...

Quoted 2 lines

These "functions" need to be started (ie probe()) after the pef2256 setup was done. So a start (probe()) order relationship is needed.

Just like in all other cases, so I really do not see here anything special.

Quoted 3 lines

If a MFD driver needs independent children to handle independent functions, the pef2256 does not fit well as a MFD driver.

Why? So far everything is exactly the same.

Quoted 4 lines

I switched from misc to MFD just to handle child DT nodes instead of having phandles. Using child DT nodes instead of phandles is really a good thing and need to be kept.

Your DT bindings and nodes are not related to driver design. It does not matter for Devicetree if you put it to misc or MFD.

It does not matter for driver whether you call it in Devicetree foo or bar.

Quoted 1 line

The switch to MFD was probably not the best thing to do.

Maybe, but your existing arguments are not really related...

Quoted 3 lines

What do you think if I switched back the pef2256 "main" driver (pef2256.c) to misc ?

Why? What would it solve? You want to stuff pinctrl driver to misc, to avoid Lee's review? No. Pinctrl goes to pinctrl. Not to misc. Not to MFD.

Quoted 19 lines

Quoted 9 lines

Quoted 9 lines

Also, I sent a RFC related to HDLC and PHY. In this RFC, the pef2256 is considered as a PHY and handled in the PHY subsystem.
https://lore.kernel.org/linux-kernel/[email protected]/

Quoted 7 lines

Quoted 3 lines

Quoted 1 line

* Run checkpatch.pl

I did.

There are tons of weird indentation,e.g.:
+#define PEF2256_2X_PC_XPC_XLT (0x8 << 0)
^^^^ there is only one space after #define

I ran checkpatch.pl, not checkpatch.pl --strict.

Quoted 8 lines

The spaces related the #define can be seen on many other drivers.

#define FOO_REG_BAR 0x10
#define FOO_REG_BAR_BIT0 BIT(0)
#define FOO_REG_BAR_BIT4 BIT(4)

The first line is the register offset and the other lines (indented) are the bits description related to this register.

I don't think we have such convention in general and argument that some drivers do it in some subsystems is never a good argument. If they also misspell things or use Hungarian notation, shall we do the same?

Although maybe it is fine for Lee. I find it unreadable.

git grep '#define \+[A-Z]' | wc -l
73889
git grep '#define [A-Z]' | wc -l
3996054

In MFD there is only one driver doing this.

Most of other cases are net and gpu.

Best regards,
Krzysztof

Quoted 18 lines · Herve Codina

Hi Lee,

Quoted 9 lines · On Thu, 30 Mar 2023 17:05:10 +0100 Lee Jones <[email protected]> wrote:

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

My goodness!

It's been a long time since I've seen anything quite like this.

Yes, old things but working on recent kernel.

Quoted 1 line

Quoted 8 lines

Quoted 3 lines

My suggestion to you:

* Split this up into components that fit functional subsystems

It is done. The audio part is present in ASoC subsystem (path 5 in this series). pinctrl function is implemented in this driver and, as I don't want to share registers, I would prefer to keep this function inside this driver.

The amount of defines and huge functions like pef2256_setup_e1() contradict it.

Even the pef2256_setup_e1() really does not follow Linux coding style - you know the size requirement, right?

pef2256_get_groups_count, struct pinmux_ops and others - this is pinctrl, not MFD! They cannot be in MFD driver.

Quoted 6 lines

Also, I sent a RFC related to HDLC and PHY. In this RFC, the pef2256 is considered as a PHY and handled in the PHY subsystem.
https://lore.kernel.org/linux-kernel/[email protected]/

Quoted 3 lines

Quoted 1 line

* Run checkpatch.pl

I did.

There are tons of weird indentation,e.g.:
+#define PEF2256_2X_PC_XPC_XLT (0x8 << 0)
^^^^ there is only one space after #define

... and other style issues:

CHECK: Please don't use multiple blank lines
CHECK: spaces preferred around that '+' (ctx:VxV)
CHECK: Alignment should match open parenthesis
CHECK: Macro argument reuse '_groups' - possible side-effects?
CHECK: usleep_range is preferred over udelay; see Documentation/timers/timers-howto.rst
CHECK: spaces preferred around that '/' (ctx:VxV)

Quoted 12 lines

Quoted 4 lines

* Remove all of the debug prints

I can do that in the next iteration if really needed.

Quoted 1 line

* Move all of the defines out to a header file

These defines are related to registers. As I don't want to share these registers, is it really necessary to use a header file for them ?

Quoted 5 lines

Quoted 1 line

* Be more verbose in your documentation / comments

I can improve the API documentation present in include/mfd/pef2256.h. Do you thing that is necessary ? Only a few devices will use this API.

Krzysztof

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

My goodness!

It's been a long time since I've seen anything quite like this.

My suggestion to you:

* Split this up into components that fit functional subsystems

* Run checkpatch.pl

* Remove all of the debug prints

* Move all of the defines out to a header file

* Be more verbose in your documentation / comments

* Consider using simple-mfd to probe child devices.

Quoted 10 lines

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

---
 drivers/mfd/Kconfig         |   17 +
 drivers/mfd/Makefile        |    1 +
 drivers/mfd/pef2256.c       | 1355 +++++++++++++++++++++++++++++++++++
 include/linux/mfd/pef2256.h |   28 +
 4 files changed, 1401 insertions(+)
 create mode 100644 drivers/mfd/pef2256.c
 create mode 100644 include/linux/mfd/pef2256.h  

--
Lee Jones [李琼斯]