On Fri, 27 Mar 2020, Tim Harvey wrote: > The Gateworks System Controller (GSC) is an I2C slave controller > implemented with an MSP430 micro-controller whose firmware embeds the > following features: > - I/O expander (16 GPIO's) using PCA955x protocol > - Real Time Clock using DS1672 protocol > - User EEPROM using AT24 protocol > - HWMON using custom protocol > - Interrupt controller with tamper detect, user pushbotton > - Watchdog controller capable of full board power-cycle > - Power Control capable of full board power-cycle > > see http://trac.gateworks.com/wiki/gsc for more details > > Cc: Randy Dunlap <rdunlap@xxxxxxxxxxxxx> > Signed-off-by: Tim Harvey <tharvey@xxxxxxxxxxxxx> > --- > v8: > - whitespace fixes > - describe sub-devices in Kconfig > - add error print for invalid command > - update copyright > - use devm_of_platform_populate > - use probe_new > - move hwmon's regmap init to hwmon > > v7: > - remove irq from private data struct > > v6: > - remove duplicate signature and fix commit log > > v5: > - simplify powerdown function > > v4: > - remove hwmon max reg check/define > - fix powerdown function > > v3: > - rename gsc->gateworks-gsc > - remove uncecessary include for linux/mfd/core.h > - upercase I2C in comments > - remove i2c debug > - remove uncecessary comments > - don't use KBUILD_MODNAME for name > - remove unnecessary v1/v2/v3 tracking > - unregister hwmon i2c adapter on remove > > v2: > - change license comment block style > - remove COMPILE_TEST (Randy) > - fixed whitespace issues > - replaced a printk with dev_err > --- > MAINTAINERS | 8 ++ > drivers/mfd/Kconfig | 10 ++ > drivers/mfd/Makefile | 1 + > drivers/mfd/gateworks-gsc.c | 288 ++++++++++++++++++++++++++++++++++++++++++++ > include/linux/mfd/gsc.h | 73 +++++++++++ > 5 files changed, 380 insertions(+) > create mode 100644 drivers/mfd/gateworks-gsc.c > create mode 100644 include/linux/mfd/gsc.h > > diff --git a/MAINTAINERS b/MAINTAINERS > index 56765f5..bb79b60 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -6839,6 +6839,14 @@ F: tools/testing/selftests/futex/ > F: tools/perf/bench/futex* > F: Documentation/*futex* > > +GATEWORKS SYSTEM CONTROLLER (GSC) DRIVER > +M: Tim Harvey <tharvey@xxxxxxxxxxxxx> > +M: Robert Jones <rjones@xxxxxxxxxxxxx> > +S: Maintained > +F: Documentation/devicetree/bindings/mfd/gateworks-gsc.yaml > +F: drivers/mfd/gateworks-gsc.c > +F: include/linux/mfd/gsc.h > + > GCC PLUGINS > M: Kees Cook <keescook@xxxxxxxxxxxx> > R: Emese Revfy <re.emese@xxxxxxxxx> > diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig > index 4209008..d84725a 100644 > --- a/drivers/mfd/Kconfig > +++ b/drivers/mfd/Kconfig > @@ -407,6 +407,16 @@ config MFD_EXYNOS_LPASS > Select this option to enable support for Samsung Exynos Low Power > Audio Subsystem. > > +config MFD_GATEWORKS_GSC > + tristate "Gateworks System Controller" > + depends on (I2C && OF) > + select MFD_CORE > + select REGMAP_I2C > + select REGMAP_IRQ > + help > + Enable support for the Gateworks System Controller found > + on Gateworks Single Board Computers. Please expand on the help section some more. What is the purpose of the device? What sub-devices make up the GSC? What does this device driver offer/provide? > config MFD_MC13XXX > tristate > depends on (SPI_MASTER || I2C) > diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile > index aed99f0..c82b442 100644 > --- a/drivers/mfd/Makefile > +++ b/drivers/mfd/Makefile > @@ -15,6 +15,7 @@ obj-$(CONFIG_MFD_BCM590XX) += bcm590xx.o > obj-$(CONFIG_MFD_BD9571MWV) += bd9571mwv.o > obj-$(CONFIG_MFD_CROS_EC_DEV) += cros_ec_dev.o > obj-$(CONFIG_MFD_EXYNOS_LPASS) += exynos-lpass.o > +obj-$(CONFIG_MFD_GATEWORKS_GSC) += gateworks-gsc.o > > obj-$(CONFIG_HTC_PASIC3) += htc-pasic3.o > obj-$(CONFIG_HTC_I2CPLD) += htc-i2cpld.o > diff --git a/drivers/mfd/gateworks-gsc.c b/drivers/mfd/gateworks-gsc.c > new file mode 100644 > index 00000000..020e1e1 > --- /dev/null > +++ b/drivers/mfd/gateworks-gsc.c > @@ -0,0 +1,288 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * The Gateworks System Controller (GSC) is a multi-function > + * device designed for use in Gateworks Single Board Computers. > + * The control interface is I2C, with an interrupt. The device supports > + * system functions such as pushbutton monitoring, multiple ADC's for > + * voltage and temperature, fan controller, and watchdog monitor. Looks like good information for the help. > + * Copyright (C) 2020 Gateworks Corporation > + */ Nit: '\n' here. > +#include <linux/device.h> > +#include <linux/i2c.h> > +#include <linux/interrupt.h> > +#include <linux/mfd/gsc.h> > +#include <linux/module.h> > +#include <linux/mutex.h> > +#include <linux/of.h> > +#include <linux/of_platform.h> > +#include <linux/platform_device.h> > +#include <linux/regmap.h> > + > +/* > + * The GSC suffers from an errata where occasionally during > + * ADC cycles the chip can NAK I2C transactions. To ensure we have reliable > + * register access we place retries around register access. > + */ > +#define I2C_RETRIES 3 > + > +static int gsc_regmap_regwrite(void *context, unsigned int reg, > + unsigned int val) > +{ > + struct i2c_client *client = context; > + int retry, ret; > + > + for (retry = 0; retry < I2C_RETRIES; retry++) { > + ret = i2c_smbus_write_byte_data(client, reg, val); > + /* > + * -EAGAIN returned when the i2c host controller is busy > + * -EIO returned when i2c device is busy > + */ > + if (ret != -EAGAIN && ret != -EIO) > + break; > + } > + > + return 0; > +} > + > +static int gsc_regmap_regread(void *context, unsigned int reg, > + unsigned int *val) > +{ > + struct i2c_client *client = context; > + int retry, ret; > + > + for (retry = 0; retry < I2C_RETRIES; retry++) { > + ret = i2c_smbus_read_byte_data(client, reg); > + /* > + * -EAGAIN returned when the i2c host controller is busy > + * -EIO returned when i2c device is busy > + */ > + if (ret != -EAGAIN && ret != -EIO) > + break; > + } > + *val = ret & 0xff; > + > + return 0; > +} > + > +/* > + * gsc_powerdown - API to use GSC to power down board for a specific time > + * > + * secs - number of seconds to remain powered off > + */ > +static int gsc_powerdown(struct gsc_dev *gsc, unsigned long secs) > +{ > + int ret; > + unsigned char regs[4]; > + > + dev_info(&gsc->i2c->dev, "GSC powerdown for %ld seconds\n", > + secs); > + > + regs[0] = secs & 0xff; > + regs[1] = (secs >> 8) & 0xff; > + regs[2] = (secs >> 16) & 0xff; > + regs[3] = (secs >> 24) & 0xff; Would put_unaligned_{b,l}e32() make sense here? > + ret = regmap_bulk_write(gsc->regmap, GSC_TIME_ADD, regs, 4); > + if (ret) > + return ret; > + > + regs[0] = 1 << GSC_CTRL_1_LATCH_SLEEP_ADD; BIT() Why does this value need to be loaded into a variable? > + ret = regmap_update_bits(gsc->regmap, GSC_CTRL_1, regs[0], regs[0]); > + if (ret) > + return ret; > + > + regs[0] = (1 << GSC_CTRL_1_ACTIVATE_SLEEP) | > + (1 << GSC_CTRL_1_SLEEP_ENABLE); BIT() > + ret = regmap_update_bits(gsc->regmap, GSC_CTRL_1, regs[0], regs[0]); > + > + return ret; > +} > + > +static ssize_t gsc_show(struct device *dev, struct device_attribute *attr, > + char *buf) > +{ > + struct gsc_dev *gsc = dev_get_drvdata(dev); > + const char *name = attr->attr.name; > + int rz = 0; > + > + if (strcasecmp(name, "fw_version") == 0) > + rz = sprintf(buf, "%d\n", gsc->fwver); > + else if (strcasecmp(name, "fw_crc") == 0) > + rz = sprintf(buf, "0x%04x\n", gsc->fwcrc); > + else > + dev_err(dev, "invalid command: '%s'\n", name); > + > + return rz; > +} > + > +static ssize_t gsc_store(struct device *dev, struct device_attribute *attr, > + const char *buf, size_t count) > +{ > + struct gsc_dev *gsc = dev_get_drvdata(dev); > + const char *name = attr->attr.name; > + long value; > + > + if (strcasecmp(name, "powerdown") == 0) { > + if (kstrtol(buf, 0, &value) == 0) > + gsc_powerdown(gsc, value); > + } else { > + dev_err(dev, "invalid command: '%s\n", name); > + } > + > + return count; > +} > + > +static struct device_attribute attr_fwver = > + __ATTR(fw_version, 0440, gsc_show, NULL); > +static struct device_attribute attr_fwcrc = > + __ATTR(fw_crc, 0440, gsc_show, NULL); > +static struct device_attribute attr_pwrdown = > + __ATTR(powerdown, 0220, NULL, gsc_store); > + > +static struct attribute *gsc_attrs[] = { > + &attr_fwver.attr, > + &attr_fwcrc.attr, > + &attr_pwrdown.attr, > + NULL, > +}; > + > +static struct attribute_group attr_group = { > + .attrs = gsc_attrs, > +}; > + > +static const struct of_device_id gsc_of_match[] = { > + { .compatible = "gw,gsc", }, > + { } > +}; > +MODULE_DEVICE_TABLE(of, gsc_of_match); > + > +static const struct regmap_config gsc_regmap_config = { > + .reg_bits = 8, > + .val_bits = 8, > + .cache_type = REGCACHE_NONE, > + .max_register = 0xf, Nicer if this was defined as part of an enum of registers I think. > +}; > + > +static const struct regmap_irq gsc_irqs[] = { > + REGMAP_IRQ_REG(GSC_IRQ_PB, 0, BIT(GSC_IRQ_PB)), > + REGMAP_IRQ_REG(GSC_IRQ_KEY_ERASED, 0, BIT(GSC_IRQ_KEY_ERASED)), > + REGMAP_IRQ_REG(GSC_IRQ_EEPROM_WP, 0, BIT(GSC_IRQ_EEPROM_WP)), > + REGMAP_IRQ_REG(GSC_IRQ_RESV, 0, BIT(GSC_IRQ_RESV)), > + REGMAP_IRQ_REG(GSC_IRQ_GPIO, 0, BIT(GSC_IRQ_GPIO)), > + REGMAP_IRQ_REG(GSC_IRQ_TAMPER, 0, BIT(GSC_IRQ_TAMPER)), > + REGMAP_IRQ_REG(GSC_IRQ_WDT_TIMEOUT, 0, BIT(GSC_IRQ_WDT_TIMEOUT)), > + REGMAP_IRQ_REG(GSC_IRQ_SWITCH_HOLD, 0, BIT(GSC_IRQ_SWITCH_HOLD)), > +}; > + > +static const struct regmap_irq_chip gsc_irq_chip = { > + .name = "gateworks-gsc", > + .irqs = gsc_irqs, > + .num_irqs = ARRAY_SIZE(gsc_irqs), > + .num_regs = 1, > + .status_base = GSC_IRQ_STATUS, > + .mask_base = GSC_IRQ_ENABLE, > + .mask_invert = true, > + .ack_base = GSC_IRQ_STATUS, > + .ack_invert = true, > +}; > + > +static int gsc_probe(struct i2c_client *client) > +{ > + struct device *dev = &client->dev; > + struct gsc_dev *gsc; > + int ret; > + unsigned int reg; > + > + gsc = devm_kzalloc(dev, sizeof(*gsc), GFP_KERNEL); > + if (!gsc) > + return -ENOMEM; > + > + gsc->dev = &client->dev; > + gsc->i2c = client; > + i2c_set_clientdata(client, gsc); > + > + gsc->bus.reg_write = gsc_regmap_regwrite; > + gsc->bus.reg_read = gsc_regmap_regread; Why do you need to store these in ddata? > + gsc->regmap = devm_regmap_init(dev, &gsc->bus, client, > + &gsc_regmap_config); > + if (IS_ERR(gsc->regmap)) > + return PTR_ERR(gsc->regmap); > + > + if (regmap_read(gsc->regmap, GSC_FW_VER, ®)) > + return -EIO; > + gsc->fwver = reg; > + > + regmap_read(gsc->regmap, GSC_FW_CRC, ®); > + gsc->fwcrc = reg; > + regmap_read(gsc->regmap, GSC_FW_CRC + 1, ®); > + gsc->fwcrc |= reg << 8; > + > + gsc->i2c_hwmon = i2c_new_dummy_device(client->adapter, GSC_HWMON); > + if (!gsc->i2c_hwmon) { > + dev_err(dev, "Failed to allocate I2C device for HWMON\n"); > + return -ENODEV; > + } > + i2c_set_clientdata(gsc->i2c_hwmon, gsc); You already have this set. Just 'get' it from client->dev.parent (or whatever it is). > + ret = devm_regmap_add_irq_chip(dev, gsc->regmap, client->irq, > + IRQF_ONESHOT | IRQF_SHARED | > + IRQF_TRIGGER_FALLING, 0, > + &gsc_irq_chip, &gsc->irq_chip_data); > + if (ret) > + goto err_regmap; > + > + dev_info(dev, "Gateworks System Controller v%d: fw 0x%04x\n", > + gsc->fwver, gsc->fwcrc); > + > + ret = sysfs_create_group(&dev->kobj, &attr_group); > + if (ret) > + dev_err(dev, "failed to create sysfs attrs\n"); > + > + ret = devm_of_platform_populate(dev); > + if (ret) > + goto err_sysfs; > + > + return 0; > + > +err_sysfs: > + sysfs_remove_group(&dev->kobj, &attr_group); > +err_regmap: > + i2c_unregister_device(gsc->i2c_hwmon); If you use devm_* you can omit this line. > + return ret; > +} > + > +static int gsc_remove(struct i2c_client *client) > +{ > + struct gsc_dev *gsc = i2c_get_clientdata(client); > + > + sysfs_remove_group(&client->dev.kobj, &attr_group); > + i2c_unregister_device(gsc->i2c_hwmon); If you use devm_* you can omit this line. > + return 0; > +} > + > +static const struct i2c_device_id gsc_id_table[] = { > + {"gsc", GSC_MISC }, Space after the '{'. > + { /* sentinel */ } No need for this comment. > +}; > +MODULE_DEVICE_TABLE(i2c, gsc_id_table); > + > +static struct i2c_driver gsc_driver = { > + .driver = { > + .name = "gateworks-gsc", > + .of_match_table = gsc_of_match, > + }, > + .id_table = gsc_id_table, > + .probe_new = gsc_probe, > + .remove = gsc_remove, > +}; > + Nit: Superfluous '\n'. > +module_i2c_driver(gsc_driver); > + > +MODULE_AUTHOR("Tim Harvey <tharvey@xxxxxxxxxxxxx>"); > +MODULE_DESCRIPTION("I2C Core interface for GSC"); > +MODULE_LICENSE("GPL v2"); > diff --git a/include/linux/mfd/gsc.h b/include/linux/mfd/gsc.h > new file mode 100644 > index 00000000..fc33647 > --- /dev/null > +++ b/include/linux/mfd/gsc.h > @@ -0,0 +1,73 @@ > +/* SPDX-License-Identifier: GPL-2.0 > + * > + * Copyright (C) 2020 Gateworks Corporation > + */ > +#ifndef __LINUX_MFD_GSC_H_ > +#define __LINUX_MFD_GSC_H_ > + > +#include <linux/regmap.h> > + > +/* Device Addresses */ > +#define GSC_MISC 0x20 > +#define GSC_UPDATE 0x21 > +#define GSC_GPIO 0x23 > +#define GSC_HWMON 0x29 > +#define GSC_EEPROM0 0x50 > +#define GSC_EEPROM1 0x51 > +#define GSC_EEPROM2 0x52 > +#define GSC_EEPROM3 0x53 > +#define GSC_RTC 0x68 > + > +/* Register offsets */ > +#define GSC_CTRL_0 0x00 > +#define GSC_CTRL_1 0x01 > +#define GSC_TIME 0x02 > +#define GSC_TIME_ADD 0x06 > +#define GSC_IRQ_STATUS 0x0A > +#define GSC_IRQ_ENABLE 0x0B > +#define GSC_FW_CRC 0x0C > +#define GSC_FW_VER 0x0E > +#define GSC_WP 0x0F > + > +/* Bit definitions */ > +#define GSC_CTRL_0_PB_HARD_RESET 0 > +#define GSC_CTRL_0_PB_CLEAR_SECURE_KEY 1 > +#define GSC_CTRL_0_PB_SOFT_POWER_DOWN 2 > +#define GSC_CTRL_0_PB_BOOT_ALTERNATE 3 > +#define GSC_CTRL_0_PERFORM_CRC 4 > +#define GSC_CTRL_0_TAMPER_DETECT 5 > +#define GSC_CTRL_0_SWITCH_HOLD 6 > + > +#define GSC_CTRL_1_SLEEP_ENABLE 0 > +#define GSC_CTRL_1_ACTIVATE_SLEEP 1 > +#define GSC_CTRL_1_LATCH_SLEEP_ADD 2 > +#define GSC_CTRL_1_SLEEP_NOWAKEPB 3 > +#define GSC_CTRL_1_WDT_TIME 4 > +#define GSC_CTRL_1_WDT_ENABLE 5 > +#define GSC_CTRL_1_SWITCH_BOOT_ENABLE 6 > +#define GSC_CTRL_1_SWITCH_BOOT_CLEAR 7 > + > +#define GSC_IRQ_PB 0 > +#define GSC_IRQ_KEY_ERASED 1 > +#define GSC_IRQ_EEPROM_WP 2 > +#define GSC_IRQ_RESV 3 > +#define GSC_IRQ_GPIO 4 > +#define GSC_IRQ_TAMPER 5 > +#define GSC_IRQ_WDT_TIMEOUT 6 > +#define GSC_IRQ_SWITCH_HOLD 7 > + > +struct gsc_dev { > + struct device *dev; > + > + struct i2c_client *i2c; /* 0x20: interrupt controller, WDT */ > + struct i2c_client *i2c_hwmon; /* 0x29: hwmon, fan controller */ > + > + struct regmap *regmap; > + struct regmap_bus bus; > + struct regmap_irq_chip_data *irq_chip_data; What is this used by? > + > + unsigned int fwver; > + unsigned short fwcrc; > +}; Please ensure all of these properties are used outside of .probe(). If they're not, please remove them. > +#endif /* __LINUX_MFD_GSC_H_ */ -- Lee Jones [李琼斯] Linaro Services Technical Lead Linaro.org │ Open source software for ARM SoCs Follow Linaro: Facebook | Twitter | Blog
