| Message ID | 20230117094425.19004-1-Delphine_CC_Chiu@Wiwynn.com |
|---|---|
| Headers | show |
| Series | Meta control logic device support | expand |
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
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, ®); > > + 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
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
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
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
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