diff mbox

rtc: ds1374: Add trickle charger device tree binding

Message ID 1492468810-17224-1-git-send-email-mdf@kernel.org
State Rejected
Headers show

Commit Message

Moritz Fischer April 17, 2017, 10:40 p.m. UTC 
Introduce a device tree binding for specifying the trickle charger
configuration for ds1374. This is based on the code for ds13390.

Signed-off-by: Moritz Fischer <mdf@kernel.org>
---
 .../devicetree/bindings/rtc/dallas,ds1374.txt      | 18 ++++++++
 drivers/rtc/rtc-ds1374.c                           | 54 ++++++++++++++++++++++
 2 files changed, 72 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/rtc/dallas,ds1374.txt

Comments

Rob Herring April 20, 2017, 3:56 p.m. UTC | #1 
On Mon, Apr 17, 2017 at 03:40:10PM -0700, Moritz Fischer wrote:
> Introduce a device tree binding for specifying the trickle charger
> configuration for ds1374. This is based on the code for ds13390.
> 
> Signed-off-by: Moritz Fischer <mdf@kernel.org>
> ---
>  .../devicetree/bindings/rtc/dallas,ds1374.txt      | 18 ++++++++
>  drivers/rtc/rtc-ds1374.c                           | 54 ++++++++++++++++++++++
>  2 files changed, 72 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/rtc/dallas,ds1374.txt
> 
> diff --git a/Documentation/devicetree/bindings/rtc/dallas,ds1374.txt b/Documentation/devicetree/bindings/rtc/dallas,ds1374.txt
> new file mode 100644
> index 0000000..4cf5bd7
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/rtc/dallas,ds1374.txt
> @@ -0,0 +1,18 @@
> +* Dallas DS1374		I2C Real-Time Clock / WDT

Please remove from trivial-devices.txt, too. (which is moving in 4.12 
BTW)

> +
> +Required properties:
> +- compatible: Should contain "dallas,ds1374".
> +- reg: I2C address for chip
> +
> +Optional properties:
> +- trickle-resistor-ohms : Selected resistor for trickle charger
> +	Values usable for ds1374 are 250, 2000, 4000
> +	Should be given if trickle charger should be enabled
> +- trickle-diode-disable : Do not use internal trickle charger diode
> +	Should be given if internal trickle charger diode should be disabled

These should have vendor prefix unless you think they are common.

> +Example:
> +	ds1374: rtc@0 {
> +		compatible = "dallas,ds1374";
> +		trickle-resistor-ohms = <250>;
> +		reg = <0>;
> +	};
Moritz Fischer April 20, 2017, 5:25 p.m. UTC | #2 
On Thu, Apr 20, 2017 at 10:56:34AM -0500, Rob Herring wrote:
> On Mon, Apr 17, 2017 at 03:40:10PM -0700, Moritz Fischer wrote:
> > Introduce a device tree binding for specifying the trickle charger
> > configuration for ds1374. This is based on the code for ds13390.
> > 
> > Signed-off-by: Moritz Fischer <mdf@kernel.org>
> > ---
> >  .../devicetree/bindings/rtc/dallas,ds1374.txt      | 18 ++++++++
> >  drivers/rtc/rtc-ds1374.c                           | 54 ++++++++++++++++++++++
> >  2 files changed, 72 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/rtc/dallas,ds1374.txt
> > 
> > diff --git a/Documentation/devicetree/bindings/rtc/dallas,ds1374.txt b/Documentation/devicetree/bindings/rtc/dallas,ds1374.txt
> > new file mode 100644
> > index 0000000..4cf5bd7
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/rtc/dallas,ds1374.txt
> > @@ -0,0 +1,18 @@
> > +* Dallas DS1374		I2C Real-Time Clock / WDT
> 
> Please remove from trivial-devices.txt, too. (which is moving in 4.12 
> BTW)

Ok, I'll redo this on top of b7e252fcddfa573bb1ee275b53bba6cef85671d4
(Documentation: devicetree: move trivial-devices out of I2C realm) then.

> 
> > +
> > +Required properties:
> > +- compatible: Should contain "dallas,ds1374".
> > +- reg: I2C address for chip
> > +
> > +Optional properties:
> > +- trickle-resistor-ohms : Selected resistor for trickle charger
> > +	Values usable for ds1374 are 250, 2000, 4000
> > +	Should be given if trickle charger should be enabled
> > +- trickle-diode-disable : Do not use internal trickle charger diode
> > +	Should be given if internal trickle charger diode should be disabled
> 
> These should have vendor prefix unless you think they are common.

Well works at least for maxim, dallas & different models like
ds1390, ds1374, so I figured I'd keep the bindings the same.

> 
> > +Example:
> > +	ds1374: rtc@0 {
> > +		compatible = "dallas,ds1374";
> > +		trickle-resistor-ohms = <250>;
> > +		reg = <0>;
> > +	};

Thanks,

Moritz
Moritz Fischer April 22, 2017, 5:54 p.m. UTC | #3 
On Thu, Apr 20, 2017 at 10:25 AM, Moritz Fischer <mdf@kernel.org> wrote:
> On Thu, Apr 20, 2017 at 10:56:34AM -0500, Rob Herring wrote:
>> On Mon, Apr 17, 2017 at 03:40:10PM -0700, Moritz Fischer wrote:
>> > Introduce a device tree binding for specifying the trickle charger
>> > configuration for ds1374. This is based on the code for ds13390.
>> >
>> > Signed-off-by: Moritz Fischer <mdf@kernel.org>
>> > ---
>> >  .../devicetree/bindings/rtc/dallas,ds1374.txt      | 18 ++++++++
>> >  drivers/rtc/rtc-ds1374.c                           | 54 ++++++++++++++++++++++
>> >  2 files changed, 72 insertions(+)
>> >  create mode 100644 Documentation/devicetree/bindings/rtc/dallas,ds1374.txt
>> >
>> > diff --git a/Documentation/devicetree/bindings/rtc/dallas,ds1374.txt b/Documentation/devicetree/bindings/rtc/dallas,ds1374.txt
>> > new file mode 100644
>> > index 0000000..4cf5bd7
>> > --- /dev/null
>> > +++ b/Documentation/devicetree/bindings/rtc/dallas,ds1374.txt
>> > @@ -0,0 +1,18 @@
>> > +* Dallas DS1374            I2C Real-Time Clock / WDT
>>
>> Please remove from trivial-devices.txt, too. (which is moving in 4.12
>> BTW)
>
> Ok, I'll redo this on top of b7e252fcddfa573bb1ee275b53bba6cef85671d4
> (Documentation: devicetree: move trivial-devices out of I2C realm) then.

Follow up question, right now one selects between WDT and ALARM mode with
a CONFIG_RTC_DRV_DS1374_WDT=y statically at compile time.

I'd like to add a 'dallas,mode = <DS1374_WDT>;' or
dallas,enable-watchdog property
to the binding, same goes for the ability to remap the WDT reset output to the
interrupt pin (which is currently not supported, but my hardware needs this).

Would be the right way to add the remapping something like
'dallas,remap-reset-to-int' ?

Ideas? This change would obviously break people's setups where they
select one or
the other behavior via the build time option. Is that acceptable seen
that relying on
build time CONFIG_FOO seems like a bad assumption to begin with?

A bit of background: I currently started cleaning up a bunch of issues
in this driver,
like refactoring it to use the watchdog framework instead of open
coding everything,
make setting the timeout actually work (right now it the timeout to
tick conversion is
hosed).

Thanks,

Moritz
Alexandre Belloni April 24, 2017, 5:16 p.m. UTC | #4 
On 22/04/2017 at 10:54:23 -0700, Moritz Fischer wrote:
> On Thu, Apr 20, 2017 at 10:25 AM, Moritz Fischer <mdf@kernel.org> wrote:
> > On Thu, Apr 20, 2017 at 10:56:34AM -0500, Rob Herring wrote:
> >> On Mon, Apr 17, 2017 at 03:40:10PM -0700, Moritz Fischer wrote:
> >> > Introduce a device tree binding for specifying the trickle charger
> >> > configuration for ds1374. This is based on the code for ds13390.
> >> >
> >> > Signed-off-by: Moritz Fischer <mdf@kernel.org>
> >> > ---
> >> >  .../devicetree/bindings/rtc/dallas,ds1374.txt      | 18 ++++++++
> >> >  drivers/rtc/rtc-ds1374.c                           | 54 ++++++++++++++++++++++
> >> >  2 files changed, 72 insertions(+)
> >> >  create mode 100644 Documentation/devicetree/bindings/rtc/dallas,ds1374.txt
> >> >
> >> > diff --git a/Documentation/devicetree/bindings/rtc/dallas,ds1374.txt b/Documentation/devicetree/bindings/rtc/dallas,ds1374.txt
> >> > new file mode 100644
> >> > index 0000000..4cf5bd7
> >> > --- /dev/null
> >> > +++ b/Documentation/devicetree/bindings/rtc/dallas,ds1374.txt
> >> > @@ -0,0 +1,18 @@
> >> > +* Dallas DS1374            I2C Real-Time Clock / WDT
> >>
> >> Please remove from trivial-devices.txt, too. (which is moving in 4.12
> >> BTW)
> >
> > Ok, I'll redo this on top of b7e252fcddfa573bb1ee275b53bba6cef85671d4
> > (Documentation: devicetree: move trivial-devices out of I2C realm) then.
> 
> Follow up question, right now one selects between WDT and ALARM mode with
> a CONFIG_RTC_DRV_DS1374_WDT=y statically at compile time.
> 
> I'd like to add a 'dallas,mode = <DS1374_WDT>;' or
> dallas,enable-watchdog property
> to the binding, same goes for the ability to remap the WDT reset output to the
> interrupt pin (which is currently not supported, but my hardware needs this).
> 
> Would be the right way to add the remapping something like
> 'dallas,remap-reset-to-int' ?
> 
> Ideas? This change would obviously break people's setups where they
> select one or
> the other behavior via the build time option. Is that acceptable seen
> that relying on
> build time CONFIG_FOO seems like a bad assumption to begin with?
> 
> A bit of background: I currently started cleaning up a bunch of issues
> in this driver,
> like refactoring it to use the watchdog framework instead of open
> coding everything,
> make setting the timeout actually work (right now it the timeout to
> tick conversion is
> hosed).
> 

I think I would do something with MFD as you were suggesting on IRC. You
can have a look at the flexcom driver (atmel-flexcom.c) which is simply
selecting a mode and then using whatever IP driver already existed
(UART, I2C or SPI).

I didn't have a close look but maybe that fits. Then, as you are
concerned wtih backward compatibility, you could enforce the mode from
the MFD driver, depending on the CONFIG_RTC_DRV_DS1374_WDT value.
Moritz Fischer April 24, 2017, 9:28 p.m. UTC | #5 
Hi Alex,

On Mon, Apr 24, 2017 at 07:16:39PM +0200, Alexandre Belloni wrote:

> I think I would do something with MFD as you were suggesting on IRC. You
> can have a look at the flexcom driver (atmel-flexcom.c) which is simply
> selecting a mode and then using whatever IP driver already existed
> (UART, I2C or SPI).

That's an interesting approach, so the idea is to basically use the MFD
to switch an existing driver one way or another via pdata? That could work.

> 
> I didn't have a close look but maybe that fits. Then, as you are
> concerned wtih backward compatibility, you could enforce the mode from
> the MFD driver, depending on the CONFIG_RTC_DRV_DS1374_WDT value.

This is where I'm currently at:

https://git.kernel.org/pub/scm/linux/kernel/git/mdf/linux.git/log/?h=wip/mfd-ds1374-rfc

Still a WIP, maybe can get out a patchset by the end of the week ...

Thanks,
Moritz
diff mbox

Patch

diff --git a/Documentation/devicetree/bindings/rtc/dallas,ds1374.txt b/Documentation/devicetree/bindings/rtc/dallas,ds1374.txt
new file mode 100644
index 0000000..4cf5bd7
--- /dev/null
+++ b/Documentation/devicetree/bindings/rtc/dallas,ds1374.txt
@@ -0,0 +1,18 @@ 
+* Dallas DS1374		I2C Real-Time Clock / WDT
+
+Required properties:
+- compatible: Should contain "dallas,ds1374".
+- reg: I2C address for chip
+
+Optional properties:
+- trickle-resistor-ohms : Selected resistor for trickle charger
+	Values usable for ds1374 are 250, 2000, 4000
+	Should be given if trickle charger should be enabled
+- trickle-diode-disable : Do not use internal trickle charger diode
+	Should be given if internal trickle charger diode should be disabled
+Example:
+	ds1374: rtc@0 {
+		compatible = "dallas,ds1374";
+		trickle-resistor-ohms = <250>;
+		reg = <0>;
+	};
diff --git a/drivers/rtc/rtc-ds1374.c b/drivers/rtc/rtc-ds1374.c
index 4cd115e..873475d 100644
--- a/drivers/rtc/rtc-ds1374.c
+++ b/drivers/rtc/rtc-ds1374.c
@@ -53,6 +53,13 @@ 
 #define DS1374_REG_SR_AF	0x01 /* Alarm Flag */
 #define DS1374_REG_TCR		0x09 /* Trickle Charge */
 
+#define DS1374_TRICKLE_CHARGER_ENABLE	0xA0
+#define DS1374_TRICKLE_CHARGER_250_OHM	0x01
+#define DS1374_TRICKLE_CHARGER_2K_OHM	0x02
+#define DS1374_TRICKLE_CHARGER_4K_OHM	0x03
+#define DS1374_TRICKLE_CHARGER_NO_DIODE	0x04
+#define DS1374_TRICKLE_CHARGER_DIODE	0x08
+
 static const struct i2c_device_id ds1374_id[] = {
 	{ "ds1374", 0 },
 	{ }
@@ -597,6 +604,49 @@  static struct notifier_block ds1374_wdt_notifier = {
 };
 
 #endif /*CONFIG_RTC_DRV_DS1374_WDT*/
+
+static int ds1374_trickle_of_init(struct i2c_client *client)
+{
+	u32 ohms = 0;
+	u8 value;
+	int ret;
+
+	if (of_property_read_u32(client->dev.of_node, "trickle-resistor-ohms",
+				 &ohms))
+		return 0;
+
+	/* Enable charger */
+	value = DS1374_TRICKLE_CHARGER_ENABLE;
+	if (of_property_read_bool(client->dev.of_node, "trickle-diode-disable"))
+		value |= DS1374_TRICKLE_CHARGER_NO_DIODE;
+	else
+		value |= DS1374_TRICKLE_CHARGER_DIODE;
+
+	/* Resistor select */
+	switch (ohms) {
+	case 250:
+		value |= DS1374_TRICKLE_CHARGER_250_OHM;
+		break;
+	case 2000:
+		value |= DS1374_TRICKLE_CHARGER_2K_OHM;
+		break;
+	case 4000:
+		value |= DS1374_TRICKLE_CHARGER_4K_OHM;
+		break;
+	default:
+		dev_warn(&client->dev,
+			 "Unsupported ohm value %02ux in dt\n", ohms);
+		return -EINVAL;
+	}
+	dev_dbg(&client->dev, "Trickle charge value is 0x%02x\n", value);
+
+	ret = i2c_smbus_write_byte_data(client, DS1374_REG_TCR, value);
+	if (ret < 0)
+		return ret;
+
+	return 0;
+}
+
 /*
  *****************************************************************************
  *
@@ -620,6 +670,10 @@  static int ds1374_probe(struct i2c_client *client,
 	INIT_WORK(&ds1374->work, ds1374_work);
 	mutex_init(&ds1374->mutex);
 
+	ret = ds1374_trickle_of_init(client);
+	if (ret)
+		return ret;
+
 	ret = ds1374_check_rtc_status(client);
 	if (ret)
 		return ret;