Linux-Hwmon Archive on lore.kernel.org
 help / color / Atom feed
From: Tim Harvey <tharvey@gateworks.com>
To: Lee Jones <lee.jones@linaro.org>
Cc: Jean Delvare <jdelvare@suse.com>,
	Guenter Roeck <linux@roeck-us.net>,
	Linux HWMON List <linux-hwmon@vger.kernel.org>,
	Rob Herring <robh+dt@kernel.org>,
	Frank Rowand <frowand.list@gmail.com>,
	Device Tree Mailing List <devicetree@vger.kernel.org>,
	open list <linux-kernel@vger.kernel.org>,
	Robert Jones <rjones@gateworks.com>,
	Randy Dunlap <rdunlap@infradead.org>
Subject: Re: [PATCH v7 2/3] mfd: add Gateworks System Controller core driver
Date: Thu, 26 Mar 2020 14:55:54 -0700
Message-ID: <CAJ+vNU0pPW3n6dnUH4n_3ZtmwGYdtc6gwUkg+aWJYPF7bFUXzQ@mail.gmail.com> (raw)
In-Reply-To: <20200326101702.GD603801@dell>

On Thu, Mar 26, 2020 at 3:16 AM Lee Jones <lee.jones@linaro.org> 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, &regmap_gsc, 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;
>
> 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, &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);
> > +
> > +     gsc->regmap_hwmon = devm_regmap_init(dev, &regmap_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

  reply index

Thread overview: 8+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-03-20 20:35 [PATCH v7 0/3] Add support for the Gateworks System Controller Tim Harvey
2020-03-20 20:35 ` [PATCH v7 1/3] dt-bindings: mfd: Add Gateworks System Controller bindings Tim Harvey
2020-03-26 19:13   ` Rob Herring
2020-03-20 20:35 ` [PATCH v7 2/3] mfd: add Gateworks System Controller core driver Tim Harvey
2020-03-26 10:17   ` Lee Jones
2020-03-26 21:55     ` Tim Harvey [this message]
2020-03-20 20:35 ` [PATCH v7 3/3] hwmon: add Gateworks System Controller support Tim Harvey
2020-03-22 21:03   ` Guenter Roeck

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=CAJ+vNU0pPW3n6dnUH4n_3ZtmwGYdtc6gwUkg+aWJYPF7bFUXzQ@mail.gmail.com \
    --to=tharvey@gateworks.com \
    --cc=devicetree@vger.kernel.org \
    --cc=frowand.list@gmail.com \
    --cc=jdelvare@suse.com \
    --cc=lee.jones@linaro.org \
    --cc=linux-hwmon@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux@roeck-us.net \
    --cc=rdunlap@infradead.org \
    --cc=rjones@gateworks.com \
    --cc=robh+dt@kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-Hwmon Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-hwmon/0 linux-hwmon/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-hwmon linux-hwmon/ https://lore.kernel.org/linux-hwmon \
		linux-hwmon@vger.kernel.org
	public-inbox-index linux-hwmon

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-hwmon


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git