[PATCH v1 0/3] Meta control logic device support

[PATCH v1 0/3] Meta control logic device support

[Delphine CC Chiu]

v1 - Support Meta control logic device driver
   - Add Meta vendor prefix
   - Add CLD binding document
   - Add CLD driver

Delphine CC Chiu (3):
  dt-bindings: vendor-prefixes: Add an entry for Meta
  dt-bindings: soc: meta: Add meta cld driver bindings
  misc: Add meta cld driver

 .../misc/meta,control-logic-device.txt        |  37 ++
 .../devicetree/bindings/vendor-prefixes.yaml  |   2 +
 MAINTAINERS                                   |   6 +
 drivers/misc/Kconfig                          |   9 +
 drivers/misc/Makefile                         |   1 +
 drivers/misc/control-logic-device.c           | 443 ++++++++++++++++++
 6 files changed, 498 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/misc/meta,control-logic-device.txt
 create mode 100644 drivers/misc/control-logic-device.c

-- 
2.17.1

[PATCH v1 1/3] dt-bindings: vendor-prefixes: Add an entry for Meta

[Delphine CC Chiu]

Add "meta" entry for Meta Inc.

Signed-off-by: Delphine CC Chiu <Delphine_CC_Chiu@Wiwynn.com>
---
 Documentation/devicetree/bindings/vendor-prefixes.yaml | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/Documentation/devicetree/bindings/vendor-prefixes.yaml b/Documentation/devicetree/bindings/vendor-prefixes.yaml
index 6e323a380294..8de562e61ff6 100644
--- a/Documentation/devicetree/bindings/vendor-prefixes.yaml
+++ b/Documentation/devicetree/bindings/vendor-prefixes.yaml
@@ -793,6 +793,8 @@ patternProperties:
     description: Cisco Meraki, LLC
   "^merrii,.*":
     description: Merrii Technology Co., Ltd.
+  "^meta,.*":
+    description: Meta Inc.
   "^methode,.*":
     description: Methode Electronics, Inc.
   "^micrel,.*":
-- 
2.17.1

[PATCH v1 2/3] dt-bindings: soc: meta: Add meta cld driver bindings

[Delphine CC Chiu]

Add a device tree bindings for Meta control logic device.

Signed-off-by: Delphine CC Chiu <Delphine_CC_Chiu@Wiwynn.com>
---
 .../misc/meta,control-logic-device.txt        | 37 +++++++++++++++++++
 1 file changed, 37 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/misc/meta,control-logic-device.txt

diff --git a/Documentation/devicetree/bindings/misc/meta,control-logic-device.txt b/Documentation/devicetree/bindings/misc/meta,control-logic-device.txt
new file mode 100644
index 000000000000..e966368e2fd6
--- /dev/null
+++ b/Documentation/devicetree/bindings/misc/meta,control-logic-device.txt
@@ -0,0 +1,37 @@
+* Meta Control Logic Device(CLD) Driver
+
+PROPERTIES
+
+- compatible:
+  Usage:      required
+  Type:       <string-list>
+  Definition: must include "meta,control-logic-device-<driver version>."
+              For v1, the register number, name and mode can be set in dts file.
+              Driver exports the filesystem following the dts setting.
+
+- reg:
+  Usage:      required
+  Definition: physical address
+
+- register-map:
+	Usage:      optional
+	type:       <string-list>
+	Definition: this property is an array of strings defining the names, modes
+		    and offsets of the CLD registers. The string format is "<name>:<mode>:<offset>".
+		<name>: the system file name
+		<mode>: the system file access mode. Should be r, w or rw.
+		<offset>: the register offset. If it begins with 0x the number will be
+			  parsed as a hexadecimal, if it otherwise begins with 0, it will
+			  be parsed as an octal number. Otherwise it will be parsed as a
+			  decimal.
+
+EXAMPLE
+	cld: cld@0f {
+		compatible = "meta,control-logic-device-v1";
+		reg = <0x0f>;
+		registers-map =
+			"uart-selection:rw:0x00",
+			"led-identify:rw:0x01",
+			"led-critical:rw:0x02";
+	};
+
-- 
2.17.1

[PATCH v1 3/3] misc: Add meta cld driver

[Delphine CC Chiu]

Add support for meta control-logic-device driver. The CLD manages the
server system power squence and other state such as host-power-state,
uart-selection and presense-slots. The baseboard management controller
(BMC) can access the CLD through I2C.

The version 1 of CLD driver is supported. The registers number, name
and mode of CLD can be defined in dts file for version 1. The driver
exports the filesystem following the dts setting.

Signed-off-by: Delphine CC Chiu <Delphine_CC_Chiu@Wiwynn.com>
Tested-by: Bonnie Lo <Bonnie_Lo@Wiwynn.com>
---
 MAINTAINERS                         |   6 +
 drivers/misc/Kconfig                |   9 +
 drivers/misc/Makefile               |   1 +
 drivers/misc/control-logic-device.c | 443 ++++++++++++++++++++++++++++
 4 files changed, 459 insertions(+)
 create mode 100644 drivers/misc/control-logic-device.c

diff --git a/MAINTAINERS b/MAINTAINERS
index 7483853880b6..46e250a2c334 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -13388,6 +13388,12 @@ T:	git git://linuxtv.org/media_tree.git
 F:	Documentation/devicetree/bindings/media/amlogic,gx-vdec.yaml
 F:	drivers/staging/media/meson/vdec/
 
+META CPLD DRIVER
+M:	Delphine CC Chiu <Delphine_CC_Chiu@Wiwynn.com>
+L:	linux-i2c@vger.kernel.org
+S:	Maintained
+F:	Documentation/devicetree/bindings/misc/meta,control-logic-device.txt
+
 METHODE UDPU SUPPORT
 M:	Vladimir Vid <vladimir.vid@sartura.hr>
 S:	Maintained
diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
index 358ad56f6524..21d3bf820438 100644
--- a/drivers/misc/Kconfig
+++ b/drivers/misc/Kconfig
@@ -496,6 +496,15 @@ config VCPU_STALL_DETECTOR
 
 	  If you do not intend to run this kernel as a guest, say N.
 
+config CONTROL_LOGIC_DEVICE
+        bool "Meta Control Logic Device Driver"
+        depends on I2C && REGMAP
+        help
+          Say yes here to add support for the Meta CLD device driver. The dirver
+          is used to monitor CLD register value and notify the application if
+          value is changed. Application also can access the CLD register value
+          through this driver.
+
 source "drivers/misc/c2port/Kconfig"
 source "drivers/misc/eeprom/Kconfig"
 source "drivers/misc/cb710/Kconfig"
diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile
index ac9b3e757ba1..6fcdd575a143 100644
--- a/drivers/misc/Makefile
+++ b/drivers/misc/Makefile
@@ -62,3 +62,4 @@ obj-$(CONFIG_HI6421V600_IRQ)	+= hi6421v600-irq.o
 obj-$(CONFIG_OPEN_DICE)		+= open-dice.o
 obj-$(CONFIG_GP_PCI1XXXX)	+= mchp_pci1xxxx/
 obj-$(CONFIG_VCPU_STALL_DETECTOR)	+= vcpu_stall_detector.o
+obj-$(CONFIG_CONTROL_LOGIC_DEVICE) += control-logic-device.o
diff --git a/drivers/misc/control-logic-device.c b/drivers/misc/control-logic-device.c
new file mode 100644
index 000000000000..07113dcb0836
--- /dev/null
+++ b/drivers/misc/control-logic-device.c
@@ -0,0 +1,443 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright (c) 2022 Meta Inc.
+ */
+
+ #include <linux/module.h>
+#include <linux/i2c.h>
+#include <linux/regmap.h>
+#include <linux/sysfs.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
+#include <linux/stddef.h>
+#include <linux/kthread.h>
+
+#define CLD_LABEL_LEN  64 /* label = "name:mode:offset" */
+#define to_cld_reg(x) container_of(x, struct cld_data, bin)
+
+struct cld_register_info {
+	const char *name;
+	umode_t mode;
+	u8 reg;
+	u8 value;
+};
+
+struct cld_data {
+	struct list_head list;
+	struct bin_attribute bin;
+	struct kernfs_node *kn;
+	struct cld_register_info info;
+};
+
+struct cld_client {
+	struct bin_attribute new_register_bin;
+	struct bin_attribute delet_register_bin;
+	struct regmap *regmap;
+	struct device *dev;
+	struct cld_data *data;
+	struct task_struct *task;
+	size_t num_attributes;
+};
+
+struct meta_cld_of_ops {
+	int (*load_registers)(struct cld_client *cld);
+};
+
+static struct regmap_config cld_regmap_config = {
+	.reg_bits = 8,
+	.val_bits = 8,
+	.cache_type = REGCACHE_NONE,
+};
+
+static ssize_t cld_register_read(struct file *flip, struct kobject *kobj,
+				 struct bin_attribute *attr, char *buf,
+				 loff_t pos, size_t count)
+{
+	int ret;
+	unsigned int value;
+	struct device *dev = kobj_to_dev(kobj);
+	struct cld_client *cld = dev_get_drvdata(dev);
+	struct cld_data *data = to_cld_reg(attr);
+
+	ret = regmap_read(cld->regmap, data->info.reg, &value);
+	if (ret != 0)
+		return ret;
+
+	if ((data->info.mode & 0400) == 0400)
+		data->info.value = value;
+
+	snprintf(buf, sizeof(value), "%d\n", value);
+
+	return strlen(buf);
+}
+
+static ssize_t cld_register_write(struct file *flip, struct kobject *kobj,
+				  struct bin_attribute *attr, char *buf,
+				  loff_t pos, size_t count)
+{
+	int ret;
+	unsigned long val;
+	struct device *dev = kobj_to_dev(kobj);
+	struct cld_client *cld = dev_get_drvdata(dev);
+	struct cld_data *data = to_cld_reg(attr);
+
+	ret = kstrtoul(buf, 0, &val);
+	if (ret)
+		goto out;
+
+	if (val >= BIT(8)) {
+		ret = -EINVAL;
+		goto out;
+	}
+
+	ret = regmap_write(cld->regmap, data->info.reg, val);
+	if (ret)
+		goto out;
+
+out:
+	return ret ? ret : strlen(buf);
+}
+
+static int cld_bin_register(struct cld_register_info info,
+			    struct cld_client *cld)
+{
+	int ret;
+	struct cld_data *data, *pos;
+	struct bin_attribute *bin;
+	size_t length;
+
+	list_for_each_entry(pos, &cld->data->list, list) {
+		length = (strlen(info.name) > strlen(pos->info.name)) ?
+			 strlen(info.name):strlen(pos->info.name);
+		if (!strncasecmp(info.name, pos->info.name, length))
+			return -EEXIST;
+	}
+
+	data = devm_kzalloc(cld->dev, sizeof(*data), GFP_KERNEL);
+	if (!data)
+		return -ENOMEM;
+
+	bin = &data->bin;
+	data->info = info;
+
+	sysfs_bin_attr_init(bin);
+	bin->attr.name = kstrdup(info.name, GFP_KERNEL);
+	if (!bin->attr.name)
+		return -EINVAL;
+	bin->attr.mode = info.mode;
+	bin->read = cld_register_read;
+	bin->write = cld_register_write;
+	bin->size = 1;
+	ret = sysfs_create_bin_file(&cld->dev->kobj, bin);
+	if (ret) {
+		dev_err(cld->dev, "%s: failed to create file: %d\n",
+			info.name, ret);
+		goto out;
+	}
+
+	data->kn = kernfs_find_and_get(cld->dev->kobj.sd, bin->attr.name);
+	if (!data->kn) {
+		sysfs_remove_bin_file(&cld->dev->kobj, bin);
+		ret = -EFAULT;
+		goto out;
+	}
+
+	list_add_tail(&data->list, &cld->data->list);
+	++cld->num_attributes;
+out:
+	return ret;
+}
+
+static int cld_bin_unregister(struct cld_register_info info,
+			      struct cld_client *cld)
+{
+	size_t length;
+	struct cld_data *pos;
+
+	list_for_each_entry(pos, &cld->data->list, list) {
+		length = (strlen(info.name) > strlen(pos->info.name)) ?
+			 strlen(info.name):strlen(pos->info.name);
+		if ((!strncasecmp(info.name, pos->info.name, length)) &&
+		    (info.mode == pos->info.mode) &&
+		    (info.reg == pos->info.reg)) {
+			kernfs_put(pos->kn);
+			sysfs_remove_bin_file(&cld->dev->kobj,
+					      &pos->bin);
+			list_del(&pos->list);
+			devm_kfree(cld->dev, pos);
+			--cld->num_attributes;
+			return 0;
+		}
+	}
+
+	dev_err(cld->dev, "%s: cannot match cld data\n", info.name);
+	return -EINVAL;
+}
+
+static int cld_parse_label(char *label, struct cld_register_info *info)
+{
+	char name[CLD_LABEL_LEN] = { 0 };
+	char *mode_str, *reg_str;
+	int rc, i;
+	unsigned long reg;
+	umode_t mode;
+	size_t length;
+	struct {
+		char *mode;
+		int value;
+	} options[] = {
+		{"rw", 0600},
+		{"r", 0400},
+		{"w", 0200},
+	};
+
+	strncpy(name, label, CLD_LABEL_LEN);
+
+	mode_str = strchr(name, ':');
+	if (!mode_str)
+		return -EINVAL;
+	*mode_str++ = '\0';
+
+	reg_str = strchr(mode_str, ':');
+	if (!reg_str)
+		return -EINVAL;
+	*reg_str++ = '\0';
+
+	if ((*name == '\0') || (*mode_str == '\0') || (*reg_str == '\0'))
+		return -EINVAL;
+
+	for (i = 0; i < ARRAY_SIZE(options); i++) {
+		length = (strlen(options[i].mode) > strlen(mode_str)) ?
+			 strlen(options[i].mode):strlen(mode_str);
+		if (strncasecmp(mode_str, options[i].mode, length) == 0) {
+			rc = kstrtoul(reg_str, 0, &reg);
+			if (rc)
+				return -EINVAL;
+			mode = options[i].value;
+			break;
+		}
+	}
+	if ((i >= ARRAY_SIZE(options)) || (reg >= BIT(8)))
+		return -EINVAL;
+
+	info->name = kstrdup(name, GFP_KERNEL);
+	if (!info->name)
+		return -EINVAL;
+	info->reg = reg;
+	info->mode = mode;
+	return 0;
+}
+
+static int cld_load_registers(struct cld_client *cld, const char *property)
+{
+	const char *label;
+	int count, ret, i;
+	struct cld_register_info info;
+	struct device *dev = cld->dev;
+
+	count = of_property_count_strings(dev->of_node, property);
+	if (count <= 0)
+		return -EINVAL;
+
+	for (i = 0; i < count; i++) {
+		ret = of_property_read_string_index(dev->of_node, property, i,
+						    &label);
+		if (ret) {
+			dev_err(dev, ": failed to get label for index %d\n",
+				i);
+			continue;
+		}
+
+		ret = cld_parse_label((char *)label, &info);
+		if (ret) {
+			dev_err(cld->dev, "%s: failed to parse label\n",
+				label);
+			continue;
+		}
+		cld_bin_register(info, cld);
+	}
+	return (count == cld->num_attributes) ? 0 : (-EINVAL);
+}
+
+static int meta_cld_of_v1_load_registers(struct cld_client *cld)
+{
+	int ret;
+
+	ret = cld_load_registers(cld, "registers-map");
+	if (ret)
+		dev_dbg(cld->dev, "failed to load registers\n");
+
+	return 0;
+}
+
+static ssize_t cld_sysfs_new_register(struct file *filp, struct kobject *kobj,
+				      struct bin_attribute *attr,
+				      char *buf, loff_t pos, size_t count)
+{
+	int ret;
+	struct device *dev = kobj_to_dev(kobj);
+	struct cld_client *cld = dev_get_drvdata(dev);
+	struct cld_register_info info;
+
+	ret = cld_parse_label(buf, &info);
+	if (ret) {
+		dev_err(cld->dev, "failed to parse label\n");
+		goto out;
+	}
+	ret = cld_bin_register(info, cld);
+
+out:
+	return (!ret) ? count : ret;
+}
+
+static ssize_t cld_sysfs_delete_register(struct file *filp,
+					 struct kobject *kobj,
+					 struct bin_attribute *attr,
+					 char *buf, loff_t pos, size_t count)
+{
+	int ret;
+	struct device *dev = kobj_to_dev(kobj);
+	struct cld_client *cld = dev_get_drvdata(dev);
+	struct cld_register_info info;
+
+	ret = cld_parse_label(buf, &info);
+	if (ret) {
+		dev_err(cld->dev, "failed to parse label\n");
+		goto out;
+	}
+
+	ret = cld_bin_unregister(info, cld);
+
+out:
+	return (!ret) ? count : ret;
+}
+
+static int cld_monitor_thread(void *client)
+{
+	struct cld_client *cld = client;
+	unsigned int value;
+	int ret;
+	struct cld_data *pos;
+
+	while (!kthread_should_stop()) {
+		list_for_each_entry(pos, &cld->data->list, list) {
+			if ((pos->info.mode & 0400) == 0400) {
+				ret = regmap_read(cld->regmap, pos->info.reg,
+						  &value);
+				if (ret)
+					continue;
+				if (pos->info.value != value) {
+					pos->info.value = value;
+					kernfs_notify(pos->kn);
+				}
+			}
+		}
+		msleep(50);
+	}
+	return 0;
+}
+
+static int cld_probe(struct i2c_client *client,
+		     const struct i2c_device_id *id)
+{
+	int ret = 0;
+	const struct meta_cld_of_ops *ops;
+	struct device *dev = &client->dev;
+	struct cld_client *cld;
+
+	cld = devm_kzalloc(dev, sizeof(*cld), GFP_KERNEL);
+	if (!cld) {
+		ret = -ENOMEM;
+		goto out;
+	}
+
+	cld->dev = dev;
+	cld->num_attributes = 0;
+	cld->regmap = devm_regmap_init_i2c(client, &cld_regmap_config);
+	cld->data = devm_kzalloc(cld->dev, sizeof(struct cld_data),
+				 GFP_KERNEL);
+	if (!cld->data) {
+		ret = -ENOMEM;
+		goto out;
+	}
+
+	INIT_LIST_HEAD(&cld->data->list);
+
+	sysfs_bin_attr_init(&cld->new_register_bin);
+	cld->new_register_bin.attr.name = "new_register";
+	cld->new_register_bin.attr.mode = 0200;
+	cld->new_register_bin.write = cld_sysfs_new_register;
+	ret = sysfs_create_bin_file(&dev->kobj, &cld->new_register_bin);
+	if (ret)
+		goto out;
+
+	sysfs_bin_attr_init(&cld->delet_register_bin);
+	cld->delet_register_bin.attr.name = "delete_register";
+	cld->delet_register_bin.attr.mode = 0200;
+	cld->delet_register_bin.write = cld_sysfs_delete_register;
+	ret = sysfs_create_bin_file(&dev->kobj, &cld->delet_register_bin);
+	if (ret)
+		goto out;
+
+	ops = of_device_get_match_data(cld->dev);
+	if (!ops) {
+		ret = -EINVAL;
+		goto out;
+	}
+
+	ret = ops->load_registers(cld);
+
+	cld->task = kthread_run(cld_monitor_thread, (void *)cld,
+				"register_monitor");
+	if (IS_ERR(cld->task)) {
+		ret = PTR_ERR(cld->task);
+		cld->task = NULL;
+		goto out;
+	}
+
+	dev_set_drvdata(dev, cld);
+
+out:
+	return ret;
+}
+
+static int cld_remove(struct i2c_client *client)
+{
+	struct device *dev = &client->dev;
+	struct cld_client *cld = dev_get_drvdata(dev);
+
+	if (cld->task) {
+		kthread_stop(cld->task);
+		cld->task = NULL;
+	}
+
+	devm_kfree(dev, cld);
+	return 0;
+}
+
+static const struct meta_cld_of_ops of_v1_ops = {
+	.load_registers = meta_cld_of_v1_load_registers,
+};
+
+static const struct of_device_id cld_of_match[] = {
+	{
+		.compatible = "meta,control-logic-device-v1",
+		.data = &of_v1_ops,
+	},
+	{}
+};
+
+MODULE_DEVICE_TABLE(of, cld_of_match)
+
+static struct i2c_driver cld = {
+	.driver = { .name = "cld-driver",
+		    .of_match_table = of_match_ptr(cld_of_match) },
+	.probe = cld_probe,
+	.remove = cld_remove,
+};
+
+module_i2c_driver(cld);
+
+MODULE_AUTHOR("Ren Chen <ren_chen@wiwynn.com>");
+MODULE_DESCRIPTION("Meta control logic device driver");
+MODULE_LICENSE("GPL");
-- 
2.17.1

Re: [PATCH v1 3/3] misc: Add meta cld driver

[Greg Kroah-Hartman]

On Tue, Jan 17, 2023 at 05:44:22PM +0800, Delphine CC Chiu wrote:
> Add support for meta control-logic-device driver. The CLD manages the
> server system power squence and other state such as host-power-state,
> uart-selection and presense-slots. The baseboard management controller
> (BMC) can access the CLD through I2C.
> 
> The version 1 of CLD driver is supported. The registers number, name
> and mode of CLD can be defined in dts file for version 1. The driver
> exports the filesystem following the dts setting.
> 
> Signed-off-by: Delphine CC Chiu <Delphine_CC_Chiu@Wiwynn.com>
> Tested-by: Bonnie Lo <Bonnie_Lo@Wiwynn.com>
> ---
>  MAINTAINERS                         |   6 +
>  drivers/misc/Kconfig                |   9 +
>  drivers/misc/Makefile               |   1 +
>  drivers/misc/control-logic-device.c | 443 ++++++++++++++++++++++++++++

That is a very generic name for a very specific driver.  Please make it
more hardware-specific.

Also, you add a bunch of undocumented sysfs files here, which require a
Documentation/ABI/ entries so that we can review the code to verify it
does what you all think it does.

And finally, why is this needed to be a kernel driver at all?  Why can't
you control this all through the userspace i2c api?

One review comment:

> +static int cld_remove(struct i2c_client *client)
> +{
> +	struct device *dev = &client->dev;
> +	struct cld_client *cld = dev_get_drvdata(dev);
> +
> +	if (cld->task) {
> +		kthread_stop(cld->task);
> +		cld->task = NULL;
> +	}
> +
> +	devm_kfree(dev, cld);

Whenever you see this line in code, it's almost always a huge red flag
that someone is not using the devm_* api properly.  I think that is most
certainly the case here.

thanks,

greg k-h

Re: [PATCH v1 1/3] dt-bindings: vendor-prefixes: Add an entry for Meta

[Krzysztof Kozlowski]

On 17/01/2023 10:44, Delphine CC Chiu wrote:
> Add "meta" entry for Meta Inc.

You need to be a bit more specific. There is Meta Platforms Inc. but not
Meta Inc., so you might be talking about different companies. Describe
shortly the company (e.g. website, NASDAQ ticker)

Best regards,
Krzysztof

Re: [PATCH v1 2/3] dt-bindings: soc: meta: Add meta cld driver bindings

[Krzysztof Kozlowski]

On 17/01/2023 10:44, Delphine CC Chiu wrote:
> Add a device tree bindings for Meta control logic device.

Subject: drop second/last, redundant "bindings". The "dt-bindings"
prefix is already stating that these are bindings.

Subject: Drop "driver"

> 
> Signed-off-by: Delphine CC Chiu <Delphine_CC_Chiu@Wiwynn.com>
> ---
>  .../misc/meta,control-logic-device.txt        | 37 +++++++++++++++++++
>  1 file changed, 37 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/misc/meta,control-logic-device.txt
> 
> diff --git a/Documentation/devicetree/bindings/misc/meta,control-logic-device.txt b/Documentation/devicetree/bindings/misc/meta,control-logic-device.txt
> new file mode 100644
> index 000000000000..e966368e2fd6
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/misc/meta,control-logic-device.txt

New bindings only in DT schema format. Start
Documentation/devicetree/bindings/writing-schema.rst,
example-schema.yaml and other docs.



Best regards,
Krzysztof

Re: [PATCH v1 3/3] misc: Add meta cld driver

[Krzysztof Kozlowski]

On 17/01/2023 10:44, Delphine CC Chiu wrote:
> Add support for meta control-logic-device driver. The CLD manages the
> server system power squence and other state such as host-power-state,
> uart-selection and presense-slots. The baseboard management controller
> (BMC) can access the CLD through I2C.
> 
> The version 1 of CLD driver is supported. The registers number, name
> and mode of CLD can be defined in dts file for version 1. The driver
> exports the filesystem following the dts setting.
> 
> Signed-off-by: Delphine CC Chiu <Delphine_CC_Chiu@Wiwynn.com>
> Tested-by: Bonnie Lo <Bonnie_Lo@Wiwynn.com>
> ---
>  MAINTAINERS                         |   6 +
>  drivers/misc/Kconfig                |   9 +
>  drivers/misc/Makefile               |   1 +
>  drivers/misc/control-logic-device.c | 443 ++++++++++++++++++++++++++++
>  4 files changed, 459 insertions(+)
>  create mode 100644 drivers/misc/control-logic-device.c
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 7483853880b6..46e250a2c334 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -13388,6 +13388,12 @@ T:	git git://linuxtv.org/media_tree.git
>  F:	Documentation/devicetree/bindings/media/amlogic,gx-vdec.yaml
>  F:	drivers/staging/media/meson/vdec/
>  
> +META CPLD DRIVER
> +M:	Delphine CC Chiu <Delphine_CC_Chiu@Wiwynn.com>
> +L:	linux-i2c@vger.kernel.org
> +S:	Maintained
> +F:	Documentation/devicetree/bindings/misc/meta,control-logic-device.txt

Missing entries for driver code.

> +
>  METHODE UDPU SUPPORT
>  M:	Vladimir Vid <vladimir.vid@sartura.hr>
>  S:	Maintained
> diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
> index 358ad56f6524..21d3bf820438 100644
> --- a/drivers/misc/Kconfig
> +++ b/drivers/misc/Kconfig
> @@ -496,6 +496,15 @@ config VCPU_STALL_DETECTOR
>  
>  	  If you do not intend to run this kernel as a guest, say N.
>  
> +config CONTROL_LOGIC_DEVICE
> +        bool "Meta Control Logic Device Driver"

Too generic...

> +        depends on I2C && REGMAP
> +        help
> +          Say yes here to add support for the Meta CLD device driver. The dirver
> +          is used to monitor CLD register value and notify the application if
> +          value is changed. Application also can access the CLD register value
> +          through this driver.
> +
>  source "drivers/misc/c2port/Kconfig"
>  source "drivers/misc/eeprom/Kconfig"
>  source "drivers/misc/cb710/Kconfig"
> diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile
> index ac9b3e757ba1..6fcdd575a143 100644
> --- a/drivers/misc/Makefile
> +++ b/drivers/misc/Makefile
> @@ -62,3 +62,4 @@ obj-$(CONFIG_HI6421V600_IRQ)	+= hi6421v600-irq.o
>  obj-$(CONFIG_OPEN_DICE)		+= open-dice.o
>  obj-$(CONFIG_GP_PCI1XXXX)	+= mchp_pci1xxxx/
>  obj-$(CONFIG_VCPU_STALL_DETECTOR)	+= vcpu_stall_detector.o
> +obj-$(CONFIG_CONTROL_LOGIC_DEVICE) += control-logic-device.o

Does not look like ordered by name.

> diff --git a/drivers/misc/control-logic-device.c b/drivers/misc/control-logic-device.c
> new file mode 100644
> index 000000000000..07113dcb0836
> --- /dev/null
> +++ b/drivers/misc/control-logic-device.c
> @@ -0,0 +1,443 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (c) 2022 Meta Inc.
> + */
> +
> + #include <linux/module.h>

Don't prepend with spaces. Check the code for such trivial style issues
first.

> +#include <linux/i2c.h>
> +#include <linux/regmap.h>
> +#include <linux/sysfs.h>
> +#include <linux/of.h>
> +#include <linux/of_device.h>
> +#include <linux/stddef.h>
> +#include <linux/kthread.h>
> +
> +#define CLD_LABEL_LEN  64 /* label = "name:mode:offset" */
> +#define to_cld_reg(x) container_of(x, struct cld_data, bin)
> +
> +struct cld_register_info {
> +	const char *name;
> +	umode_t mode;
> +	u8 reg;
> +	u8 value;
> +};
> +
> +struct cld_data {
> +	struct list_head list;
> +	struct bin_attribute bin;
> +	struct kernfs_node *kn;
> +	struct cld_register_info info;
> +};
> +
> +struct cld_client {
> +	struct bin_attribute new_register_bin;
> +	struct bin_attribute delet_register_bin;
> +	struct regmap *regmap;
> +	struct device *dev;
> +	struct cld_data *data;
> +	struct task_struct *task;
> +	size_t num_attributes;
> +};
> +
> +struct meta_cld_of_ops {
> +	int (*load_registers)(struct cld_client *cld);
> +};
> +
> +static struct regmap_config cld_regmap_config = {
> +	.reg_bits = 8,
> +	.val_bits = 8,
> +	.cache_type = REGCACHE_NONE,
> +};
> +
> +static ssize_t cld_register_read(struct file *flip, struct kobject *kobj,
> +				 struct bin_attribute *attr, char *buf,
> +				 loff_t pos, size_t count)
> +{
> +	int ret;
> +	unsigned int value;
> +	struct device *dev = kobj_to_dev(kobj);
> +	struct cld_client *cld = dev_get_drvdata(dev);
> +	struct cld_data *data = to_cld_reg(attr);
> +
> +	ret = regmap_read(cld->regmap, data->info.reg, &value);
> +	if (ret != 0)
> +		return ret;
> +
> +	if ((data->info.mode & 0400) == 0400)
> +		data->info.value = value;
> +
> +	snprintf(buf, sizeof(value), "%d\n", value);
> +
> +	return strlen(buf);
> +}
> +
> +static ssize_t cld_register_write(struct file *flip, struct kobject *kobj,
> +				  struct bin_attribute *attr, char *buf,
> +				  loff_t pos, size_t count)
> +{
> +	int ret;
> +	unsigned long val;
> +	struct device *dev = kobj_to_dev(kobj);
> +	struct cld_client *cld = dev_get_drvdata(dev);
> +	struct cld_data *data = to_cld_reg(attr);
> +
> +	ret = kstrtoul(buf, 0, &val);
> +	if (ret)
> +		goto out;
> +
> +	if (val >= BIT(8)) {
> +		ret = -EINVAL;
> +		goto out;
> +	}
> +
> +	ret = regmap_write(cld->regmap, data->info.reg, val);
> +	if (ret)
> +		goto out;
> +
> +out:
> +	return ret ? ret : strlen(buf);
> +}
> +
> +static int cld_bin_register(struct cld_register_info info,
> +			    struct cld_client *cld)
> +{
> +	int ret;
> +	struct cld_data *data, *pos;
> +	struct bin_attribute *bin;
> +	size_t length;
> +
> +	list_for_each_entry(pos, &cld->data->list, list) {
> +		length = (strlen(info.name) > strlen(pos->info.name)) ?
> +			 strlen(info.name):strlen(pos->info.name);
> +		if (!strncasecmp(info.name, pos->info.name, length))
> +			return -EEXIST;
> +	}
> +
> +	data = devm_kzalloc(cld->dev, sizeof(*data), GFP_KERNEL);
> +	if (!data)
> +		return -ENOMEM;
> +
> +	bin = &data->bin;
> +	data->info = info;
> +
> +	sysfs_bin_attr_init(bin);
> +	bin->attr.name = kstrdup(info.name, GFP_KERNEL);
> +	if (!bin->attr.name)
> +		return -EINVAL;
> +	bin->attr.mode = info.mode;
> +	bin->read = cld_register_read;
> +	bin->write = cld_register_write;
> +	bin->size = 1;
> +	ret = sysfs_create_bin_file(&cld->dev->kobj, bin);
> +	if (ret) {
> +		dev_err(cld->dev, "%s: failed to create file: %d\n",
> +			info.name, ret);
> +		goto out;
> +	}
> +
> +	data->kn = kernfs_find_and_get(cld->dev->kobj.sd, bin->attr.name);
> +	if (!data->kn) {
> +		sysfs_remove_bin_file(&cld->dev->kobj, bin);
> +		ret = -EFAULT;
> +		goto out;
> +	}
> +
> +	list_add_tail(&data->list, &cld->data->list);
> +	++cld->num_attributes;
> +out:
> +	return ret;
> +}
> +
> +static int cld_bin_unregister(struct cld_register_info info,
> +			      struct cld_client *cld)
> +{
> +	size_t length;
> +	struct cld_data *pos;
> +
> +	list_for_each_entry(pos, &cld->data->list, list) {
> +		length = (strlen(info.name) > strlen(pos->info.name)) ?
> +			 strlen(info.name):strlen(pos->info.name);
> +		if ((!strncasecmp(info.name, pos->info.name, length)) &&
> +		    (info.mode == pos->info.mode) &&
> +		    (info.reg == pos->info.reg)) {
> +			kernfs_put(pos->kn);
> +			sysfs_remove_bin_file(&cld->dev->kobj,
> +					      &pos->bin);
> +			list_del(&pos->list);
> +			devm_kfree(cld->dev, pos);
> +			--cld->num_attributes;
> +			return 0;
> +		}
> +	}
> +
> +	dev_err(cld->dev, "%s: cannot match cld data\n", info.name);
> +	return -EINVAL;
> +}
> +
> +static int cld_parse_label(char *label, struct cld_register_info *info)
> +{
> +	char name[CLD_LABEL_LEN] = { 0 };
> +	char *mode_str, *reg_str;
> +	int rc, i;
> +	unsigned long reg;
> +	umode_t mode;
> +	size_t length;
> +	struct {
> +		char *mode;
> +		int value;
> +	} options[] = {
> +		{"rw", 0600},
> +		{"r", 0400},
> +		{"w", 0200},
> +	};
> +
> +	strncpy(name, label, CLD_LABEL_LEN);
> +
> +	mode_str = strchr(name, ':');
> +	if (!mode_str)
> +		return -EINVAL;
> +	*mode_str++ = '\0';
> +
> +	reg_str = strchr(mode_str, ':');
> +	if (!reg_str)
> +		return -EINVAL;
> +	*reg_str++ = '\0';
> +
> +	if ((*name == '\0') || (*mode_str == '\0') || (*reg_str == '\0'))
> +		return -EINVAL;
> +
> +	for (i = 0; i < ARRAY_SIZE(options); i++) {
> +		length = (strlen(options[i].mode) > strlen(mode_str)) ?
> +			 strlen(options[i].mode):strlen(mode_str);
> +		if (strncasecmp(mode_str, options[i].mode, length) == 0) {
> +			rc = kstrtoul(reg_str, 0, &reg);
> +			if (rc)
> +				return -EINVAL;
> +			mode = options[i].value;
> +			break;
> +		}
> +	}
> +	if ((i >= ARRAY_SIZE(options)) || (reg >= BIT(8)))
> +		return -EINVAL;
> +
> +	info->name = kstrdup(name, GFP_KERNEL);
> +	if (!info->name)
> +		return -EINVAL;
> +	info->reg = reg;
> +	info->mode = mode;
> +	return 0;
> +}
> +
> +static int cld_load_registers(struct cld_client *cld, const char *property)
> +{
> +	const char *label;
> +	int count, ret, i;
> +	struct cld_register_info info;
> +	struct device *dev = cld->dev;
> +
> +	count = of_property_count_strings(dev->of_node, property);
> +	if (count <= 0)
> +		return -EINVAL;
> +
> +	for (i = 0; i < count; i++) {
> +		ret = of_property_read_string_index(dev->of_node, property, i,
> +						    &label);
> +		if (ret) {
> +			dev_err(dev, ": failed to get label for index %d\n",
> +				i);
> +			continue;
> +		}
> +
> +		ret = cld_parse_label((char *)label, &info);
> +		if (ret) {
> +			dev_err(cld->dev, "%s: failed to parse label\n",
> +				label);
> +			continue;
> +		}
> +		cld_bin_register(info, cld);
> +	}
> +	return (count == cld->num_attributes) ? 0 : (-EINVAL);
> +}
> +
> +static int meta_cld_of_v1_load_registers(struct cld_client *cld)
> +{
> +	int ret;
> +
> +	ret = cld_load_registers(cld, "registers-map");
> +	if (ret)
> +		dev_dbg(cld->dev, "failed to load registers\n");
> +
> +	return 0;
> +}
> +
> +static ssize_t cld_sysfs_new_register(struct file *filp, struct kobject *kobj,
> +				      struct bin_attribute *attr,
> +				      char *buf, loff_t pos, size_t count)
> +{
> +	int ret;
> +	struct device *dev = kobj_to_dev(kobj);
> +	struct cld_client *cld = dev_get_drvdata(dev);
> +	struct cld_register_info info;
> +
> +	ret = cld_parse_label(buf, &info);
> +	if (ret) {
> +		dev_err(cld->dev, "failed to parse label\n");
> +		goto out;
> +	}
> +	ret = cld_bin_register(info, cld);
> +
> +out:
> +	return (!ret) ? count : ret;
> +}
> +
> +static ssize_t cld_sysfs_delete_register(struct file *filp,
> +					 struct kobject *kobj,
> +					 struct bin_attribute *attr,
> +					 char *buf, loff_t pos, size_t count)
> +{
> +	int ret;
> +	struct device *dev = kobj_to_dev(kobj);
> +	struct cld_client *cld = dev_get_drvdata(dev);
> +	struct cld_register_info info;
> +
> +	ret = cld_parse_label(buf, &info);
> +	if (ret) {
> +		dev_err(cld->dev, "failed to parse label\n");
> +		goto out;
> +	}
> +
> +	ret = cld_bin_unregister(info, cld);
> +
> +out:
> +	return (!ret) ? count : ret;
> +}
> +
> +static int cld_monitor_thread(void *client)
> +{
> +	struct cld_client *cld = client;
> +	unsigned int value;
> +	int ret;
> +	struct cld_data *pos;
> +
> +	while (!kthread_should_stop()) {
> +		list_for_each_entry(pos, &cld->data->list, list) {
> +			if ((pos->info.mode & 0400) == 0400) {
> +				ret = regmap_read(cld->regmap, pos->info.reg,
> +						  &value);
> +				if (ret)
> +					continue;
> +				if (pos->info.value != value) {
> +					pos->info.value = value;
> +					kernfs_notify(pos->kn);
> +				}
> +			}
> +		}
> +		msleep(50);
> +	}
> +	return 0;
> +}
> +
> +static int cld_probe(struct i2c_client *client,
> +		     const struct i2c_device_id *id)
> +{
> +	int ret = 0;
> +	const struct meta_cld_of_ops *ops;
> +	struct device *dev = &client->dev;
> +	struct cld_client *cld;
> +
> +	cld = devm_kzalloc(dev, sizeof(*cld), GFP_KERNEL);
> +	if (!cld) {
> +		ret = -ENOMEM;

return

> +		goto out;
> +	}
> +
> +	cld->dev = dev;
> +	cld->num_attributes = 0;
> +	cld->regmap = devm_regmap_init_i2c(client, &cld_regmap_config);
> +	cld->data = devm_kzalloc(cld->dev, sizeof(struct cld_data),

sizeof(*)

> +				 GFP_KERNEL);
> +	if (!cld->data) {
> +		ret = -ENOMEM;

return


> +		goto out;
> +	}
> +
> +	INIT_LIST_HEAD(&cld->data->list);
> +
> +	sysfs_bin_attr_init(&cld->new_register_bin);
> +	cld->new_register_bin.attr.name = "new_register";
> +	cld->new_register_bin.attr.mode = 0200;
> +	cld->new_register_bin.write = cld_sysfs_new_register;
> +	ret = sysfs_create_bin_file(&dev->kobj, &cld->new_register_bin);
> +	if (ret)
> +		goto out;
> +
> +	sysfs_bin_attr_init(&cld->delet_register_bin);
> +	cld->delet_register_bin.attr.name = "delete_register";
> +	cld->delet_register_bin.attr.mode = 0200;
> +	cld->delet_register_bin.write = cld_sysfs_delete_register;
> +	ret = sysfs_create_bin_file(&dev->kobj, &cld->delet_register_bin);
> +	if (ret)
> +		goto out;

return

> +
> +	ops = of_device_get_match_data(cld->dev);
> +	if (!ops) {
> +		ret = -EINVAL;

return

> +		goto out;
> +	}
> +
> +	ret = ops->load_registers(cld);
> +
> +	cld->task = kthread_run(cld_monitor_thread, (void *)cld,
> +				"register_monitor");

Why do you need the thread? What do you monitor? All this should be
explained and documented.

> +	if (IS_ERR(cld->task)) {
> +		ret = PTR_ERR(cld->task);

return

> +		cld->task = NULL;

Drop

> +		goto out;
> +	}
> +
> +	dev_set_drvdata(dev, cld);

This should happen before starting thread.

> +
> +out:
> +	return ret;
> +}
> +
> +static int cld_remove(struct i2c_client *client)
> +{
> +	struct device *dev = &client->dev;
> +	struct cld_client *cld = dev_get_drvdata(dev);
> +
> +	if (cld->task) {
> +		kthread_stop(cld->task);
> +		cld->task = NULL;
> +	}
> +
> +	devm_kfree(dev, cld);
> +	return 0;
> +}
> +
> +static const struct meta_cld_of_ops of_v1_ops = {
> +	.load_registers = meta_cld_of_v1_load_registers,
> +};
> +
> +static const struct of_device_id cld_of_match[] = {

This would warn now. Drop of_match_ptr.

> +	{
> +		.compatible = "meta,control-logic-device-v1",
> +		.data = &of_v1_ops,
> +	},
> +	{}
> +};
> +
> +MODULE_DEVICE_TABLE(of, cld_of_match)
> +
> +static struct i2c_driver cld = {
> +	.driver = { .name = "cld-driver",
> +		    .of_match_table = of_match_ptr(cld_of_match) },

Blank line before }

> +	.probe = cld_probe,
> +	.remove = cld_remove,
> +};
> +
> +module_i2c_driver(cld);
> +
> +MODULE_AUTHOR("Ren Chen <ren_chen@wiwynn.com>");
> +MODULE_DESCRIPTION("Meta control logic device driver");
> +MODULE_LICENSE("GPL");

Best regards,
Krzysztof

Re: [PATCH v1 3/3] misc: Add meta cld driver

[Linus Walleij]

Hi Delphine,

thanks for your patch!

On Tue, Jan 17, 2023 at 10:46 AM Delphine CC Chiu
<Delphine_CC_Chiu@wiwynn.com> wrote:

> Add support for meta control-logic-device driver. The CLD manages the
> server system power squence and other state such as host-power-state,
> uart-selection and presense-slots. The baseboard management controller
> (BMC) can access the CLD through I2C.
>
> The version 1 of CLD driver is supported. The registers number, name
> and mode of CLD can be defined in dts file for version 1. The driver
> exports the filesystem following the dts setting.
>
> Signed-off-by: Delphine CC Chiu <Delphine_CC_Chiu@Wiwynn.com>
> Tested-by: Bonnie Lo <Bonnie_Lo@Wiwynn.com>

Why should this driver be in drivers/misc and not drivers/mfd?
MFS has support code for spawning child devices for the LED
you are also creating for example, so please use that.

> +#include <linux/sysfs.h>
(...)
> +#include <linux/kthread.h>
(...)

> +static ssize_t cld_register_read(struct file *flip, struct kobject *kobj,
> +                                struct bin_attribute *attr, char *buf,
> +                                loff_t pos, size_t count)
> +{
(...)
> +       snprintf(buf, sizeof(value), "%d\n", value);
(...)
> +static ssize_t cld_register_write(struct file *flip, struct kobject *kobj,
> +                                 struct bin_attribute *attr, char *buf,
> +                                 loff_t pos, size_t count)
> +{
> +       ret = kstrtoul(buf, 0, &val);
(...)

Writing and reading some random regmap registers is something
that the regmap debugfs already can do.

> +static int cld_bin_register(struct cld_register_info info,
> +                           struct cld_client *cld)
> +{

And this is for reading and writing binary blobs.

It looks like something that should be using the firmware
API.

If the purpose of the driver is to open a hole from userspace
down to the hardware, as Greg says why not just use
userspace I2C then?

It seems a bit dangerous to relay whatever the ASIC is doing
to userspace though.

Are you sure you can't use any of the existing kernel functionality
for doing what these userspace "hole" is doing?

There is drivers/power etc for power control and I bet it can
be extended if need be.

Yours,
Linus Walleij

RE: [PATCH v1 3/3] misc: Add meta cld driver

[Delphine_CC_Chiu/WYHQ/Wiwynn]

Hi Greg,

Thanks for your comment!

> -----Original Message-----
> From: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Sent: Tuesday, January 17, 2023 6:15 PM
> To: Delphine_CC_Chiu/WYHQ/Wiwynn <Delphine_CC_Chiu@wiwynn.com>
> Cc: patrick@stwcx.xyz; Derek Kiernan <derek.kiernan@xilinx.com>; Dragan
> Cvetic <dragan.cvetic@xilinx.com>; Arnd Bergmann <arnd@arndb.de>;
> garnermic@fb.com; Rob Herring <robh+dt@kernel.org>; Krzysztof Kozlowski
> <krzysztof.kozlowski+dt@linaro.org>; Stanislav Jakubek
> <stano.jakubek@gmail.com>; Linus Walleij <linus.walleij@linaro.org>; Samuel
> Holland <samuel@sholland.org>; linux-i2c@vger.kernel.org;
> devicetree@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH v1 3/3] misc: Add meta cld driver
> 
>   Security Reminder: Please be aware that this email is sent by an external
> sender.
> 
> On Tue, Jan 17, 2023 at 05:44:22PM +0800, Delphine CC Chiu wrote:
> > Add support for meta control-logic-device driver. The CLD manages the
> > server system power squence and other state such as host-power-state,
> > uart-selection and presense-slots. The baseboard management controller
> > (BMC) can access the CLD through I2C.
> >
> > The version 1 of CLD driver is supported. The registers number, name
> > and mode of CLD can be defined in dts file for version 1. The driver
> > exports the filesystem following the dts setting.
> >
> > Signed-off-by: Delphine CC Chiu <Delphine_CC_Chiu@Wiwynn.com>
> > Tested-by: Bonnie Lo <Bonnie_Lo@Wiwynn.com>
> > ---
> >  MAINTAINERS                         |   6 +
> >  drivers/misc/Kconfig                |   9 +
> >  drivers/misc/Makefile               |   1 +
> >  drivers/misc/control-logic-device.c | 443
> > ++++++++++++++++++++++++++++
> 
> That is a very generic name for a very specific driver.  Please make it more
> hardware-specific.

In server project, there is a component (control-logic-device). This component manages the server status including whole system power sequence, status and other devices presence status. And baseboard management controller (BMC) on server can acquire the information from CLD device through I2C.
Currently, our customer plan to follow the spec to design the computing server. 
We would like to change the naming from CLD to "compute CPLD".
Do you have any suggestion?

> 
> Also, you add a bunch of undocumented sysfs files here, which require a
> Documentation/ABI/ entries so that we can review the code to verify it does
> what you all think it does.

We will add the document in Documentation/ABI/testing folder.

> 
> And finally, why is this needed to be a kernel driver at all?  Why can't you
> control this all through the userspace i2c api?

After discussing with our customer, they prefer the userspace access the physical device through driver not I2C API.
There is an example on the OpenBMC Gerrit.
https://gerrit.openbmc.org/c/openbmc/phosphor-buttons/+/60807

> 
> One review comment:
> 
> > +static int cld_remove(struct i2c_client *client) {
> > +     struct device *dev = &client->dev;
> > +     struct cld_client *cld = dev_get_drvdata(dev);
> > +
> > +     if (cld->task) {
> > +             kthread_stop(cld->task);
> > +             cld->task = NULL;
> > +     }
> > +
> > +     devm_kfree(dev, cld);
> 
> Whenever you see this line in code, it's almost always a huge red flag that
> someone is not using the devm_* api properly.  I think that is most certainly
> the case here.

Do you mean the dev_free function is no need in this remove function?

> 
> thanks,
> 
> greg k-h

Thanks,
Delphine

RE: [PATCH v1 2/3] dt-bindings: soc: meta: Add meta cld driver bindings

[Delphine_CC_Chiu/WYHQ/Wiwynn]

Hi Krzysztof,

Thanks for your comment.

> -----Original Message-----
> From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> Sent: Tuesday, January 17, 2023 6:50 PM
> To: Delphine_CC_Chiu/WYHQ/Wiwynn <Delphine_CC_Chiu@wiwynn.com>;
> patrick@stwcx.xyz; Rob Herring <robh+dt@kernel.org>; Krzysztof Kozlowski
> <krzysztof.kozlowski+dt@linaro.org>
> Cc: garnermic@fb.com; Derek Kiernan <derek.kiernan@xilinx.com>; Dragan
> Cvetic <dragan.cvetic@xilinx.com>; Arnd Bergmann <arnd@arndb.de>; Greg
> Kroah-Hartman <gregkh@linuxfoundation.org>; Stanislav Jakubek
> <stano.jakubek@gmail.com>; Daniel Palmer <daniel@0x0f.com>; Linus Walleij
> <linus.walleij@linaro.org>; Samuel Holland <samuel@sholland.org>;
> linux-i2c@vger.kernel.org; devicetree@vger.kernel.org;
> linux-kernel@vger.kernel.org
> Subject: Re: [PATCH v1 2/3] dt-bindings: soc: meta: Add meta cld driver
> bindings
> 
>   Security Reminder: Please be aware that this email is sent by an external
> sender.
> 
> On 17/01/2023 10:44, Delphine CC Chiu wrote:
> > Add a device tree bindings for Meta control logic device.
> 
> Subject: drop second/last, redundant "bindings". The "dt-bindings"
> prefix is already stating that these are bindings.

We will remove the bindings from the subject.

> 
> Subject: Drop "driver"

We will revise the subject.

> 
> >
> > Signed-off-by: Delphine CC Chiu <Delphine_CC_Chiu@Wiwynn.com>
> > ---
> >  .../misc/meta,control-logic-device.txt        | 37 +++++++++++++++++++
> >  1 file changed, 37 insertions(+)
> >  create mode 100644
> > Documentation/devicetree/bindings/misc/meta,control-logic-device.txt
> >
> > diff --git
> > a/Documentation/devicetree/bindings/misc/meta,control-logic-device.txt
> > b/Documentation/devicetree/bindings/misc/meta,control-logic-device.txt
> > new file mode 100644
> > index 000000000000..e966368e2fd6
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/misc/meta,control-logic-device
> > +++ .txt
> 
> New bindings only in DT schema format. Start
> Documentation/devicetree/bindings/writing-schema.rst,
> example-schema.yaml and other docs.

We will rewrite the binding document following the schema.

> 
> 
> 
> Best regards,
> Krzysztof

Thanks,
Delphine

RE: [PATCH v1 3/3] misc: Add meta cld driver

[Delphine_CC_Chiu/WYHQ/Wiwynn]

Hi Krzysztof,

Thanks for your review comment.

> -----Original Message-----
> From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> Sent: Tuesday, January 17, 2023 6:55 PM
> To: Delphine_CC_Chiu/WYHQ/Wiwynn <Delphine_CC_Chiu@wiwynn.com>;
> patrick@stwcx.xyz; Derek Kiernan <derek.kiernan@xilinx.com>; Dragan Cvetic
> <dragan.cvetic@xilinx.com>; Arnd Bergmann <arnd@arndb.de>; Greg
> Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: garnermic@fb.com; Rob Herring <robh+dt@kernel.org>; Krzysztof
> Kozlowski <krzysztof.kozlowski+dt@linaro.org>; Stanislav Jakubek
> <stano.jakubek@gmail.com>; Linus Walleij <linus.walleij@linaro.org>; Samuel
> Holland <samuel@sholland.org>; linux-i2c@vger.kernel.org;
> devicetree@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH v1 3/3] misc: Add meta cld driver
> 
>   Security Reminder: Please be aware that this email is sent by an external
> sender.
> 
> On 17/01/2023 10:44, Delphine CC Chiu wrote:
> > Add support for meta control-logic-device driver. The CLD manages the
> > server system power squence and other state such as host-power-state,
> > uart-selection and presense-slots. The baseboard management controller
> > (BMC) can access the CLD through I2C.
> >
> > The version 1 of CLD driver is supported. The registers number, name
> > and mode of CLD can be defined in dts file for version 1. The driver
> > exports the filesystem following the dts setting.
> >
> > Signed-off-by: Delphine CC Chiu <Delphine_CC_Chiu@Wiwynn.com>
> > Tested-by: Bonnie Lo <Bonnie_Lo@Wiwynn.com>
> > ---
> >  MAINTAINERS                         |   6 +
> >  drivers/misc/Kconfig                |   9 +
> >  drivers/misc/Makefile               |   1 +
> >  drivers/misc/control-logic-device.c | 443
> > ++++++++++++++++++++++++++++
> >  4 files changed, 459 insertions(+)
> >  create mode 100644 drivers/misc/control-logic-device.c
> >
> > diff --git a/MAINTAINERS b/MAINTAINERS index
> > 7483853880b6..46e250a2c334 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -13388,6 +13388,12 @@ T:   git git://linuxtv.org/media_tree.git
> >  F:   Documentation/devicetree/bindings/media/amlogic,gx-vdec.yaml
> >  F:   drivers/staging/media/meson/vdec/
> >
> > +META CPLD DRIVER
> > +M:   Delphine CC Chiu <Delphine_CC_Chiu@Wiwynn.com>
> > +L:   linux-i2c@vger.kernel.org
> > +S:   Maintained
> > +F:
> Documentation/devicetree/bindings/misc/meta,control-logic-device.txt
> 
> Missing entries for driver code.

We saw there are entries defined in MAINTAINERS file(M, R, L, S, W, Q, B, C, P, T, F, X, N, K).
Could you guide us which entries are must to have?

> 
> > +
> >  METHODE UDPU SUPPORT
> >  M:   Vladimir Vid <vladimir.vid@sartura.hr>
> >  S:   Maintained
> > diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig index
> > 358ad56f6524..21d3bf820438 100644
> > --- a/drivers/misc/Kconfig
> > +++ b/drivers/misc/Kconfig
> > @@ -496,6 +496,15 @@ config VCPU_STALL_DETECTOR
> >
> >         If you do not intend to run this kernel as a guest, say N.
> >
> > +config CONTROL_LOGIC_DEVICE
> > +        bool "Meta Control Logic Device Driver"
> 
> Too generic...
> 
> > +        depends on I2C && REGMAP
> > +        help
> > +          Say yes here to add support for the Meta CLD device driver. The
> dirver
> > +          is used to monitor CLD register value and notify the application
> if
> > +          value is changed. Application also can access the CLD register
> value
> > +          through this driver.
> > +
> >  source "drivers/misc/c2port/Kconfig"
> >  source "drivers/misc/eeprom/Kconfig"
> >  source "drivers/misc/cb710/Kconfig"
> > diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile index
> > ac9b3e757ba1..6fcdd575a143 100644
> > --- a/drivers/misc/Makefile
> > +++ b/drivers/misc/Makefile
> > @@ -62,3 +62,4 @@ obj-$(CONFIG_HI6421V600_IRQ)        +=
> hi6421v600-irq.o
> >  obj-$(CONFIG_OPEN_DICE)              += open-dice.o
> >  obj-$(CONFIG_GP_PCI1XXXX)    += mchp_pci1xxxx/
> >  obj-$(CONFIG_VCPU_STALL_DETECTOR)    += vcpu_stall_detector.o
> > +obj-$(CONFIG_CONTROL_LOGIC_DEVICE) += control-logic-device.o
> 
> Does not look like ordered by name.

The file look like without ordered by name so we added the configuration in the tail.
Do we need to order the Makefile?

> 
> > diff --git a/drivers/misc/control-logic-device.c
> > b/drivers/misc/control-logic-device.c
> > new file mode 100644
> > index 000000000000..07113dcb0836
> > --- /dev/null
> > +++ b/drivers/misc/control-logic-device.c
> > @@ -0,0 +1,443 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/*
> > + * Copyright (c) 2022 Meta Inc.
> > + */
> > +
> > + #include <linux/module.h>
> 
> Don't prepend with spaces. Check the code for such trivial style issues first.

We will revise this.

> 
> > +#include <linux/i2c.h>
> > +#include <linux/regmap.h>
> > +#include <linux/sysfs.h>
> > +#include <linux/of.h>
> > +#include <linux/of_device.h>
> > +#include <linux/stddef.h>
> > +#include <linux/kthread.h>
> > +
> > +#define CLD_LABEL_LEN  64 /* label = "name:mode:offset" */ #define
> > +to_cld_reg(x) container_of(x, struct cld_data, bin)
> > +
> > +struct cld_register_info {
> > +     const char *name;
> > +     umode_t mode;
> > +     u8 reg;
> > +     u8 value;
> > +};
> > +
> > +struct cld_data {
> > +     struct list_head list;
> > +     struct bin_attribute bin;
> > +     struct kernfs_node *kn;
> > +     struct cld_register_info info;
> > +};
> > +
> > +struct cld_client {
> > +     struct bin_attribute new_register_bin;
> > +     struct bin_attribute delet_register_bin;
> > +     struct regmap *regmap;
> > +     struct device *dev;
> > +     struct cld_data *data;
> > +     struct task_struct *task;
> > +     size_t num_attributes;
> > +};
> > +
> > +struct meta_cld_of_ops {
> > +     int (*load_registers)(struct cld_client *cld); };
> > +
> > +static struct regmap_config cld_regmap_config = {
> > +     .reg_bits = 8,
> > +     .val_bits = 8,
> > +     .cache_type = REGCACHE_NONE,
> > +};
> > +
> > +static ssize_t cld_register_read(struct file *flip, struct kobject *kobj,
> > +                              struct bin_attribute *attr, char *buf,
> > +                              loff_t pos, size_t count) {
> > +     int ret;
> > +     unsigned int value;
> > +     struct device *dev = kobj_to_dev(kobj);
> > +     struct cld_client *cld = dev_get_drvdata(dev);
> > +     struct cld_data *data = to_cld_reg(attr);
> > +
> > +     ret = regmap_read(cld->regmap, data->info.reg, &value);
> > +     if (ret != 0)
> > +             return ret;
> > +
> > +     if ((data->info.mode & 0400) == 0400)
> > +             data->info.value = value;
> > +
> > +     snprintf(buf, sizeof(value), "%d\n", value);
> > +
> > +     return strlen(buf);
> > +}
> > +
> > +static ssize_t cld_register_write(struct file *flip, struct kobject *kobj,
> > +                               struct bin_attribute *attr, char *buf,
> > +                               loff_t pos, size_t count) {
> > +     int ret;
> > +     unsigned long val;
> > +     struct device *dev = kobj_to_dev(kobj);
> > +     struct cld_client *cld = dev_get_drvdata(dev);
> > +     struct cld_data *data = to_cld_reg(attr);
> > +
> > +     ret = kstrtoul(buf, 0, &val);
> > +     if (ret)
> > +             goto out;
> > +
> > +     if (val >= BIT(8)) {
> > +             ret = -EINVAL;
> > +             goto out;
> > +     }
> > +
> > +     ret = regmap_write(cld->regmap, data->info.reg, val);
> > +     if (ret)
> > +             goto out;
> > +
> > +out:
> > +     return ret ? ret : strlen(buf);
> > +}
> > +
> > +static int cld_bin_register(struct cld_register_info info,
> > +                         struct cld_client *cld) {
> > +     int ret;
> > +     struct cld_data *data, *pos;
> > +     struct bin_attribute *bin;
> > +     size_t length;
> > +
> > +     list_for_each_entry(pos, &cld->data->list, list) {
> > +             length = (strlen(info.name) > strlen(pos->info.name)) ?
> > +                      strlen(info.name):strlen(pos->info.name);
> > +             if (!strncasecmp(info.name, pos->info.name, length))
> > +                     return -EEXIST;
> > +     }
> > +
> > +     data = devm_kzalloc(cld->dev, sizeof(*data), GFP_KERNEL);
> > +     if (!data)
> > +             return -ENOMEM;
> > +
> > +     bin = &data->bin;
> > +     data->info = info;
> > +
> > +     sysfs_bin_attr_init(bin);
> > +     bin->attr.name = kstrdup(info.name, GFP_KERNEL);
> > +     if (!bin->attr.name)
> > +             return -EINVAL;
> > +     bin->attr.mode = info.mode;
> > +     bin->read = cld_register_read;
> > +     bin->write = cld_register_write;
> > +     bin->size = 1;
> > +     ret = sysfs_create_bin_file(&cld->dev->kobj, bin);
> > +     if (ret) {
> > +             dev_err(cld->dev, "%s: failed to create file: %d\n",
> > +                     info.name, ret);
> > +             goto out;
> > +     }
> > +
> > +     data->kn = kernfs_find_and_get(cld->dev->kobj.sd, bin->attr.name);
> > +     if (!data->kn) {
> > +             sysfs_remove_bin_file(&cld->dev->kobj, bin);
> > +             ret = -EFAULT;
> > +             goto out;
> > +     }
> > +
> > +     list_add_tail(&data->list, &cld->data->list);
> > +     ++cld->num_attributes;
> > +out:
> > +     return ret;
> > +}
> > +
> > +static int cld_bin_unregister(struct cld_register_info info,
> > +                           struct cld_client *cld) {
> > +     size_t length;
> > +     struct cld_data *pos;
> > +
> > +     list_for_each_entry(pos, &cld->data->list, list) {
> > +             length = (strlen(info.name) > strlen(pos->info.name)) ?
> > +                      strlen(info.name):strlen(pos->info.name);
> > +             if ((!strncasecmp(info.name, pos->info.name, length)) &&
> > +                 (info.mode == pos->info.mode) &&
> > +                 (info.reg == pos->info.reg)) {
> > +                     kernfs_put(pos->kn);
> > +                     sysfs_remove_bin_file(&cld->dev->kobj,
> > +                                           &pos->bin);
> > +                     list_del(&pos->list);
> > +                     devm_kfree(cld->dev, pos);
> > +                     --cld->num_attributes;
> > +                     return 0;
> > +             }
> > +     }
> > +
> > +     dev_err(cld->dev, "%s: cannot match cld data\n", info.name);
> > +     return -EINVAL;
> > +}
> > +
> > +static int cld_parse_label(char *label, struct cld_register_info
> > +*info) {
> > +     char name[CLD_LABEL_LEN] = { 0 };
> > +     char *mode_str, *reg_str;
> > +     int rc, i;
> > +     unsigned long reg;
> > +     umode_t mode;
> > +     size_t length;
> > +     struct {
> > +             char *mode;
> > +             int value;
> > +     } options[] = {
> > +             {"rw", 0600},
> > +             {"r", 0400},
> > +             {"w", 0200},
> > +     };
> > +
> > +     strncpy(name, label, CLD_LABEL_LEN);
> > +
> > +     mode_str = strchr(name, ':');
> > +     if (!mode_str)
> > +             return -EINVAL;
> > +     *mode_str++ = '\0';
> > +
> > +     reg_str = strchr(mode_str, ':');
> > +     if (!reg_str)
> > +             return -EINVAL;
> > +     *reg_str++ = '\0';
> > +
> > +     if ((*name == '\0') || (*mode_str == '\0') || (*reg_str == '\0'))
> > +             return -EINVAL;
> > +
> > +     for (i = 0; i < ARRAY_SIZE(options); i++) {
> > +             length = (strlen(options[i].mode) > strlen(mode_str)) ?
> > +                      strlen(options[i].mode):strlen(mode_str);
> > +             if (strncasecmp(mode_str, options[i].mode, length) == 0) {
> > +                     rc = kstrtoul(reg_str, 0, &reg);
> > +                     if (rc)
> > +                             return -EINVAL;
> > +                     mode = options[i].value;
> > +                     break;
> > +             }
> > +     }
> > +     if ((i >= ARRAY_SIZE(options)) || (reg >= BIT(8)))
> > +             return -EINVAL;
> > +
> > +     info->name = kstrdup(name, GFP_KERNEL);
> > +     if (!info->name)
> > +             return -EINVAL;
> > +     info->reg = reg;
> > +     info->mode = mode;
> > +     return 0;
> > +}
> > +
> > +static int cld_load_registers(struct cld_client *cld, const char
> > +*property) {
> > +     const char *label;
> > +     int count, ret, i;
> > +     struct cld_register_info info;
> > +     struct device *dev = cld->dev;
> > +
> > +     count = of_property_count_strings(dev->of_node, property);
> > +     if (count <= 0)
> > +             return -EINVAL;
> > +
> > +     for (i = 0; i < count; i++) {
> > +             ret = of_property_read_string_index(dev->of_node,
> property, i,
> > +                                                 &label);
> > +             if (ret) {
> > +                     dev_err(dev, ": failed to get label for index %d\n",
> > +                             i);
> > +                     continue;
> > +             }
> > +
> > +             ret = cld_parse_label((char *)label, &info);
> > +             if (ret) {
> > +                     dev_err(cld->dev, "%s: failed to parse label\n",
> > +                             label);
> > +                     continue;
> > +             }
> > +             cld_bin_register(info, cld);
> > +     }
> > +     return (count == cld->num_attributes) ? 0 : (-EINVAL); }
> > +
> > +static int meta_cld_of_v1_load_registers(struct cld_client *cld) {
> > +     int ret;
> > +
> > +     ret = cld_load_registers(cld, "registers-map");
> > +     if (ret)
> > +             dev_dbg(cld->dev, "failed to load registers\n");
> > +
> > +     return 0;
> > +}
> > +
> > +static ssize_t cld_sysfs_new_register(struct file *filp, struct kobject *kobj,
> > +                                   struct bin_attribute *attr,
> > +                                   char *buf, loff_t pos, size_t
> > +count) {
> > +     int ret;
> > +     struct device *dev = kobj_to_dev(kobj);
> > +     struct cld_client *cld = dev_get_drvdata(dev);
> > +     struct cld_register_info info;
> > +
> > +     ret = cld_parse_label(buf, &info);
> > +     if (ret) {
> > +             dev_err(cld->dev, "failed to parse label\n");
> > +             goto out;
> > +     }
> > +     ret = cld_bin_register(info, cld);
> > +
> > +out:
> > +     return (!ret) ? count : ret;
> > +}
> > +
> > +static ssize_t cld_sysfs_delete_register(struct file *filp,
> > +                                      struct kobject *kobj,
> > +                                      struct bin_attribute *attr,
> > +                                      char *buf, loff_t pos, size_t
> > +count) {
> > +     int ret;
> > +     struct device *dev = kobj_to_dev(kobj);
> > +     struct cld_client *cld = dev_get_drvdata(dev);
> > +     struct cld_register_info info;
> > +
> > +     ret = cld_parse_label(buf, &info);
> > +     if (ret) {
> > +             dev_err(cld->dev, "failed to parse label\n");
> > +             goto out;
> > +     }
> > +
> > +     ret = cld_bin_unregister(info, cld);
> > +
> > +out:
> > +     return (!ret) ? count : ret;
> > +}
> > +
> > +static int cld_monitor_thread(void *client) {
> > +     struct cld_client *cld = client;
> > +     unsigned int value;
> > +     int ret;
> > +     struct cld_data *pos;
> > +
> > +     while (!kthread_should_stop()) {
> > +             list_for_each_entry(pos, &cld->data->list, list) {
> > +                     if ((pos->info.mode & 0400) == 0400) {
> > +                             ret = regmap_read(cld->regmap,
> pos->info.reg,
> > +                                               &value);
> > +                             if (ret)
> > +                                     continue;
> > +                             if (pos->info.value != value) {
> > +                                     pos->info.value = value;
> > +                                     kernfs_notify(pos->kn);
> > +                             }
> > +                     }
> > +             }
> > +             msleep(50);
> > +     }
> > +     return 0;
> > +}
> > +
> > +static int cld_probe(struct i2c_client *client,
> > +                  const struct i2c_device_id *id) {
> > +     int ret = 0;
> > +     const struct meta_cld_of_ops *ops;
> > +     struct device *dev = &client->dev;
> > +     struct cld_client *cld;
> > +
> > +     cld = devm_kzalloc(dev, sizeof(*cld), GFP_KERNEL);
> > +     if (!cld) {
> > +             ret = -ENOMEM;
> 
> return

We will revise this.

> 
> > +             goto out;
> > +     }
> > +
> > +     cld->dev = dev;
> > +     cld->num_attributes = 0;
> > +     cld->regmap = devm_regmap_init_i2c(client, &cld_regmap_config);
> > +     cld->data = devm_kzalloc(cld->dev, sizeof(struct cld_data),
> 
> sizeof(*)
> 
> > +                              GFP_KERNEL);
> > +     if (!cld->data) {
> > +             ret = -ENOMEM;
> 
> return

We will revise this.

> 
> 
> > +             goto out;
> > +     }
> > +
> > +     INIT_LIST_HEAD(&cld->data->list);
> > +
> > +     sysfs_bin_attr_init(&cld->new_register_bin);
> > +     cld->new_register_bin.attr.name = "new_register";
> > +     cld->new_register_bin.attr.mode = 0200;
> > +     cld->new_register_bin.write = cld_sysfs_new_register;
> > +     ret = sysfs_create_bin_file(&dev->kobj, &cld->new_register_bin);
> > +     if (ret)
> > +             goto out;
> > +
> > +     sysfs_bin_attr_init(&cld->delet_register_bin);
> > +     cld->delet_register_bin.attr.name = "delete_register";
> > +     cld->delet_register_bin.attr.mode = 0200;
> > +     cld->delet_register_bin.write = cld_sysfs_delete_register;
> > +     ret = sysfs_create_bin_file(&dev->kobj, &cld->delet_register_bin);
> > +     if (ret)
> > +             goto out;
> 
> return

We will revise this.

> 
> > +
> > +     ops = of_device_get_match_data(cld->dev);
> > +     if (!ops) {
> > +             ret = -EINVAL;
> 
> return

We will revise this.

> 
> > +             goto out;
> > +     }
> > +
> > +     ret = ops->load_registers(cld);
> > +
> > +     cld->task = kthread_run(cld_monitor_thread, (void *)cld,
> > +                             "register_monitor");
> 
> Why do you need the thread? What do you monitor? All this should be
> explained and documented.

The thread monitoring all the readable offsets and notify the userspace through kernfs_notify function if the offset value is changed.
We will add the description in binding document.

> 
> > +     if (IS_ERR(cld->task)) {
> > +             ret = PTR_ERR(cld->task);
> 
> return

We will revise this.

> 
> > +             cld->task = NULL;
> 
> Drop
> 
> > +             goto out;
> > +     }
> > +
> > +     dev_set_drvdata(dev, cld);
> 
> This should happen before starting thread.

We will revise this.

> 
> > +
> > +out:
> > +     return ret;
> > +}
> > +
> > +static int cld_remove(struct i2c_client *client) {
> > +     struct device *dev = &client->dev;
> > +     struct cld_client *cld = dev_get_drvdata(dev);
> > +
> > +     if (cld->task) {
> > +             kthread_stop(cld->task);
> > +             cld->task = NULL;
> > +     }
> > +
> > +     devm_kfree(dev, cld);
> > +     return 0;
> > +}
> > +
> > +static const struct meta_cld_of_ops of_v1_ops = {
> > +     .load_registers = meta_cld_of_v1_load_registers, };
> > +
> > +static const struct of_device_id cld_of_match[] = {
> 
> This would warn now. Drop of_match_ptr.

We will fix this.

> 
> > +     {
> > +             .compatible = "meta,control-logic-device-v1",
> > +             .data = &of_v1_ops,
> > +     },
> > +     {}
> > +};
> > +
> > +MODULE_DEVICE_TABLE(of, cld_of_match)
> > +
> > +static struct i2c_driver cld = {
> > +     .driver = { .name = "cld-driver",
> > +                 .of_match_table = of_match_ptr(cld_of_match) },
> 
> Blank line before }

We will fix this.

> 
> > +     .probe = cld_probe,
> > +     .remove = cld_remove,
> > +};
> > +
> > +module_i2c_driver(cld);
> > +
> > +MODULE_AUTHOR("Ren Chen <ren_chen@wiwynn.com>");
> > +MODULE_DESCRIPTION("Meta control logic device driver");
> > +MODULE_LICENSE("GPL");
> 
> Best regards,
> Krzysztof

Thanks,
Delphine

RE: [PATCH v1 3/3] misc: Add meta cld driver

[Delphine_CC_Chiu/WYHQ/Wiwynn]

Hi Linus Walleij,

Thanks for your review comment.

> -----Original Message-----
> From: Linus Walleij <linus.walleij@linaro.org>
> Sent: Tuesday, January 17, 2023 7:40 PM
> To: Delphine_CC_Chiu/WYHQ/Wiwynn <Delphine_CC_Chiu@wiwynn.com>
> Cc: patrick@stwcx.xyz; Derek Kiernan <derek.kiernan@xilinx.com>; Dragan
> Cvetic <dragan.cvetic@xilinx.com>; Arnd Bergmann <arnd@arndb.de>; Greg
> Kroah-Hartman <gregkh@linuxfoundation.org>; garnermic@fb.com; Rob
> Herring <robh+dt@kernel.org>; Krzysztof Kozlowski
> <krzysztof.kozlowski+dt@linaro.org>; Stanislav Jakubek
> <stano.jakubek@gmail.com>; Samuel Holland <samuel@sholland.org>;
> linux-i2c@vger.kernel.org; devicetree@vger.kernel.org;
> linux-kernel@vger.kernel.org; Lee Jones <lee@kernel.org>; Sebastian Reichel
> <sre@kernel.org>
> Subject: Re: [PATCH v1 3/3] misc: Add meta cld driver
> 
>   Security Reminder: Please be aware that this email is sent by an external
> sender.
> 
> Hi Delphine,
> 
> thanks for your patch!
> 
> On Tue, Jan 17, 2023 at 10:46 AM Delphine CC Chiu
> <Delphine_CC_Chiu@wiwynn.com> wrote:
> 
> > Add support for meta control-logic-device driver. The CLD manages the
> > server system power squence and other state such as host-power-state,
> > uart-selection and presense-slots. The baseboard management controller
> > (BMC) can access the CLD through I2C.
> >
> > The version 1 of CLD driver is supported. The registers number, name
> > and mode of CLD can be defined in dts file for version 1. The driver
> > exports the filesystem following the dts setting.
> >
> > Signed-off-by: Delphine CC Chiu <Delphine_CC_Chiu@Wiwynn.com>
> > Tested-by: Bonnie Lo <Bonnie_Lo@Wiwynn.com>
> 
> Why should this driver be in drivers/misc and not drivers/mfd?

The cld device is not a physical ASIC. It's a controller based on FPGA device and the FPGA may be Altera or Lattice.
So, we put the cld driver in misc folder. Is the cld driver suitable to put in mfd folder?

> MFS has support code for spawning child devices for the LED you are also
> creating for example, so please use that.

Could you please guide us which device driver we can refer?

> 
> > +#include <linux/sysfs.h>
> (...)
> > +#include <linux/kthread.h>
> (...)
> 
> > +static ssize_t cld_register_read(struct file *flip, struct kobject *kobj,
> > +                                struct bin_attribute *attr, char *buf,
> > +                                loff_t pos, size_t count) {
> (...)
> > +       snprintf(buf, sizeof(value), "%d\n", value);
> (...)
> > +static ssize_t cld_register_write(struct file *flip, struct kobject *kobj,
> > +                                 struct bin_attribute *attr, char
> *buf,
> > +                                 loff_t pos, size_t count) {
> > +       ret = kstrtoul(buf, 0, &val);
> (...)
> 
> Writing and reading some random regmap registers is something that the
> regmap debugfs already can do.
> 
> > +static int cld_bin_register(struct cld_register_info info,
> > +                           struct cld_client *cld) {
> 
> And this is for reading and writing binary blobs.
> 
> It looks like something that should be using the firmware API.
> 
> If the purpose of the driver is to open a hole from userspace down to the
> hardware, as Greg says why not just use userspace I2C then?
> 
> It seems a bit dangerous to relay whatever the ASIC is doing to userspace
> though.
> 
> Are you sure you can't use any of the existing kernel functionality for doing
> what these userspace "hole" is doing?
> 
> There is drivers/power etc for power control and I bet it can be extended if
> need be.

We will discuss with our customer.

> 
> Yours,
> Linus Walleij

Thanks,
Delphine

Re: [PATCH v1 3/3] misc: Add meta cld driver

[Krzysztof Kozlowski]

On 13/03/2023 09:48, Delphine_CC_Chiu/WYHQ/Wiwynn wrote:
> Hi Krzysztof,
> 
> Thanks for your review comment.
> 
>> -----Original Message-----
>> From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
>> Sent: Tuesday, January 17, 2023 6:55 PM
>> To: Delphine_CC_Chiu/WYHQ/Wiwynn <Delphine_CC_Chiu@wiwynn.com>;
>> patrick@stwcx.xyz; Derek Kiernan <derek.kiernan@xilinx.com>; Dragan Cvetic
>> <dragan.cvetic@xilinx.com>; Arnd Bergmann <arnd@arndb.de>; Greg
>> Kroah-Hartman <gregkh@linuxfoundation.org>
>> Cc: garnermic@fb.com; Rob Herring <robh+dt@kernel.org>; Krzysztof
>> Kozlowski <krzysztof.kozlowski+dt@linaro.org>; Stanislav Jakubek
>> <stano.jakubek@gmail.com>; Linus Walleij <linus.walleij@linaro.org>; Samuel
>> Holland <samuel@sholland.org>; linux-i2c@vger.kernel.org;
>> devicetree@vger.kernel.org; linux-kernel@vger.kernel.org
>> Subject: Re: [PATCH v1 3/3] misc: Add meta cld driver
>>
>>   Security Reminder: Please be aware that this email is sent by an external
>> sender.
>>
>> On 17/01/2023 10:44, Delphine CC Chiu wrote:
>>> Add support for meta control-logic-device driver. The CLD manages the
>>> server system power squence and other state such as host-power-state,
>>> uart-selection and presense-slots. The baseboard management controller
>>> (BMC) can access the CLD through I2C.
>>>
>>> The version 1 of CLD driver is supported. The registers number, name
>>> and mode of CLD can be defined in dts file for version 1. The driver
>>> exports the filesystem following the dts setting.
>>>
>>> Signed-off-by: Delphine CC Chiu <Delphine_CC_Chiu@Wiwynn.com>
>>> Tested-by: Bonnie Lo <Bonnie_Lo@Wiwynn.com>
>>> ---
>>>  MAINTAINERS                         |   6 +
>>>  drivers/misc/Kconfig                |   9 +
>>>  drivers/misc/Makefile               |   1 +
>>>  drivers/misc/control-logic-device.c | 443
>>> ++++++++++++++++++++++++++++
>>>  4 files changed, 459 insertions(+)
>>>  create mode 100644 drivers/misc/control-logic-device.c
>>>
>>> diff --git a/MAINTAINERS b/MAINTAINERS index
>>> 7483853880b6..46e250a2c334 100644
>>> --- a/MAINTAINERS
>>> +++ b/MAINTAINERS
>>> @@ -13388,6 +13388,12 @@ T:   git git://linuxtv.org/media_tree.git
>>>  F:   Documentation/devicetree/bindings/media/amlogic,gx-vdec.yaml
>>>  F:   drivers/staging/media/meson/vdec/
>>>
>>> +META CPLD DRIVER
>>> +M:   Delphine CC Chiu <Delphine_CC_Chiu@Wiwynn.com>
>>> +L:   linux-i2c@vger.kernel.org
>>> +S:   Maintained
>>> +F:
>> Documentation/devicetree/bindings/misc/meta,control-logic-device.txt
>>
>> Missing entries for driver code.
> 
> We saw there are entries defined in MAINTAINERS file(M, R, L, S, W, Q, B, C, P, T, F, X, N, K).
> Could you guide us which entries are must to have?

The driver paths.

>>>  obj-$(CONFIG_OPEN_DICE)              += open-dice.o
>>>  obj-$(CONFIG_GP_PCI1XXXX)    += mchp_pci1xxxx/
>>>  obj-$(CONFIG_VCPU_STALL_DETECTOR)    += vcpu_stall_detector.o
>>> +obj-$(CONFIG_CONTROL_LOGIC_DEVICE) += control-logic-device.o
>>
>> Does not look like ordered by name.
> 
> The file look like without ordered by name so we added the configuration in the tail.

Indeed, but don't add stuff to the end. Conflict-prone.



Best regards,
Krzysztof

Re: [PATCH v1 3/3] misc: Add meta cld driver

[Greg Kroah-Hartman]

On Mon, Mar 13, 2023 at 08:47:45AM +0000, Delphine_CC_Chiu/WYHQ/Wiwynn wrote:
> Hi Greg,
> 
> Thanks for your comment!
> 
> > -----Original Message-----
> > From: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > Sent: Tuesday, January 17, 2023 6:15 PM
> > To: Delphine_CC_Chiu/WYHQ/Wiwynn <Delphine_CC_Chiu@wiwynn.com>
> > Cc: patrick@stwcx.xyz; Derek Kiernan <derek.kiernan@xilinx.com>; Dragan
> > Cvetic <dragan.cvetic@xilinx.com>; Arnd Bergmann <arnd@arndb.de>;
> > garnermic@fb.com; Rob Herring <robh+dt@kernel.org>; Krzysztof Kozlowski
> > <krzysztof.kozlowski+dt@linaro.org>; Stanislav Jakubek
> > <stano.jakubek@gmail.com>; Linus Walleij <linus.walleij@linaro.org>; Samuel
> > Holland <samuel@sholland.org>; linux-i2c@vger.kernel.org;
> > devicetree@vger.kernel.org; linux-kernel@vger.kernel.org
> > Subject: Re: [PATCH v1 3/3] misc: Add meta cld driver
> > 
> >   Security Reminder: Please be aware that this email is sent by an external
> > sender.
> > 
> > On Tue, Jan 17, 2023 at 05:44:22PM +0800, Delphine CC Chiu wrote:
> > > Add support for meta control-logic-device driver. The CLD manages the
> > > server system power squence and other state such as host-power-state,
> > > uart-selection and presense-slots. The baseboard management controller
> > > (BMC) can access the CLD through I2C.
> > >
> > > The version 1 of CLD driver is supported. The registers number, name
> > > and mode of CLD can be defined in dts file for version 1. The driver
> > > exports the filesystem following the dts setting.
> > >
> > > Signed-off-by: Delphine CC Chiu <Delphine_CC_Chiu@Wiwynn.com>
> > > Tested-by: Bonnie Lo <Bonnie_Lo@Wiwynn.com>
> > > ---
> > >  MAINTAINERS                         |   6 +
> > >  drivers/misc/Kconfig                |   9 +
> > >  drivers/misc/Makefile               |   1 +
> > >  drivers/misc/control-logic-device.c | 443
> > > ++++++++++++++++++++++++++++
> > 
> > That is a very generic name for a very specific driver.  Please make it more
> > hardware-specific.
> 
> In server project, there is a component (control-logic-device). This component manages the server status including whole system power sequence, status and other devices presence status. And baseboard management controller (BMC) on server can acquire the information from CLD device through I2C.
> Currently, our customer plan to follow the spec to design the computing server. 
> We would like to change the naming from CLD to "compute CPLD".
> Do you have any suggestion?

Make it something hardware/vendor specific please.  As is, this is very
generic.  Remember, this is a name you will be using to refer to for the
next 20+ years.

> > Also, you add a bunch of undocumented sysfs files here, which require a
> > Documentation/ABI/ entries so that we can review the code to verify it does
> > what you all think it does.
> 
> We will add the document in Documentation/ABI/testing folder.
> 
> > 
> > And finally, why is this needed to be a kernel driver at all?  Why can't you
> > control this all through the userspace i2c api?
> 
> After discussing with our customer, they prefer the userspace access the physical device through driver not I2C API.
> There is an example on the OpenBMC Gerrit.
> https://gerrit.openbmc.org/c/openbmc/phosphor-buttons/+/60807

I do not understand, if functionalty can be done in userspace, it should
be done there, UNLESS you have a generic way of handling multiple
hardware devices as the same type (i.e. keyboard, sensor, etc.)  There
does not seem to be any generic interface here, so again, why can't you
just do it all in userspace?  What is forcing a kernel driver for this?

> > One review comment:
> > 
> > > +static int cld_remove(struct i2c_client *client) {
> > > +     struct device *dev = &client->dev;
> > > +     struct cld_client *cld = dev_get_drvdata(dev);
> > > +
> > > +     if (cld->task) {
> > > +             kthread_stop(cld->task);
> > > +             cld->task = NULL;
> > > +     }
> > > +
> > > +     devm_kfree(dev, cld);
> > 
> > Whenever you see this line in code, it's almost always a huge red flag that
> > someone is not using the devm_* api properly.  I think that is most certainly
> > the case here.
> 
> Do you mean the dev_free function is no need in this remove function?

Why do you think it is needed here?  If it is needed, please document it
with a comment explaining why this is required as it is not how the api
is designed to be used at all.

thanks,

greg k-h

Re: [PATCH v1 3/3] misc: Add meta cld driver

[Linus Walleij]

On Mon, Mar 13, 2023 at 9:48 AM Delphine_CC_Chiu/WYHQ/Wiwynn
<Delphine_CC_Chiu@wiwynn.com> wrote:
> > On Tue, Jan 17, 2023 at 10:46 AM Delphine CC Chiu
> > <Delphine_CC_Chiu@wiwynn.com> wrote:

> > Why should this driver be in drivers/misc and not drivers/mfd?
>
> The cld device is not a physical ASIC.

Nobody cares about the difference, that is only a convention.
It has a stable hardware/software API that is all that matters.

> It's a controller based on FPGA device and the FPGA may be Altera or Lattice.
> So, we put the cld driver in misc folder. Is the cld driver suitable to put in mfd folder?

Yes.

> > MFS has support code for spawning child devices for the LED you are also
> > creating for example, so please use that.
>
> Could you please guide us which device driver we can refer?

For example drivers/mfd/stmfx.c another firmware-driven FPGA thing.

Yours,
Linus Walleij