| Message ID | 1465970379-14703-4-git-send-email-andrew.smirnov@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
On Tue, Jun 14, 2016 at 10:59:29PM -0700, Andrey Smirnov wrote: > Add DS1341 specific power-saving options that allow to disable certain > functional aspects of the chip in order to minimize its power > consumption. This description doesn't match that you are adding a new binding. It is preferred that bindings are a separate patch. > > Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com> > --- > .../devicetree/bindings/rtc/dallas,ds1341.txt | 23 ++++++++++++++++++ > drivers/rtc/rtc-ds1307.c | 28 ++++++++++++++++++++++ > 2 files changed, 51 insertions(+) > create mode 100644 Documentation/devicetree/bindings/rtc/dallas,ds1341.txt > > diff --git a/Documentation/devicetree/bindings/rtc/dallas,ds1341.txt b/Documentation/devicetree/bindings/rtc/dallas,ds1341.txt > new file mode 100644 > index 0000000..b8be7a4 > --- /dev/null > +++ b/Documentation/devicetree/bindings/rtc/dallas,ds1341.txt > @@ -0,0 +1,23 @@ > +* Dallas DS1341 I2C Serial Real-Time Clock > + > +Required properties: > + > +- compatible: Should contain "dallas,ds1341". > + > +- reg: I2C address for chip > + > +Optional properties: > + > +- disable-oscillator-stop-flag : Configure chip to disable oscillator > + fault detection circuitry > + > +- enable-glitch-filter : Configure chip to enable crystal oscillator > + output glitch filtering What determines setting these properties or not? They should have vendor prefix and be explicit that they are boolean. Rob
On Sun, Jun 19, 2016 at 7:29 AM, Rob Herring <robh@kernel.org> wrote: > On Tue, Jun 14, 2016 at 10:59:29PM -0700, Andrey Smirnov wrote: >> Add DS1341 specific power-saving options that allow to disable certain >> functional aspects of the chip in order to minimize its power >> consumption. > > This description doesn't match that you are adding a new binding. It is > preferred that bindings are a separate patch. OK, will split this patch into two in v2. > >> >> Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com> >> --- >> .../devicetree/bindings/rtc/dallas,ds1341.txt | 23 ++++++++++++++++++ >> drivers/rtc/rtc-ds1307.c | 28 ++++++++++++++++++++++ >> 2 files changed, 51 insertions(+) >> create mode 100644 Documentation/devicetree/bindings/rtc/dallas,ds1341.txt >> >> diff --git a/Documentation/devicetree/bindings/rtc/dallas,ds1341.txt b/Documentation/devicetree/bindings/rtc/dallas,ds1341.txt >> new file mode 100644 >> index 0000000..b8be7a4 >> --- /dev/null >> +++ b/Documentation/devicetree/bindings/rtc/dallas,ds1341.txt >> @@ -0,0 +1,23 @@ >> +* Dallas DS1341 I2C Serial Real-Time Clock >> + >> +Required properties: >> + >> +- compatible: Should contain "dallas,ds1341". >> + >> +- reg: I2C address for chip >> + >> +Optional properties: >> + >> +- disable-oscillator-stop-flag : Configure chip to disable oscillator >> + fault detection circuitry >> + >> +- enable-glitch-filter : Configure chip to enable crystal oscillator >> + output glitch filtering > > What determines setting these properties or not? Setting those properties allows drastically reduce RTC's power consumption at the expense of reliability and quality of service. In my use case, DS1341 is powered by a supercap and enabling those two setting allows to increase holdup from less than a day do 2+ weeks. > > They should have vendor prefix and be explicit that they are boolean. I was trying to be consistent with ds1339 and ds1390 bindings which do not have vendor prefixes. Will fix in v2. Thank you, Andrey Smirnov
On Sun, Jun 19, 2016 at 11:12:55AM -0700, Andrey Smirnov wrote: > On Sun, Jun 19, 2016 at 7:29 AM, Rob Herring <robh@kernel.org> wrote: > > On Tue, Jun 14, 2016 at 10:59:29PM -0700, Andrey Smirnov wrote: > >> Add DS1341 specific power-saving options that allow to disable certain > >> functional aspects of the chip in order to minimize its power > >> consumption. > > > > This description doesn't match that you are adding a new binding. It is > > preferred that bindings are a separate patch. > > OK, will split this patch into two in v2. > > > > >> > >> Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com> > >> --- > >> .../devicetree/bindings/rtc/dallas,ds1341.txt | 23 ++++++++++++++++++ > >> drivers/rtc/rtc-ds1307.c | 28 ++++++++++++++++++++++ > >> 2 files changed, 51 insertions(+) > >> create mode 100644 Documentation/devicetree/bindings/rtc/dallas,ds1341.txt > >> > >> diff --git a/Documentation/devicetree/bindings/rtc/dallas,ds1341.txt b/Documentation/devicetree/bindings/rtc/dallas,ds1341.txt > >> new file mode 100644 > >> index 0000000..b8be7a4 > >> --- /dev/null > >> +++ b/Documentation/devicetree/bindings/rtc/dallas,ds1341.txt > >> @@ -0,0 +1,23 @@ > >> +* Dallas DS1341 I2C Serial Real-Time Clock > >> + > >> +Required properties: > >> + > >> +- compatible: Should contain "dallas,ds1341". > >> + > >> +- reg: I2C address for chip > >> + > >> +Optional properties: > >> + > >> +- disable-oscillator-stop-flag : Configure chip to disable oscillator > >> + fault detection circuitry > >> + > >> +- enable-glitch-filter : Configure chip to enable crystal oscillator > >> + output glitch filtering > > > > What determines setting these properties or not? > > Setting those properties allows drastically reduce RTC's power > consumption at the expense of reliability and quality of service. In > my use case, DS1341 is powered by a supercap and enabling those two > setting allows to increase holdup from less than a day do 2+ weeks. So wouldn't you want to set one mode while running and the lower power mode while suspended? I'm trying to understand the frequency of changing this. If it is always one setting for a board, then yes it belongs in DT. If it is a user decision, then it probably shouldn't be in DT. Seeing as these are reused, I've probably already had this discussion... > > They should have vendor prefix and be explicit that they are boolean. > > I was trying to be consistent with ds1339 and ds1390 bindings which do > not have vendor prefixes. Will fix in v2. Okay, then they are fine if you are using existing properties. Perhaps these should all be in a common binding doc though. Rob
On 21/06/2016 at 15:49:04 -0500, Rob Herring wrote : > So wouldn't you want to set one mode while running and the lower power > mode while suspended? I'm trying to understand the frequency of changing > this. If it is always one setting for a board, then yes it belongs in > DT. If it is a user decision, then it probably shouldn't be in DT. > > Seeing as these are reused, I've probably already had this discussion... > I would agree with Rob here. It may be better to provide a sysfs interface to configure that particular behavior. This is usually ok because the use case is: - the RTC is not configured, time has never been set - time is set for the first time - the user can set the oscillator mode/detection/... - on subsequent reboots, the mode is kept alongside the time and date I would advise against trying to set a mode automatically in the driver because you may have unexpected power cuts and it may then let the RTC consume more power than what you really want. > > > They should have vendor prefix and be explicit that they are boolean. > > > > I was trying to be consistent with ds1339 and ds1390 bindings which do > > not have vendor prefixes. Will fix in v2. > > Okay, then they are fine if you are using existing properties. Perhaps > these should all be in a common binding doc though. > I'll try to collect the existing common properties and write that doc this week.
> So wouldn't you want to set one mode while running and the lower power > mode while suspended? I'm trying to understand the frequency of changing > this. If it is always one setting for a board, then yes it belongs in > DT. If it is a user decision, then it probably shouldn't be in DT. I don't really see a use-case where you'd want setting that dynamically. I might be wrong, but IMHO, power consumption of a system in suspended mode would dwarf that of an RTC, so there's really much to gain by enabling this feature dynamically. Where it matters the most is time setting retention when the system is powered off and RTC is ticking off of a battery or a some other power storage device. So in my opinion it is more of a system design question where one has to choose if reliability of RTC data is more important (detection of oscillator faults and higher oscillator glitch immunity) and bigger power storage device is needed or higher risk of RTC "malfunction" is acceptable and cheaper/more convenient power storage device can be used > > Seeing as these are reused, I've probably already had this discussion... > >> > They should have vendor prefix and be explicit that they are boolean. >> >> I was trying to be consistent with ds1339 and ds1390 bindings which do >> not have vendor prefixes. Will fix in v2. > > Okay, then they are fine if you are using existing properties. Perhaps > these should all be in a common binding doc though. I think we have a misunderstanding. What I meant by "trying to be consistent" was that bindings for other DS1307 variants do not prefix their own properties with vendor name. Your comment about properties being reused makes me suspicious that I misled you to believe that other chip variants use those exact properties which is not the case. Andrey
On Tue, Jun 21, 2016 at 2:07 PM, Alexandre Belloni <alexandre.belloni@free-electrons.com> wrote: > On 21/06/2016 at 15:49:04 -0500, Rob Herring wrote : >> So wouldn't you want to set one mode while running and the lower power >> mode while suspended? I'm trying to understand the frequency of changing >> this. If it is always one setting for a board, then yes it belongs in >> DT. If it is a user decision, then it probably shouldn't be in DT. >> >> Seeing as these are reused, I've probably already had this discussion... >> > > I would agree with Rob here. It may be better to provide a sysfs > interface to configure that particular behavior. I don't see any value in doing that, could you give me a realistic example of a scenario in which a user would want to spend some of uptime with RTC oscillator fault detection/glitch filtering disabled and then enable it? > This is usually ok because the use case is: > - the RTC is not configured, time has never been set > - time is set for the first time > - the user can set the oscillator mode/detection/... Unfortunately exposing that feature using sysfs gives you a leaky abstraction and your userspace instead of using a generic RTC starts using DS1341 RTC. So to accommodate for that a user would have to: a) Write + integrate a userspace tool to set the mode (which IMHO is decided upon once and doesn't change) b) If a board design is new and there's a chance of moving this chip to a different I2C bus, the code above would have to account for that and not hardcore sysfs path c) If board's BSP is intended to be used in multiple generations of a product, not all of which would use DS1341, it would be necessary to accommodate for that by either more code in the tool or an additional BSP build configuration variant > - on subsequent reboots, the mode is kept alongside the time and date > This assumes that your bootloader leaves those mode bits alone. > I would advise against trying to set a mode automatically in the driver > because you may have unexpected power cuts and it may then let the RTC > consume more power than what you really want. > I fell like I am not understanding you correctly. Why would moving configuration decision logic into userspace improve the situation in case of unexpected power loss? Andrey
On Tue, Jun 21, 2016 at 7:34 PM, Andrey Smirnov <andrew.smirnov@gmail.com> wrote: > On Tue, Jun 21, 2016 at 2:07 PM, Alexandre Belloni > <alexandre.belloni@free-electrons.com> wrote: >> On 21/06/2016 at 15:49:04 -0500, Rob Herring wrote : >>> So wouldn't you want to set one mode while running and the lower power >>> mode while suspended? I'm trying to understand the frequency of changing >>> this. If it is always one setting for a board, then yes it belongs in >>> DT. If it is a user decision, then it probably shouldn't be in DT. >>> >>> Seeing as these are reused, I've probably already had this discussion... >>> >> >> I would agree with Rob here. It may be better to provide a sysfs >> interface to configure that particular behavior. > > I don't see any value in doing that, could you give me a realistic > example of a scenario in which a user would want to spend some of > uptime with RTC oscillator fault detection/glitch filtering disabled > and then enable it? > >> This is usually ok because the use case is: >> - the RTC is not configured, time has never been set >> - time is set for the first time >> - the user can set the oscillator mode/detection/... > > Unfortunately exposing that feature using sysfs gives you a leaky > abstraction and your userspace instead of using a generic RTC starts > using DS1341 RTC. So to accommodate for that a user would have to: > > a) Write + integrate a userspace tool to set the mode (which IMHO is > decided upon once and doesn't change) > b) If a board design is new and there's a chance of moving this chip > to a different I2C bus, the code above would have to account for that > and not hardcore sysfs path > c) If board's BSP is intended to be used in multiple generations of a > product, not all of which would use DS1341, it would be necessary to > accommodate for that by either more code in the tool or an additional > BSP build configuration variant > >> - on subsequent reboots, the mode is kept alongside the time and date >> > > This assumes that your bootloader leaves those mode bits alone. > >> I would advise against trying to set a mode automatically in the driver >> because you may have unexpected power cuts and it may then let the RTC >> consume more power than what you really want. >> > > I fell like I am not understanding you correctly. Why would moving > configuration decision logic into userspace improve the situation in > case of unexpected power loss? > > Andrey Alexandre, what's the status of this discussion/patchset? Would it be more acceptable if I dropped all of the refactoring and just kept the code adding new feature + DT binding for it? I am more than happy to drop all but essential parts of the patches if that would allow us to make progress. Thank you, Andrey
On 21/06/2016 at 19:34:56 -0700, Andrey Smirnov wrote : > On Tue, Jun 21, 2016 at 2:07 PM, Alexandre Belloni > <alexandre.belloni@free-electrons.com> wrote: > > On 21/06/2016 at 15:49:04 -0500, Rob Herring wrote : > >> So wouldn't you want to set one mode while running and the lower power > >> mode while suspended? I'm trying to understand the frequency of changing > >> this. If it is always one setting for a board, then yes it belongs in > >> DT. If it is a user decision, then it probably shouldn't be in DT. > >> > >> Seeing as these are reused, I've probably already had this discussion... > >> > > > > I would agree with Rob here. It may be better to provide a sysfs > > interface to configure that particular behavior. > > I don't see any value in doing that, could you give me a realistic > example of a scenario in which a user would want to spend some of > uptime with RTC oscillator fault detection/glitch filtering disabled > and then enable it? > Well, the issue is not being dynamic, it is differentiating between hardware description and user configuration. Configuration must not be in DT. And this choice is definitively not hardware related (as opposed to the trickle charging that depends on the battery that is used on the board). > > This is usually ok because the use case is: > > - the RTC is not configured, time has never been set > > - time is set for the first time > > - the user can set the oscillator mode/detection/... > > Unfortunately exposing that feature using sysfs gives you a leaky > abstraction and your userspace instead of using a generic RTC starts > using DS1341 RTC. So to accommodate for that a user would have to: > > a) Write + integrate a userspace tool to set the mode (which IMHO is > decided upon once and doesn't change) > b) If a board design is new and there's a chance of moving this chip > to a different I2C bus, the code above would have to account for that > and not hardcore sysfs path > c) If board's BSP is intended to be used in multiple generations of a > product, not all of which would use DS1341, it would be necessary to > accommodate for that by either more code in the tool or an additional > BSP build configuration variant > Well, it doesn't leak abstraction as long as all the RTC that are able to disable the oscillator failure detection use the same ABI. > > - on subsequent reboots, the mode is kept alongside the time and date > > > > This assumes that your bootloader leaves those mode bits alone. > Well, if that is not the case, the bootloader as to be fixed anyway and silently changing the configuration back using DT is probably worse. > > I would advise against trying to set a mode automatically in the driver > > because you may have unexpected power cuts and it may then let the RTC > > consume more power than what you really want. > > > > I fell like I am not understanding you correctly. Why would moving > configuration decision logic into userspace improve the situation in > case of unexpected power loss? > I was reacting to Rob's idea to running in the lower power mode when suspend. I'm pretty sure this is not a good idea. I'd leave the whole policy to userspace.
On Tue, Jul 19, 2016 at 3:47 PM, Alexandre Belloni <alexandre.belloni@free-electrons.com> wrote: > On 21/06/2016 at 19:34:56 -0700, Andrey Smirnov wrote : >> On Tue, Jun 21, 2016 at 2:07 PM, Alexandre Belloni >> <alexandre.belloni@free-electrons.com> wrote: >> > On 21/06/2016 at 15:49:04 -0500, Rob Herring wrote : >> >> So wouldn't you want to set one mode while running and the lower power >> >> mode while suspended? I'm trying to understand the frequency of changing >> >> this. If it is always one setting for a board, then yes it belongs in >> >> DT. If it is a user decision, then it probably shouldn't be in DT. >> >> >> >> Seeing as these are reused, I've probably already had this discussion... >> >> >> > >> > I would agree with Rob here. It may be better to provide a sysfs >> > interface to configure that particular behavior. >> >> I don't see any value in doing that, could you give me a realistic >> example of a scenario in which a user would want to spend some of >> uptime with RTC oscillator fault detection/glitch filtering disabled >> and then enable it? >> > > Well, the issue is not being dynamic, it is differentiating between > hardware description and user configuration. Configuration must not be in > DT. Why? And I don't mean in a generic sense, but in this particular case. What is gained by not having this bit of configuration, whose only consumer is the driver, in the device tree file? > And this choice is definitively not hardware related (as opposed to > the trickle charging that depends on the battery that is used on the > board). There's most certainly plenty of precedents of non hardware-related in device tree, first two that come to mind are "chosen" node and "local-mac-address" property and, granted, those might be exceptions/legacy bindings that are just there to stay, but even if you look at RTC subsystem rtc-cmos.txt, atmel,at91sam-rtc.txt and possibly rtc-st-lpc.txt are providing bindings that are not exactly hardware related. Rtc-cmos.txt is especially noticeable example since it literally does what I am trying to do -- allows the user to specify initial values to certain registers that would be initialized by the driver. > >> > This is usually ok because the use case is: >> > - the RTC is not configured, time has never been set >> > - time is set for the first time >> > - the user can set the oscillator mode/detection/... >> >> Unfortunately exposing that feature using sysfs gives you a leaky >> abstraction and your userspace instead of using a generic RTC starts >> using DS1341 RTC. So to accommodate for that a user would have to: >> >> a) Write + integrate a userspace tool to set the mode (which IMHO is >> decided upon once and doesn't change) >> b) If a board design is new and there's a chance of moving this chip >> to a different I2C bus, the code above would have to account for that >> and not hardcore sysfs path >> c) If board's BSP is intended to be used in multiple generations of a >> product, not all of which would use DS1341, it would be necessary to >> accommodate for that by either more code in the tool or an additional >> BSP build configuration variant >> > > Well, it doesn't leak abstraction as long as all the RTC that are able > to disable the oscillator failure detection use the same ABI. Correct me if I am wrong, but no such, at least semi-standardized, ABI exist as of today, correct? If that is so, then what you are saying is that the abstraction doesn't leak as long as you put it inside of a new abstraction that doesn't leak. I am not arguing that it is impossible to create a new one that would allow to hide hardware differences, I am positive it is, what I am arguing is that to do so is a lot of work for as far as I can see for no gain. > >> > - on subsequent reboots, the mode is kept alongside the time and date >> > >> >> This assumes that your bootloader leaves those mode bits alone. >> > > Well, if that is not the case, the bootloader as to be fixed anyway and > silently changing the configuration back using DT is probably worse. > How so? Consider the following two scenarios with assumption that the bootloader is broken: - Bits set wrong by bootloader, then corrected by kernel, device is powered off RTC consumes expected amount of current - Bits set wrong by bootloader, kernel does nothing, device is powered off RTC consumes more than anticipated and we drain the power storage device and loose time. What do you you think former is worse than latter? Andrey
On 19/07/2016 at 16:56:56 -0700, Andrey Smirnov wrote : > >> I don't see any value in doing that, could you give me a realistic > >> example of a scenario in which a user would want to spend some of > >> uptime with RTC oscillator fault detection/glitch filtering disabled > >> and then enable it? > >> > > > > Well, the issue is not being dynamic, it is differentiating between > > hardware description and user configuration. Configuration must not be in > > DT. > > Why? And I don't mean in a generic sense, but in this particular case. > What is gained by not having this bit of configuration, whose only > consumer is the driver, in the device tree file? > Because configuration doesn't belong to DT. DT is about hardware description, not configuration. > > And this choice is definitively not hardware related (as opposed to > > the trickle charging that depends on the battery that is used on the > > board). > > There's most certainly plenty of precedents of non hardware-related in > device tree, first two that come to mind are "chosen" node and > "local-mac-address" property and, granted, those might be > exceptions/legacy bindings that are just there to stay, but even if > you look at RTC subsystem rtc-cmos.txt, atmel,at91sam-rtc.txt and > possibly rtc-st-lpc.txt are providing bindings that are not exactly > hardware related. > > Rtc-cmos.txt is especially noticeable example since it literally does > what I am trying to do -- allows the user to specify initial values to > certain registers that would be initialized by the driver. > Well, the RTC subsystem has been left unmaintained for a while and it is not because we made mistakes in the past that we have to make them again. rtc-st-lpc is only hardware related. The mode it is used in is board dependant. And I have a plan to change how the gpbr register is allocated for the at91 RTT. I agree that rtc-cmos is an example of what not to do. > > Well, it doesn't leak abstraction as long as all the RTC that are able > > to disable the oscillator failure detection use the same ABI. > > Correct me if I am wrong, but no such, at least semi-standardized, ABI > exist as of today, correct? If that is so, then what you are saying is > that the abstraction doesn't leak as long as you put it inside of a > new abstraction that doesn't leak. I am not arguing that it is > impossible to create a new one that would allow to hide hardware > differences, I am positive it is, what I am arguing is that to do so > is a lot of work for as far as I can see for no gain. > You are correct, that ABI doesn't exist and I'm asking to create it. That is how kernel development happens. > > > >> > - on subsequent reboots, the mode is kept alongside the time and date > >> > > >> > >> This assumes that your bootloader leaves those mode bits alone. > >> > > > > Well, if that is not the case, the bootloader as to be fixed anyway and > > silently changing the configuration back using DT is probably worse. > > > > How so? Consider the following two scenarios with assumption that the > bootloader is broken: > > - Bits set wrong by bootloader, then corrected by kernel, device is > powered off RTC consumes expected amount of current > > - Bits set wrong by bootloader, kernel does nothing, device is > powered off RTC consumes more than anticipated and we drain the power > storage device and loose time. > > What do you you think former is worse than latter? > Whether is is done in the kernel or in userspace doesn't change much to that use case.
On Wed, Jul 20, 2016 at 2:02 AM, Alexandre Belloni <alexandre.belloni@free-electrons.com> wrote: > On 19/07/2016 at 16:56:56 -0700, Andrey Smirnov wrote : >> >> I don't see any value in doing that, could you give me a realistic >> >> example of a scenario in which a user would want to spend some of >> >> uptime with RTC oscillator fault detection/glitch filtering disabled >> >> and then enable it? >> >> >> > >> > Well, the issue is not being dynamic, it is differentiating between >> > hardware description and user configuration. Configuration must not be in >> > DT. >> >> Why? And I don't mean in a generic sense, but in this particular case. >> What is gained by not having this bit of configuration, whose only >> consumer is the driver, in the device tree file? >> > > Because configuration doesn't belong to DT. DT is about hardware > description, not configuration. That doesn't really answer my question. You just re-iterating some maxim without explaining what is the point behind applying it. > > >> > And this choice is definitively not hardware related (as opposed to >> > the trickle charging that depends on the battery that is used on the >> > board). >> >> There's most certainly plenty of precedents of non hardware-related in >> device tree, first two that come to mind are "chosen" node and >> "local-mac-address" property and, granted, those might be >> exceptions/legacy bindings that are just there to stay, but even if >> you look at RTC subsystem rtc-cmos.txt, atmel,at91sam-rtc.txt and >> possibly rtc-st-lpc.txt are providing bindings that are not exactly >> hardware related. >> >> Rtc-cmos.txt is especially noticeable example since it literally does >> what I am trying to do -- allows the user to specify initial values to >> certain registers that would be initialized by the driver. >> > > Well, the RTC subsystem has been left unmaintained for a while and it is > not because we made mistakes in the past that we have to make them > again. > > rtc-st-lpc is only hardware related. The mode it is used in is board > dependant. And I have a plan to change how the gpbr register is > allocated for the at91 RTT. I agree that rtc-cmos is an example of what > not to do. > >> > Well, it doesn't leak abstraction as long as all the RTC that are able >> > to disable the oscillator failure detection use the same ABI. >> >> Correct me if I am wrong, but no such, at least semi-standardized, ABI >> exist as of today, correct? If that is so, then what you are saying is >> that the abstraction doesn't leak as long as you put it inside of a >> new abstraction that doesn't leak. I am not arguing that it is >> impossible to create a new one that would allow to hide hardware >> differences, I am positive it is, what I am arguing is that to do so >> is a lot of work for as far as I can see for no gain. >> > > You are correct, that ABI doesn't exist and I'm asking to create it. > That is how kernel development happens. OK, I don't think this is going anywhere. I am sure by now you know very well, that I am not going to agree to that. I'll just drop this part, gut the patchset to it's bare minimum and re-submit it. Andrey
On 20/07/2016 at 07:36:55 -0700, Andrey Smirnov wrote : > On Wed, Jul 20, 2016 at 2:02 AM, Alexandre Belloni > <alexandre.belloni@free-electrons.com> wrote: > > On 19/07/2016 at 16:56:56 -0700, Andrey Smirnov wrote : > >> >> I don't see any value in doing that, could you give me a realistic > >> >> example of a scenario in which a user would want to spend some of > >> >> uptime with RTC oscillator fault detection/glitch filtering disabled > >> >> and then enable it? > >> >> > >> > > >> > Well, the issue is not being dynamic, it is differentiating between > >> > hardware description and user configuration. Configuration must not be in > >> > DT. > >> > >> Why? And I don't mean in a generic sense, but in this particular case. > >> What is gained by not having this bit of configuration, whose only > >> consumer is the driver, in the device tree file? > >> > > > > Because configuration doesn't belong to DT. DT is about hardware > > description, not configuration. > > That doesn't really answer my question. You just re-iterating some > maxim without explaining what is the point behind applying it. > Well, that is from the device tree specification and how the device tree maintainers want it...
On Wed, Jul 20, 2016 at 8:38 AM, Alexandre Belloni <alexandre.belloni@free-electrons.com> wrote: > On 20/07/2016 at 07:36:55 -0700, Andrey Smirnov wrote : >> On Wed, Jul 20, 2016 at 2:02 AM, Alexandre Belloni >> <alexandre.belloni@free-electrons.com> wrote: >> > On 19/07/2016 at 16:56:56 -0700, Andrey Smirnov wrote : >> >> >> I don't see any value in doing that, could you give me a realistic >> >> >> example of a scenario in which a user would want to spend some of >> >> >> uptime with RTC oscillator fault detection/glitch filtering disabled >> >> >> and then enable it? >> >> >> >> >> > >> >> > Well, the issue is not being dynamic, it is differentiating between >> >> > hardware description and user configuration. Configuration must not be in >> >> > DT. >> >> >> >> Why? And I don't mean in a generic sense, but in this particular case. >> >> What is gained by not having this bit of configuration, whose only >> >> consumer is the driver, in the device tree file? >> >> >> > >> > Because configuration doesn't belong to DT. DT is about hardware >> > description, not configuration. >> >> That doesn't really answer my question. You just re-iterating some >> maxim without explaining what is the point behind applying it. >> > > Well, that is from the device tree specification and how the device tree > maintainers want it... And yet, we have whole subsystems such as "nvmem" and I am sure plenty of other smaller examples where that maxim is being applied very lax if at all. So it seems people can and do make compromises regarding the "no configuration in DT" rule if pros of doing so outweighs the cons. Andrey
diff --git a/Documentation/devicetree/bindings/rtc/dallas,ds1341.txt b/Documentation/devicetree/bindings/rtc/dallas,ds1341.txt new file mode 100644 index 0000000..b8be7a4 --- /dev/null +++ b/Documentation/devicetree/bindings/rtc/dallas,ds1341.txt @@ -0,0 +1,23 @@ +* Dallas DS1341 I2C Serial Real-Time Clock + +Required properties: + +- compatible: Should contain "dallas,ds1341". + +- reg: I2C address for chip + +Optional properties: + +- disable-oscillator-stop-flag : Configure chip to disable oscillator + fault detection circuitry + +- enable-glitch-filter : Configure chip to enable crystal oscillator + output glitch filtering + +Example: + ds1341: rtc@68 { + compatible = "dallas,ds1341"; + disable-oscillator-stop-flag; + enable-glitch-filter; + reg = <0x68>; + }; diff --git a/drivers/rtc/rtc-ds1307.c b/drivers/rtc/rtc-ds1307.c index c618c22..54cc527 100644 --- a/drivers/rtc/rtc-ds1307.c +++ b/drivers/rtc/rtc-ds1307.c @@ -78,6 +78,7 @@ enum ds_type { #define DS1337_REG_CONTROL 0x0e # define DS1337_BIT_nEOSC 0x80 # define DS1339_BIT_BBSQI 0x20 +# define DS1341_BIT_EGFIL 0x20 # define DS3231_BIT_BBSQW 0x40 /* same as BBSQI */ # define DS1337_BIT_RS2 0x10 # define DS1337_BIT_RS1 0x08 @@ -93,7 +94,9 @@ enum ds_type { # define DS1340_BIT_OSF 0x80 #define DS1337_REG_STATUS 0x0f # define DS1337_BIT_OSF 0x80 +# define DS1341_BIT_DOSF 0x40 # define DS3231_BIT_EN32KHZ 0x08 +# define DS1341_BIT_ECLK 0x04 # define DS1337_BIT_A2I 0x02 # define DS1337_BIT_A1I 0x01 #define DS1339_REG_ALARM1_SECS 0x07 @@ -1319,6 +1322,31 @@ static int ds1307_probe(struct i2c_client *client, if (ds1307->regs[0] & DS1337_BIT_nEOSC) ds1307->regs[0] &= ~DS1337_BIT_nEOSC; + if (ds1307->type == ds_1341) { + /* Make sure we are not generating square wave + * output */ + ds1307->regs[1] &= ~DS1341_BIT_ECLK; + + if (of_property_read_bool(client->dev.of_node, + "disable-oscillator-stop-flag")) + ds1307->regs[1] |= DS1341_BIT_DOSF; + else + ds1307->regs[1] &= ~DS1341_BIT_DOSF; + + if (of_property_read_bool(client->dev.of_node, + "enable-glitch-filter")) + ds1307->regs[0] |= DS1341_BIT_EGFIL; + else + ds1307->regs[0] &= ~DS1341_BIT_EGFIL; + + /* + * Write status register. Control register + * would be set by the code below + */ + i2c_smbus_write_byte_data(client, DS1337_REG_STATUS, + ds1307->regs[1]); + } + /* * Disable the square wave and both alarms. * For some variants, be sure alarms can trigger when we're
Add DS1341 specific power-saving options that allow to disable certain functional aspects of the chip in order to minimize its power consumption. Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com> --- .../devicetree/bindings/rtc/dallas,ds1341.txt | 23 ++++++++++++++++++ drivers/rtc/rtc-ds1307.c | 28 ++++++++++++++++++++++ 2 files changed, 51 insertions(+) create mode 100644 Documentation/devicetree/bindings/rtc/dallas,ds1341.txt