* [PATCH] rtc: ds1307: add trickle charger device tree binding
@ 2014-08-28 12:42 Matti Vaittinen
2014-08-28 12:59 ` Mark Rutland
0 siblings, 1 reply; 22+ messages in thread
From: Matti Vaittinen @ 2014-08-28 12:42 UTC (permalink / raw)
To: a.zummo
Cc: jason, linux, jic23, arno, jgunthorpe, san, hs, devicetree,
linux-kernel, rtc-linux, Sverdlin Alexander, matti.vaittinen
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.
+- trickle_reg : Trickle charger register address. Defaults to 0x10 for ds1339.
+
+Example:
+ ds1339: rtc@68 {
+ compatible = "dallas,ds1339";
+ trickle_setup= <0x5>;
+ reg = <0x68>;
+ };
+
+
diff --git a/drivers/rtc/rtc-ds1307.c b/drivers/rtc/rtc-ds1307.c
index f03d5ba..f9b1244 100644
--- a/drivers/rtc/rtc-ds1307.c
+++ b/drivers/rtc/rtc-ds1307.c
@@ -126,9 +126,10 @@ struct chip_desc {
u16 nvram_offset;
u16 nvram_size;
u16 trickle_charger_reg;
+ u8 trickle_charger_setup;
};
-static const struct chip_desc chips[last_ds_type] = {
+static struct chip_desc chips[last_ds_type] = {
[ds_1307] = {
.nvram_offset = 8,
.nvram_size = 56,
@@ -835,13 +836,37 @@ ds1307_nvram_write(struct file *filp, struct kobject *kobj,
/*----------------------------------------------------------------------*/
+static void trickle_charger_of_init(struct chip_desc *chip,
+ struct device_node *node)
+{
+ int plen = 0;
+ const uint32_t *setup;
+ const uint32_t *reg;
+
+ setup = of_get_property(node, "trickle_setup" , &plen);
+ if (plen != 4)
+ goto out;
+ chip->trickle_charger_setup = (u8)*setup;
+ plen = 0;
+ reg = of_get_property(node, "trickle_reg" , &plen);
+ if (plen == 4)
+ chip->trickle_charger_reg = (u16)*reg;
+ return;
+out:
+ pr_debug("%s: No trickle charger data in dt for %s rtc chip\n",
+ __func__, (node && node->name)?node->name:"ds13XX");
+}
+
+/*----------------------------------------------------------------------*/
+
+
static int ds1307_probe(struct i2c_client *client,
const struct i2c_device_id *id)
{
struct ds1307 *ds1307;
int err = -ENODEV;
int tmp;
- const struct chip_desc *chip = &chips[id->driver_data];
+ struct chip_desc *chip = &chips[id->driver_data];
struct i2c_adapter *adapter = to_i2c_adapter(client->dev.parent);
bool want_irq = false;
unsigned char *buf;
@@ -866,9 +891,20 @@ static int ds1307_probe(struct i2c_client *client,
ds1307->client = client;
ds1307->type = id->driver_data;
- if (pdata && pdata->trickle_charger_setup && chip->trickle_charger_reg)
+ if (!pdata && client->dev.of_node)
+ trickle_charger_of_init(chip, client->dev.of_node);
+ /* Get trickle property */
+ else if (pdata && pdata->trickle_charger_setup)
+ chip->trickle_charger_setup = pdata->trickle_charger_setup;
+
+ if (chip->trickle_charger_setup && chip->trickle_charger_reg) {
+ dev_dbg(&client->dev, "writing trickle charger info 0x%x to 0x%x\n",
+ DS13XX_TRICKLE_CHARGER_MAGIC | chip->trickle_charger_setup,
+ chip->trickle_charger_reg);
i2c_smbus_write_byte_data(client, chip->trickle_charger_reg,
- DS13XX_TRICKLE_CHARGER_MAGIC | pdata->trickle_charger_setup);
+ DS13XX_TRICKLE_CHARGER_MAGIC |
+ chip->trickle_charger_setup);
+ }
buf = ds1307->regs;
if (i2c_check_functionality(adapter, I2C_FUNC_SMBUS_I2C_BLOCK)) {
--
1.8.3.1
--
=============================================
Matti Vaittinen
Senile SW Specialist
FINLAND
~~ When things go utterly wrong vim users can always type :help! ~~
^ permalink raw reply related [flat|nested] 22+ messages in thread
* Re: [PATCH] rtc: ds1307: add trickle charger device tree binding
2014-08-28 12:42 [PATCH] rtc: ds1307: add trickle charger device tree binding Matti Vaittinen
@ 2014-08-28 12:59 ` Mark Rutland
2014-08-28 15:51 ` Guenter Roeck
2014-08-29 7:41 ` Matti Vaittinen
0 siblings, 2 replies; 22+ messages in thread
From: Mark Rutland @ 2014-08-28 12:59 UTC (permalink / raw)
To: Matti Vaittinen
Cc: a.zummo, jason, linux, jic23, arno, jgunthorpe, san, hs,
devicetree, linux-kernel, rtc-linux, Sverdlin Alexander
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.
s/_/-/ in property names please.
What do these bits do? is there any documentation available?
Why do they need to be in the DT?
Why do we not have a higher-level description?
> +- trickle_reg : Trickle charger register address. Defaults to 0x10 for ds1339.
Is this expected to change? Why does this need to be in the DT?
> +
> +Example:
> + ds1339: rtc@68 {
> + compatible = "dallas,ds1339";
> + trickle_setup= <0x5>;
> + reg = <0x68>;
> + };
> +
> +
> diff --git a/drivers/rtc/rtc-ds1307.c b/drivers/rtc/rtc-ds1307.c
> index f03d5ba..f9b1244 100644
> --- a/drivers/rtc/rtc-ds1307.c
> +++ b/drivers/rtc/rtc-ds1307.c
> @@ -126,9 +126,10 @@ struct chip_desc {
> u16 nvram_offset;
> u16 nvram_size;
> u16 trickle_charger_reg;
> + u8 trickle_charger_setup;
> };
>
> -static const struct chip_desc chips[last_ds_type] = {
> +static struct chip_desc chips[last_ds_type] = {
> [ds_1307] = {
> .nvram_offset = 8,
> .nvram_size = 56,
> @@ -835,13 +836,37 @@ ds1307_nvram_write(struct file *filp, struct kobject *kobj,
>
> /*----------------------------------------------------------------------*/
>
> +static void trickle_charger_of_init(struct chip_desc *chip,
> + struct device_node *node)
> +{
> + int plen = 0;
> + const uint32_t *setup;
> + const uint32_t *reg;
> +
> + setup = of_get_property(node, "trickle_setup" , &plen);
> + if (plen != 4)
> + goto out;
> + chip->trickle_charger_setup = (u8)*setup;
This isn't endian-clean, and the casting is unnecessary.
Use the of_property_read_u32 accessor, with a u32 temporary variable.
> + plen = 0;
> + reg = of_get_property(node, "trickle_reg" , &plen);
> + if (plen == 4)
> + chip->trickle_charger_reg = (u16)*reg;
Likewise.
Mark.
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] rtc: ds1307: add trickle charger device tree binding
2014-08-28 12:59 ` Mark Rutland
@ 2014-08-28 15:51 ` Guenter Roeck
2014-08-28 16:10 ` Mark Rutland
2014-08-29 7:41 ` Matti Vaittinen
1 sibling, 1 reply; 22+ messages in thread
From: Guenter Roeck @ 2014-08-28 15:51 UTC (permalink / raw)
To: Mark Rutland
Cc: Matti Vaittinen, a.zummo, jason, jic23, arno, jgunthorpe, san,
hs, devicetree, linux-kernel, rtc-linux, Sverdlin Alexander
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.
> s/_/-/ in property names please.
>
> What do these bits do? is there any documentation available?
>
> Why do they need to be in the DT?
>
> Why do we not have a higher-level description?
>
> > +- trickle_reg : Trickle charger register address. Defaults to 0x10 for ds1339.
>
> Is this expected to change? Why does this need to be in the DT?
>
A quick look into the code suggests that this is a chip specific register,
and the register address is set by the driver depending on the chip type.
I don't think it should be configurable via dt.
Guenter
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] rtc: ds1307: add trickle charger device tree binding
2014-08-28 15:51 ` Guenter Roeck
@ 2014-08-28 16:10 ` Mark Rutland
2014-08-28 16:48 ` Guenter Roeck
0 siblings, 1 reply; 22+ messages in thread
From: Mark Rutland @ 2014-08-28 16:10 UTC (permalink / raw)
To: Guenter Roeck
Cc: Matti Vaittinen, a.zummo, jason, jic23, arno, jgunthorpe, san,
hs, devicetree, linux-kernel, rtc-linux, Sverdlin Alexander
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?
> > s/_/-/ in property names please.
> >
> > What do these bits do? is there any documentation available?
> >
> > Why do they need to be in the DT?
> >
> > Why do we not have a higher-level description?
> >
> > > +- trickle_reg : Trickle charger register address. Defaults to 0x10 for ds1339.
> >
> > Is this expected to change? Why does this need to be in the DT?
> >
> A quick look into the code suggests that this is a chip specific register,
> and the register address is set by the driver depending on the chip type.
> I don't think it should be configurable via dt.
Ok.
Cheers,
Mark.
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] rtc: ds1307: add trickle charger device tree binding
2014-08-28 16:10 ` Mark Rutland
@ 2014-08-28 16:48 ` Guenter Roeck
2014-08-28 17:28 ` Jason Cooper
0 siblings, 1 reply; 22+ messages in thread
From: Guenter Roeck @ 2014-08-28 16:48 UTC (permalink / raw)
To: Mark Rutland
Cc: Matti Vaittinen, a.zummo, jason, jic23, arno, jgunthorpe, san,
hs, devicetree, linux-kernel, rtc-linux, Sverdlin Alexander
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.
Note that the relevant bits are the lower 4 bits, not the
lower 5 bits, so the above text is wrong.
Guenter
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] rtc: ds1307: add trickle charger device tree binding
2014-08-28 16:48 ` Guenter Roeck
@ 2014-08-28 17:28 ` Jason Cooper
2014-08-28 17:40 ` Guenter Roeck
0 siblings, 1 reply; 22+ messages in thread
From: Jason Cooper @ 2014-08-28 17:28 UTC (permalink / raw)
To: Guenter Roeck
Cc: Mark Rutland, Matti Vaittinen, a.zummo, jic23, arno, jgunthorpe,
san, hs, devicetree, linux-kernel, rtc-linux, Sverdlin Alexander
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.
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] rtc: ds1307: add trickle charger device tree binding
2014-08-28 17:28 ` Jason Cooper
@ 2014-08-28 17:40 ` Guenter Roeck
2014-08-29 7:34 ` Matti Vaittinen
0 siblings, 1 reply; 22+ messages in thread
From: Guenter Roeck @ 2014-08-28 17:40 UTC (permalink / raw)
To: Jason Cooper
Cc: Mark Rutland, Matti Vaittinen, a.zummo, jic23, arno, jgunthorpe,
san, hs, devicetree, linux-kernel, rtc-linux, Sverdlin Alexander
On Thu, Aug 28, 2014 at 01:28:42PM -0400, Jason Cooper wrote:
> 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.
>
Yes, that sounds reasonable as well.
Guenter
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] rtc: ds1307: add trickle charger device tree binding
2014-08-28 17:40 ` Guenter Roeck
@ 2014-08-29 7:34 ` Matti Vaittinen
2014-08-29 10:40 ` Mark Rutland
0 siblings, 1 reply; 22+ messages in thread
From: Matti Vaittinen @ 2014-08-29 7:34 UTC (permalink / raw)
To: ext Guenter Roeck
Cc: Jason Cooper, Mark Rutland, a.zummo, jic23, arno, jgunthorpe,
san, hs, devicetree, linux-kernel, rtc-linux, Sverdlin Alexander
On Thu, Aug 28, 2014 at 10:40:34AM -0700, ext Guenter Roeck wrote:
> On Thu, Aug 28, 2014 at 01:28:42PM -0400, Jason Cooper wrote:
> > 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.
> > > > > > >
> > > > > > > +++ 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.
> >
>
> Yes, that sounds reasonable as well.
>
I thought of this too. However the ds1307 seems to be designed to work
with bunch of chips. What then when next chip supported by this driver
introduces 75, 1000 and 5000 ohm resistors? Or something else. (Or add
some other configuration). I do not see this approach to be maintainable
in long run. I see strong possibility of polluting dt with endless
amount of defines. Furthermore I believe the benefits of these defines
would be negligible compared to effort maintaining defines and
documentation of them causes. Surely the one who needs to add dt node
for this chip in his board's dt has the manual for the chip he has on
board. Especially so if he knows the trickle charger there and wants to
configure it. The plain resistor type still gives zero information
without knowing the other details.
--
=============================================
Matti Vaittinen
Senile SW Specialist
FINLAND
~~ When things go utterly wrong vim users can always type :help! ~~
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] rtc: ds1307: add trickle charger device tree binding
2014-08-28 12:59 ` Mark Rutland
2014-08-28 15:51 ` Guenter Roeck
@ 2014-08-29 7:41 ` Matti Vaittinen
1 sibling, 0 replies; 22+ messages in thread
From: Matti Vaittinen @ 2014-08-29 7:41 UTC (permalink / raw)
To: ext Mark Rutland
Cc: a.zummo, jason, linux, jic23, arno, jgunthorpe, san, hs,
devicetree, linux-kernel, rtc-linux, Sverdlin Alexander
On Thu, Aug 28, 2014 at 01:59:15PM +0100, ext 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.
> >
> > 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.
>
> s/_/-/ in property names please.
I'll fix this, thanks.
> Why do we not have a higher-level description?
I can add the meanings for this specific chip in the binding document,
however I would still keep the raw register value as the dt entry due
to reasons explained in my previous email.
>
> > +- trickle_reg : Trickle charger register address. Defaults to 0x10 for ds1339.
>
> Is this expected to change? Why does this need to be in the DT?
I was thinking it might be handly as I do not know the details of all
variants of this chip. However I guess it could be left out.
>
> > +
> > +Example:
> > + ds1339: rtc@68 {
> > + compatible = "dallas,ds1339";
> > + trickle_setup= <0x5>;
> > + reg = <0x68>;
> > + };
> > +
> > +
> > diff --git a/drivers/rtc/rtc-ds1307.c b/drivers/rtc/rtc-ds1307.c
> > index f03d5ba..f9b1244 100644
> > --- a/drivers/rtc/rtc-ds1307.c
> > +++ b/drivers/rtc/rtc-ds1307.c
> > @@ -126,9 +126,10 @@ struct chip_desc {
> > u16 nvram_offset;
> > u16 nvram_size;
> > u16 trickle_charger_reg;
> > + u8 trickle_charger_setup;
> > };
> >
> > -static const struct chip_desc chips[last_ds_type] = {
> > +static struct chip_desc chips[last_ds_type] = {
> > [ds_1307] = {
> > .nvram_offset = 8,
> > .nvram_size = 56,
> > @@ -835,13 +836,37 @@ ds1307_nvram_write(struct file *filp, struct kobject *kobj,
> >
> > /*----------------------------------------------------------------------*/
> >
> > +static void trickle_charger_of_init(struct chip_desc *chip,
> > + struct device_node *node)
> > +{
> > + int plen = 0;
> > + const uint32_t *setup;
> > + const uint32_t *reg;
> > +
> > + setup = of_get_property(node, "trickle_setup" , &plen);
> > + if (plen != 4)
> > + goto out;
> > + chip->trickle_charger_setup = (u8)*setup;
>
> This isn't endian-clean, and the casting is unnecessary.
>
> Use the of_property_read_u32 accessor, with a u32 temporary variable.
I'll fix this, thanks.
>
> > + plen = 0;
> > + reg = of_get_property(node, "trickle_reg" , &plen);
> > + if (plen == 4)
> > + chip->trickle_charger_reg = (u16)*reg;
>
> Likewise.
>
I'll drop the register address from dt alltogether.
I will fix these issues and prepare a new patch.
Br. Matti Vaittinen
--
=============================================
Matti Vaittinen
Senile SW Specialist
FINLAND
~~ When things go utterly wrong vim users can always type :help! ~~
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] rtc: ds1307: add trickle charger device tree binding
2014-08-29 7:34 ` Matti Vaittinen
@ 2014-08-29 10:40 ` Mark Rutland
2014-08-29 12:19 ` Matti Vaittinen
` (2 more replies)
0 siblings, 3 replies; 22+ messages in thread
From: Mark Rutland @ 2014-08-29 10:40 UTC (permalink / raw)
To: Matti Vaittinen
Cc: ext Guenter Roeck, Jason Cooper, a.zummo, jic23, arno,
jgunthorpe, san, hs, devicetree, linux-kernel, rtc-linux,
Sverdlin Alexander
On Fri, Aug 29, 2014 at 08:34:25AM +0100, Matti Vaittinen wrote:
> On Thu, Aug 28, 2014 at 10:40:34AM -0700, ext Guenter Roeck wrote:
> > On Thu, Aug 28, 2014 at 01:28:42PM -0400, Jason Cooper wrote:
> > > 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.
> > > > > > > >
> > > > > > > > +++ 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.
> > >
> >
> > Yes, that sounds reasonable as well.
> >
> I thought of this too. However the ds1307 seems to be designed to work
> with bunch of chips. What then when next chip supported by this driver
> introduces 75, 1000 and 5000 ohm resistors? Or something else. (Or add
> some other configuration). I do not see this approach to be
> maintainable in long run.
If a new chip comes out with new features, the driver will need an
update anyhow. We have no guarantee that the register placement will be
the same, let alone the layout.
If and when said new chip comes out we allocate a new compatible string.
If it's compatible iwith (i.e. is a superset of) an existing device's
programming model, we add that existing string as a fallback in the
compatible list so old kernels can drive the subset of features they
understand.
> I see strong possibility of polluting dt with endless amount of
> defines. Furthermore I believe the benefits of these defines would be
> negligible compared to effort maintaining defines and documentation of
> them causes. Surely the one who needs to add dt node for this chip in
> his board's dt has the manual for the chip he has on board. Especially
> so if he knows the trickle charger there and wants to configure it.
> The plain resistor type still gives zero information without knowing
> the other details.
I would rather that the driver had some idea of what it were doing
rather than being a glorified copy routine.
I would suggest we have two properties that describe the resistor's
rating and whether or not there is a diode:
trickle-resistor-ohms = <250>
diode-connected;
That's easy for a human to write and/or validate, we can easily extend
it in future, requires no proliferation of macros, and describes the
hardware rather than telling software what to do.
The driver becomes a little more complicated, but gains sanity checking,
which is a good thing.
I'm still worried that we have no idea what the device is intended to
charge. Surely that's important?
Thanks,
Mark.
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] rtc: ds1307: add trickle charger device tree binding
2014-08-29 10:40 ` Mark Rutland
@ 2014-08-29 12:19 ` Matti Vaittinen
2014-08-29 12:24 ` Jason Cooper
2014-09-08 13:52 ` Pavel Machek
2 siblings, 0 replies; 22+ messages in thread
From: Matti Vaittinen @ 2014-08-29 12:19 UTC (permalink / raw)
To: ext Mark Rutland
Cc: ext Guenter Roeck, Jason Cooper, a.zummo, jic23, arno,
jgunthorpe, san, hs, devicetree, linux-kernel, rtc-linux,
Sverdlin Alexander
On Fri, Aug 29, 2014 at 11:40:02AM +0100, ext Mark Rutland wrote:
> On Fri, Aug 29, 2014 at 08:34:25AM +0100, Matti Vaittinen wrote:
> > On Thu, Aug 28, 2014 at 10:40:34AM -0700, ext Guenter Roeck wrote:
> > > On Thu, Aug 28, 2014 at 01:28:42PM -0400, Jason Cooper wrote:
> > > > On Thu, Aug 28, 2014 at 09:48:25AM -0700, Guenter Roeck wrote:
> > > >
> > > > 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.
> > > >
> > >
> > > Yes, that sounds reasonable as well.
> > >
> > I thought of this too. However the ds1307 seems to be designed to work
> > with bunch of chips. What then when next chip supported by this driver
> > introduces 75, 1000 and 5000 ohm resistors? Or something else. (Or add
> > some other configuration). I do not see this approach to be
> > maintainable in long run.
>
> If a new chip comes out with new features, the driver will need an
> update anyhow. We have no guarantee that the register placement will be
> the same, let alone the layout.
This is why I originally also added the trickle_reg property. But let's
leave it out.
>
> If and when said new chip comes out we allocate a new compatible string.
> If it's compatible iwith (i.e. is a superset of) an existing device's
> programming model, we add that existing string as a fallback in the
> compatible list so old kernels can drive the subset of features they
> understand.
Yes.
>
> > I see strong possibility of polluting dt with endless amount of
> > defines. Furthermore I believe the benefits of these defines would be
> > negligible compared to effort maintaining defines and documentation of
> > them causes. Surely the one who needs to add dt node for this chip in
> > his board's dt has the manual for the chip he has on board. Especially
> > so if he knows the trickle charger there and wants to configure it.
> > The plain resistor type still gives zero information without knowing
> > the other details.
>
> I would rather that the driver had some idea of what it were doing
> rather than being a glorified copy routine.
>
> I would suggest we have two properties that describe the resistor's
> rating and whether or not there is a diode:
>
> trickle-resistor-ohms = <250>
> diode-connected;
>
> That's easy for a human to write and/or validate, we can easily extend
> it in future, requires no proliferation of macros, and describes the
> hardware rather than telling software what to do.
>
> The driver becomes a little more complicated, but gains sanity checking,
> which is a good thing.
This looks like a nice way. Problem is that I can not provide support for
all chips ds1307 supports. I have access to ds1339 variant only. I assume
the driver should thus reject this dt information for all other chips for
now.
>
> I'm still worried that we have no idea what the device is intended to
> charge. Surely that's important?
In my case it will charge capcitor which provides Vbackup for short
power breaks.
Br.
--Matti Vaittinen
--
=============================================
Matti Vaittinen
Senile SW Specialist
FINLAND
~~ When things go utterly wrong vim users can always type :help! ~~
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] rtc: ds1307: add trickle charger device tree binding
2014-08-29 10:40 ` Mark Rutland
2014-08-29 12:19 ` Matti Vaittinen
@ 2014-08-29 12:24 ` Jason Cooper
2014-08-29 12:42 ` Mark Rutland
2014-09-08 13:52 ` Pavel Machek
2 siblings, 1 reply; 22+ messages in thread
From: Jason Cooper @ 2014-08-29 12:24 UTC (permalink / raw)
To: Mark Rutland
Cc: Matti Vaittinen, ext Guenter Roeck, a.zummo, jic23, arno,
jgunthorpe, san, hs, devicetree, linux-kernel, rtc-linux,
Sverdlin Alexander
On Fri, Aug 29, 2014 at 11:40:02AM +0100, Mark Rutland wrote:
> On Fri, Aug 29, 2014 at 08:34:25AM +0100, Matti Vaittinen wrote:
> > On Thu, Aug 28, 2014 at 10:40:34AM -0700, ext Guenter Roeck wrote:
> > > On Thu, Aug 28, 2014 at 01:28:42PM -0400, Jason Cooper wrote:
> > > > 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.
> > > > > > > > >
> > > > > > > > > +++ 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.
> > > >
> > >
> > > Yes, that sounds reasonable as well.
> > >
> > I thought of this too. However the ds1307 seems to be designed to work
> > with bunch of chips. What then when next chip supported by this driver
> > introduces 75, 1000 and 5000 ohm resistors? Or something else. (Or add
> > some other configuration). I do not see this approach to be
> > maintainable in long run.
>
> If a new chip comes out with new features, the driver will need an
> update anyhow. We have no guarantee that the register placement will be
> the same, let alone the layout.
Yes, I'm reminded of our conversations a year ago in Edinburgh. It's
impossible to engineer a perfect, future-proof binding. But that
being said...
> If and when said new chip comes out we allocate a new compatible string.
> If it's compatible iwith (i.e. is a superset of) an existing device's
> programming model, we add that existing string as a fallback in the
> compatible list so old kernels can drive the subset of features they
> understand.
>
> > I see strong possibility of polluting dt with endless amount of
> > defines. Furthermore I believe the benefits of these defines would be
> > negligible compared to effort maintaining defines and documentation of
> > them causes. Surely the one who needs to add dt node for this chip in
> > his board's dt has the manual for the chip he has on board. Especially
> > so if he knows the trickle charger there and wants to configure it.
> > The plain resistor type still gives zero information without knowing
> > the other details.
>
> I would rather that the driver had some idea of what it were doing
> rather than being a glorified copy routine.
>
> I would suggest we have two properties that describe the resistor's
> rating and whether or not there is a diode:
>
> trickle-resistor-ohms = <250>
> diode-connected;
I much prefer this solution over my own suggestion. With one small
change, s/diode-connected/trickle-diode-enable/ Does that sound ok?
> That's easy for a human to write and/or validate, we can easily extend
> it in future, requires no proliferation of macros, and describes the
> hardware rather than telling software what to do.
>
> The driver becomes a little more complicated, but gains sanity checking,
> which is a good thing.
>
> I'm still worried that we have no idea what the device is intended to
> charge. Surely that's important?
In the docs it said a variety of batteries and supercaps. Yeah, not
much help...
thx,
Jason.
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] rtc: ds1307: add trickle charger device tree binding
2014-08-29 12:24 ` Jason Cooper
@ 2014-08-29 12:42 ` Mark Rutland
2014-08-29 12:48 ` Jason Cooper
0 siblings, 1 reply; 22+ messages in thread
From: Mark Rutland @ 2014-08-29 12:42 UTC (permalink / raw)
To: Jason Cooper
Cc: Matti Vaittinen, ext Guenter Roeck, a.zummo, jic23, arno,
jgunthorpe, san, hs, devicetree, linux-kernel, rtc-linux,
Sverdlin Alexander
On Fri, Aug 29, 2014 at 01:24:52PM +0100, Jason Cooper wrote:
> On Fri, Aug 29, 2014 at 11:40:02AM +0100, Mark Rutland wrote:
> > On Fri, Aug 29, 2014 at 08:34:25AM +0100, Matti Vaittinen wrote:
> > > On Thu, Aug 28, 2014 at 10:40:34AM -0700, ext Guenter Roeck wrote:
> > > > On Thu, Aug 28, 2014 at 01:28:42PM -0400, Jason Cooper wrote:
> > > > > 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.
> > > > > > > > > >
> > > > > > > > > > +++ 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.
> > > > >
> > > >
> > > > Yes, that sounds reasonable as well.
> > > >
> > > I thought of this too. However the ds1307 seems to be designed to work
> > > with bunch of chips. What then when next chip supported by this driver
> > > introduces 75, 1000 and 5000 ohm resistors? Or something else. (Or add
> > > some other configuration). I do not see this approach to be
> > > maintainable in long run.
> >
> > If a new chip comes out with new features, the driver will need an
> > update anyhow. We have no guarantee that the register placement will be
> > the same, let alone the layout.
>
> Yes, I'm reminded of our conversations a year ago in Edinburgh. It's
> impossible to engineer a perfect, future-proof binding. But that
> being said...
>
> > If and when said new chip comes out we allocate a new compatible string.
> > If it's compatible iwith (i.e. is a superset of) an existing device's
> > programming model, we add that existing string as a fallback in the
> > compatible list so old kernels can drive the subset of features they
> > understand.
> >
> > > I see strong possibility of polluting dt with endless amount of
> > > defines. Furthermore I believe the benefits of these defines would be
> > > negligible compared to effort maintaining defines and documentation of
> > > them causes. Surely the one who needs to add dt node for this chip in
> > > his board's dt has the manual for the chip he has on board. Especially
> > > so if he knows the trickle charger there and wants to configure it.
> > > The plain resistor type still gives zero information without knowing
> > > the other details.
> >
> > I would rather that the driver had some idea of what it were doing
> > rather than being a glorified copy routine.
> >
> > I would suggest we have two properties that describe the resistor's
> > rating and whether or not there is a diode:
> >
> > trickle-resistor-ohms = <250>
> > diode-connected;
>
> I much prefer this solution over my own suggestion. With one small
> change, s/diode-connected/trickle-diode-enable/ Does that sound ok?
I'm not too keen on 'enable'; I was under the impression that this
described whether or not there was an external diode. Perhaps I've
misunderstood?
Thanks,
Mark.
> > That's easy for a human to write and/or validate, we can easily extend
> > it in future, requires no proliferation of macros, and describes the
> > hardware rather than telling software what to do.
> >
> > The driver becomes a little more complicated, but gains sanity checking,
> > which is a good thing.
> >
> > I'm still worried that we have no idea what the device is intended to
> > charge. Surely that's important?
>
> In the docs it said a variety of batteries and supercaps. Yeah, not
> much help...
>
> thx,
>
> Jason.
>
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] rtc: ds1307: add trickle charger device tree binding
2014-08-29 12:42 ` Mark Rutland
@ 2014-08-29 12:48 ` Jason Cooper
2014-08-29 13:03 ` Mark Rutland
2014-08-29 14:06 ` Matti Vaittinen
0 siblings, 2 replies; 22+ messages in thread
From: Jason Cooper @ 2014-08-29 12:48 UTC (permalink / raw)
To: Mark Rutland
Cc: Matti Vaittinen, ext Guenter Roeck, a.zummo, jic23, arno,
jgunthorpe, san, hs, devicetree, linux-kernel, rtc-linux,
Sverdlin Alexander
On Fri, Aug 29, 2014 at 01:42:16PM +0100, Mark Rutland wrote:
> On Fri, Aug 29, 2014 at 01:24:52PM +0100, Jason Cooper wrote:
> > On Fri, Aug 29, 2014 at 11:40:02AM +0100, Mark Rutland wrote:
...
Someone wrote:
> > > > > > > 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
...
> > > I would suggest we have two properties that describe the resistor's
> > > rating and whether or not there is a diode:
> > >
> > > trickle-resistor-ohms = <250>
> > > diode-connected;
> >
> > I much prefer this solution over my own suggestion. With one small
> > change, s/diode-connected/trickle-diode-enable/ Does that sound ok?
>
> I'm not too keen on 'enable'; I was under the impression that this
> described whether or not there was an external diode. Perhaps I've
> misunderstood?
iiuc from the link ds1339 ds, the register configuration is enabling
internal diodes and resistors for the trickle charge configuration. I
may have read it incorrectly, though.
In either case, my main point was to prepend 'trickle-' to the property
to more accurately describe what it is toggling.
thx,
Jason.
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] rtc: ds1307: add trickle charger device tree binding
2014-08-29 12:48 ` Jason Cooper
@ 2014-08-29 13:03 ` Mark Rutland
2014-08-29 14:06 ` Matti Vaittinen
1 sibling, 0 replies; 22+ messages in thread
From: Mark Rutland @ 2014-08-29 13:03 UTC (permalink / raw)
To: Jason Cooper
Cc: Matti Vaittinen, ext Guenter Roeck, a.zummo, jic23, arno,
jgunthorpe, san, hs, devicetree, linux-kernel, rtc-linux,
Sverdlin Alexander
On Fri, Aug 29, 2014 at 01:48:29PM +0100, Jason Cooper wrote:
> On Fri, Aug 29, 2014 at 01:42:16PM +0100, Mark Rutland wrote:
> > On Fri, Aug 29, 2014 at 01:24:52PM +0100, Jason Cooper wrote:
> > > On Fri, Aug 29, 2014 at 11:40:02AM +0100, Mark Rutland wrote:
> ...
> Someone wrote:
> > > > > > > > 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
> ...
> > > > I would suggest we have two properties that describe the resistor's
> > > > rating and whether or not there is a diode:
> > > >
> > > > trickle-resistor-ohms = <250>
> > > > diode-connected;
> > >
> > > I much prefer this solution over my own suggestion. With one small
> > > change, s/diode-connected/trickle-diode-enable/ Does that sound ok?
> >
> > I'm not too keen on 'enable'; I was under the impression that this
> > described whether or not there was an external diode. Perhaps I've
> > misunderstood?
>
> iiuc from the link ds1339 ds, the register configuration is enabling
> internal diodes and resistors for the trickle charge configuration. I
> may have read it incorrectly, though.
Ok.
> In either case, my main point was to prepend 'trickle-' to the property
> to more accurately describe what it is toggling.
That's fine by me. My only concern was 'enable' due to the
internal/external confusion.
Thanks,
Mark.
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] rtc: ds1307: add trickle charger device tree binding
2014-08-29 12:48 ` Jason Cooper
2014-08-29 13:03 ` Mark Rutland
@ 2014-08-29 14:06 ` Matti Vaittinen
1 sibling, 0 replies; 22+ messages in thread
From: Matti Vaittinen @ 2014-08-29 14:06 UTC (permalink / raw)
To: ext Jason Cooper
Cc: Mark Rutland, ext Guenter Roeck, a.zummo, jic23, arno,
jgunthorpe, san, hs, devicetree, linux-kernel, rtc-linux,
Sverdlin Alexander
On Fri, Aug 29, 2014 at 08:48:29AM -0400, ext Jason Cooper wrote:
> On Fri, Aug 29, 2014 at 01:42:16PM +0100, Mark Rutland wrote:
> > On Fri, Aug 29, 2014 at 01:24:52PM +0100, Jason Cooper wrote:
> > > On Fri, Aug 29, 2014 at 11:40:02AM +0100, Mark Rutland wrote:
> ...
> Someone wrote:
> > > > > > > > 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
> ...
> > > > I would suggest we have two properties that describe the resistor's
> > > > rating and whether or not there is a diode:
> > > >
> > > > trickle-resistor-ohms = <250>
> > > > diode-connected;
> > >
> > > I much prefer this solution over my own suggestion. With one small
> > > change, s/diode-connected/trickle-diode-enable/ Does that sound ok?
> >
> > I'm not too keen on 'enable'; I was under the impression that this
> > described whether or not there was an external diode. Perhaps I've
> > misunderstood?
>
> iiuc from the link ds1339 ds, the register configuration is enabling
> internal diodes and resistors for the trickle charge configuration. I
> may have read it incorrectly, though.
Yes. It is about using internal diode.
I will cook new patch based on these comments on monday.
Regards
Matti Vaittinen
--
=============================================
Matti Vaittinen
Senile SW Specialist
FINLAND
~~ When things go utterly wrong vim users can always type :help! ~~
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] rtc: ds1307: add trickle charger device tree binding
2014-08-29 10:40 ` Mark Rutland
2014-08-29 12:19 ` Matti Vaittinen
2014-08-29 12:24 ` Jason Cooper
@ 2014-09-08 13:52 ` Pavel Machek
2014-09-08 14:58 ` Mark Rutland
2 siblings, 1 reply; 22+ messages in thread
From: Pavel Machek @ 2014-09-08 13:52 UTC (permalink / raw)
To: Mark Rutland
Cc: Matti Vaittinen, ext Guenter Roeck, Jason Cooper, a.zummo, jic23,
arno, jgunthorpe, san, hs, devicetree, linux-kernel, rtc-linux,
Sverdlin Alexander
Hi!
> I would suggest we have two properties that describe the resistor's
> rating and whether or not there is a diode:
>
> trickle-resistor-ohms = <250>
> diode-connected;
>
> That's easy for a human to write and/or validate, we can easily extend
> it in future, requires no proliferation of macros, and describes the
> hardware rather than telling software what to do.
>
> The driver becomes a little more complicated, but gains sanity checking,
> which is a good thing.
Certainly looks better than register bit defines.
As rtc-bq32k has similar options, I'm interested, too. I believe we
should add
trickle-charge-enable;
(we may not want to charge at all), and I believe the diode should be
disconnect-diode;
... With diode connected, charge is slower, and that probably should
be the default value. We don't want to give too much current in
default case. (bq32k has hard-coded resistor value for
diode/not-diode case).
Thanks and best regards,
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] rtc: ds1307: add trickle charger device tree binding
2014-09-08 13:52 ` Pavel Machek
@ 2014-09-08 14:58 ` Mark Rutland
2014-09-09 6:22 ` Matti Vaittinen
0 siblings, 1 reply; 22+ messages in thread
From: Mark Rutland @ 2014-09-08 14:58 UTC (permalink / raw)
To: Pavel Machek
Cc: Matti Vaittinen, ext Guenter Roeck, Jason Cooper, a.zummo, jic23,
arno, jgunthorpe, san, hs, devicetree, linux-kernel, rtc-linux,
Sverdlin Alexander
On Mon, Sep 08, 2014 at 02:52:42PM +0100, Pavel Machek wrote:
> Hi!
>
> > I would suggest we have two properties that describe the resistor's
> > rating and whether or not there is a diode:
> >
> > trickle-resistor-ohms = <250>
> > diode-connected;
> >
> > That's easy for a human to write and/or validate, we can easily extend
> > it in future, requires no proliferation of macros, and describes the
> > hardware rather than telling software what to do.
> >
> > The driver becomes a little more complicated, but gains sanity checking,
> > which is a good thing.
>
> Certainly looks better than register bit defines.
>
> As rtc-bq32k has similar options, I'm interested, too. I believe we
> should add
>
> trickle-charge-enable;
>
> (we may not want to charge at all), and I believe the diode should be
>
> disconnect-diode;
>
> ... With diode connected, charge is slower, and that probably should
> be the default value. We don't want to give too much current in
> default case. (bq32k has hard-coded resistor value for
> diode/not-diode case).
I agree that if one case is less likely to be problematic / damaging
that should be the default.
Mark.
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] rtc: ds1307: add trickle charger device tree binding
2014-09-08 14:58 ` Mark Rutland
@ 2014-09-09 6:22 ` Matti Vaittinen
2014-09-09 11:34 ` Pavel Machek
0 siblings, 1 reply; 22+ messages in thread
From: Matti Vaittinen @ 2014-09-09 6:22 UTC (permalink / raw)
To: ext Mark Rutland
Cc: Pavel Machek, ext Guenter Roeck, Jason Cooper, a.zummo, jic23,
arno, jgunthorpe, san, hs, devicetree, linux-kernel, rtc-linux,
Sverdlin Alexander
On Mon, Sep 08, 2014 at 03:58:25PM +0100, ext Mark Rutland wrote:
> On Mon, Sep 08, 2014 at 02:52:42PM +0100, Pavel Machek wrote:
> > > I would suggest we have two properties that describe the resistor's
> > > rating and whether or not there is a diode:
> > >
> > > trickle-resistor-ohms = <250>
> > > diode-connected;
> > >
> > > That's easy for a human to write and/or validate, we can easily extend
> > > it in future, requires no proliferation of macros, and describes the
> > > hardware rather than telling software what to do.
> > >
> > > The driver becomes a little more complicated, but gains sanity checking,
> > > which is a good thing.
> >
> > Certainly looks better than register bit defines.
> >
> > As rtc-bq32k has similar options, I'm interested, too. I believe we
> > should add
> >
> > trickle-charge-enable;
I am unsure about this. It makes sense for devices where we cannot
select resistor but just enable or disable charger. Is there any such
devices? For devices like ds1339 this makes no sense. For them it is
simpler to just enable the charger if resistor value is specified, and
default the charger to be off. But I do not see a problem in supporting
the trickle-charge-enable; for devices needing this (independently from
the ds1307 driver).
> >
> > (we may not want to charge at all), and I believe the diode should be
> >
> > disconnect-diode;
> >
> > ... With diode connected, charge is slower, and that probably should
> > be the default value. We don't want to give too much current in
> > default case. (bq32k has hard-coded resistor value for
> > diode/not-diode case).
>
> I agree that if one case is less likely to be problematic / damaging
> that should be the default.
_Maybe_
diode-connected = <1>; or
diode-connected = <0>;
or both
diode-connected; and
diode-disconnected;
could be used and let the driver to choose the defaults? But yes, I have
no problem with inverting the default to be diode-connected.
Then one question regarding the "process". Now if I cook up fourth patch
with inverted diode-connected default for ds1339, should I collect new
acks for this new patch? (V3 was acked by Alessandro and Jason.) I assume
yes.
>
> Mark.
--
=============================================
Matti Vaittinen
Senile SW Specialist
FINLAND
~~ When things go utterly wrong vim users can always type :help! ~~
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] rtc: ds1307: add trickle charger device tree binding
2014-09-09 6:22 ` Matti Vaittinen
@ 2014-09-09 11:34 ` Pavel Machek
2014-09-09 11:42 ` Jason Cooper
0 siblings, 1 reply; 22+ messages in thread
From: Pavel Machek @ 2014-09-09 11:34 UTC (permalink / raw)
To: Matti Vaittinen
Cc: ext Mark Rutland, ext Guenter Roeck, Jason Cooper, a.zummo,
jic23, arno, jgunthorpe, san, hs, devicetree, linux-kernel,
rtc-linux, Sverdlin Alexander
Hi!
> > > > trickle-resistor-ohms = <250>
> > > > diode-connected;
> > > >
> > > > That's easy for a human to write and/or validate, we can easily extend
> > > > it in future, requires no proliferation of macros, and describes the
> > > > hardware rather than telling software what to do.
> > > >
> > > > The driver becomes a little more complicated, but gains sanity checking,
> > > > which is a good thing.
> > >
> > > Certainly looks better than register bit defines.
> > >
> > > As rtc-bq32k has similar options, I'm interested, too. I believe we
> > > should add
> > >
> > > trickle-charge-enable;
>
> I am unsure about this. It makes sense for devices where we cannot
> select resistor but just enable or disable charger. Is there any such
> devices? For devices like ds1339 this makes no sense. For them it is
> simpler to just enable the charger if resistor value is specified, and
> default the charger to be off. But I do not see a problem in supporting
> the trickle-charge-enable; for devices needing this (independently from
> the ds1307 driver).
Ok, just cc me on next version of the driver, and I'll adjust bq32k to
be similar.
> > > (we may not want to charge at all), and I believe the diode should be
> > >
> > > disconnect-diode;
> > >
> > > ... With diode connected, charge is slower, and that probably should
> > > be the default value. We don't want to give too much current in
> > > default case. (bq32k has hard-coded resistor value for
> > > diode/not-diode case).
> >
> > I agree that if one case is less likely to be problematic / damaging
> > that should be the default.
>
> _Maybe_
> diode-connected = <1>; or
> diode-connected = <0>;
That should work.
Actually, make it "connect-diode = <0/1>;", because hardware can
actually select if the diode is connected or not.
> Then one question regarding the "process". Now if I cook up fourth patch
> with inverted diode-connected default for ds1339, should I collect new
> acks for this new patch? (V3 was acked by Alessandro and Jason.) I assume
> yes.
It depends if patch changed "a lot". I guess you can keep the acks.
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] rtc: ds1307: add trickle charger device tree binding
2014-09-09 11:34 ` Pavel Machek
@ 2014-09-09 11:42 ` Jason Cooper
2014-09-09 13:48 ` VS: " Vaittinen, Matti (NSN - FI/Oulu)
0 siblings, 1 reply; 22+ messages in thread
From: Jason Cooper @ 2014-09-09 11:42 UTC (permalink / raw)
To: Pavel Machek
Cc: Matti Vaittinen, ext Mark Rutland, ext Guenter Roeck, a.zummo,
jic23, arno, jgunthorpe, san, hs, devicetree, linux-kernel,
rtc-linux, Sverdlin Alexander
On Tue, Sep 09, 2014 at 01:34:25PM +0200, Pavel Machek wrote:
...
> > _Maybe_
> > diode-connected = <1>; or
> > diode-connected = <0>;
>
> That should work.
meh. The presence or absence of the property should express the
boolean.
The safe state should be the default (property absent), and the property
name should describe the state when the property is present.
thx,
Jason.
^ permalink raw reply [flat|nested] 22+ messages in thread
* VS: [PATCH] rtc: ds1307: add trickle charger device tree binding
2014-09-09 11:42 ` Jason Cooper
@ 2014-09-09 13:48 ` Vaittinen, Matti (NSN - FI/Oulu)
0 siblings, 0 replies; 22+ messages in thread
From: Vaittinen, Matti (NSN - FI/Oulu) @ 2014-09-09 13:48 UTC (permalink / raw)
To: ext Jason Cooper, Pavel Machek
Cc: ext Mark Rutland, ext Guenter Roeck, a.zummo, jic23, arno,
jgunthorpe, san, hs, devicetree, linux-kernel, rtc-linux,
Sverdlin, Alexander (NSN - DE/Ulm)
Lähettäjä: ext Jason Cooper
> meh. The presence or absence of the property should express the
boolean.
>
> The safe state should be the default (property absent), and the property
name should describe the state when the property is present.
This is What the v5 of patch does. It introduces boolean flag trickle-diode-disable. Diode enabled should be safer and it is also the default.
Jason, I added your ack to that patch, thanks.
Br. Matti Vaittinen
^ permalink raw reply [flat|nested] 22+ messages in thread
end of thread, other threads:[~2014-09-09 13:49 UTC | newest]
Thread overview: 22+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-08-28 12:42 [PATCH] rtc: ds1307: add trickle charger device tree binding Matti Vaittinen
2014-08-28 12:59 ` Mark Rutland
2014-08-28 15:51 ` Guenter Roeck
2014-08-28 16:10 ` Mark Rutland
2014-08-28 16:48 ` Guenter Roeck
2014-08-28 17:28 ` Jason Cooper
2014-08-28 17:40 ` Guenter Roeck
2014-08-29 7:34 ` Matti Vaittinen
2014-08-29 10:40 ` Mark Rutland
2014-08-29 12:19 ` Matti Vaittinen
2014-08-29 12:24 ` Jason Cooper
2014-08-29 12:42 ` Mark Rutland
2014-08-29 12:48 ` Jason Cooper
2014-08-29 13:03 ` Mark Rutland
2014-08-29 14:06 ` Matti Vaittinen
2014-09-08 13:52 ` Pavel Machek
2014-09-08 14:58 ` Mark Rutland
2014-09-09 6:22 ` Matti Vaittinen
2014-09-09 11:34 ` Pavel Machek
2014-09-09 11:42 ` Jason Cooper
2014-09-09 13:48 ` VS: " Vaittinen, Matti (NSN - FI/Oulu)
2014-08-29 7:41 ` Matti Vaittinen
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).