[PATCH 1/6] mfd: Add Delta TN48M CPLD driver

[PATCH 1/6] mfd: Add Delta TN48M CPLD driver

[Robert Marko]

Delta TN48M switches have a Lattice CPLD that serves
multiple purposes including being a GPIO expander.
So lets add the MFD core driver for it.

Signed-off-by: Robert Marko <robert.marko@sartura.hr>
---
 drivers/mfd/Kconfig       |  13 +++
 drivers/mfd/Makefile      |   1 +
 drivers/mfd/tn48m-cpld.c  | 181 ++++++++++++++++++++++++++++++++++++++
 include/linux/mfd/tn48m.h |  30 +++++++
 4 files changed, 225 insertions(+)
 create mode 100644 drivers/mfd/tn48m-cpld.c
 create mode 100644 include/linux/mfd/tn48m.h

diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
index b74efa469e90..809041f98d71 100644
--- a/drivers/mfd/Kconfig
+++ b/drivers/mfd/Kconfig
@@ -297,6 +297,19 @@ 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_CORE
+	select REGMAP_I2C
+	help
+	  Select this option to enable support for Delta Networks TN48M switch
+	  CPLD. It consists of GPIO and hwmon drivers.
+	  CPLD provides GPIOS-s for the SFP slots as well as power supply
+	  related information.
+	  Driver provides debugfs information about the board model as
+	  well as hardware and CPLD revision information.
+
 config PMIC_DA903X
 	bool "Dialog Semiconductor DA9030/DA9034 PMIC Support"
 	depends on I2C=y
diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
index 834f5463af28..974663341f08 100644
--- a/drivers/mfd/Makefile
+++ b/drivers/mfd/Makefile
@@ -27,6 +27,7 @@ obj-$(CONFIG_MFD_TI_LP87565)	+= lp87565.o
 obj-$(CONFIG_MFD_DAVINCI_VOICECODEC)	+= davinci_voicecodec.o
 obj-$(CONFIG_MFD_DM355EVM_MSP)	+= dm355evm_msp.o
 obj-$(CONFIG_MFD_TI_AM335X_TSCADC)	+= ti_am335x_tscadc.o
+obj-$(CONFIG_MFD_TN48M_CPLD)	+= tn48m-cpld.o
 
 obj-$(CONFIG_MFD_STA2X11)	+= sta2x11-mfd.o
 obj-$(CONFIG_MFD_STMPE)		+= stmpe.o
diff --git a/drivers/mfd/tn48m-cpld.c b/drivers/mfd/tn48m-cpld.c
new file mode 100644
index 000000000000..b84510fb630a
--- /dev/null
+++ b/drivers/mfd/tn48m-cpld.c
@@ -0,0 +1,181 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Delta TN48M CPLD parent driver
+ *
+ * Copyright 2020 Sartura Ltd
+ *
+ * Author: Robert Marko <robert.marko@sartura.hr>
+ */
+
+#include <linux/debugfs.h>
+#include <linux/i2c.h>
+#include <linux/kernel.h>
+#include <linux/mfd/core.h>
+#include <linux/mfd/tn48m.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+#include <linux/slab.h>
+
+static const struct mfd_cell tn48m_cell[] = {};
+
+static const struct regmap_config tn48m_regmap_config = {
+	.reg_bits = 8,
+	.val_bits = 8,
+	.max_register = 0x40,
+};
+
+static int hardware_version_show(struct seq_file *s, void *data)
+{
+	struct tn48m_data *priv = s->private;
+	unsigned int regval;
+	char *buf;
+
+	regmap_read(priv->regmap, HARDWARE_VERSION_ID, &regval);
+
+	switch (FIELD_GET(HARDWARE_VERSION_MASK, regval)) {
+	case HARDWARE_VERSION_EVT1:
+		buf = "EVT1";
+		break;
+	case HARDWARE_VERSION_EVT2:
+		buf = "EVT2";
+		break;
+	case HARDWARE_VERSION_DVT:
+		buf = "DVT";
+		break;
+	case HARDWARE_VERSION_PVT:
+		buf = "PVT";
+		break;
+	default:
+		buf = "Unknown";
+		break;
+	}
+
+	seq_printf(s, "%s\n", buf);
+
+	return 0;
+}
+
+DEFINE_SHOW_ATTRIBUTE(hardware_version);
+
+static int board_id_show(struct seq_file *s, void *data)
+{
+	struct tn48m_data *priv = s->private;
+	unsigned int regval;
+	char *buf;
+
+	regmap_read(priv->regmap, BOARD_ID, &regval);
+
+	switch (regval) {
+	case BOARD_ID_TN48M:
+		buf = "TN48M";
+		break;
+	case BOARD_ID_TN48M_P:
+		buf = "TN48-P";
+		break;
+	default:
+		buf = "Unknown";
+		break;
+	}
+
+	seq_printf(s, "%s\n", buf);
+
+	return 0;
+}
+
+DEFINE_SHOW_ATTRIBUTE(board_id);
+
+static int code_version_show(struct seq_file *s, void *data)
+{
+	struct tn48m_data *priv = s->private;
+	unsigned int regval;
+
+	regmap_read(priv->regmap, CPLD_CODE_VERSION, &regval);
+
+	seq_printf(s, "%d\n", regval);
+
+	return 0;
+}
+
+DEFINE_SHOW_ATTRIBUTE(code_version);
+
+static void tn48m_init_debugfs(struct tn48m_data *data)
+{
+	data->debugfs_dir = debugfs_create_dir(data->client->name, NULL);
+
+	debugfs_create_file("hardware_version",
+			    0400,
+			    data->debugfs_dir,
+			    data,
+			    &hardware_version_fops);
+
+	debugfs_create_file("board_id",
+			    0400,
+			    data->debugfs_dir,
+			    data,
+			    &board_id_fops);
+
+	debugfs_create_file("code_version",
+			    0400,
+			    data->debugfs_dir,
+			    data,
+			    &code_version_fops);
+}
+
+static int tn48m_probe(struct i2c_client *client)
+{
+	struct tn48m_data *data;
+	int ret;
+
+	data = devm_kzalloc(&client->dev, sizeof(*data), GFP_KERNEL);
+	if (!data)
+		return -ENOMEM;
+
+	data->client = client;
+	data->dev = &client->dev;
+	i2c_set_clientdata(client, data);
+
+	data->regmap = devm_regmap_init_i2c(client, &tn48m_regmap_config);
+	if (IS_ERR(data->regmap)) {
+		dev_err(data->dev, "Failed to allocate regmap\n");
+		return PTR_ERR(data->regmap);
+	}
+
+	ret = devm_mfd_add_devices(data->dev, PLATFORM_DEVID_AUTO, tn48m_cell,
+				   ARRAY_SIZE(tn48m_cell), NULL, 0, NULL);
+	if (ret)
+		dev_err(data->dev, "Failed to register sub-devices %d\n", ret);
+
+	tn48m_init_debugfs(data);
+
+	return ret;
+}
+
+static int tn48m_remove(struct i2c_client *client)
+{
+	struct tn48m_data *data = i2c_get_clientdata(client);
+
+	debugfs_remove_recursive(data->debugfs_dir);
+
+	return 0;
+}
+
+static const struct of_device_id tn48m_of_match[] = {
+	{ .compatible = "delta,tn48m-cpld"},
+	{ }
+};
+MODULE_DEVICE_TABLE(of, tn48m_of_match);
+
+static struct i2c_driver tn48m_driver = {
+	.driver = {
+		.name = "tn48m-cpld",
+		.of_match_table = tn48m_of_match,
+	},
+	.probe_new	= tn48m_probe,
+	.remove		= tn48m_remove,
+};
+module_i2c_driver(tn48m_driver);
+
+MODULE_AUTHOR("Robert Marko <robert.marko@sartura.hr>");
+MODULE_DESCRIPTION("Delta TN48M CPLD parent driver");
+MODULE_LICENSE("GPL");
diff --git a/include/linux/mfd/tn48m.h b/include/linux/mfd/tn48m.h
new file mode 100644
index 000000000000..551c550efa54
--- /dev/null
+++ b/include/linux/mfd/tn48m.h
@@ -0,0 +1,30 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * Copyright 2020 Sartura Ltd
+ */
+
+#ifndef __TN48M_H__
+#define __TN48M_H__
+
+#include <linux/device.h>
+#include <linux/regmap.h>
+
+#define HARDWARE_VERSION_ID	0x0
+#define HARDWARE_VERSION_MASK	GENMASK(3, 0)
+#define HARDWARE_VERSION_EVT1	0
+#define HARDWARE_VERSION_EVT2	1
+#define HARDWARE_VERSION_DVT	2
+#define HARDWARE_VERSION_PVT	3
+#define BOARD_ID		0x1
+#define BOARD_ID_TN48M		0xa
+#define BOARD_ID_TN48M_P	0xb
+#define CPLD_CODE_VERSION	0x2
+
+struct tn48m_data {
+	struct device *dev;
+	struct regmap *regmap;
+	struct i2c_client *client;
+	struct dentry *debugfs_dir;
+};
+
+#endif
-- 
2.31.1

[PATCH 2/6] dt-bindings: gpio: Add Delta TN48M CPLD GPIO bindings

[Robert Marko]

CPLD inside of the Delta TN48M does not number GPIOs
at all, so in order to ensure numbering lets use bindigs.

Signed-off-by: Robert Marko <robert.marko@sartura.hr>
---
 include/dt-bindings/gpio/tn48m-gpio.h | 26 ++++++++++++++++++++++++++
 1 file changed, 26 insertions(+)
 create mode 100644 include/dt-bindings/gpio/tn48m-gpio.h

diff --git a/include/dt-bindings/gpio/tn48m-gpio.h b/include/dt-bindings/gpio/tn48m-gpio.h
new file mode 100644
index 000000000000..4ece4826d746
--- /dev/null
+++ b/include/dt-bindings/gpio/tn48m-gpio.h
@@ -0,0 +1,26 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * GPIO definitions for Delta TN48M CPLD GPIO driver
+ *
+ * Copyright 2020 Sartura Ltd
+ *
+ * Author: Robert Marko <robert.marko@sartura.hr>
+ */
+
+#ifndef _DT_BINDINGS_TN48M_GPIO_H
+#define _DT_BINDINGS_TN48M_GPIO_H
+
+#define SFP_TX_DISABLE_52	0
+#define SFP_TX_DISABLE_51	1
+#define SFP_TX_DISABLE_50	2
+#define SFP_TX_DISABLE_49	3
+#define SFP_PRESENT_52		4
+#define SFP_PRESENT_51		5
+#define SFP_PRESENT_50		6
+#define SFP_PRESENT_49		7
+#define SFP_LOS_52		8
+#define SFP_LOS_51		9
+#define SFP_LOS_50		10
+#define SFP_LOS_49		11
+
+#endif /* _DT_BINDINGS_TN48M_GPIO_H */
-- 
2.31.1

Re: [PATCH 2/6] dt-bindings: gpio: Add Delta TN48M CPLD GPIO bindings

[Rob Herring]

On Fri, Apr 30, 2021 at 02:35:07PM +0200, Robert Marko wrote:
> CPLD inside of the Delta TN48M does not number GPIOs
> at all, so in order to ensure numbering lets use bindigs.

Looking at the code, I'd make the gpio number something like '(offset << 
8) | bit' rather than just making up an index.

We don't normally have defines for GPIO numbers either.

> 
> Signed-off-by: Robert Marko <robert.marko@sartura.hr>
> ---
>  include/dt-bindings/gpio/tn48m-gpio.h | 26 ++++++++++++++++++++++++++
>  1 file changed, 26 insertions(+)
>  create mode 100644 include/dt-bindings/gpio/tn48m-gpio.h
> 
> diff --git a/include/dt-bindings/gpio/tn48m-gpio.h b/include/dt-bindings/gpio/tn48m-gpio.h
> new file mode 100644
> index 000000000000..4ece4826d746
> --- /dev/null
> +++ b/include/dt-bindings/gpio/tn48m-gpio.h
> @@ -0,0 +1,26 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * GPIO definitions for Delta TN48M CPLD GPIO driver
> + *
> + * Copyright 2020 Sartura Ltd
> + *
> + * Author: Robert Marko <robert.marko@sartura.hr>
> + */
> +
> +#ifndef _DT_BINDINGS_TN48M_GPIO_H
> +#define _DT_BINDINGS_TN48M_GPIO_H
> +
> +#define SFP_TX_DISABLE_52	0
> +#define SFP_TX_DISABLE_51	1
> +#define SFP_TX_DISABLE_50	2
> +#define SFP_TX_DISABLE_49	3
> +#define SFP_PRESENT_52		4
> +#define SFP_PRESENT_51		5
> +#define SFP_PRESENT_50		6
> +#define SFP_PRESENT_49		7
> +#define SFP_LOS_52		8
> +#define SFP_LOS_51		9
> +#define SFP_LOS_50		10
> +#define SFP_LOS_49		11
> +
> +#endif /* _DT_BINDINGS_TN48M_GPIO_H */
> -- 
> 2.31.1
> 

[PATCH 3/6] 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.

Since there is no logical GPIO numbering arbitrary one is used
along dt-bindings to make it humanly readable.

Signed-off-by: Robert Marko <robert.marko@sartura.hr>
---
 drivers/gpio/Kconfig      |  12 +++
 drivers/gpio/Makefile     |   1 +
 drivers/gpio/gpio-tn48m.c | 191 ++++++++++++++++++++++++++++++++++++++
 drivers/mfd/tn48m-cpld.c  |   6 +-
 include/linux/mfd/tn48m.h |   3 +
 5 files changed, 212 insertions(+), 1 deletion(-)
 create mode 100644 drivers/gpio/gpio-tn48m.c

diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index e3607ec4c2e8..e17d4416786a 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -1310,6 +1310,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
+	depends on OF_GPIO
+	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 c58a90a3c3b1..271fb806475e 100644
--- a/drivers/gpio/Makefile
+++ b/drivers/gpio/Makefile
@@ -145,6 +145,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..4e859e4c339c
--- /dev/null
+++ b/drivers/gpio/gpio-tn48m.c
@@ -0,0 +1,191 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Delta TN48M CPLD GPIO driver
+ *
+ * Copyright 2020 Sartura Ltd
+ *
+ * Author: Robert Marko <robert.marko@sartura.hr>
+ */
+
+#include <linux/gpio/driver.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+
+#include <linux/mfd/tn48m.h>
+#include <dt-bindings/gpio/tn48m-gpio.h>
+
+struct tn48m_gpio {
+	struct gpio_chip chip;
+	struct tn48m_data *data;
+};
+
+static int tn48m_gpio_get_direction(struct gpio_chip *chip,
+				    unsigned int offset)
+{
+	switch (offset) {
+	case SFP_TX_DISABLE_52:
+	case SFP_TX_DISABLE_51:
+	case SFP_TX_DISABLE_50:
+	case SFP_TX_DISABLE_49:
+		return GPIO_LINE_DIRECTION_OUT;
+	case SFP_PRESENT_52:
+	case SFP_PRESENT_51:
+	case SFP_PRESENT_50:
+	case SFP_PRESENT_49:
+	case SFP_LOS_52:
+	case SFP_LOS_51:
+	case SFP_LOS_50:
+	case SFP_LOS_49:
+		return GPIO_LINE_DIRECTION_IN;
+	default:
+		return -EINVAL;
+	}
+}
+
+static int tn48m_gpio_get_reg(unsigned int offset)
+{
+	switch (offset) {
+	case SFP_TX_DISABLE_52:
+	case SFP_TX_DISABLE_51:
+	case SFP_TX_DISABLE_50:
+	case SFP_TX_DISABLE_49:
+		return SFP_TX_DISABLE;
+	case SFP_PRESENT_52:
+	case SFP_PRESENT_51:
+	case SFP_PRESENT_50:
+	case SFP_PRESENT_49:
+		return SFP_PRESENT;
+	case SFP_LOS_52:
+	case SFP_LOS_51:
+	case SFP_LOS_50:
+	case SFP_LOS_49:
+		return SFP_LOS;
+	default:
+		return -EINVAL;
+	}
+}
+
+static int tn48m_gpio_get_mask(unsigned int offset)
+{
+	switch (offset) {
+	case SFP_TX_DISABLE_52:
+	case SFP_PRESENT_52:
+	case SFP_LOS_52:
+		return BIT(3);
+	case SFP_TX_DISABLE_51:
+	case SFP_PRESENT_51:
+	case SFP_LOS_51:
+		return BIT(2);
+	case SFP_TX_DISABLE_50:
+	case SFP_PRESENT_50:
+	case SFP_LOS_50:
+		return BIT(1);
+	case SFP_TX_DISABLE_49:
+	case SFP_PRESENT_49:
+	case SFP_LOS_49:
+		return BIT(0);
+	default:
+		return -EINVAL;
+	}
+}
+
+static int tn48m_gpio_get(struct gpio_chip *chip, unsigned int offset)
+{
+	struct tn48m_gpio *gpio = gpiochip_get_data(chip);
+	unsigned int regval;
+	int ret;
+
+	ret = regmap_read(gpio->data->regmap,
+			  tn48m_gpio_get_reg(offset),
+			  &regval);
+	if (ret < 0)
+		return ret;
+
+	return regval & tn48m_gpio_get_mask(offset);
+}
+
+static void tn48m_gpio_set(struct gpio_chip *chip, unsigned int offset,
+			   int value)
+{
+	struct tn48m_gpio *gpio = gpiochip_get_data(chip);
+
+	regmap_update_bits(gpio->data->regmap,
+			   tn48m_gpio_get_reg(offset),
+			   tn48m_gpio_get_mask(offset),
+			   value ? tn48m_gpio_get_mask(offset) : 0);
+}
+
+static int tn48m_gpio_direction_output(struct gpio_chip *chip,
+				       unsigned int offset, int value)
+{
+	tn48m_gpio_set(chip, offset, value);
+
+	return 0;
+}
+
+/*
+ * Required for SFP as it calls gpiod_direction_input()
+ * and if its missing TX disable GPIO will print an
+ * error and not be controlled anymore.
+ */
+static int tn48m_gpio_direction_input(struct gpio_chip *chip,
+				      unsigned int offset)
+{
+	return 0;
+}
+
+static const struct gpio_chip tn48m_template_chip = {
+	.label			= "tn48m-gpio",
+	.owner			= THIS_MODULE,
+	.get_direction		= tn48m_gpio_get_direction,
+	.direction_output	= tn48m_gpio_direction_output,
+	.direction_input	= tn48m_gpio_direction_input,
+	.get			= tn48m_gpio_get,
+	.set			= tn48m_gpio_set,
+	.base			= -1,
+	.ngpio			= 12,
+	.can_sleep		= true,
+};
+
+static int tn48m_gpio_probe(struct platform_device *pdev)
+{
+	struct tn48m_gpio *gpio;
+	int ret;
+
+	gpio = devm_kzalloc(&pdev->dev, sizeof(*gpio), GFP_KERNEL);
+	if (!gpio)
+		return -ENOMEM;
+
+	platform_set_drvdata(pdev, gpio);
+
+	gpio->data = dev_get_drvdata(pdev->dev.parent);
+	gpio->chip = tn48m_template_chip;
+	gpio->chip.parent = gpio->data->dev;
+
+	ret = devm_gpiochip_add_data(&pdev->dev, &gpio->chip, gpio);
+	if (ret < 0) {
+		dev_err(&pdev->dev, "Could not register gpiochip, %d\n", ret);
+		return ret;
+	}
+
+	return 0;
+}
+
+static const struct platform_device_id tn48m_gpio_id_table[] = {
+	{ "delta,tn48m-gpio", },
+	{ }
+};
+MODULE_DEVICE_TABLE(platform, tn48m_gpio_id_table);
+
+static struct platform_driver tn48m_gpio_driver = {
+	.driver = {
+		.name = "tn48m-gpio",
+	},
+	.probe = tn48m_gpio_probe,
+	.id_table = tn48m_gpio_id_table,
+};
+module_platform_driver(tn48m_gpio_driver);
+
+MODULE_AUTHOR("Robert Marko <robert.marko@sartura.hr>");
+MODULE_DESCRIPTION("Delta TN48M CPLD GPIO driver");
+MODULE_LICENSE("GPL");
diff --git a/drivers/mfd/tn48m-cpld.c b/drivers/mfd/tn48m-cpld.c
index b84510fb630a..f22a15ddd22d 100644
--- a/drivers/mfd/tn48m-cpld.c
+++ b/drivers/mfd/tn48m-cpld.c
@@ -17,7 +17,11 @@
 #include <linux/regmap.h>
 #include <linux/slab.h>
 
-static const struct mfd_cell tn48m_cell[] = {};
+static const struct mfd_cell tn48m_cell[] = {
+	{
+		.name = "delta,tn48m-gpio",
+	}
+};
 
 static const struct regmap_config tn48m_regmap_config = {
 	.reg_bits = 8,
diff --git a/include/linux/mfd/tn48m.h b/include/linux/mfd/tn48m.h
index 551c550efa54..9cc2b04c8d69 100644
--- a/include/linux/mfd/tn48m.h
+++ b/include/linux/mfd/tn48m.h
@@ -19,6 +19,9 @@
 #define BOARD_ID_TN48M		0xa
 #define BOARD_ID_TN48M_P	0xb
 #define CPLD_CODE_VERSION	0x2
+#define SFP_TX_DISABLE		0x31
+#define SFP_PRESENT		0x3a
+#define SFP_LOS			0x40
 
 struct tn48m_data {
 	struct device *dev;
-- 
2.31.1

Re: [PATCH 3/6] gpio: Add Delta TN48M CPLD GPIO driver

[Rob Herring]

On Fri, Apr 30, 2021 at 02:35:08PM +0200, Robert Marko 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.
> 
> Since there is no logical GPIO numbering arbitrary one is used
> along dt-bindings to make it humanly readable.
> 
> Signed-off-by: Robert Marko <robert.marko@sartura.hr>
> ---
>  drivers/gpio/Kconfig      |  12 +++
>  drivers/gpio/Makefile     |   1 +
>  drivers/gpio/gpio-tn48m.c | 191 ++++++++++++++++++++++++++++++++++++++
>  drivers/mfd/tn48m-cpld.c  |   6 +-
>  include/linux/mfd/tn48m.h |   3 +
>  5 files changed, 212 insertions(+), 1 deletion(-)
>  create mode 100644 drivers/gpio/gpio-tn48m.c


> +static const struct platform_device_id tn48m_gpio_id_table[] = {
> +	{ "delta,tn48m-gpio", },

Looks like a compatible, but is not. I think you can drop this and just 
use 'tm48m-gpio' (the driver name).

Same for hwmon.

Rob

> +	{ }
> +};
> +MODULE_DEVICE_TABLE(platform, tn48m_gpio_id_table);
> +
> +static struct platform_driver tn48m_gpio_driver = {
> +	.driver = {
> +		.name = "tn48m-gpio",
> +	},
> +	.probe = tn48m_gpio_probe,
> +	.id_table = tn48m_gpio_id_table,
> +};
> +module_platform_driver(tn48m_gpio_driver);

Re: [PATCH 3/6] gpio: Add Delta TN48M CPLD GPIO driver

[Michael Walle]

Am 2021-05-06 16:00, schrieb Rob Herring:
> On Fri, Apr 30, 2021 at 02:35:08PM +0200, Robert Marko 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.
>> 
>> Since there is no logical GPIO numbering arbitrary one is used
>> along dt-bindings to make it humanly readable.
>> 
>> Signed-off-by: Robert Marko <robert.marko@sartura.hr>
>> ---
>>  drivers/gpio/Kconfig      |  12 +++
>>  drivers/gpio/Makefile     |   1 +
>>  drivers/gpio/gpio-tn48m.c | 191 
>> ++++++++++++++++++++++++++++++++++++++
>>  drivers/mfd/tn48m-cpld.c  |   6 +-
>>  include/linux/mfd/tn48m.h |   3 +
>>  5 files changed, 212 insertions(+), 1 deletion(-)
>>  create mode 100644 drivers/gpio/gpio-tn48m.c
> 
> 
>> +static const struct platform_device_id tn48m_gpio_id_table[] = {
>> +	{ "delta,tn48m-gpio", },
> 
> Looks like a compatible, but is not. I think you can drop this and just
> use 'tm48m-gpio' (the driver name).

I'm just curious, why isn't the vendor included here (as there
might be a chance for name clashes in the future).

-michael

Re: [PATCH 3/6] gpio: Add Delta TN48M CPLD GPIO driver

[Luka Perkov]

Hello Michael,

On Thu, May 6, 2021 at 6:40 PM Michael Walle <michael@walle.cc> wrote:
>
> Am 2021-05-06 16:00, schrieb Rob Herring:
> > On Fri, Apr 30, 2021 at 02:35:08PM +0200, Robert Marko 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.
> >>
> >> Since there is no logical GPIO numbering arbitrary one is used
> >> along dt-bindings to make it humanly readable.
> >>
> >> Signed-off-by: Robert Marko <robert.marko@sartura.hr>
> >> ---
> >>  drivers/gpio/Kconfig      |  12 +++
> >>  drivers/gpio/Makefile     |   1 +
> >>  drivers/gpio/gpio-tn48m.c | 191
> >> ++++++++++++++++++++++++++++++++++++++
> >>  drivers/mfd/tn48m-cpld.c  |   6 +-
> >>  include/linux/mfd/tn48m.h |   3 +
> >>  5 files changed, 212 insertions(+), 1 deletion(-)
> >>  create mode 100644 drivers/gpio/gpio-tn48m.c
> >
> >
> >> +static const struct platform_device_id tn48m_gpio_id_table[] = {
> >> +    { "delta,tn48m-gpio", },
> >
> > Looks like a compatible, but is not. I think you can drop this and just
> > use 'tm48m-gpio' (the driver name).
>
> I'm just curious, why isn't the vendor included here (as there
> might be a chance for name clashes in the future).

I'm looping in Clement from Delta as well as the Upstream Working
Group from DENT [0].

Thanks,

Luka

[0] https://dent.dev/

Re: [PATCH 3/6] gpio: Add Delta TN48M CPLD GPIO driver

[Robert Marko]

On Thu, May 6, 2021 at 6:40 PM Michael Walle <michael@walle.cc> wrote:
>
> Am 2021-05-06 16:00, schrieb Rob Herring:
> > On Fri, Apr 30, 2021 at 02:35:08PM +0200, Robert Marko 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.
> >>
> >> Since there is no logical GPIO numbering arbitrary one is used
> >> along dt-bindings to make it humanly readable.
> >>
> >> Signed-off-by: Robert Marko <robert.marko@sartura.hr>
> >> ---
> >>  drivers/gpio/Kconfig      |  12 +++
> >>  drivers/gpio/Makefile     |   1 +
> >>  drivers/gpio/gpio-tn48m.c | 191
> >> ++++++++++++++++++++++++++++++++++++++
> >>  drivers/mfd/tn48m-cpld.c  |   6 +-
> >>  include/linux/mfd/tn48m.h |   3 +
> >>  5 files changed, 212 insertions(+), 1 deletion(-)
> >>  create mode 100644 drivers/gpio/gpio-tn48m.c
> >
> >
> >> +static const struct platform_device_id tn48m_gpio_id_table[] = {
> >> +    { "delta,tn48m-gpio", },
> >
> > Looks like a compatible, but is not. I think you can drop this and just
> > use 'tm48m-gpio' (the driver name).
>
> I'm just curious, why isn't the vendor included here (as there
> might be a chance for name clashes in the future).

It's my oversight, I have converted it to use simple I2C MFD so its
OF based now.

I will update the driver's name with the vendor's name.

Robert
>
> -michael



-- 
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 3/6] gpio: Add Delta TN48M CPLD GPIO driver

[Michael Walle]

Am 2021-04-30 14:35, schrieb 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.
> 
> Since there is no logical GPIO numbering arbitrary one is used
> along dt-bindings to make it humanly readable.

Can gpio/gpio-regmap.c be used here? See gpio/gpio-sl28cpld.c
for an example.

-michael

Re: [PATCH 3/6] gpio: Add Delta TN48M CPLD GPIO driver

[Robert Marko]

On Thu, May 6, 2021 at 6:38 PM Michael Walle <michael@walle.cc> wrote:
>
> Am 2021-04-30 14:35, schrieb 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.
> >
> > Since there is no logical GPIO numbering arbitrary one is used
> > along dt-bindings to make it humanly readable.
>
> Can gpio/gpio-regmap.c be used here? See gpio/gpio-sl28cpld.c
> for an example.

I suppose it can be used, I need to look into it more to see how could
translation to register and mask be done with it.

I have adapted the driver to work with simple I2C MFD so far.

It still leaves me with a finding the logical way to number the GPIO-s.

Robert
>
> -michael



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

[PATCH 4/6] hwmon: Add Delta TN48M CPLD HWMON driver

[Robert Marko]

Delta TN48M has a CPLD that also monitors the power supply
statuses.

These are useful to be presented to the users, so lets
add a driver for HWMON part of the CPLD.

Signed-off-by: Robert Marko <robert.marko@sartura.hr>
---
 drivers/hwmon/Kconfig       |  10 +++
 drivers/hwmon/Makefile      |   1 +
 drivers/hwmon/tn48m-hwmon.c | 148 ++++++++++++++++++++++++++++++++++++
 drivers/mfd/tn48m-cpld.c    |   3 +
 include/linux/mfd/tn48m.h   |   1 +
 5 files changed, 163 insertions(+)
 create mode 100644 drivers/hwmon/tn48m-hwmon.c

diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
index 54f04e61fb83..89271dfeb775 100644
--- a/drivers/hwmon/Kconfig
+++ b/drivers/hwmon/Kconfig
@@ -1924,6 +1924,16 @@ config SENSORS_TMP513
 	  This driver can also be built as a module. If so, the module
 	  will be called tmp513.
 
+config SENSORS_TN48M
+	tristate "Delta Networks TN48M switch CPLD HWMON driver"
+	depends on MFD_TN48M_CPLD
+	help
+	  If you say yes here you get support for Delta Networks TN48M
+	  CPLD.
+
+	  This driver can also be built as a module. If so, the module
+	  will be called tn48m-hwmon.
+
 config SENSORS_VEXPRESS
 	tristate "Versatile Express"
 	depends on VEXPRESS_CONFIG
diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
index fe38e8a5c979..22e470057ffe 100644
--- a/drivers/hwmon/Makefile
+++ b/drivers/hwmon/Makefile
@@ -187,6 +187,7 @@ obj-$(CONFIG_SENSORS_TMP108)	+= tmp108.o
 obj-$(CONFIG_SENSORS_TMP401)	+= tmp401.o
 obj-$(CONFIG_SENSORS_TMP421)	+= tmp421.o
 obj-$(CONFIG_SENSORS_TMP513)	+= tmp513.o
+obj-$(CONFIG_SENSORS_TN48M)	+= tn48m-hwmon.o
 obj-$(CONFIG_SENSORS_VEXPRESS)	+= vexpress-hwmon.o
 obj-$(CONFIG_SENSORS_VIA_CPUTEMP)+= via-cputemp.o
 obj-$(CONFIG_SENSORS_VIA686A)	+= via686a.o
diff --git a/drivers/hwmon/tn48m-hwmon.c b/drivers/hwmon/tn48m-hwmon.c
new file mode 100644
index 000000000000..80fd18d74f1d
--- /dev/null
+++ b/drivers/hwmon/tn48m-hwmon.c
@@ -0,0 +1,148 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Delta TN48M CPLD HWMON driver
+ *
+ * Copyright 2020 Sartura Ltd
+ *
+ * Author: Robert Marko <robert.marko@sartura.hr>
+ */
+
+#include <linux/bitfield.h>
+#include <linux/hwmon-sysfs.h>
+#include <linux/hwmon.h>
+#include <linux/kernel.h>
+#include <linux/mod_devicetable.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+
+#include <linux/mfd/tn48m.h>
+
+#define PSU1_PRESENT_MASK	BIT(0)
+#define PSU2_PRESENT_MASK	BIT(1)
+#define PSU1_POWERGOOD_MASK	BIT(2)
+#define PSU2_POWERGOOD_MASK	BIT(3)
+#define PSU1_ALERT_MASK		BIT(4)
+#define PSU2_ALERT_MASK		BIT(5)
+
+static int board_id_get(struct tn48m_data *data)
+{
+	unsigned int regval;
+
+	regmap_read(data->regmap, BOARD_ID, &regval);
+
+	switch (regval) {
+	case BOARD_ID_TN48M:
+		return BOARD_ID_TN48M;
+	case BOARD_ID_TN48M_P:
+		return BOARD_ID_TN48M_P;
+	default:
+		return -EINVAL;
+	}
+}
+
+static ssize_t psu_present_show(struct device *dev,
+				struct device_attribute *attr, char *buf)
+{
+	struct sensor_device_attribute_2 *attr2 = to_sensor_dev_attr_2(attr);
+	struct tn48m_data *data = dev_get_drvdata(dev);
+	unsigned int regval, status;
+
+	if (board_id_get(data) == BOARD_ID_TN48M_P) {
+		regmap_read(data->regmap, attr2->nr, &regval);
+
+		if (attr2->index == 1)
+			status = !FIELD_GET(PSU1_PRESENT_MASK, regval);
+		else
+			status = !FIELD_GET(PSU2_PRESENT_MASK, regval);
+	} else
+		status = 0;
+
+	return sprintf(buf, "%d\n", status);
+}
+
+static ssize_t psu_pg_show(struct device *dev,
+			   struct device_attribute *attr, char *buf)
+{
+	struct sensor_device_attribute_2 *attr2 = to_sensor_dev_attr_2(attr);
+	struct tn48m_data *data = dev_get_drvdata(dev);
+	unsigned int regval, status;
+
+	regmap_read(data->regmap, attr2->nr, &regval);
+
+	if (attr2->index == 1)
+		status = FIELD_GET(PSU1_POWERGOOD_MASK, regval);
+	else
+		status = FIELD_GET(PSU2_POWERGOOD_MASK, regval);
+
+	return sprintf(buf, "%d\n", status);
+}
+
+static ssize_t psu_alert_show(struct device *dev,
+			      struct device_attribute *attr, char *buf)
+{
+	struct sensor_device_attribute_2 *attr2 = to_sensor_dev_attr_2(attr);
+	struct tn48m_data *data = dev_get_drvdata(dev);
+	unsigned int regval, status;
+
+	if (board_id_get(data) == BOARD_ID_TN48M_P) {
+		regmap_read(data->regmap, attr2->nr, &regval);
+
+		if (attr2->index == 1)
+			status = !FIELD_GET(PSU1_ALERT_MASK, regval);
+		else
+			status = !FIELD_GET(PSU2_ALERT_MASK, regval);
+	} else
+		status = 0;
+
+	return sprintf(buf, "%d\n", status);
+}
+
+static SENSOR_DEVICE_ATTR_2_RO(psu1_present, psu_present, PSU_STATUS, 1);
+static SENSOR_DEVICE_ATTR_2_RO(psu2_present, psu_present, PSU_STATUS, 2);
+static SENSOR_DEVICE_ATTR_2_RO(psu1_pg, psu_pg, PSU_STATUS, 1);
+static SENSOR_DEVICE_ATTR_2_RO(psu2_pg, psu_pg, PSU_STATUS, 2);
+static SENSOR_DEVICE_ATTR_2_RO(psu1_alert, psu_alert, PSU_STATUS, 1);
+static SENSOR_DEVICE_ATTR_2_RO(psu2_alert, psu_alert, PSU_STATUS, 2);
+
+static struct attribute *tn48m_hwmon_attrs[] = {
+	&sensor_dev_attr_psu1_present.dev_attr.attr,
+	&sensor_dev_attr_psu2_present.dev_attr.attr,
+	&sensor_dev_attr_psu1_pg.dev_attr.attr,
+	&sensor_dev_attr_psu2_pg.dev_attr.attr,
+	&sensor_dev_attr_psu1_alert.dev_attr.attr,
+	&sensor_dev_attr_psu2_alert.dev_attr.attr,
+	NULL
+};
+
+ATTRIBUTE_GROUPS(tn48m_hwmon);
+
+static int tn48m_hwmon_probe(struct platform_device *pdev)
+{
+	struct tn48m_data *data = dev_get_drvdata(pdev->dev.parent);
+	struct device *hwmon_dev;
+
+	hwmon_dev = devm_hwmon_device_register_with_groups(&pdev->dev,
+							   "tn48m_hwmon",
+							   data,
+							   tn48m_hwmon_groups);
+	return PTR_ERR_OR_ZERO(hwmon_dev);
+}
+
+static const struct platform_device_id tn48m_hwmon_id_table[] = {
+	{ "delta,tn48m-hwmon", },
+	{ }
+};
+MODULE_DEVICE_TABLE(platform, tn48m_hwmon_id_table);
+
+static struct platform_driver tn48m_hwmon_driver = {
+	.driver = {
+		.name = "tn48m-hwmon",
+	},
+	.probe = tn48m_hwmon_probe,
+	.id_table = tn48m_hwmon_id_table,
+};
+module_platform_driver(tn48m_hwmon_driver);
+
+MODULE_AUTHOR("Robert Marko <robert.marko@sartura.hr>");
+MODULE_DESCRIPTION("Delta TN48M CPLD HWMON driver");
+MODULE_LICENSE("GPL");
diff --git a/drivers/mfd/tn48m-cpld.c b/drivers/mfd/tn48m-cpld.c
index f22a15ddd22d..4d837aca01e7 100644
--- a/drivers/mfd/tn48m-cpld.c
+++ b/drivers/mfd/tn48m-cpld.c
@@ -20,6 +20,9 @@
 static const struct mfd_cell tn48m_cell[] = {
 	{
 		.name = "delta,tn48m-gpio",
+	},
+	{
+		.name = "delta,tn48m-hwmon",
 	}
 };
 
diff --git a/include/linux/mfd/tn48m.h b/include/linux/mfd/tn48m.h
index 9cc2b04c8d69..eb2cfc3a5db7 100644
--- a/include/linux/mfd/tn48m.h
+++ b/include/linux/mfd/tn48m.h
@@ -22,6 +22,7 @@
 #define SFP_TX_DISABLE		0x31
 #define SFP_PRESENT		0x3a
 #define SFP_LOS			0x40
+#define PSU_STATUS		0xa
 
 struct tn48m_data {
 	struct device *dev;
-- 
2.31.1

Re: [PATCH 4/6] hwmon: Add Delta TN48M CPLD HWMON driver

[Guenter Roeck]

On 4/30/21 5:35 AM, Robert Marko wrote:
> Delta TN48M has a CPLD that also monitors the power supply
> statuses.
> 
> These are useful to be presented to the users, so lets
> add a driver for HWMON part of the CPLD.
> 
> Signed-off-by: Robert Marko <robert.marko@sartura.hr>
> ---
>  drivers/hwmon/Kconfig       |  10 +++
>  drivers/hwmon/Makefile      |   1 +
>  drivers/hwmon/tn48m-hwmon.c | 148 ++++++++++++++++++++++++++++++++++++
>  drivers/mfd/tn48m-cpld.c    |   3 +
>  include/linux/mfd/tn48m.h   |   1 +
>  5 files changed, 163 insertions(+)
>  create mode 100644 drivers/hwmon/tn48m-hwmon.c
> 
> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
> index 54f04e61fb83..89271dfeb775 100644
> --- a/drivers/hwmon/Kconfig
> +++ b/drivers/hwmon/Kconfig
> @@ -1924,6 +1924,16 @@ config SENSORS_TMP513
>  	  This driver can also be built as a module. If so, the module
>  	  will be called tmp513.
>  
> +config SENSORS_TN48M
> +	tristate "Delta Networks TN48M switch CPLD HWMON driver"
> +	depends on MFD_TN48M_CPLD
> +	help
> +	  If you say yes here you get support for Delta Networks TN48M
> +	  CPLD.
> +
> +	  This driver can also be built as a module. If so, the module
> +	  will be called tn48m-hwmon.
> +
>  config SENSORS_VEXPRESS
>  	tristate "Versatile Express"
>  	depends on VEXPRESS_CONFIG
> diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
> index fe38e8a5c979..22e470057ffe 100644
> --- a/drivers/hwmon/Makefile
> +++ b/drivers/hwmon/Makefile
> @@ -187,6 +187,7 @@ obj-$(CONFIG_SENSORS_TMP108)	+= tmp108.o
>  obj-$(CONFIG_SENSORS_TMP401)	+= tmp401.o
>  obj-$(CONFIG_SENSORS_TMP421)	+= tmp421.o
>  obj-$(CONFIG_SENSORS_TMP513)	+= tmp513.o
> +obj-$(CONFIG_SENSORS_TN48M)	+= tn48m-hwmon.o
>  obj-$(CONFIG_SENSORS_VEXPRESS)	+= vexpress-hwmon.o
>  obj-$(CONFIG_SENSORS_VIA_CPUTEMP)+= via-cputemp.o
>  obj-$(CONFIG_SENSORS_VIA686A)	+= via686a.o
> diff --git a/drivers/hwmon/tn48m-hwmon.c b/drivers/hwmon/tn48m-hwmon.c
> new file mode 100644
> index 000000000000..80fd18d74f1d
> --- /dev/null
> +++ b/drivers/hwmon/tn48m-hwmon.c
> @@ -0,0 +1,148 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Delta TN48M CPLD HWMON driver
> + *
> + * Copyright 2020 Sartura Ltd
> + *
> + * Author: Robert Marko <robert.marko@sartura.hr>
> + */
> +
> +#include <linux/bitfield.h>
> +#include <linux/hwmon-sysfs.h>
> +#include <linux/hwmon.h>
> +#include <linux/kernel.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +
> +#include <linux/mfd/tn48m.h>
> +
> +#define PSU1_PRESENT_MASK	BIT(0)
> +#define PSU2_PRESENT_MASK	BIT(1)
> +#define PSU1_POWERGOOD_MASK	BIT(2)
> +#define PSU2_POWERGOOD_MASK	BIT(3)
> +#define PSU1_ALERT_MASK		BIT(4)
> +#define PSU2_ALERT_MASK		BIT(5)
> +
> +static int board_id_get(struct tn48m_data *data)
> +{
> +	unsigned int regval;
> +
> +	regmap_read(data->regmap, BOARD_ID, &regval);
> +
> +	switch (regval) {
> +	case BOARD_ID_TN48M:
> +		return BOARD_ID_TN48M;
> +	case BOARD_ID_TN48M_P:
> +		return BOARD_ID_TN48M_P;
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
> +static ssize_t psu_present_show(struct device *dev,
> +				struct device_attribute *attr, char *buf)
> +{
> +	struct sensor_device_attribute_2 *attr2 = to_sensor_dev_attr_2(attr);
> +	struct tn48m_data *data = dev_get_drvdata(dev);
> +	unsigned int regval, status;
> +
> +	if (board_id_get(data) == BOARD_ID_TN48M_P) {
> +		regmap_read(data->regmap, attr2->nr, &regval);
> +
> +		if (attr2->index == 1)
> +			status = !FIELD_GET(PSU1_PRESENT_MASK, regval);
> +		else
> +			status = !FIELD_GET(PSU2_PRESENT_MASK, regval);
> +	} else
> +		status = 0;
> +
> +	return sprintf(buf, "%d\n", status);
> +}
> +
> +static ssize_t psu_pg_show(struct device *dev,
> +			   struct device_attribute *attr, char *buf)
> +{
> +	struct sensor_device_attribute_2 *attr2 = to_sensor_dev_attr_2(attr);
> +	struct tn48m_data *data = dev_get_drvdata(dev);
> +	unsigned int regval, status;
> +
> +	regmap_read(data->regmap, attr2->nr, &regval);
> +
> +	if (attr2->index == 1)
> +		status = FIELD_GET(PSU1_POWERGOOD_MASK, regval);
> +	else
> +		status = FIELD_GET(PSU2_POWERGOOD_MASK, regval);
> +
> +	return sprintf(buf, "%d\n", status);
> +}
> +
> +static ssize_t psu_alert_show(struct device *dev,
> +			      struct device_attribute *attr, char *buf)
> +{
> +	struct sensor_device_attribute_2 *attr2 = to_sensor_dev_attr_2(attr);
> +	struct tn48m_data *data = dev_get_drvdata(dev);
> +	unsigned int regval, status;
> +
> +	if (board_id_get(data) == BOARD_ID_TN48M_P) {
> +		regmap_read(data->regmap, attr2->nr, &regval);
> +
> +		if (attr2->index == 1)
> +			status = !FIELD_GET(PSU1_ALERT_MASK, regval);
> +		else
> +			status = !FIELD_GET(PSU2_ALERT_MASK, regval);
> +	} else
> +		status = 0;
> +
> +	return sprintf(buf, "%d\n", status);
> +}
> +
> +static SENSOR_DEVICE_ATTR_2_RO(psu1_present, psu_present, PSU_STATUS, 1);
> +static SENSOR_DEVICE_ATTR_2_RO(psu2_present, psu_present, PSU_STATUS, 2);
> +static SENSOR_DEVICE_ATTR_2_RO(psu1_pg, psu_pg, PSU_STATUS, 1);
> +static SENSOR_DEVICE_ATTR_2_RO(psu2_pg, psu_pg, PSU_STATUS, 2);
> +static SENSOR_DEVICE_ATTR_2_RO(psu1_alert, psu_alert, PSU_STATUS, 1);
> +static SENSOR_DEVICE_ATTR_2_RO(psu2_alert, psu_alert, PSU_STATUS, 2);
> +
> +static struct attribute *tn48m_hwmon_attrs[] = {
> +	&sensor_dev_attr_psu1_present.dev_attr.attr,
> +	&sensor_dev_attr_psu2_present.dev_attr.attr,
> +	&sensor_dev_attr_psu1_pg.dev_attr.attr,
> +	&sensor_dev_attr_psu2_pg.dev_attr.attr,
> +	&sensor_dev_attr_psu1_alert.dev_attr.attr,
> +	&sensor_dev_attr_psu2_alert.dev_attr.attr,

Literally none of those attributes are standard hwmon attributes.
I don't know what this is, but it is not a hardware monitoring driver.

> +	NULL
> +};
> +
> +ATTRIBUTE_GROUPS(tn48m_hwmon);
> +
> +static int tn48m_hwmon_probe(struct platform_device *pdev)
> +{
> +	struct tn48m_data *data = dev_get_drvdata(pdev->dev.parent);
> +	struct device *hwmon_dev;
> +
> +	hwmon_dev = devm_hwmon_device_register_with_groups(&pdev->dev,
> +							   "tn48m_hwmon",
> +							   data,
> +							   tn48m_hwmon_groups);

Please use devm_hwmon_device_register_with_info() to register hwmon devices.
Of course, that only makes sense for actual hardware monitoring drivers
which do support standard attributes.

> +	return PTR_ERR_OR_ZERO(hwmon_dev);
> +}
> +
> +static const struct platform_device_id tn48m_hwmon_id_table[] = {
> +	{ "delta,tn48m-hwmon", },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(platform, tn48m_hwmon_id_table);
> +
> +static struct platform_driver tn48m_hwmon_driver = {
> +	.driver = {
> +		.name = "tn48m-hwmon",
> +	},
> +	.probe = tn48m_hwmon_probe,
> +	.id_table = tn48m_hwmon_id_table,
> +};
> +module_platform_driver(tn48m_hwmon_driver);
> +
> +MODULE_AUTHOR("Robert Marko <robert.marko@sartura.hr>");
> +MODULE_DESCRIPTION("Delta TN48M CPLD HWMON driver");
> +MODULE_LICENSE("GPL");
> diff --git a/drivers/mfd/tn48m-cpld.c b/drivers/mfd/tn48m-cpld.c
> index f22a15ddd22d..4d837aca01e7 100644
> --- a/drivers/mfd/tn48m-cpld.c
> +++ b/drivers/mfd/tn48m-cpld.c
> @@ -20,6 +20,9 @@
>  static const struct mfd_cell tn48m_cell[] = {
>  	{
>  		.name = "delta,tn48m-gpio",
> +	},
> +	{
> +		.name = "delta,tn48m-hwmon",
>  	}
>  };
>  
> diff --git a/include/linux/mfd/tn48m.h b/include/linux/mfd/tn48m.h
> index 9cc2b04c8d69..eb2cfc3a5db7 100644
> --- a/include/linux/mfd/tn48m.h
> +++ b/include/linux/mfd/tn48m.h
> @@ -22,6 +22,7 @@
>  #define SFP_TX_DISABLE		0x31
>  #define SFP_PRESENT		0x3a
>  #define SFP_LOS			0x40
> +#define PSU_STATUS		0xa
>  
>  struct tn48m_data {
>  	struct device *dev;
> 

Re: [PATCH 4/6] hwmon: Add Delta TN48M CPLD HWMON driver

[Robert Marko]

On Fri, Apr 30, 2021 at 3:12 PM Guenter Roeck <linux@roeck-us.net> wrote:
>
> On 4/30/21 5:35 AM, Robert Marko wrote:
> > Delta TN48M has a CPLD that also monitors the power supply
> > statuses.
> >
> > These are useful to be presented to the users, so lets
> > add a driver for HWMON part of the CPLD.
> >
> > Signed-off-by: Robert Marko <robert.marko@sartura.hr>
> > ---
> >  drivers/hwmon/Kconfig       |  10 +++
> >  drivers/hwmon/Makefile      |   1 +
> >  drivers/hwmon/tn48m-hwmon.c | 148 ++++++++++++++++++++++++++++++++++++
> >  drivers/mfd/tn48m-cpld.c    |   3 +
> >  include/linux/mfd/tn48m.h   |   1 +
> >  5 files changed, 163 insertions(+)
> >  create mode 100644 drivers/hwmon/tn48m-hwmon.c
> >
> > diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
> > index 54f04e61fb83..89271dfeb775 100644
> > --- a/drivers/hwmon/Kconfig
> > +++ b/drivers/hwmon/Kconfig
> > @@ -1924,6 +1924,16 @@ config SENSORS_TMP513
> >         This driver can also be built as a module. If so, the module
> >         will be called tmp513.
> >
> > +config SENSORS_TN48M
> > +     tristate "Delta Networks TN48M switch CPLD HWMON driver"
> > +     depends on MFD_TN48M_CPLD
> > +     help
> > +       If you say yes here you get support for Delta Networks TN48M
> > +       CPLD.
> > +
> > +       This driver can also be built as a module. If so, the module
> > +       will be called tn48m-hwmon.
> > +
> >  config SENSORS_VEXPRESS
> >       tristate "Versatile Express"
> >       depends on VEXPRESS_CONFIG
> > diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
> > index fe38e8a5c979..22e470057ffe 100644
> > --- a/drivers/hwmon/Makefile
> > +++ b/drivers/hwmon/Makefile
> > @@ -187,6 +187,7 @@ obj-$(CONFIG_SENSORS_TMP108)      += tmp108.o
> >  obj-$(CONFIG_SENSORS_TMP401) += tmp401.o
> >  obj-$(CONFIG_SENSORS_TMP421) += tmp421.o
> >  obj-$(CONFIG_SENSORS_TMP513) += tmp513.o
> > +obj-$(CONFIG_SENSORS_TN48M)  += tn48m-hwmon.o
> >  obj-$(CONFIG_SENSORS_VEXPRESS)       += vexpress-hwmon.o
> >  obj-$(CONFIG_SENSORS_VIA_CPUTEMP)+= via-cputemp.o
> >  obj-$(CONFIG_SENSORS_VIA686A)        += via686a.o
> > diff --git a/drivers/hwmon/tn48m-hwmon.c b/drivers/hwmon/tn48m-hwmon.c
> > new file mode 100644
> > index 000000000000..80fd18d74f1d
> > --- /dev/null
> > +++ b/drivers/hwmon/tn48m-hwmon.c
> > @@ -0,0 +1,148 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/*
> > + * Delta TN48M CPLD HWMON driver
> > + *
> > + * Copyright 2020 Sartura Ltd
> > + *
> > + * Author: Robert Marko <robert.marko@sartura.hr>
> > + */
> > +
> > +#include <linux/bitfield.h>
> > +#include <linux/hwmon-sysfs.h>
> > +#include <linux/hwmon.h>
> > +#include <linux/kernel.h>
> > +#include <linux/mod_devicetable.h>
> > +#include <linux/module.h>
> > +#include <linux/platform_device.h>
> > +
> > +#include <linux/mfd/tn48m.h>
> > +
> > +#define PSU1_PRESENT_MASK    BIT(0)
> > +#define PSU2_PRESENT_MASK    BIT(1)
> > +#define PSU1_POWERGOOD_MASK  BIT(2)
> > +#define PSU2_POWERGOOD_MASK  BIT(3)
> > +#define PSU1_ALERT_MASK              BIT(4)
> > +#define PSU2_ALERT_MASK              BIT(5)
> > +
> > +static int board_id_get(struct tn48m_data *data)
> > +{
> > +     unsigned int regval;
> > +
> > +     regmap_read(data->regmap, BOARD_ID, &regval);
> > +
> > +     switch (regval) {
> > +     case BOARD_ID_TN48M:
> > +             return BOARD_ID_TN48M;
> > +     case BOARD_ID_TN48M_P:
> > +             return BOARD_ID_TN48M_P;
> > +     default:
> > +             return -EINVAL;
> > +     }
> > +}
> > +
> > +static ssize_t psu_present_show(struct device *dev,
> > +                             struct device_attribute *attr, char *buf)
> > +{
> > +     struct sensor_device_attribute_2 *attr2 = to_sensor_dev_attr_2(attr);
> > +     struct tn48m_data *data = dev_get_drvdata(dev);
> > +     unsigned int regval, status;
> > +
> > +     if (board_id_get(data) == BOARD_ID_TN48M_P) {
> > +             regmap_read(data->regmap, attr2->nr, &regval);
> > +
> > +             if (attr2->index == 1)
> > +                     status = !FIELD_GET(PSU1_PRESENT_MASK, regval);
> > +             else
> > +                     status = !FIELD_GET(PSU2_PRESENT_MASK, regval);
> > +     } else
> > +             status = 0;
> > +
> > +     return sprintf(buf, "%d\n", status);
> > +}
> > +
> > +static ssize_t psu_pg_show(struct device *dev,
> > +                        struct device_attribute *attr, char *buf)
> > +{
> > +     struct sensor_device_attribute_2 *attr2 = to_sensor_dev_attr_2(attr);
> > +     struct tn48m_data *data = dev_get_drvdata(dev);
> > +     unsigned int regval, status;
> > +
> > +     regmap_read(data->regmap, attr2->nr, &regval);
> > +
> > +     if (attr2->index == 1)
> > +             status = FIELD_GET(PSU1_POWERGOOD_MASK, regval);
> > +     else
> > +             status = FIELD_GET(PSU2_POWERGOOD_MASK, regval);
> > +
> > +     return sprintf(buf, "%d\n", status);
> > +}
> > +
> > +static ssize_t psu_alert_show(struct device *dev,
> > +                           struct device_attribute *attr, char *buf)
> > +{
> > +     struct sensor_device_attribute_2 *attr2 = to_sensor_dev_attr_2(attr);
> > +     struct tn48m_data *data = dev_get_drvdata(dev);
> > +     unsigned int regval, status;
> > +
> > +     if (board_id_get(data) == BOARD_ID_TN48M_P) {
> > +             regmap_read(data->regmap, attr2->nr, &regval);
> > +
> > +             if (attr2->index == 1)
> > +                     status = !FIELD_GET(PSU1_ALERT_MASK, regval);
> > +             else
> > +                     status = !FIELD_GET(PSU2_ALERT_MASK, regval);
> > +     } else
> > +             status = 0;
> > +
> > +     return sprintf(buf, "%d\n", status);
> > +}
> > +
> > +static SENSOR_DEVICE_ATTR_2_RO(psu1_present, psu_present, PSU_STATUS, 1);
> > +static SENSOR_DEVICE_ATTR_2_RO(psu2_present, psu_present, PSU_STATUS, 2);
> > +static SENSOR_DEVICE_ATTR_2_RO(psu1_pg, psu_pg, PSU_STATUS, 1);
> > +static SENSOR_DEVICE_ATTR_2_RO(psu2_pg, psu_pg, PSU_STATUS, 2);
> > +static SENSOR_DEVICE_ATTR_2_RO(psu1_alert, psu_alert, PSU_STATUS, 1);
> > +static SENSOR_DEVICE_ATTR_2_RO(psu2_alert, psu_alert, PSU_STATUS, 2);
> > +
> > +static struct attribute *tn48m_hwmon_attrs[] = {
> > +     &sensor_dev_attr_psu1_present.dev_attr.attr,
> > +     &sensor_dev_attr_psu2_present.dev_attr.attr,
> > +     &sensor_dev_attr_psu1_pg.dev_attr.attr,
> > +     &sensor_dev_attr_psu2_pg.dev_attr.attr,
> > +     &sensor_dev_attr_psu1_alert.dev_attr.attr,
> > +     &sensor_dev_attr_psu2_alert.dev_attr.attr,
>
> Literally none of those attributes are standard hwmon attributes.
> I don't know what this is, but it is not a hardware monitoring driver.

Yes, I agree that it does not expose any of the standard attributes, but these
are the only ones the CPLD exposes.

I don't know where else to put them, MFD driver did not seem logical to me.
>
> > +     NULL
> > +};
> > +
> > +ATTRIBUTE_GROUPS(tn48m_hwmon);
> > +
> > +static int tn48m_hwmon_probe(struct platform_device *pdev)
> > +{
> > +     struct tn48m_data *data = dev_get_drvdata(pdev->dev.parent);
> > +     struct device *hwmon_dev;
> > +
> > +     hwmon_dev = devm_hwmon_device_register_with_groups(&pdev->dev,
> > +                                                        "tn48m_hwmon",
> > +                                                        data,
> > +                                                        tn48m_hwmon_groups);
>
> Please use devm_hwmon_device_register_with_info() to register hwmon devices.
> Of course, that only makes sense for actual hardware monitoring drivers
> which do support standard attributes.

Yes, devm_hwmon_device_register_with_info() made no sense without any of the
standard attributes.

Robert
>
> > +     return PTR_ERR_OR_ZERO(hwmon_dev);
> > +}
> > +
> > +static const struct platform_device_id tn48m_hwmon_id_table[] = {
> > +     { "delta,tn48m-hwmon", },
> > +     { }
> > +};
> > +MODULE_DEVICE_TABLE(platform, tn48m_hwmon_id_table);
> > +
> > +static struct platform_driver tn48m_hwmon_driver = {
> > +     .driver = {
> > +             .name = "tn48m-hwmon",
> > +     },
> > +     .probe = tn48m_hwmon_probe,
> > +     .id_table = tn48m_hwmon_id_table,
> > +};
> > +module_platform_driver(tn48m_hwmon_driver);
> > +
> > +MODULE_AUTHOR("Robert Marko <robert.marko@sartura.hr>");
> > +MODULE_DESCRIPTION("Delta TN48M CPLD HWMON driver");
> > +MODULE_LICENSE("GPL");
> > diff --git a/drivers/mfd/tn48m-cpld.c b/drivers/mfd/tn48m-cpld.c
> > index f22a15ddd22d..4d837aca01e7 100644
> > --- a/drivers/mfd/tn48m-cpld.c
> > +++ b/drivers/mfd/tn48m-cpld.c
> > @@ -20,6 +20,9 @@
> >  static const struct mfd_cell tn48m_cell[] = {
> >       {
> >               .name = "delta,tn48m-gpio",
> > +     },
> > +     {
> > +             .name = "delta,tn48m-hwmon",
> >       }
> >  };
> >
> > diff --git a/include/linux/mfd/tn48m.h b/include/linux/mfd/tn48m.h
> > index 9cc2b04c8d69..eb2cfc3a5db7 100644
> > --- a/include/linux/mfd/tn48m.h
> > +++ b/include/linux/mfd/tn48m.h
> > @@ -22,6 +22,7 @@
> >  #define SFP_TX_DISABLE               0x31
> >  #define SFP_PRESENT          0x3a
> >  #define SFP_LOS                      0x40
> > +#define PSU_STATUS           0xa
> >
> >  struct tn48m_data {
> >       struct device *dev;
> >
>


-- 
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 4/6] hwmon: Add Delta TN48M CPLD HWMON driver

[Guenter Roeck]

On 5/19/21 5:01 AM, Robert Marko wrote:
> On Fri, Apr 30, 2021 at 3:12 PM Guenter Roeck <linux@roeck-us.net> wrote:
>>
>> On 4/30/21 5:35 AM, Robert Marko wrote:
>>> Delta TN48M has a CPLD that also monitors the power supply
>>> statuses.
>>>
>>> These are useful to be presented to the users, so lets
>>> add a driver for HWMON part of the CPLD.
>>>
>>> Signed-off-by: Robert Marko <robert.marko@sartura.hr>
>>> ---
>>>   drivers/hwmon/Kconfig       |  10 +++
>>>   drivers/hwmon/Makefile      |   1 +
>>>   drivers/hwmon/tn48m-hwmon.c | 148 ++++++++++++++++++++++++++++++++++++
>>>   drivers/mfd/tn48m-cpld.c    |   3 +
>>>   include/linux/mfd/tn48m.h   |   1 +
>>>   5 files changed, 163 insertions(+)
>>>   create mode 100644 drivers/hwmon/tn48m-hwmon.c
>>>
>>> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
>>> index 54f04e61fb83..89271dfeb775 100644
>>> --- a/drivers/hwmon/Kconfig
>>> +++ b/drivers/hwmon/Kconfig
>>> @@ -1924,6 +1924,16 @@ config SENSORS_TMP513
>>>          This driver can also be built as a module. If so, the module
>>>          will be called tmp513.
>>>
>>> +config SENSORS_TN48M
>>> +     tristate "Delta Networks TN48M switch CPLD HWMON driver"
>>> +     depends on MFD_TN48M_CPLD
>>> +     help
>>> +       If you say yes here you get support for Delta Networks TN48M
>>> +       CPLD.
>>> +
>>> +       This driver can also be built as a module. If so, the module
>>> +       will be called tn48m-hwmon.
>>> +
>>>   config SENSORS_VEXPRESS
>>>        tristate "Versatile Express"
>>>        depends on VEXPRESS_CONFIG
>>> diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
>>> index fe38e8a5c979..22e470057ffe 100644
>>> --- a/drivers/hwmon/Makefile
>>> +++ b/drivers/hwmon/Makefile
>>> @@ -187,6 +187,7 @@ obj-$(CONFIG_SENSORS_TMP108)      += tmp108.o
>>>   obj-$(CONFIG_SENSORS_TMP401) += tmp401.o
>>>   obj-$(CONFIG_SENSORS_TMP421) += tmp421.o
>>>   obj-$(CONFIG_SENSORS_TMP513) += tmp513.o
>>> +obj-$(CONFIG_SENSORS_TN48M)  += tn48m-hwmon.o
>>>   obj-$(CONFIG_SENSORS_VEXPRESS)       += vexpress-hwmon.o
>>>   obj-$(CONFIG_SENSORS_VIA_CPUTEMP)+= via-cputemp.o
>>>   obj-$(CONFIG_SENSORS_VIA686A)        += via686a.o
>>> diff --git a/drivers/hwmon/tn48m-hwmon.c b/drivers/hwmon/tn48m-hwmon.c
>>> new file mode 100644
>>> index 000000000000..80fd18d74f1d
>>> --- /dev/null
>>> +++ b/drivers/hwmon/tn48m-hwmon.c
>>> @@ -0,0 +1,148 @@
>>> +// SPDX-License-Identifier: GPL-2.0-only
>>> +/*
>>> + * Delta TN48M CPLD HWMON driver
>>> + *
>>> + * Copyright 2020 Sartura Ltd
>>> + *
>>> + * Author: Robert Marko <robert.marko@sartura.hr>
>>> + */
>>> +
>>> +#include <linux/bitfield.h>
>>> +#include <linux/hwmon-sysfs.h>
>>> +#include <linux/hwmon.h>
>>> +#include <linux/kernel.h>
>>> +#include <linux/mod_devicetable.h>
>>> +#include <linux/module.h>
>>> +#include <linux/platform_device.h>
>>> +
>>> +#include <linux/mfd/tn48m.h>
>>> +
>>> +#define PSU1_PRESENT_MASK    BIT(0)
>>> +#define PSU2_PRESENT_MASK    BIT(1)
>>> +#define PSU1_POWERGOOD_MASK  BIT(2)
>>> +#define PSU2_POWERGOOD_MASK  BIT(3)
>>> +#define PSU1_ALERT_MASK              BIT(4)
>>> +#define PSU2_ALERT_MASK              BIT(5)
>>> +
>>> +static int board_id_get(struct tn48m_data *data)
>>> +{
>>> +     unsigned int regval;
>>> +
>>> +     regmap_read(data->regmap, BOARD_ID, &regval);
>>> +
>>> +     switch (regval) {
>>> +     case BOARD_ID_TN48M:
>>> +             return BOARD_ID_TN48M;
>>> +     case BOARD_ID_TN48M_P:
>>> +             return BOARD_ID_TN48M_P;
>>> +     default:
>>> +             return -EINVAL;
>>> +     }
>>> +}
>>> +
>>> +static ssize_t psu_present_show(struct device *dev,
>>> +                             struct device_attribute *attr, char *buf)
>>> +{
>>> +     struct sensor_device_attribute_2 *attr2 = to_sensor_dev_attr_2(attr);
>>> +     struct tn48m_data *data = dev_get_drvdata(dev);
>>> +     unsigned int regval, status;
>>> +
>>> +     if (board_id_get(data) == BOARD_ID_TN48M_P) {
>>> +             regmap_read(data->regmap, attr2->nr, &regval);
>>> +
>>> +             if (attr2->index == 1)
>>> +                     status = !FIELD_GET(PSU1_PRESENT_MASK, regval);
>>> +             else
>>> +                     status = !FIELD_GET(PSU2_PRESENT_MASK, regval);
>>> +     } else
>>> +             status = 0;
>>> +
>>> +     return sprintf(buf, "%d\n", status);
>>> +}
>>> +
>>> +static ssize_t psu_pg_show(struct device *dev,
>>> +                        struct device_attribute *attr, char *buf)
>>> +{
>>> +     struct sensor_device_attribute_2 *attr2 = to_sensor_dev_attr_2(attr);
>>> +     struct tn48m_data *data = dev_get_drvdata(dev);
>>> +     unsigned int regval, status;
>>> +
>>> +     regmap_read(data->regmap, attr2->nr, &regval);
>>> +
>>> +     if (attr2->index == 1)
>>> +             status = FIELD_GET(PSU1_POWERGOOD_MASK, regval);
>>> +     else
>>> +             status = FIELD_GET(PSU2_POWERGOOD_MASK, regval);
>>> +
>>> +     return sprintf(buf, "%d\n", status);
>>> +}
>>> +
>>> +static ssize_t psu_alert_show(struct device *dev,
>>> +                           struct device_attribute *attr, char *buf)
>>> +{
>>> +     struct sensor_device_attribute_2 *attr2 = to_sensor_dev_attr_2(attr);
>>> +     struct tn48m_data *data = dev_get_drvdata(dev);
>>> +     unsigned int regval, status;
>>> +
>>> +     if (board_id_get(data) == BOARD_ID_TN48M_P) {
>>> +             regmap_read(data->regmap, attr2->nr, &regval);
>>> +
>>> +             if (attr2->index == 1)
>>> +                     status = !FIELD_GET(PSU1_ALERT_MASK, regval);
>>> +             else
>>> +                     status = !FIELD_GET(PSU2_ALERT_MASK, regval);
>>> +     } else
>>> +             status = 0;
>>> +
>>> +     return sprintf(buf, "%d\n", status);
>>> +}
>>> +
>>> +static SENSOR_DEVICE_ATTR_2_RO(psu1_present, psu_present, PSU_STATUS, 1);
>>> +static SENSOR_DEVICE_ATTR_2_RO(psu2_present, psu_present, PSU_STATUS, 2);
>>> +static SENSOR_DEVICE_ATTR_2_RO(psu1_pg, psu_pg, PSU_STATUS, 1);
>>> +static SENSOR_DEVICE_ATTR_2_RO(psu2_pg, psu_pg, PSU_STATUS, 2);
>>> +static SENSOR_DEVICE_ATTR_2_RO(psu1_alert, psu_alert, PSU_STATUS, 1);
>>> +static SENSOR_DEVICE_ATTR_2_RO(psu2_alert, psu_alert, PSU_STATUS, 2);
>>> +
>>> +static struct attribute *tn48m_hwmon_attrs[] = {
>>> +     &sensor_dev_attr_psu1_present.dev_attr.attr,
>>> +     &sensor_dev_attr_psu2_present.dev_attr.attr,
>>> +     &sensor_dev_attr_psu1_pg.dev_attr.attr,
>>> +     &sensor_dev_attr_psu2_pg.dev_attr.attr,
>>> +     &sensor_dev_attr_psu1_alert.dev_attr.attr,
>>> +     &sensor_dev_attr_psu2_alert.dev_attr.attr,
>>
>> Literally none of those attributes are standard hwmon attributes.
>> I don't know what this is, but it is not a hardware monitoring driver.
> 
> Yes, I agree that it does not expose any of the standard attributes, but these
> are the only ones the CPLD exposes.
> 
> I don't know where else to put them, MFD driver did not seem logical to me.
>>
>>> +     NULL
>>> +};
>>> +
>>> +ATTRIBUTE_GROUPS(tn48m_hwmon);
>>> +
>>> +static int tn48m_hwmon_probe(struct platform_device *pdev)
>>> +{
>>> +     struct tn48m_data *data = dev_get_drvdata(pdev->dev.parent);
>>> +     struct device *hwmon_dev;
>>> +
>>> +     hwmon_dev = devm_hwmon_device_register_with_groups(&pdev->dev,
>>> +                                                        "tn48m_hwmon",
>>> +                                                        data,
>>> +                                                        tn48m_hwmon_groups);
>>
>> Please use devm_hwmon_device_register_with_info() to register hwmon devices.
>> Of course, that only makes sense for actual hardware monitoring drivers
>> which do support standard attributes.
> 
> Yes, devm_hwmon_device_register_with_info() made no sense without any of the
> standard attributes.
> 

I would suggest to expose the information using debugfs.
Again, this is not a hardware monitoring driver.

Guenter

> Robert
>>
>>> +     return PTR_ERR_OR_ZERO(hwmon_dev);
>>> +}
>>> +
>>> +static const struct platform_device_id tn48m_hwmon_id_table[] = {
>>> +     { "delta,tn48m-hwmon", },
>>> +     { }
>>> +};
>>> +MODULE_DEVICE_TABLE(platform, tn48m_hwmon_id_table);
>>> +
>>> +static struct platform_driver tn48m_hwmon_driver = {
>>> +     .driver = {
>>> +             .name = "tn48m-hwmon",
>>> +     },
>>> +     .probe = tn48m_hwmon_probe,
>>> +     .id_table = tn48m_hwmon_id_table,
>>> +};
>>> +module_platform_driver(tn48m_hwmon_driver);
>>> +
>>> +MODULE_AUTHOR("Robert Marko <robert.marko@sartura.hr>");
>>> +MODULE_DESCRIPTION("Delta TN48M CPLD HWMON driver");
>>> +MODULE_LICENSE("GPL");
>>> diff --git a/drivers/mfd/tn48m-cpld.c b/drivers/mfd/tn48m-cpld.c
>>> index f22a15ddd22d..4d837aca01e7 100644
>>> --- a/drivers/mfd/tn48m-cpld.c
>>> +++ b/drivers/mfd/tn48m-cpld.c
>>> @@ -20,6 +20,9 @@
>>>   static const struct mfd_cell tn48m_cell[] = {
>>>        {
>>>                .name = "delta,tn48m-gpio",
>>> +     },
>>> +     {
>>> +             .name = "delta,tn48m-hwmon",
>>>        }
>>>   };
>>>
>>> diff --git a/include/linux/mfd/tn48m.h b/include/linux/mfd/tn48m.h
>>> index 9cc2b04c8d69..eb2cfc3a5db7 100644
>>> --- a/include/linux/mfd/tn48m.h
>>> +++ b/include/linux/mfd/tn48m.h
>>> @@ -22,6 +22,7 @@
>>>   #define SFP_TX_DISABLE               0x31
>>>   #define SFP_PRESENT          0x3a
>>>   #define SFP_LOS                      0x40
>>> +#define PSU_STATUS           0xa
>>>
>>>   struct tn48m_data {
>>>        struct device *dev;
>>>
>>
> 
> 

[PATCH 5/6] dt-bindings: mfd: Add Delta TN48M CPLD drivers bindings

[Robert Marko]

Add binding document for the Delta TN48M CPLD drivers.

Signed-off-by: Robert Marko <robert.marko@sartura.hr>
---
 .../bindings/mfd/delta,tn48m-cpld.yaml        | 59 +++++++++++++++++++
 1 file changed, 59 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/mfd/delta,tn48m-cpld.yaml

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..7d81943d3d27
--- /dev/null
+++ b/Documentation/devicetree/bindings/mfd/delta,tn48m-cpld.yaml
@@ -0,0 +1,59 @@
+# 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
+
+  gpio-controller: true
+
+  "#gpio-cells":
+    const: 2
+    description:
+      The first cell is the pin number and the second cell is used to specify
+      the gpio active state.
+
+required:
+  - compatible
+  - reg
+  - gpio-controller
+  - "#gpio-cells"
+
+additionalProperties: false
+
+examples:
+  - |
+    i2c {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        cpld@41 {
+            compatible = "delta,tn48m-cpld";
+            reg = <0x41>;
+
+            gpio-controller;
+            #gpio-cells = <2>;
+        };
+    };
-- 
2.31.1

Re: [PATCH 5/6] dt-bindings: mfd: Add Delta TN48M CPLD drivers bindings

[Rob Herring]

On Fri, 30 Apr 2021 14:35:10 +0200, Robert Marko wrote:
> Add binding document for the Delta TN48M CPLD drivers.
> 
> Signed-off-by: Robert Marko <robert.marko@sartura.hr>
> ---
>  .../bindings/mfd/delta,tn48m-cpld.yaml        | 59 +++++++++++++++++++
>  1 file changed, 59 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/mfd/delta,tn48m-cpld.yaml
> 

Reviewed-by: Rob Herring <robh@kernel.org>

[PATCH 6/6] 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>
---
 MAINTAINERS | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index 9450e052f1b1..7193624868c7 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -5096,6 +5096,16 @@ 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/mfd/delta,tn48m-cpld.yaml
+F:	drivers/gpio/gpio-tn48m.c
+F:	drivers/hwmon/tn48m-hwmon.c
+F:	drivers/mfd/tn48m-cpld.c
+F:	include/dt-bindings/gpio/tn48m-gpio.h
+F:	include/linux/mfd/tn48m.h
+
 DENALI NAND DRIVER
 L:	linux-mtd@lists.infradead.org
 S:	Orphan
-- 
2.31.1

Re: [PATCH 1/6] mfd: Add Delta TN48M CPLD driver

[Michael Walle]

Hi Robert,

Am 2021-04-30 14:35, schrieb Robert Marko:
> Delta TN48M switches have a Lattice CPLD that serves
> multiple purposes including being a GPIO expander.
> So lets add the MFD core driver for it.

Did you have a look at mfd/simple-mfd-i2c.c?

-michael

Re: [PATCH 1/6] mfd: Add Delta TN48M CPLD driver

[Robert Marko]

On Thu, May 6, 2021 at 6:34 PM Michael Walle <michael@walle.cc> wrote:
>
> Hi Robert,
>
> Am 2021-04-30 14:35, schrieb Robert Marko:
> > Delta TN48M switches have a Lattice CPLD that serves
> > multiple purposes including being a GPIO expander.
> > So lets add the MFD core driver for it.
>
> Did you have a look at mfd/simple-mfd-i2c.c?

Hi Michael,

Yes, that was my first idea but we have a requirement to expose CPLD
information via debugfs as there are userspace applications using it.
And simple-mfd-i2c does not allow us to do so.

Regards,
Robert
>
> -michael



-- 
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 1/6] mfd: Add Delta TN48M CPLD driver

[Michael Walle]

Hi,

Am 2021-05-19 13:53, schrieb Robert Marko:
> On Thu, May 6, 2021 at 6:34 PM Michael Walle <michael@walle.cc> wrote:
>> Am 2021-04-30 14:35, schrieb Robert Marko:
>> > Delta TN48M switches have a Lattice CPLD that serves
>> > multiple purposes including being a GPIO expander.
>> > So lets add the MFD core driver for it.
>> 
>> Did you have a look at mfd/simple-mfd-i2c.c?
> 
> Yes, that was my first idea but we have a requirement to expose CPLD
> information via debugfs as there are userspace applications using it.
> And simple-mfd-i2c does not allow us to do so.

Mh, last time Lee wasn't very fond of having a driver that just 
populates
sub-drivers while doing almost nothing itself. See
https://lore.kernel.org/lkml/20200605065709.GD3714@dell/

That being said, I'd also like to expose our CPLD version, but until now
haven't found a good solution.

-michael

Re: [PATCH 1/6] mfd: Add Delta TN48M CPLD driver

[Lee Jones]

On Wed, 19 May 2021, Michael Walle wrote:

> Hi,
> 
> Am 2021-05-19 13:53, schrieb Robert Marko:
> > On Thu, May 6, 2021 at 6:34 PM Michael Walle <michael@walle.cc> wrote:
> > > Am 2021-04-30 14:35, schrieb Robert Marko:
> > > > Delta TN48M switches have a Lattice CPLD that serves
> > > > multiple purposes including being a GPIO expander.
> > > > So lets add the MFD core driver for it.
> > > 
> > > Did you have a look at mfd/simple-mfd-i2c.c?
> > 
> > Yes, that was my first idea but we have a requirement to expose CPLD
> > information via debugfs as there are userspace applications using it.
> > And simple-mfd-i2c does not allow us to do so.
> 
> Mh, last time Lee wasn't very fond of having a driver that just populates
> sub-drivers while doing almost nothing itself. See
> https://lore.kernel.org/lkml/20200605065709.GD3714@dell/

Right.  I still feel that way.

> That being said, I'd also like to expose our CPLD version, but until now
> haven't found a good solution.

Why though?  Does S/W *need* it?

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

Re: [PATCH 1/6] mfd: Add Delta TN48M CPLD driver

[Robert Marko]

On Thu, May 20, 2021 at 8:49 AM Lee Jones <lee.jones@linaro.org> wrote:
>
> On Wed, 19 May 2021, Michael Walle wrote:
>
> > Hi,
> >
> > Am 2021-05-19 13:53, schrieb Robert Marko:
> > > On Thu, May 6, 2021 at 6:34 PM Michael Walle <michael@walle.cc> wrote:
> > > > Am 2021-04-30 14:35, schrieb Robert Marko:
> > > > > Delta TN48M switches have a Lattice CPLD that serves
> > > > > multiple purposes including being a GPIO expander.
> > > > > So lets add the MFD core driver for it.
> > > >
> > > > Did you have a look at mfd/simple-mfd-i2c.c?
> > >
> > > Yes, that was my first idea but we have a requirement to expose CPLD
> > > information via debugfs as there are userspace applications using it.
> > > And simple-mfd-i2c does not allow us to do so.
> >
> > Mh, last time Lee wasn't very fond of having a driver that just populates
> > sub-drivers while doing almost nothing itself. See
> > https://lore.kernel.org/lkml/20200605065709.GD3714@dell/
>
> Right.  I still feel that way.
>
> > That being said, I'd also like to expose our CPLD version, but until now
> > haven't found a good solution.
>
> Why though?  Does S/W *need* it?
Because we have userspace S/W that uses it as the same CPLD is in
multiple variants of the board but the correct board model is set during
manufacturing and we can read it from the CPLD.

We also have information about PSU1 and PSU2(Some models only)
power good, whether they are present and some other info that I need
to expose as these are monitored in userspace.

I planned to do that via the hwmon driver but according to Guenther they
are not hwmon attributes and I agree.

Would it be possible to have a dedicated driver that would only expose
the required information via debugfs?
Then I could simply use the simple I2C MFD driver with only a GPIO
driver on top of it.

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 1/6] mfd: Add Delta TN48M CPLD driver

[Lee Jones]

On Fri, 21 May 2021, Robert Marko wrote:

> On Thu, May 20, 2021 at 8:49 AM Lee Jones <lee.jones@linaro.org> wrote:
> >
> > On Wed, 19 May 2021, Michael Walle wrote:
> >
> > > Hi,
> > >
> > > Am 2021-05-19 13:53, schrieb Robert Marko:
> > > > On Thu, May 6, 2021 at 6:34 PM Michael Walle <michael@walle.cc> wrote:
> > > > > Am 2021-04-30 14:35, schrieb Robert Marko:
> > > > > > Delta TN48M switches have a Lattice CPLD that serves
> > > > > > multiple purposes including being a GPIO expander.
> > > > > > So lets add the MFD core driver for it.
> > > > >
> > > > > Did you have a look at mfd/simple-mfd-i2c.c?
> > > >
> > > > Yes, that was my first idea but we have a requirement to expose CPLD
> > > > information via debugfs as there are userspace applications using it.
> > > > And simple-mfd-i2c does not allow us to do so.
> > >
> > > Mh, last time Lee wasn't very fond of having a driver that just populates
> > > sub-drivers while doing almost nothing itself. See
> > > https://lore.kernel.org/lkml/20200605065709.GD3714@dell/
> >
> > Right.  I still feel that way.
> >
> > > That being said, I'd also like to expose our CPLD version, but until now
> > > haven't found a good solution.
> >
> > Why though?  Does S/W *need* it?
> Because we have userspace S/W that uses it as the same CPLD is in
> multiple variants of the board but the correct board model is set during
> manufacturing and we can read it from the CPLD.
> 
> We also have information about PSU1 and PSU2(Some models only)
> power good, whether they are present and some other info that I need
> to expose as these are monitored in userspace.
> 
> I planned to do that via the hwmon driver but according to Guenther they
> are not hwmon attributes and I agree.
> 
> Would it be possible to have a dedicated driver that would only expose
> the required information via debugfs?
> Then I could simply use the simple I2C MFD driver with only a GPIO
> driver on top of it.

Yes, I was going to suggest that.

It should probably live in drivers/misc.

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

Re: [PATCH 1/6] mfd: Add Delta TN48M CPLD driver

[Robert Marko]

On Fri, May 21, 2021 at 11:04 AM Lee Jones <lee.jones@linaro.org> wrote:
>
> On Fri, 21 May 2021, Robert Marko wrote:
>
> > On Thu, May 20, 2021 at 8:49 AM Lee Jones <lee.jones@linaro.org> wrote:
> > >
> > > On Wed, 19 May 2021, Michael Walle wrote:
> > >
> > > > Hi,
> > > >
> > > > Am 2021-05-19 13:53, schrieb Robert Marko:
> > > > > On Thu, May 6, 2021 at 6:34 PM Michael Walle <michael@walle.cc> wrote:
> > > > > > Am 2021-04-30 14:35, schrieb Robert Marko:
> > > > > > > Delta TN48M switches have a Lattice CPLD that serves
> > > > > > > multiple purposes including being a GPIO expander.
> > > > > > > So lets add the MFD core driver for it.
> > > > > >
> > > > > > Did you have a look at mfd/simple-mfd-i2c.c?
> > > > >
> > > > > Yes, that was my first idea but we have a requirement to expose CPLD
> > > > > information via debugfs as there are userspace applications using it.
> > > > > And simple-mfd-i2c does not allow us to do so.
> > > >
> > > > Mh, last time Lee wasn't very fond of having a driver that just populates
> > > > sub-drivers while doing almost nothing itself. See
> > > > https://lore.kernel.org/lkml/20200605065709.GD3714@dell/
> > >
> > > Right.  I still feel that way.
> > >
> > > > That being said, I'd also like to expose our CPLD version, but until now
> > > > haven't found a good solution.
> > >
> > > Why though?  Does S/W *need* it?
> > Because we have userspace S/W that uses it as the same CPLD is in
> > multiple variants of the board but the correct board model is set during
> > manufacturing and we can read it from the CPLD.
> >
> > We also have information about PSU1 and PSU2(Some models only)
> > power good, whether they are present and some other info that I need
> > to expose as these are monitored in userspace.
> >
> > I planned to do that via the hwmon driver but according to Guenther they
> > are not hwmon attributes and I agree.
> >
> > Would it be possible to have a dedicated driver that would only expose
> > the required information via debugfs?
> > Then I could simply use the simple I2C MFD driver with only a GPIO
> > driver on top of it.
>
> Yes, I was going to suggest that.
>
> It should probably live in drivers/misc.

OK, that works for me.
I will then rework this for v2.

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 1/6] mfd: Add Delta TN48M CPLD driver

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 2520 bytes --]

Hi Robert,

I love your patch! Yet something to improve:

[auto build test ERROR on lee-mfd/for-mfd-next]
[also build test ERROR on hwmon/hwmon-next gpio/for-next v5.12 next-20210506]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Robert-Marko/mfd-Add-Delta-TN48M-CPLD-driver/20210430-203709
base:   https://git.kernel.org/pub/scm/linux/kernel/git/lee/mfd.git for-mfd-next
config: x86_64-allyesconfig (attached as .config)
compiler: gcc-9 (Debian 9.3.0-22) 9.3.0
reproduce (this is a W=1 build):
        # https://github.com/0day-ci/linux/commit/4a3a37836c56a383a726a52892d69bfdd1cf5d4c
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Robert-Marko/mfd-Add-Delta-TN48M-CPLD-driver/20210430-203709
        git checkout 4a3a37836c56a383a726a52892d69bfdd1cf5d4c
        # save the attached .config to linux build tree
        make W=1 W=1 ARCH=x86_64 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   drivers/mfd/tn48m-cpld.c: In function 'hardware_version_show':
>> drivers/mfd/tn48m-cpld.c:36:10: error: implicit declaration of function 'FIELD_GET' [-Werror=implicit-function-declaration]
      36 |  switch (FIELD_GET(HARDWARE_VERSION_MASK, regval)) {
         |          ^~~~~~~~~
   cc1: some warnings being treated as errors


vim +/FIELD_GET +36 drivers/mfd/tn48m-cpld.c

    27	
    28	static int hardware_version_show(struct seq_file *s, void *data)
    29	{
    30		struct tn48m_data *priv = s->private;
    31		unsigned int regval;
    32		char *buf;
    33	
    34		regmap_read(priv->regmap, HARDWARE_VERSION_ID, &regval);
    35	
  > 36		switch (FIELD_GET(HARDWARE_VERSION_MASK, regval)) {
    37		case HARDWARE_VERSION_EVT1:
    38			buf = "EVT1";
    39			break;
    40		case HARDWARE_VERSION_EVT2:
    41			buf = "EVT2";
    42			break;
    43		case HARDWARE_VERSION_DVT:
    44			buf = "DVT";
    45			break;
    46		case HARDWARE_VERSION_PVT:
    47			buf = "PVT";
    48			break;
    49		default:
    50			buf = "Unknown";
    51			break;
    52		}
    53	
    54		seq_printf(s, "%s\n", buf);
    55	
    56		return 0;
    57	}
    58	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 64733 bytes --]

Re: [PATCH 1/6] mfd: Add Delta TN48M CPLD driver

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 2594 bytes --]

Hi Robert,

I love your patch! Yet something to improve:

[auto build test ERROR on lee-mfd/for-mfd-next]
[also build test ERROR on hwmon/hwmon-next gpio/for-next v5.12 next-20210506]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Robert-Marko/mfd-Add-Delta-TN48M-CPLD-driver/20210430-203709
base:   https://git.kernel.org/pub/scm/linux/kernel/git/lee/mfd.git for-mfd-next
config: x86_64-allyesconfig (attached as .config)
compiler: gcc-9 (Debian 9.3.0-22) 9.3.0
reproduce (this is a W=1 build):
        # https://github.com/0day-ci/linux/commit/4a3a37836c56a383a726a52892d69bfdd1cf5d4c
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Robert-Marko/mfd-Add-Delta-TN48M-CPLD-driver/20210430-203709
        git checkout 4a3a37836c56a383a726a52892d69bfdd1cf5d4c
        # save the attached .config to linux build tree
        make W=1 W=1 ARCH=x86_64 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   drivers/mfd/tn48m-cpld.c: In function 'hardware_version_show':
>> drivers/mfd/tn48m-cpld.c:36:10: error: implicit declaration of function 'FIELD_GET' [-Werror=implicit-function-declaration]
      36 |  switch (FIELD_GET(HARDWARE_VERSION_MASK, regval)) {
         |          ^~~~~~~~~
   cc1: some warnings being treated as errors


vim +/FIELD_GET +36 drivers/mfd/tn48m-cpld.c

    27	
    28	static int hardware_version_show(struct seq_file *s, void *data)
    29	{
    30		struct tn48m_data *priv = s->private;
    31		unsigned int regval;
    32		char *buf;
    33	
    34		regmap_read(priv->regmap, HARDWARE_VERSION_ID, &regval);
    35	
  > 36		switch (FIELD_GET(HARDWARE_VERSION_MASK, regval)) {
    37		case HARDWARE_VERSION_EVT1:
    38			buf = "EVT1";
    39			break;
    40		case HARDWARE_VERSION_EVT2:
    41			buf = "EVT2";
    42			break;
    43		case HARDWARE_VERSION_DVT:
    44			buf = "DVT";
    45			break;
    46		case HARDWARE_VERSION_PVT:
    47			buf = "PVT";
    48			break;
    49		default:
    50			buf = "Unknown";
    51			break;
    52		}
    53	
    54		seq_printf(s, "%s\n", buf);
    55	
    56		return 0;
    57	}
    58	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org

[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 64733 bytes --]

Re: [PATCH 1/6] mfd: Add Delta TN48M CPLD driver

[Lee Jones]

On Fri, 30 Apr 2021, Robert Marko wrote:

> Delta TN48M switches have a Lattice CPLD that serves
> multiple purposes including being a GPIO expander.
> So lets add the MFD core driver for it.
> 
> Signed-off-by: Robert Marko <robert.marko@sartura.hr>
> ---
>  drivers/mfd/Kconfig       |  13 +++
>  drivers/mfd/Makefile      |   1 +
>  drivers/mfd/tn48m-cpld.c  | 181 ++++++++++++++++++++++++++++++++++++++
>  include/linux/mfd/tn48m.h |  30 +++++++
>  4 files changed, 225 insertions(+)
>  create mode 100644 drivers/mfd/tn48m-cpld.c
>  create mode 100644 include/linux/mfd/tn48m.h
> 
> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> index b74efa469e90..809041f98d71 100644
> --- a/drivers/mfd/Kconfig
> +++ b/drivers/mfd/Kconfig
> @@ -297,6 +297,19 @@ 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_CORE
> +	select REGMAP_I2C
> +	help
> +	  Select this option to enable support for Delta Networks TN48M switch
> +	  CPLD. It consists of GPIO and hwmon drivers.
> +	  CPLD provides GPIOS-s for the SFP slots as well as power supply
> +	  related information.
> +	  Driver provides debugfs information about the board model as
> +	  well as hardware and CPLD revision information.

No need for every sentence to be it's own paragraphs.

Please re-align to be a single chunk.

>  config PMIC_DA903X
>  	bool "Dialog Semiconductor DA9030/DA9034 PMIC Support"
>  	depends on I2C=y
> diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
> index 834f5463af28..974663341f08 100644
> --- a/drivers/mfd/Makefile
> +++ b/drivers/mfd/Makefile
> @@ -27,6 +27,7 @@ obj-$(CONFIG_MFD_TI_LP87565)	+= lp87565.o
>  obj-$(CONFIG_MFD_DAVINCI_VOICECODEC)	+= davinci_voicecodec.o
>  obj-$(CONFIG_MFD_DM355EVM_MSP)	+= dm355evm_msp.o
>  obj-$(CONFIG_MFD_TI_AM335X_TSCADC)	+= ti_am335x_tscadc.o
> +obj-$(CONFIG_MFD_TN48M_CPLD)	+= tn48m-cpld.o
>  
>  obj-$(CONFIG_MFD_STA2X11)	+= sta2x11-mfd.o
>  obj-$(CONFIG_MFD_STMPE)		+= stmpe.o
> diff --git a/drivers/mfd/tn48m-cpld.c b/drivers/mfd/tn48m-cpld.c
> new file mode 100644
> index 000000000000..b84510fb630a
> --- /dev/null
> +++ b/drivers/mfd/tn48m-cpld.c
> @@ -0,0 +1,181 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Delta TN48M CPLD parent driver
> + *
> + * Copyright 2020 Sartura Ltd

This is out of date.

> + * Author: Robert Marko <robert.marko@sartura.hr>
> + */
> +
> +#include <linux/debugfs.h>
> +#include <linux/i2c.h>
> +#include <linux/kernel.h>
> +#include <linux/mfd/core.h>
> +#include <linux/mfd/tn48m.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/regmap.h>
> +#include <linux/slab.h>
> +
> +static const struct mfd_cell tn48m_cell[] = {};

Please populate this.

Without it, this is not an MFD.

> +static const struct regmap_config tn48m_regmap_config = {
> +	.reg_bits = 8,
> +	.val_bits = 8,
> +	.max_register = 0x40,
> +};
> +
> +static int hardware_version_show(struct seq_file *s, void *data)
> +{
> +	struct tn48m_data *priv = s->private;
> +	unsigned int regval;
> +	char *buf;
> +
> +	regmap_read(priv->regmap, HARDWARE_VERSION_ID, &regval);
> +
> +	switch (FIELD_GET(HARDWARE_VERSION_MASK, regval)) {
> +	case HARDWARE_VERSION_EVT1:
> +		buf = "EVT1";
> +		break;
> +	case HARDWARE_VERSION_EVT2:
> +		buf = "EVT2";
> +		break;
> +	case HARDWARE_VERSION_DVT:
> +		buf = "DVT";
> +		break;
> +	case HARDWARE_VERSION_PVT:
> +		buf = "PVT";
> +		break;
> +	default:
> +		buf = "Unknown";
> +		break;
> +	}
> +
> +	seq_printf(s, "%s\n", buf);
> +
> +	return 0;
> +}
> +

Please drop this '\n'.

> +DEFINE_SHOW_ATTRIBUTE(hardware_version);
> +
> +static int board_id_show(struct seq_file *s, void *data)
> +{
> +	struct tn48m_data *priv = s->private;
> +	unsigned int regval;
> +	char *buf;
> +
> +	regmap_read(priv->regmap, BOARD_ID, &regval);
> +
> +	switch (regval) {
> +	case BOARD_ID_TN48M:
> +		buf = "TN48M";
> +		break;
> +	case BOARD_ID_TN48M_P:
> +		buf = "TN48-P";
> +		break;
> +	default:
> +		buf = "Unknown";
> +		break;
> +	}
> +
> +	seq_printf(s, "%s\n", buf);
> +
> +	return 0;
> +}
> +

Please drop this '\n'.

> +DEFINE_SHOW_ATTRIBUTE(board_id);
> +
> +static int code_version_show(struct seq_file *s, void *data)
> +{
> +	struct tn48m_data *priv = s->private;
> +	unsigned int regval;
> +
> +	regmap_read(priv->regmap, CPLD_CODE_VERSION, &regval);
> +
> +	seq_printf(s, "%d\n", regval);
> +
> +	return 0;
> +}
> +

Please drop this '\n'.

> +DEFINE_SHOW_ATTRIBUTE(code_version);
> +
> +static void tn48m_init_debugfs(struct tn48m_data *data)
> +{
> +	data->debugfs_dir = debugfs_create_dir(data->client->name, NULL);
> +
> +	debugfs_create_file("hardware_version",
> +			    0400,
> +			    data->debugfs_dir,
> +			    data,
> +			    &hardware_version_fops);
> +
> +	debugfs_create_file("board_id",
> +			    0400,
> +			    data->debugfs_dir,
> +			    data,
> +			    &board_id_fops);
> +
> +	debugfs_create_file("code_version",
> +			    0400,
> +			    data->debugfs_dir,
> +			    data,
> +			    &code_version_fops);
> +}

Does S/W actually do anything useful with these files?

Or are they just there for the sake of it?

If the latter, just print them to the kernel log and have done.

> +static int tn48m_probe(struct i2c_client *client)
> +{
> +	struct tn48m_data *data;

'ddata' for both please.

> +	int ret;
> +
> +	data = devm_kzalloc(&client->dev, sizeof(*data), GFP_KERNEL);
> +	if (!data)
> +		return -ENOMEM;
> +
> +	data->client = client;
> +	data->dev = &client->dev;
> +	i2c_set_clientdata(client, data);
> +
> +	data->regmap = devm_regmap_init_i2c(client, &tn48m_regmap_config);
> +	if (IS_ERR(data->regmap)) {
> +		dev_err(data->dev, "Failed to allocate regmap\n");
> +		return PTR_ERR(data->regmap);
> +	}
> +
> +	ret = devm_mfd_add_devices(data->dev, PLATFORM_DEVID_AUTO, tn48m_cell,
> +				   ARRAY_SIZE(tn48m_cell), NULL, 0, NULL);
> +	if (ret)
> +		dev_err(data->dev, "Failed to register sub-devices %d\n", ret);
> +
> +	tn48m_init_debugfs(data);
> +
> +	return ret;
> +}
> +
> +static int tn48m_remove(struct i2c_client *client)
> +{
> +	struct tn48m_data *data = i2c_get_clientdata(client);
> +
> +	debugfs_remove_recursive(data->debugfs_dir);
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id tn48m_of_match[] = {
> +	{ .compatible = "delta,tn48m-cpld"},

Missing ' ' before the '}'.

> +	{ }
> +};
> +MODULE_DEVICE_TABLE(of, tn48m_of_match);
> +
> +static struct i2c_driver tn48m_driver = {
> +	.driver = {
> +		.name = "tn48m-cpld",
> +		.of_match_table = tn48m_of_match,
> +	},
> +	.probe_new	= tn48m_probe,
> +	.remove		= tn48m_remove,
> +};
> +module_i2c_driver(tn48m_driver);
> +
> +MODULE_AUTHOR("Robert Marko <robert.marko@sartura.hr>");
> +MODULE_DESCRIPTION("Delta TN48M CPLD parent driver");
> +MODULE_LICENSE("GPL");
> diff --git a/include/linux/mfd/tn48m.h b/include/linux/mfd/tn48m.h
> new file mode 100644
> index 000000000000..551c550efa54
> --- /dev/null
> +++ b/include/linux/mfd/tn48m.h
> @@ -0,0 +1,30 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * Copyright 2020 Sartura Ltd

Out of date.

> + */
> +
> +#ifndef __TN48M_H__
> +#define __TN48M_H__

Better prefix with MFD.

> +#include <linux/device.h>
> +#include <linux/regmap.h>
> +
> +#define HARDWARE_VERSION_ID	0x0
> +#define HARDWARE_VERSION_MASK	GENMASK(3, 0)
> +#define HARDWARE_VERSION_EVT1	0
> +#define HARDWARE_VERSION_EVT2	1
> +#define HARDWARE_VERSION_DVT	2
> +#define HARDWARE_VERSION_PVT	3
> +#define BOARD_ID		0x1
> +#define BOARD_ID_TN48M		0xa
> +#define BOARD_ID_TN48M_P	0xb
> +#define CPLD_CODE_VERSION	0x2
> +
> +struct tn48m_data {
> +	struct device *dev;
> +	struct regmap *regmap;
> +	struct i2c_client *client;

You don't need both 'dev' and 'client'.

> +	struct dentry *debugfs_dir;
> +};
> +
> +#endif

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