Message ID | 1492468810-17224-1-git-send-email-mdf@kernel.org |
---|---|
State | Changes Requested, archived |
Headers | show |
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>; > + }; -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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
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 -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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.
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 --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;
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