[v8,2/3] mfd: add Gateworks System Controller core driver

Message ID	1585341214-25285-3-git-send-email-tharvey@gateworks.com (mailing list archive)
State	Not Applicable
Headers	show Return-Path: <SRS0=0/BK=5M=vger.kernel.org=linux-hwmon-owner@kernel.org> From: Tim Harvey <tharvey@gateworks.com> To: Lee Jones <lee.jones@linaro.org>, Jean Delvare <jdelvare@suse.com>, Guenter Roeck <linux@roeck-us.net>, linux-hwmon@vger.kernel.org, Rob Herring <robh+dt@kernel.org>, Frank Rowand <frowand.list@gmail.com>, devicetree@vger.kernel.org, linux-kernel@vger.kernel.org, Robert Jones <rjones@gateworks.com> Cc: Tim Harvey <tharvey@gateworks.com>, Randy Dunlap <rdunlap@infradead.org> Subject: [PATCH v8 2/3] mfd: add Gateworks System Controller core driver Date: Fri, 27 Mar 2020 13:33:33 -0700 Message-Id: <1585341214-25285-3-git-send-email-tharvey@gateworks.com> In-Reply-To: <1585341214-25285-1-git-send-email-tharvey@gateworks.com> References: <1585341214-25285-1-git-send-email-tharvey@gateworks.com> Sender: linux-hwmon-owner@vger.kernel.org Precedence: bulk
Series	Add support for the Gateworks System Controller \| expand [v8,0/3] Add support for the Gateworks System Controller [v8,1/3] dt-bindings: mfd: Add Gateworks System Controller bindings [v8,2/3] mfd: add Gateworks System Controller core driver [v8,3/3] hwmon: add Gateworks System Controller support

Tim Harvey March 27, 2020, 8:33 p.m. UTC

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@infradead.org>
Signed-off-by: Tim Harvey <tharvey@gateworks.com>
---
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

Tim Harvey April 8, 2020, 3:36 p.m. UTC | #1

On Fri, Mar 27, 2020 at 1:33 PM Tim Harvey <tharvey@gateworks.com> 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@infradead.org>
> Signed-off-by: Tim Harvey <tharvey@gateworks.com>
> ---
> 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@gateworks.com>
> +M:     Robert Jones <rjones@gateworks.com>
> +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@chromium.org>
>  R:     Emese Revfy <re.emese@gmail.com>
> 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.
> +
>  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.
> + *
> + * Copyright (C) 2020 Gateworks Corporation
> + */
> +#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;
> +
> +       ret = regmap_bulk_write(gsc->regmap, GSC_TIME_ADD, regs, 4);
> +       if (ret)
> +               return ret;
> +
> +       regs[0] = 1 << GSC_CTRL_1_LATCH_SLEEP_ADD;
> +
> +       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);
> +
> +       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,
> +};
> +
> +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;
> +       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, &reg))
> +               return -EIO;
> +       gsc->fwver = reg;
> +
> +       regmap_read(gsc->regmap, GSC_FW_CRC, &reg);
> +       gsc->fwcrc = reg;
> +       regmap_read(gsc->regmap, GSC_FW_CRC + 1, &reg);
> +       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);
> +
> +       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);
> +
> +       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);
> +
> +       return 0;
> +}
> +
> +static const struct i2c_device_id gsc_id_table[] = {
> +       {"gsc", GSC_MISC },
> +       { /* sentinel */ }
> +};
> +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,
> +};
> +
> +module_i2c_driver(gsc_driver);
> +
> +MODULE_AUTHOR("Tim Harvey <tharvey@gateworks.com>");
> +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;
> +
> +       unsigned int fwver;
> +       unsigned short fwcrc;
> +};
> +
> +#endif /* __LINUX_MFD_GSC_H_ */
> --
> 2.7.4
>

Lee,

Do you have time to look at this? Rob has reviewed the dt-bindings and
Guenter has reviewed the hwmon driver so this is the only patch
remaining in the series that needs review/sign-off.

Best Regards,

Tim

Lee Jones April 14, 2020, 8:09 a.m. UTC | #2

On Wed, 08 Apr 2020, Tim Harvey wrote:

> On Fri, Mar 27, 2020 at 1:33 PM Tim Harvey <tharvey@gateworks.com> 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@infradead.org>
> > Signed-off-by: Tim Harvey <tharvey@gateworks.com>
> > ---
> >  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

[...]

Please clip your responses.

> Lee,
> 
> Do you have time to look at this? Rob has reviewed the dt-bindings and
> Guenter has reviewed the hwmon driver so this is the only patch
> remaining in the series that needs review/sign-off.

It's on the list.

Please bear in mind that the merge-window has been open for the past 2
weeks.  Usually reviewing cadence slows down during this period.  It
should be ramping up again this week.

Lee Jones April 28, 2020, 9:44 a.m. UTC | #3

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@infradead.org>
> Signed-off-by: Tim Harvey <tharvey@gateworks.com>
> ---
> 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@gateworks.com>
> +M:	Robert Jones <rjones@gateworks.com>
> +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@chromium.org>
>  R:	Emese Revfy <re.emese@gmail.com>
> 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, &reg))
> +		return -EIO;
> +	gsc->fwver = reg;
> +
> +	regmap_read(gsc->regmap, GSC_FW_CRC, &reg);
> +	gsc->fwcrc = reg;
> +	regmap_read(gsc->regmap, GSC_FW_CRC + 1, &reg);
> +	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@gateworks.com>");
> +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_ */

Tim Harvey April 28, 2020, 7:21 p.m. UTC | #4

On Tue, Apr 28, 2020 at 2:44 AM Lee Jones <lee.jones@linaro.org> wrote:
>
<snip>
> > +
> > +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?

Lee,

Thanks for the review!

I need the remap_bus* for devm_regmap_init() in the hwmon sub-module driver:

hwmon->regmap = devm_regmap_init(dev, &gsc->bus, gsc->i2c_hwmon,
&gsc_hwmon_regmap_config);

Is there something easier I'm missing?

Thanks,

Tim

Lee Jones April 29, 2020, 6:33 a.m. UTC | #5

On Tue, 28 Apr 2020, Tim Harvey wrote:

> On Tue, Apr 28, 2020 at 2:44 AM Lee Jones <lee.jones@linaro.org> wrote:
> >
> <snip>
> > > +
> > > +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?
> 
> Lee,
> 
> Thanks for the review!
> 
> I need the remap_bus* for devm_regmap_init() in the hwmon sub-module driver:
> 
> hwmon->regmap = devm_regmap_init(dev, &gsc->bus, gsc->i2c_hwmon,
> &gsc_hwmon_regmap_config);
> 
> Is there something easier I'm missing?

This is an odd setup.  I haven't seen one driver registering another
driver's Regmap call-backs before, related or otherwise.  Normally the
Regmap is setup (initialised) in the parent driver and child drivers
just make use of it.  Here it looks like you are registering 2
separate Regmaps, but using the same call-backs for both, which seems
wrong to me.

Tim Harvey April 29, 2020, 2:50 p.m. UTC | #6

On Tue, Apr 28, 2020 at 11:33 PM Lee Jones <lee.jones@linaro.org> wrote:
>
> On Tue, 28 Apr 2020, Tim Harvey wrote:
>
> > On Tue, Apr 28, 2020 at 2:44 AM Lee Jones <lee.jones@linaro.org> wrote:
> > >
> > <snip>
> > > > +
> > > > +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?
> >
> > Lee,
> >
> > Thanks for the review!
> >
> > I need the remap_bus* for devm_regmap_init() in the hwmon sub-module driver:
> >
> > hwmon->regmap = devm_regmap_init(dev, &gsc->bus, gsc->i2c_hwmon,
> > &gsc_hwmon_regmap_config);
> >
> > Is there something easier I'm missing?
>
> This is an odd setup.  I haven't seen one driver registering another
> driver's Regmap call-backs before, related or otherwise.  Normally the
> Regmap is setup (initialised) in the parent driver and child drivers
> just make use of it.  Here it looks like you are registering 2
> separate Regmaps, but using the same call-backs for both, which seems
> wrong to me.
>

Lee,

It is perhaps an odd setup. The hwmon sub-device is at a different i2c
slave address than the other sub-devices. The same callbacks are used
for reg read/write to take advantage of the retries due to the errata
resulting in occasional NAK'd register reads.

Tim

Lee Jones April 30, 2020, 8:28 a.m. UTC | #7

On Wed, 29 Apr 2020, Tim Harvey wrote:

> On Tue, Apr 28, 2020 at 11:33 PM Lee Jones <lee.jones@linaro.org> wrote:
> >
> > On Tue, 28 Apr 2020, Tim Harvey wrote:
> >
> > > On Tue, Apr 28, 2020 at 2:44 AM Lee Jones <lee.jones@linaro.org> wrote:
> > > >
> > > <snip>
> > > > > +
> > > > > +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?
> > >
> > > Lee,
> > >
> > > Thanks for the review!
> > >
> > > I need the remap_bus* for devm_regmap_init() in the hwmon sub-module driver:
> > >
> > > hwmon->regmap = devm_regmap_init(dev, &gsc->bus, gsc->i2c_hwmon,
> > > &gsc_hwmon_regmap_config);
> > >
> > > Is there something easier I'm missing?
> >
> > This is an odd setup.  I haven't seen one driver registering another
> > driver's Regmap call-backs before, related or otherwise.  Normally the
> > Regmap is setup (initialised) in the parent driver and child drivers
> > just make use of it.  Here it looks like you are registering 2
> > separate Regmaps, but using the same call-backs for both, which seems
> > wrong to me.
> >
> 
> Lee,
> 
> It is perhaps an odd setup. The hwmon sub-device is at a different i2c
> slave address than the other sub-devices. The same callbacks are used
> for reg read/write to take advantage of the retries due to the errata
> resulting in occasional NAK'd register reads.

Then I suggest putting them somewhere shared or exporting them.

Passing pointers to the via ddata sounds a bit batty.

[v8,2/3] mfd: add Gateworks System Controller core driver

Commit Message

Comments

Patch