Re: [PATCH] rtc: ds1307: add trickle charger device tree binding

Jason Cooper
2014-08-28T17:29:01Z

On Thu, Aug 28, 2014 at 09:48:25AM -0700, Guenter Roeck wrote:
> On Thu, Aug 28, 2014 at 05:10:25PM +0100, Mark Rutland wrote:
> > On Thu, Aug 28, 2014 at 04:51:57PM +0100, Guenter Roeck wrote:
> > > On Thu, Aug 28, 2014 at 01:59:15PM +0100, Mark Rutland wrote:
> > > > On Thu, Aug 28, 2014 at 01:42:44PM +0100, Matti Vaittinen wrote:
> > > > > Patch adding support for specifying trickle charger setup from device
> > > > > tree. Patch is based on linux-next tree.
> > > > >
> > > > >
> > > > > Some DS13XX devices have "trickle chargers". Introduce a device tree binding
> > > > > for specifying the setup and register values.
> > > > >
> > > > > Signed-off-by: Matti Vaittinen <matti.vaittinen@nsn.com>
> > > > > ---
> > > > > .../devicetree/bindings/i2c/trivial-devices.txt | 1 -
> > > > > .../devicetree/bindings/rtc/dallas,ds1339.txt | 19 ++++++++++
> > > > > drivers/rtc/rtc-ds1307.c | 44 ++++++++++++++++++++--
> > > > > 3 files changed, 59 insertions(+), 5 deletions(-)
> > > > > create mode 100644 Documentation/devicetree/bindings/rtc/dallas,ds1339.txt
> > > > >
> > > > > diff --git a/Documentation/devicetree/bindings/i2c/trivial-devices.txt b/Documentation/devicetree/bindings/i2c/trivial-devices.txt
> > > > > index 6af570e..e9206a4 100644
> > > > > --- a/Documentation/devicetree/bindings/i2c/trivial-devices.txt
> > > > > +++ b/Documentation/devicetree/bindings/i2c/trivial-devices.txt
> > > > > @@ -35,7 +35,6 @@ catalyst,24c32 i2c serial eeprom
> > > > > cirrus,cs42l51 Cirrus Logic CS42L51 audio codec
> > > > > dallas,ds1307 64 x 8, Serial, I2C Real-Time Clock
> > > > > dallas,ds1338 I2C RTC with 56-Byte NV RAM
> > > > > -dallas,ds1339 I2C Serial Real-Time Clock
> > > > > dallas,ds1340 I2C RTC with Trickle Charger
> > > > > dallas,ds1374 I2C, 32-Bit Binary Counter Watchdog RTC with Trickle Charger and Reset Input/Output
> > > > > dallas,ds1631 High-Precision Digital Thermometer
> > > > > diff --git a/Documentation/devicetree/bindings/rtc/dallas,ds1339.txt b/Documentation/devicetree/bindings/rtc/dallas,ds1339.txt
> > > > > new file mode 100644
> > > > > index 0000000..9faf40e
> > > > > --- /dev/null
> > > > > +++ b/Documentation/devicetree/bindings/rtc/dallas,ds1339.txt
> > > > > @@ -0,0 +1,19 @@
> > > > > +* Dallas DS1339 I2C Serial Real-Time Clock
> > > > > +
> > > > > +Required properties:
> > > > > +- compatible: Should contain "dallas,ds1339".
> > > > > +- reg: I2C address for chip
> > > > > +
> > > > > +Optional properties:
> > > > > +- trickle_setup : Used Trickle Charger configuration,
> > > > > + corresponding to 5 lowest bits in trickle charger register.
> > > >
> > > The value is provided via platform data, so it is platform specific
> > > and presumably needs to be configurable. I did, however, not find
> > > a single in-kernel driver which is actually setting it.
> > > So this is a good question.
> >
> > I'm uncomfortable adding a field we don't understand to DT.
> >
> > Is there any publicly-available documentation for the device?
> >
>
> Lots ;-)
>
> http://datasheets.maximintegrated.com/en/ds/DS1307.pdf
> http://datasheets.maximintegrated.com/en/ds/DS1337.pdf
> http://datasheets.maximintegrated.com/en/ds/DS1338.pdf
> http://datasheets.maximintegrated.com/en/ds/DS1339.pdf
> http://datasheets.maximintegrated.com/en/ds/DS1340.pdf
> http://datasheets.maximintegrated.com/en/ds/DS1388.pdf
> http://datasheets.maximintegrated.com/en/ds/DS3231.pdf
>
> Code suggests that DS1339, DS1339, and DS1340 have the register.
>
> Looking into the datasheets, the configuration consists of two parts:
> - diode connected or not
> - trickle charger resistor value (250, 2000, or 4000 ohm)
>
> Given that, it seems to me that those values should be configured
> explicitly instead of using a binary value, and that the driver
> should perform the conversion from dt entry to register value.

iiuc, there is no way for the kernel to determine what is being trickle
charged, and thus no way to determine how it should set this register.

It may be a bit of overkill, but I think a DT macro would be the most
maintainable solution here:

#define DS1339_TRCKL_DIODE 0x08
#define DS1339_TRCKL_NODIODE 0x04
#define DS1339_TRCKL_R250 0x01
#define DS1339_TRCKL_R2000 0x02
#define DS1339_TRCKL_R4000 0x03

trickle_setup = <DS1339_TRCKL_DIODE | DS1339_TRCKL_R250>;

And the driver would take care of oring it with the enable pattern.

thx,

Jason.