[PATCH v2 1/4] mfd: simple-mfd-i2c: Add Delta TN48M CPLD support

[PATCH v2 1/4] mfd: simple-mfd-i2c: Add Delta TN48M CPLD support

[Robert Marko]

Delta TN48M switches have a Lattice CPLD that serves
multiple purposes including being a GPIO expander.

So, lets use the simple I2C MFD driver to provide the MFD core.

Also add a virtual symbol which pulls in the simple-mfd-i2c driver and
provide a common symbol on which the subdevice drivers can depend on.

Signed-off-by: Robert Marko <robert.marko@sartura.hr>
---
Changes in v2:
* Drop the custom MFD driver and header
* Use simple I2C MFD driver

 drivers/mfd/Kconfig          | 10 ++++++++++
 drivers/mfd/simple-mfd-i2c.c |  1 +
 2 files changed, 11 insertions(+)

diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
index 5c7f2b100191..1042424c5678 100644
--- a/drivers/mfd/Kconfig
+++ b/drivers/mfd/Kconfig
@@ -297,6 +297,16 @@ config MFD_ASIC3
 	  This driver supports the ASIC3 multifunction chip found on many
 	  PDAs (mainly iPAQ and HTC based ones)
 
+config MFD_TN48M_CPLD
+	tristate "Delta Networks TN48M switch CPLD driver"
+	depends on I2C
+	select MFD_SIMPLE_MFD_I2C
+	help
+	  Select this option to enable support for Delta Networks TN48M switch
+	  CPLD. It consists of MFD and GPIO drivers. CPLD provides GPIOS-s
+	  for the SFP slots as well as power supply related information.
+	  SFP support depends on the GPIO driver being selected.
+
 config PMIC_DA903X
 	bool "Dialog Semiconductor DA9030/DA9034 PMIC Support"
 	depends on I2C=y
diff --git a/drivers/mfd/simple-mfd-i2c.c b/drivers/mfd/simple-mfd-i2c.c
index 87f684cff9a1..af8e91781417 100644
--- a/drivers/mfd/simple-mfd-i2c.c
+++ b/drivers/mfd/simple-mfd-i2c.c
@@ -39,6 +39,7 @@ static int simple_mfd_i2c_probe(struct i2c_client *i2c)
 
 static const struct of_device_id simple_mfd_i2c_of_match[] = {
 	{ .compatible = "kontron,sl28cpld" },
+	{ .compatible = "delta,tn48m-cpld" },
 	{}
 };
 MODULE_DEVICE_TABLE(of, simple_mfd_i2c_of_match);
-- 
2.31.1

[PATCH v2 2/4] gpio: Add Delta TN48M CPLD GPIO driver

[Robert Marko]

Delta TN48M CPLD is used as a GPIO expander for the SFP GPIOs.

It is a mix of input only and output only pins.

Signed-off-by: Robert Marko <robert.marko@sartura.hr>
---
Changes in v2:
* Rewrite to use simple I2C MFD and GPIO regmap
* Drop DT bindings for pin numbering

 drivers/gpio/Kconfig      | 12 ++++++
 drivers/gpio/Makefile     |  1 +
 drivers/gpio/gpio-tn48m.c | 89 +++++++++++++++++++++++++++++++++++++++
 3 files changed, 102 insertions(+)
 create mode 100644 drivers/gpio/gpio-tn48m.c

diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index 1dd0ec6727fd..46caf72960e6 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -1332,6 +1332,18 @@ config GPIO_TIMBERDALE
 	help
 	Add support for the GPIO IP in the timberdale FPGA.
 
+config GPIO_TN48M_CPLD
+	tristate "Delta Networks TN48M switch CPLD GPIO driver"
+	depends on MFD_TN48M_CPLD
+	select GPIO_REGMAP
+	help
+	  This enables support for the GPIOs found on the Delta
+	  Networks TN48M switch CPLD.
+	  They are used for inputs and outputs on the SFP slots.
+
+	  This driver can also be built as a module. If so, the
+	  module will be called gpio-tn48m.
+
 config GPIO_TPS65086
 	tristate "TI TPS65086 GPO"
 	depends on MFD_TPS65086
diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
index d7c81e1611a4..07fa6419305f 100644
--- a/drivers/gpio/Makefile
+++ b/drivers/gpio/Makefile
@@ -147,6 +147,7 @@ obj-$(CONFIG_GPIO_TEGRA186)		+= gpio-tegra186.o
 obj-$(CONFIG_GPIO_TEGRA)		+= gpio-tegra.o
 obj-$(CONFIG_GPIO_THUNDERX)		+= gpio-thunderx.o
 obj-$(CONFIG_GPIO_TIMBERDALE)		+= gpio-timberdale.o
+obj-$(CONFIG_GPIO_TN48M_CPLD)		+= gpio-tn48m.o
 obj-$(CONFIG_GPIO_TPIC2810)		+= gpio-tpic2810.o
 obj-$(CONFIG_GPIO_TPS65086)		+= gpio-tps65086.o
 obj-$(CONFIG_GPIO_TPS65218)		+= gpio-tps65218.o
diff --git a/drivers/gpio/gpio-tn48m.c b/drivers/gpio/gpio-tn48m.c
new file mode 100644
index 000000000000..41484c002826
--- /dev/null
+++ b/drivers/gpio/gpio-tn48m.c
@@ -0,0 +1,89 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Delta TN48M CPLD GPIO driver
+ *
+ * Copyright 2021 Sartura Ltd
+ *
+ * Author: Robert Marko <robert.marko@sartura.hr>
+ */
+
+#include <linux/device.h>
+#include <linux/gpio/driver.h>
+#include <linux/gpio/regmap.h>
+#include <linux/kernel.h>
+#include <linux/mod_devicetable.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+
+enum tn48m_gpio_type {
+	TN48M_SFP_TX_DISABLE = 1,
+	TN48M_SFP_PRESENT,
+	TN48M_SFP_LOS,
+};
+
+static int tn48m_gpio_probe(struct platform_device *pdev)
+{
+	struct gpio_regmap_config config = {0};
+	enum tn48m_gpio_type type;
+	struct regmap *regmap;
+	u32 base;
+	int ret;
+
+	if (!pdev->dev.parent)
+		return -ENODEV;
+
+	type = (uintptr_t)device_get_match_data(&pdev->dev);
+	if (!type)
+		return -ENODEV;
+
+	ret = device_property_read_u32(&pdev->dev, "reg", &base);
+	if (ret)
+		return -EINVAL;
+
+	regmap = dev_get_regmap(pdev->dev.parent, NULL);
+	if (!regmap)
+		return -ENODEV;
+
+	config.regmap = regmap;
+	config.parent = &pdev->dev;
+	config.ngpio = 4;
+
+	switch (type) {
+	case TN48M_SFP_TX_DISABLE:
+		config.reg_set_base = base;
+		break;
+	case TN48M_SFP_PRESENT:
+		config.reg_dat_base = base;
+		break;
+	case TN48M_SFP_LOS:
+		config.reg_dat_base = base;
+		break;
+	default:
+		dev_err(&pdev->dev, "unknown type %d\n", type);
+		return -ENODEV;
+	}
+
+	return PTR_ERR_OR_ZERO(devm_gpio_regmap_register(&pdev->dev, &config));
+}
+
+static const struct of_device_id tn48m_gpio_of_match[] = {
+	{ .compatible = "delta,tn48m-gpio-sfp-tx-disable", .data = (void *)TN48M_SFP_TX_DISABLE },
+	{ .compatible = "delta,tn48m-gpio-sfp-present", .data = (void *)TN48M_SFP_PRESENT },
+	{ .compatible = "delta,tn48m-gpio-sfp-los", .data = (void *)TN48M_SFP_LOS },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, tn48m_gpio_of_match);
+
+static struct platform_driver tn48m_gpio_driver = {
+	.driver = {
+		.name = "delta-tn48m-gpio",
+		.of_match_table = tn48m_gpio_of_match,
+	},
+	.probe = tn48m_gpio_probe,
+};
+module_platform_driver(tn48m_gpio_driver);
+
+MODULE_AUTHOR("Robert Marko <robert.marko@sartura.hr>");
+MODULE_DESCRIPTION("Delta TN48M CPLD GPIO driver");
+MODULE_LICENSE("GPL");
-- 
2.31.1

Re: [PATCH v2 2/4] gpio: Add Delta TN48M CPLD GPIO driver

[Andy Shevchenko]

On Mon, May 24, 2021 at 3:06 PM Robert Marko <robert.marko@sartura.hr> wrote:
>
> Delta TN48M CPLD is used as a GPIO expander for the SFP GPIOs.
>
> It is a mix of input only and output only pins.

LGTM!
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>

> Signed-off-by: Robert Marko <robert.marko@sartura.hr>
> ---
> Changes in v2:
> * Rewrite to use simple I2C MFD and GPIO regmap
> * Drop DT bindings for pin numbering
>
>  drivers/gpio/Kconfig      | 12 ++++++
>  drivers/gpio/Makefile     |  1 +
>  drivers/gpio/gpio-tn48m.c | 89 +++++++++++++++++++++++++++++++++++++++
>  3 files changed, 102 insertions(+)
>  create mode 100644 drivers/gpio/gpio-tn48m.c
>
> diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
> index 1dd0ec6727fd..46caf72960e6 100644
> --- a/drivers/gpio/Kconfig
> +++ b/drivers/gpio/Kconfig
> @@ -1332,6 +1332,18 @@ config GPIO_TIMBERDALE
>         help
>         Add support for the GPIO IP in the timberdale FPGA.
>
> +config GPIO_TN48M_CPLD
> +       tristate "Delta Networks TN48M switch CPLD GPIO driver"
> +       depends on MFD_TN48M_CPLD
> +       select GPIO_REGMAP
> +       help
> +         This enables support for the GPIOs found on the Delta
> +         Networks TN48M switch CPLD.
> +         They are used for inputs and outputs on the SFP slots.
> +
> +         This driver can also be built as a module. If so, the
> +         module will be called gpio-tn48m.
> +
>  config GPIO_TPS65086
>         tristate "TI TPS65086 GPO"
>         depends on MFD_TPS65086
> diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
> index d7c81e1611a4..07fa6419305f 100644
> --- a/drivers/gpio/Makefile
> +++ b/drivers/gpio/Makefile
> @@ -147,6 +147,7 @@ obj-$(CONFIG_GPIO_TEGRA186)         += gpio-tegra186.o
>  obj-$(CONFIG_GPIO_TEGRA)               += gpio-tegra.o
>  obj-$(CONFIG_GPIO_THUNDERX)            += gpio-thunderx.o
>  obj-$(CONFIG_GPIO_TIMBERDALE)          += gpio-timberdale.o
> +obj-$(CONFIG_GPIO_TN48M_CPLD)          += gpio-tn48m.o
>  obj-$(CONFIG_GPIO_TPIC2810)            += gpio-tpic2810.o
>  obj-$(CONFIG_GPIO_TPS65086)            += gpio-tps65086.o
>  obj-$(CONFIG_GPIO_TPS65218)            += gpio-tps65218.o
> diff --git a/drivers/gpio/gpio-tn48m.c b/drivers/gpio/gpio-tn48m.c
> new file mode 100644
> index 000000000000..41484c002826
> --- /dev/null
> +++ b/drivers/gpio/gpio-tn48m.c
> @@ -0,0 +1,89 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Delta TN48M CPLD GPIO driver
> + *
> + * Copyright 2021 Sartura Ltd
> + *
> + * Author: Robert Marko <robert.marko@sartura.hr>
> + */
> +
> +#include <linux/device.h>
> +#include <linux/gpio/driver.h>
> +#include <linux/gpio/regmap.h>
> +#include <linux/kernel.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/regmap.h>
> +
> +enum tn48m_gpio_type {
> +       TN48M_SFP_TX_DISABLE = 1,
> +       TN48M_SFP_PRESENT,
> +       TN48M_SFP_LOS,
> +};
> +
> +static int tn48m_gpio_probe(struct platform_device *pdev)
> +{
> +       struct gpio_regmap_config config = {0};
> +       enum tn48m_gpio_type type;
> +       struct regmap *regmap;
> +       u32 base;
> +       int ret;
> +
> +       if (!pdev->dev.parent)
> +               return -ENODEV;
> +
> +       type = (uintptr_t)device_get_match_data(&pdev->dev);
> +       if (!type)
> +               return -ENODEV;
> +
> +       ret = device_property_read_u32(&pdev->dev, "reg", &base);
> +       if (ret)
> +               return -EINVAL;
> +
> +       regmap = dev_get_regmap(pdev->dev.parent, NULL);
> +       if (!regmap)
> +               return -ENODEV;
> +
> +       config.regmap = regmap;
> +       config.parent = &pdev->dev;
> +       config.ngpio = 4;
> +
> +       switch (type) {
> +       case TN48M_SFP_TX_DISABLE:
> +               config.reg_set_base = base;
> +               break;
> +       case TN48M_SFP_PRESENT:
> +               config.reg_dat_base = base;
> +               break;
> +       case TN48M_SFP_LOS:
> +               config.reg_dat_base = base;
> +               break;
> +       default:
> +               dev_err(&pdev->dev, "unknown type %d\n", type);
> +               return -ENODEV;
> +       }
> +
> +       return PTR_ERR_OR_ZERO(devm_gpio_regmap_register(&pdev->dev, &config));
> +}
> +
> +static const struct of_device_id tn48m_gpio_of_match[] = {
> +       { .compatible = "delta,tn48m-gpio-sfp-tx-disable", .data = (void *)TN48M_SFP_TX_DISABLE },
> +       { .compatible = "delta,tn48m-gpio-sfp-present", .data = (void *)TN48M_SFP_PRESENT },
> +       { .compatible = "delta,tn48m-gpio-sfp-los", .data = (void *)TN48M_SFP_LOS },
> +       { }
> +};
> +MODULE_DEVICE_TABLE(of, tn48m_gpio_of_match);
> +
> +static struct platform_driver tn48m_gpio_driver = {
> +       .driver = {
> +               .name = "delta-tn48m-gpio",
> +               .of_match_table = tn48m_gpio_of_match,
> +       },
> +       .probe = tn48m_gpio_probe,
> +};
> +module_platform_driver(tn48m_gpio_driver);
> +
> +MODULE_AUTHOR("Robert Marko <robert.marko@sartura.hr>");
> +MODULE_DESCRIPTION("Delta TN48M CPLD GPIO driver");
> +MODULE_LICENSE("GPL");
> --
> 2.31.1
>


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 2/4] gpio: Add Delta TN48M CPLD GPIO driver

[Bartosz Golaszewski]

On Mon, May 24, 2021 at 2:05 PM Robert Marko <robert.marko@sartura.hr> wrote:
>
> Delta TN48M CPLD is used as a GPIO expander for the SFP GPIOs.
>
> It is a mix of input only and output only pins.
>
> Signed-off-by: Robert Marko <robert.marko@sartura.hr>
> ---
> Changes in v2:
> * Rewrite to use simple I2C MFD and GPIO regmap
> * Drop DT bindings for pin numbering
>
>  drivers/gpio/Kconfig      | 12 ++++++
>  drivers/gpio/Makefile     |  1 +
>  drivers/gpio/gpio-tn48m.c | 89 +++++++++++++++++++++++++++++++++++++++
>  3 files changed, 102 insertions(+)
>  create mode 100644 drivers/gpio/gpio-tn48m.c
>
> diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
> index 1dd0ec6727fd..46caf72960e6 100644
> --- a/drivers/gpio/Kconfig
> +++ b/drivers/gpio/Kconfig
> @@ -1332,6 +1332,18 @@ config GPIO_TIMBERDALE
>         help
>         Add support for the GPIO IP in the timberdale FPGA.
>
> +config GPIO_TN48M_CPLD
> +       tristate "Delta Networks TN48M switch CPLD GPIO driver"
> +       depends on MFD_TN48M_CPLD
> +       select GPIO_REGMAP
> +       help
> +         This enables support for the GPIOs found on the Delta
> +         Networks TN48M switch CPLD.
> +         They are used for inputs and outputs on the SFP slots.
> +
> +         This driver can also be built as a module. If so, the
> +         module will be called gpio-tn48m.
> +
>  config GPIO_TPS65086
>         tristate "TI TPS65086 GPO"
>         depends on MFD_TPS65086
> diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
> index d7c81e1611a4..07fa6419305f 100644
> --- a/drivers/gpio/Makefile
> +++ b/drivers/gpio/Makefile
> @@ -147,6 +147,7 @@ obj-$(CONFIG_GPIO_TEGRA186)         += gpio-tegra186.o
>  obj-$(CONFIG_GPIO_TEGRA)               += gpio-tegra.o
>  obj-$(CONFIG_GPIO_THUNDERX)            += gpio-thunderx.o
>  obj-$(CONFIG_GPIO_TIMBERDALE)          += gpio-timberdale.o
> +obj-$(CONFIG_GPIO_TN48M_CPLD)          += gpio-tn48m.o
>  obj-$(CONFIG_GPIO_TPIC2810)            += gpio-tpic2810.o
>  obj-$(CONFIG_GPIO_TPS65086)            += gpio-tps65086.o
>  obj-$(CONFIG_GPIO_TPS65218)            += gpio-tps65218.o
> diff --git a/drivers/gpio/gpio-tn48m.c b/drivers/gpio/gpio-tn48m.c
> new file mode 100644
> index 000000000000..41484c002826
> --- /dev/null
> +++ b/drivers/gpio/gpio-tn48m.c
> @@ -0,0 +1,89 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Delta TN48M CPLD GPIO driver
> + *
> + * Copyright 2021 Sartura Ltd
> + *
> + * Author: Robert Marko <robert.marko@sartura.hr>
> + */
> +
> +#include <linux/device.h>
> +#include <linux/gpio/driver.h>
> +#include <linux/gpio/regmap.h>
> +#include <linux/kernel.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/regmap.h>
> +
> +enum tn48m_gpio_type {
> +       TN48M_SFP_TX_DISABLE = 1,
> +       TN48M_SFP_PRESENT,
> +       TN48M_SFP_LOS,
> +};
> +
> +static int tn48m_gpio_probe(struct platform_device *pdev)
> +{
> +       struct gpio_regmap_config config = {0};
> +       enum tn48m_gpio_type type;
> +       struct regmap *regmap;
> +       u32 base;
> +       int ret;
> +
> +       if (!pdev->dev.parent)
> +               return -ENODEV;
> +
> +       type = (uintptr_t)device_get_match_data(&pdev->dev);
> +       if (!type)
> +               return -ENODEV;
> +
> +       ret = device_property_read_u32(&pdev->dev, "reg", &base);
> +       if (ret)
> +               return -EINVAL;
> +
> +       regmap = dev_get_regmap(pdev->dev.parent, NULL);
> +       if (!regmap)
> +               return -ENODEV;
> +
> +       config.regmap = regmap;
> +       config.parent = &pdev->dev;
> +       config.ngpio = 4;
> +
> +       switch (type) {
> +       case TN48M_SFP_TX_DISABLE:
> +               config.reg_set_base = base;
> +               break;
> +       case TN48M_SFP_PRESENT:
> +               config.reg_dat_base = base;
> +               break;
> +       case TN48M_SFP_LOS:
> +               config.reg_dat_base = base;
> +               break;
> +       default:
> +               dev_err(&pdev->dev, "unknown type %d\n", type);
> +               return -ENODEV;
> +       }
> +
> +       return PTR_ERR_OR_ZERO(devm_gpio_regmap_register(&pdev->dev, &config));
> +}
> +
> +static const struct of_device_id tn48m_gpio_of_match[] = {
> +       { .compatible = "delta,tn48m-gpio-sfp-tx-disable", .data = (void *)TN48M_SFP_TX_DISABLE },
> +       { .compatible = "delta,tn48m-gpio-sfp-present", .data = (void *)TN48M_SFP_PRESENT },
> +       { .compatible = "delta,tn48m-gpio-sfp-los", .data = (void *)TN48M_SFP_LOS },
> +       { }
> +};
> +MODULE_DEVICE_TABLE(of, tn48m_gpio_of_match);
> +
> +static struct platform_driver tn48m_gpio_driver = {
> +       .driver = {
> +               .name = "delta-tn48m-gpio",
> +               .of_match_table = tn48m_gpio_of_match,
> +       },
> +       .probe = tn48m_gpio_probe,
> +};
> +module_platform_driver(tn48m_gpio_driver);
> +
> +MODULE_AUTHOR("Robert Marko <robert.marko@sartura.hr>");
> +MODULE_DESCRIPTION("Delta TN48M CPLD GPIO driver");
> +MODULE_LICENSE("GPL");
> --
> 2.31.1
>

Acked-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>

I guess this will go through the MFD tree?

Bart

Re: [PATCH v2 2/4] gpio: Add Delta TN48M CPLD GPIO driver

[Linus Walleij]

On Mon, May 24, 2021 at 2:05 PM Robert Marko <robert.marko@sartura.hr> wrote:

> Delta TN48M CPLD is used as a GPIO expander for the SFP GPIOs.
>
> It is a mix of input only and output only pins.
>
> Signed-off-by: Robert Marko <robert.marko@sartura.hr>

Looks really good!
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

[PATCH v2 3/4] dt-bindings: mfd: Add Delta TN48M CPLD drivers bindings

[Robert Marko]

Add binding documents for the Delta TN48M CPLD drivers.

Signed-off-by: Robert Marko <robert.marko@sartura.hr>
---
Changes in v2:
* Implement MFD as a simple I2C MFD
* Add GPIO bindings as separate

 .../bindings/gpio/delta,tn48m-gpio.yaml       | 42 ++++++++++
 .../bindings/mfd/delta,tn48m-cpld.yaml        | 81 +++++++++++++++++++
 2 files changed, 123 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/gpio/delta,tn48m-gpio.yaml
 create mode 100644 Documentation/devicetree/bindings/mfd/delta,tn48m-cpld.yaml

diff --git a/Documentation/devicetree/bindings/gpio/delta,tn48m-gpio.yaml b/Documentation/devicetree/bindings/gpio/delta,tn48m-gpio.yaml
new file mode 100644
index 000000000000..aca646aecb12
--- /dev/null
+++ b/Documentation/devicetree/bindings/gpio/delta,tn48m-gpio.yaml
@@ -0,0 +1,42 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/gpio/delta,tn48m-gpio.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Delta Networks TN48M CPLD GPIO controller
+
+maintainers:
+  - Robert Marko <robert.marko@sartura.hr>
+
+description: |
+  This module is part of the Delta TN48M multi-function device. For more
+  details see ../mfd/delta,tn48m-cpld.yaml.
+
+  GPIO controller module provides GPIO-s for the SFP slots.
+  It is split into 3 controllers, one output only for the SFP TX disable
+  pins, one input only for the SFP present pins and one input only for
+  the SFP LOS pins.
+
+properties:
+  compatible:
+    enum:
+      - delta,tn48m-gpio-sfp-tx-disable
+      - delta,tn48m-gpio-sfp-present
+      - delta,tn48m-gpio-sfp-los
+
+  reg:
+    maxItems: 1
+
+  "#gpio-cells":
+    const: 2
+
+  gpio-controller: true
+
+required:
+  - compatible
+  - reg
+  - "#gpio-cells"
+  - gpio-controller
+
+additionalProperties: false
diff --git a/Documentation/devicetree/bindings/mfd/delta,tn48m-cpld.yaml b/Documentation/devicetree/bindings/mfd/delta,tn48m-cpld.yaml
new file mode 100644
index 000000000000..055e09129f86
--- /dev/null
+++ b/Documentation/devicetree/bindings/mfd/delta,tn48m-cpld.yaml
@@ -0,0 +1,81 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/mfd/delta,tn48m-cpld.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Delta Networks TN48M CPLD controller
+
+maintainers:
+  - Robert Marko <robert.marko@sartura.hr>
+
+description: |
+  Lattice CPLD onboard the TN48M switches is used for system
+  management.
+
+  It provides information about the hardware model, revision,
+  PSU status etc.
+
+  It is also being used as a GPIO expander for the SFP slots.
+
+properties:
+  compatible:
+    const: delta,tn48m-cpld
+
+  reg:
+    description:
+      I2C device address.
+    maxItems: 1
+
+  "#address-cells":
+    const: 1
+
+  "#size-cells":
+    const: 0
+
+required:
+  - compatible
+  - reg
+  - "#address-cells"
+  - "#size-cells"
+
+patternProperties:
+  "^gpio(@[0-9a-f]+)?$":
+    $ref: ../gpio/delta,tn48m-gpio.yaml
+
+additionalProperties: false
+
+examples:
+  - |
+    i2c {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        cpld@41 {
+            compatible = "delta,tn48m-cpld";
+            reg = <0x41>;
+            #address-cells = <1>;
+            #size-cells = <0>;
+
+            gpio@31 {
+                compatible = "delta,tn48m-gpio-sfp-tx-disable";
+                reg = <0x31>;
+                gpio-controller;
+                #gpio-cells = <2>;
+            };
+
+            gpio@3a {
+                compatible = "delta,tn48m-gpio-sfp-present";
+                reg = <0x3a>;
+                gpio-controller;
+                #gpio-cells = <2>;
+            };
+
+            gpio@40 {
+                compatible = "delta,tn48m-gpio-sfp-los";
+                reg = <0x40>;
+                gpio-controller;
+                #gpio-cells = <2>;
+            };
+        };
+    };
-- 
2.31.1

Re: [PATCH v2 3/4] dt-bindings: mfd: Add Delta TN48M CPLD drivers bindings

[Rob Herring]

On Mon, May 24, 2021 at 02:05:38PM +0200, Robert Marko wrote:
> Add binding documents for the Delta TN48M CPLD drivers.
> 
> Signed-off-by: Robert Marko <robert.marko@sartura.hr>
> ---
> Changes in v2:
> * Implement MFD as a simple I2C MFD
> * Add GPIO bindings as separate

I don't understand why this changed. This doesn't look like an MFD to 
me. Make your binding complete if there are missing functions. 
Otherwise, stick with what I already ok'ed.

> 
>  .../bindings/gpio/delta,tn48m-gpio.yaml       | 42 ++++++++++
>  .../bindings/mfd/delta,tn48m-cpld.yaml        | 81 +++++++++++++++++++
>  2 files changed, 123 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/gpio/delta,tn48m-gpio.yaml
>  create mode 100644 Documentation/devicetree/bindings/mfd/delta,tn48m-cpld.yaml
> 
> diff --git a/Documentation/devicetree/bindings/gpio/delta,tn48m-gpio.yaml b/Documentation/devicetree/bindings/gpio/delta,tn48m-gpio.yaml
> new file mode 100644
> index 000000000000..aca646aecb12
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/gpio/delta,tn48m-gpio.yaml
> @@ -0,0 +1,42 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/gpio/delta,tn48m-gpio.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Delta Networks TN48M CPLD GPIO controller
> +
> +maintainers:
> +  - Robert Marko <robert.marko@sartura.hr>
> +
> +description: |
> +  This module is part of the Delta TN48M multi-function device. For more
> +  details see ../mfd/delta,tn48m-cpld.yaml.
> +
> +  GPIO controller module provides GPIO-s for the SFP slots.
> +  It is split into 3 controllers, one output only for the SFP TX disable
> +  pins, one input only for the SFP present pins and one input only for
> +  the SFP LOS pins.
> +
> +properties:
> +  compatible:
> +    enum:
> +      - delta,tn48m-gpio-sfp-tx-disable
> +      - delta,tn48m-gpio-sfp-present
> +      - delta,tn48m-gpio-sfp-los

The function of the 'general purpose' IO should not be encoded into the 
compatible name. Is each instance.

> +
> +  reg:
> +    maxItems: 1
> +
> +  "#gpio-cells":
> +    const: 2
> +
> +  gpio-controller: true
> +
> +required:
> +  - compatible
> +  - reg
> +  - "#gpio-cells"
> +  - gpio-controller
> +
> +additionalProperties: false
> diff --git a/Documentation/devicetree/bindings/mfd/delta,tn48m-cpld.yaml b/Documentation/devicetree/bindings/mfd/delta,tn48m-cpld.yaml
> new file mode 100644
> index 000000000000..055e09129f86
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mfd/delta,tn48m-cpld.yaml
> @@ -0,0 +1,81 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/mfd/delta,tn48m-cpld.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Delta Networks TN48M CPLD controller
> +
> +maintainers:
> +  - Robert Marko <robert.marko@sartura.hr>
> +
> +description: |
> +  Lattice CPLD onboard the TN48M switches is used for system
> +  management.
> +
> +  It provides information about the hardware model, revision,
> +  PSU status etc.
> +
> +  It is also being used as a GPIO expander for the SFP slots.
> +
> +properties:
> +  compatible:
> +    const: delta,tn48m-cpld
> +
> +  reg:
> +    description:
> +      I2C device address.
> +    maxItems: 1
> +
> +  "#address-cells":
> +    const: 1
> +
> +  "#size-cells":
> +    const: 0
> +
> +required:
> +  - compatible
> +  - reg
> +  - "#address-cells"
> +  - "#size-cells"
> +
> +patternProperties:
> +  "^gpio(@[0-9a-f]+)?$":
> +    $ref: ../gpio/delta,tn48m-gpio.yaml
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    i2c {
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +
> +        cpld@41 {
> +            compatible = "delta,tn48m-cpld";
> +            reg = <0x41>;
> +            #address-cells = <1>;
> +            #size-cells = <0>;
> +
> +            gpio@31 {
> +                compatible = "delta,tn48m-gpio-sfp-tx-disable";
> +                reg = <0x31>;

Encode the register address into the gpio cells.

> +                gpio-controller;
> +                #gpio-cells = <2>;
> +            };
> +
> +            gpio@3a {
> +                compatible = "delta,tn48m-gpio-sfp-present";
> +                reg = <0x3a>;
> +                gpio-controller;
> +                #gpio-cells = <2>;
> +            };
> +
> +            gpio@40 {
> +                compatible = "delta,tn48m-gpio-sfp-los";
> +                reg = <0x40>;
> +                gpio-controller;
> +                #gpio-cells = <2>;
> +            };
> +        };
> +    };
> -- 
> 2.31.1
> 

Re: [PATCH v2 3/4] dt-bindings: mfd: Add Delta TN48M CPLD drivers bindings

[Lee Jones]

On Mon, 24 May 2021, Rob Herring wrote:

> On Mon, May 24, 2021 at 02:05:38PM +0200, Robert Marko wrote:
> > Add binding documents for the Delta TN48M CPLD drivers.
> > 
> > Signed-off-by: Robert Marko <robert.marko@sartura.hr>
> > ---
> > Changes in v2:
> > * Implement MFD as a simple I2C MFD
> > * Add GPIO bindings as separate
> 
> I don't understand why this changed. This doesn't look like an MFD to 
> me. Make your binding complete if there are missing functions. 
> Otherwise, stick with what I already ok'ed.

Right.  What else, besides GPIO, does this do?

> >  .../bindings/gpio/delta,tn48m-gpio.yaml       | 42 ++++++++++
> >  .../bindings/mfd/delta,tn48m-cpld.yaml        | 81 +++++++++++++++++++
> >  2 files changed, 123 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/gpio/delta,tn48m-gpio.yaml
> >  create mode 100644 Documentation/devicetree/bindings/mfd/delta,tn48m-cpld.yaml

-- 
Lee Jones [李琼斯]
Senior Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH v2 3/4] dt-bindings: mfd: Add Delta TN48M CPLD drivers bindings

[Robert Marko]

On Tue, May 25, 2021 at 9:46 AM Lee Jones <lee.jones@linaro.org> wrote:
>
> On Mon, 24 May 2021, Rob Herring wrote:
>
> > On Mon, May 24, 2021 at 02:05:38PM +0200, Robert Marko wrote:
> > > Add binding documents for the Delta TN48M CPLD drivers.
> > >
> > > Signed-off-by: Robert Marko <robert.marko@sartura.hr>
> > > ---
> > > Changes in v2:
> > > * Implement MFD as a simple I2C MFD
> > > * Add GPIO bindings as separate
> >
> > I don't understand why this changed. This doesn't look like an MFD to
> > me. Make your binding complete if there are missing functions.
> > Otherwise, stick with what I already ok'ed.
>
> Right.  What else, besides GPIO, does this do?

It currently does not do anything else as hwmon driver was essentially
NACK-ed for not exposing standard attributes.
The CPLD itself has PSU status-related information, bootstrap related
information,
various resets for the CPU-s, OOB ethernet PHY, information on the exact board
model it's running etc.

PSU and model-related info stuff is gonna be exposed via a misc driver
in debugfs as
we have user-space SW depending on that.
I thought we agreed on that as v1 MFD driver was exposing those directly and
not doing anything else.
So I moved to use the simple I2C MFD driver, this is all modeled on the sl28cpld
which currently uses the same driver and then GPIO regmap as I do.

Other stuff like the resets is probably gonna get exposed later when
it's required
to control it directly.

Regards,
Robert
>
> > >  .../bindings/gpio/delta,tn48m-gpio.yaml       | 42 ++++++++++
> > >  .../bindings/mfd/delta,tn48m-cpld.yaml        | 81 +++++++++++++++++++
> > >  2 files changed, 123 insertions(+)
> > >  create mode 100644 Documentation/devicetree/bindings/gpio/delta,tn48m-gpio.yaml
> > >  create mode 100644 Documentation/devicetree/bindings/mfd/delta,tn48m-cpld.yaml
>
> --
> Lee Jones [李琼斯]
> Senior Technical Lead - Developer Services
> Linaro.org │ Open source software for Arm SoCs
> Follow Linaro: Facebook | Twitter | Blog



-- 
Robert Marko
Staff Embedded Linux Engineer
Sartura Ltd.
Lendavska ulica 16a
10000 Zagreb, Croatia
Email: robert.marko@sartura.hr
Web: www.sartura.hr

Re: [PATCH v2 3/4] dt-bindings: mfd: Add Delta TN48M CPLD drivers bindings

[Lee Jones]

On Tue, 25 May 2021, Robert Marko wrote:

> On Tue, May 25, 2021 at 9:46 AM Lee Jones <lee.jones@linaro.org> wrote:
> >
> > On Mon, 24 May 2021, Rob Herring wrote:
> >
> > > On Mon, May 24, 2021 at 02:05:38PM +0200, Robert Marko wrote:
> > > > Add binding documents for the Delta TN48M CPLD drivers.
> > > >
> > > > Signed-off-by: Robert Marko <robert.marko@sartura.hr>
> > > > ---
> > > > Changes in v2:
> > > > * Implement MFD as a simple I2C MFD
> > > > * Add GPIO bindings as separate
> > >
> > > I don't understand why this changed. This doesn't look like an MFD to
> > > me. Make your binding complete if there are missing functions.
> > > Otherwise, stick with what I already ok'ed.
> >
> > Right.  What else, besides GPIO, does this do?
> 
> It currently does not do anything else as hwmon driver was essentially
> NACK-ed for not exposing standard attributes.

Once this provides more than GPIO capabilities i.e. becomes a proper
Multi-Function Device, then it can use the MFD framework.  Until then,
it's a GPIO device I'm afraid.

Are you going to re-author the HWMON driver to conform?

> The CPLD itself has PSU status-related information, bootstrap related
> information,
> various resets for the CPU-s, OOB ethernet PHY, information on the exact board
> model it's running etc.
> 
> PSU and model-related info stuff is gonna be exposed via a misc driver
> in debugfs as
> we have user-space SW depending on that.
> I thought we agreed on that as v1 MFD driver was exposing those directly and
> not doing anything else.

Yes, we agreed that creating an MFD driver just to expose chip
attributes was not an acceptable solution.

> So I moved to use the simple I2C MFD driver, this is all modeled on the sl28cpld
> which currently uses the same driver and then GPIO regmap as I do.
> 
> Other stuff like the resets is probably gonna get exposed later when
> it's required
> to control it directly.

In order for this driver to tick the MFD box, it's going to need more
than one function.

> > > >  .../bindings/gpio/delta,tn48m-gpio.yaml       | 42 ++++++++++
> > > >  .../bindings/mfd/delta,tn48m-cpld.yaml        | 81 +++++++++++++++++++
> > > >  2 files changed, 123 insertions(+)
> > > >  create mode 100644 Documentation/devicetree/bindings/gpio/delta,tn48m-gpio.yaml
> > > >  create mode 100644 Documentation/devicetree/bindings/mfd/delta,tn48m-cpld.yaml
> >
> 
> 
> 

-- 
Lee Jones [李琼斯]
Senior Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH v2 3/4] dt-bindings: mfd: Add Delta TN48M CPLD drivers bindings

[Robert Marko]

On Wed, May 26, 2021 at 9:52 AM Lee Jones <lee.jones@linaro.org> wrote:
>
> On Tue, 25 May 2021, Robert Marko wrote:
>
> > On Tue, May 25, 2021 at 9:46 AM Lee Jones <lee.jones@linaro.org> wrote:
> > >
> > > On Mon, 24 May 2021, Rob Herring wrote:
> > >
> > > > On Mon, May 24, 2021 at 02:05:38PM +0200, Robert Marko wrote:
> > > > > Add binding documents for the Delta TN48M CPLD drivers.
> > > > >
> > > > > Signed-off-by: Robert Marko <robert.marko@sartura.hr>
> > > > > ---
> > > > > Changes in v2:
> > > > > * Implement MFD as a simple I2C MFD
> > > > > * Add GPIO bindings as separate
> > > >
> > > > I don't understand why this changed. This doesn't look like an MFD to
> > > > me. Make your binding complete if there are missing functions.
> > > > Otherwise, stick with what I already ok'ed.
> > >
> > > Right.  What else, besides GPIO, does this do?
> >
> > It currently does not do anything else as hwmon driver was essentially
> > NACK-ed for not exposing standard attributes.
>
> Once this provides more than GPIO capabilities i.e. becomes a proper
> Multi-Function Device, then it can use the MFD framework.  Until then,
> it's a GPIO device I'm afraid.
>
> Are you going to re-author the HWMON driver to conform?
hwmon cannot be reathored as it has no standard hwmon attributes.

>
> > The CPLD itself has PSU status-related information, bootstrap related
> > information,
> > various resets for the CPU-s, OOB ethernet PHY, information on the exact board
> > model it's running etc.
> >
> > PSU and model-related info stuff is gonna be exposed via a misc driver
> > in debugfs as
> > we have user-space SW depending on that.
> > I thought we agreed on that as v1 MFD driver was exposing those directly and
> > not doing anything else.
>
> Yes, we agreed that creating an MFD driver just to expose chip
> attributes was not an acceptable solution.
>
> > So I moved to use the simple I2C MFD driver, this is all modeled on the sl28cpld
> > which currently uses the same driver and then GPIO regmap as I do.
> >
> > Other stuff like the resets is probably gonna get exposed later when
> > it's required
> > to control it directly.
>
> In order for this driver to tick the MFD box, it's going to need more
> than one function.

Understood, would a debug driver count or I can expose the resets via
a reset driver
as we have a future use for them?

Regards,
Robert
>
> > > > >  .../bindings/gpio/delta,tn48m-gpio.yaml       | 42 ++++++++++
> > > > >  .../bindings/mfd/delta,tn48m-cpld.yaml        | 81 +++++++++++++++++++
> > > > >  2 files changed, 123 insertions(+)
> > > > >  create mode 100644 Documentation/devicetree/bindings/gpio/delta,tn48m-gpio.yaml
> > > > >  create mode 100644 Documentation/devicetree/bindings/mfd/delta,tn48m-cpld.yaml
> > >
> >
> >
> >
>
> --
> Lee Jones [李琼斯]
> Senior Technical Lead - Developer Services
> Linaro.org │ Open source software for Arm SoCs
> Follow Linaro: Facebook | Twitter | Blog



-- 
Robert Marko
Staff Embedded Linux Engineer
Sartura Ltd.
Lendavska ulica 16a
10000 Zagreb, Croatia
Email: robert.marko@sartura.hr
Web: www.sartura.hr

Re: [PATCH v2 3/4] dt-bindings: mfd: Add Delta TN48M CPLD drivers bindings

[Lee Jones]

On Mon, 31 May 2021, Robert Marko wrote:

> On Wed, May 26, 2021 at 9:52 AM Lee Jones <lee.jones@linaro.org> wrote:
> >
> > On Tue, 25 May 2021, Robert Marko wrote:
> >
> > > On Tue, May 25, 2021 at 9:46 AM Lee Jones <lee.jones@linaro.org> wrote:
> > > >
> > > > On Mon, 24 May 2021, Rob Herring wrote:
> > > >
> > > > > On Mon, May 24, 2021 at 02:05:38PM +0200, Robert Marko wrote:
> > > > > > Add binding documents for the Delta TN48M CPLD drivers.
> > > > > >
> > > > > > Signed-off-by: Robert Marko <robert.marko@sartura.hr>
> > > > > > ---
> > > > > > Changes in v2:
> > > > > > * Implement MFD as a simple I2C MFD
> > > > > > * Add GPIO bindings as separate
> > > > >
> > > > > I don't understand why this changed. This doesn't look like an MFD to
> > > > > me. Make your binding complete if there are missing functions.
> > > > > Otherwise, stick with what I already ok'ed.
> > > >
> > > > Right.  What else, besides GPIO, does this do?
> > >
> > > It currently does not do anything else as hwmon driver was essentially
> > > NACK-ed for not exposing standard attributes.
> >
> > Once this provides more than GPIO capabilities i.e. becomes a proper
> > Multi-Function Device, then it can use the MFD framework.  Until then,
> > it's a GPIO device I'm afraid.
> >
> > Are you going to re-author the HWMON driver to conform?
> hwmon cannot be reathored as it has no standard hwmon attributes.
> 
> >
> > > The CPLD itself has PSU status-related information, bootstrap related
> > > information,
> > > various resets for the CPU-s, OOB ethernet PHY, information on the exact board
> > > model it's running etc.
> > >
> > > PSU and model-related info stuff is gonna be exposed via a misc driver
> > > in debugfs as
> > > we have user-space SW depending on that.
> > > I thought we agreed on that as v1 MFD driver was exposing those directly and
> > > not doing anything else.
> >
> > Yes, we agreed that creating an MFD driver just to expose chip
> > attributes was not an acceptable solution.
> >
> > > So I moved to use the simple I2C MFD driver, this is all modeled on the sl28cpld
> > > which currently uses the same driver and then GPIO regmap as I do.
> > >
> > > Other stuff like the resets is probably gonna get exposed later when
> > > it's required
> > > to control it directly.
> >
> > In order for this driver to tick the MFD box, it's going to need more
> > than one function.
> 
> Understood, would a debug driver count or I can expose the resets via
> a reset driver
> as we have a future use for them?

CPLDs and FPGAs are funny ones and are often difficult to support in
Linux.  Especially if they can change their behaviour.

It's hard to make a solid suggestion as to how your device is handled
without knowing the intricacies of the device.

Why do you require one single Regmap anyway?  Are they register banks
not neatly separated on a per-function basis?

-- 
Lee Jones [李琼斯]
Senior Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH v2 3/4] dt-bindings: mfd: Add Delta TN48M CPLD drivers bindings

[Lee Jones]

On Tue, 01 Jun 2021, Lee Jones wrote:

> On Mon, 31 May 2021, Robert Marko wrote:
> 
> > On Wed, May 26, 2021 at 9:52 AM Lee Jones <lee.jones@linaro.org> wrote:
> > >
> > > On Tue, 25 May 2021, Robert Marko wrote:
> > >
> > > > On Tue, May 25, 2021 at 9:46 AM Lee Jones <lee.jones@linaro.org> wrote:
> > > > >
> > > > > On Mon, 24 May 2021, Rob Herring wrote:
> > > > >
> > > > > > On Mon, May 24, 2021 at 02:05:38PM +0200, Robert Marko wrote:
> > > > > > > Add binding documents for the Delta TN48M CPLD drivers.
> > > > > > >
> > > > > > > Signed-off-by: Robert Marko <robert.marko@sartura.hr>
> > > > > > > ---
> > > > > > > Changes in v2:
> > > > > > > * Implement MFD as a simple I2C MFD
> > > > > > > * Add GPIO bindings as separate
> > > > > >
> > > > > > I don't understand why this changed. This doesn't look like an MFD to
> > > > > > me. Make your binding complete if there are missing functions.
> > > > > > Otherwise, stick with what I already ok'ed.
> > > > >
> > > > > Right.  What else, besides GPIO, does this do?
> > > >
> > > > It currently does not do anything else as hwmon driver was essentially
> > > > NACK-ed for not exposing standard attributes.
> > >
> > > Once this provides more than GPIO capabilities i.e. becomes a proper
> > > Multi-Function Device, then it can use the MFD framework.  Until then,
> > > it's a GPIO device I'm afraid.
> > >
> > > Are you going to re-author the HWMON driver to conform?
> > hwmon cannot be reathored as it has no standard hwmon attributes.
> > 
> > >
> > > > The CPLD itself has PSU status-related information, bootstrap related
> > > > information,
> > > > various resets for the CPU-s, OOB ethernet PHY, information on the exact board
> > > > model it's running etc.
> > > >
> > > > PSU and model-related info stuff is gonna be exposed via a misc driver
> > > > in debugfs as
> > > > we have user-space SW depending on that.
> > > > I thought we agreed on that as v1 MFD driver was exposing those directly and
> > > > not doing anything else.
> > >
> > > Yes, we agreed that creating an MFD driver just to expose chip
> > > attributes was not an acceptable solution.
> > >
> > > > So I moved to use the simple I2C MFD driver, this is all modeled on the sl28cpld
> > > > which currently uses the same driver and then GPIO regmap as I do.
> > > >
> > > > Other stuff like the resets is probably gonna get exposed later when
> > > > it's required
> > > > to control it directly.
> > >
> > > In order for this driver to tick the MFD box, it's going to need more
> > > than one function.
> > 
> > Understood, would a debug driver count or I can expose the resets via
> > a reset driver
> > as we have a future use for them?
> 
> CPLDs and FPGAs are funny ones and are often difficult to support in
> Linux.  Especially if they can change their behaviour.
> 
> It's hard to make a solid suggestion as to how your device is handled
> without knowing the intricacies of the device.
> 
> Why do you require one single Regmap anyway?  Are they register banks
> not neatly separated on a per-function basis?

Also, if this is really just a GPIO expander, can't the GPIO driver
output something to /sysfs that identifies it to userspace instead?

-- 
Lee Jones [李琼斯]
Senior Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH v2 3/4] dt-bindings: mfd: Add Delta TN48M CPLD drivers bindings

[Robert Marko]

On Tue, Jun 1, 2021 at 10:22 AM Lee Jones <lee.jones@linaro.org> wrote:
>
> On Tue, 01 Jun 2021, Lee Jones wrote:
>
> > On Mon, 31 May 2021, Robert Marko wrote:
> >
> > > On Wed, May 26, 2021 at 9:52 AM Lee Jones <lee.jones@linaro.org> wrote:
> > > >
> > > > On Tue, 25 May 2021, Robert Marko wrote:
> > > >
> > > > > On Tue, May 25, 2021 at 9:46 AM Lee Jones <lee.jones@linaro.org> wrote:
> > > > > >
> > > > > > On Mon, 24 May 2021, Rob Herring wrote:
> > > > > >
> > > > > > > On Mon, May 24, 2021 at 02:05:38PM +0200, Robert Marko wrote:
> > > > > > > > Add binding documents for the Delta TN48M CPLD drivers.
> > > > > > > >
> > > > > > > > Signed-off-by: Robert Marko <robert.marko@sartura.hr>
> > > > > > > > ---
> > > > > > > > Changes in v2:
> > > > > > > > * Implement MFD as a simple I2C MFD
> > > > > > > > * Add GPIO bindings as separate
> > > > > > >
> > > > > > > I don't understand why this changed. This doesn't look like an MFD to
> > > > > > > me. Make your binding complete if there are missing functions.
> > > > > > > Otherwise, stick with what I already ok'ed.
> > > > > >
> > > > > > Right.  What else, besides GPIO, does this do?
> > > > >
> > > > > It currently does not do anything else as hwmon driver was essentially
> > > > > NACK-ed for not exposing standard attributes.
> > > >
> > > > Once this provides more than GPIO capabilities i.e. becomes a proper
> > > > Multi-Function Device, then it can use the MFD framework.  Until then,
> > > > it's a GPIO device I'm afraid.
> > > >
> > > > Are you going to re-author the HWMON driver to conform?
> > > hwmon cannot be reathored as it has no standard hwmon attributes.
> > >
> > > >
> > > > > The CPLD itself has PSU status-related information, bootstrap related
> > > > > information,
> > > > > various resets for the CPU-s, OOB ethernet PHY, information on the exact board
> > > > > model it's running etc.
> > > > >
> > > > > PSU and model-related info stuff is gonna be exposed via a misc driver
> > > > > in debugfs as
> > > > > we have user-space SW depending on that.
> > > > > I thought we agreed on that as v1 MFD driver was exposing those directly and
> > > > > not doing anything else.
> > > >
> > > > Yes, we agreed that creating an MFD driver just to expose chip
> > > > attributes was not an acceptable solution.
> > > >
> > > > > So I moved to use the simple I2C MFD driver, this is all modeled on the sl28cpld
> > > > > which currently uses the same driver and then GPIO regmap as I do.
> > > > >
> > > > > Other stuff like the resets is probably gonna get exposed later when
> > > > > it's required
> > > > > to control it directly.
> > > >
> > > > In order for this driver to tick the MFD box, it's going to need more
> > > > than one function.
> > >
> > > Understood, would a debug driver count or I can expose the resets via
> > > a reset driver
> > > as we have a future use for them?
> >
> > CPLDs and FPGAs are funny ones and are often difficult to support in
> > Linux.  Especially if they can change their behaviour.
> >
> > It's hard to make a solid suggestion as to how your device is handled
> > without knowing the intricacies of the device.
> >
> > Why do you require one single Regmap anyway?  Are they register banks
> > not neatly separated on a per-function basis?
>
> Also, if this is really just a GPIO expander, can't the GPIO driver
> output something to /sysfs that identifies it to userspace instead?

I replied to your previous reply instead of this one directly.
It's not just a GPIO expander, it also provides resets to all of the HW
and a lot of debugging information.
Note that other switches use the same CPLD but with more features
so I want to just extend these drivers and add for example hwmon.

It's not just about it identifying itself, it offers a lot of various
debug info,
quite literally down to what CPU has access to the serial console on the
front and their bootstrap pins.

So, I want to expose the CPLD version, code version, switch model,
PSU status pins and a lot more using a separate driver as they
don't really belong to any other subsystem than misc using debugfs.

I hope this clears things up,
Robert
>
> --
> Lee Jones [李琼斯]
> Senior Technical Lead - Developer Services
> Linaro.org │ Open source software for Arm SoCs
> Follow Linaro: Facebook | Twitter | Blog



-- 
Robert Marko
Staff Embedded Linux Engineer
Sartura Ltd.
Lendavska ulica 16a
10000 Zagreb, Croatia
Email: robert.marko@sartura.hr
Web: www.sartura.hr

Re: [PATCH v2 3/4] dt-bindings: mfd: Add Delta TN48M CPLD drivers bindings

[Lee Jones]

On Tue, 01 Jun 2021, Robert Marko wrote:

> On Tue, Jun 1, 2021 at 10:22 AM Lee Jones <lee.jones@linaro.org> wrote:
> >
> > On Tue, 01 Jun 2021, Lee Jones wrote:
> >
> > > On Mon, 31 May 2021, Robert Marko wrote:
> > >
> > > > On Wed, May 26, 2021 at 9:52 AM Lee Jones <lee.jones@linaro.org> wrote:
> > > > >
> > > > > On Tue, 25 May 2021, Robert Marko wrote:
> > > > >
> > > > > > On Tue, May 25, 2021 at 9:46 AM Lee Jones <lee.jones@linaro.org> wrote:
> > > > > > >
> > > > > > > On Mon, 24 May 2021, Rob Herring wrote:
> > > > > > >
> > > > > > > > On Mon, May 24, 2021 at 02:05:38PM +0200, Robert Marko wrote:
> > > > > > > > > Add binding documents for the Delta TN48M CPLD drivers.
> > > > > > > > >
> > > > > > > > > Signed-off-by: Robert Marko <robert.marko@sartura.hr>
> > > > > > > > > ---
> > > > > > > > > Changes in v2:
> > > > > > > > > * Implement MFD as a simple I2C MFD
> > > > > > > > > * Add GPIO bindings as separate
> > > > > > > >
> > > > > > > > I don't understand why this changed. This doesn't look like an MFD to
> > > > > > > > me. Make your binding complete if there are missing functions.
> > > > > > > > Otherwise, stick with what I already ok'ed.
> > > > > > >
> > > > > > > Right.  What else, besides GPIO, does this do?
> > > > > >
> > > > > > It currently does not do anything else as hwmon driver was essentially
> > > > > > NACK-ed for not exposing standard attributes.
> > > > >
> > > > > Once this provides more than GPIO capabilities i.e. becomes a proper
> > > > > Multi-Function Device, then it can use the MFD framework.  Until then,
> > > > > it's a GPIO device I'm afraid.
> > > > >
> > > > > Are you going to re-author the HWMON driver to conform?
> > > > hwmon cannot be reathored as it has no standard hwmon attributes.
> > > >
> > > > >
> > > > > > The CPLD itself has PSU status-related information, bootstrap related
> > > > > > information,
> > > > > > various resets for the CPU-s, OOB ethernet PHY, information on the exact board
> > > > > > model it's running etc.
> > > > > >
> > > > > > PSU and model-related info stuff is gonna be exposed via a misc driver
> > > > > > in debugfs as
> > > > > > we have user-space SW depending on that.
> > > > > > I thought we agreed on that as v1 MFD driver was exposing those directly and
> > > > > > not doing anything else.
> > > > >
> > > > > Yes, we agreed that creating an MFD driver just to expose chip
> > > > > attributes was not an acceptable solution.
> > > > >
> > > > > > So I moved to use the simple I2C MFD driver, this is all modeled on the sl28cpld
> > > > > > which currently uses the same driver and then GPIO regmap as I do.
> > > > > >
> > > > > > Other stuff like the resets is probably gonna get exposed later when
> > > > > > it's required
> > > > > > to control it directly.
> > > > >
> > > > > In order for this driver to tick the MFD box, it's going to need more
> > > > > than one function.
> > > >
> > > > Understood, would a debug driver count or I can expose the resets via
> > > > a reset driver
> > > > as we have a future use for them?
> > >
> > > CPLDs and FPGAs are funny ones and are often difficult to support in
> > > Linux.  Especially if they can change their behaviour.
> > >
> > > It's hard to make a solid suggestion as to how your device is handled
> > > without knowing the intricacies of the device.
> > >
> > > Why do you require one single Regmap anyway?  Are they register banks
> > > not neatly separated on a per-function basis?
> >
> > Also, if this is really just a GPIO expander, can't the GPIO driver
> > output something to /sysfs that identifies it to userspace instead?
> 
> I replied to your previous reply instead of this one directly.
> It's not just a GPIO expander, it also provides resets to all of the HW
> and a lot of debugging information.
> Note that other switches use the same CPLD but with more features
> so I want to just extend these drivers and add for example hwmon.
> 
> It's not just about it identifying itself, it offers a lot of various
> debug info,
> quite literally down to what CPU has access to the serial console on the
> front and their bootstrap pins.
> 
> So, I want to expose the CPLD version, code version, switch model,
> PSU status pins and a lot more using a separate driver as they
> don't really belong to any other subsystem than misc using debugfs.

drivers/soc is also an option for devices like these.

-- 
Lee Jones [李琼斯]
Senior Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH v2 3/4] dt-bindings: mfd: Add Delta TN48M CPLD drivers bindings

[Robert Marko]

On Tue, Jun 1, 2021 at 11:31 AM Lee Jones <lee.jones@linaro.org> wrote:
>
> On Tue, 01 Jun 2021, Robert Marko wrote:
>
> > On Tue, Jun 1, 2021 at 10:22 AM Lee Jones <lee.jones@linaro.org> wrote:
> > >
> > > On Tue, 01 Jun 2021, Lee Jones wrote:
> > >
> > > > On Mon, 31 May 2021, Robert Marko wrote:
> > > >
> > > > > On Wed, May 26, 2021 at 9:52 AM Lee Jones <lee.jones@linaro.org> wrote:
> > > > > >
> > > > > > On Tue, 25 May 2021, Robert Marko wrote:
> > > > > >
> > > > > > > On Tue, May 25, 2021 at 9:46 AM Lee Jones <lee.jones@linaro.org> wrote:
> > > > > > > >
> > > > > > > > On Mon, 24 May 2021, Rob Herring wrote:
> > > > > > > >
> > > > > > > > > On Mon, May 24, 2021 at 02:05:38PM +0200, Robert Marko wrote:
> > > > > > > > > > Add binding documents for the Delta TN48M CPLD drivers.
> > > > > > > > > >
> > > > > > > > > > Signed-off-by: Robert Marko <robert.marko@sartura.hr>
> > > > > > > > > > ---
> > > > > > > > > > Changes in v2:
> > > > > > > > > > * Implement MFD as a simple I2C MFD
> > > > > > > > > > * Add GPIO bindings as separate
> > > > > > > > >
> > > > > > > > > I don't understand why this changed. This doesn't look like an MFD to
> > > > > > > > > me. Make your binding complete if there are missing functions.
> > > > > > > > > Otherwise, stick with what I already ok'ed.
> > > > > > > >
> > > > > > > > Right.  What else, besides GPIO, does this do?
> > > > > > >
> > > > > > > It currently does not do anything else as hwmon driver was essentially
> > > > > > > NACK-ed for not exposing standard attributes.
> > > > > >
> > > > > > Once this provides more than GPIO capabilities i.e. becomes a proper
> > > > > > Multi-Function Device, then it can use the MFD framework.  Until then,
> > > > > > it's a GPIO device I'm afraid.
> > > > > >
> > > > > > Are you going to re-author the HWMON driver to conform?
> > > > > hwmon cannot be reathored as it has no standard hwmon attributes.
> > > > >
> > > > > >
> > > > > > > The CPLD itself has PSU status-related information, bootstrap related
> > > > > > > information,
> > > > > > > various resets for the CPU-s, OOB ethernet PHY, information on the exact board
> > > > > > > model it's running etc.
> > > > > > >
> > > > > > > PSU and model-related info stuff is gonna be exposed via a misc driver
> > > > > > > in debugfs as
> > > > > > > we have user-space SW depending on that.
> > > > > > > I thought we agreed on that as v1 MFD driver was exposing those directly and
> > > > > > > not doing anything else.
> > > > > >
> > > > > > Yes, we agreed that creating an MFD driver just to expose chip
> > > > > > attributes was not an acceptable solution.
> > > > > >
> > > > > > > So I moved to use the simple I2C MFD driver, this is all modeled on the sl28cpld
> > > > > > > which currently uses the same driver and then GPIO regmap as I do.
> > > > > > >
> > > > > > > Other stuff like the resets is probably gonna get exposed later when
> > > > > > > it's required
> > > > > > > to control it directly.
> > > > > >
> > > > > > In order for this driver to tick the MFD box, it's going to need more
> > > > > > than one function.
> > > > >
> > > > > Understood, would a debug driver count or I can expose the resets via
> > > > > a reset driver
> > > > > as we have a future use for them?
> > > >
> > > > CPLDs and FPGAs are funny ones and are often difficult to support in
> > > > Linux.  Especially if they can change their behaviour.
> > > >
> > > > It's hard to make a solid suggestion as to how your device is handled
> > > > without knowing the intricacies of the device.
> > > >
> > > > Why do you require one single Regmap anyway?  Are they register banks
> > > > not neatly separated on a per-function basis?
> > >
> > > Also, if this is really just a GPIO expander, can't the GPIO driver
> > > output something to /sysfs that identifies it to userspace instead?
> >
> > I replied to your previous reply instead of this one directly.
> > It's not just a GPIO expander, it also provides resets to all of the HW
> > and a lot of debugging information.
> > Note that other switches use the same CPLD but with more features
> > so I want to just extend these drivers and add for example hwmon.
> >
> > It's not just about it identifying itself, it offers a lot of various
> > debug info,
> > quite literally down to what CPU has access to the serial console on the
> > front and their bootstrap pins.
> >
> > So, I want to expose the CPLD version, code version, switch model,
> > PSU status pins and a lot more using a separate driver as they
> > don't really belong to any other subsystem than misc using debugfs.
>
> drivers/soc is also an option for devices like these.

I have completely forgotten about that, it's a potential place.

Regards,
Robert
>
> --
> Lee Jones [李琼斯]
> Senior Technical Lead - Developer Services
> Linaro.org │ Open source software for Arm SoCs
> Follow Linaro: Facebook | Twitter | Blog



-- 
Robert Marko
Staff Embedded Linux Engineer
Sartura Ltd.
Lendavska ulica 16a
10000 Zagreb, Croatia
Email: robert.marko@sartura.hr
Web: www.sartura.hr

Re: [PATCH v2 3/4] dt-bindings: mfd: Add Delta TN48M CPLD drivers bindings

[Robert Marko]

On Tue, Jun 1, 2021 at 10:19 AM Lee Jones <lee.jones@linaro.org> wrote:
>
> On Mon, 31 May 2021, Robert Marko wrote:
>
> > On Wed, May 26, 2021 at 9:52 AM Lee Jones <lee.jones@linaro.org> wrote:
> > >
> > > On Tue, 25 May 2021, Robert Marko wrote:
> > >
> > > > On Tue, May 25, 2021 at 9:46 AM Lee Jones <lee.jones@linaro.org> wrote:
> > > > >
> > > > > On Mon, 24 May 2021, Rob Herring wrote:
> > > > >
> > > > > > On Mon, May 24, 2021 at 02:05:38PM +0200, Robert Marko wrote:
> > > > > > > Add binding documents for the Delta TN48M CPLD drivers.
> > > > > > >
> > > > > > > Signed-off-by: Robert Marko <robert.marko@sartura.hr>
> > > > > > > ---
> > > > > > > Changes in v2:
> > > > > > > * Implement MFD as a simple I2C MFD
> > > > > > > * Add GPIO bindings as separate
> > > > > >
> > > > > > I don't understand why this changed. This doesn't look like an MFD to
> > > > > > me. Make your binding complete if there are missing functions.
> > > > > > Otherwise, stick with what I already ok'ed.
> > > > >
> > > > > Right.  What else, besides GPIO, does this do?
> > > >
> > > > It currently does not do anything else as hwmon driver was essentially
> > > > NACK-ed for not exposing standard attributes.
> > >
> > > Once this provides more than GPIO capabilities i.e. becomes a proper
> > > Multi-Function Device, then it can use the MFD framework.  Until then,
> > > it's a GPIO device I'm afraid.
> > >
> > > Are you going to re-author the HWMON driver to conform?
> > hwmon cannot be reathored as it has no standard hwmon attributes.
> >
> > >
> > > > The CPLD itself has PSU status-related information, bootstrap related
> > > > information,
> > > > various resets for the CPU-s, OOB ethernet PHY, information on the exact board
> > > > model it's running etc.
> > > >
> > > > PSU and model-related info stuff is gonna be exposed via a misc driver
> > > > in debugfs as
> > > > we have user-space SW depending on that.
> > > > I thought we agreed on that as v1 MFD driver was exposing those directly and
> > > > not doing anything else.
> > >
> > > Yes, we agreed that creating an MFD driver just to expose chip
> > > attributes was not an acceptable solution.
> > >
> > > > So I moved to use the simple I2C MFD driver, this is all modeled on the sl28cpld
> > > > which currently uses the same driver and then GPIO regmap as I do.
> > > >
> > > > Other stuff like the resets is probably gonna get exposed later when
> > > > it's required
> > > > to control it directly.
> > >
> > > In order for this driver to tick the MFD box, it's going to need more
> > > than one function.
> >
> > Understood, would a debug driver count or I can expose the resets via
> > a reset driver
> > as we have a future use for them?
>
> CPLDs and FPGAs are funny ones and are often difficult to support in
> Linux.  Especially if they can change their behaviour.
>
> It's hard to make a solid suggestion as to how your device is handled
> without knowing the intricacies of the device.

Yeah, I understand.
This one is a generic CPLD used in multiple switch models, however in this
switch model, it has the smallest set of features.
Things that are usable for the kernel and userspace it provides are SFP pins,
resets and debug information.
Debug information is quite detailed actually.

I have added the reset driver in v3 as that is something that was gonna get
added later as well as it exposes resets for the ethernet PHY-s and U-boot
messes with the OOB PHY configuration.

>
> Why do you require one single Regmap anyway?  Are they register banks
> not neatly separated on a per-function basis?

For GPIO and reset drivers, I could get away with each of them
registering a regmap
but the debug driver will require accessing certain registers from their space.
Also, I see using a single regmap that is provided by a generic driver
much simpler
and cleaner than doing that in each of the child drivers.

Regards,
Robert

>
> --
> Lee Jones [李琼斯]
> Senior Technical Lead - Developer Services
> Linaro.org │ Open source software for Arm SoCs
> Follow Linaro: Facebook | Twitter | Blog



-- 
Robert Marko
Staff Embedded Linux Engineer
Sartura Ltd.
Lendavska ulica 16a
10000 Zagreb, Croatia
Email: robert.marko@sartura.hr
Web: www.sartura.hr

Re: [PATCH v2 3/4] dt-bindings: mfd: Add Delta TN48M CPLD drivers bindings

[Lee Jones]

On Tue, 01 Jun 2021, Robert Marko wrote:

> On Tue, Jun 1, 2021 at 10:19 AM Lee Jones <lee.jones@linaro.org> wrote:
> >
> > On Mon, 31 May 2021, Robert Marko wrote:
> >
> > > On Wed, May 26, 2021 at 9:52 AM Lee Jones <lee.jones@linaro.org> wrote:
> > > >
> > > > On Tue, 25 May 2021, Robert Marko wrote:
> > > >
> > > > > On Tue, May 25, 2021 at 9:46 AM Lee Jones <lee.jones@linaro.org> wrote:
> > > > > >
> > > > > > On Mon, 24 May 2021, Rob Herring wrote:
> > > > > >
> > > > > > > On Mon, May 24, 2021 at 02:05:38PM +0200, Robert Marko wrote:
> > > > > > > > Add binding documents for the Delta TN48M CPLD drivers.
> > > > > > > >
> > > > > > > > Signed-off-by: Robert Marko <robert.marko@sartura.hr>
> > > > > > > > ---
> > > > > > > > Changes in v2:
> > > > > > > > * Implement MFD as a simple I2C MFD
> > > > > > > > * Add GPIO bindings as separate
> > > > > > >
> > > > > > > I don't understand why this changed. This doesn't look like an MFD to
> > > > > > > me. Make your binding complete if there are missing functions.
> > > > > > > Otherwise, stick with what I already ok'ed.
> > > > > >
> > > > > > Right.  What else, besides GPIO, does this do?
> > > > >
> > > > > It currently does not do anything else as hwmon driver was essentially
> > > > > NACK-ed for not exposing standard attributes.
> > > >
> > > > Once this provides more than GPIO capabilities i.e. becomes a proper
> > > > Multi-Function Device, then it can use the MFD framework.  Until then,
> > > > it's a GPIO device I'm afraid.
> > > >
> > > > Are you going to re-author the HWMON driver to conform?
> > > hwmon cannot be reathored as it has no standard hwmon attributes.
> > >
> > > >
> > > > > The CPLD itself has PSU status-related information, bootstrap related
> > > > > information,
> > > > > various resets for the CPU-s, OOB ethernet PHY, information on the exact board
> > > > > model it's running etc.
> > > > >
> > > > > PSU and model-related info stuff is gonna be exposed via a misc driver
> > > > > in debugfs as
> > > > > we have user-space SW depending on that.
> > > > > I thought we agreed on that as v1 MFD driver was exposing those directly and
> > > > > not doing anything else.
> > > >
> > > > Yes, we agreed that creating an MFD driver just to expose chip
> > > > attributes was not an acceptable solution.
> > > >
> > > > > So I moved to use the simple I2C MFD driver, this is all modeled on the sl28cpld
> > > > > which currently uses the same driver and then GPIO regmap as I do.
> > > > >
> > > > > Other stuff like the resets is probably gonna get exposed later when
> > > > > it's required
> > > > > to control it directly.
> > > >
> > > > In order for this driver to tick the MFD box, it's going to need more
> > > > than one function.
> > >
> > > Understood, would a debug driver count or I can expose the resets via
> > > a reset driver
> > > as we have a future use for them?
> >
> > CPLDs and FPGAs are funny ones and are often difficult to support in
> > Linux.  Especially if they can change their behaviour.
> >
> > It's hard to make a solid suggestion as to how your device is handled
> > without knowing the intricacies of the device.
> 
> Yeah, I understand.
> This one is a generic CPLD used in multiple switch models, however in this
> switch model, it has the smallest set of features.
> Things that are usable for the kernel and userspace it provides are SFP pins,
> resets and debug information.
> Debug information is quite detailed actually.
> 
> I have added the reset driver in v3 as that is something that was gonna get
> added later as well as it exposes resets for the ethernet PHY-s and U-boot
> messes with the OOB PHY configuration.
> 
> >
> > Why do you require one single Regmap anyway?  Are they register banks
> > not neatly separated on a per-function basis?
> 
> For GPIO and reset drivers, I could get away with each of them
> registering a regmap
> but the debug driver will require accessing certain registers from their space.

> Also, I see using a single regmap that is provided by a generic driver
> much simpler and cleaner than doing that in each of the child drivers.

Obviously not. :)

-- 
Lee Jones [李琼斯]
Senior Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH v2 3/4] dt-bindings: mfd: Add Delta TN48M CPLD drivers bindings

[Michael Walle]

Am 2021-06-01 10:19, schrieb Lee Jones:
> Why do you require one single Regmap anyway?  Are they register banks
> not neatly separated on a per-function basis?

AFAIK you can only have one I2C device driver per device, hence the
simple-mfd-i2c.

-michael

Re: [PATCH v2 3/4] dt-bindings: mfd: Add Delta TN48M CPLD drivers bindings

[Robert Marko]

On Tue, Jun 1, 2021 at 3:54 PM Michael Walle <michael@walle.cc> wrote:
>
> Am 2021-06-01 10:19, schrieb Lee Jones:
> > Why do you require one single Regmap anyway?  Are they register banks
> > not neatly separated on a per-function basis?
>
> AFAIK you can only have one I2C device driver per device, hence the
> simple-mfd-i2c.
>
> -michael

That is my understanding as well.

Regards,
Robert


-- 
Robert Marko
Staff Embedded Linux Engineer
Sartura Ltd.
Lendavska ulica 16a
10000 Zagreb, Croatia
Email: robert.marko@sartura.hr
Web: www.sartura.hr

Re: [PATCH v2 3/4] dt-bindings: mfd: Add Delta TN48M CPLD drivers bindings

[Lee Jones]

On Tue, 01 Jun 2021, Michael Walle wrote:

> Am 2021-06-01 10:19, schrieb Lee Jones:
> > Why do you require one single Regmap anyway?  Are they register banks
> > not neatly separated on a per-function basis?
> 
> AFAIK you can only have one I2C device driver per device, hence the
> simple-mfd-i2c.

Sorry, can you provide more detail.

-- 
Lee Jones [李琼斯]
Senior Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH v2 3/4] dt-bindings: mfd: Add Delta TN48M CPLD drivers bindings

[Lee Jones]

On Tue, 01 Jun 2021, Lee Jones wrote:

> On Tue, 01 Jun 2021, Michael Walle wrote:
> 
> > Am 2021-06-01 10:19, schrieb Lee Jones:
> > > Why do you require one single Regmap anyway?  Are they register banks
> > > not neatly separated on a per-function basis?
> > 
> > AFAIK you can only have one I2C device driver per device, hence the
> > simple-mfd-i2c.
> 
> Sorry, can you provide more detail.

I'd still like further explanation to be sure, but if you mean what I
think you mean then, no, I don't think that's correct.

The point of simple-mfd-i2c is to provide an I2C device offering
multiple functions, but does so via a non-separated/linear register-
set, with an entry point and an opportunity to register its interwoven 
bank of registers via Regmap.

However, if you can get away with not registering your entire register
set as a single Regmap chunk, then all the better.  This will allow
you to use the OF provided 'simple-mfd' compatible instead.

Now, if you're talking about Regmap not supporting multiple
registrations with only a single I2C address, this *may* very well be
the case, but IIRC, I've spoken to Mark about this previously and he
said the extension to make this possible would be trivial.

So we have to take this on a device-by-device basis an decide what is
best at the time of submission.

-- 
Lee Jones [李琼斯]
Senior Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH v2 3/4] dt-bindings: mfd: Add Delta TN48M CPLD drivers bindings

[Robert Marko]

On Tue, Jun 1, 2021 at 4:48 PM Lee Jones <lee.jones@linaro.org> wrote:
>
> On Tue, 01 Jun 2021, Lee Jones wrote:
>
> > On Tue, 01 Jun 2021, Michael Walle wrote:
> >
> > > Am 2021-06-01 10:19, schrieb Lee Jones:
> > > > Why do you require one single Regmap anyway?  Are they register banks
> > > > not neatly separated on a per-function basis?
> > >
> > > AFAIK you can only have one I2C device driver per device, hence the
> > > simple-mfd-i2c.
> >
> > Sorry, can you provide more detail.
>
> I'd still like further explanation to be sure, but if you mean what I
> think you mean then, no, I don't think that's correct.
>
> The point of simple-mfd-i2c is to provide an I2C device offering
> multiple functions, but does so via a non-separated/linear register-
> set, with an entry point and an opportunity to register its interwoven
> bank of registers via Regmap.
>
> However, if you can get away with not registering your entire register
> set as a single Regmap chunk, then all the better.  This will allow
> you to use the OF provided 'simple-mfd' compatible instead.
>
> Now, if you're talking about Regmap not supporting multiple
> registrations with only a single I2C address, this *may* very well be
> the case, but IIRC, I've spoken to Mark about this previously and he
> said the extension to make this possible would be trivial.

This is my understanding, that you cannot have multiple regmap registrations
with on the same I2C address.
At least that is how it was the last time I tested.
That is why I went the MFD way.

Regards,
Robert
>
> So we have to take this on a device-by-device basis an decide what is
> best at the time of submission.
>
> --
> Lee Jones [李琼斯]
> Senior Technical Lead - Developer Services
> Linaro.org │ Open source software for Arm SoCs
> Follow Linaro: Facebook | Twitter | Blog



-- 
Robert Marko
Staff Embedded Linux Engineer
Sartura Ltd.
Lendavska ulica 16a
10000 Zagreb, Croatia
Email: robert.marko@sartura.hr
Web: www.sartura.hr

Re: [PATCH v2 3/4] dt-bindings: mfd: Add Delta TN48M CPLD drivers bindings

[Lee Jones]

On Wed, 02 Jun 2021, Robert Marko wrote:

> On Tue, Jun 1, 2021 at 4:48 PM Lee Jones <lee.jones@linaro.org> wrote:
> >
> > On Tue, 01 Jun 2021, Lee Jones wrote:
> >
> > > On Tue, 01 Jun 2021, Michael Walle wrote:
> > >
> > > > Am 2021-06-01 10:19, schrieb Lee Jones:
> > > > > Why do you require one single Regmap anyway?  Are they register banks
> > > > > not neatly separated on a per-function basis?
> > > >
> > > > AFAIK you can only have one I2C device driver per device, hence the
> > > > simple-mfd-i2c.
> > >
> > > Sorry, can you provide more detail.
> >
> > I'd still like further explanation to be sure, but if you mean what I
> > think you mean then, no, I don't think that's correct.
> >
> > The point of simple-mfd-i2c is to provide an I2C device offering
> > multiple functions, but does so via a non-separated/linear register-
> > set, with an entry point and an opportunity to register its interwoven
> > bank of registers via Regmap.
> >
> > However, if you can get away with not registering your entire register
> > set as a single Regmap chunk, then all the better.  This will allow
> > you to use the OF provided 'simple-mfd' compatible instead.
> >
> > Now, if you're talking about Regmap not supporting multiple
> > registrations with only a single I2C address, this *may* very well be
> > the case, but IIRC, I've spoken to Mark about this previously and he
> > said the extension to make this possible would be trivial.
> 
> This is my understanding, that you cannot have multiple regmap registrations
> with on the same I2C address.
> At least that is how it was the last time I tested.
> That is why I went the MFD way.

I've just clarified with Mark.

There does not appear to be such a restriction.

-- 
Lee Jones [李琼斯]
Senior Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH v2 3/4] dt-bindings: mfd: Add Delta TN48M CPLD drivers bindings

[Michael Walle]

Am 2021-06-01 16:48, schrieb Lee Jones:
> On Tue, 01 Jun 2021, Lee Jones wrote:
> 
>> On Tue, 01 Jun 2021, Michael Walle wrote:
>> 
>> > Am 2021-06-01 10:19, schrieb Lee Jones:
>> > > Why do you require one single Regmap anyway?  Are they register banks
>> > > not neatly separated on a per-function basis?
>> >
>> > AFAIK you can only have one I2C device driver per device, hence the
>> > simple-mfd-i2c.
>> 
>> Sorry, can you provide more detail.
> 
> I'd still like further explanation to be sure, but if you mean what I
> think you mean then, no, I don't think that's correct.

We've already discussed this:

https://lore.kernel.org/lkml/20200622075145.1464020-1-lee.jones@linaro.org/
https://lore.kernel.org/lkml/20200605065709.GD3714@dell/

And how would a device tree binding look like if you have multiple
i2c devices with the same i2c address?

-michael

Re: [PATCH v2 3/4] dt-bindings: mfd: Add Delta TN48M CPLD drivers bindings

[Lee Jones]

On Wed, 02 Jun 2021, Michael Walle wrote:

> Am 2021-06-01 16:48, schrieb Lee Jones:
> > On Tue, 01 Jun 2021, Lee Jones wrote:
> > 
> > > On Tue, 01 Jun 2021, Michael Walle wrote:
> > > 
> > > > Am 2021-06-01 10:19, schrieb Lee Jones:
> > > > > Why do you require one single Regmap anyway?  Are they register banks
> > > > > not neatly separated on a per-function basis?
> > > >
> > > > AFAIK you can only have one I2C device driver per device, hence the
> > > > simple-mfd-i2c.
> > > 
> > > Sorry, can you provide more detail.
> > 
> > I'd still like further explanation to be sure, but if you mean what I
> > think you mean then, no, I don't think that's correct.
> 
> We've already discussed this:
> 
> https://lore.kernel.org/lkml/20200622075145.1464020-1-lee.jones@linaro.org/
> https://lore.kernel.org/lkml/20200605065709.GD3714@dell/
> 
> And how would a device tree binding look like if you have multiple
> i2c devices with the same i2c address?

Ah yes, I remember.

I suppose the saving grace of this scenario is the presence of the
Reset driver, since this confirms the device's MFD status.  If it were
missing however, I'm not entirely sure how we'd solve this issue.

I'll go have another look at the latest submission.  Bear with.

-- 
Lee Jones [李琼斯]
Senior Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH v2 3/4] dt-bindings: mfd: Add Delta TN48M CPLD drivers bindings

[Robert Marko]

On Tue, May 25, 2021 at 1:09 AM Rob Herring <robh@kernel.org> wrote:
>
> On Mon, May 24, 2021 at 02:05:38PM +0200, Robert Marko wrote:
> > Add binding documents for the Delta TN48M CPLD drivers.
> >
> > Signed-off-by: Robert Marko <robert.marko@sartura.hr>
> > ---
> > Changes in v2:
> > * Implement MFD as a simple I2C MFD
> > * Add GPIO bindings as separate
>
> I don't understand why this changed. This doesn't look like an MFD to
> me. Make your binding complete if there are missing functions.
> Otherwise, stick with what I already ok'ed.

It changed because the custom driver was dropped at Lee Jones-es request,
and simple-mfd-i2c is now used.

>
> >
> >  .../bindings/gpio/delta,tn48m-gpio.yaml       | 42 ++++++++++
> >  .../bindings/mfd/delta,tn48m-cpld.yaml        | 81 +++++++++++++++++++
> >  2 files changed, 123 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/gpio/delta,tn48m-gpio.yaml
> >  create mode 100644 Documentation/devicetree/bindings/mfd/delta,tn48m-cpld.yaml
> >
> > diff --git a/Documentation/devicetree/bindings/gpio/delta,tn48m-gpio.yaml b/Documentation/devicetree/bindings/gpio/delta,tn48m-gpio.yaml
> > new file mode 100644
> > index 000000000000..aca646aecb12
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/gpio/delta,tn48m-gpio.yaml
> > @@ -0,0 +1,42 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/gpio/delta,tn48m-gpio.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Delta Networks TN48M CPLD GPIO controller
> > +
> > +maintainers:
> > +  - Robert Marko <robert.marko@sartura.hr>
> > +
> > +description: |
> > +  This module is part of the Delta TN48M multi-function device. For more
> > +  details see ../mfd/delta,tn48m-cpld.yaml.
> > +
> > +  GPIO controller module provides GPIO-s for the SFP slots.
> > +  It is split into 3 controllers, one output only for the SFP TX disable
> > +  pins, one input only for the SFP present pins and one input only for
> > +  the SFP LOS pins.
> > +
> > +properties:
> > +  compatible:
> > +    enum:
> > +      - delta,tn48m-gpio-sfp-tx-disable
> > +      - delta,tn48m-gpio-sfp-present
> > +      - delta,tn48m-gpio-sfp-los
>
> The function of the 'general purpose' IO should not be encoded into the
> compatible name. Is each instance.

They are not general-purpose, they are hard-wired pins.
This is how the driver knows whether its output or input only,
and it's been reviewed by Andy Shevchenko.
It was weird for me as well, but that is how GPIO regmap works.

It was modeled by the sl28cpld GPIO driver as well as the rest of the docs
as that CPLD has similar features supported to what this initial support does.
>
> > +
> > +  reg:
> > +    maxItems: 1
> > +
> > +  "#gpio-cells":
> > +    const: 2
> > +
> > +  gpio-controller: true
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +  - "#gpio-cells"
> > +  - gpio-controller
> > +
> > +additionalProperties: false
> > diff --git a/Documentation/devicetree/bindings/mfd/delta,tn48m-cpld.yaml b/Documentation/devicetree/bindings/mfd/delta,tn48m-cpld.yaml
> > new file mode 100644
> > index 000000000000..055e09129f86
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/mfd/delta,tn48m-cpld.yaml
> > @@ -0,0 +1,81 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/mfd/delta,tn48m-cpld.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Delta Networks TN48M CPLD controller
> > +
> > +maintainers:
> > +  - Robert Marko <robert.marko@sartura.hr>
> > +
> > +description: |
> > +  Lattice CPLD onboard the TN48M switches is used for system
> > +  management.
> > +
> > +  It provides information about the hardware model, revision,
> > +  PSU status etc.
> > +
> > +  It is also being used as a GPIO expander for the SFP slots.
> > +
> > +properties:
> > +  compatible:
> > +    const: delta,tn48m-cpld
> > +
> > +  reg:
> > +    description:
> > +      I2C device address.
> > +    maxItems: 1
> > +
> > +  "#address-cells":
> > +    const: 1
> > +
> > +  "#size-cells":
> > +    const: 0
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +  - "#address-cells"
> > +  - "#size-cells"
> > +
> > +patternProperties:
> > +  "^gpio(@[0-9a-f]+)?$":
> > +    $ref: ../gpio/delta,tn48m-gpio.yaml
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > +  - |
> > +    i2c {
> > +        #address-cells = <1>;
> > +        #size-cells = <0>;
> > +
> > +        cpld@41 {
> > +            compatible = "delta,tn48m-cpld";
> > +            reg = <0x41>;
> > +            #address-cells = <1>;
> > +            #size-cells = <0>;
> > +
> > +            gpio@31 {
> > +                compatible = "delta,tn48m-gpio-sfp-tx-disable";
> > +                reg = <0x31>;
>
> Encode the register address into the gpio cells.
Do you have an example of that?
I have modeled this on sl28cpld GPIO driver which does the same thing
as it's the easiest way and simply reusing standard properties.
>
> > +                gpio-controller;
> > +                #gpio-cells = <2>;
> > +            };
> > +
> > +            gpio@3a {
> > +                compatible = "delta,tn48m-gpio-sfp-present";
> > +                reg = <0x3a>;
> > +                gpio-controller;
> > +                #gpio-cells = <2>;
> > +            };
> > +
> > +            gpio@40 {
> > +                compatible = "delta,tn48m-gpio-sfp-los";
> > +                reg = <0x40>;
> > +                gpio-controller;
> > +                #gpio-cells = <2>;
> > +            };
> > +        };
> > +    };
> > --
> > 2.31.1
> >



-- 
Robert Marko
Staff Embedded Linux Engineer
Sartura Ltd.
Lendavska ulica 16a
10000 Zagreb, Croatia
Email: robert.marko@sartura.hr
Web: www.sartura.hr

Re: [PATCH v2 3/4] dt-bindings: mfd: Add Delta TN48M CPLD drivers bindings

[Rob Herring]

On Tue, May 25, 2021 at 4:47 AM Robert Marko <robert.marko@sartura.hr> wrote:
>
> On Tue, May 25, 2021 at 1:09 AM Rob Herring <robh@kernel.org> wrote:
> >
> > On Mon, May 24, 2021 at 02:05:38PM +0200, Robert Marko wrote:
> > > Add binding documents for the Delta TN48M CPLD drivers.
> > >
> > > Signed-off-by: Robert Marko <robert.marko@sartura.hr>
> > > ---
> > > Changes in v2:
> > > * Implement MFD as a simple I2C MFD
> > > * Add GPIO bindings as separate
> >
> > I don't understand why this changed. This doesn't look like an MFD to
> > me. Make your binding complete if there are missing functions.
> > Otherwise, stick with what I already ok'ed.
>
> It changed because the custom driver was dropped at Lee Jones-es request,
> and simple-mfd-i2c is now used.

To a certain extent, I don't care about the driver. A binding can't
know what an OS wants in terms of structure and driver structure could
evolve.

> > >  .../bindings/gpio/delta,tn48m-gpio.yaml       | 42 ++++++++++
> > >  .../bindings/mfd/delta,tn48m-cpld.yaml        | 81 +++++++++++++++++++
> > >  2 files changed, 123 insertions(+)
> > >  create mode 100644 Documentation/devicetree/bindings/gpio/delta,tn48m-gpio.yaml
> > >  create mode 100644 Documentation/devicetree/bindings/mfd/delta,tn48m-cpld.yaml
> > >
> > > diff --git a/Documentation/devicetree/bindings/gpio/delta,tn48m-gpio.yaml b/Documentation/devicetree/bindings/gpio/delta,tn48m-gpio.yaml
> > > new file mode 100644
> > > index 000000000000..aca646aecb12
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/gpio/delta,tn48m-gpio.yaml
> > > @@ -0,0 +1,42 @@
> > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > > +%YAML 1.2
> > > +---
> > > +$id: http://devicetree.org/schemas/gpio/delta,tn48m-gpio.yaml#
> > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > +
> > > +title: Delta Networks TN48M CPLD GPIO controller
> > > +
> > > +maintainers:
> > > +  - Robert Marko <robert.marko@sartura.hr>
> > > +
> > > +description: |
> > > +  This module is part of the Delta TN48M multi-function device. For more
> > > +  details see ../mfd/delta,tn48m-cpld.yaml.
> > > +
> > > +  GPIO controller module provides GPIO-s for the SFP slots.
> > > +  It is split into 3 controllers, one output only for the SFP TX disable
> > > +  pins, one input only for the SFP present pins and one input only for
> > > +  the SFP LOS pins.
> > > +
> > > +properties:
> > > +  compatible:
> > > +    enum:
> > > +      - delta,tn48m-gpio-sfp-tx-disable
> > > +      - delta,tn48m-gpio-sfp-present
> > > +      - delta,tn48m-gpio-sfp-los
> >
> > The function of the 'general purpose' IO should not be encoded into the
> > compatible name. Is each instance.
>
> They are not general-purpose, they are hard-wired pins.
> This is how the driver knows whether its output or input only,

Why does the driver need to know that? The user of the pins (the SFP
cage) knows the direction and should configure them the right way.

> and it's been reviewed by Andy Shevchenko.
> It was weird for me as well, but that is how GPIO regmap works.
>
> It was modeled by the sl28cpld GPIO driver as well as the rest of the docs
> as that CPLD has similar features supported to what this initial support does.

That one is at least just encoding the programming model, not the
connection. Maybe the driver didn't need to know there either, but I
can't study everyone's h/w in depth.

That one is also 8 GPIOs per instance, not 1.

> > > +  reg:
> > > +    maxItems: 1
> > > +
> > > +  "#gpio-cells":
> > > +    const: 2
> > > +
> > > +  gpio-controller: true
> > > +
> > > +required:
> > > +  - compatible
> > > +  - reg
> > > +  - "#gpio-cells"
> > > +  - gpio-controller
> > > +
> > > +additionalProperties: false
> > > diff --git a/Documentation/devicetree/bindings/mfd/delta,tn48m-cpld.yaml b/Documentation/devicetree/bindings/mfd/delta,tn48m-cpld.yaml
> > > new file mode 100644
> > > index 000000000000..055e09129f86
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/mfd/delta,tn48m-cpld.yaml
> > > @@ -0,0 +1,81 @@
> > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > > +%YAML 1.2
> > > +---
> > > +$id: http://devicetree.org/schemas/mfd/delta,tn48m-cpld.yaml#
> > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > +
> > > +title: Delta Networks TN48M CPLD controller
> > > +
> > > +maintainers:
> > > +  - Robert Marko <robert.marko@sartura.hr>
> > > +
> > > +description: |
> > > +  Lattice CPLD onboard the TN48M switches is used for system
> > > +  management.
> > > +
> > > +  It provides information about the hardware model, revision,
> > > +  PSU status etc.
> > > +
> > > +  It is also being used as a GPIO expander for the SFP slots.
> > > +
> > > +properties:
> > > +  compatible:
> > > +    const: delta,tn48m-cpld
> > > +
> > > +  reg:
> > > +    description:
> > > +      I2C device address.
> > > +    maxItems: 1
> > > +
> > > +  "#address-cells":
> > > +    const: 1
> > > +
> > > +  "#size-cells":
> > > +    const: 0
> > > +
> > > +required:
> > > +  - compatible
> > > +  - reg
> > > +  - "#address-cells"
> > > +  - "#size-cells"
> > > +
> > > +patternProperties:
> > > +  "^gpio(@[0-9a-f]+)?$":
> > > +    $ref: ../gpio/delta,tn48m-gpio.yaml
> > > +
> > > +additionalProperties: false
> > > +
> > > +examples:
> > > +  - |
> > > +    i2c {
> > > +        #address-cells = <1>;
> > > +        #size-cells = <0>;
> > > +
> > > +        cpld@41 {
> > > +            compatible = "delta,tn48m-cpld";
> > > +            reg = <0x41>;
> > > +            #address-cells = <1>;
> > > +            #size-cells = <0>;
> > > +
> > > +            gpio@31 {
> > > +                compatible = "delta,tn48m-gpio-sfp-tx-disable";
> > > +                reg = <0x31>;
> >
> > Encode the register address into the gpio cells.
> Do you have an example of that?

The 'gpio number' in the first cell is totally up to the GPIO provider
(really all the cells are and are opaque to the consumer, but GPIO is
fairly standardized). So most of the time it's just the bit offset or
bit and register offsets when multiple 32-bit registers.

Rob

Re: [PATCH v2 3/4] dt-bindings: mfd: Add Delta TN48M CPLD drivers bindings

[Robert Marko]

On Tue, May 25, 2021 at 11:44 PM Rob Herring <robh@kernel.org> wrote:
>
> On Tue, May 25, 2021 at 4:47 AM Robert Marko <robert.marko@sartura.hr> wrote:
> >
> > On Tue, May 25, 2021 at 1:09 AM Rob Herring <robh@kernel.org> wrote:
> > >
> > > On Mon, May 24, 2021 at 02:05:38PM +0200, Robert Marko wrote:
> > > > Add binding documents for the Delta TN48M CPLD drivers.
> > > >
> > > > Signed-off-by: Robert Marko <robert.marko@sartura.hr>
> > > > ---
> > > > Changes in v2:
> > > > * Implement MFD as a simple I2C MFD
> > > > * Add GPIO bindings as separate
> > >
> > > I don't understand why this changed. This doesn't look like an MFD to
> > > me. Make your binding complete if there are missing functions.
> > > Otherwise, stick with what I already ok'ed.
> >
> > It changed because the custom driver was dropped at Lee Jones-es request,
> > and simple-mfd-i2c is now used.
>
> To a certain extent, I don't care about the driver. A binding can't
> know what an OS wants in terms of structure and driver structure could
> evolve.

To a certain degree, I also agree, but in this case, it had to change.
Otherwise, it would not represent the actual driver and its requirements.

I agree that it was not a true MFD with only one consumer, I have
added a reset driver
in v3.
I was planning on adding it later anyway.

>
> > > >  .../bindings/gpio/delta,tn48m-gpio.yaml       | 42 ++++++++++
> > > >  .../bindings/mfd/delta,tn48m-cpld.yaml        | 81 +++++++++++++++++++
> > > >  2 files changed, 123 insertions(+)
> > > >  create mode 100644 Documentation/devicetree/bindings/gpio/delta,tn48m-gpio.yaml
> > > >  create mode 100644 Documentation/devicetree/bindings/mfd/delta,tn48m-cpld.yaml
> > > >
> > > > diff --git a/Documentation/devicetree/bindings/gpio/delta,tn48m-gpio.yaml b/Documentation/devicetree/bindings/gpio/delta,tn48m-gpio.yaml
> > > > new file mode 100644
> > > > index 000000000000..aca646aecb12
> > > > --- /dev/null
> > > > +++ b/Documentation/devicetree/bindings/gpio/delta,tn48m-gpio.yaml
> > > > @@ -0,0 +1,42 @@
> > > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > > > +%YAML 1.2
> > > > +---
> > > > +$id: http://devicetree.org/schemas/gpio/delta,tn48m-gpio.yaml#
> > > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > > +
> > > > +title: Delta Networks TN48M CPLD GPIO controller
> > > > +
> > > > +maintainers:
> > > > +  - Robert Marko <robert.marko@sartura.hr>
> > > > +
> > > > +description: |
> > > > +  This module is part of the Delta TN48M multi-function device. For more
> > > > +  details see ../mfd/delta,tn48m-cpld.yaml.
> > > > +
> > > > +  GPIO controller module provides GPIO-s for the SFP slots.
> > > > +  It is split into 3 controllers, one output only for the SFP TX disable
> > > > +  pins, one input only for the SFP present pins and one input only for
> > > > +  the SFP LOS pins.
> > > > +
> > > > +properties:
> > > > +  compatible:
> > > > +    enum:
> > > > +      - delta,tn48m-gpio-sfp-tx-disable
> > > > +      - delta,tn48m-gpio-sfp-present
> > > > +      - delta,tn48m-gpio-sfp-los
> > >
> > > The function of the 'general purpose' IO should not be encoded into the
> > > compatible name. Is each instance.
> >
> > They are not general-purpose, they are hard-wired pins.
> > This is how the driver knows whether its output or input only,
>
> Why does the driver need to know that? The user of the pins (the SFP
> cage) knows the direction and should configure them the right way.

Because the GPIO regmap driver requires this information as well as setting
the register for the directions it supports, the GPIO core requires it as well.
You cant allow setting a direction that is not supported as GPIO core
and other subsystems depend on knowing what directions are supported.

>
> > and it's been reviewed by Andy Shevchenko.
> > It was weird for me as well, but that is how GPIO regmap works.
> >
> > It was modeled by the sl28cpld GPIO driver as well as the rest of the docs
> > as that CPLD has similar features supported to what this initial support does.
>
> That one is at least just encoding the programming model, not the
> connection. Maybe the driver didn't need to know there either, but I
> can't study everyone's h/w in depth.

Understood, but the driver received 2 reviews and one ACK, so the code is
good and reviewed.

>
> That one is also 8 GPIOs per instance, not 1.

In this case, it's 4 pins per instance.

>
> > > > +  reg:
> > > > +    maxItems: 1
> > > > +
> > > > +  "#gpio-cells":
> > > > +    const: 2
> > > > +
> > > > +  gpio-controller: true
> > > > +
> > > > +required:
> > > > +  - compatible
> > > > +  - reg
> > > > +  - "#gpio-cells"
> > > > +  - gpio-controller
> > > > +
> > > > +additionalProperties: false
> > > > diff --git a/Documentation/devicetree/bindings/mfd/delta,tn48m-cpld.yaml b/Documentation/devicetree/bindings/mfd/delta,tn48m-cpld.yaml
> > > > new file mode 100644
> > > > index 000000000000..055e09129f86
> > > > --- /dev/null
> > > > +++ b/Documentation/devicetree/bindings/mfd/delta,tn48m-cpld.yaml
> > > > @@ -0,0 +1,81 @@
> > > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > > > +%YAML 1.2
> > > > +---
> > > > +$id: http://devicetree.org/schemas/mfd/delta,tn48m-cpld.yaml#
> > > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > > +
> > > > +title: Delta Networks TN48M CPLD controller
> > > > +
> > > > +maintainers:
> > > > +  - Robert Marko <robert.marko@sartura.hr>
> > > > +
> > > > +description: |
> > > > +  Lattice CPLD onboard the TN48M switches is used for system
> > > > +  management.
> > > > +
> > > > +  It provides information about the hardware model, revision,
> > > > +  PSU status etc.
> > > > +
> > > > +  It is also being used as a GPIO expander for the SFP slots.
> > > > +
> > > > +properties:
> > > > +  compatible:
> > > > +    const: delta,tn48m-cpld
> > > > +
> > > > +  reg:
> > > > +    description:
> > > > +      I2C device address.
> > > > +    maxItems: 1
> > > > +
> > > > +  "#address-cells":
> > > > +    const: 1
> > > > +
> > > > +  "#size-cells":
> > > > +    const: 0
> > > > +
> > > > +required:
> > > > +  - compatible
> > > > +  - reg
> > > > +  - "#address-cells"
> > > > +  - "#size-cells"
> > > > +
> > > > +patternProperties:
> > > > +  "^gpio(@[0-9a-f]+)?$":
> > > > +    $ref: ../gpio/delta,tn48m-gpio.yaml
> > > > +
> > > > +additionalProperties: false
> > > > +
> > > > +examples:
> > > > +  - |
> > > > +    i2c {
> > > > +        #address-cells = <1>;
> > > > +        #size-cells = <0>;
> > > > +
> > > > +        cpld@41 {
> > > > +            compatible = "delta,tn48m-cpld";
> > > > +            reg = <0x41>;
> > > > +            #address-cells = <1>;
> > > > +            #size-cells = <0>;
> > > > +
> > > > +            gpio@31 {
> > > > +                compatible = "delta,tn48m-gpio-sfp-tx-disable";
> > > > +                reg = <0x31>;
> > >
> > > Encode the register address into the gpio cells.
> > Do you have an example of that?
>
> The 'gpio number' in the first cell is totally up to the GPIO provider
> (really all the cells are and are opaque to the consumer, but GPIO is
> fairly standardized). So most of the time it's just the bit offset or
> bit and register offsets when multiple 32-bit registers.

As the GPIO regmap is the ideal use case for using "reg" to get the
register address
and the driver itself has received multiple reviews and has been ACK-ed I would
prefer to leave it as is.

Regards,
Robert

>
> Rob



-- 
Robert Marko
Staff Embedded Linux Engineer
Sartura Ltd.
Lendavska ulica 16a
10000 Zagreb, Croatia
Email: robert.marko@sartura.hr
Web: www.sartura.hr

[PATCH v2 4/4] MAINTAINERS: Add Delta Networks TN48M CPLD drivers

[Robert Marko]

Add maintainers entry for the Delta Networks TN48M
CPLD MFD drivers.

Signed-off-by: Robert Marko <robert.marko@sartura.hr>
---
Changes in v2:
* Drop no more existing files

 MAINTAINERS | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index 81e1edeceae4..dd2bcb8c7756 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -5180,6 +5180,13 @@ W:	https://linuxtv.org
 T:	git git://linuxtv.org/media_tree.git
 F:	drivers/media/platform/sti/delta
 
+DELTA NETWORKS TN48M CPLD DRIVERS
+M:	Robert Marko <robert.marko@sartura.hr>
+S:	Maintained
+F:	Documentation/devicetree/bindings/gpio/delta,tn48m-gpio.yaml
+F:	Documentation/devicetree/bindings/mfd/delta,tn48m-cpld.yaml
+F:	drivers/gpio/gpio-tn48m.c
+
 DENALI NAND DRIVER
 L:	linux-mtd@lists.infradead.org
 S:	Orphan
-- 
2.31.1