Re: [PATCH v7 2/3] mfd: add Gateworks System Controller core driver
From: Tim Harvey
Date: Thu Mar 26 2020 - 17:56:10 EST
On Thu, Mar 26, 2020 at 3:16 AM Lee Jones <lee.jones@xxxxxxxxxx> wrote:
>
> On Fri, 20 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
> >
<snip>
> > 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 describe which sub-devices are attached.
Hi Lee, thanks for the review!
I will add more description for v8:
Enable common support for the Gateworks System Controller
found on Gateworks Single Board Computers. This device supports
an interrupt controller providing interrupts for pushbutton
events and a sub-device for HWMON and FAN controller via the
gsc-hwmon driver.
>
> > 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..8566123
> > --- /dev/null
> > +++ b/drivers/mfd/gateworks-gsc.c
> > @@ -0,0 +1,291 @@
> > +// 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.
>
> When are you planning on adding support for the other devices?
The pushbutton monitoring is supported via interrupts and the ADC's
and fan controller are supported via the gsc-hwmon driver in this
series. The watchdog is not yet supported via a driver and I'm not
sure if/when I will submit one as the API is rather strange.
>
<snip>
> > +/*
> > + * 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];
>
> No error checking? Could be down for a very long time.
understood, but I'm not sure I want to dictate what the max amount of
time to power-down should be. Some remote-sensing use cases may want
to power-down for days, or weeks. I agree powering down for years
seems strange but again, I don't want to dictate usage. If they
accidently power down for a length of time they did not mean to they
still have to remove power, battery, or press a button to power back
on.
>
<snip>
>
> > +gsc_probe(struct i2c_client *client, const struct i2c_device_id *id)
> > +{
> > + 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->regmap = devm_regmap_init(dev, ®map_gsc, 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;
>
> You not checking this for a valid value?
it's a single byte unsigned char, there really isn't an invalid value.
>
> > + 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);
> > +
> > + gsc->regmap_hwmon = devm_regmap_init(dev, ®map_gsc, gsc->i2c_hwmon,
> > + &gsc_regmap_hwmon_config);
> > + if (IS_ERR(gsc->regmap_hwmon)) {
> > + ret = PTR_ERR(gsc->regmap_hwmon);
> > + dev_err(dev, "failed to allocate register map: %d\n", ret);
> > + goto err_regmap;
> > + }
>
> Why can't the HWMON driver fetch its own Regmap?
I can move this to hwmon. It means I have to share the regmap_bus in
struct gsc_dev instead of sharing the regmap but I suppose it makes
more sense to have hwmon register it's own.
Everything else you suggest makes complete sense and I will put into v8.
Best Regards,
Tim