Re: [PATCH v3 2/4] mfd: add Gateworks System Controller core driver

From: Andrew Lunn <andrew@lunn.ch>
To: Tim Harvey <tharvey@gateworks.com>
Cc: Lee Jones <lee.jones@linaro.org>,
	Rob Herring <robh+dt@kernel.org>,
	Mark Rutland <mark.rutland@arm.com>,
	Mark Brown <broonie@kernel.org>,
	Dmitry Torokhov <dmitry.torokhov@gmail.com>,
	Wim Van Sebroeck <wim@iguana.be>,
	Guenter Roeck <linux@roeck-us.net>,
	linux-hwmon@vger.kernel.org, devicetree@vger.kernel.org,
	linux-watchdog@vger.kernel.org,
	Randy Dunlap <rdunlap@infradead.org>,
	linux-kernel@vger.kernel.org, linux-input@vger.kernel.org,
	linux-arm-kernel@lists.infradead.org
Subject: Re: [PATCH v3 2/4] mfd: add Gateworks System Controller core driver
Date: Tue, 3 Apr 2018 18:47:15 +0200	[thread overview]
Message-ID: <20180403164715.GA5676@lunn.ch> (raw)
In-Reply-To: <CAJ+vNU3-gaTcMAzo5wB6oSfE-JQC3NBVRJS5zFMTE-nVpD4F5g@mail.gmail.com>

On Tue, Apr 03, 2018 at 08:48:27AM -0700, Tim Harvey wrote:
> On Wed, Mar 28, 2018 at 8:14 AM, 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
> >
> <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];
> > +
> > +       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);
> > +
> > +       return ret;
> > +}
> 
> Any feedback on the 'powerdown' sysfs attribute that hooks to this
> function? This allows the GSC to disable the board primary power
> supply for 2^32 seconds and is often used to 'reset' the board
> although it could also be used to put the board in a power down state
> longer. I'm wondering if there is a more appropriate API for this in
> the kernel that I don't know about.

Hi Tim

RTC can cause wakeup when an alarm is set. It looks like the DS1672
does not have this. But you are emulating the DS1672 right? You could
add a second emulated RTC which does support an alarm? DS3232?

> I would also like to register a restart handler using this but I
> believe that ARM restart handlers currently can not use I2C - is that
> correct?

There are plenty which use GPIOs, or UARTs. Not seen any which use
i2c. What do you think does not work at this point?

     Andrew