Re: [PATCH v2 1/3] sysfs: Support is_visible() on binary attributes

[Guenter Roeck]

On 09/14/2015 05:34 AM, Emilio López wrote:

According to the sysfs header file:

 "The returned value will replace static permissions defined in
  struct attribute or struct bin_attribute."

but this isn't the case, as is_visible is only called on struct attribute
only. This patch introduces a new is_bin_visible() function to implement
the same functionality for binary attributes, and updates documentation
accordingly.

Signed-off-by: Emilio López <emilio.lo...@collabora.co.uk>


Nitpick below, but otherwise looks ok to me.

Reviewed-by: Guenter Roeck <li...@roeck-us.net>

Guenter


---

Changes from v1:
  - Don't overload is_visible, introduce is_bin_visible instead as
discussed on the list.

  fs/sysfs/group.c  | 17 +++--
  include/linux/sysfs.h | 18 ++
  2 files changed, 29 insertions(+), 6 deletions(-)

diff --git a/fs/sysfs/group.c b/fs/sysfs/group.c
index 39a0199..51b56e6 100644
--- a/fs/sysfs/group.c
+++ b/fs/sysfs/group.c
@@ -73,13 +73,26 @@ static int create_files(struct kernfs_node *parent, struct 
kobject *kobj,
}

if (grp->bin_attrs) {
-   for (bin_attr = grp->bin_attrs; *bin_attr; bin_attr++) {
+   for (i = 0, bin_attr = grp->bin_attrs; *bin_attr; i++, 
bin_attr++) {
+   umode_t mode = (*bin_attr)->attr.mode;
+
if (update)
kernfs_remove_by_name(parent,
(*bin_attr)->attr.name);
+   if (grp->is_bin_visible) {
+   mode = grp->is_bin_visible(kobj, *bin_attr, i);
+   if (!mode)
+   continue;
+   }
+
+   WARN(mode & ~(SYSFS_PREALLOC | 0664),
+"Attribute %s: Invalid permissions 0%o\n",
+(*bin_attr)->attr.name, mode);
+
+   mode &= SYSFS_PREALLOC | 0664;


Strictly speaking, the mode validation for binary attributes is new and 
logically
separate. Should it be mentioned in the commit log, or even be a separate patch 
?


error = sysfs_add_file_mode_ns(parent,
&(*bin_attr)->attr, true,
-   (*bin_attr)->attr.mode, NULL);
+   mode, NULL);
if (error)
break;
}
diff --git a/include/linux/sysfs.h b/include/linux/sysfs.h
index 9f65758..2f66050 100644
--- a/include/linux/sysfs.h
+++ b/include/linux/sysfs.h
@@ -64,10 +64,18 @@ do {
\
   *a new subdirectory with this name.
   * @is_visible:   Optional: Function to return permissions associated 
with an
   *attribute of the group. Will be called repeatedly for each
- * attribute in the group. Only read/write permissions as well as
- * SYSFS_PREALLOC are accepted. Must return 0 if an attribute is
- * not visible. The returned value will replace static permissions
- * defined in struct attribute or struct bin_attribute.
+ * non-binary attribute in the group. Only read/write
+ * permissions as well as SYSFS_PREALLOC are accepted. Must
+ * return 0 if an attribute is not visible. The returned value
+ * will replace static permissions defined in struct attribute.
+ * @is_bin_visible:
+ * Optional: Function to return permissions associated with a
+ * binary attribute of the group. Will be called repeatedly
+ * for each binary attribute in the group. Only read/write
+ * permissions as well as SYSFS_PREALLOC are accepted. Must
+ * return 0 if a binary attribute is not visible. The returned
+ * value will replace static permissions defined in
+ * struct bin_attribute.
   * @attrs:Pointer to NULL terminated list of attributes.
   * @bin_attrs:Pointer to NULL terminated list of binary attributes.
   *Either attrs or bin_attrs or both must be provided.
@@ -76,6 +84,8 @@ struct attribute_group {
const char  *name;
umode_t (*is_visible)(struct kobject *,
  struct attribute *, int);
+   umode_t (*is_bin_visible)(struct kobject *,
+ struct bin_attribute *, int);
struct attribute**attrs;
struct bin_attribute**bin_attrs;
  };



--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/3] sysfs: Fix is_visible() support for binary attributes

[Guenter Roeck]

On 09/09/2015 06:14 AM, Emilio López wrote:

On 09/09/15 01:12, Guenter Roeck wrote:

On 09/08/2015 08:58 PM, Greg KH wrote:

On Tue, Sep 08, 2015 at 06:10:16PM -0700, Guenter Roeck wrote:

Hi Emilio,

On 09/08/2015 05:51 PM, Emilio López wrote:

Hi Greg & Guenter,


[ ... ]


Unless I am missing something, this is not explained anywhere,
but it is
not entirely trivial to understand. I think it should be documented.


I agree. I couldn't find any mention of what this int was supposed
to be by looking at Documentation/ (is_visible is not even mentioned
:/) or include/linux/sysfs.h. Once we settle on something I'll
document it before sending a v2.


In the include file ? No strong preference, though.


By the way, I wrote a quick coccinelle script to match is_visible()
users which reference the index (included below), and it found
references to drivers which do not seem to use any binary
attributes, so I believe changing the index meaning shouldn't be an
issue.


Good.


I agree, make i the number of the bin attribute and that should solve
this issue.


No, that would conflict with the "normal" use of is_visible for
non-binary
attributes, and make the index all but useless, since the
is_visible function
would have to search through all the attributes anyway to figure
out which one
is being checked.


Yeah, using the same indexes would be somewhat pointless, although
not many seem to be using it anyway (only 14 files matched). Others
seem to be comparing the attr* instead. An alternative would be to
use negative indexes for binary attributes and positive indexes for
normal attributes.


... and I probably wrote or reviewed a significant percentage of
those ;-).

Using negative numbers for binary attributes is an interesting idea.
Kind of unusual, though. Greg, any thoughts on that ?


Ick, no, that's a mess, maybe we just could drop the index alltogether?



No, please don't. Having to manually compare dozens of index pointers
would be
even more of a mess.


So, what about keeping it the way it is in the patch, and documenting it thoroughly? 
Otherwise, we could introduce another "is_bin_visible" function to do this same 
thing but just on binary attributes, but I'd rather not add a new function pointer if 
possible.



I would prefer to keep and document it.

Guenter

--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/3] sysfs: Fix is_visible() support for binary attributes

[Guenter Roeck]

On Tue, Sep 08, 2015 at 12:10:02PM -0700, Greg KH wrote:
> On Tue, Sep 08, 2015 at 08:30:13AM -0700, Guenter Roeck wrote:
> > Emilio,
> > 
> > On Tue, Sep 08, 2015 at 09:07:44AM -0300, Emilio López wrote:
> > > According to the sysfs header file:
> > > 
> > > "The returned value will replace static permissions defined in
> > >  struct attribute or struct bin_attribute."
> > > 
> > > but this isn't the case, as is_visible is only called on
> > > struct attribute only. This patch adds the code paths required
> > > to support is_visible() on binary attributes.
> > > 
> > > Signed-off-by: Emilio López <emilio.lo...@collabora.co.uk>
> > > ---
> > >  fs/sysfs/group.c | 22 ++
> > >  1 file changed, 18 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/fs/sysfs/group.c b/fs/sysfs/group.c
> > > index 39a0199..eb6996a 100644
> > > --- a/fs/sysfs/group.c
> > > +++ b/fs/sysfs/group.c
> > > @@ -37,10 +37,10 @@ static int create_files(struct kernfs_node *parent, 
> > > struct kobject *kobj,
> > >  {
> > >   struct attribute *const *attr;
> > >   struct bin_attribute *const *bin_attr;
> > > - int error = 0, i;
> > > + int error = 0, i = 0;
> > >  
> > >   if (grp->attrs) {
> > > - for (i = 0, attr = grp->attrs; *attr && !error; i++, attr++) {
> > > + for (attr = grp->attrs; *attr && !error; i++, attr++) {
> > >   umode_t mode = (*attr)->mode;
> > >  
> > >   /*
> > > @@ -73,13 +73,27 @@ static int create_files(struct kernfs_node *parent, 
> > > struct kobject *kobj,
> > >   }
> > >  
> > >   if (grp->bin_attrs) {
> > > - for (bin_attr = grp->bin_attrs; *bin_attr; bin_attr++) {
> > > + for (bin_attr = grp->bin_attrs; *bin_attr; i++, bin_attr++) {
> > > + umode_t mode = (*bin_attr)->attr.mode;
> > > +
> > >   if (update)
> > >   kernfs_remove_by_name(parent,
> > >   (*bin_attr)->attr.name);
> > > + if (grp->is_visible) {
> > > + mode = grp->is_visible(kobj,
> > > +&(*bin_attr)->attr, i);
> > 
> > With this, if 'n' is the number of non-binary attributes,
> > 
> > for i < n:
> > The index passed to is_visible points to a non-binary attribute.
> > for i >= n:
> > The index passed to is_visible points to the (index - n)th binary
> > attribute.
> > 
> > Unless I am missing something, this is not explained anywhere, but it is
> > not entirely trivial to understand. I think it should be documented.
> 
> I agree, make i the number of the bin attribute and that should solve
> this issue.
> 
No, that would conflict with the "normal" use of is_visible for non-binary
attributes, and make the index all but useless, since the is_visible function
would have to search through all the attributes anyway to figure out which one
is being checked.

Guenter
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/3] sysfs: Fix is_visible() support for binary attributes

[Guenter Roeck]

Emilio,

On Tue, Sep 08, 2015 at 09:07:44AM -0300, Emilio López wrote:
> According to the sysfs header file:
> 
> "The returned value will replace static permissions defined in
>  struct attribute or struct bin_attribute."
> 
> but this isn't the case, as is_visible is only called on
> struct attribute only. This patch adds the code paths required
> to support is_visible() on binary attributes.
> 
> Signed-off-by: Emilio López 
> ---
>  fs/sysfs/group.c | 22 ++
>  1 file changed, 18 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/sysfs/group.c b/fs/sysfs/group.c
> index 39a0199..eb6996a 100644
> --- a/fs/sysfs/group.c
> +++ b/fs/sysfs/group.c
> @@ -37,10 +37,10 @@ static int create_files(struct kernfs_node *parent, 
> struct kobject *kobj,
>  {
>   struct attribute *const *attr;
>   struct bin_attribute *const *bin_attr;
> - int error = 0, i;
> + int error = 0, i = 0;
>  
>   if (grp->attrs) {
> - for (i = 0, attr = grp->attrs; *attr && !error; i++, attr++) {
> + for (attr = grp->attrs; *attr && !error; i++, attr++) {
>   umode_t mode = (*attr)->mode;
>  
>   /*
> @@ -73,13 +73,27 @@ static int create_files(struct kernfs_node *parent, 
> struct kobject *kobj,
>   }
>  
>   if (grp->bin_attrs) {
> - for (bin_attr = grp->bin_attrs; *bin_attr; bin_attr++) {
> + for (bin_attr = grp->bin_attrs; *bin_attr; i++, bin_attr++) {
> + umode_t mode = (*bin_attr)->attr.mode;
> +
>   if (update)
>   kernfs_remove_by_name(parent,
>   (*bin_attr)->attr.name);
> + if (grp->is_visible) {
> + mode = grp->is_visible(kobj,
> +&(*bin_attr)->attr, i);

With this, if 'n' is the number of non-binary attributes,

for i < n:
The index passed to is_visible points to a non-binary attribute.
for i >= n:
The index passed to is_visible points to the (index - n)th binary
attribute.

Unless I am missing something, this is not explained anywhere, but it is
not entirely trivial to understand. I think it should be documented.

Also, it might be a good idea to check through existing code to ensure
that this change doesn't accidentially cause trouble (existing drivers
implementing both attribute types may not expect to see an index variable
pointing to a value larger than the number of elements in the attrs array).

Thanks,
Guenter
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/3] sysfs: Fix is_visible() support for binary attributes

[Guenter Roeck]

Hi Emilio,

On 09/08/2015 05:51 PM, Emilio López wrote:

Hi Greg & Guenter,


[ ... ]


Unless I am missing something, this is not explained anywhere, but it is
not entirely trivial to understand. I think it should be documented.


I agree. I couldn't find any mention of what this int was supposed to be by 
looking at Documentation/ (is_visible is not even mentioned :/) or 
include/linux/sysfs.h. Once we settle on something I'll document it before 
sending a v2.


In the include file ? No strong preference, though.


By the way, I wrote a quick coccinelle script to match is_visible() users which 
reference the index (included below), and it found references to drivers which 
do not seem to use any binary attributes, so I believe changing the index 
meaning shouldn't be an issue.


Good.


I agree, make i the number of the bin attribute and that should solve
this issue.


No, that would conflict with the "normal" use of is_visible for non-binary
attributes, and make the index all but useless, since the is_visible function
would have to search through all the attributes anyway to figure out which one
is being checked.


Yeah, using the same indexes would be somewhat pointless, although not many 
seem to be using it anyway (only 14 files matched). Others seem to be comparing 
the attr* instead. An alternative would be to use negative indexes for binary 
attributes and positive indexes for normal attributes.


... and I probably wrote or reviewed a significant percentage of those ;-).

Using negative numbers for binary attributes is an interesting idea.
Kind of unusual, though. Greg, any thoughts on that ?

Guenter

--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/3] sysfs: Fix is_visible() support for binary attributes

[Guenter Roeck]

On 09/08/2015 08:58 PM, Greg KH wrote:

On Tue, Sep 08, 2015 at 06:10:16PM -0700, Guenter Roeck wrote:

Hi Emilio,

On 09/08/2015 05:51 PM, Emilio López wrote:

Hi Greg & Guenter,


[ ... ]


Unless I am missing something, this is not explained anywhere, but it is
not entirely trivial to understand. I think it should be documented.


I agree. I couldn't find any mention of what this int was supposed to be by 
looking at Documentation/ (is_visible is not even mentioned :/) or 
include/linux/sysfs.h. Once we settle on something I'll document it before 
sending a v2.


In the include file ? No strong preference, though.


By the way, I wrote a quick coccinelle script to match is_visible() users which 
reference the index (included below), and it found references to drivers which 
do not seem to use any binary attributes, so I believe changing the index 
meaning shouldn't be an issue.


Good.


I agree, make i the number of the bin attribute and that should solve
this issue.


No, that would conflict with the "normal" use of is_visible for non-binary
attributes, and make the index all but useless, since the is_visible function
would have to search through all the attributes anyway to figure out which one
is being checked.


Yeah, using the same indexes would be somewhat pointless, although not many 
seem to be using it anyway (only 14 files matched). Others seem to be comparing 
the attr* instead. An alternative would be to use negative indexes for binary 
attributes and positive indexes for normal attributes.


... and I probably wrote or reviewed a significant percentage of those ;-).

Using negative numbers for binary attributes is an interesting idea.
Kind of unusual, though. Greg, any thoughts on that ?


Ick, no, that's a mess, maybe we just could drop the index alltogether?



No, please don't. Having to manually compare dozens of index pointers would be
even more of a mess.

Guenter


--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH V3] Watchdog: Fix parent of watchdog_devices

[Guenter Roeck]

On Wed, Aug 19, 2015 at 08:58:24AM +0530, Pratyush Anand wrote:
 /sys/class/watchdog/watchdogn/device/modalias can help to identify the
 driver/module for a given watchdog node. However, many wdt devices does not 
 set
 its parent and so, we do not see an entry for device in sysfs for such 
 devices.
 
 This patch fixes parent of watchdog_device so that
 /sys/class/watchdog/watchdogn/device is populated.
 
 Exceptions: booke, diag288, mpc8xxx, octeon, softdog and w83627hf -- They do 
 not
 have any parent. Not sure, how we can we identify driver for these devices.
 
If you happen to send out another revision, please reduce the number of columns
in the description to 75 or less to avoid the checkpatch warning.
Also, it would help if you can add all the Reviewed-by: and Acked-by: tags.

 Signed-off-by: Pratyush Anand pan...@redhat.com

Passes my build and qemu tests, and the kbuild test robot is happy this time.

Acked-by: Guenter Roeck li...@roeck-us.net

Guenter

 ---
 Changes since v2:
 Fixed drivers/watchdog/txx9wdt.c:134:20: error: 'pdev' undeclared
 Reported-by: kbuild test robot fengguang...@intel.com
 Changes since v1:
 Squash all commits of V1 into a single patch.
 V1 was here:
 http://www.spinics.net/lists/linux-watchdog/msg07220.html
 
  drivers/misc/mei/wd.c   | 1 +
  drivers/watchdog/bcm2835_wdt.c  | 1 +
  drivers/watchdog/bcm47xx_wdt.c  | 1 +
  drivers/watchdog/bcm_kona_wdt.c | 1 +
  drivers/watchdog/coh901327_wdt.c| 1 +
  drivers/watchdog/da9052_wdt.c   | 1 +
  drivers/watchdog/da9055_wdt.c   | 1 +
  drivers/watchdog/da9062_wdt.c   | 1 +
  drivers/watchdog/da9063_wdt.c   | 1 +
  drivers/watchdog/davinci_wdt.c  | 1 +
  drivers/watchdog/digicolor_wdt.c| 1 +
  drivers/watchdog/ep93xx_wdt.c   | 1 +
  drivers/watchdog/gpio_wdt.c | 1 +
  drivers/watchdog/ie6xx_wdt.c| 1 +
  drivers/watchdog/intel-mid_wdt.c| 1 +
  drivers/watchdog/jz4740_wdt.c   | 1 +
  drivers/watchdog/mena21_wdt.c   | 1 +
  drivers/watchdog/menf21bmc_wdt.c| 1 +
  drivers/watchdog/omap_wdt.c | 1 +
  drivers/watchdog/orion_wdt.c| 1 +
  drivers/watchdog/pnx4008_wdt.c  | 1 +
  drivers/watchdog/qcom-wdt.c | 1 +
  drivers/watchdog/retu_wdt.c | 1 +
  drivers/watchdog/rt2880_wdt.c   | 1 +
  drivers/watchdog/s3c2410_wdt.c  | 1 +
  drivers/watchdog/shwdt.c| 1 +
  drivers/watchdog/sirfsoc_wdt.c  | 1 +
  drivers/watchdog/sp805_wdt.c| 1 +
  drivers/watchdog/st_lpc_wdt.c   | 1 +
  drivers/watchdog/stmp3xxx_rtc_wdt.c | 1 +
  drivers/watchdog/tegra_wdt.c| 1 +
  drivers/watchdog/twl4030_wdt.c  | 1 +
  drivers/watchdog/txx9wdt.c  | 1 +
  drivers/watchdog/ux500_wdt.c| 1 +
  drivers/watchdog/via_wdt.c  | 1 +
  drivers/watchdog/wm831x_wdt.c   | 1 +
  drivers/watchdog/wm8350_wdt.c   | 1 +
  37 files changed, 37 insertions(+)
 
 diff --git a/drivers/misc/mei/wd.c b/drivers/misc/mei/wd.c
 index 2bc0f5089f82..b346638833b0 100644
 --- a/drivers/misc/mei/wd.c
 +++ b/drivers/misc/mei/wd.c
 @@ -364,6 +364,7 @@ int mei_watchdog_register(struct mei_device *dev)
  
   int ret;
  
 + amt_wd_dev.parent = dev-dev;
   /* unlock to perserve correct locking order */
   mutex_unlock(dev-device_lock);
   ret = watchdog_register_device(amt_wd_dev);
 diff --git a/drivers/watchdog/bcm2835_wdt.c b/drivers/watchdog/bcm2835_wdt.c
 index 7116968dee12..66c3e656a616 100644
 --- a/drivers/watchdog/bcm2835_wdt.c
 +++ b/drivers/watchdog/bcm2835_wdt.c
 @@ -182,6 +182,7 @@ static int bcm2835_wdt_probe(struct platform_device *pdev)
   watchdog_set_drvdata(bcm2835_wdt_wdd, wdt);
   watchdog_init_timeout(bcm2835_wdt_wdd, heartbeat, dev);
   watchdog_set_nowayout(bcm2835_wdt_wdd, nowayout);
 + bcm2835_wdt_wdd.parent = pdev-dev;
   err = watchdog_register_device(bcm2835_wdt_wdd);
   if (err) {
   dev_err(dev, Failed to register watchdog device);
 diff --git a/drivers/watchdog/bcm47xx_wdt.c b/drivers/watchdog/bcm47xx_wdt.c
 index b28a072abf78..4064a43f1360 100644
 --- a/drivers/watchdog/bcm47xx_wdt.c
 +++ b/drivers/watchdog/bcm47xx_wdt.c
 @@ -209,6 +209,7 @@ static int bcm47xx_wdt_probe(struct platform_device *pdev)
  
   wdt-wdd.info = bcm47xx_wdt_info;
   wdt-wdd.timeout = WDT_DEFAULT_TIME;
 + wdt-wdd.parent = pdev-dev;
   ret = wdt-wdd.ops-set_timeout(wdt-wdd, timeout);
   if (ret)
   goto err_timer;
 diff --git a/drivers/watchdog/bcm_kona_wdt.c b/drivers/watchdog/bcm_kona_wdt.c
 index 22d8ae65772a..e0c98423f2c9 100644
 --- a/drivers/watchdog/bcm_kona_wdt.c
 +++ b/drivers/watchdog/bcm_kona_wdt.c
 @@ -319,6 +319,7 @@ static int bcm_kona_wdt_probe(struct platform_device 
 *pdev)
   spin_lock_init(wdt-lock);
   platform_set_drvdata(pdev, wdt);
   watchdog_set_drvdata(bcm_kona_wdt_wdd, wdt);
 + bcm_kona_wdt_wdd.parent = pdev-dev;
  
   ret = bcm_kona_wdt_set_timeout_reg

Re: [PATCH v2] thermal: consistently use int for temperatures

[Guenter Roeck]

On 07/24/2015 03:11 PM, Pavel Machek wrote:

On Fri 2015-07-24 06:59:26, Guenter Roeck wrote:

On 07/23/2015 11:29 PM, Sascha Hauer wrote:

On Thu, Jul 23, 2015 at 02:07:59PM +0200, Pavel Machek wrote:

On Tue 2015-07-21 09:21:32, Sascha Hauer wrote:

The thermal code uses int, long and unsigned long for temperatures
in different places.

Using an unsigned type limits the thermal framework to positive
temperatures without need. Also several drivers currently will report
temperatures near UINT_MAX for temperatures below 0°C. This will probably
immediately shut the machine down due to overtemperature if started below
0°C.

'long' is 64bit on several architectures. This is not needed since INT_MAX °mC
is above the melting point of all known materials.


Can we do something like

typedef millicelsius_t int;

...to document the units?


I am not very fond of typedefs and I am not sure this adds any value. I
could change it when more people ask for it, but I just sent the new
version without this.



I thought we are supposed to not introduce new typedefs anyway.


You are not supposed to typedef struct, but typedef for millicelsius_t
would be ok. And it is your only chance if you want people to pay
attention. If you make it int, someone will pass it to long or
something else..


Seems to me that would be just lazyness. The same person might use 'long'
even if millicelsius_t is defined. A typedef doesn't preclude people
from ignoring it.

Guenter

--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] thermal: consistently use int for temperatures

[Guenter Roeck]

On 07/23/2015 11:29 PM, Sascha Hauer wrote:

On Thu, Jul 23, 2015 at 02:07:59PM +0200, Pavel Machek wrote:

On Tue 2015-07-21 09:21:32, Sascha Hauer wrote:

The thermal code uses int, long and unsigned long for temperatures
in different places.

Using an unsigned type limits the thermal framework to positive
temperatures without need. Also several drivers currently will report
temperatures near UINT_MAX for temperatures below 0°C. This will probably
immediately shut the machine down due to overtemperature if started below
0°C.

'long' is 64bit on several architectures. This is not needed since INT_MAX °mC
is above the melting point of all known materials.


Can we do something like

typedef millicelsius_t int;

...to document the units?


I am not very fond of typedefs and I am not sure this adds any value. I
could change it when more people ask for it, but I just sent the new
version without this.



I thought we are supposed to not introduce new typedefs anyway.

Guenter


--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 6/6] hwmon: pwm-fan: Update the duty cycle inorder to control the pwm-fan

[Guenter Roeck]

On 04/12/2015 11:44 AM, Anand Moon wrote:

pwm_config() must be called with a duty cycle of 0 prior to calling
pwm_disable() to ensure that the pwm signal is set to low.

Changes since v1 : None.
Changes since v2 : None
Changes since v3 : Simplify the comment.

Reported-by: Markus Reichl m.rei...@fivetechno.de
Tested-by: Markus Reichl m.rei...@fivetechno.de
Reviewed-by: Lukasz Majewski l.majew...@samsung.com
Reviewed-by: Sjoerd Simons sjoerd.sim...@collabora.co.uk
Signed-off-by: Anand Moon linux.am...@gmail.com


Applied to hwmon-next (after removing the changelog from
the commit message).

Thanks,
Guenter

--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 6/6] hwmon: pwm-fan: Update the duty cycle inorder to control the pwm-fan

[Guenter Roeck]

On 04/12/2015 07:54 AM, Anand Moon wrote:

In order to disable the PWM we need to update using following sequence.

pwm_config(pwm, 0, period);
pwm_disable(pwm);

pwm_config() with a zero duty cycle to make it clear the timer and update the 
PWM registers.
pwm_disable will clear the TCON_AUTORELOAD(tcon_chan) and update the PWM 
register.


There is no TCON_AUTORELOAD in this driver. Future developers will have no idea
what you are talking about here. Please provide a generic comment.


Next change in state will get trigger unless a new PWM cycle happened.


That sentence is difficult to parse. Actually, I have no idea what it is 
supposed to mean.


pwm_config(pwm, duty, period);
pwm_enable(pwm);

Through pwm_config we update the duty cycle and period and update the PWM 
register.
pwm_enable will update the state to TCON_MANUALUPDATE(tcon_chan)
and TCON_START(tcon_chan) | TCON_AUTORELOAD(tcon_chan) and update the PWM 
register.


This sentence does not make sense in the context of the pwm-fan driver.


Reported-by: Markus Reichl m.rei...@fivetechno.de
Tested-by: Markus Reichl m.rei...@fivetechno.de
Reviewed-by: Lukasz Majewski l.majew...@samsung.com
Reviewed-by: Sjoerd Simons sjoerd.sim...@collabora.co.uk
Signed-off-by: Anand Moon linux.am...@gmail.com
---
  drivers/hwmon/pwm-fan.c | 10 --
  1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/drivers/hwmon/pwm-fan.c b/drivers/hwmon/pwm-fan.c
index 7c83dc4..f25c841 100644
--- a/drivers/hwmon/pwm-fan.c
+++ b/drivers/hwmon/pwm-fan.c
@@ -44,26 +44,24 @@ static int  __set_pwm(struct pwm_fan_ctx *ctx, unsigned 
long pwm)
int ret = 0;

mutex_lock(ctx-lock);
+


Please refrain from making unnecessary whitespace changes.

Thanks,
Guenter

--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 6/6] hwmon: pwm-fan: Update the duty cycle inorder to control the pwm-fan

[Guenter Roeck]

On 04/12/2015 08:29 AM, Anand Moon wrote:

hi Guenter,

I am referring to #linux/drivers/pwm/pwm-samsung.c

Will update the comment.


Sure, but that doesn't help as patch description for the pwm-fan driver.

Guenter


-Anand Moon

On 12 April 2015 at 20:37, Guenter Roeck li...@roeck-us.net wrote:

On 04/12/2015 07:54 AM, Anand Moon wrote:


In order to disable the PWM we need to update using following sequence.

 pwm_config(pwm, 0, period);
 pwm_disable(pwm);

pwm_config() with a zero duty cycle to make it clear the timer and update
the PWM registers.
pwm_disable will clear the TCON_AUTORELOAD(tcon_chan) and update the PWM
register.


There is no TCON_AUTORELOAD in this driver. Future developers will have no
idea
what you are talking about here. Please provide a generic comment.


Next change in state will get trigger unless a new PWM cycle happened.


That sentence is difficult to parse. Actually, I have no idea what it is
supposed to mean.


 pwm_config(pwm, duty, period);
 pwm_enable(pwm);

Through pwm_config we update the duty cycle and period and update the PWM
register.
pwm_enable will update the state to TCON_MANUALUPDATE(tcon_chan)
and TCON_START(tcon_chan) | TCON_AUTORELOAD(tcon_chan) and update the PWM
register.


This sentence does not make sense in the context of the pwm-fan driver.


Reported-by: Markus Reichl m.rei...@fivetechno.de
Tested-by: Markus Reichl m.rei...@fivetechno.de
Reviewed-by: Lukasz Majewski l.majew...@samsung.com
Reviewed-by: Sjoerd Simons sjoerd.sim...@collabora.co.uk
Signed-off-by: Anand Moon linux.am...@gmail.com
---
   drivers/hwmon/pwm-fan.c | 10 --
   1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/drivers/hwmon/pwm-fan.c b/drivers/hwmon/pwm-fan.c
index 7c83dc4..f25c841 100644
--- a/drivers/hwmon/pwm-fan.c
+++ b/drivers/hwmon/pwm-fan.c
@@ -44,26 +44,24 @@ static int  __set_pwm(struct pwm_fan_ctx *ctx,
unsigned long pwm)
 int ret = 0;

 mutex_lock(ctx-lock);
+



Please refrain from making unnecessary whitespace changes.

Thanks,
Guenter





--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 6/6] hwmon: pwm-fan: Update the duty cycle inorder to control the pwm-fan

[Guenter Roeck]

On 04/08/2015 01:44 AM, Lukasz Majewski wrote:

Hi Anand,


Below changes depend on following patch.
https://patchwork.kernel.org/patch/5944061/

Update the pwm_config with duty then update the pwm_disable
to poweroff the cpu fan.

Tested on OdroidXU3 board.

Signed-off-by: Anand Moon linux.am...@gmail.com
---
  drivers/hwmon/pwm-fan.c | 10 --
  1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/drivers/hwmon/pwm-fan.c b/drivers/hwmon/pwm-fan.c
index 7c83dc4..f25c841 100644
--- a/drivers/hwmon/pwm-fan.c
+++ b/drivers/hwmon/pwm-fan.c
@@ -44,26 +44,24 @@ static int  __set_pwm(struct pwm_fan_ctx *ctx,
unsigned long pwm) int ret = 0;

mutex_lock(ctx-lock);
+
if (ctx-pwm_value == pwm)
goto exit_set_pwm_err;

-   if (pwm == 0) {
-   pwm_disable(ctx-pwm);
-   goto exit_set_pwm;
-   }
-
duty = DIV_ROUND_UP(pwm * (ctx-pwm-period - 1), MAX_PWM);
ret = pwm_config(ctx-pwm, duty, ctx-pwm-period);
if (ret)
goto exit_set_pwm_err;

+   if (pwm == 0)
+   pwm_disable(ctx-pwm);
+
if (ctx-pwm_value == 0) {
ret = pwm_enable(ctx-pwm);
if (ret)
goto exit_set_pwm_err;
}

-exit_set_pwm:
ctx-pwm_value = pwm;
  exit_set_pwm_err:
mutex_unlock(ctx-lock);


Reviewed-by: Lukasz Majewski l.majew...@samsung.com

BTW: I've added Guenter to CC.


I don't have a copy of the original patch, so I can't apply it.

Guenter

--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 6/6] hwmon: pwm-fan: Update the duty cycle inorder to control the pwm-fan

[Guenter Roeck]

On Wed, Apr 08, 2015 at 10:44:15AM +0200, Lukasz Majewski wrote:
 Hi Anand,
 
  Below changes depend on following patch.
  https://patchwork.kernel.org/patch/5944061/
  
  Update the pwm_config with duty then update the pwm_disable
  to poweroff the cpu fan.
  

Unfortunately, the patch does not include an explanation why it is needed.

The original code presumably did not update the duty cycle because
pwm was about to be disabled anyway. That kind of made sense to me.
Updating the duty cycle to 0 just to disable the pwm channel right
afterwards does not immediately make sense.

Given that, I would expect to see a rationale here. Why is this patch needed ?
Does it fix a bug ? If yes, pelase describe the bug. If not, what is the
purpose of this patch ?

Maybe that is all explained in patch 0/6, which I was not copied on. Even
if so, the reationale will be needed in the changelog to explain to future
developers why this change was made.

Thanks,
Guenter

  Tested on OdroidXU3 board.
  
  Signed-off-by: Anand Moon linux.am...@gmail.com
  ---
   drivers/hwmon/pwm-fan.c | 10 --
   1 file changed, 4 insertions(+), 6 deletions(-)
  
  diff --git a/drivers/hwmon/pwm-fan.c b/drivers/hwmon/pwm-fan.c
  index 7c83dc4..f25c841 100644
  --- a/drivers/hwmon/pwm-fan.c
  +++ b/drivers/hwmon/pwm-fan.c
  @@ -44,26 +44,24 @@ static int  __set_pwm(struct pwm_fan_ctx *ctx,
  unsigned long pwm) int ret = 0;
   
  mutex_lock(ctx-lock);
  +

[ please refrain from unnecessary whitespace changes ]

  if (ctx-pwm_value == pwm)
  goto exit_set_pwm_err;
   
  -   if (pwm == 0) {
  -   pwm_disable(ctx-pwm);
  -   goto exit_set_pwm;
  -   }
  -
  duty = DIV_ROUND_UP(pwm * (ctx-pwm-period - 1), MAX_PWM);
  ret = pwm_config(ctx-pwm, duty, ctx-pwm-period);
  if (ret)
  goto exit_set_pwm_err;
   
  +   if (pwm == 0)
  +   pwm_disable(ctx-pwm);
  +
  if (ctx-pwm_value == 0) {
  ret = pwm_enable(ctx-pwm);
  if (ret)
  goto exit_set_pwm_err;
  }
   
  -exit_set_pwm:
  ctx-pwm_value = pwm;
   exit_set_pwm_err:
  mutex_unlock(ctx-lock);
 
 Reviewed-by: Lukasz Majewski l.majew...@samsung.com
 
 BTW: I've added Guenter to CC.
 
 -- 
 Best regards,
 
 Lukasz Majewski
 
 Samsung RD Institute Poland (SRPOL) | Linux Platform Group
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 6/6] hwmon: pwm-fan: Update the duty cycle inorder to control the pwm-fan

[Guenter Roeck]

On Wed, Apr 08, 2015 at 09:32:05PM +0530, Anand Moon wrote:
 Hi Guenter,
 
 Initially the board bootup the cooling level state is 0.
 So update the duty cycle and this power off the fan.
 As their is no state change the fan will not spin.
 
 Once the temperature sensor is reached to alert temperature it changes state.
 With the state change the fan cools the CPU and then stop's
 
 I have observed this state change with tmon utility in 
 linux/tools/thermal/tmon/
 
Sorry, I am missing something. I still don't see what problem you are fixing
with this patch. What behavior is wrong with the current code, and how does your
patch fix it ?

Guenter

 -Anand Moon
 
 On 8 April 2015 at 21:02, Guenter Roeck li...@roeck-us.net wrote:
  On Wed, Apr 08, 2015 at 10:44:15AM +0200, Lukasz Majewski wrote:
  Hi Anand,
 
   Below changes depend on following patch.
   https://patchwork.kernel.org/patch/5944061/
  
   Update the pwm_config with duty then update the pwm_disable
   to poweroff the cpu fan.
  
 
  Unfortunately, the patch does not include an explanation why it is needed.
 
  The original code presumably did not update the duty cycle because
  pwm was about to be disabled anyway. That kind of made sense to me.
  Updating the duty cycle to 0 just to disable the pwm channel right
  afterwards does not immediately make sense.
 
  Given that, I would expect to see a rationale here. Why is this patch 
  needed ?
  Does it fix a bug ? If yes, pelase describe the bug. If not, what is the
  purpose of this patch ?
 
  Maybe that is all explained in patch 0/6, which I was not copied on. Even
  if so, the reationale will be needed in the changelog to explain to future
  developers why this change was made.
 
  Thanks,
  Guenter
 
   Tested on OdroidXU3 board.
  
   Signed-off-by: Anand Moon linux.am...@gmail.com
   ---
drivers/hwmon/pwm-fan.c | 10 --
1 file changed, 4 insertions(+), 6 deletions(-)
  
   diff --git a/drivers/hwmon/pwm-fan.c b/drivers/hwmon/pwm-fan.c
   index 7c83dc4..f25c841 100644
   --- a/drivers/hwmon/pwm-fan.c
   +++ b/drivers/hwmon/pwm-fan.c
   @@ -44,26 +44,24 @@ static int  __set_pwm(struct pwm_fan_ctx *ctx,
   unsigned long pwm) int ret = 0;
  
   mutex_lock(ctx-lock);
   +
 
  [ please refrain from unnecessary whitespace changes ]
 
   if (ctx-pwm_value == pwm)
   goto exit_set_pwm_err;
  
   -   if (pwm == 0) {
   -   pwm_disable(ctx-pwm);
   -   goto exit_set_pwm;
   -   }
   -
   duty = DIV_ROUND_UP(pwm * (ctx-pwm-period - 1), MAX_PWM);
   ret = pwm_config(ctx-pwm, duty, ctx-pwm-period);
   if (ret)
   goto exit_set_pwm_err;
  
   +   if (pwm == 0)
   +   pwm_disable(ctx-pwm);
   +
   if (ctx-pwm_value == 0) {
   ret = pwm_enable(ctx-pwm);
   if (ret)
   goto exit_set_pwm_err;
   }
  
   -exit_set_pwm:
   ctx-pwm_value = pwm;
exit_set_pwm_err:
   mutex_unlock(ctx-lock);
 
  Reviewed-by: Lukasz Majewski l.majew...@samsung.com
 
  BTW: I've added Guenter to CC.
 
  --
  Best regards,
 
  Lukasz Majewski
 
  Samsung RD Institute Poland (SRPOL) | Linux Platform Group
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v6 6/6] hwmon: pwm-fan: Code for using PWM FAN as a cooling device

[Guenter Roeck]

On 02/26/2015 05:59 AM, Lukasz Majewski wrote:

The PWM FAN device can now be used as a thermal cooling device. Necessary
infrastructure has been added in this commit.

Signed-off-by: Lukasz Majewski l.majew...@samsung.com
Acked-by: Eduardo Valentin edubez...@gmail.com
---
Changes for v2:
- Replace pwm_fan_cooling_states with pwm_fan_cooling_levels
- Update ctx-pwm_fan_state when correct data from device tree is available
- Using therma_cdev_update() when thermal is ready for controlling the fan
Changes for v3:
- Rename patch heading
- pwm_fan_of_get_cooling_data() now returns -EINVAL when no cooling-levels
   property defined
- register of cooling device only when proper cooling data is present
Changes for v4:
- None
Changes for v5:
- Check for IS_ENABLED(CONFIG_THERMAL) has been added to prevent from
   executing thermal_* specific functions
Changes for v6:
- Adding missing ctx == NULL check in pwm_fan_get_max_state()
- Call to thermal_cooling_device_unregister(ctx-cdev); at pwm_fan_remove()
- struct thermal_cooling_device *cdev; added to struct pwm_fan_ctx
---
  drivers/hwmon/pwm-fan.c | 89 -
  1 file changed, 88 insertions(+), 1 deletion(-)

[ ... ]


@@ -200,6 +286,7 @@ static int pwm_fan_remove(struct platform_device *pdev)
  {
struct pwm_fan_ctx *ctx = platform_get_drvdata(pdev);

+   thermal_cooling_device_unregister(ctx-cdev);


Unfortunately there is still no prototype for this if CONFIG_THERMAL
is not configured.

Two options: Yet another revision, or wait a week until the prototypes
are (hopefully) available and submit a patch without the conditionals.

Guenter

--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v5 0/6] hwmon: thermal: Odroid U3: Provide support for Odroid U3 fan

[Guenter Roeck]

On Wed, Feb 25, 2015 at 04:34:16PM +0100, Lukasz Majewski wrote:
 Presented patches add support for Odroid's U3 optional CPU FAN, which uses PWM
 subsystem for low level control.
 
 After successful probe it registers itself as a cooling device for thermal
 subsystem.
 
 This driver also supports devices without DTS specified.
 
 To provide correct functionality, new properties to device tree description 
 for
 Exynos4412 and in particular Odroid U3 have been added.
 
 Those patches were tested at Exynos4412 - Odroid U3 board.
 
 Patches were applied on:
 linux-soc-thermal/fixes branch (Linux v4.0-rc1)
 SHA1: b71d399c7f2fe06b60b96155ec0b9ae167334e4a
 
 Kamil Debski (1):
   ARM: dts: Add pwm-fan node to the Odroid-U3 board
 
 Lukasz Majewski (5):
   Documentation: dts: Documentation entry to explain how to use PWM FAN
 as a cooling device
   ARM: dts: Add properties to use pwm-fan device as a cooling device in
 Odroid U3
   hwmon: pwm-fan: Extract __set_pwm() function to only modify PWM duty
 cycle
   hwmon: pwm-fan: Read PWM FAN configuration from device tree
   hwmon: pwm-fan: Code for using PWM FAN as a cooling device
 
  .../devicetree/bindings/hwmon/pwm-fan.txt  |  25 +++-
  arch/arm/boot/dts/exynos4.dtsi |   2 +-
  arch/arm/boot/dts/exynos4412-odroidu3.dts  |  43 ++
  drivers/hwmon/pwm-fan.c| 166 
 +++--
  4 files changed, 220 insertions(+), 16 deletions(-)
 
For the series:

Acked-by: Guenter Roeck li...@roeck-us.net

Should I take it through hwmon ? Might make sense given the majority
of the changes is in hwmon code.

Thanks,
Guenter
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v5 0/6] hwmon: thermal: Odroid U3: Provide support for Odroid U3 fan

[Guenter Roeck]

On Wed, Feb 25, 2015 at 02:29:18PM -0400, Eduardo Valentin wrote:
 Guenter,
 
 On Wed, Feb 25, 2015 at 09:18:20AM -0800, Guenter Roeck wrote:
  On Wed, Feb 25, 2015 at 04:34:16PM +0100, Lukasz Majewski wrote:
   Presented patches add support for Odroid's U3 optional CPU FAN, which 
   uses PWM
   subsystem for low level control.
   
   After successful probe it registers itself as a cooling device for thermal
   subsystem.
   
   This driver also supports devices without DTS specified.
   
   To provide correct functionality, new properties to device tree 
   description for
   Exynos4412 and in particular Odroid U3 have been added.
   
   Those patches were tested at Exynos4412 - Odroid U3 board.
   
   Patches were applied on:
   linux-soc-thermal/fixes branch (Linux v4.0-rc1)
   SHA1: b71d399c7f2fe06b60b96155ec0b9ae167334e4a
   
   Kamil Debski (1):
 ARM: dts: Add pwm-fan node to the Odroid-U3 board
   
   Lukasz Majewski (5):
 Documentation: dts: Documentation entry to explain how to use PWM FAN
   as a cooling device
 ARM: dts: Add properties to use pwm-fan device as a cooling device in
   Odroid U3
 hwmon: pwm-fan: Extract __set_pwm() function to only modify PWM duty
   cycle
 hwmon: pwm-fan: Read PWM FAN configuration from device tree
 hwmon: pwm-fan: Code for using PWM FAN as a cooling device
   
.../devicetree/bindings/hwmon/pwm-fan.txt  |  25 +++-
arch/arm/boot/dts/exynos4.dtsi |   2 +-
arch/arm/boot/dts/exynos4412-odroidu3.dts  |  43 ++
drivers/hwmon/pwm-fan.c| 166 
   +++--
4 files changed, 220 insertions(+), 16 deletions(-)
   
  For the series:
  
  Acked-by: Guenter Roeck li...@roeck-us.net
  
  Should I take it through hwmon ? Might make sense given the majority
  of the changes is in hwmon code.
 
 I believe the series should go via your tree, yes. I had only minor
 comments in the code added for the cooling device code, as it lacks the
 unregistration call in the .remove callback. 
 
 Also, the DTS changes may generate conflicts with platform code. Lukasz
 may probably ask  Kukjin Kim to add them via the samsung tree.
 
Ok, I'll wait for the updated patch and then add the hwmon parts to hwmon-next.

Thanks,
Guenter
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 7/8] hwmon: pwm-fan: Read PWM FAN configuration from device tree

[Guenter Roeck]

On Mon, Feb 23, 2015 at 05:51:22PM +0100, Lukasz Majewski wrote:
 Hi Guenter,
 
  On Mon, Feb 23, 2015 at 05:13:36PM +0100, Lukasz Majewski wrote:
   Hi Guenter,
   
  [ ... ]
  

If devicetree is not configured, of_property_count_elems_of_size
returns -ENOSYS, which is returned, causing the driver to fail
loading.
   
   Has of_property_count_elems_of_size() returns -ENOSYS?
   
   Maybe something has changed, but in my linux-vanila (3.19-rc4)
   at ./drivers/of/base.c it returns -EINVAL, -ENODATA or number of
   elements.
   
   Have I missed something?
   
  Hi Lukasz,
  
  Yes, you have. Check include/linux/of.h, line 484, in latest mainline.
 
 Ok. Now I got it.
 
 The above situation shouldn't happen if I put of_find_property() check
 on the very beginning of this function (it returns NULL when DT support
 is not compiled).
 

Correct.

 The function would look as follows:
 
 int 
 pwm_fan_of_get_cooling_data(struct device *dev, struct pwm_fan_ctx
 *ctx) 
 {   
 struct device_node *np = dev-of_node;
   int num, i, ret;
 
   if (!of_find_property(np, cooling-levels, NULL))
   return 0;
 
   ret = of_property_count_u32_elems(np, cooling-levels);
   if (ret = 0) {
   dev_err(dev, Wrong data!\n);
   return ret;

This should probably be something like 

return ret ? : -EINVAL;

or ret == 0 is not an error, and you should not display an error message
in that case.

Thanks,
Guenter
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 7/8] hwmon: pwm-fan: Read PWM FAN configuration from device tree

[Guenter Roeck]

On Mon, Feb 23, 2015 at 05:13:36PM +0100, Lukasz Majewski wrote:
 Hi Guenter,
 
[ ... ]

  
  If devicetree is not configured, of_property_count_elems_of_size
  returns -ENOSYS, which is returned, causing the driver to fail
  loading.
 
 Has of_property_count_elems_of_size() returns -ENOSYS?
 
 Maybe something has changed, but in my linux-vanila (3.19-rc4)
 at ./drivers/of/base.c it returns -EINVAL, -ENODATA or number of
 elements.
 
 Have I missed something?
 
Hi Lukasz,

Yes, you have. Check include/linux/of.h, line 484, in latest mainline.

Guenter
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 7/8] hwmon: pwm-fan: Read PWM FAN configuration from device tree

[Guenter Roeck]

On 02/18/2015 02:07 AM, Lukasz Majewski wrote:

This patch provides code for reading PWM FAN configuration data via
device tree. The pwm-fan can work with full speed when configuration
is not provided. However, errors are propagated when wrong DT bindings
are found.
Additionally the struct pwm_fan_ctx has been extended.

Signed-off-by: Lukasz Majewski l.majew...@samsung.com
---
Changes for v2:
- Rename pwm_fan_max_states to pwm_fan_cooling_levels
- Moving pwm_fan_of_get_cooling_data() call after setting end enabling PWM FAN
- pwm_fan_of_get_cooling_data() now can fail - preserving old behaviour
- Remove unnecessary dev_err() call
Changes for v3:
- Patch's headline has been reedited
- pwm_fan_of_get_cooling_data() return code is now being checked.
- of_property_count_elems_of_size() is now used instead of_find_property()
- More verbose patch description added
Changes for v4:
- dev_err() has been removed from pwm_fan_of_get_cooling_data()
- Returning -EINVAL when cooling-levels are defined in DT, but doesn't
   have the value
---
  drivers/hwmon/pwm-fan.c | 52 -
  1 file changed, 51 insertions(+), 1 deletion(-)

diff --git a/drivers/hwmon/pwm-fan.c b/drivers/hwmon/pwm-fan.c
index bd42d39..82cd06a 100644
--- a/drivers/hwmon/pwm-fan.c
+++ b/drivers/hwmon/pwm-fan.c
@@ -30,7 +30,10 @@
  struct pwm_fan_ctx {
struct mutex lock;
struct pwm_device *pwm;
-   unsigned char pwm_value;
+   unsigned int pwm_value;
+   unsigned int pwm_fan_state;
+   unsigned int pwm_fan_max_state;
+   unsigned int *pwm_fan_cooling_levels;
  };

  static int  __set_pwm(struct pwm_fan_ctx *ctx, unsigned long pwm)
@@ -100,6 +103,48 @@ static struct attribute *pwm_fan_attrs[] = {

  ATTRIBUTE_GROUPS(pwm_fan);

+int pwm_fan_of_get_cooling_data(struct device *dev, struct pwm_fan_ctx *ctx)
+{
+   struct device_node *np = dev-of_node;
+   int num, i, ret;
+
+   ret = of_property_count_elems_of_size(np, cooling-levels,
+ sizeof(u32));
+
+   if (ret == -EINVAL)
+   return 0;


The function returns -EINVAL if there is no such property,
but also if prop-length % elem_size != 0. The latter _would_
be an error.

Overall I don't entirely understand why you do not call
of_find_property first. If that returns NULL, you would know for sure
that the property does not exist, and you would not have to second
guess the returned error from of_property_count_elems_of_size.

On a side note, there is of_property_count_u32_elems() to count
properties of size u32.


+
+   if (ret = 0) {
+   dev_err(dev, Wrong data!\n);
+   return ret ? ret : -EINVAL;
+   }


If devicetree is not configured, of_property_count_elems_of_size
returns -ENOSYS, which is returned, causing the driver to fail loading.


+
+   num = ret;
+   ctx-pwm_fan_cooling_levels = devm_kzalloc(dev, num * sizeof(u32),
+  GFP_KERNEL);
+   if (!ctx-pwm_fan_cooling_levels)
+   return -ENOMEM;
+
+   ret = of_property_read_u32_array(np, cooling-levels,
+ctx-pwm_fan_cooling_levels, num);
+   if (ret) {
+   dev_err(dev, Property 'cooling-levels' cannot be read!\n);
+   return ret;
+   }
+
+   for (i = 0; i  num; i++) {
+   if (ctx-pwm_fan_cooling_levels[i]  MAX_PWM) {
+   dev_err(dev, PWM fan state[%d]:%d  %d\n, i,
+   ctx-pwm_fan_cooling_levels[i], MAX_PWM);
+   return -EINVAL;
+   }
+   }
+
+   ctx-pwm_fan_max_state = num - 1;
+
+   return 0;
+}
+
  static int pwm_fan_probe(struct platform_device *pdev)
  {
struct device *hwmon;
@@ -145,6 +190,11 @@ static int pwm_fan_probe(struct platform_device *pdev)
pwm_disable(ctx-pwm);
return PTR_ERR(hwmon);
}
+
+   ret = pwm_fan_of_get_cooling_data(pdev-dev, ctx);
+   if (ret)
+   return ret;
+
return 0;
  }




--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 7/8] hwmon: pwm-fan: Read PWM FAN configuration from device tree

[Guenter Roeck]

On 02/08/2015 01:36 PM, Lukasz Majewski wrote:

On Fri, 6 Feb 2015 10:36:57 -0800
Guenter Roeck li...@roeck-us.net wrote:


On Fri, Feb 06, 2015 at 05:59:07PM +0100, Lukasz Majewski wrote:

This patch provides code for reading PWM FAN configuration data via
device tree. The pwm-fan can work with full speed when configuration
is not provided. However, errors are propagated when wrong DT
bindings are found.
Additionally the struct pwm_fan_ctx has been extended.

Signed-off-by: Lukasz Majewski l.majew...@samsung.com
---
Changes for v2:
- Rename pwm_fan_max_states to pwm_fan_cooling_levels
- Moving pwm_fan_of_get_cooling_data() call after setting end
enabling PWM FAN
- pwm_fan_of_get_cooling_data() now can fail - preserving old
behaviour
- Remove unnecessary dev_err() call
Changes for v3:
- Patch's headline has been reedited
- pwm_fan_of_get_cooling_data() return code is now being checked.
- of_property_count_elems_of_size() is now used instead
of_find_property()
- More verbose patch description added
---
  drivers/hwmon/pwm-fan.c | 54
- 1 file changed,
53 insertions(+), 1 deletion(-)

diff --git a/drivers/hwmon/pwm-fan.c b/drivers/hwmon/pwm-fan.c
index 870e100..f3f5843 100644
--- a/drivers/hwmon/pwm-fan.c
+++ b/drivers/hwmon/pwm-fan.c
@@ -30,7 +30,10 @@
  struct pwm_fan_ctx {
struct mutex lock;
struct pwm_device *pwm;
-   unsigned char pwm_value;
+   unsigned int pwm_value;
+   unsigned int pwm_fan_state;
+   unsigned int pwm_fan_max_state;
+   unsigned int *pwm_fan_cooling_levels;
  };

  static int  __set_pwm(struct pwm_fan_ctx *ctx, unsigned long pwm)
@@ -100,6 +103,50 @@ static struct attribute *pwm_fan_attrs[] = {

  ATTRIBUTE_GROUPS(pwm_fan);

+int pwm_fan_of_get_cooling_data(struct device *dev, struct
pwm_fan_ctx *ctx) +{
+   struct device_node *np = dev-of_node;
+   int num, i, ret;
+
+   ret = of_property_count_elems_of_size(np, cooling-levels,
+ sizeof(u32));
+
+   if (ret == -EINVAL) {
+   dev_err(dev, Property 'cooling-levels' not
found\n);
+   return 0;


Returning 0 is obviously not an error, otherwise you would not return
0 here. So dev_err is wrong.


pr_info would be more appropriate here.


Also, the message itself is wrong; the
property may well be there but have a wrong size.


As fair as I remember the -EINVAL is set only when the property is not
defined. Such situation is correct and our pwm-fan driver should work
with or without it.



of_property_count_elems_of_size returns -EINVAL if np is NULL or if
there is no peoperty or prop-length % elem_size != 0, and -ENODATA
if there is no value associated with the property.

If -EINVAL is not an error, please no message. Noisy drivers are
just that, noisy.




+   }
+
+   if (ret = 0) {
+   dev_err(dev, Wrong data!\n);
+   return ret;
+   }


This will result in the driver failing to load if devicetree is not
configured (of_property_count_elems_of_size will return -ENOSYS).


As you pointed out in the comment to v2:

It is OK if the cooling-levels is not defined in DT. However, if it
has broken definition, then we should return error. This is what we do
here.


You don't return an error, you return 0, at least in some circumstances.


This is not acceptable. Also, if the call returns 0 you don't return
an error but display a Wrong data! error message. Either it is an
error or it is not, but not both.


This is an error. cooling-levels with zero elements is regarded as a
broken property.


It returns -ENOSYS if DT is not configured, which is still unacceptable.
And, again, if ret == 0 is an error, you should return an error code,
not display an error message and return 0.

Guenter

--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 6/8] hwmon: pwm-fan: Extract __set_pwm() function to only modify PWM duty cycle

[Guenter Roeck]

On Fri, Feb 06, 2015 at 05:59:06PM +0100, Lukasz Majewski wrote:
 It was necessary to decouple code handling writing to sysfs from the one
 responsible for setting PWM of the fan.
 Due to that, new __set_pwm() method was extracted, which is responsible for
 only setting new PWM duty cycle.
 
 Signed-off-by: Lukasz Majewski l.majew...@samsung.com
 ---
 Changes for v2:
 - None
 Changes for v3:
 - The commit headline has been reedited.
 ---
  drivers/hwmon/pwm-fan.c | 35 ++-
  1 file changed, 22 insertions(+), 13 deletions(-)
 
 diff --git a/drivers/hwmon/pwm-fan.c b/drivers/hwmon/pwm-fan.c
 index 1991d903..870e100 100644
 --- a/drivers/hwmon/pwm-fan.c
 +++ b/drivers/hwmon/pwm-fan.c
 @@ -33,21 +33,15 @@ struct pwm_fan_ctx {
   unsigned char pwm_value;
  };
  
 -static ssize_t set_pwm(struct device *dev, struct device_attribute *attr,
 -const char *buf, size_t count)
 +static int  __set_pwm(struct pwm_fan_ctx *ctx, unsigned long pwm)
  {
 - struct pwm_fan_ctx *ctx = dev_get_drvdata(dev);
 - unsigned long pwm, duty;
 - ssize_t ret;
 -
 - if (kstrtoul(buf, 10, pwm) || pwm  MAX_PWM)
 - return -EINVAL;
 -
 - mutex_lock(ctx-lock);
 + unsigned long duty;
 + int ret;
  
   if (ctx-pwm_value == pwm)
 - goto exit_set_pwm_no_change;
 + return 0;
  
Why did you move this check outside the lock ? With this change there 
is no guarantee that pwm_value wasn't changed while waiting for the lock.

Guenter

 + mutex_lock(ctx-lock);
   if (pwm == 0) {
   pwm_disable(ctx-pwm);
   goto exit_set_pwm;
 @@ -66,13 +60,28 @@ static ssize_t set_pwm(struct device *dev, struct 
 device_attribute *attr,
  
  exit_set_pwm:
   ctx-pwm_value = pwm;
 -exit_set_pwm_no_change:
 - ret = count;
  exit_set_pwm_err:
   mutex_unlock(ctx-lock);
   return ret;
  }
  
 +static ssize_t set_pwm(struct device *dev, struct device_attribute *attr,
 +const char *buf, size_t count)
 +{
 + struct pwm_fan_ctx *ctx = dev_get_drvdata(dev);
 + unsigned long pwm;
 + int ret;
 +
 + if (kstrtoul(buf, 10, pwm) || pwm  MAX_PWM)
 + return -EINVAL;
 +
 + ret = __set_pwm(ctx, pwm);
 + if (ret)
 + return ret;
 +
 + return count;
 +}
 +
  static ssize_t show_pwm(struct device *dev,
   struct device_attribute *attr, char *buf)
  {
 -- 
 2.0.0.rc2
 
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 7/8] hwmon: pwm-fan: Read PWM FAN configuration from device tree

[Guenter Roeck]

On Fri, Feb 06, 2015 at 05:59:07PM +0100, Lukasz Majewski wrote:
 This patch provides code for reading PWM FAN configuration data via
 device tree. The pwm-fan can work with full speed when configuration
 is not provided. However, errors are propagated when wrong DT bindings
 are found.
 Additionally the struct pwm_fan_ctx has been extended.
 
 Signed-off-by: Lukasz Majewski l.majew...@samsung.com
 ---
 Changes for v2:
 - Rename pwm_fan_max_states to pwm_fan_cooling_levels
 - Moving pwm_fan_of_get_cooling_data() call after setting end enabling PWM FAN
 - pwm_fan_of_get_cooling_data() now can fail - preserving old behaviour
 - Remove unnecessary dev_err() call
 Changes for v3:
 - Patch's headline has been reedited
 - pwm_fan_of_get_cooling_data() return code is now being checked.
 - of_property_count_elems_of_size() is now used instead of_find_property()
 - More verbose patch description added
 ---
  drivers/hwmon/pwm-fan.c | 54 
 -
  1 file changed, 53 insertions(+), 1 deletion(-)
 
 diff --git a/drivers/hwmon/pwm-fan.c b/drivers/hwmon/pwm-fan.c
 index 870e100..f3f5843 100644
 --- a/drivers/hwmon/pwm-fan.c
 +++ b/drivers/hwmon/pwm-fan.c
 @@ -30,7 +30,10 @@
  struct pwm_fan_ctx {
   struct mutex lock;
   struct pwm_device *pwm;
 - unsigned char pwm_value;
 + unsigned int pwm_value;
 + unsigned int pwm_fan_state;
 + unsigned int pwm_fan_max_state;
 + unsigned int *pwm_fan_cooling_levels;
  };
  
  static int  __set_pwm(struct pwm_fan_ctx *ctx, unsigned long pwm)
 @@ -100,6 +103,50 @@ static struct attribute *pwm_fan_attrs[] = {
  
  ATTRIBUTE_GROUPS(pwm_fan);
  
 +int pwm_fan_of_get_cooling_data(struct device *dev, struct pwm_fan_ctx *ctx)
 +{
 + struct device_node *np = dev-of_node;
 + int num, i, ret;
 +
 + ret = of_property_count_elems_of_size(np, cooling-levels,
 +   sizeof(u32));
 +
 + if (ret == -EINVAL) {
 + dev_err(dev, Property 'cooling-levels' not found\n);
 + return 0;

Returning 0 is obviously not an error, otherwise you would not return 0 here.
So dev_err is wrong. Also, the message itself is wrong; the property may
well be there but have a wrong size.

 + }
 +
 + if (ret = 0) {
 + dev_err(dev, Wrong data!\n);
 + return ret;
 + }

This will result in the driver failing to load if devicetree is not configured
(of_property_count_elems_of_size will return -ENOSYS). This is not acceptable.
Also, if the call returns 0 you don't return an error but display a Wrong 
data!
error message. Either it is an error or it is not, but not both.

Guenter
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] hwmon: (ina2xx) Add ina231 compatible string

[Guenter Roeck]

On Wed, Jan 14, 2015 at 03:57:32PM -0800, Kevin Hilman wrote:
 From: Kevin Hilman khil...@linaro.org
 
 Add support for ina231 as compatible string.
 
 Tested with the Exynos5422-based odroid-xu3 board which has on-board
 INA231 sensors.
 
 Signed-off-by: Kevin Hilman khil...@linaro.org

Hi Kevin,

can you also update Documentation/hwmon/ina2xx and Documentation/hwmon/Kconfig ?

Thanks,
Guenter
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 7/8] hwmon: thermal: Read PWM FAN configuration from device tree

[Guenter Roeck]

On Mon, Dec 22, 2014 at 05:27:47PM +0100, Lukasz Majewski wrote:
 Code for reading PWM FAN configuration data via device tree.
 
 Signed-off-by: Lukasz Majewski l.majew...@samsung.com
 ---

The headline is quite misleading. Please provide the affected subsystem (hwmon)
and the affected driver (pwm-fan) in the hwmon-customary form
hwmon: (pwm-fan) Description

The Description should explain what the patch is about.

 Changes for v2:
 - Rename pwm_fan_max_states to pwm_fan_cooling_levels
 - Moving pwm_fan_of_get_cooling_data() call after setting end enabling PWM FAN
 - pwm_fan_of_get_cooling_data() now can fail - preserving old behaviour
 - Remove unnecessary dev_err() call
 ---
  drivers/hwmon/pwm-fan.c | 48 +++-
  1 file changed, 47 insertions(+), 1 deletion(-)
 
 diff --git a/drivers/hwmon/pwm-fan.c b/drivers/hwmon/pwm-fan.c
 index 870e100..8e68308 100644
 --- a/drivers/hwmon/pwm-fan.c
 +++ b/drivers/hwmon/pwm-fan.c
 @@ -30,7 +30,10 @@
  struct pwm_fan_ctx {
   struct mutex lock;
   struct pwm_device *pwm;
 - unsigned char pwm_value;
 + unsigned int pwm_value;
 + unsigned int pwm_fan_state;
 + unsigned int pwm_fan_max_state;
 + unsigned int *pwm_fan_cooling_levels;
  };
  
  static int  __set_pwm(struct pwm_fan_ctx *ctx, unsigned long pwm)
 @@ -100,6 +103,47 @@ static struct attribute *pwm_fan_attrs[] = {
  
  ATTRIBUTE_GROUPS(pwm_fan);
  
 +int pwm_fan_of_get_cooling_data(struct device *dev, struct pwm_fan_ctx *ctx)
 +{
 + struct device_node *np = dev-of_node;
 + struct property *pp;
 + int len, num, i;
 +
 + pp = of_find_property(np, cooling-levels, len);
 + if (!pp) {
 + dev_err(dev, Property: 'cooling-levels' not found\n);
 + return -EINVAL;
 + }
 +
 + if (len == 0) {
 + dev_err(dev, Length wrong value!\n);

Semantics. Wrong length would be better.

 + return -EINVAL;
 + }
 +

of_property_count_elems_of_size() might be more appropriate here.

 + ctx-pwm_fan_cooling_levels = devm_kzalloc(dev, len, GFP_KERNEL);
 + if (!ctx-pwm_fan_cooling_levels)
 + return -ENOMEM;
 +
 + num = len / sizeof(u32);

What if 'num' is 0 ? Is that guaranteed to never happen ?

 + if (of_property_read_u32_array(np, pp-name,
 +ctx-pwm_fan_cooling_levels, num)) {
 + dev_err(dev, Property: %s cannot be read!\n, pp-name);

The ':' after 'Property' in those error messages doesn't seem to make much sense
to me.

 + return -EINVAL;

of_property_read_u32_array() returns an error code. Please use it.

 + }
 +
 + for (i = 0; i  num; i++) {
 + if (ctx-pwm_fan_cooling_levels[i]  MAX_PWM) {
 + dev_err(dev, PWM fan state[%d]:%d  %d\n, i,
 + ctx-pwm_fan_cooling_levels[i], MAX_PWM);
 + return -EINVAL;
 + }
 + }
 +
 + ctx-pwm_fan_max_state = num - 1;
 +
 + return 0;
 +}
 +
  static int pwm_fan_probe(struct platform_device *pdev)
  {
   struct device *hwmon;
 @@ -145,6 +189,8 @@ static int pwm_fan_probe(struct platform_device *pdev)
   pwm_disable(ctx-pwm);
   return PTR_ERR(hwmon);
   }
 +
 + pwm_fan_of_get_cooling_data(pdev-dev, ctx);

Now this is odd. Adding a function to return an error code just to ignore it
doesn't make much sense. While it makes sense to ignore errors if there is no
cooling data in the devicetree, errors due to bad devicetree data should not be
ignored.

I am also a bit concerned that you make this call _after_ instantiating the
hwmon device; this may potentially result in a race condition. Please ensure
that this is not the case.

Thanks,
Guenter
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 6/8] hwmon: thermal: Extract __set_pwm() function to only modify PWM duty cycle

[Guenter Roeck]

On Mon, Dec 22, 2014 at 05:27:46PM +0100, Lukasz Majewski wrote:
 It was necessary to decouple code handling writing to sysfs from the one
 responsible for setting PWM of the fan.
 Due to that, new __set_pwm() method was extracted, which is responsible for
 only setting new PWM duty cycle.
 
 Signed-off-by: Lukasz Majewski l.majew...@samsung.com
 ---

Please provide the affected subsystem and the affected driver in the header.
While it may make sense to explain that the patch is to prepare the driver for
thermal use, this should be part of the descriptive text. The patch is,
however, not not a thermal subsystem related patch. The 'thermal:' in the
headline is thus misleading, and you should drop it.

Guenter
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/9] hwmon: dts: Doc: Add DTS doc to explain how to use PWM FAN as a cooling device

[Guenter Roeck]

On Fri, Dec 19, 2014 at 04:32:24PM +0100, Lukasz Majewski wrote:
 Hi Sjoerd,
 
 Thanks for your feedback and sorry for a late reply.
 
  On Thu, 2014-12-18 at 11:13 +0100, Lukasz Majewski wrote:
   Several new properties to allow PWM fan working as a cooling device
   have been combined into this single commit.
   
   Signed-off-by: Lukasz Majewski l.majew...@samsung.com
   ---
.../devicetree/bindings/hwmon/pwm-fan.txt  | 28
   ++ 1 file changed, 28 insertions(+)
   
   diff --git a/Documentation/devicetree/bindings/hwmon/pwm-fan.txt
   b/Documentation/devicetree/bindings/hwmon/pwm-fan.txt index
   610757c..3877810 100644 ---
   a/Documentation/devicetree/bindings/hwmon/pwm-fan.txt +++
   b/Documentation/devicetree/bindings/hwmon/pwm-fan.txt @@ -3,10
   +3,38 @@ Bindings for a fan connected to the PWM lines Required
   properties:
- compatible : pwm-fan
- pwms   : the PWM that is used to control the PWM fan
   +- cooling-pwm-values  : PWM duty cycle values relative to
   + cooling-max-pwm-value correspondig to
   + proper cooling states
   +- default-pulse-width : Property specifying default pulse
   width for FAN
   + at system boot (zero to disable FAN on
   boot).
   + Allowed range is 0 to 255
  
  The 0..255 range seems somewhat random. Would be nicer to either use
  the approach of pwm-backlight (iotw, have the range go from the first
  to the last entry of cooling-pwm-values) 
 
 I'm OK to change the default-pulse-width to be similar to
 default-brightness-level (as it is in
 Documentation/devicetree/bindings/video/backlight/pwm-backlight.txt)
 
  or simply have be the duty
  lenght in NS as entries instead of the current indirection.
 
 I'd prefer to keep the indirection - as it is utilized in the current
 pwm-fan.c driver.
 
FWIW, devicetree information is supposed to be implementation independent.
So this is a poor argument.

 
 Enabling pan to full RPM was the default behaviour in the current
 pwm-fan.c file.
 
 To be honest, there is no need to enable fan to full RPM speed in this
 board for following reasons:
 1. In Odroid the FAN is optional (stacked on top of a heat sink) - very
 often it is just enough to only have the heat sink.
 
 2. Odroid has thermal enabled by default and IMHO it would be more
 feasible to allow thermal to control fan from the very beginning.
 
 However, I can also understand if the policy for hwmon implies a rule
 to enable by default all fans to full RPM speed.
 
Why and how does that all suggest that the current default behavior
should be changed ?

Guenter
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/9] hwmon: dts: Doc: Add DTS doc to explain how to use PWM FAN as a cooling device

[Guenter Roeck]

On 12/18/2014 02:13 AM, Lukasz Majewski wrote:

Several new properties to allow PWM fan working as a cooling device have been
combined into this single commit.

Signed-off-by: Lukasz Majewski l.majew...@samsung.com
---
  .../devicetree/bindings/hwmon/pwm-fan.txt  | 28 ++
  1 file changed, 28 insertions(+)

diff --git a/Documentation/devicetree/bindings/hwmon/pwm-fan.txt 
b/Documentation/devicetree/bindings/hwmon/pwm-fan.txt
index 610757c..3877810 100644
--- a/Documentation/devicetree/bindings/hwmon/pwm-fan.txt
+++ b/Documentation/devicetree/bindings/hwmon/pwm-fan.txt
@@ -3,10 +3,38 @@ Bindings for a fan connected to the PWM lines
  Required properties:
  - compatible  : pwm-fan
  - pwms: the PWM that is used to control the PWM fan
+- cooling-pwm-values  : PWM duty cycle values relative to
+   cooling-max-pwm-value correspondig to
+   proper cooling states


I don't understand what you mean with relative to. Please elaborate.
Do you mean associated with ?

Where is cooling-max-pwm-value defined, and how is this all expected
to relate to the maximum duty cycle value provided by the pwm device ?


+- default-pulse-width : Property specifying default pulse width for FAN
+   at system boot (zero to disable FAN on boot).
+   Allowed range is 0 to 255


You'll need dt maintainer approval for the new properties.

One thing I wonder about though is why you use default-pulse-width
and not default-pwm. Seems to be arbitrary. I don't see pulse-width
used anywhere in the upstream kernel.

I am somewhat concerned that you define the new properties as mandatory.
That means existing configurations will fail, which does not seem to be
a good idea. It would be more appropriate to not configure the thermal device
if the new properties are not provided.

The newly introduced semantics also conflicts with the current semantics,
which sets the initial duty cycle initially to the maximum allowed duty cycle
as reported by the pwm device itself.

Guenter


+
+Thorough description of the following bindings:
+   cooling-min-state = 0;
+   cooling-max-state = 3;
+   #cooling-cells = 2;
+   thermal-zone {
+   cpu_thermal: cpu-thermal {
+   cooling-maps {
+   map0 {
+trip = cpu_alert1;
+cooling-device = fan0 0 1;
+   };
+   };
+   };
+
+for PWM FAN used as cooling device can be found at:
+./Documentation/devicetree/bindings/thermal/thermal.txt

  Example:
pwm-fan {
compatible = pwm-fan;
status = okay;
pwms = pwm 0 1 0;
+   cooling-min-state = 0;
+   cooling-max-state = 3;
+   #cooling-cells = 2;
+   cooling-pwm-values = 0 102 170 255;
+   default-pulse-width = 0;
};



--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v7 08/11] arm/arm64: Unexport restart handlers

[Guenter Roeck]

On 12/04/2014 05:36 AM, Geert Uytterhoeven wrote:

Hi Günther,

On Wed, Aug 20, 2014 at 2:45 AM, Guenter Roeck li...@roeck-us.net wrote:

Implementing a restart handler in a module don't make sense
as there would be no guarantee that the module is loaded when
a restart is needed. Unexport arm_pm_restart to ensure that
no one gets the idea to do it anyway.


Why not? I was just going to do that, but I got greeted by:



Because you should register a restart handler instead, like the other
drivers in the same directory now do.


ERROR: arm_pm_restart [drivers/power/reset/rmobile-reset.ko] undefined!

So now we have to make sure all reset drivers for a zillion different
hardware devices are builtin, and can't be modular?


No. All those drivers need to do is to register a restart handler using
the API provided in the patch series.

Ultimately all restart handlers should do that and arm_pm_restart should
go away entirely. That was the point of the patch series.

Guenter

--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v7 08/11] arm/arm64: Unexport restart handlers

[Guenter Roeck]

On 12/04/2014 06:44 AM, Geert Uytterhoeven wrote:

Hi Günther,

On Thu, Dec 4, 2014 at 3:26 PM, Guenter Roeck li...@roeck-us.net wrote:

On 12/04/2014 05:36 AM, Geert Uytterhoeven wrote:

On Wed, Aug 20, 2014 at 2:45 AM, Guenter Roeck li...@roeck-us.net wrote:

Implementing a restart handler in a module don't make sense
as there would be no guarantee that the module is loaded when
a restart is needed. Unexport arm_pm_restart to ensure that
no one gets the idea to do it anyway.


Why not? I was just going to do that, but I got greeted by:


Because you should register a restart handler instead, like the other
drivers in the same directory now do.


That's a different thing. there would be no guarantee that the module is
loaded when a restart is needed is also valid for restart handlers...



Not really, because you are supposed to unregister the restart handler
on unload. Sure, you can instead clear arm_pm_reastart and leave the system
with no means to restart ...

Guenter


ERROR: arm_pm_restart [drivers/power/reset/rmobile-reset.ko] undefined!

So now we have to make sure all reset drivers for a zillion different
hardware devices are builtin, and can't be modular?


No. All those drivers need to do is to register a restart handler using
the API provided in the patch series.

Ultimately all restart handlers should do that and arm_pm_restart should
go away entirely. That was the point of the patch series.


Good. That's what I'm doing right know ;-)

Gr{oetje,eeting}s,

 Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say programmer or something like that.
 -- Linus Torvalds



--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v7 08/11] arm/arm64: Unexport restart handlers

[Guenter Roeck]

On Thu, Dec 04, 2014 at 04:06:22PM +0100, Arnd Bergmann wrote:
 On Thursday 04 December 2014 06:51:49 Guenter Roeck wrote:
  On 12/04/2014 06:44 AM, Geert Uytterhoeven wrote:
   On Thu, Dec 4, 2014 at 3:26 PM, Guenter Roeck li...@roeck-us.net wrote:
   On 12/04/2014 05:36 AM, Geert Uytterhoeven wrote:
   On Wed, Aug 20, 2014 at 2:45 AM, Guenter Roeck li...@roeck-us.net 
   wrote:
   Implementing a restart handler in a module don't make sense
   as there would be no guarantee that the module is loaded when
   a restart is needed. Unexport arm_pm_restart to ensure that
   no one gets the idea to do it anyway.
  
   Why not? I was just going to do that, but I got greeted by:
  
   Because you should register a restart handler instead, like the other
   drivers in the same directory now do.
  
   That's a different thing. there would be no guarantee that the module is
   loaded when a restart is needed is also valid for restart handlers...
  
  
  Not really, because you are supposed to unregister the restart handler
  on unload. Sure, you can instead clear arm_pm_reastart and leave the system
  with no means to restart ...
 
 I agree with Geert that your commit message was confusing, it sounds like
 you were referring to drivers that are not yet loaded, while the problem
 that you are really address is drivers that have been unloaded later.
 
Yes, I realize that now. I should probably have added the rationale
for the entire series to this commit. I'll try to do better next time.

Guenter
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 2/2] ARM: EXYNOS: PMU: move restart code into pmu driver

[Guenter Roeck]

On Mon, Nov 17, 2014 at 04:51:13PM +0530, Pankaj Dubey wrote:
 Let's register restart handler from PMU driver for reboot
 functionality. So that we can remove restart hooks from
 machine specific file, and thus moving ahead when PMU moved
 to driver folder, this functionality can be reused for ARM64
 based Exynos SoC's.
 
 Signed-off-by: Pankaj Dubey pankaj.du...@samsung.com
 ---
  arch/arm/mach-exynos/common.h |1 -
  arch/arm/mach-exynos/exynos.c |6 --
  arch/arm/mach-exynos/pmu.c|   23 +++
  3 files changed, 23 insertions(+), 7 deletions(-)
 
 diff --git a/arch/arm/mach-exynos/common.h b/arch/arm/mach-exynos/common.h
 index 431be1b..865f878 100644
 --- a/arch/arm/mach-exynos/common.h
 +++ b/arch/arm/mach-exynos/common.h
 @@ -12,7 +12,6 @@
  #ifndef __ARCH_ARM_MACH_EXYNOS_COMMON_H
  #define __ARCH_ARM_MACH_EXYNOS_COMMON_H
  
 -#include linux/reboot.h
  #include linux/of.h
  
  #define EXYNOS3250_SOC_ID0xE3472000
 diff --git a/arch/arm/mach-exynos/exynos.c b/arch/arm/mach-exynos/exynos.c
 index 8f995b7..c13d083 100644
 --- a/arch/arm/mach-exynos/exynos.c
 +++ b/arch/arm/mach-exynos/exynos.c
 @@ -87,11 +87,6 @@ static struct map_desc exynos5_iodesc[] __initdata = {
   },
  };
  
 -static void exynos_restart(enum reboot_mode mode, const char *cmd)
 -{
 - __raw_writel(0x1, pmu_base_addr + EXYNOS_SWRESET);
 -}
 -
  static struct platform_device exynos_cpuidle = {
   .name  = exynos_cpuidle,
  #ifdef CONFIG_ARM_EXYNOS_CPUIDLE
 @@ -316,7 +311,6 @@ DT_MACHINE_START(EXYNOS_DT, SAMSUNG EXYNOS (Flattened 
 Device Tree))
   .init_machine   = exynos_dt_machine_init,
   .init_late  = exynos_init_late,
   .dt_compat  = exynos_dt_compat,
 - .restart= exynos_restart,
   .reserve= exynos_reserve,
   .dt_fixup   = exynos_dt_fixup,
  MACHINE_END
 diff --git a/arch/arm/mach-exynos/pmu.c b/arch/arm/mach-exynos/pmu.c
 index 6c8a76d..35f4062 100644
 --- a/arch/arm/mach-exynos/pmu.c
 +++ b/arch/arm/mach-exynos/pmu.c
 @@ -11,8 +11,11 @@
  
  #include linux/io.h
  #include linux/of.h
 +#include linux/of_address.h
  #include linux/platform_device.h
  #include linux/delay.h
 +#include linux/notifier.h
 +#include linux/reboot.h
  
  
  #include exynos-pmu.h
 @@ -716,6 +719,13 @@ static void exynos5420_pmu_init(void)
   pr_info(EXYNOS5420 PMU initialized\n);
  }
  
 +static int pmu_restart_notify(struct notifier_block *this,
 + unsigned long code, void *unused)
 +{
 + pmu_raw_writel(0x1, EXYNOS_SWRESET);
 +
 + return NOTIFY_DONE;
 +}
  
  static const struct exynos_pmu_data exynos4210_pmu_data = {
   .pmu_config = exynos4210_pmu_config,
 @@ -765,11 +775,20 @@ static const struct of_device_id 
 exynos_pmu_of_device_ids[] = {
   { /*sentinel*/ },
  };
  
 +/*
 + * Exynos PMU reboot notifier, handles reboot functionality

restart, really.

 + */
 +static struct notifier_block pmu_restart_handler = {
 + .notifier_call = pmu_restart_notify,
 + .priority = 128,
 +};
 +
  static int exynos_pmu_probe(struct platform_device *pdev)
  {
   const struct of_device_id *match;
   struct device *dev = pdev-dev;
   struct resource *res;
 + int ret;
  
   res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
   pmu_base_addr = devm_ioremap_resource(dev, res);
 @@ -794,6 +813,10 @@ static int exynos_pmu_probe(struct platform_device *pdev)
  
   platform_set_drvdata(pdev, pmu_context);
  
 + ret = register_restart_handler(pmu_restart_handler);
 + if (ret)
 + dev_err(dev, can't register restart handler err=%d\n, ret);
 +

dev_warn might be more appropriate, since you ignore the error.
But that is a nitpick, really, as well as the above.

Acked-by: Guenter Roeck li...@roeck-us.net

   dev_dbg(dev, Exynos PMU Driver probe done\n);
   return 0;
  }
 -- 
 1.7.9.5
 
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 1/2] clk: samsung: exynos5440: move restart code into clock driver

[Guenter Roeck]

On Mon, Nov 17, 2014 at 04:51:12PM +0530, Pankaj Dubey wrote:
 Let's register restart handler for Exynos5440 from it's clock driver
 for reboot functionality. So that we can cleanup restart hooks from
 machine specific file.
 
 CC: Sylwester Nawrocki s.nawro...@samsung.com
 CC: Tomasz Figa tomasz.f...@gmail.com
 Signed-off-by: Pankaj Dubey pankaj.du...@samsung.com

reboot-restart, and consider using pr_warn instead of pr_err.

Since those are nitpicks

Acked-by: Guenter Roeck li...@roeck-us.net

 ---
  arch/arm/mach-exynos/exynos.c|   19 +--
  drivers/clk/samsung/clk-exynos5440.c |   29 -
  2 files changed, 29 insertions(+), 19 deletions(-)
 
 diff --git a/arch/arm/mach-exynos/exynos.c b/arch/arm/mach-exynos/exynos.c
 index 8f638ad..8f995b7 100644
 --- a/arch/arm/mach-exynos/exynos.c
 +++ b/arch/arm/mach-exynos/exynos.c
 @@ -89,24 +89,7 @@ static struct map_desc exynos5_iodesc[] __initdata = {
  
  static void exynos_restart(enum reboot_mode mode, const char *cmd)
  {
 - struct device_node *np;
 - u32 val = 0x1;
 - void __iomem *addr = pmu_base_addr + EXYNOS_SWRESET;
 -
 - if (of_machine_is_compatible(samsung,exynos5440)) {
 - u32 status;
 - np = of_find_compatible_node(NULL, NULL, 
 samsung,exynos5440-clock);
 -
 - addr = of_iomap(np, 0) + 0xbc;
 - status = __raw_readl(addr);
 -
 - addr = of_iomap(np, 0) + 0xcc;
 - val = __raw_readl(addr);
 -
 - val = (val  0x) | (status  0x);
 - }
 -
 - __raw_writel(val, addr);
 + __raw_writel(0x1, pmu_base_addr + EXYNOS_SWRESET);
  }
  
  static struct platform_device exynos_cpuidle = {
 diff --git a/drivers/clk/samsung/clk-exynos5440.c 
 b/drivers/clk/samsung/clk-exynos5440.c
 index 00d1d00..f43b925 100644
 --- a/drivers/clk/samsung/clk-exynos5440.c
 +++ b/drivers/clk/samsung/clk-exynos5440.c
 @@ -15,6 +15,8 @@
  #include linux/clk-provider.h
  #include linux/of.h
  #include linux/of_address.h
 +#include linux/notifier.h
 +#include linux/reboot.h
  
  #include clk.h
  #include clk-pll.h
 @@ -23,6 +25,8 @@
  #define CPU_CLK_STATUS   0xfc
  #define MISC_DOUT1   0x558
  
 +static void __iomem *reg_base;
 +
  /* parent clock name list */
  PNAME(mout_armclk_p) = { cplla, cpllb };
  PNAME(mout_spi_p)= { div125, div200 };
 @@ -89,10 +93,30 @@ static const struct of_device_id ext_clk_match[] 
 __initconst = {
   {},
  };
  
 +static int exynos5440_clk_restart_notify(struct notifier_block *this,
 + unsigned long code, void *unused)
 +{
 + u32 val, status;
 +
 + status = readl_relaxed(reg_base + 0xbc);
 + val = readl_relaxed(reg_base + 0xcc);
 + val = (val  0x) | (status  0x);
 + writel_relaxed(val, reg_base + 0xcc);
 +
 + return NOTIFY_DONE;
 +}
 +
 +/*
 + * Exynos5440 Clock reboot notifier, handles reboot functionality
 + */
 +static struct notifier_block exynos5440_clk_restart_handler = {
 + .notifier_call = exynos5440_clk_restart_notify,
 + .priority = 128,
 +};
 +
  /* register exynos5440 clocks */
  static void __init exynos5440_clk_init(struct device_node *np)
  {
 - void __iomem *reg_base;
   struct samsung_clk_provider *ctx;
  
   reg_base = of_iomap(np, 0);
 @@ -125,6 +149,9 @@ static void __init exynos5440_clk_init(struct device_node 
 *np)
  
   samsung_clk_of_add_provider(np, ctx);
  
 + if (register_restart_handler(exynos5440_clk_restart_handler))
 + pr_err(exynos5440 clock can't register restart handler\n);
 +
   pr_info(Exynos5440: arm_clk = %ldHz\n, _get_rate(arm_clk));
   pr_info(exynos5440 clock initialization complete\n);
  }
 -- 
 1.7.9.5
 
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v6 35/48] arm: Register with kernel power-off handler

[Guenter Roeck]

Register with kernel power-off handler instead of setting pm_power_off
directly. Always use register_power_off_handler_simple as there is no
indication that more than one power-off handler is registered.

If the power-off handler only resets the system or puts the CPU in sleep mode,
select the fallback priority to indicate that the power-off handler is one
of last resort. If the power-off handler powers off the system, select the
default priority.

Cc: Russell King li...@arm.linux.org.uk
Signed-off-by: Guenter Roeck li...@roeck-us.net
---
v6:
- This patch: No change.
  Global: Replaced priority defines with enum.
v5:
- Rebase to v3.18-rc3
v4:
- No change
v3:
- Replace poweroff in all newly introduced variables and in text
  with power_off or power-off as appropriate
- Replace POWEROFF_PRIORITY_xxx with POWER_OFF_PRIORITY_xxx
v2:
- Use defines to specify poweroff handler priorities
- Drop changes in arch/arm/mach-at91/setup.c (file removed upstream)

 arch/arm/kernel/psci.c | 3 ++-
 arch/arm/mach-at91/board-gsia18s.c | 3 ++-
 arch/arm/mach-bcm/board_bcm2835.c  | 3 ++-
 arch/arm/mach-cns3xxx/cns3420vb.c  | 3 ++-
 arch/arm/mach-cns3xxx/core.c   | 3 ++-
 arch/arm/mach-highbank/highbank.c  | 3 ++-
 arch/arm/mach-imx/mach-mx31moboard.c   | 3 ++-
 arch/arm/mach-iop32x/em7210.c  | 3 ++-
 arch/arm/mach-iop32x/glantank.c| 3 ++-
 arch/arm/mach-iop32x/iq31244.c | 3 ++-
 arch/arm/mach-iop32x/n2100.c   | 3 ++-
 arch/arm/mach-ixp4xx/dsmg600-setup.c   | 3 ++-
 arch/arm/mach-ixp4xx/nas100d-setup.c   | 3 ++-
 arch/arm/mach-ixp4xx/nslu2-setup.c | 3 ++-
 arch/arm/mach-omap2/board-omap3touchbook.c | 3 ++-
 arch/arm/mach-orion5x/board-mss2.c | 3 ++-
 arch/arm/mach-orion5x/dns323-setup.c   | 9 ++---
 arch/arm/mach-orion5x/kurobox_pro-setup.c  | 3 ++-
 arch/arm/mach-orion5x/ls-chl-setup.c   | 3 ++-
 arch/arm/mach-orion5x/ls_hgl-setup.c   | 3 ++-
 arch/arm/mach-orion5x/lsmini-setup.c   | 3 ++-
 arch/arm/mach-orion5x/mv2120-setup.c   | 3 ++-
 arch/arm/mach-orion5x/net2big-setup.c  | 3 ++-
 arch/arm/mach-orion5x/terastation_pro2-setup.c | 3 ++-
 arch/arm/mach-orion5x/ts209-setup.c| 3 ++-
 arch/arm/mach-orion5x/ts409-setup.c| 3 ++-
 arch/arm/mach-pxa/corgi.c  | 3 ++-
 arch/arm/mach-pxa/mioa701.c| 3 ++-
 arch/arm/mach-pxa/poodle.c | 3 ++-
 arch/arm/mach-pxa/spitz.c  | 3 ++-
 arch/arm/mach-pxa/tosa.c   | 3 ++-
 arch/arm/mach-pxa/viper.c  | 3 ++-
 arch/arm/mach-pxa/z2.c | 7 ---
 arch/arm/mach-pxa/zeus.c   | 7 ---
 arch/arm/mach-s3c24xx/mach-gta02.c | 3 ++-
 arch/arm/mach-s3c24xx/mach-jive.c  | 3 ++-
 arch/arm/mach-s3c24xx/mach-vr1000.c| 3 ++-
 arch/arm/mach-s3c64xx/mach-smartq.c| 3 ++-
 arch/arm/mach-sa1100/generic.c | 3 ++-
 arch/arm/mach-sa1100/simpad.c  | 3 ++-
 arch/arm/mach-u300/regulator.c | 3 ++-
 arch/arm/mach-vt8500/vt8500.c  | 3 ++-
 arch/arm/xen/enlighten.c   | 3 ++-
 43 files changed, 94 insertions(+), 49 deletions(-)

diff --git a/arch/arm/kernel/psci.c b/arch/arm/kernel/psci.c
index f73891b..a7a2b4a 100644
--- a/arch/arm/kernel/psci.c
+++ b/arch/arm/kernel/psci.c
@@ -264,7 +264,8 @@ static int psci_0_2_init(struct device_node *np)
 
arm_pm_restart = psci_sys_reset;
 
-   pm_power_off = psci_sys_poweroff;
+   register_power_off_handler_simple(psci_sys_poweroff,
+ POWER_OFF_PRIORITY_DEFAULT);
 
 out_put_node:
of_node_put(np);
diff --git a/arch/arm/mach-at91/board-gsia18s.c 
b/arch/arm/mach-at91/board-gsia18s.c
index bf5cc55..e628c4a 100644
--- a/arch/arm/mach-at91/board-gsia18s.c
+++ b/arch/arm/mach-at91/board-gsia18s.c
@@ -521,7 +521,8 @@ static void gsia18s_power_off(void)
 
 static int __init gsia18s_power_off_init(void)
 {
-   pm_power_off = gsia18s_power_off;
+   register_power_off_handler_simple(gsia18s_power_off,
+ POWER_OFF_PRIORITY_DEFAULT);
return 0;
 }
 
diff --git a/arch/arm/mach-bcm/board_bcm2835.c 
b/arch/arm/mach-bcm/board_bcm2835.c
index 70f2f39..1d75c76 100644
--- a/arch/arm/mach-bcm/board_bcm2835.c
+++ b/arch/arm/mach-bcm/board_bcm2835.c
@@ -111,7 +111,8 @@ static void __init bcm2835_init(void)
 
bcm2835_setup_restart();
if (wdt_regs)
-   pm_power_off = bcm2835_power_off;
+   register_power_off_handler_simple(bcm2835_power_off,
+ POWER_OFF_PRIORITY_FALLBACK);
 
bcm2835_init_clocks();
 
diff --git a/arch/arm/mach

[PATCH v5 35/48] arm: Register with kernel power-off handler

[Guenter Roeck]

Register with kernel power-off handler instead of setting pm_power_off
directly. Always use register_power_off_handler_simple as there is no
indication that more than one power-off handler is registered.

If the power-off handler only resets the system or puts the CPU in sleep mode,
select the fallback priority to indicate that the power-off handler is one
of last resort. If the power-off handler powers off the system, select the
default priority.

Cc: Russell King li...@arm.linux.org.uk
Signed-off-by: Guenter Roeck li...@roeck-us.net
---
v5:
- Rebase to v3.18-rc3
v4:
- No change
v3:
- Replace poweroff in all newly introduced variables and in text
  with power_off or power-off as appropriate
- Replace POWEROFF_PRIORITY_xxx with POWER_OFF_PRIORITY_xxx
v2:
- Use defines to specify poweroff handler priorities
- Drop changes in arch/arm/mach-at91/setup.c (file removed upstream)

 arch/arm/kernel/psci.c | 3 ++-
 arch/arm/mach-at91/board-gsia18s.c | 3 ++-
 arch/arm/mach-bcm/board_bcm2835.c  | 3 ++-
 arch/arm/mach-cns3xxx/cns3420vb.c  | 3 ++-
 arch/arm/mach-cns3xxx/core.c   | 3 ++-
 arch/arm/mach-highbank/highbank.c  | 3 ++-
 arch/arm/mach-imx/mach-mx31moboard.c   | 3 ++-
 arch/arm/mach-iop32x/em7210.c  | 3 ++-
 arch/arm/mach-iop32x/glantank.c| 3 ++-
 arch/arm/mach-iop32x/iq31244.c | 3 ++-
 arch/arm/mach-iop32x/n2100.c   | 3 ++-
 arch/arm/mach-ixp4xx/dsmg600-setup.c   | 3 ++-
 arch/arm/mach-ixp4xx/nas100d-setup.c   | 3 ++-
 arch/arm/mach-ixp4xx/nslu2-setup.c | 3 ++-
 arch/arm/mach-omap2/board-omap3touchbook.c | 3 ++-
 arch/arm/mach-orion5x/board-mss2.c | 3 ++-
 arch/arm/mach-orion5x/dns323-setup.c   | 9 ++---
 arch/arm/mach-orion5x/kurobox_pro-setup.c  | 3 ++-
 arch/arm/mach-orion5x/ls-chl-setup.c   | 3 ++-
 arch/arm/mach-orion5x/ls_hgl-setup.c   | 3 ++-
 arch/arm/mach-orion5x/lsmini-setup.c   | 3 ++-
 arch/arm/mach-orion5x/mv2120-setup.c   | 3 ++-
 arch/arm/mach-orion5x/net2big-setup.c  | 3 ++-
 arch/arm/mach-orion5x/terastation_pro2-setup.c | 3 ++-
 arch/arm/mach-orion5x/ts209-setup.c| 3 ++-
 arch/arm/mach-orion5x/ts409-setup.c| 3 ++-
 arch/arm/mach-pxa/corgi.c  | 3 ++-
 arch/arm/mach-pxa/mioa701.c| 3 ++-
 arch/arm/mach-pxa/poodle.c | 3 ++-
 arch/arm/mach-pxa/spitz.c  | 3 ++-
 arch/arm/mach-pxa/tosa.c   | 3 ++-
 arch/arm/mach-pxa/viper.c  | 3 ++-
 arch/arm/mach-pxa/z2.c | 7 ---
 arch/arm/mach-pxa/zeus.c   | 7 ---
 arch/arm/mach-s3c24xx/mach-gta02.c | 3 ++-
 arch/arm/mach-s3c24xx/mach-jive.c  | 3 ++-
 arch/arm/mach-s3c24xx/mach-vr1000.c| 3 ++-
 arch/arm/mach-s3c64xx/mach-smartq.c| 3 ++-
 arch/arm/mach-sa1100/generic.c | 3 ++-
 arch/arm/mach-sa1100/simpad.c  | 3 ++-
 arch/arm/mach-u300/regulator.c | 3 ++-
 arch/arm/mach-vt8500/vt8500.c  | 3 ++-
 arch/arm/xen/enlighten.c   | 3 ++-
 43 files changed, 94 insertions(+), 49 deletions(-)

diff --git a/arch/arm/kernel/psci.c b/arch/arm/kernel/psci.c
index f73891b..a7a2b4a 100644
--- a/arch/arm/kernel/psci.c
+++ b/arch/arm/kernel/psci.c
@@ -264,7 +264,8 @@ static int psci_0_2_init(struct device_node *np)
 
arm_pm_restart = psci_sys_reset;
 
-   pm_power_off = psci_sys_poweroff;
+   register_power_off_handler_simple(psci_sys_poweroff,
+ POWER_OFF_PRIORITY_DEFAULT);
 
 out_put_node:
of_node_put(np);
diff --git a/arch/arm/mach-at91/board-gsia18s.c 
b/arch/arm/mach-at91/board-gsia18s.c
index bf5cc55..e628c4a 100644
--- a/arch/arm/mach-at91/board-gsia18s.c
+++ b/arch/arm/mach-at91/board-gsia18s.c
@@ -521,7 +521,8 @@ static void gsia18s_power_off(void)
 
 static int __init gsia18s_power_off_init(void)
 {
-   pm_power_off = gsia18s_power_off;
+   register_power_off_handler_simple(gsia18s_power_off,
+ POWER_OFF_PRIORITY_DEFAULT);
return 0;
 }
 
diff --git a/arch/arm/mach-bcm/board_bcm2835.c 
b/arch/arm/mach-bcm/board_bcm2835.c
index 70f2f39..1d75c76 100644
--- a/arch/arm/mach-bcm/board_bcm2835.c
+++ b/arch/arm/mach-bcm/board_bcm2835.c
@@ -111,7 +111,8 @@ static void __init bcm2835_init(void)
 
bcm2835_setup_restart();
if (wdt_regs)
-   pm_power_off = bcm2835_power_off;
+   register_power_off_handler_simple(bcm2835_power_off,
+ POWER_OFF_PRIORITY_FALLBACK);
 
bcm2835_init_clocks();
 
diff --git a/arch/arm/mach-cns3xxx/cns3420vb.c 
b/arch/arm/mach-cns3xxx/cns3420vb.c
index 6428bcc7..5c50461

[PATCH v3 35/47] arm: Register with kernel power-off handler

[Guenter Roeck]

Register with kernel power-off handler instead of setting pm_power_off
directly. Always use register_power_off_handler_simple as there is no
indication that more than one power-off handler is registered.

If the power-off handler only resets the system or puts the CPU in sleep mode,
select the fallback priority to indicate that the power-off handler is one
of last resort. If the power-off handler powers off the system, select the
default priority.

Cc: Russell King li...@arm.linux.org.uk
Signed-off-by: Guenter Roeck li...@roeck-us.net
---
v3:
- Replace poweroff in all newly introduced variables and in text
  with power_off or power-off as appropriate
- Replace POWEROFF_PRIORITY_xxx with POWER_OFF_PRIORITY_xxx
v2:
- Use defines to specify poweroff handler priorities
- Drop changes in arch/arm/mach-at91/setup.c (file removed upstream)

 arch/arm/kernel/psci.c | 3 ++-
 arch/arm/mach-at91/board-gsia18s.c | 3 ++-
 arch/arm/mach-bcm/board_bcm2835.c  | 3 ++-
 arch/arm/mach-cns3xxx/cns3420vb.c  | 3 ++-
 arch/arm/mach-cns3xxx/core.c   | 3 ++-
 arch/arm/mach-highbank/highbank.c  | 3 ++-
 arch/arm/mach-imx/mach-mx31moboard.c   | 3 ++-
 arch/arm/mach-iop32x/em7210.c  | 3 ++-
 arch/arm/mach-iop32x/glantank.c| 3 ++-
 arch/arm/mach-iop32x/iq31244.c | 3 ++-
 arch/arm/mach-iop32x/n2100.c   | 3 ++-
 arch/arm/mach-ixp4xx/dsmg600-setup.c   | 3 ++-
 arch/arm/mach-ixp4xx/nas100d-setup.c   | 3 ++-
 arch/arm/mach-ixp4xx/nslu2-setup.c | 3 ++-
 arch/arm/mach-omap2/board-omap3touchbook.c | 3 ++-
 arch/arm/mach-orion5x/board-mss2.c | 3 ++-
 arch/arm/mach-orion5x/dns323-setup.c   | 9 ++---
 arch/arm/mach-orion5x/kurobox_pro-setup.c  | 3 ++-
 arch/arm/mach-orion5x/ls-chl-setup.c   | 3 ++-
 arch/arm/mach-orion5x/ls_hgl-setup.c   | 3 ++-
 arch/arm/mach-orion5x/lsmini-setup.c   | 3 ++-
 arch/arm/mach-orion5x/mv2120-setup.c   | 3 ++-
 arch/arm/mach-orion5x/net2big-setup.c  | 3 ++-
 arch/arm/mach-orion5x/terastation_pro2-setup.c | 3 ++-
 arch/arm/mach-orion5x/ts209-setup.c| 3 ++-
 arch/arm/mach-orion5x/ts409-setup.c| 3 ++-
 arch/arm/mach-pxa/corgi.c  | 3 ++-
 arch/arm/mach-pxa/mioa701.c| 3 ++-
 arch/arm/mach-pxa/poodle.c | 3 ++-
 arch/arm/mach-pxa/spitz.c  | 3 ++-
 arch/arm/mach-pxa/tosa.c   | 3 ++-
 arch/arm/mach-pxa/viper.c  | 3 ++-
 arch/arm/mach-pxa/z2.c | 7 ---
 arch/arm/mach-pxa/zeus.c   | 7 ---
 arch/arm/mach-s3c24xx/mach-gta02.c | 3 ++-
 arch/arm/mach-s3c24xx/mach-jive.c  | 3 ++-
 arch/arm/mach-s3c24xx/mach-vr1000.c| 3 ++-
 arch/arm/mach-s3c64xx/mach-smartq.c| 3 ++-
 arch/arm/mach-sa1100/generic.c | 3 ++-
 arch/arm/mach-sa1100/simpad.c  | 3 ++-
 arch/arm/mach-u300/regulator.c | 3 ++-
 arch/arm/mach-vt8500/vt8500.c  | 3 ++-
 arch/arm/xen/enlighten.c   | 3 ++-
 43 files changed, 94 insertions(+), 49 deletions(-)

diff --git a/arch/arm/kernel/psci.c b/arch/arm/kernel/psci.c
index f73891b..a7a2b4a 100644
--- a/arch/arm/kernel/psci.c
+++ b/arch/arm/kernel/psci.c
@@ -264,7 +264,8 @@ static int psci_0_2_init(struct device_node *np)
 
arm_pm_restart = psci_sys_reset;
 
-   pm_power_off = psci_sys_poweroff;
+   register_power_off_handler_simple(psci_sys_poweroff,
+ POWER_OFF_PRIORITY_DEFAULT);
 
 out_put_node:
of_node_put(np);
diff --git a/arch/arm/mach-at91/board-gsia18s.c 
b/arch/arm/mach-at91/board-gsia18s.c
index bf5cc55..e628c4a 100644
--- a/arch/arm/mach-at91/board-gsia18s.c
+++ b/arch/arm/mach-at91/board-gsia18s.c
@@ -521,7 +521,8 @@ static void gsia18s_power_off(void)
 
 static int __init gsia18s_power_off_init(void)
 {
-   pm_power_off = gsia18s_power_off;
+   register_power_off_handler_simple(gsia18s_power_off,
+ POWER_OFF_PRIORITY_DEFAULT);
return 0;
 }
 
diff --git a/arch/arm/mach-bcm/board_bcm2835.c 
b/arch/arm/mach-bcm/board_bcm2835.c
index 70f2f39..1d75c76 100644
--- a/arch/arm/mach-bcm/board_bcm2835.c
+++ b/arch/arm/mach-bcm/board_bcm2835.c
@@ -111,7 +111,8 @@ static void __init bcm2835_init(void)
 
bcm2835_setup_restart();
if (wdt_regs)
-   pm_power_off = bcm2835_power_off;
+   register_power_off_handler_simple(bcm2835_power_off,
+ POWER_OFF_PRIORITY_FALLBACK);
 
bcm2835_init_clocks();
 
diff --git a/arch/arm/mach-cns3xxx/cns3420vb.c 
b/arch/arm/mach-cns3xxx/cns3420vb.c
index 6428bcc7..5c50461 100644
--- a/arch/arm/mach-cns3xxx

[PATCH v2 35/47] arm: Register with kernel poweroff handler

[Guenter Roeck]

Register with kernel poweroff handler instead of setting pm_power_off
directly. Always use register_power_off_handler_simple as there is no
indication that more than one poweroff handler is registered.

If the poweroff handler only resets the system or puts the CPU in sleep mode,
select the fallback priority to indicate that the poweroff handler is one
of last resort. If the poweroff handler powers off the system, select the
default priority.

Cc: Russell King li...@arm.linux.org.uk
Signed-off-by: Guenter Roeck li...@roeck-us.net
---
- Use defines to specify poweroff handler priorities
- Drop changes in arch/arm/mach-at91/setup.c (file removed upstream)

 arch/arm/kernel/psci.c | 3 ++-
 arch/arm/mach-at91/board-gsia18s.c | 3 ++-
 arch/arm/mach-bcm/board_bcm2835.c  | 3 ++-
 arch/arm/mach-cns3xxx/cns3420vb.c  | 3 ++-
 arch/arm/mach-cns3xxx/core.c   | 3 ++-
 arch/arm/mach-highbank/highbank.c  | 3 ++-
 arch/arm/mach-imx/mach-mx31moboard.c   | 3 ++-
 arch/arm/mach-iop32x/em7210.c  | 3 ++-
 arch/arm/mach-iop32x/glantank.c| 3 ++-
 arch/arm/mach-iop32x/iq31244.c | 3 ++-
 arch/arm/mach-iop32x/n2100.c   | 3 ++-
 arch/arm/mach-ixp4xx/dsmg600-setup.c   | 3 ++-
 arch/arm/mach-ixp4xx/nas100d-setup.c   | 3 ++-
 arch/arm/mach-ixp4xx/nslu2-setup.c | 3 ++-
 arch/arm/mach-omap2/board-omap3touchbook.c | 3 ++-
 arch/arm/mach-orion5x/board-mss2.c | 3 ++-
 arch/arm/mach-orion5x/dns323-setup.c   | 9 ++---
 arch/arm/mach-orion5x/kurobox_pro-setup.c  | 3 ++-
 arch/arm/mach-orion5x/ls-chl-setup.c   | 3 ++-
 arch/arm/mach-orion5x/ls_hgl-setup.c   | 3 ++-
 arch/arm/mach-orion5x/lsmini-setup.c   | 3 ++-
 arch/arm/mach-orion5x/mv2120-setup.c   | 3 ++-
 arch/arm/mach-orion5x/net2big-setup.c  | 3 ++-
 arch/arm/mach-orion5x/terastation_pro2-setup.c | 3 ++-
 arch/arm/mach-orion5x/ts209-setup.c| 3 ++-
 arch/arm/mach-orion5x/ts409-setup.c| 3 ++-
 arch/arm/mach-pxa/corgi.c  | 3 ++-
 arch/arm/mach-pxa/mioa701.c| 3 ++-
 arch/arm/mach-pxa/poodle.c | 3 ++-
 arch/arm/mach-pxa/spitz.c  | 3 ++-
 arch/arm/mach-pxa/tosa.c   | 3 ++-
 arch/arm/mach-pxa/viper.c  | 3 ++-
 arch/arm/mach-pxa/z2.c | 7 ---
 arch/arm/mach-pxa/zeus.c   | 7 ---
 arch/arm/mach-s3c24xx/mach-gta02.c | 3 ++-
 arch/arm/mach-s3c24xx/mach-jive.c  | 3 ++-
 arch/arm/mach-s3c24xx/mach-vr1000.c| 3 ++-
 arch/arm/mach-s3c64xx/mach-smartq.c| 3 ++-
 arch/arm/mach-sa1100/generic.c | 3 ++-
 arch/arm/mach-sa1100/simpad.c  | 3 ++-
 arch/arm/mach-u300/regulator.c | 3 ++-
 arch/arm/mach-vt8500/vt8500.c  | 3 ++-
 arch/arm/xen/enlighten.c   | 3 ++-
 43 files changed, 94 insertions(+), 49 deletions(-)

diff --git a/arch/arm/kernel/psci.c b/arch/arm/kernel/psci.c
index f73891b..4917c99 100644
--- a/arch/arm/kernel/psci.c
+++ b/arch/arm/kernel/psci.c
@@ -264,7 +264,8 @@ static int psci_0_2_init(struct device_node *np)
 
arm_pm_restart = psci_sys_reset;
 
-   pm_power_off = psci_sys_poweroff;
+   register_power_off_handler_simple(psci_sys_poweroff,
+ POWEROFF_PRIORITY_DEFAULT);
 
 out_put_node:
of_node_put(np);
diff --git a/arch/arm/mach-at91/board-gsia18s.c 
b/arch/arm/mach-at91/board-gsia18s.c
index bf5cc55..cb5d1c3 100644
--- a/arch/arm/mach-at91/board-gsia18s.c
+++ b/arch/arm/mach-at91/board-gsia18s.c
@@ -521,7 +521,8 @@ static void gsia18s_power_off(void)
 
 static int __init gsia18s_power_off_init(void)
 {
-   pm_power_off = gsia18s_power_off;
+   register_power_off_handler_simple(gsia18s_power_off,
+ POWEROFF_PRIORITY_DEFAULT);
return 0;
 }
 
diff --git a/arch/arm/mach-bcm/board_bcm2835.c 
b/arch/arm/mach-bcm/board_bcm2835.c
index 70f2f39..307ebc1 100644
--- a/arch/arm/mach-bcm/board_bcm2835.c
+++ b/arch/arm/mach-bcm/board_bcm2835.c
@@ -111,7 +111,8 @@ static void __init bcm2835_init(void)
 
bcm2835_setup_restart();
if (wdt_regs)
-   pm_power_off = bcm2835_power_off;
+   register_power_off_handler_simple(bcm2835_power_off,
+ POWEROFF_PRIORITY_FALLBACK);
 
bcm2835_init_clocks();
 
diff --git a/arch/arm/mach-cns3xxx/cns3420vb.c 
b/arch/arm/mach-cns3xxx/cns3420vb.c
index 6428bcc7..3f48979 100644
--- a/arch/arm/mach-cns3xxx/cns3420vb.c
+++ b/arch/arm/mach-cns3xxx/cns3420vb.c
@@ -224,7 +224,8 @@ static void __init cns3420_init(void)
cns3xxx_ahci_init();
cns3xxx_sdhci_init();
 
-   pm_power_off

Re: [PATCH v3 0/2] Handle reboot for Exynos SoC via restart_handler

[Guenter Roeck]

On Tue, Oct 07, 2014 at 01:23:19PM +0530, Pankaj Dubey wrote:
 This patch removes restart hook from machine_desc of Exynos, and moves
 respective code into reboot_notifiers.

Nitpick:

s/reboot_notifiers/restart_notifiers/

Guenter

 Exynos5440 handles reboot via clock register so let's register a
 restart_handler in Exynos5440 clock driver.
 For rest Exynos SoC, reboot is handled via PMU SWRESET register so
 let's register a restart_handler in PMU driver for handling this.
 
 This patch is inspired and dependent on following patch series[1] from
 Geunter Roeck.
 
 Also this patch is dependent on PMU platform driver patch [2] by me, and
 has been prepared on top of it.
 
 [1]: kernel: Add support for kernel restart handler call chain
 https://lkml.org/lkml/2014/8/19/652
 
 [2]: ARM: Exynos: Convert PMU implementation into a platform driver
 https://lkml.org/lkml/2014/10/6/89
 
 I have tested reboot functionality on Exynos3250 based board, on
 kgene/for-next as well as linux-next's next-20140925 tag.
 
 Patch v2 and related discussion can be found here
 http://www.spinics.net/lists/linux-samsung-soc/msg37457.html
 
 Changes since v2:
   - Used register_restart_handler instead of register_reboot_notifier.
   - Updated notifier names and commit message accordingly.
 
 Changes since v1:
   - Addressed review comments from Tomasz Figa. Removed usage of 
 local variables where ever unnecessary.
   - Make reg_base as global in clk-exynos5440.c file, to avoid 
 iomapping it again in reboot_notifier handler.
 
 Pankaj Dubey (2):
   clk: samsung: exynos5440: move restart code into clock driver
   ARM: EXYNOS: PMU: move restart code into pmu driver
 
  arch/arm/mach-exynos/common.h|1 -
  arch/arm/mach-exynos/exynos.c|   23 ---
  arch/arm/mach-exynos/pmu.c   |   24 
  drivers/clk/samsung/clk-exynos5440.c |   29 -
  4 files changed, 52 insertions(+), 25 deletions(-)
 
 -- 
 1.7.9.5
 
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 2/2] ARM: EXYNOS: PMU: move restart code into pmu driver

[Guenter Roeck]

On 10/01/2014 04:36 AM, Pankaj Dubey wrote:

Let's register reboot_notifier from PMU driver for reboot
functionality. So that we can remove restart hooks from
machine specific file, and thus moving ahead when PMU moved
to driver folder, this functionality can be reused for ARM64
based Exynos SoC's.

Signed-off-by: Pankaj Dubey pankaj.du...@samsung.com
---
  arch/arm/mach-exynos/common.h |1 -
  arch/arm/mach-exynos/exynos.c |6 --
  arch/arm/mach-exynos/pmu.c|   25 +
  3 files changed, 25 insertions(+), 7 deletions(-)

diff --git a/arch/arm/mach-exynos/common.h b/arch/arm/mach-exynos/common.h
index 431be1b..865f878 100644
--- a/arch/arm/mach-exynos/common.h
+++ b/arch/arm/mach-exynos/common.h
@@ -12,7 +12,6 @@
  #ifndef __ARCH_ARM_MACH_EXYNOS_COMMON_H
  #define __ARCH_ARM_MACH_EXYNOS_COMMON_H

-#include linux/reboot.h
  #include linux/of.h

  #define EXYNOS3250_SOC_ID 0xE3472000
diff --git a/arch/arm/mach-exynos/exynos.c b/arch/arm/mach-exynos/exynos.c
index aa394cb..3aa75b8e 100644
--- a/arch/arm/mach-exynos/exynos.c
+++ b/arch/arm/mach-exynos/exynos.c
@@ -137,11 +137,6 @@ static struct map_desc exynos5_iodesc[] __initdata = {
},
  };

-static void exynos_restart(enum reboot_mode mode, const char *cmd)
-{
-   __raw_writel(0x1, pmu_base_addr + EXYNOS_SWRESET);
-}
-
  static struct platform_device exynos_cpuidle = {
.name  = exynos_cpuidle,
  #ifdef CONFIG_ARM_EXYNOS_CPUIDLE
@@ -365,7 +360,6 @@ DT_MACHINE_START(EXYNOS_DT, SAMSUNG EXYNOS (Flattened Device 
Tree))
.init_machine   = exynos_dt_machine_init,
.init_late  = exynos_init_late,
.dt_compat  = exynos_dt_compat,
-   .restart= exynos_restart,
.reserve= exynos_reserve,
.dt_fixup   = exynos_dt_fixup,
  MACHINE_END
diff --git a/arch/arm/mach-exynos/pmu.c b/arch/arm/mach-exynos/pmu.c
index 1993e08..c0855a5 100644
--- a/arch/arm/mach-exynos/pmu.c
+++ b/arch/arm/mach-exynos/pmu.c
@@ -11,7 +11,10 @@

  #include linux/io.h
  #include linux/of.h
+#include linux/of_address.h
  #include linux/platform_device.h
+#include linux/notifier.h
+#include linux/reboot.h

  #include exynos-pmu.h
  #include regs-pmu.h
@@ -439,6 +442,15 @@ static void exynos5250_pmu_init(void)
pmu_raw_writel(value, EXYNOS5_MASK_WDTRESET_REQUEST);
  }

+static int pmu_reboot_notify_handler(struct notifier_block *this,
+   unsigned long code, void *unused)
+{
+   if (code == SYS_RESTART)
+   pmu_raw_writel(0x1, EXYNOS_SWRESET);
+
+   return NOTIFY_DONE;
+}
+
  static const struct exynos_pmu_data exynos4210_pmu_data = {
.pmu_config = exynos4210_pmu_config,
  };
@@ -478,11 +490,20 @@ static const struct of_device_id 
exynos_pmu_of_device_ids[] = {
{ /*sentinel*/ },
  };

+/*
+ * Exynos PMU reboot notifier, handles reboot functionality
+ */
+static struct notifier_block pmu_reboot_notifier = {
+   .notifier_call = pmu_reboot_notify_handler,
+   .priority = 128,
+};
+
  static int exynos_pmu_probe(struct platform_device *pdev)
  {
const struct of_device_id *match;
struct device *dev = pdev-dev;
struct resource *res;
+   int ret;

res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
pmu_base_addr = devm_ioremap_resource(dev, res);
@@ -507,6 +528,10 @@ static int exynos_pmu_probe(struct platform_device *pdev)

platform_set_drvdata(pdev, pmu_context);

+   ret = register_reboot_notifier(pmu_reboot_notifier);
+   if (ret)
+   dev_err(dev, can't register reboot notifier err=%d\n, ret);
+
dev_dbg(dev, Exynos PMU Driver probe done\n);
return 0;
  }



Something went wrong here.

You don't want to register with reboot_notifier, but with restart_notifier.
The code is not SYS_RESTART, but the value of reboot_mode.

The same applies to the other patch as well.

Guenter

--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v7 00/11] kernel: Add support for restart handler call chain

[Guenter Roeck]

On Tue, Sep 30, 2014 at 02:20:02PM -0700, Andrew Morton wrote:
 On Tue, 19 Aug 2014 17:45:27 -0700 Guenter Roeck li...@roeck-us.net wrote:
 
  Introduce a system restart handler call chain to solve the described 
  problems.
 
 So someone has merged eight of these patches into linux-next but these
 three:
 
 watchdog-s3c2410-add-restart-handler.patch
 clk-samsung-register-restart-handlers-for-s3c2412-and-s3c2443.patch
 clk-rockchip-add-restart-handler.patch
 
 were omitted.  What's up?

Most likely PBKC on my side; Looks like I forgot to add those when I created
the immutable branch for others to merge. Sorry for that :-(.

Having said that, I somehow thought that the clock patches would go in through
the clock tree. Heiko, did I get that wrong ? Separately, I sent a pull request
that includes the watchdog patch to Wim.

Thanks,
Guenter
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v7 00/11] kernel: Add support for restart handler call chain

[Guenter Roeck]

On 09/30/2014 04:40 PM, Stephen Rothwell wrote:

Hi Guenter,

On Tue, 30 Sep 2014 15:30:00 -0700 Guenter Roeck li...@roeck-us.net wrote:


On Tue, Sep 30, 2014 at 02:20:02PM -0700, Andrew Morton wrote:

On Tue, 19 Aug 2014 17:45:27 -0700 Guenter Roeck li...@roeck-us.net wrote:


Introduce a system restart handler call chain to solve the described problems.


So someone has merged eight of these patches into linux-next but these
three:

watchdog-s3c2410-add-restart-handler.patch
clk-samsung-register-restart-handlers-for-s3c2412-and-s3c2443.patch
clk-rockchip-add-restart-handler.patch

were omitted.  What's up?


Most likely PBKC on my side; Looks like I forgot to add those when I created
the immutable branch for others to merge. Sorry for that :-(.

Having said that, I somehow thought that the clock patches would go in through
the clock tree. Heiko, did I get that wrong ? Separately, I sent a pull request
that includes the watchdog patch to Wim.


So far, that immutable branch has been merged into the battery tree
(and thus into linux-next) by Sebastian in order to add (I assume):

18a702e0de98 power: reset: use restart_notifier mechanism for msm-poweroff
371bb20d6927 power: Add simple gpio-restart driver

on top of it.

So, I guess the watchdog and clk trees also need to merge that
immutable branch and then add their respective patches from the mmotm
series to their trees.


Yes, and now I remember why I did not include those patches: They are not
authored by myself. I thought it was not appropriate for me to include them
in the branch I created. Maybe flawed thinking, but that was my reasoning.

Guenter

--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [09/14] watchdog: s3c2410_wdt: Add support for Watchdog device on Exynos7

[Guenter Roeck]

On Wed, Aug 27, 2014 at 03:17:11PM +0530, Naveen Krishna Chatradhi wrote:
 Exynos7 SoC has a Watchdog for Atlas (A57) cores
 This patch adds support for the Atlas watchdog.
 
 Signed-off-by: Naveen Krishna Chatradhi ch.nav...@samsung.com
 Cc: Wim Van Sebroeck w...@iguana.be

Reviewed-by: Guenter Roeck li...@roeck-us.net

 ---
  .../devicetree/bindings/watchdog/samsung-wdt.txt   |1 +
  drivers/watchdog/s3c2410_wdt.c |   11 +++
  2 files changed, 12 insertions(+)
 
 diff --git a/Documentation/devicetree/bindings/watchdog/samsung-wdt.txt 
 b/Documentation/devicetree/bindings/watchdog/samsung-wdt.txt
 index cfff375..8f3d96a 100644
 --- a/Documentation/devicetree/bindings/watchdog/samsung-wdt.txt
 +++ b/Documentation/devicetree/bindings/watchdog/samsung-wdt.txt
 @@ -9,6 +9,7 @@ Required properties:
   (a) samsung,s3c2410-wdt for Exynos4 and previous SoCs
   (b) samsung,exynos5250-wdt for Exynos5250
   (c) samsung,exynos5420-wdt for Exynos5420
 + (c) samsung,exynos7-wdt for Exynos7
  
  - reg : base physical address of the controller and length of memory mapped
   region.
 diff --git a/drivers/watchdog/s3c2410_wdt.c b/drivers/watchdog/s3c2410_wdt.c
 index 7c6ccd0..015256e 100644
 --- a/drivers/watchdog/s3c2410_wdt.c
 +++ b/drivers/watchdog/s3c2410_wdt.c
 @@ -155,6 +155,15 @@ static const struct s3c2410_wdt_variant 
 drv_data_exynos5420 = {
   .quirks = QUIRK_HAS_PMU_CONFIG | QUIRK_HAS_RST_STAT,
  };
  
 +static const struct s3c2410_wdt_variant drv_data_exynos7 = {
 + .disable_reg = EXYNOS5_WDT_DISABLE_REG_OFFSET,
 + .mask_reset_reg = EXYNOS5_WDT_MASK_RESET_REG_OFFSET,
 + .mask_bit = 0,
 + .rst_stat_reg = EXYNOS5_RST_STAT_REG_OFFSET,
 + .rst_stat_bit = 23, /* A57 WDTRESET */
 + .quirks = QUIRK_HAS_PMU_CONFIG | QUIRK_HAS_RST_STAT,
 +};
 +
  static const struct of_device_id s3c2410_wdt_match[] = {
   { .compatible = samsung,s3c2410-wdt,
 .data = drv_data_s3c2410 },
 @@ -162,6 +171,8 @@ static const struct of_device_id s3c2410_wdt_match[] = {
 .data = drv_data_exynos5250 },
   { .compatible = samsung,exynos5420-wdt,
 .data = drv_data_exynos5420 },
 + { .compatible = samsung,exynos7-wdt,
 +   .data = drv_data_exynos7 },
   {},
  };
  MODULE_DEVICE_TABLE(of, s3c2410_wdt_match);
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v7 00/11] kernel: Add support for restart handler call chain

[Guenter Roeck]

On Tue, Aug 19, 2014 at 05:45:27PM -0700, Guenter Roeck wrote:
 Various drivers implement architecture and/or device specific means
 to restart (reset) the system. Various mechanisms have been implemented
 to support those schemes. The best known mechanism is arm_pm_restart,
 which is a function pointer to be set either from platform specific code
 or from drivers. Another mechanism is to use hardware watchdogs to issue
 a reset; this mechanism is used if there is no other method available
 to reset a board or system. Two examples are alim7101_wdt, which currently
 uses the reboot notifier to trigger a reset, and moxart_wdt, which registers
 the arm_pm_restart function. Several other restart drivers for arm, all
 directly calling arm_pm_restart, are in the process of being integrated
 into the kernel. All those drivers would benefit from the new API.
 
 The existing mechanisms have a number of drawbacks. Typically only one scheme
 to restart the system is supported (at least if arm_pm_restart is used).
 At least in theory there can be multiple means to restart the system, some of
 which may be less desirable (for example one mechanism may only reset the CPU,
 while another may reset the entire system). Using arm_pm_restart can also be
 racy if the function pointer is set from a driver, as the driver may be in
 the process of being unloaded when arm_pm_restart is called.
 Using the reboot notifier is always racy, as it is unknown if and when
 other functions using the reboot notifier have completed execution
 by the time the watchdog fires.
 
 Introduce a system restart handler call chain to solve the described problems.
 This call chain is expected to be executed from the architecture specific
 machine_restart() function. Drivers providing system restart functionality
 (such as the watchdog drivers mentioned above) are expected to register
 with this call chain. By using the priority field in the notifier block,
 callers can control restart handler execution sequence and thus ensure that
 the restart handler with the optimal restart capabilities for a given system
 is called first.
 
 Since the first revision of this patchset, a number of separate patch
 submissions have been made which either depend on it or could make use of it.
 
 http://www.spinics.net/linux/lists/arm-kernel/msg344796.html
   registers three notifiers.
 https://lkml.org/lkml/2014/7/8/962
   would benefit from it.
 
 Patch 1 of this series implements the restart handler function. Patches 2 and 
 3
 implement calling the restart handler chain from arm and arm64 restart code.
 
 Patch 4 modifies the restart-poweroff driver to no longer call arm_pm_restart
 directly but machine_restart. This is done to avoid calling arm_pm_restart
 from more than one place. The change makes the driver architecture 
 independent,
 so it would be possible to drop the arm dependency from its Kconfig entry.
 
 Patch 5 and 6 convert existing restart handlers in the watchdog subsystem
 to use the restart handler. Patch 7 unexports arm_pm_restart to ensure
 that no one gets the idea to implement a restart handler as module.
 
 The entire patch series, including additional patches depending on it,
 is available from
 https://git.kernel.org/cgit/linux/kernel/git/groeck/linux-staging.git/
 in branch 'restart-staging'.
 
Hi Andrew,

I think this series is ready for upstream integration. Question now
is how we should proceed to get it actually integrated.

I can see a number of options:
- You take patch #1, the rest goes in through maintainer trees.
- You take all patches after we get missing maintainer Acks.
- I send a pull request directly to Linus after we get missing
  maintainer Acks.

What do you think would be the best way to proceed ?

Thanks,
Guenter
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v7 02/11] power/restart: Call machine_restart instead of arm_pm_restart

[Guenter Roeck]

On 08/21/2014 01:39 PM, Sebastian Reichel wrote:

Hi,

On Tue, Aug 19, 2014 at 05:45:29PM -0700, Guenter Roeck wrote:

machine_restart is supported on non-ARM platforms, and and ultimately calls
arm_pm_restart, so dont call arm_pm_restart directly but use the more
generic function.

Cc: Russell King li...@arm.linux.org.uk
Signed-off-by: Guenter Roeck li...@roeck-us.net
Acked-by: Catalin Marinas catalin.mari...@arm.com
Acked-by: Heiko Stuebner he...@sntech.de


Acked-by: Sebastian Reichel s...@kernel.org

Do you want me to take the patch via battery-2.6.git?



Hi Sebastian,

thanks a lot for the Ack. I am not sure if it is better to submit all
patches together or through individual maintainer trees. Let's wait for
Andrew's suggestion how we should proceed.

Thanks,
Guenter

--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v7 00/11] kernel: Add support for restart handler call chain

[Guenter Roeck]

On 08/23/2014 04:00 PM, Heiko Stübner wrote:

Am Samstag, 23. August 2014, 09:35:05 schrieb Guenter Roeck:

On Tue, Aug 19, 2014 at 05:45:27PM -0700, Guenter Roeck wrote:

Various drivers implement architecture and/or device specific means
to restart (reset) the system. Various mechanisms have been implemented
to support those schemes. The best known mechanism is arm_pm_restart,
which is a function pointer to be set either from platform specific code
or from drivers. Another mechanism is to use hardware watchdogs to issue
a reset; this mechanism is used if there is no other method available
to reset a board or system. Two examples are alim7101_wdt, which currently
uses the reboot notifier to trigger a reset, and moxart_wdt, which
registers the arm_pm_restart function. Several other restart drivers for
arm, all directly calling arm_pm_restart, are in the process of being
integrated into the kernel. All those drivers would benefit from the new
API.

The existing mechanisms have a number of drawbacks. Typically only one
scheme to restart the system is supported (at least if arm_pm_restart is
used). At least in theory there can be multiple means to restart the
system, some of which may be less desirable (for example one mechanism
may only reset the CPU, while another may reset the entire system). Using
arm_pm_restart can also be racy if the function pointer is set from a
driver, as the driver may be in the process of being unloaded when
arm_pm_restart is called.
Using the reboot notifier is always racy, as it is unknown if and when
other functions using the reboot notifier have completed execution
by the time the watchdog fires.

Introduce a system restart handler call chain to solve the described
problems. This call chain is expected to be executed from the
architecture specific machine_restart() function. Drivers providing
system restart functionality (such as the watchdog drivers mentioned
above) are expected to register with this call chain. By using the
priority field in the notifier block, callers can control restart handler
execution sequence and thus ensure that the restart handler with the
optimal restart capabilities for a given system is called first.

Since the first revision of this patchset, a number of separate patch
submissions have been made which either depend on it or could make use of
it.

http://www.spinics.net/linux/lists/arm-kernel/msg344796.html

registers three notifiers.

https://lkml.org/lkml/2014/7/8/962

would benefit from it.

Patch 1 of this series implements the restart handler function. Patches 2
and 3 implement calling the restart handler chain from arm and arm64
restart code.

Patch 4 modifies the restart-poweroff driver to no longer call
arm_pm_restart directly but machine_restart. This is done to avoid
calling arm_pm_restart from more than one place. The change makes the
driver architecture independent, so it would be possible to drop the arm
dependency from its Kconfig entry.

Patch 5 and 6 convert existing restart handlers in the watchdog subsystem
to use the restart handler. Patch 7 unexports arm_pm_restart to ensure
that no one gets the idea to implement a restart handler as module.

The entire patch series, including additional patches depending on it,
is available from
https://git.kernel.org/cgit/linux/kernel/git/groeck/linux-staging.git/
in branch 'restart-staging'.


Hi Andrew,

I think this series is ready for upstream integration. Question now
is how we should proceed to get it actually integrated.

I can see a number of options:
- You take patch #1, the rest goes in through maintainer trees.


I don't think you can split the patches like this. Patch1 introduces
(un)register_restart_handler functions used by later patches in the series.
You therefore cannot really split the series, as otherwise you would get build
failures in the individual trees.


No, it would simply delay integration of the entire series by a release
or two. First two patches go in first, then #3 and #4, then the rest.

I don't like that option too much either, but it is better than nothing.

Guenter

--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v7 02/11] power/restart: Call machine_restart instead of arm_pm_restart

[Guenter Roeck]

On Thu, Aug 21, 2014 at 12:30:44PM -0700, Doug Anderson wrote:
 Guenter,
 
 On Wed, Aug 20, 2014 at 9:42 PM, Guenter Roeck li...@roeck-us.net wrote:
  On Wed, Aug 20, 2014 at 09:10:31PM -0700, Doug Anderson wrote:
  Guenter,
 
  On Tue, Aug 19, 2014 at 5:45 PM, Guenter Roeck li...@roeck-us.net wrote:
   machine_restart is supported on non-ARM platforms, and and ultimately 
   calls
   arm_pm_restart, so dont call arm_pm_restart directly but use the more
   generic function.
  
   Cc: Russell King li...@arm.linux.org.uk
 
  Do you need to submit this to his patch tracker to get him to pick it
  up?  How are you envisioning that this series land?  It crosses a lot
  of boundaries so I guess will need a reasonable amount of coordination
  between maintainers...
 
 
  If I get an Acked-by: from all maintainers, I could send a pull request
  to Linus directly. How do I send a patch to Russell's patch tracker ?
  I thought I copied all mailing lists suggested by get_maintainer.pl,
  but maybe I missed one.
 
 Ah, OK.  Perhaps it's best to ignore me, then.  ;)  FYI: Russell's

No, I appreciate the information.

 Patch System is at http://www.arm.linux.org.uk/developer/patches/
 and is used for getting individual patches into branches maintained by
 Russel.  If you're just going to send a pull request for the series
 then I don't think you need it.
 
I copied both linux-arm-kernel and Russell himself, so hopefully he is aware
of the series and can let me know if he wants me to make any further changes
to it, if it is ready to go, or if I need to do anything else.

Thanks,
Guenter
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v7 04/11] arm: Support restart through restart handler call chain

[Guenter Roeck]

On Fri, Aug 22, 2014 at 03:32:42AM +0200, Andreas Färber wrote:
 Hi,
 
 Am 20.08.2014 02:45, schrieb Guenter Roeck:
  The kernel core now supports a restart handler call chain for system
  restart functions.
  
  With this change, the arm_pm_restart callback is now optional, so
  drop its initialization and check if it is set before calling it.
  Only call the kernel restart handler if arm_pm_restart is not set.
 [...]
  diff --git a/arch/arm/kernel/process.c b/arch/arm/kernel/process.c
  index 81ef686..ea279f7 100644
  --- a/arch/arm/kernel/process.c
  +++ b/arch/arm/kernel/process.c
  @@ -114,17 +114,13 @@ void soft_restart(unsigned long addr)
  BUG();
   }
   
  -static void null_restart(enum reboot_mode reboot_mode, const char *cmd)
  -{
  -}
  -
   /*
* Function pointers to optional machine specific functions
*/
   void (*pm_power_off)(void);
   EXPORT_SYMBOL(pm_power_off);
   
  -void (*arm_pm_restart)(enum reboot_mode reboot_mode, const char *cmd) = 
  null_restart;
  +void (*arm_pm_restart)(enum reboot_mode reboot_mode, const char *cmd);
 
 Stupid newbie question maybe, but isn't this variable uninitialized now,
 like any non-static variable in C99? Or does the kernel assure that all
 such fields are zero-initialized?
 
It is initialized with NULL, like all other global and static variables in the
kernel (and like pm_power_off a few lines above).

Guenter
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v7 02/11] power/restart: Call machine_restart instead of arm_pm_restart

[Guenter Roeck]

On Wed, Aug 20, 2014 at 09:10:31PM -0700, Doug Anderson wrote:
 Guenter,
 
 On Tue, Aug 19, 2014 at 5:45 PM, Guenter Roeck li...@roeck-us.net wrote:
  machine_restart is supported on non-ARM platforms, and and ultimately calls
  arm_pm_restart, so dont call arm_pm_restart directly but use the more
  generic function.
 
  Cc: Russell King li...@arm.linux.org.uk
 
 Do you need to submit this to his patch tracker to get him to pick it
 up?  How are you envisioning that this series land?  It crosses a lot
 of boundaries so I guess will need a reasonable amount of coordination
 between maintainers...
 
 
If I get an Acked-by: from all maintainers, I could send a pull request
to Linus directly. How do I send a patch to Russell's patch tracker ?
I thought I copied all mailing lists suggested by get_maintainer.pl,
but maybe I missed one.

Thanks,
Guenter
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v7 11/11] clk: rockchip: add restart handler

[Guenter Roeck]

From: Heiko Stübner he...@sntech.de

Add infrastructure to write the correct value to the restart register and
register the restart notifier for both rk3188 (including rk3066) and rk3288.

Signed-off-by: Heiko Stuebner he...@sntech.de
Signed-off-by: Guenter Roeck li...@roeck-us.net
---
v7: Added patch to series.

 drivers/clk/rockchip/clk-rk3188.c |  2 ++
 drivers/clk/rockchip/clk-rk3288.c |  2 ++
 drivers/clk/rockchip/clk.c| 25 +
 drivers/clk/rockchip/clk.h|  1 +
 4 files changed, 30 insertions(+)

diff --git a/drivers/clk/rockchip/clk-rk3188.c 
b/drivers/clk/rockchip/clk-rk3188.c
index a83a6d8..71b661a 100644
--- a/drivers/clk/rockchip/clk-rk3188.c
+++ b/drivers/clk/rockchip/clk-rk3188.c
@@ -631,6 +631,8 @@ static void __init rk3188_common_clk_init(struct 
device_node *np)
 
rockchip_register_softrst(np, 9, reg_base + RK2928_SOFTRST_CON(0),
  ROCKCHIP_SOFTRST_HIWORD_MASK);
+
+   rockchip_register_restart_notifier(RK2928_GLB_SRST_FST);
 }
 
 static void __init rk3066a_clk_init(struct device_node *np)
diff --git a/drivers/clk/rockchip/clk-rk3288.c 
b/drivers/clk/rockchip/clk-rk3288.c
index 0d8c6c5..b604217 100644
--- a/drivers/clk/rockchip/clk-rk3288.c
+++ b/drivers/clk/rockchip/clk-rk3288.c
@@ -713,5 +713,7 @@ static void __init rk3288_clk_init(struct device_node *np)
 
rockchip_register_softrst(np, 9, reg_base + RK3288_SOFTRST_CON(0),
  ROCKCHIP_SOFTRST_HIWORD_MASK);
+
+   rockchip_register_restart_notifier(RK3288_GLB_SRST_FST);
 }
 CLK_OF_DECLARE(rk3288_cru, rockchip,rk3288-cru, rk3288_clk_init);
diff --git a/drivers/clk/rockchip/clk.c b/drivers/clk/rockchip/clk.c
index 278cf9d..aa41433 100644
--- a/drivers/clk/rockchip/clk.c
+++ b/drivers/clk/rockchip/clk.c
@@ -25,6 +25,7 @@
 #include linux/clk-provider.h
 #include linux/mfd/syscon.h
 #include linux/regmap.h
+#include linux/reboot.h
 #include clk.h
 
 /**
@@ -242,3 +243,27 @@ void __init rockchip_clk_register_branches(
rockchip_clk_add_lookup(clk, list-id);
}
 }
+
+static unsigned int reg_restart;
+static int rockchip_restart_notify(struct notifier_block *this,
+  unsigned long mode, void *cmd)
+{
+   writel(0xfdb9, reg_base + reg_restart);
+   return NOTIFY_DONE;
+}
+
+static struct notifier_block rockchip_restart_handler = {
+   .notifier_call = rockchip_restart_notify,
+   .priority = 128,
+};
+
+void __init rockchip_register_restart_notifier(unsigned int reg)
+{
+   int ret;
+
+   reg_restart = reg;
+   ret = register_restart_handler(rockchip_restart_handler);
+   if (ret)
+   pr_err(%s: cannot register restart handler, %d\n,
+  __func__, ret);
+}
diff --git a/drivers/clk/rockchip/clk.h b/drivers/clk/rockchip/clk.h
index 887cbde..0b5eab5 100644
--- a/drivers/clk/rockchip/clk.h
+++ b/drivers/clk/rockchip/clk.h
@@ -329,6 +329,7 @@ void rockchip_clk_register_branches(struct 
rockchip_clk_branch *clk_list,
unsigned int nr_clk);
 void rockchip_clk_register_plls(struct rockchip_pll_clock *pll_list,
unsigned int nr_pll, int grf_lock_offset);
+void rockchip_register_restart_notifier(unsigned int reg);
 
 #define ROCKCHIP_SOFTRST_HIWORD_MASK   BIT(0)
 
-- 
1.9.1

--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v7 10/11] clk: samsung: register restart handlers for s3c2412 and s3c2443

[Guenter Roeck]

From: Heiko Stübner he...@sntech.de

S3C2412, S3C2443 and their derivatives contain a special software-reset
register in their system-controller.

Therefore register a restart handler for those.

Tested on a s3c2416-based board, s3c2412 compile-tested.

Signed-off-by: Heiko Stuebner he...@sntech.de
Signed-off-by: Guenter Roeck li...@roeck-us.net
---
v7: Added patch to series.

 drivers/clk/samsung/clk-s3c2412.c | 29 +
 drivers/clk/samsung/clk-s3c2443.c | 19 +++
 2 files changed, 48 insertions(+)

diff --git a/drivers/clk/samsung/clk-s3c2412.c 
b/drivers/clk/samsung/clk-s3c2412.c
index 34af09f..2ceedaf 100644
--- a/drivers/clk/samsung/clk-s3c2412.c
+++ b/drivers/clk/samsung/clk-s3c2412.c
@@ -14,6 +14,7 @@
 #include linux/of.h
 #include linux/of_address.h
 #include linux/syscore_ops.h
+#include linux/reboot.h
 
 #include dt-bindings/clock/s3c2412.h
 
@@ -26,6 +27,7 @@
 #define CLKCON 0x0c
 #define CLKDIVN0x14
 #define CLKSRC 0x1c
+#define SWRST  0x30
 
 /* list of PLLs to be registered */
 enum s3c2412_plls {
@@ -204,6 +206,28 @@ struct samsung_clock_alias s3c2412_aliases[] __initdata = {
ALIAS(MSYSCLK, NULL, fclk),
 };
 
+static int s3c2412_restart(struct notifier_block *this,
+  unsigned long mode, void *cmd)
+{
+   /* errata Watch-dog/Software Reset Problem specifies that
+* this reset must be done with the SYSCLK sourced from
+* EXTCLK instead of FOUT to avoid a glitch in the reset
+* mechanism.
+*
+* See the watchdog section of the S3C2412 manual for more
+* information on this fix.
+*/
+
+   __raw_writel(0x00, reg_base + CLKSRC);
+   __raw_writel(0x533C2412, reg_base + SWRST);
+   return NOTIFY_DONE;
+}
+
+static struct notifier_block s3c2412_restart_handler = {
+   .notifier_call = s3c2412_restart,
+   .priority = 129,
+};
+
 /*
  * fixed rate clocks generated outside the soc
  * Only necessary until the devicetree-move is complete
@@ -233,6 +257,7 @@ void __init s3c2412_common_clk_init(struct device_node *np, 
unsigned long xti_f,
unsigned long ext_f, void __iomem *base)
 {
struct samsung_clk_provider *ctx;
+   int ret;
reg_base = base;
 
if (np) {
@@ -267,6 +292,10 @@ void __init s3c2412_common_clk_init(struct device_node 
*np, unsigned long xti_f,
s3c2412_clk_sleep_init();
 
samsung_clk_of_add_provider(np, ctx);
+
+   ret = register_restart_handler(s3c2412_restart_handler);
+   if (ret)
+   pr_warn(cannot register restart handler, %d\n, ret);
 }
 
 static void __init s3c2412_clk_init(struct device_node *np)
diff --git a/drivers/clk/samsung/clk-s3c2443.c 
b/drivers/clk/samsung/clk-s3c2443.c
index c92f853..0c3c182 100644
--- a/drivers/clk/samsung/clk-s3c2443.c
+++ b/drivers/clk/samsung/clk-s3c2443.c
@@ -14,6 +14,7 @@
 #include linux/of.h
 #include linux/of_address.h
 #include linux/syscore_ops.h
+#include linux/reboot.h
 
 #include dt-bindings/clock/s3c2443.h
 
@@ -33,6 +34,7 @@
 #define HCLKCON0x30
 #define PCLKCON0x34
 #define SCLKCON0x38
+#define SWRST  0x44
 
 /* the soc types */
 enum supported_socs {
@@ -354,6 +356,18 @@ struct samsung_clock_alias s3c2450_aliases[] __initdata = {
ALIAS(PCLK_I2C1, s3c2410-i2c.1, i2c),
 };
 
+static int s3c2443_restart(struct notifier_block *this,
+  unsigned long mode, void *cmd)
+{
+   __raw_writel(0x533c2443, reg_base + SWRST);
+   return NOTIFY_DONE;
+}
+
+static struct notifier_block s3c2443_restart_handler = {
+   .notifier_call = s3c2443_restart,
+   .priority = 129,
+};
+
 /*
  * fixed rate clocks generated outside the soc
  * Only necessary until the devicetree-move is complete
@@ -378,6 +392,7 @@ void __init s3c2443_common_clk_init(struct device_node *np, 
unsigned long xti_f,
void __iomem *base)
 {
struct samsung_clk_provider *ctx;
+   int ret;
reg_base = base;
 
if (np) {
@@ -447,6 +462,10 @@ void __init s3c2443_common_clk_init(struct device_node 
*np, unsigned long xti_f,
s3c2443_clk_sleep_init();
 
samsung_clk_of_add_provider(np, ctx);
+
+   ret = register_restart_handler(s3c2443_restart_handler);
+   if (ret)
+   pr_warn(cannot register restart handler, %d\n, ret);
 }
 
 static void __init s3c2416_clk_init(struct device_node *np)
-- 
1.9.1

--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v7 07/11] watchdog: sunxi: Register restart handler with kernel restart handler

[Guenter Roeck]

The kernel core now provides an API to trigger a system restart.
Register with it instead of setting arm_pm_restart.

Signed-off-by: Guenter Roeck li...@roeck-us.net
Acked-by: Heiko Stuebner he...@sntech.de
---
v7: Added patch to series. Necessary since the restart handler in the driver
is now available upstream.

 drivers/watchdog/sunxi_wdt.c | 31 ---
 1 file changed, 20 insertions(+), 11 deletions(-)

diff --git a/drivers/watchdog/sunxi_wdt.c b/drivers/watchdog/sunxi_wdt.c
index 60deb9d..480bb55 100644
--- a/drivers/watchdog/sunxi_wdt.c
+++ b/drivers/watchdog/sunxi_wdt.c
@@ -21,14 +21,13 @@
 #include linux/kernel.h
 #include linux/module.h
 #include linux/moduleparam.h
+#include linux/notifier.h
 #include linux/of.h
 #include linux/platform_device.h
 #include linux/reboot.h
 #include linux/types.h
 #include linux/watchdog.h
 
-#include asm/system_misc.h
-
 #define WDT_MAX_TIMEOUT 16
 #define WDT_MIN_TIMEOUT 1
 #define WDT_MODE_TIMEOUT(n) ((n)  3)
@@ -50,6 +49,7 @@ static unsigned int timeout = WDT_MAX_TIMEOUT;
 struct sunxi_wdt_dev {
struct watchdog_device wdt_dev;
void __iomem *wdt_base;
+   struct notifier_block restart_handler;
 };
 
 /*
@@ -74,24 +74,29 @@ static const int wdt_timeout_map[] = {
[16] = 0xB, /* 16s */
 };
 
-static void __iomem *reboot_wdt_base;
 
-static void sun4i_wdt_restart(enum reboot_mode mode, const char *cmd)
+static int sunxi_restart_handle(struct notifier_block *this, unsigned long 
mode,
+   void *cmd)
 {
+   struct sunxi_wdt_dev *sunxi_wdt = container_of(this,
+  struct sunxi_wdt_dev,
+  restart_handler);
+   void __iomem *wdt_base = sunxi_wdt-wdt_base;
+
/* Enable timer and set reset bit in the watchdog */
-   writel(WDT_MODE_EN | WDT_MODE_RST_EN, reboot_wdt_base + WDT_MODE);
+   writel(WDT_MODE_EN | WDT_MODE_RST_EN, wdt_base + WDT_MODE);
 
/*
 * Restart the watchdog. The default (and lowest) interval
 * value for the watchdog is 0.5s.
 */
-   writel(WDT_CTRL_RELOAD, reboot_wdt_base + WDT_CTRL);
+   writel(WDT_CTRL_RELOAD, wdt_base + WDT_CTRL);
 
while (1) {
mdelay(5);
-   writel(WDT_MODE_EN | WDT_MODE_RST_EN,
-  reboot_wdt_base + WDT_MODE);
+   writel(WDT_MODE_EN | WDT_MODE_RST_EN, wdt_base + WDT_MODE);
}
+   return NOTIFY_DONE;
 }
 
 static int sunxi_wdt_ping(struct watchdog_device *wdt_dev)
@@ -205,8 +210,12 @@ static int sunxi_wdt_probe(struct platform_device *pdev)
if (unlikely(err))
return err;
 
-   reboot_wdt_base = sunxi_wdt-wdt_base;
-   arm_pm_restart = sun4i_wdt_restart;
+   sunxi_wdt-restart_handler.notifier_call = sunxi_restart_handle;
+   sunxi_wdt-restart_handler.priority = 128;
+   err = register_restart_handler(sunxi_wdt-restart_handler);
+   if (err)
+   dev_err(pdev-dev,
+   cannot register restart handler (err=%d)\n, err);
 
dev_info(pdev-dev, Watchdog enabled (timeout=%d sec, nowayout=%d),
sunxi_wdt-wdt_dev.timeout, nowayout);
@@ -218,7 +227,7 @@ static int sunxi_wdt_remove(struct platform_device *pdev)
 {
struct sunxi_wdt_dev *sunxi_wdt = platform_get_drvdata(pdev);
 
-   arm_pm_restart = NULL;
+   unregister_restart_handler(sunxi_wdt-restart_handler);
 
watchdog_unregister_device(sunxi_wdt-wdt_dev);
watchdog_set_drvdata(sunxi_wdt-wdt_dev, NULL);
-- 
1.9.1

--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v7 08/11] arm/arm64: Unexport restart handlers

[Guenter Roeck]

Implementing a restart handler in a module don't make sense
as there would be no guarantee that the module is loaded when
a restart is needed. Unexport arm_pm_restart to ensure that
no one gets the idea to do it anyway.

Signed-off-by: Guenter Roeck li...@roeck-us.net
Acked-by: Catalin Marinas catalin.mari...@arm.com
Acked-by: Heiko Stuebner he...@sntech.de
---
v7: No change
v6: No change
v5: No change
v4: No change
v3: No change
v2: No change

 arch/arm/kernel/process.c   | 1 -
 arch/arm64/kernel/process.c | 1 -
 2 files changed, 2 deletions(-)

diff --git a/arch/arm/kernel/process.c b/arch/arm/kernel/process.c
index ea279f7..250b6f6 100644
--- a/arch/arm/kernel/process.c
+++ b/arch/arm/kernel/process.c
@@ -121,7 +121,6 @@ void (*pm_power_off)(void);
 EXPORT_SYMBOL(pm_power_off);
 
 void (*arm_pm_restart)(enum reboot_mode reboot_mode, const char *cmd);
-EXPORT_SYMBOL_GPL(arm_pm_restart);
 
 /*
  * This is our default idle handler.
diff --git a/arch/arm64/kernel/process.c b/arch/arm64/kernel/process.c
index 0d3fb9f..398ab05 100644
--- a/arch/arm64/kernel/process.c
+++ b/arch/arm64/kernel/process.c
@@ -98,7 +98,6 @@ void (*pm_power_off)(void);
 EXPORT_SYMBOL_GPL(pm_power_off);
 
 void (*arm_pm_restart)(enum reboot_mode reboot_mode, const char *cmd);
-EXPORT_SYMBOL_GPL(arm_pm_restart);
 
 /*
  * This is our default idle handler.
-- 
1.9.1

--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v7 06/11] watchdog: alim7101: Register restart handler with kernel restart handler

[Guenter Roeck]

The kernel core now provides an API to trigger a system restart.
Register with it to restart the system instead of misusing the
reboot notifier.

Signed-off-by: Guenter Roeck li...@roeck-us.net
Acked-by: Catalin Marinas catalin.mari...@arm.com
Acked-by: Heiko Stuebner he...@sntech.de
---
v7: No change.
v6: No change.
v5: Function and variable renames: *notifier - *handler.
v4: Set restart notifier priority to 128.
v3: No change.
v2: No change.

 drivers/watchdog/alim7101_wdt.c | 42 +++--
 1 file changed, 32 insertions(+), 10 deletions(-)

diff --git a/drivers/watchdog/alim7101_wdt.c b/drivers/watchdog/alim7101_wdt.c
index 996b2f7..665e0e7 100644
--- a/drivers/watchdog/alim7101_wdt.c
+++ b/drivers/watchdog/alim7101_wdt.c
@@ -301,6 +301,28 @@ static struct miscdevice wdt_miscdev = {
.fops   =   wdt_fops,
 };
 
+static int wdt_restart_handle(struct notifier_block *this, unsigned long mode,
+ void *cmd)
+{
+   /*
+* Cobalt devices have no way of rebooting themselves other
+* than getting the watchdog to pull reset, so we restart the
+* watchdog on reboot with no heartbeat.
+*/
+   wdt_change(WDT_ENABLE);
+
+   /* loop until the watchdog fires */
+   while (true)
+   ;
+
+   return NOTIFY_DONE;
+}
+
+static struct notifier_block wdt_restart_handler = {
+   .notifier_call = wdt_restart_handle,
+   .priority = 128,
+};
+
 /*
  * Notifier for system down
  */
@@ -311,15 +333,6 @@ static int wdt_notify_sys(struct notifier_block *this,
if (code == SYS_DOWN || code == SYS_HALT)
wdt_turnoff();
 
-   if (code == SYS_RESTART) {
-   /*
-* Cobalt devices have no way of rebooting themselves other
-* than getting the watchdog to pull reset, so we restart the
-* watchdog on reboot with no heartbeat
-*/
-   wdt_change(WDT_ENABLE);
-   pr_info(Watchdog timer is now enabled with no heartbeat - 
should reboot in ~1 second\n);
-   }
return NOTIFY_DONE;
 }
 
@@ -338,6 +351,7 @@ static void __exit alim7101_wdt_unload(void)
/* Deregister */
misc_deregister(wdt_miscdev);
unregister_reboot_notifier(wdt_notifier);
+   unregister_restart_handler(wdt_restart_handler);
pci_dev_put(alim7101_pmu);
 }
 
@@ -390,11 +404,17 @@ static int __init alim7101_wdt_init(void)
goto err_out;
}
 
+   rc = register_restart_handler(wdt_restart_handler);
+   if (rc) {
+   pr_err(cannot register restart handler (err=%d)\n, rc);
+   goto err_out_reboot;
+   }
+
rc = misc_register(wdt_miscdev);
if (rc) {
pr_err(cannot register miscdev on minor=%d (err=%d)\n,
   wdt_miscdev.minor, rc);
-   goto err_out_reboot;
+   goto err_out_restart;
}
 
if (nowayout)
@@ -404,6 +424,8 @@ static int __init alim7101_wdt_init(void)
timeout, nowayout);
return 0;
 
+err_out_restart:
+   unregister_restart_handler(wdt_restart_handler);
 err_out_reboot:
unregister_reboot_notifier(wdt_notifier);
 err_out:
-- 
1.9.1

--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v7 02/11] power/restart: Call machine_restart instead of arm_pm_restart

[Guenter Roeck]

machine_restart is supported on non-ARM platforms, and and ultimately calls
arm_pm_restart, so dont call arm_pm_restart directly but use the more
generic function.

Cc: Russell King li...@arm.linux.org.uk
Signed-off-by: Guenter Roeck li...@roeck-us.net
Acked-by: Catalin Marinas catalin.mari...@arm.com
Acked-by: Heiko Stuebner he...@sntech.de
---
v7: No change.
v6: No change.
v5: No change.
v4: No change.
v3: No change.
v2: Added patch.

 drivers/power/reset/restart-poweroff.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/power/reset/restart-poweroff.c 
b/drivers/power/reset/restart-poweroff.c
index 3e51f8d..edd707e 100644
--- a/drivers/power/reset/restart-poweroff.c
+++ b/drivers/power/reset/restart-poweroff.c
@@ -20,7 +20,8 @@
 
 static void restart_poweroff_do_poweroff(void)
 {
-   arm_pm_restart(REBOOT_HARD, NULL);
+   reboot_mode = REBOOT_HARD;
+   machine_restart(NULL);
 }
 
 static int restart_poweroff_probe(struct platform_device *pdev)
-- 
1.9.1

--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v7 04/11] arm: Support restart through restart handler call chain

[Guenter Roeck]

The kernel core now supports a restart handler call chain for system
restart functions.

With this change, the arm_pm_restart callback is now optional, so
drop its initialization and check if it is set before calling it.
Only call the kernel restart handler if arm_pm_restart is not set.

Signed-off-by: Guenter Roeck li...@roeck-us.net
Acked-by: Catalin Marinas catalin.mari...@arm.com
Acked-by: Heiko Stuebner he...@sntech.de
---
v7: Dropped null_restart and made arm_pm_restart truly optional.
v6: No change.
v5: Renamed restart function to do_kernel_restart
v4: No change.
v3: Use wrapper function to execute notifier call chain.
v2: Only call notifier call chain if arm_pm_restart is not set.
Do not include linux/watchdog.h.

 arch/arm/kernel/process.c | 11 +--
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/arch/arm/kernel/process.c b/arch/arm/kernel/process.c
index 81ef686..ea279f7 100644
--- a/arch/arm/kernel/process.c
+++ b/arch/arm/kernel/process.c
@@ -114,17 +114,13 @@ void soft_restart(unsigned long addr)
BUG();
 }
 
-static void null_restart(enum reboot_mode reboot_mode, const char *cmd)
-{
-}
-
 /*
  * Function pointers to optional machine specific functions
  */
 void (*pm_power_off)(void);
 EXPORT_SYMBOL(pm_power_off);
 
-void (*arm_pm_restart)(enum reboot_mode reboot_mode, const char *cmd) = 
null_restart;
+void (*arm_pm_restart)(enum reboot_mode reboot_mode, const char *cmd);
 EXPORT_SYMBOL_GPL(arm_pm_restart);
 
 /*
@@ -230,7 +226,10 @@ void machine_restart(char *cmd)
local_irq_disable();
smp_send_stop();
 
-   arm_pm_restart(reboot_mode, cmd);
+   if (arm_pm_restart)
+   arm_pm_restart(reboot_mode, cmd);
+   else
+   do_kernel_restart(cmd);
 
/* Give a grace period for failure to restart of 1s */
mdelay(1000);
-- 
1.9.1

--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v7 05/11] watchdog: moxart: Register restart handler with kernel restart handler

[Guenter Roeck]

The kernel now provides an API to trigger a system restart.
Register with it instead of setting arm_pm_restart.

Signed-off-by: Guenter Roeck li...@roeck-us.net
Acked-by: Catalin Marinas catalin.mari...@arm.com
Acked-by: Heiko Stuebner he...@sntech.de
---
v7: No change.
v6: No change.
v5: Functions and variables renamed: *notifier - *handler
v4: Set notifier priority to 128.
v3: Move struct notifier_block into struct moxart_wdt_dev.
Drop static variable previously needed to access struct moxart_wdt_dev
from notifier function; use container_of instead.
v2: No change.

 drivers/watchdog/moxart_wdt.c | 32 
 1 file changed, 20 insertions(+), 12 deletions(-)

diff --git a/drivers/watchdog/moxart_wdt.c b/drivers/watchdog/moxart_wdt.c
index 4aa3a8a..a64405b 100644
--- a/drivers/watchdog/moxart_wdt.c
+++ b/drivers/watchdog/moxart_wdt.c
@@ -15,12 +15,12 @@
 #include linux/module.h
 #include linux/err.h
 #include linux/kernel.h
+#include linux/notifier.h
 #include linux/platform_device.h
+#include linux/reboot.h
 #include linux/watchdog.h
 #include linux/moduleparam.h
 
-#include asm/system_misc.h
-
 #define REG_COUNT  0x4
 #define REG_MODE   0x8
 #define REG_ENABLE 0xC
@@ -29,17 +29,22 @@ struct moxart_wdt_dev {
struct watchdog_device dev;
void __iomem *base;
unsigned int clock_frequency;
+   struct notifier_block restart_handler;
 };
 
-static struct moxart_wdt_dev *moxart_restart_ctx;
-
 static int heartbeat;
 
-static void moxart_wdt_restart(enum reboot_mode reboot_mode, const char *cmd)
+static int moxart_restart_handle(struct notifier_block *this,
+unsigned long mode, void *cmd)
 {
-   writel(1, moxart_restart_ctx-base + REG_COUNT);
-   writel(0x5ab9, moxart_restart_ctx-base + REG_MODE);
-   writel(0x03, moxart_restart_ctx-base + REG_ENABLE);
+   struct moxart_wdt_dev *moxart_wdt = container_of(this,
+struct moxart_wdt_dev,
+restart_handler);
+   writel(1, moxart_wdt-base + REG_COUNT);
+   writel(0x5ab9, moxart_wdt-base + REG_MODE);
+   writel(0x03, moxart_wdt-base + REG_ENABLE);
+
+   return NOTIFY_DONE;
 }
 
 static int moxart_wdt_stop(struct watchdog_device *wdt_dev)
@@ -136,8 +141,12 @@ static int moxart_wdt_probe(struct platform_device *pdev)
if (err)
return err;
 
-   moxart_restart_ctx = moxart_wdt;
-   arm_pm_restart = moxart_wdt_restart;
+   moxart_wdt-restart_handler.notifier_call = moxart_restart_handle;
+   moxart_wdt-restart_handler.priority = 128;
+   err = register_restart_handler(moxart_wdt-restart_handler);
+   if (err)
+   dev_err(dev, cannot register restart notifier (err=%d)\n,
+   err);
 
dev_dbg(dev, Watchdog enabled (heartbeat=%d sec, nowayout=%d)\n,
moxart_wdt-dev.timeout, nowayout);
@@ -149,9 +158,8 @@ static int moxart_wdt_remove(struct platform_device *pdev)
 {
struct moxart_wdt_dev *moxart_wdt = platform_get_drvdata(pdev);
 
-   arm_pm_restart = NULL;
+   unregister_restart_handler(moxart_wdt-restart_handler);
moxart_wdt_stop(moxart_wdt-dev);
-   watchdog_unregister_device(moxart_wdt-dev);
 
return 0;
 }
-- 
1.9.1

--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v7 01/11] kernel: Add support for kernel restart handler call chain

[Guenter Roeck]

Various drivers implement architecture and/or device specific means
to restart (reset) the system. Various mechanisms have been implemented
to support those schemes. The best known mechanism is arm_pm_restart,
which is a function pointer to be set either from platform specific code
or from drivers. Another mechanism is to use hardware watchdogs to issue
a reset; this mechanism is used if there is no other method available
to reset a board or system. Two examples are alim7101_wdt, which currently
uses the reboot notifier to trigger a reset, and moxart_wdt, which registers
the arm_pm_restart function.

The existing mechanisms have a number of drawbacks. Typically only one scheme
to restart the system is supported (at least if arm_pm_restart is used).
At least in theory there can be multiple means to restart the system, some of
which may be less desirable (for example one mechanism may only reset the CPU,
while another may reset the entire system). Using arm_pm_restart can also be
racy if the function pointer is set from a driver, as the driver may be in
the process of being unloaded when arm_pm_restart is called.
Using the reboot notifier is always racy, as it is unknown if and when
other functions using the reboot notifier have completed execution
by the time the watchdog fires.

Introduce a system restart handler call chain to solve the described problems.
This call chain is expected to be executed from the architecture specific
machine_restart() function. Drivers providing system restart functionality
(such as the watchdog drivers mentioned above) are expected to register
with this call chain. By using the priority field in the notifier block,
callers can control restart handler execution sequence and thus ensure that
the restart handler with the optimal restart capabilities for a given system
is called first.

Signed-off-by: Guenter Roeck li...@roeck-us.net
Acked-by: Catalin Marinas catalin.mari...@arm.com
Acked-by: Heiko Stuebner he...@sntech.de
---
v7: Rebased to v3.17-rc1
v6: Use atomic notifier call chain
v5: Function renames:
register_restart_notifier - register_restart_handler
unregister_restart_notifier - unregister_restart_handler
kernel_restart_notify - do_kernel_restart
v4: Document and suggest values for notifier priorities
v3: Add kernel_restart_notify wrapper function to execute notifier.
Improve documentation.
Move restart_notifier_list into kernel/reboot.c and make it static.
v2: No change.

 include/linux/reboot.h |  3 ++
 kernel/reboot.c| 81 ++
 2 files changed, 84 insertions(+)

diff --git a/include/linux/reboot.h b/include/linux/reboot.h
index 48bf152..67fc8fc 100644
--- a/include/linux/reboot.h
+++ b/include/linux/reboot.h
@@ -38,6 +38,9 @@ extern int reboot_force;
 extern int register_reboot_notifier(struct notifier_block *);
 extern int unregister_reboot_notifier(struct notifier_block *);
 
+extern int register_restart_handler(struct notifier_block *);
+extern int unregister_restart_handler(struct notifier_block *);
+extern void do_kernel_restart(char *cmd);
 
 /*
  * Architecture-specific implementations of sys_reboot commands.
diff --git a/kernel/reboot.c b/kernel/reboot.c
index a3a9e24..5925f5a 100644
--- a/kernel/reboot.c
+++ b/kernel/reboot.c
@@ -104,6 +104,87 @@ int unregister_reboot_notifier(struct notifier_block *nb)
 }
 EXPORT_SYMBOL(unregister_reboot_notifier);
 
+/*
+ * Notifier list for kernel code which wants to be called
+ * to restart the system.
+ */
+static ATOMIC_NOTIFIER_HEAD(restart_handler_list);
+
+/**
+ * register_restart_handler - Register function to be called to reset
+ *the system
+ * @nb: Info about handler function to be called
+ * @nb-priority:  Handler priority. Handlers should follow the
+ * following guidelines for setting priorities.
+ * 0:  Restart handler of last resort,
+ * with limited restart capabilities
+ * 128:Default restart handler; use if no other
+ * restart handler is expected to be available,
+ * and/or if restart functionality is
+ * sufficient to restart the entire system
+ * 255:Highest priority restart handler, will
+ * preempt all other restart handlers
+ *
+ * Registers a function with code to be called to restart the
+ * system.
+ *
+ * Registered functions will be called from machine_restart as last
+ * step of the restart sequence (if the architecture specific
+ * machine_restart function calls do_kernel_restart - see below
+ * for details).
+ * Registered functions are expected to restart the system immediately.
+ * If more than one function is registered, the restart handler priority
+ * selects which function will be called first

[PATCH v7 03/11] arm64: Support restart through restart handler call chain

[Guenter Roeck]

The kernel core now supports a restart handler call chain to restart
the system. Call it if arm_pm_restart is not set.

Signed-off-by: Guenter Roeck li...@roeck-us.net
Acked-by: Catalin Marinas catalin.mari...@arm.com
Acked-by: Heiko Stuebner he...@sntech.de
---
v7: No change.
v6: No change.
v5: Renamed restart function to do_kernel_restart
v4: No change.
v3: Use wrapper function to execute notifier call chain.
v2: Only call notifier call chain if arm_pm_restart is not set.
Do not include linux/watchdog.h.

 arch/arm64/kernel/process.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/arm64/kernel/process.c b/arch/arm64/kernel/process.c
index 1309d64..0d3fb9f 100644
--- a/arch/arm64/kernel/process.c
+++ b/arch/arm64/kernel/process.c
@@ -180,6 +180,8 @@ void machine_restart(char *cmd)
/* Now call the architecture specific reboot code. */
if (arm_pm_restart)
arm_pm_restart(reboot_mode, cmd);
+   else
+   do_kernel_restart(cmd);
 
/*
 * Whoops - the architecture was unable to reboot.
-- 
1.9.1

--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC PATCH 1/3] watchdog: s3c2410: add restart notifier

[Guenter Roeck]

On Tue, Jul 08, 2014 at 04:21:09PM +0200, Tomasz Figa wrote:
 Hi Heiko,
 
 On 06.07.2014 20:42, Heiko Stübner wrote:
  On a lot of Samsung systems the watchdog is responsible for restarting the
  system and until now this code was contained in 
  plat-samsung/watchdog-reset.c .
  
  With the introduction of the restart notifiers, this code can now move into
  driver itself, removing the need for arch-specific code.
  
  Tested on a S3C2442 based GTA02
  Signed-off-by: Heiko Stuebner he...@sntech.de
  ---
   drivers/watchdog/s3c2410_wdt.c | 33 +
   1 file changed, 33 insertions(+)
  
  diff --git a/drivers/watchdog/s3c2410_wdt.c b/drivers/watchdog/s3c2410_wdt.c
  index 7c6ccd0..3f89912 100644
  --- a/drivers/watchdog/s3c2410_wdt.c
  +++ b/drivers/watchdog/s3c2410_wdt.c
  @@ -41,6 +41,7 @@
   #include linux/of.h
   #include linux/mfd/syscon.h
   #include linux/regmap.h
  +#include linux/reboot.h
   
   #define S3C2410_WTCON  0x00
   #define S3C2410_WTDAT  0x04
  @@ -438,6 +439,31 @@ static inline void 
  s3c2410wdt_cpufreq_deregister(struct s3c2410_wdt *wdt)
   }
   #endif
   
  +static struct s3c2410_wdt *s3c2410wdt_restart_ctx;
 
 This isn't the most elegant way to store context data. Maybe you could
 embed the notifier_block struct into s3c2410_wdt struct and then use
 container of to retrieve it from s3c2410wdt_restart_notify()?
 
Excellent idea. I'll do that for the moxart handler as well.

Guenter
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/4 v2] devicetree: bindings: Document murata vendor prefix

[Guenter Roeck]

On 06/24/2014 11:29 PM, Naveen Krishna Chatradhi wrote:

Add Murata Manufacturing Co., Ltd. to the list of device tree
vendor prefixes.

Murata manufactures NTC (Negative Temperature Coefficient) based
Thermistors for small scale applications like Mobiles and PDAs.

Signed-off-by: Naveen Krishna Chatradhi ch.nav...@samsung.com
Acked-by: Mark Rutland mark.rutl...@arm.com
Cc: Guenter Roeck li...@roeck-us.net


Applied.

Thanks,
Guenter

--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/4 v2] hwmon: ntc_thermistor: Use the manufacturer name properly

[Guenter Roeck]

On 06/24/2014 11:29 PM, Naveen Krishna Chatradhi wrote:

Murata Manufacturing Co., Ltd is the vendor for
NTC (Negative Temperature coefficient) based Thermistors.
But, the driver extensively uses NTC as the vendor name.

This patch corrects the vendor name also updates the
compatibility strings according to the vendor-prefix.txt

Note: Drivers continue to support the previous compatible strings
but further addition of these compatible strings in device tree
is deprecated.

Signed-off-by: Naveen Krishna Chatradhi ch.nav...@samsung.com
Cc: Guenter Roeck li...@roeck-us.net


Applied.

Guenter


--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 0/4 v2] hwmon: ntc_thermistor: prepose vendor prefix change

[Guenter Roeck]

On 06/25/2014 03:57 AM, Kukjin Kim wrote:

Naveen Krishna Chatradhi wrote:



+ Jean Delvare, Guenter Roeck

I'm adding maintainers for drivers/hwmon/ntc* but I'm not sure.

Hi,

This series looks good to me. I will take 3/4 and 4/4 for exynos DT changes once
hwmon/ntc maintainer pick the others.



I applied both patches. Currently going through my test cycle.
I'll send them to Linus Thursday or Friday.

Guenter

--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 00/22] Random ARM randconfig fixes in drivers

[Guenter Roeck]

On Thu, May 08, 2014 at 04:46:51PM +0200, Arnd Bergmann wrote:
 These are a bunch of fixes I had to do to get all randconfig
 configurations on ARM working. Most of these are really old
 bugs, but there are also some new ones. I don't think any of
 them require a backport to linux-stable.
 
 I have checked that they are all still required on yesterday's
 linux-next kernel. Please apply on the appropriate trees unless
 there are objections.
 
Is this series of patches also going to fix arm:allmodconfig ?

Thanks,
Guenter
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] watchdog: s3c2410_wdt: Remove unneeded initialization

[Guenter Roeck]

On 03/04/2014 01:34 AM, Sachin Kamat wrote:

Initializing clk to NULL as a reset/error condition does not
help as NULL is not an invalid condition w.r.t clk. Remove this
initialization altogether as there is no state retention.

Signed-off-by: Sachin Kamat sachin.ka...@linaro.org


Reviewed-by: Guenter Roeck li...@roeck-us.net


--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH V12 1/3] ARM: dts: Add pmu sysreg node to exynos5250 and exynos5420 dtsi files

[Guenter Roeck]

On Fri, Dec 06, 2013 at 11:17:46AM +0530, Leela Krishna Amudala wrote:
 This patch adds pmusysreg node to exynos5250 and exynos5420 dtsi files to
 handle PMU register accesses in a centralized way using syscon driver
 
 Signed-off-by: Leela Krishna Amudala l.kris...@samsung.com
 Reviewed-by: Tomasz Figa t.f...@samsung.com
 Reviewed-by: Doug Anderson diand...@chromium.org
 Tested-by: Doug Anderson diand...@chromium.org

Acked-by: Guenter Roeck li...@roeck-us.net
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH V12 2/3] watchdog: s3c2410_wdt: use syscon regmap interface to configure pmu register

[Guenter Roeck]

On Fri, Dec 06, 2013 at 11:17:47AM +0530, Leela Krishna Amudala wrote:
 Add device tree support for exynos5250 and 5420 SoCs and use syscon regmap 
 interface
 to configure AUTOMATIC_WDT_RESET_DISABLE and MASK_WDT_RESET_REQUEST registers 
 of PMU
 to mask/unmask enable/disable of watchdog in probe and s2r scenarios.
 
 Signed-off-by: Leela Krishna Amudala l.kris...@samsung.com
 Signed-off-by: Doug Anderson diand...@chromium.org

Reviewed-by: Guenter Roeck li...@roeck-us.net
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH V12 3/3] ARM: dts: update watchdog device nodes for Exynos5250 and Exynos5420

[Guenter Roeck]

On Fri, Dec 06, 2013 at 11:17:48AM +0530, Leela Krishna Amudala wrote:
 In Exynos5 series SoCs, PMU has registers to enable/disable mask/unmask
 watchdog timer which is not the case with s3c series SoCs so, there is a
 need to have different compatible names for watchdog to handle these pmu
 registers access.
 
 Hence this patch removes watchdog node from Exynos5.dtsi common file and
 make it separate by updating existing node in Exynos5250 and adding new node
 to Exynos5420. This patch also makes the watchdog node enabled by default
 
 Signed-off-by: Leela Krishna Amudala l.kris...@samsung.com
 Reviewed-by: Tomasz Figa t.f...@samsung.com
 Reviewed-by: Doug Anderson diand...@chromium.org
 Tested-by: Doug Anderson diand...@chromium.org

Acked-by: Guenter Roeck li...@roeck-us.net
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 2/2] watchdog: s3c2410_wdt: Report when the watchdog reset the system

[Guenter Roeck]

On Thu, Dec 05, 2013 at 10:15:29AM -0800, Doug Anderson wrote:
 A good watchdog driver is supposed to report when it was responsible
 for resetting the system.  Implement this for the s3c2410, at least on
 exynos5250 and exynos5420 where we already have a pointer to the PMU
 registers to read the information.
 
 Note that exynos4 SoCs also provide the reset status, but providing
 that is left as an exercise for future changes and is not plumbed up
 in this patch series.  Also note the exynos4 SoCs don't appear to need
 any PMU config, which is why this patch separates the concepts of
 having PMU Registers vs. needing PMU Config.
 
 Signed-off-by: Doug Anderson diand...@chromium.org
 ---
 Changes in v2:
 - Explained QUIRK organization in patch descritpion.
 - Reduced indentation as per Olof and Tomasz suggestion.
 - Now atop proposed v12 of Leela Krishna's patches.

Hi Doug,

The patch doesn't apply on top of v12, unfortunately.

 - NEEDS = HAS, EXYNOS5 prefix
 
  drivers/watchdog/s3c2410_wdt.c | 42 
 +++---
  1 file changed, 39 insertions(+), 3 deletions(-)
 
 diff --git a/drivers/watchdog/s3c2410_wdt.c b/drivers/watchdog/s3c2410_wdt.c
 index 6a00299..45a9503 100644
 --- a/drivers/watchdog/s3c2410_wdt.c
 +++ b/drivers/watchdog/s3c2410_wdt.c
 @@ -62,9 +62,15 @@
  #define CONFIG_S3C2410_WATCHDOG_ATBOOT   (0)
  #define CONFIG_S3C2410_WATCHDOG_DEFAULT_TIME (15)
  
 +#define EXYNOS5_RST_STAT_REG_OFFSET  0x0404
  #define EXYNOS5_WDT_DISABLE_REG_OFFSET   0x0408
  #define EXYNOS5_WDT_MASK_RESET_REG_OFFSET0x040c
  #define QUIRK_HAS_PMU_CONFIG (1  0)
 +#define QUIRK_HAS_RST_STAT   (1  1)
 +
 +/* These quirks require that we have a PMU register map */
 +#define QUIRKS_HAVE_PMUREG   (QUIRK_HAS_PMU_CONFIG | \
 +  QUIRK_HAS_RST_STAT)
  
  static bool nowayout = WATCHDOG_NOWAYOUT;
  static int tmr_margin;
 @@ -98,6 +104,9 @@ MODULE_PARM_DESC(debug, Watchdog debug, set to 1 for 
 debug (default 0));
   * timer reset functionality.
   * @mask_bit: Bit number for the watchdog timer in the disable register and 
 the
   * mask reset register.
 + * @rst_stat_reg: Offset in pmureg for the register that has the reset 
 status.
 + * @rst_stat_bit: Bit number in the rst_stat register indicating a watchdog
 + * reset.
   * @quirks: A bitfield of quirks.
   */
  
 @@ -105,6 +114,8 @@ struct s3c2410_wdt_variant {
   int disable_reg;
   int mask_reset_reg;
   int mask_bit;
 + int rst_stat_reg;
 + int rst_stat_bit;
   u32 quirks;
  };
  
 @@ -131,14 +142,20 @@ static const struct s3c2410_wdt_variant 
 drv_data_exynos5250  = {
   .disable_reg = EXYNOS5_WDT_DISABLE_REG_OFFSET,
   .mask_reset_reg = EXYNOS5_WDT_MASK_RESET_REG_OFFSET,
   .mask_bit = 20,
 - .quirks = QUIRK_HAS_PMU_CONFIG
 + .rst_stat_reg = EXYNOS5_RST_STAT_REG_OFFSET,
 + .rst_stat_bit = 20,
 + .quirks = QUIRK_HAS_PMU_CONFIG |
 + QUIRK_HAS_RST_STAT,

Any reason not to use QUIRKS_HAVE_PMUREG ?

  };
  
  static const struct s3c2410_wdt_variant drv_data_exynos5420 = {
   .disable_reg = EXYNOS5_WDT_DISABLE_REG_OFFSET,
   .mask_reset_reg = EXYNOS5_WDT_MASK_RESET_REG_OFFSET,
   .mask_bit = 0,
 - .quirks = QUIRK_HAS_PMU_CONFIG
 + .rst_stat_reg = EXYNOS5_RST_STAT_REG_OFFSET,
 + .rst_stat_bit = 9,
 + .quirks = QUIRK_HAS_PMU_CONFIG |
 + QUIRK_HAS_RST_STAT,

Same here. Even if you don't use QUIRKS_HAVE_PMUREG, the continuation line is 
not needed.

Thanks,
Guenter
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3] watchdog: s3c2410_wdt: Report when the watchdog reset the system

[Guenter Roeck]

On Fri, Dec 06, 2013 at 01:08:07PM -0800, Doug Anderson wrote:
 A good watchdog driver is supposed to report when it was responsible
 for resetting the system.  Implement this for the s3c2410, at least on
 exynos5250 and exynos5420 where we already have a pointer to the PMU
 registers to read the information.
 
 Note that exynos4 SoCs also provide the reset status, but providing
 that is left as an exercise for future changes and is not plumbed up
 in this patch series.  Also note the exynos4 SoCs don't appear to need
 any PMU config, which is why this patch separates the concepts of
 having PMU Registers vs. needing PMU Config.
 
 Signed-off-by: Doug Anderson diand...@chromium.org

Reviewed-by: Guenter Roeck li...@roeck-us.net
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH V12 1/3] ARM: dts: Add pmu sysreg node to exynos5250 and exynos5420 dtsi files

[Guenter Roeck]

On Fri, Dec 06, 2013 at 03:01:23PM -0800, Doug Anderson wrote:
 Guenter,
 
 On Fri, Dec 6, 2013 at 11:47 AM, Guenter Roeck li...@roeck-us.net wrote:
  On Fri, Dec 06, 2013 at 11:17:46AM +0530, Leela Krishna Amudala wrote:
  This patch adds pmusysreg node to exynos5250 and exynos5420 dtsi files to
  handle PMU register accesses in a centralized way using syscon driver
 
  Signed-off-by: Leela Krishna Amudala l.kris...@samsung.com
  Reviewed-by: Tomasz Figa t.f...@samsung.com
  Reviewed-by: Doug Anderson diand...@chromium.org
  Tested-by: Doug Anderson diand...@chromium.org
 
  Acked-by: Guenter Roeck li...@roeck-us.net
 
 I'm curious of your opinion of Sylwester's requests, since your Ack
 came in after his requests.  Would you like to see a respin of this,
 or do you think it's gone through enough spinning and are thinking it
 would go in as-is?
 

My Ack means I am ok with this patch without further changes.
My reaction to the new comments was along the line of sigh.
Whoever comments now had more than 10 chances to comment earlier,
which should have been way enough, and thus deserves to be ignored.
Sorry if that statement happens to offend any listener, but I am not
really known for my patience.

Guenter
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] watchdog: s3c2410_wdt: Report when the watchdog reset the system

[Guenter Roeck]

On Thu, Dec 05, 2013 at 01:27:13PM +0530, Leela Krishna Amudala wrote:
 Hi Guenter Roeck,
 
 On Tue, Dec 3, 2013 at 3:06 AM, Guenter Roeck li...@roeck-us.net wrote:
 
  On Mon, Dec 02, 2013 at 12:47:53PM -0800, Olof Johansson wrote:
   On Mon, Dec 2, 2013 at 12:21 PM, Guenter Roeck li...@roeck-us.net wrote:
On Mon, Dec 02, 2013 at 10:14:41AM -0800, Doug Anderson wrote:
A good watchdog driver is supposed to report when it was responsible
for resetting the system.  Implement this for the s3c2410, at least on
exynos5250 and exynos5420 where we already have a pointer to the PMU
registers to read the information.
   
Signed-off-by: Doug Anderson diand...@chromium.org
---
This patch is based atop Leela Krishna's recent series that ends with
(ARM: dts: update watchdog device nodes for Exynos5250 and Exynos5420)
AKA https://patchwork.kernel.org/patch/3251861/.
   
 drivers/watchdog/s3c2410_wdt.c | 42 
+++---
 1 file changed, 39 insertions(+), 3 deletions(-)
   
diff --git a/drivers/watchdog/s3c2410_wdt.c 
b/drivers/watchdog/s3c2410_wdt.c
index 47f4dcf..2c87d37 100644
--- a/drivers/watchdog/s3c2410_wdt.c
+++ b/drivers/watchdog/s3c2410_wdt.c
@@ -62,9 +62,13 @@
 #define CONFIG_S3C2410_WATCHDOG_ATBOOT   (0)
 #define CONFIG_S3C2410_WATCHDOG_DEFAULT_TIME (15)
   
+#define RST_STAT_REG_OFFSET  0x0404
 #define WDT_DISABLE_REG_OFFSET   0x0408
 #define WDT_MASK_RESET_REG_OFFSET0x040c
 #define QUIRK_NEEDS_PMU_CONFIG   (1  0)
+#define QUIRK_HAS_RST_STAT   (1  1)
+#define QUIRKS_NEED_PMUREG   (QUIRK_NEEDS_PMU_CONFIG | \
+  QUIRK_HAS_RST_STAT)
   
I am not really happy about the NEED (both of them, really) here.
How about HAS instead ?
  
   Ah, I just commented on these things on our internal review site too
   on this patch. I don't even think a quirk is needed -- just use the
   presence of a non-0 rst_stat_reg to tell if you need to use regmap.
  
  Agreed; same is true for the QUIRK_NEEDS_PMU_CONFIG related registers
  as well.
 
 
 As Tomasz Figa suggested I introduced quirks,
 
'quirk' implies a workaround for non-standard or broken hardware,
not a flag indicating device specific functionality.

Guenter
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] watchdog: s3c2410_wdt: Report when the watchdog reset the system

[Guenter Roeck]

On Mon, Dec 02, 2013 at 10:14:41AM -0800, Doug Anderson wrote:
 A good watchdog driver is supposed to report when it was responsible
 for resetting the system.  Implement this for the s3c2410, at least on
 exynos5250 and exynos5420 where we already have a pointer to the PMU
 registers to read the information.
 
 Signed-off-by: Doug Anderson diand...@chromium.org
 ---
 This patch is based atop Leela Krishna's recent series that ends with
 (ARM: dts: update watchdog device nodes for Exynos5250 and Exynos5420)
 AKA https://patchwork.kernel.org/patch/3251861/.
 
  drivers/watchdog/s3c2410_wdt.c | 42 
 +++---
  1 file changed, 39 insertions(+), 3 deletions(-)
 
 diff --git a/drivers/watchdog/s3c2410_wdt.c b/drivers/watchdog/s3c2410_wdt.c
 index 47f4dcf..2c87d37 100644
 --- a/drivers/watchdog/s3c2410_wdt.c
 +++ b/drivers/watchdog/s3c2410_wdt.c
 @@ -62,9 +62,13 @@
  #define CONFIG_S3C2410_WATCHDOG_ATBOOT   (0)
  #define CONFIG_S3C2410_WATCHDOG_DEFAULT_TIME (15)
  
 +#define RST_STAT_REG_OFFSET  0x0404
  #define WDT_DISABLE_REG_OFFSET   0x0408
  #define WDT_MASK_RESET_REG_OFFSET0x040c
  #define QUIRK_NEEDS_PMU_CONFIG   (1  0)
 +#define QUIRK_HAS_RST_STAT   (1  1)
 +#define QUIRKS_NEED_PMUREG   (QUIRK_NEEDS_PMU_CONFIG | \
 +  QUIRK_HAS_RST_STAT)
  
I am not really happy about the NEED (both of them, really) here.
How about HAS instead ?

  static bool nowayout = WATCHDOG_NOWAYOUT;
  static int tmr_margin;
 @@ -98,6 +102,9 @@ MODULE_PARM_DESC(debug, Watchdog debug, set to 1 for 
 debug (default 0));
   * timer reset functionality.
   * @mask_bit: Bit number for the watchdog timer in the disable register and 
 the
   * mask reset register.
 + * @rst_stat_reg: Offset in pmureg for the register that has the reset 
 status.
 + * @rst_stat_bit: Bit number in the rst_stat register indicating a watchdog
 + * reset.
   * @quirks: A bitfield of quirks.
   */
  
 @@ -105,6 +112,8 @@ struct s3c2410_wdt_variant {
   int disable_reg;
   int mask_reset_reg;
   int mask_bit;
 + int rst_stat_reg;
 + int rst_stat_bit;
   u32 quirks;
  };
  
 @@ -131,14 +140,20 @@ static const struct s3c2410_wdt_variant 
 drv_data_exynos5250  = {
   .disable_reg = WDT_DISABLE_REG_OFFSET,
   .mask_reset_reg = WDT_MASK_RESET_REG_OFFSET,
   .mask_bit = 20,
 - .quirks = QUIRK_NEEDS_PMU_CONFIG
 + .rst_stat_reg = RST_STAT_REG_OFFSET,
 + .rst_stat_bit = 20,
 + .quirks = QUIRK_NEEDS_PMU_CONFIG |
 + QUIRK_HAS_RST_STAT,

Why not use QUIRKS_NEED_PMUREG ?

Also, is there ever a chance that the two bits don't come together ?
If not a single bit might be sufficient.

Thanks,
Guenter

  };
  
  static const struct s3c2410_wdt_variant drv_data_exynos5420 = {
   .disable_reg = WDT_DISABLE_REG_OFFSET,
   .mask_reset_reg = WDT_MASK_RESET_REG_OFFSET,
   .mask_bit = 0,
 - .quirks = QUIRK_NEEDS_PMU_CONFIG
 + .rst_stat_reg = RST_STAT_REG_OFFSET,
 + .rst_stat_bit = 9,
 + .quirks = QUIRK_NEEDS_PMU_CONFIG |
 + QUIRK_HAS_RST_STAT,
  };
  
  static const struct of_device_id s3c2410_wdt_match[] = {
 @@ -423,6 +438,25 @@ static inline void s3c2410wdt_cpufreq_deregister(struct 
 s3c2410_wdt *wdt)
  }
  #endif
  
 +static inline unsigned int s3c2410wdt_get_bootstatus(struct s3c2410_wdt *wdt)
 +{
 + unsigned int bootstatus = 0;
 + int ret;
 +
 + if (wdt-drv_data-quirks  QUIRK_HAS_RST_STAT) {
 + unsigned int rst_stat;
 +
 + ret = regmap_read(wdt-pmureg, wdt-drv_data-rst_stat_reg,
 +   rst_stat);
 + if (ret)
 + dev_warn(wdt-dev, Couldn't get RST_STAT register\n);
 + else if (rst_stat  BIT(wdt-drv_data-rst_stat_bit))
 + bootstatus |= WDIOF_CARDRESET;
 + }
 +
 + return bootstatus;
 +}
 +
  /* s3c2410_get_wdt_driver_data */
  static inline struct s3c2410_wdt_variant *
  get_wdt_drv_data(struct platform_device *pdev)
 @@ -460,7 +494,7 @@ static int s3c2410wdt_probe(struct platform_device *pdev)
   wdt-wdt_device = s3c2410_wdd;
  
   wdt-drv_data = get_wdt_drv_data(pdev);
 - if (wdt-drv_data-quirks  QUIRK_NEEDS_PMU_CONFIG) {
 + if (wdt-drv_data-quirks  QUIRKS_NEED_PMUREG) {
   wdt-pmureg = syscon_regmap_lookup_by_phandle(dev-of_node,
   
 samsung,syscon-phandle);
   if (IS_ERR(wdt-pmureg)) {
 @@ -531,6 +565,8 @@ static int s3c2410wdt_probe(struct platform_device *pdev)
  
   watchdog_set_nowayout(wdt-wdt_device, nowayout);
  
 + wdt-wdt_device.bootstatus = s3c2410wdt_get_bootstatus(wdt);
 +
   ret = watchdog_register_device(wdt-wdt_device);
   if (ret) {
   dev_err(dev, cannot register watchdog (%d)\n, ret);
 -- 
 1.8.4.1
 
 
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc 

Re: [PATCH] watchdog: s3c2410_wdt: Report when the watchdog reset the system

[Guenter Roeck]

On Mon, Dec 02, 2013 at 12:47:53PM -0800, Olof Johansson wrote:
 On Mon, Dec 2, 2013 at 12:21 PM, Guenter Roeck li...@roeck-us.net wrote:
  On Mon, Dec 02, 2013 at 10:14:41AM -0800, Doug Anderson wrote:
  A good watchdog driver is supposed to report when it was responsible
  for resetting the system.  Implement this for the s3c2410, at least on
  exynos5250 and exynos5420 where we already have a pointer to the PMU
  registers to read the information.
 
  Signed-off-by: Doug Anderson diand...@chromium.org
  ---
  This patch is based atop Leela Krishna's recent series that ends with
  (ARM: dts: update watchdog device nodes for Exynos5250 and Exynos5420)
  AKA https://patchwork.kernel.org/patch/3251861/.
 
   drivers/watchdog/s3c2410_wdt.c | 42 
  +++---
   1 file changed, 39 insertions(+), 3 deletions(-)
 
  diff --git a/drivers/watchdog/s3c2410_wdt.c 
  b/drivers/watchdog/s3c2410_wdt.c
  index 47f4dcf..2c87d37 100644
  --- a/drivers/watchdog/s3c2410_wdt.c
  +++ b/drivers/watchdog/s3c2410_wdt.c
  @@ -62,9 +62,13 @@
   #define CONFIG_S3C2410_WATCHDOG_ATBOOT   (0)
   #define CONFIG_S3C2410_WATCHDOG_DEFAULT_TIME (15)
 
  +#define RST_STAT_REG_OFFSET  0x0404
   #define WDT_DISABLE_REG_OFFSET   0x0408
   #define WDT_MASK_RESET_REG_OFFSET0x040c
   #define QUIRK_NEEDS_PMU_CONFIG   (1  0)
  +#define QUIRK_HAS_RST_STAT   (1  1)
  +#define QUIRKS_NEED_PMUREG   (QUIRK_NEEDS_PMU_CONFIG | \
  +  QUIRK_HAS_RST_STAT)
 
  I am not really happy about the NEED (both of them, really) here.
  How about HAS instead ?
 
 Ah, I just commented on these things on our internal review site too
 on this patch. I don't even think a quirk is needed -- just use the
 presence of a non-0 rst_stat_reg to tell if you need to use regmap.
 
Agreed; same is true for the QUIRK_NEEDS_PMU_CONFIG related registers
as well.

Guenter
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH V10 2/3] watchdog: s3c2410_wdt: use syscon regmap interface to configure pmu register

[Guenter Roeck]

On 11/27/2013 03:50 AM, Leela Krishna Amudala wrote:

Add device tree support for exynos5250 and 5420 SoCs and use syscon regmap 
interface
to configure AUTOMATIC_WDT_RESET_DISABLE and MASK_WDT_RESET_REQUEST registers 
of PMU
to mask/unmask enable/disable of watchdog in probe and s2r scenarios.

Signed-off-by: Leela Krishna Amudala l.kris...@samsung.com
---
  .../devicetree/bindings/watchdog/samsung-wdt.txt   |   21 ++-
  drivers/watchdog/Kconfig   |1 +
  drivers/watchdog/s3c2410_wdt.c |  164 ++--
  3 files changed, 176 insertions(+), 10 deletions(-)

diff --git a/Documentation/devicetree/bindings/watchdog/samsung-wdt.txt 
b/Documentation/devicetree/bindings/watchdog/samsung-wdt.txt
index 2aa486c..cfff375 100644
--- a/Documentation/devicetree/bindings/watchdog/samsung-wdt.txt
+++ b/Documentation/devicetree/bindings/watchdog/samsung-wdt.txt
@@ -5,10 +5,29 @@ after a preset amount of time during which the WDT reset 
event has not
  occurred.

  Required properties:
-- compatible : should be samsung,s3c2410-wdt
+- compatible : should be one among the following
+   (a) samsung,s3c2410-wdt for Exynos4 and previous SoCs
+   (b) samsung,exynos5250-wdt for Exynos5250
+   (c) samsung,exynos5420-wdt for Exynos5420
+
  - reg : base physical address of the controller and length of memory mapped
region.
  - interrupts : interrupt number to the cpu.
+- samsung,syscon-phandle : reference to syscon node (This property required 
only
+   in case of compatible being samsung,exynos5250-wdt or 
samsung,exynos5420-wdt.
+   In case of Exynos5250 and 5420 this property points to syscon node 
holding the PMU
+   base address)

  Optional properties:
  - timeout-sec : contains the watchdog timeout in seconds.
+
+Example:
+
+watchdog@101D {
+   compatible = samsung,exynos5250-wdt;
+   reg = 0x101D 0x100;
+   interrupts = 0 42 0;
+   clocks = clock 336;
+   clock-names = watchdog;
+   samsung,syscon-phandle = pmu_syscon;
+};
diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
index 5be6e91..24738c0 100644
--- a/drivers/watchdog/Kconfig
+++ b/drivers/watchdog/Kconfig
@@ -188,6 +188,7 @@ config S3C2410_WATCHDOG
tristate S3C2410 Watchdog
depends on HAVE_S3C2410_WATCHDOG
select WATCHDOG_CORE
+   select MFD_SYSCON if ARCH_EXYNOS5
help
  Watchdog timer block in the Samsung SoCs. This will reboot
  the system when the timer expires with the watchdog enabled.
diff --git a/drivers/watchdog/s3c2410_wdt.c b/drivers/watchdog/s3c2410_wdt.c
index 7d8fd04..b00b535 100644
--- a/drivers/watchdog/s3c2410_wdt.c
+++ b/drivers/watchdog/s3c2410_wdt.c
@@ -40,6 +40,8 @@
  #include linux/slab.h
  #include linux/err.h
  #include linux/of.h
+#include linux/mfd/syscon.h
+#include linux/regmap.h

  #define S3C2410_WTCON 0x00
  #define S3C2410_WTDAT 0x04
@@ -60,6 +62,10 @@
  #define CONFIG_S3C2410_WATCHDOG_ATBOOT(0)
  #define CONFIG_S3C2410_WATCHDOG_DEFAULT_TIME  (15)

+#define WDT_DISABLE_REG_OFFSET 0x0408
+#define WDT_MASK_RESET_REG_OFFSET  0x040c
+#define QUIRK_NEEDS_PMU_CONFIG (1  0)
+
  static bool nowayout  = WATCHDOG_NOWAYOUT;
  static int tmr_margin;
  static int tmr_atboot = CONFIG_S3C2410_WATCHDOG_ATBOOT;
@@ -83,6 +89,25 @@ MODULE_PARM_DESC(soft_noboot, Watchdog action, set to 1 to 
ignore reboots, 
0 to reboot (default 0));
  MODULE_PARM_DESC(debug, Watchdog debug, set to 1 for debug (default 0));

+/**
+ * struct s3c2410_wdt_variant - Per-variant config data
+ *
+ * @disable_reg: Offset in pmureg for the register that disables the watchdog
+ * timer reset functionality.
+ * @mask_reset_reg: Offset in pmureg for the register that masks the watchdog
+ * timer reset functionality.
+ * @mask_bit: Bit number for the watchdog timer in the disable register and the
+ * mask reset register.
+ * @quirks: A bitfield of quirks.
+ */
+
+struct s3c2410_wdt_variant {
+   int disable_reg;
+   int mask_reset_reg;
+   int mask_bit;
+   u32 quirks;
+};
+
  struct s3c2410_wdt {
struct device   *dev;
struct clk  *clock;
@@ -93,8 +118,50 @@ struct s3c2410_wdt {
unsigned long   wtdat_save;
struct watchdog_device  wdt_device;
struct notifier_block   freq_transition;
+   struct s3c2410_wdt_variant *drv_data;
+   struct regmap *pmureg;
+};
+
+static const struct s3c2410_wdt_variant drv_data_s3c2410 = {
+   .quirks = 0
+};
+
+#ifdef CONFIG_OF
+static const struct s3c2410_wdt_variant drv_data_exynos5250  = {
+   .disable_reg = WDT_DISABLE_REG_OFFSET,
+   .mask_reset_reg = WDT_MASK_RESET_REG_OFFSET,
+   .mask_bit = 20,
+   .quirks = QUIRK_NEEDS_PMU_CONFIG
+};
+
+static const struct s3c2410_wdt_variant drv_data_exynos5420 = {
+   .disable_reg = WDT_DISABLE_REG_OFFSET,
+   .mask_reset_reg = 

Re: [PATCH v2] watchdog: s3c2410_wdt: Only register for cpufreq on ARM_S3C24XX_CPUFREQ

[Guenter Roeck]

On 11/25/2013 03:36 PM, Doug Anderson wrote:

On modern SoCs the watchdog timer is parented on a clock that doesn't
change every time we have a cpufreq change.  That means we don't need
to constantly adjust the watchdog timer, so avoid registering for and
dealing with cpufreq transitions unless we've actually got
CONFIG_ARM_S3C24XX_CPUFREQ defined.

Note that this is more than just an optimization.  The s3c2410
watchdog driver actually pats the watchdog on every CPU frequency
change.  On modern systems these happen many times per second (even in
a system where nothing is happening).  That effectively makes any
userspace watchdog program useless (the watchdog is constantly patted
by the kernel).  If we need ARM_S3C24XX_CPUFREQ defined on a
multiplatform kernel we'll need to make sure that kernel supports
common clock and change this to user common clock framework.

Signed-off-by: Doug Anderson diand...@chromium.org


Reviewed-by: Guenter Roeck li...@roeck-us.net

--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] watchdog: s3c2410_wdt: Handle rounding a little better for timeout

[Guenter Roeck]

On 11/26/2013 10:30 AM, Doug Anderson wrote:

The existing watchdog timeout worked OK but didn't deal with
rounding in an ideal way when dividing out all of its clocks.

Specifically if you had a timeout of 32 seconds and an input clock of
, you'd end up setting a timeout of 31.9998 seconds and
reporting a timeout of 31 seconds.

Specifically DBG printouts showed:
   s3c2410wdt_set_heartbeat: count=1656, timeout=32, freq=520833
   s3c2410wdt_set_heartbeat: timeout=32, divisor=255, count=1656 (ff4f)
and the final timeout reported to the user was:
   ((count / divisor) * divisor) / freq
   (0xff4f * 255) / 520833 = 31 (truncated from 31.9998)
the technically correct value is:
   (0xff4f * 255) / (.0 / 128) = 31.9998

By using DIV_ROUND_UP we can be a little more correct.
   s3c2410wdt_set_heartbeat: count=1688, timeout=32, freq=520834
   s3c2410wdt_set_heartbeat: timeout=32, divisor=255, count=1688 (ff50)
and the final timeout reported to the user:
   (0xff50 * 255) / 520834 = 32
the technically correct value is:
   (0xff50 * 255) / (.0 / 128) = 32.0003

We'll use a DIV_ROUND_UP to solve this, generally erroring on the side
of reporting shorter values to the user and setting the watchdog to
slightly longer than requested:
* Round input frequency up to assume watchdog is counting faster.
* Round divisions by divisor up to give us extra time.

Signed-off-by: Doug Anderson diand...@chromium.org
---
  drivers/watchdog/s3c2410_wdt.c | 10 +-
  1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/watchdog/s3c2410_wdt.c b/drivers/watchdog/s3c2410_wdt.c
index 7d8fd04..fe2322b 100644
--- a/drivers/watchdog/s3c2410_wdt.c
+++ b/drivers/watchdog/s3c2410_wdt.c
@@ -188,7 +188,7 @@ static int s3c2410wdt_set_heartbeat(struct watchdog_device 
*wdd, unsigned timeou
if (timeout  1)
return -EINVAL;

-   freq /= 128;
+   freq = DIV_ROUND_UP(freq, 128);
count = timeout * freq;

DBG(%s: count=%d, timeout=%d, freq=%lu\n,
@@ -201,20 +201,20 @@ static int s3c2410wdt_set_heartbeat(struct 
watchdog_device *wdd, unsigned timeou

if (count = 0x1) {
for (divisor = 1; divisor = 0x100; divisor++) {
-   if ((count / divisor)  0x1)
+   if (DIV_ROUND_UP(count, divisor)  0x1)
break;
}


Since you are at it,
divisor = DIV_ROUND_UP(count + 1, 0x1);
might be faster, simpler, and easier to understand than the loop.

Otherwise looks good to me.

Guenter


-   if ((count / divisor) = 0x1) {
+   if (divisor  0x100) {
dev_err(wdt-dev, timeout %d too big\n, timeout);
return -EINVAL;
}
}

DBG(%s: timeout=%d, divisor=%d, count=%d (%08x)\n,
-   __func__, timeout, divisor, count, count/divisor);
+   __func__, timeout, divisor, count, DIV_ROUND_UP(count, divisor));

-   count /= divisor;
+   count = DIV_ROUND_UP(count, divisor);
wdt-count = count;

/* update the pre-scaler */



--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] watchdog: s3c2410_wdt: Handle rounding a little better for timeout

[Guenter Roeck]

On 11/26/2013 01:34 PM, Doug Anderson wrote:

Guenter,

On Tue, Nov 26, 2013 at 10:48 AM, Guenter Roeck li...@roeck-us.net wrote:

On 11/26/2013 10:30 AM, Doug Anderson wrote:


The existing watchdog timeout worked OK but didn't deal with
rounding in an ideal way when dividing out all of its clocks.

Specifically if you had a timeout of 32 seconds and an input clock of
, you'd end up setting a timeout of 31.9998 seconds and
reporting a timeout of 31 seconds.

Specifically DBG printouts showed:
s3c2410wdt_set_heartbeat: count=1656, timeout=32, freq=520833
s3c2410wdt_set_heartbeat: timeout=32, divisor=255, count=1656
(ff4f)
and the final timeout reported to the user was:
((count / divisor) * divisor) / freq
(0xff4f * 255) / 520833 = 31 (truncated from 31.9998)
the technically correct value is:
(0xff4f * 255) / (.0 / 128) = 31.9998

By using DIV_ROUND_UP we can be a little more correct.
s3c2410wdt_set_heartbeat: count=1688, timeout=32, freq=520834
s3c2410wdt_set_heartbeat: timeout=32, divisor=255, count=1688
(ff50)
and the final timeout reported to the user:
(0xff50 * 255) / 520834 = 32
the technically correct value is:
(0xff50 * 255) / (.0 / 128) = 32.0003

We'll use a DIV_ROUND_UP to solve this, generally erroring on the side
of reporting shorter values to the user and setting the watchdog to
slightly longer than requested:
* Round input frequency up to assume watchdog is counting faster.
* Round divisions by divisor up to give us extra time.

Signed-off-by: Doug Anderson diand...@chromium.org
---
   drivers/watchdog/s3c2410_wdt.c | 10 +-
   1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/watchdog/s3c2410_wdt.c
b/drivers/watchdog/s3c2410_wdt.c
index 7d8fd04..fe2322b 100644
--- a/drivers/watchdog/s3c2410_wdt.c
+++ b/drivers/watchdog/s3c2410_wdt.c
@@ -188,7 +188,7 @@ static int s3c2410wdt_set_heartbeat(struct
watchdog_device *wdd, unsigned timeou
 if (timeout  1)
 return -EINVAL;

-   freq /= 128;
+   freq = DIV_ROUND_UP(freq, 128);
 count = timeout * freq;

 DBG(%s: count=%d, timeout=%d, freq=%lu\n,
@@ -201,20 +201,20 @@ static int s3c2410wdt_set_heartbeat(struct
watchdog_device *wdd, unsigned timeou

 if (count = 0x1) {
 for (divisor = 1; divisor = 0x100; divisor++) {
-   if ((count / divisor)  0x1)
+   if (DIV_ROUND_UP(count, divisor)  0x1)
 break;
 }


Since you are at it,
 divisor = DIV_ROUND_UP(count + 1, 0x1);
might be faster, simpler, and easier to understand than the loop.


Way to see the forest for the trees!

Your math ends up with a slightly different result than the old code,
though.  One example is when the count is 0x1.  You'll end up with
a divider of 2 and I'll end up with a divider of 3.

I think we just want:

divisor = DIV_ROUND_UP(count, 0x);

...that produces the same result as the old loop, but am curious to
know why you chose the count + 1 and 0x1.



Hi Doug,

I thought the idea was to keep (count / div) less than 0x1, which you get
by dividing through 0x1. 0x1 / 0x1 = 1, though, so I added 1
to the counter. But maybe I was thinking too much ;-).

Now, 0x1 / 2 = 0x is still lower than 0x1, which is what
I thought is the requirement. Ultimately the error is small either way,
so DIV_ROUND_UP(count, 0x) is just as good to me to avoid the loop.

Thanks,
Guenter

--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] watchdog: s3c2410_wdt: Handle rounding a little better for timeout

[Guenter Roeck]

On 11/26/2013 04:57 PM, Doug Anderson wrote:

The existing watchdog timeout worked OK but didn't deal with
rounding in an ideal way when dividing out all of its clocks.

Specifically if you had a timeout of 32 seconds and an input clock of
, you'd end up setting a timeout of 31.9998 seconds and
reporting a timeout of 31 seconds.

Specifically DBG printouts showed:
   s3c2410wdt_set_heartbeat: count=1656, timeout=32, freq=520833
   s3c2410wdt_set_heartbeat: timeout=32, divisor=255, count=1656 (ff4f)
and the final timeout reported to the user was:
   ((count / divisor) * divisor) / freq
   (0xff4f * 255) / 520833 = 31 (truncated from 31.9998)
the technically correct value is:
   (0xff4f * 255) / (.0 / 128) = 31.9998

By using DIV_ROUND_UP we can be a little more correct.
   s3c2410wdt_set_heartbeat: count=1688, timeout=32, freq=520834
   s3c2410wdt_set_heartbeat: timeout=32, divisor=255, count=1688 (ff50)
and the final timeout reported to the user:
   (0xff50 * 255) / 520834 = 32
the technically correct value is:
   (0xff50 * 255) / (.0 / 128) = 32.0003

We'll use a DIV_ROUND_UP to solve this, generally erroring on the side
of reporting shorter values to the user and setting the watchdog to
slightly longer than requested:
* Round input frequency up to assume watchdog is counting faster.
* Round divisions by divisor up to give us extra time.

At the same time we can avoid a for loop by just doing the right math.

Signed-off-by: Doug Anderson diand...@chromium.org


Reviewed-by: Guenter Roeck li...@roeck-us.net


---
Changes in v2:
- Avoid a for loop as per Guenter.

  drivers/watchdog/s3c2410_wdt.c | 13 +
  1 file changed, 5 insertions(+), 8 deletions(-)

diff --git a/drivers/watchdog/s3c2410_wdt.c b/drivers/watchdog/s3c2410_wdt.c
index 7d8fd04..d9bcd6e 100644
--- a/drivers/watchdog/s3c2410_wdt.c
+++ b/drivers/watchdog/s3c2410_wdt.c
@@ -188,7 +188,7 @@ static int s3c2410wdt_set_heartbeat(struct watchdog_device 
*wdd, unsigned timeou
if (timeout  1)
return -EINVAL;

-   freq /= 128;
+   freq = DIV_ROUND_UP(freq, 128);
count = timeout * freq;

DBG(%s: count=%d, timeout=%d, freq=%lu\n,
@@ -200,21 +200,18 @@ static int s3c2410wdt_set_heartbeat(struct 
watchdog_device *wdd, unsigned timeou
*/

if (count = 0x1) {
-   for (divisor = 1; divisor = 0x100; divisor++) {
-   if ((count / divisor)  0x1)
-   break;
-   }
+   divisor = DIV_ROUND_UP(count, 0x);

-   if ((count / divisor) = 0x1) {
+   if (divisor  0x100) {
dev_err(wdt-dev, timeout %d too big\n, timeout);
return -EINVAL;
}
}

DBG(%s: timeout=%d, divisor=%d, count=%d (%08x)\n,
-   __func__, timeout, divisor, count, count/divisor);
+   __func__, timeout, divisor, count, DIV_ROUND_UP(count, divisor));

-   count /= divisor;
+   count = DIV_ROUND_UP(count, divisor);
wdt-count = count;

/* update the pre-scaler */



--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] watchdog: s3c2410_wdt: Only register for cpufreq on CPU_FREQ_S3C24XX

[Guenter Roeck]

On 11/25/2013 02:55 PM, Doug Anderson wrote:

On modern SoCs the watchdog timer is parented on a clock that doesn't
change every time we have a cpufreq change.  That means we don't need
to constantly adjust the watchdog timer, so avoid registering for and
dealing with cpufreq transitions unless we've actually got
CPU_FREQ_S3C24XX defined.

Note that this is more than just an optimization.  The s3c2410
watchdog driver actually pats the watchdog on every CPU frequency
change.  On modern systems these happen many times per second (even in
a system where nothing is happening).  That effectively makes any
userspace watchdog program useless (the watchdog is constantly patted
by the kernel).  If we need CPU_FREQ_S3C24XX defined on a
multiplatform kernel we'll need to make sure that kernel supports
common clock and change this to user common clock framework.

Signed-off-by: Doug Anderson diand...@chromium.org
---
  drivers/watchdog/s3c2410_wdt.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/watchdog/s3c2410_wdt.c b/drivers/watchdog/s3c2410_wdt.c
index 7d8fd04..4980f84 100644
--- a/drivers/watchdog/s3c2410_wdt.c
+++ b/drivers/watchdog/s3c2410_wdt.c
@@ -264,7 +264,7 @@ static irqreturn_t s3c2410wdt_irq(int irqno, void *param)
return IRQ_HANDLED;
  }

-#ifdef CONFIG_CPU_FREQ
+#ifdef CONFIG_CPU_FREQ_S3C24XX


Where is the CPU_FREQ_S3C24XX configuration option defined ? I don't see it
in the current upstream kernel, so it appears that this depends on some
out-of-tree changes.

Thanks,
Guenter

--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH V9 2/3] watchdog: s3c2410_wdt: use syscon regmap interface to configure pmu register

[Guenter Roeck]

On Mon, Nov 18, 2013 at 03:19:48PM +0530, Leela Krishna Amudala wrote:
 Add device tree support for exynos5250 and 5420 SoCs and use syscon regmap 
 interface
 to configure AUTOMATIC_WDT_RESET_DISABLE and MASK_WDT_RESET_REQUEST registers 
 of PMU
 to mask/unmask enable/disable of watchdog in probe and s2r scenarios.
 
 Signed-off-by: Leela Krishna Amudala l.kris...@samsung.com
 ---
  .../devicetree/bindings/watchdog/samsung-wdt.txt   |   21 ++-
  drivers/watchdog/Kconfig   |1 +
  drivers/watchdog/s3c2410_wdt.c |  145 
 ++--
  3 files changed, 157 insertions(+), 10 deletions(-)
 
 diff --git a/Documentation/devicetree/bindings/watchdog/samsung-wdt.txt 
 b/Documentation/devicetree/bindings/watchdog/samsung-wdt.txt
 index 2aa486c..5dea363 100644
 --- a/Documentation/devicetree/bindings/watchdog/samsung-wdt.txt
 +++ b/Documentation/devicetree/bindings/watchdog/samsung-wdt.txt
 @@ -5,10 +5,29 @@ after a preset amount of time during which the WDT reset 
 event has not
  occurred.
  
  Required properties:
 -- compatible : should be samsung,s3c2410-wdt
 +- compatible : should be one among the following
 + (a) samsung,s3c2410-wdt for Exynos4 and previous SoCs
 + (b) samsung,exynos5250-wdt for Exynos5250
 + (c) samsung,exynos5420-wdt for Exynos5420
 +
  - reg : base physical address of the controller and length of memory mapped
   region.
  - interrupts : interrupt number to the cpu.
 +- samsung,syscon-phandle : reference to syscon node (This property required 
 only
 + in case of compatible being samsung,exynos5250-wdt or 
 samsung,exynos5420-wdt.
 + In case of Exynos5250 and 5420 this property points to syscon node 
 holding the PMU
 + base address)
  
  Optional properties:
  - timeout-sec : contains the watchdog timeout in seconds.
 +
 +Example:
 +
 +watchdog@101D {
 + compatible = samsung,exynos5250-wdt;
 + reg = 0x101D 0x100;
 + interrupts = 0 42 0;
 + clocks = clock 336;
 + clock-names = watchdog;
 + samsung,syscon-phandle = pmu_sys_reg;
 +};
 diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
 index d1d53f3..0d272ae 100644
 --- a/drivers/watchdog/Kconfig
 +++ b/drivers/watchdog/Kconfig
 @@ -188,6 +188,7 @@ config S3C2410_WATCHDOG
   tristate S3C2410 Watchdog
   depends on HAVE_S3C2410_WATCHDOG
   select WATCHDOG_CORE
 + select MFD_SYSCON if ARCH_EXYNOS5
   help
 Watchdog timer block in the Samsung SoCs. This will reboot
 the system when the timer expires with the watchdog enabled.
 diff --git a/drivers/watchdog/s3c2410_wdt.c b/drivers/watchdog/s3c2410_wdt.c
 index 23aad7c..42e0fd3 100644
 --- a/drivers/watchdog/s3c2410_wdt.c
 +++ b/drivers/watchdog/s3c2410_wdt.c
 @@ -41,6 +41,8 @@
  #include linux/slab.h
  #include linux/err.h
  #include linux/of.h
 +#include linux/mfd/syscon.h
 +#include linux/regmap.h
  
  #define S3C2410_WTCON0x00
  #define S3C2410_WTDAT0x04
 @@ -61,6 +63,10 @@
  #define CONFIG_S3C2410_WATCHDOG_ATBOOT   (0)
  #define CONFIG_S3C2410_WATCHDOG_DEFAULT_TIME (15)
  
 +#define WDT_DISABLE_REG_OFFSET   0x0408
 +#define WDT_MASK_RESET_REG_OFFSET0x040c
 +#define QUIRK_NEEDS_PMU_CONFIG   (1  0)
 +
  static bool nowayout = WATCHDOG_NOWAYOUT;
  static int tmr_margin;
  static int tmr_atboot= CONFIG_S3C2410_WATCHDOG_ATBOOT;
 @@ -84,6 +90,13 @@ MODULE_PARM_DESC(soft_noboot, Watchdog action, set to 1 
 to ignore reboots, 
   0 to reboot (default 0));
  MODULE_PARM_DESC(debug, Watchdog debug, set to 1 for debug (default 0));
  
 +struct s3c2410_wdt_variant {
 + int disable_reg;
 + int mask_reset_reg;
 + int mask_bit;
 + u32 quirks;
 +};
 +
  struct s3c2410_wdt {
   struct device   *dev;
   struct clk  *clock;
 @@ -94,7 +107,49 @@ struct s3c2410_wdt {
   unsigned long   wtdat_save;
   struct watchdog_device  wdt_device;
   struct notifier_block   freq_transition;
 + struct s3c2410_wdt_variant *drv_data;
 + struct regmap *pmureg;
 +};
 +
 +static const struct s3c2410_wdt_variant drv_data_s3c2410 = {
 + .quirks = 0
 +};
 +
 +#ifdef CONFIG_OF
 +static const struct s3c2410_wdt_variant drv_data_exynos5250  = {
 + .disable_reg = WDT_DISABLE_REG_OFFSET,
 + .mask_reset_reg = WDT_MASK_RESET_REG_OFFSET,
 + .mask_bit = 20,
 + .quirks = QUIRK_NEEDS_PMU_CONFIG
 +};
 +
 +static const struct s3c2410_wdt_variant drv_data_exynos5420 = {
 + .disable_reg = WDT_DISABLE_REG_OFFSET,
 + .mask_reset_reg = WDT_MASK_RESET_REG_OFFSET,
 + .mask_bit = 0,
 + .quirks = QUIRK_NEEDS_PMU_CONFIG
 +};
 +
 +static const struct of_device_id s3c2410_wdt_match[] = {
 + { .compatible = samsung,s3c2410-wdt,
 +   .data = drv_data_s3c2410 },
 + { .compatible = samsung,exynos5250-wdt,
 +   .data = drv_data_exynos5250 },
 + { .compatible = 

Re: [PATCH V9 2/3] watchdog: s3c2410_wdt: use syscon regmap interface to configure pmu register

[Guenter Roeck]

On 11/18/2013 08:36 PM, Leela Krishna Amudala wrote:

Hi Guenter Roeck,

Thanks for reviewing the patch.


On Mon, Nov 18, 2013 at 10:12 PM, Guenter Roeck li...@roeck-us.net wrote:

On Mon, Nov 18, 2013 at 03:19:48PM +0530, Leela Krishna Amudala wrote:

Add device tree support for exynos5250 and 5420 SoCs and use syscon regmap 
interface
to configure AUTOMATIC_WDT_RESET_DISABLE and MASK_WDT_RESET_REQUEST registers 
of PMU
to mask/unmask enable/disable of watchdog in probe and s2r scenarios.

Signed-off-by: Leela Krishna Amudala l.kris...@samsung.com
---
  .../devicetree/bindings/watchdog/samsung-wdt.txt   |   21 ++-
  drivers/watchdog/Kconfig   |1 +
  drivers/watchdog/s3c2410_wdt.c |  145 ++--
  3 files changed, 157 insertions(+), 10 deletions(-)

diff --git a/Documentation/devicetree/bindings/watchdog/samsung-wdt.txt 
b/Documentation/devicetree/bindings/watchdog/samsung-wdt.txt
index 2aa486c..5dea363 100644
--- a/Documentation/devicetree/bindings/watchdog/samsung-wdt.txt
+++ b/Documentation/devicetree/bindings/watchdog/samsung-wdt.txt
@@ -5,10 +5,29 @@ after a preset amount of time during which the WDT reset 
event has not
  occurred.

  Required properties:
-- compatible : should be samsung,s3c2410-wdt
+- compatible : should be one among the following
+ (a) samsung,s3c2410-wdt for Exynos4 and previous SoCs
+ (b) samsung,exynos5250-wdt for Exynos5250
+ (c) samsung,exynos5420-wdt for Exynos5420
+
  - reg : base physical address of the controller and length of memory mapped
   region.
  - interrupts : interrupt number to the cpu.
+- samsung,syscon-phandle : reference to syscon node (This property required 
only
+ in case of compatible being samsung,exynos5250-wdt or 
samsung,exynos5420-wdt.
+ In case of Exynos5250 and 5420 this property points to syscon node 
holding the PMU
+ base address)

  Optional properties:
  - timeout-sec : contains the watchdog timeout in seconds.
+
+Example:
+
+watchdog@101D {
+ compatible = samsung,exynos5250-wdt;
+ reg = 0x101D 0x100;
+ interrupts = 0 42 0;
+ clocks = clock 336;
+ clock-names = watchdog;
+ samsung,syscon-phandle = pmu_sys_reg;
+};
diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
index d1d53f3..0d272ae 100644
--- a/drivers/watchdog/Kconfig
+++ b/drivers/watchdog/Kconfig
@@ -188,6 +188,7 @@ config S3C2410_WATCHDOG
   tristate S3C2410 Watchdog
   depends on HAVE_S3C2410_WATCHDOG
   select WATCHDOG_CORE
+ select MFD_SYSCON if ARCH_EXYNOS5
   help
 Watchdog timer block in the Samsung SoCs. This will reboot
 the system when the timer expires with the watchdog enabled.
diff --git a/drivers/watchdog/s3c2410_wdt.c b/drivers/watchdog/s3c2410_wdt.c
index 23aad7c..42e0fd3 100644
--- a/drivers/watchdog/s3c2410_wdt.c
+++ b/drivers/watchdog/s3c2410_wdt.c
@@ -41,6 +41,8 @@
  #include linux/slab.h
  #include linux/err.h
  #include linux/of.h
+#include linux/mfd/syscon.h
+#include linux/regmap.h

  #define S3C2410_WTCON0x00
  #define S3C2410_WTDAT0x04
@@ -61,6 +63,10 @@
  #define CONFIG_S3C2410_WATCHDOG_ATBOOT   (0)
  #define CONFIG_S3C2410_WATCHDOG_DEFAULT_TIME (15)

+#define WDT_DISABLE_REG_OFFSET   0x0408
+#define WDT_MASK_RESET_REG_OFFSET0x040c
+#define QUIRK_NEEDS_PMU_CONFIG   (1  0)
+
  static bool nowayout = WATCHDOG_NOWAYOUT;
  static int tmr_margin;
  static int tmr_atboot= CONFIG_S3C2410_WATCHDOG_ATBOOT;
@@ -84,6 +90,13 @@ MODULE_PARM_DESC(soft_noboot, Watchdog action, set to 1 to 
ignore reboots, 
   0 to reboot (default 0));
  MODULE_PARM_DESC(debug, Watchdog debug, set to 1 for debug (default 0));

+struct s3c2410_wdt_variant {
+ int disable_reg;
+ int mask_reset_reg;
+ int mask_bit;
+ u32 quirks;
+};
+
  struct s3c2410_wdt {
   struct device   *dev;
   struct clk  *clock;
@@ -94,7 +107,49 @@ struct s3c2410_wdt {
   unsigned long   wtdat_save;
   struct watchdog_device  wdt_device;
   struct notifier_block   freq_transition;
+ struct s3c2410_wdt_variant *drv_data;
+ struct regmap *pmureg;
+};
+
+static const struct s3c2410_wdt_variant drv_data_s3c2410 = {
+ .quirks = 0
+};
+
+#ifdef CONFIG_OF
+static const struct s3c2410_wdt_variant drv_data_exynos5250  = {
+ .disable_reg = WDT_DISABLE_REG_OFFSET,
+ .mask_reset_reg = WDT_MASK_RESET_REG_OFFSET,
+ .mask_bit = 20,
+ .quirks = QUIRK_NEEDS_PMU_CONFIG
+};
+
+static const struct s3c2410_wdt_variant drv_data_exynos5420 = {
+ .disable_reg = WDT_DISABLE_REG_OFFSET,
+ .mask_reset_reg = WDT_MASK_RESET_REG_OFFSET,
+ .mask_bit = 0,
+ .quirks = QUIRK_NEEDS_PMU_CONFIG
+};
+
+static const struct of_device_id s3c2410_wdt_match[] = {
+ { .compatible = samsung,s3c2410-wdt,
+   .data = drv_data_s3c2410 },
+ { .compatible = samsung,exynos5250-wdt

Re: [PATCH V9 2/3] watchdog: s3c2410_wdt: use syscon regmap interface to configure pmu register

[Guenter Roeck]

On 11/18/2013 09:26 PM, Leela Krishna Amudala wrote:

Hi Guenter Roeck,

On Tue, Nov 19, 2013 at 10:30 AM, Guenter Roeck li...@roeck-us.net wrote:

On 11/18/2013 08:36 PM, Leela Krishna Amudala wrote:


Hi Guenter Roeck,

Thanks for reviewing the patch.


On Mon, Nov 18, 2013 at 10:12 PM, Guenter Roeck li...@roeck-us.net
wrote:


On Mon, Nov 18, 2013 at 03:19:48PM +0530, Leela Krishna Amudala wrote:


Add device tree support for exynos5250 and 5420 SoCs and use syscon
regmap interface
to configure AUTOMATIC_WDT_RESET_DISABLE and MASK_WDT_RESET_REQUEST
registers of PMU
to mask/unmask enable/disable of watchdog in probe and s2r scenarios.

Signed-off-by: Leela Krishna Amudala l.kris...@samsung.com
---
   .../devicetree/bindings/watchdog/samsung-wdt.txt   |   21 ++-
   drivers/watchdog/Kconfig   |1 +
   drivers/watchdog/s3c2410_wdt.c |  145
++--
   3 files changed, 157 insertions(+), 10 deletions(-)

diff --git a/Documentation/devicetree/bindings/watchdog/samsung-wdt.txt
b/Documentation/devicetree/bindings/watchdog/samsung-wdt.txt
index 2aa486c..5dea363 100644
--- a/Documentation/devicetree/bindings/watchdog/samsung-wdt.txt
+++ b/Documentation/devicetree/bindings/watchdog/samsung-wdt.txt
@@ -5,10 +5,29 @@ after a preset amount of time during which the WDT
reset event has not
   occurred.

   Required properties:
-- compatible : should be samsung,s3c2410-wdt
+- compatible : should be one among the following
+ (a) samsung,s3c2410-wdt for Exynos4 and previous SoCs
+ (b) samsung,exynos5250-wdt for Exynos5250
+ (c) samsung,exynos5420-wdt for Exynos5420
+
   - reg : base physical address of the controller and length of memory
mapped
region.
   - interrupts : interrupt number to the cpu.
+- samsung,syscon-phandle : reference to syscon node (This property
required only
+ in case of compatible being samsung,exynos5250-wdt or
samsung,exynos5420-wdt.
+ In case of Exynos5250 and 5420 this property points to syscon node
holding the PMU
+ base address)

   Optional properties:
   - timeout-sec : contains the watchdog timeout in seconds.
+
+Example:
+
+watchdog@101D {
+ compatible = samsung,exynos5250-wdt;
+ reg = 0x101D 0x100;
+ interrupts = 0 42 0;
+ clocks = clock 336;
+ clock-names = watchdog;
+ samsung,syscon-phandle = pmu_sys_reg;
+};
diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
index d1d53f3..0d272ae 100644
--- a/drivers/watchdog/Kconfig
+++ b/drivers/watchdog/Kconfig
@@ -188,6 +188,7 @@ config S3C2410_WATCHDOG
tristate S3C2410 Watchdog
depends on HAVE_S3C2410_WATCHDOG
select WATCHDOG_CORE
+ select MFD_SYSCON if ARCH_EXYNOS5
help
  Watchdog timer block in the Samsung SoCs. This will reboot
  the system when the timer expires with the watchdog enabled.
diff --git a/drivers/watchdog/s3c2410_wdt.c
b/drivers/watchdog/s3c2410_wdt.c
index 23aad7c..42e0fd3 100644
--- a/drivers/watchdog/s3c2410_wdt.c
+++ b/drivers/watchdog/s3c2410_wdt.c
@@ -41,6 +41,8 @@
   #include linux/slab.h
   #include linux/err.h
   #include linux/of.h
+#include linux/mfd/syscon.h
+#include linux/regmap.h

   #define S3C2410_WTCON0x00
   #define S3C2410_WTDAT0x04
@@ -61,6 +63,10 @@
   #define CONFIG_S3C2410_WATCHDOG_ATBOOT   (0)
   #define CONFIG_S3C2410_WATCHDOG_DEFAULT_TIME (15)

+#define WDT_DISABLE_REG_OFFSET   0x0408
+#define WDT_MASK_RESET_REG_OFFSET0x040c
+#define QUIRK_NEEDS_PMU_CONFIG   (1  0)
+
   static bool nowayout = WATCHDOG_NOWAYOUT;
   static int tmr_margin;
   static int tmr_atboot= CONFIG_S3C2410_WATCHDOG_ATBOOT;
@@ -84,6 +90,13 @@ MODULE_PARM_DESC(soft_noboot, Watchdog action, set
to 1 to ignore reboots, 
0 to reboot (default 0));
   MODULE_PARM_DESC(debug, Watchdog debug, set to 1 for debug (default
0));

+struct s3c2410_wdt_variant {
+ int disable_reg;
+ int mask_reset_reg;
+ int mask_bit;
+ u32 quirks;
+};
+
   struct s3c2410_wdt {
struct device   *dev;
struct clk  *clock;
@@ -94,7 +107,49 @@ struct s3c2410_wdt {
unsigned long   wtdat_save;
struct watchdog_device  wdt_device;
struct notifier_block   freq_transition;
+ struct s3c2410_wdt_variant *drv_data;
+ struct regmap *pmureg;
+};
+
+static const struct s3c2410_wdt_variant drv_data_s3c2410 = {
+ .quirks = 0
+};
+
+#ifdef CONFIG_OF
+static const struct s3c2410_wdt_variant drv_data_exynos5250  = {
+ .disable_reg = WDT_DISABLE_REG_OFFSET,
+ .mask_reset_reg = WDT_MASK_RESET_REG_OFFSET,
+ .mask_bit = 20,
+ .quirks = QUIRK_NEEDS_PMU_CONFIG
+};
+
+static const struct s3c2410_wdt_variant drv_data_exynos5420 = {
+ .disable_reg = WDT_DISABLE_REG_OFFSET,
+ .mask_reset_reg = WDT_MASK_RESET_REG_OFFSET,
+ .mask_bit = 0,
+ .quirks = QUIRK_NEEDS_PMU_CONFIG

Re: [PATCH V5 3/3] watchdog: s3c2410_wdt: use syscon regmap interface to configure pmu register

[Guenter Roeck]

On 10/31/2013 05:29 AM, Tomasz Figa wrote:

Hi Leela,

On Thursday 31 of October 2013 11:30:50 Leela Krishna Amudala wrote:

The syscon regmap interface is used to configure AUTOMATIC_WDT_RESET_DISABLE
and MASK_WDT_RESET_REQUEST registers of PMU to mask/unmask enable/disable of
watchdog in probe and s2r scenarios.

Signed-off-by: Leela Krishna Amudala l.kris...@samsung.com
---
  .../devicetree/bindings/watchdog/samsung-wdt.txt   |   22 ++-
  drivers/watchdog/s3c2410_wdt.c |  150 +---
  2 files changed, 154 insertions(+), 18 deletions(-)


This patch looks better now, but there are still several issues remaining.
Please see my comments inline.


diff --git a/Documentation/devicetree/bindings/watchdog/samsung-wdt.txt 
b/Documentation/devicetree/bindings/watchdog/samsung-wdt.txt
index 2aa486c..0b8aa4b 100644
--- a/Documentation/devicetree/bindings/watchdog/samsung-wdt.txt
+++ b/Documentation/devicetree/bindings/watchdog/samsung-wdt.txt
@@ -5,10 +5,30 @@ after a preset amount of time during which the WDT reset 
event has not
  occurred.

  Required properties:
-- compatible : should be samsung,s3c2410-wdt
+- compatible : should be one among the following
+   (a) samsung,s3c2410-wdt for Exynos4 and previous SoCs
+   (b) samsung,exynos5250-wdt for Exynos5250
+   (c) samsung,exynos5420-wdt for Exynos5420
+
  - reg : base physical address of the controller and length of memory mapped
region.
  - interrupts : interrupt number to the cpu.
+- samsung,syscon-phandle : reference to syscon node (This property required 
only
+   in case of compatible being samsung,exynos5250-wdt or 
samsung,exynos5420-wdt.
+   In case of Exynos5250 and 5420 this property points to syscon node 
holding the PMU
+   base address)

  Optional properties:
  - timeout-sec : contains the watchdog timeout in seconds.
+
+Example:
+
+watchdog@101D {
+   compatible = samsung,exynos5250-wdt;
+   reg = 0x101D 0x100;
+   interrupts = 0 42 0;
+   clocks = clock 336;
+   clock-names = watchdog;
+   samsung,sysreg = pmu_sys_reg;


Shouldn't it be samsung,syscon-phandle?


+   status = okay;


The status property is not a part of this binding, so can be dropped
from the example.


+};
diff --git a/drivers/watchdog/s3c2410_wdt.c b/drivers/watchdog/s3c2410_wdt.c
index 23aad7c..235d160 100644
--- a/drivers/watchdog/s3c2410_wdt.c
+++ b/drivers/watchdog/s3c2410_wdt.c
@@ -41,6 +41,8 @@
  #include linux/slab.h
  #include linux/err.h
  #include linux/of.h
+#include linux/mfd/syscon.h
+#include linux/regmap.h

  #define S3C2410_WTCON 0x00
  #define S3C2410_WTDAT 0x04
@@ -61,6 +63,10 @@
  #define CONFIG_S3C2410_WATCHDOG_ATBOOT(0)
  #define CONFIG_S3C2410_WATCHDOG_DEFAULT_TIME  (15)

+#define WDT_DISABLE_REG_OFFSET 0x0408
+#define WDT_MASK_RESET_REG_OFFSET  0x040c
+#define QUIRK_NEEDS_PMU_CONFIG (1  0)
+
  static bool nowayout  = WATCHDOG_NOWAYOUT;
  static int tmr_margin;
  static int tmr_atboot = CONFIG_S3C2410_WATCHDOG_ATBOOT;
@@ -84,18 +90,61 @@ MODULE_PARM_DESC(soft_noboot, Watchdog action, set to 1 to 
ignore reboots, 
0 to reboot (default 0));
  MODULE_PARM_DESC(debug, Watchdog debug, set to 1 for debug (default 0));

+struct s3c2410_wdt_variant {
+   int disable_reg;
+   int mask_reset_reg;
+   int mask_bit;
+   u32 quirks;


nit: To avoid the need of changing the indentation in future, if fields
with longer types gets added, I would simply separate type name from field
name using a single space.


+};
+
  struct s3c2410_wdt {
-   struct device   *dev;
-   struct clk  *clock;
-   void __iomem*reg_base;
-   unsigned intcount;
-   spinlock_t  lock;
-   unsigned long   wtcon_save;
-   unsigned long   wtdat_save;
-   struct watchdog_device  wdt_device;
-   struct notifier_block   freq_transition;
+   struct device   *dev;
+   struct clk  *clock;
+   void __iomem*reg_base;
+   unsigned intcount;
+   spinlock_t  lock;
+   unsigned long   wtcon_save;
+   unsigned long   wtdat_save;
+   struct watchdog_device  wdt_device;
+   struct notifier_block   freq_transition;
+   struct s3c2410_wdt_variant  *pmu_config;
+   struct regmap   *pmureg;


And here's an example of such (unnecessary) change. I believe it would
be better to keep indendation of other fields as is, use single space for
the new field and then change remaining fields in a separate patch,
outside of this series.


+};
+
+#ifdef CONFIG_OF
+static struct s3c2410_wdt_variant pmu_config_s3c2410 = {


static const struct


+   .disable_reg = 0x0,
+   .mask_reset_reg = 0x0,
+   

Re: [PATCH V4 1/3] ARM: dts: Add pmu sysreg node to Exynos5 dtsi file

[Guenter Roeck]

On Wed, Oct 30, 2013 at 12:22:09PM +0100, Tomasz Figa wrote:
 On Wednesday 30 of October 2013 15:43:19 Sachin Kamat wrote:
  On 30 October 2013 15:39, Leela Krishna Amudala l.kris...@samsung.com 
  wrote:
   Hi,
  
   On Wed, Oct 30, 2013 at 3:22 PM, Sachin Kamat sachin.ka...@linaro.org 
   wrote:
   Hi Leela,
  
   On 30 October 2013 15:21, Leela Krishna Amudala l.kris...@samsung.com 
   wrote:
   This patch adds pmusysreg node to Exynos5 dtsi file to handle PMU
   register accesses in a centralized way using syscon driver
  
   Signed-off-by: Leela Krishna Amudala l.kris...@samsung.com
   ---
arch/arm/boot/dts/exynos5.dtsi |5 +
1 file changed, 5 insertions(+)
  
   diff --git a/arch/arm/boot/dts/exynos5.dtsi 
   b/arch/arm/boot/dts/exynos5.dtsi
   index e52b038..918e732 100644
   --- a/arch/arm/boot/dts/exynos5.dtsi
   +++ b/arch/arm/boot/dts/exynos5.dtsi
   @@ -106,4 +106,9 @@
   #size-cells = 0;
   status = disabled;
   };
   +
   +   pmu_sys_reg: pmusysreg@1004000 {
   +   compatible = syscon;
   +   reg = 0x1004 0x5000;
   +   };
};
  
   Had a look at this in a bit detail and found the following.
   The register base address for this block on 5250 and 5420 as per the
   TRM is 0x1005.
  
   Also, the binding document specifies the naming convention. According
   to it this node
   should like:
  
   sys_reg: sysreg@1005 {
  compatible = samsung,exynos5-sysreg, syscon;
  reg = 0x1005 0x500;
   };
  
  
   I know, but here my intention is not to regmap system register 
   (0x1005),
   but instead PMU register (0x1004), Hence created this node.
  
  This clashes with the existing binding for this type of node. Probably
  you will need to
  define it differently?
 
 PMU and System Registers are two completely separate entities. However
 a generic syscon binding can be used to represent both, because they are
 just collections of registers shared by multiple IPs.
 

I would suggest for the participants in this discussion to send Reviewed-by:
or Acked-by: feedback once you are happy with the patches. I am sure this
would help Wim tremendously when deciding if the series is ready for
integration.

Thanks,
Guenter
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] Documentation/watchdog: Update node name in samsung-wdt example

[Guenter Roeck]

On 10/27/2013 11:24 PM, Sachin Kamat wrote:

Update the name as per DT naming convention.

Signed-off-by: Sachin Kamat sachin.ka...@linaro.org


Reviewed-by: Guenter Roeck li...@roeck-us.net


--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] Documentation/watchdog: Update node name in samsung-wdt example

[Guenter Roeck]

On 10/27/2013 11:24 PM, Sachin Kamat wrote:

Update the name as per DT naming convention.

Signed-off-by: Sachin Kamat sachin.ka...@linaro.org
---
  .../devicetree/bindings/watchdog/samsung-wdt.txt   |2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/watchdog/samsung-wdt.txt 
b/Documentation/devicetree/bindings/watchdog/samsung-wdt.txt
index 4c798e3..0642fb8 100644
--- a/Documentation/devicetree/bindings/watchdog/samsung-wdt.txt
+++ b/Documentation/devicetree/bindings/watchdog/samsung-wdt.txt
@@ -17,7 +17,7 @@ Optional properties:

  Example:

-watchdog {
+watchdog@101D {
compatible = samsung,s3c2410-wdt;
reg = 0x101D 0x100, 0x10040408 0x4, 0x1004040c 0x4;
interrupts = 0 42 0;



Sachin,

It appears that this patch depends on some other patch which is not upstream 
yet.
It is not in -next either. Can you point to that other patch ?

Thanks,
Guenter



--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH V3] watchdog: s3c2410_wdt: remove the global variables

[Guenter Roeck]

On Wed, Aug 21, 2013 at 10:01:05PM +0200, Wim Van Sebroeck wrote:
 Hi All,
 
  Leela Krishna Amudala wrote:
   
   This patch removes the global variables in the driver file and
   group them into a structure.
   
   Signed-off-by: Leela Krishna Amudala l.kris...@samsung.com
  
  (+ Wim Van Sebroeck)
  
  Looks good to me,
  
  Acked-by: Kukjin Kim kgene@samsung.com
  
  Thanks,
  Kukjin
 
 Can someone sent me the (unquoted) patch so that I can review and add it?
 

Try this:

wget http://download.gmane.org/gmane.linux.kernel.samsung-soc/21022/21023;

Guenter
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 20/29] watchdog: s3c2410_wdt: simplify use of devm_ioremap_resource

[Guenter Roeck]

On 08/14/2013 02:11 AM, Julia Lawall wrote:

From: Julia Lawall julia.law...@lip6.fr

Remove unneeded error handling on the result of a call to
platform_get_resource when the value is passed to devm_ioremap_resource.

Move the call to platform_get_resource adjacent to the call to
devm_ioremap_resource to make the connection between them more clear.

A simplified version of the semantic patch that makes this change is as
follows: (http://coccinelle.lip6.fr/)

// smpl
@@
expression pdev,res,n,e,e1;
expression ret != 0;
identifier l;
@@

- res = platform_get_resource(pdev, IORESOURCE_MEM, n);
   ... when != res
- if (res == NULL) { ... \(goto l;\|return ret;\) }
   ... when != res
+ res = platform_get_resource(pdev, IORESOURCE_MEM, n);
   e = devm_ioremap_resource(e1, res);
// /smpl

Signed-off-by: Julia Lawall julia.law...@lip6.fr



Reviewed-by: Guenter Roeck li...@roeck-us.net


--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] spi/s3c64xx: Drop extra calls to spi_master_get in suspend/remove functions

[Guenter Roeck]

On Thu, Aug 23, 2012 at 02:40:45PM +0900, Kukjin Kim wrote:
 Guenter Roeck wrote:
  
  Suspend and resume functions call spi_master_get() without matching
  spi_master_put(). The extra references are unnecessary and cause
  subsequent
  module unload attempts to fail. Drop the calls.
  
  Signed-off-by: Guenter Roeck li...@roeck-us.net
 
 Acked-by: Kukjin Kim kgene@samsung.com
 
 (Cc'ed Mark Brown who is handling spi now)
 
 Guenter, maybe you need to re-send this patch to Mark so that he can apply
 ;-)
 
Guess you are right - and I did copy Mark on my later patches. Mark, I just
bounced the patch to you, so you should have it now.

Thanks,
Guenter

 Thanks.
 
 Best regards,
 Kgene.
 --
 Kukjin Kim kgene@samsung.com, Senior Engineer,
 SW Solution Development Team, Samsung Electronics Co., Ltd.
 
  ---
   drivers/spi/spi-s3c64xx.c |8 
   1 file changed, 4 insertions(+), 4 deletions(-)
  
  diff --git a/drivers/spi/spi-s3c64xx.c b/drivers/spi/spi-s3c64xx.c
  index 646a765..d7a87df 100644
  --- a/drivers/spi/spi-s3c64xx.c
  +++ b/drivers/spi/spi-s3c64xx.c
  @@ -1409,7 +1409,7 @@ static int s3c64xx_spi_remove(struct platform_device
  *pdev)
   #ifdef CONFIG_PM
   static int s3c64xx_spi_suspend(struct device *dev)
   {
  -   struct spi_master *master = spi_master_get(dev_get_drvdata(dev));
  +   struct spi_master *master = dev_get_drvdata(dev);
  struct s3c64xx_spi_driver_data *sdd =
  spi_master_get_devdata(master);
  
  spi_master_suspend(master);
  @@ -1428,7 +1428,7 @@ static int s3c64xx_spi_suspend(struct device *dev)
  
   static int s3c64xx_spi_resume(struct device *dev)
   {
  -   struct spi_master *master = spi_master_get(dev_get_drvdata(dev));
  +   struct spi_master *master = dev_get_drvdata(dev);
  struct s3c64xx_spi_driver_data *sdd =
  spi_master_get_devdata(master);
  struct s3c64xx_spi_info *sci = sdd-cntrlr_info;
  
  @@ -1452,7 +1452,7 @@ static int s3c64xx_spi_resume(struct device *dev)
   #ifdef CONFIG_PM_RUNTIME
   static int s3c64xx_spi_runtime_suspend(struct device *dev)
   {
  -   struct spi_master *master = spi_master_get(dev_get_drvdata(dev));
  +   struct spi_master *master = dev_get_drvdata(dev);
  struct s3c64xx_spi_driver_data *sdd =
  spi_master_get_devdata(master);
  
  clk_disable(sdd-clk);
  @@ -1463,7 +1463,7 @@ static int s3c64xx_spi_runtime_suspend(struct device
  *dev)
  
   static int s3c64xx_spi_runtime_resume(struct device *dev)
   {
  -   struct spi_master *master = spi_master_get(dev_get_drvdata(dev));
  +   struct spi_master *master = dev_get_drvdata(dev);
  struct s3c64xx_spi_driver_data *sdd =
  spi_master_get_devdata(master);
  
  clk_enable(sdd-src_clk);
  --
  1.7.9.7
 
 
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Unbalanced calls to spi_master_get in coldfire-qspi and s3c64xx SPI master drivers

[Guenter Roeck]

On Thu, Aug 16, 2012 at 02:27:49PM +0530, Thomas Abraham wrote:
 On 14 August 2012 03:14, Guenter Roeck li...@roeck-us.net wrote:
  Hi all,
 
  looking through SPI master drivers, I noticed that the following drivers 
  call
  spi_master_get() in their suspend and resume functions. Yet, there is no
  matching call to spi_master_put(), meaning the reference count will increase
  with each suspend/resume cycle.
  spi-coldfire-qspi.c
  spi-s3c64xx.c
  Other SPI master drivers also support suspend and resume, but do not call
  spi_master_get() in the suspend/resume functions. The spi-pl022 driver calls
  spi_master_suspend() and spi_master_resume() like the above, but does not
  call spi_master_get() either.
 
  This leads me to believe that the above drivers will hang on unload after a
  suspend/resume cycle due to the extra references.
 
  Can someone please have a look and confirm if my understanding is correct ?
  If so I'll send a set of patches to fix the problems.
 
 For spi-s3c64xx.c, yes it does seem that the spi_master_get() and
 spi_master_put() calls are not balanced.  The probable change could be
 
 -  struct spi_master *master = spi_master_get(dev_get_drvdata(dev));
 + struct spi_master *master = dev_get_drvdata(dev);
 
Yes, that is what I thought.

Thanks,
Guenter
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] spi/s3c64xx: Drop extra calls to spi_master_get in suspend/remove functions

[Guenter Roeck]

Suspend and resume functions call spi_master_get() without matching
spi_master_put(). The extra references are unnecessary and cause subsequent
module unload attempts to fail. Drop the calls.

Signed-off-by: Guenter Roeck li...@roeck-us.net
---
 drivers/spi/spi-s3c64xx.c |8 
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/spi/spi-s3c64xx.c b/drivers/spi/spi-s3c64xx.c
index 646a765..d7a87df 100644
--- a/drivers/spi/spi-s3c64xx.c
+++ b/drivers/spi/spi-s3c64xx.c
@@ -1409,7 +1409,7 @@ static int s3c64xx_spi_remove(struct platform_device 
*pdev)
 #ifdef CONFIG_PM
 static int s3c64xx_spi_suspend(struct device *dev)
 {
-   struct spi_master *master = spi_master_get(dev_get_drvdata(dev));
+   struct spi_master *master = dev_get_drvdata(dev);
struct s3c64xx_spi_driver_data *sdd = spi_master_get_devdata(master);
 
spi_master_suspend(master);
@@ -1428,7 +1428,7 @@ static int s3c64xx_spi_suspend(struct device *dev)
 
 static int s3c64xx_spi_resume(struct device *dev)
 {
-   struct spi_master *master = spi_master_get(dev_get_drvdata(dev));
+   struct spi_master *master = dev_get_drvdata(dev);
struct s3c64xx_spi_driver_data *sdd = spi_master_get_devdata(master);
struct s3c64xx_spi_info *sci = sdd-cntrlr_info;
 
@@ -1452,7 +1452,7 @@ static int s3c64xx_spi_resume(struct device *dev)
 #ifdef CONFIG_PM_RUNTIME
 static int s3c64xx_spi_runtime_suspend(struct device *dev)
 {
-   struct spi_master *master = spi_master_get(dev_get_drvdata(dev));
+   struct spi_master *master = dev_get_drvdata(dev);
struct s3c64xx_spi_driver_data *sdd = spi_master_get_devdata(master);
 
clk_disable(sdd-clk);
@@ -1463,7 +1463,7 @@ static int s3c64xx_spi_runtime_suspend(struct device *dev)
 
 static int s3c64xx_spi_runtime_resume(struct device *dev)
 {
-   struct spi_master *master = spi_master_get(dev_get_drvdata(dev));
+   struct spi_master *master = dev_get_drvdata(dev);
struct s3c64xx_spi_driver_data *sdd = spi_master_get_devdata(master);
 
clk_enable(sdd-src_clk);
-- 
1.7.9.7

--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] spi/s3c64xx: Drop extra calls to spi_master_get in suspend/remove functions

[Guenter Roeck]

Sigh.

s/remove/resume/ in headline.

Guenter

On Thu, Aug 16, 2012 at 08:14:25PM -0700, Guenter Roeck wrote:
 Suspend and resume functions call spi_master_get() without matching
 spi_master_put(). The extra references are unnecessary and cause subsequent
 module unload attempts to fail. Drop the calls.
 
 Signed-off-by: Guenter Roeck li...@roeck-us.net
 ---
  drivers/spi/spi-s3c64xx.c |8 
  1 file changed, 4 insertions(+), 4 deletions(-)
 
 diff --git a/drivers/spi/spi-s3c64xx.c b/drivers/spi/spi-s3c64xx.c
 index 646a765..d7a87df 100644
 --- a/drivers/spi/spi-s3c64xx.c
 +++ b/drivers/spi/spi-s3c64xx.c
 @@ -1409,7 +1409,7 @@ static int s3c64xx_spi_remove(struct platform_device 
 *pdev)
  #ifdef CONFIG_PM
  static int s3c64xx_spi_suspend(struct device *dev)
  {
 - struct spi_master *master = spi_master_get(dev_get_drvdata(dev));
 + struct spi_master *master = dev_get_drvdata(dev);
   struct s3c64xx_spi_driver_data *sdd = spi_master_get_devdata(master);
  
   spi_master_suspend(master);
 @@ -1428,7 +1428,7 @@ static int s3c64xx_spi_suspend(struct device *dev)
  
  static int s3c64xx_spi_resume(struct device *dev)
  {
 - struct spi_master *master = spi_master_get(dev_get_drvdata(dev));
 + struct spi_master *master = dev_get_drvdata(dev);
   struct s3c64xx_spi_driver_data *sdd = spi_master_get_devdata(master);
   struct s3c64xx_spi_info *sci = sdd-cntrlr_info;
  
 @@ -1452,7 +1452,7 @@ static int s3c64xx_spi_resume(struct device *dev)
  #ifdef CONFIG_PM_RUNTIME
  static int s3c64xx_spi_runtime_suspend(struct device *dev)
  {
 - struct spi_master *master = spi_master_get(dev_get_drvdata(dev));
 + struct spi_master *master = dev_get_drvdata(dev);
   struct s3c64xx_spi_driver_data *sdd = spi_master_get_devdata(master);
  
   clk_disable(sdd-clk);
 @@ -1463,7 +1463,7 @@ static int s3c64xx_spi_runtime_suspend(struct device 
 *dev)
  
  static int s3c64xx_spi_runtime_resume(struct device *dev)
  {
 - struct spi_master *master = spi_master_get(dev_get_drvdata(dev));
 + struct spi_master *master = dev_get_drvdata(dev);
   struct s3c64xx_spi_driver_data *sdd = spi_master_get_devdata(master);
  
   clk_enable(sdd-src_clk);
 -- 
 1.7.9.7
 
 
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 0/2] ARM: exynos: Enable TMU support in exynos platforms

[Guenter Roeck]

On Tue, Jul 24, 2012 at 12:20:33PM +0530, Sachin Kamat wrote:
 Hi Amit,
 
 On 14 July 2012 12:55, amit kachhap amit.kach...@gmail.com wrote:
  On Fri, Jul 13, 2012 at 8:11 PM, Guenter Roeck li...@roeck-us.net wrote:
  On Fri, Jul 13, 2012 at 08:28:18PM +0900, Kukjin Kim wrote:
  Amit Daniel Kachhap wrote:
  
   These TMU enablement patches are needed for exynos4 and exynos5 TMU
   driver patches sent earlier. The link for those are
   http://www.spinics.net/lists/lm-sensors/msg35858.html.
  
  How was going on above patches? I can't see them you said in linux-next 
  now,
  it means if I apply this series in my tree, problem will be happened :(
 
  Note1: I've seen Rafael's updating exynos4_tmu patch which is using struct
  dev_pm_ops for pm and applied by Guenter.
 
  If that is in the way, I can drop it, to be applied after the move. Let me 
  know.
  There is now another patch pending, to convert the driver to use devm_ 
  functions.
  Please let me know what you want me to do with it.
 
  Hi Guenter,
 
  I rebased my V5 patch series with  hwmon-next branch so Rafael's pm
  changes are already included. Regarding the devm_ patch by sachin
  please hold it.
 
 Guenter has already sent the pull request to Linus for v3.6 [1].
 That does not include your patches. I do not understand why you want
 to hold back my patch when you can rebase your series on top of it.
 
 [1] https://lkml.org/lkml/2012/7/23/414
 
Linus doesn't like rebases, especially not from one tree to another.
As it is, it may even be that he rejects my pull request because I rebased
to 3.5 before sending the pull request to him (I'll definitely not do that
again).

In addition, it may well be that some of your changes no longer apply
after the driver moved to its new location. So it is overall safer to move
the driver first, and convert it to devm_ functions afterwards.

If anything, I should not have applied Rafael's patch to the driver either.

Guenter
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 0/2] ARM: exynos: Enable TMU support in exynos platforms

[Guenter Roeck]

On Tue, Jul 24, 2012 at 09:53:11PM +0200, Rafael J. Wysocki wrote:
[ ... ]
  If anything, I should not have applied Rafael's patch to the driver either.
 
 Well, it looks like I sent it at a wrong time.  Sorry about that.
 
My fault, not yours.

Guenter
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 0/2] ARM: exynos: Enable TMU support in exynos platforms

[Guenter Roeck]

On Fri, Jul 13, 2012 at 08:28:18PM +0900, Kukjin Kim wrote:
 Amit Daniel Kachhap wrote:
  
  These TMU enablement patches are needed for exynos4 and exynos5 TMU
  driver patches sent earlier. The link for those are
  http://www.spinics.net/lists/lm-sensors/msg35858.html.
  
 How was going on above patches? I can't see them you said in linux-next now,
 it means if I apply this series in my tree, problem will be happened :(
 
 Note1: I've seen Rafael's updating exynos4_tmu patch which is using struct
 dev_pm_ops for pm and applied by Guenter.
 
If that is in the way, I can drop it, to be applied after the move. Let me know.
There is now another patch pending, to convert the driver to use devm_ 
functions.
Please let me know what you want me to do with it.

 Note2: would be helpful if you could do adding me in Cc of exynos tmu
 patches...
 
If you want to be copied, it might make sense to add yourself as maintainer for
this file. Otherwise you can not expect people to know.

Thanks,
Guenter
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[sachin.ka...@linaro.org: [PATCH] hwmon: exynos4_tmu: Use devm_* functions]

[Guenter Roeck]

Fyi.

Guenter

- Forwarded message from Sachin Kamat sachin.ka...@linaro.org -

Date: Fri, 13 Jul 2012 14:16:27 +0530
From: Sachin Kamat sachin.ka...@linaro.org
To: lm-sens...@lm-sensors.org
Cc: kh...@linux-fr.org, li...@roeck-us.net, sachin.ka...@linaro.org, 
patc...@linaro.org
Subject: [PATCH] hwmon: exynos4_tmu: Use devm_* functions
X-Mailer: git-send-email 1.7.4.1

devm_* functions are used to replace kzalloc, request_mem_region, ioremap
and request_irq functions in probe call. With the usage of devm_* functions
explicit freeing and unmapping is not required.

Signed-off-by: Sachin Kamat sachin.ka...@linaro.org
---
 drivers/hwmon/exynos4_tmu.c |   44 --
 1 files changed, 9 insertions(+), 35 deletions(-)

diff --git a/drivers/hwmon/exynos4_tmu.c b/drivers/hwmon/exynos4_tmu.c
index e912059..ee6e26c 100644
--- a/drivers/hwmon/exynos4_tmu.c
+++ b/drivers/hwmon/exynos4_tmu.c
@@ -356,7 +356,8 @@ static int __devinit exynos4_tmu_probe(struct 
platform_device *pdev)
return -ENODEV;
}
 
-   data = kzalloc(sizeof(struct exynos4_tmu_data), GFP_KERNEL);
+   data = devm_kzalloc(pdev-dev, sizeof(struct exynos4_tmu_data),
+   GFP_KERNEL);
if (!data) {
dev_err(pdev-dev, Failed to allocate driver structure\n);
return -ENOMEM;
@@ -364,48 +365,36 @@ static int __devinit exynos4_tmu_probe(struct 
platform_device *pdev)
 
data-irq = platform_get_irq(pdev, 0);
if (data-irq  0) {
-   ret = data-irq;
dev_err(pdev-dev, Failed to get platform irq\n);
-   goto err_free;
+   return data-irq;
}
 
INIT_WORK(data-irq_work, exynos4_tmu_work);
 
data-mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
if (!data-mem) {
-   ret = -ENOENT;
dev_err(pdev-dev, Failed to get platform resource\n);
-   goto err_free;
+   return -ENOENT;
}
 
-   data-mem = request_mem_region(data-mem-start,
-   resource_size(data-mem), pdev-name);
-   if (!data-mem) {
-   ret = -ENODEV;
-   dev_err(pdev-dev, Failed to request memory region\n);
-   goto err_free;
-   }
-
-   data-base = ioremap(data-mem-start, resource_size(data-mem));
+   data-base = devm_request_and_ioremap(pdev-dev, data-mem);
if (!data-base) {
-   ret = -ENODEV;
dev_err(pdev-dev, Failed to ioremap memory\n);
-   goto err_mem_region;
+   return -ENODEV;
}
 
-   ret = request_irq(data-irq, exynos4_tmu_irq,
+   ret = devm_request_irq(pdev-dev, data-irq, exynos4_tmu_irq,
IRQF_TRIGGER_RISING,
exynos4-tmu, data);
if (ret) {
dev_err(pdev-dev, Failed to request irq: %d\n, data-irq);
-   goto err_io_remap;
+   return ret;
}
 
data-clk = clk_get(NULL, tmu_apbif);
if (IS_ERR(data-clk)) {
-   ret = PTR_ERR(data-clk);
dev_err(pdev-dev, Failed to get clock\n);
-   goto err_irq;
+   return PTR_ERR(data-clk);
}
 
data-pdata = pdata;
@@ -440,14 +429,6 @@ err_create_group:
 err_clk:
platform_set_drvdata(pdev, NULL);
clk_put(data-clk);
-err_irq:
-   free_irq(data-irq, data);
-err_io_remap:
-   iounmap(data-base);
-err_mem_region:
-   release_mem_region(data-mem-start, resource_size(data-mem));
-err_free:
-   kfree(data);
 
return ret;
 }
@@ -463,15 +444,8 @@ static int __devexit exynos4_tmu_remove(struct 
platform_device *pdev)
 
clk_put(data-clk);
 
-   free_irq(data-irq, data);
-
-   iounmap(data-base);
-   release_mem_region(data-mem-start, resource_size(data-mem));
-
platform_set_drvdata(pdev, NULL);
 
-   kfree(data);
-
return 0;
 }
 
-- 
1.7.4.1



- End forwarded message -
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 2/5] hwmon: exynos4: Move thermal sensor driver to driver/thermal directory

[Guenter Roeck]

On Sat, May 12, 2012 at 05:40:42AM -0400, Amit Daniel Kachhap wrote:
 This movement is needed because the hwmon entries and corresponding
 sysfs interface is a duplicate of utilities already provided by
 driver/thermal/thermal_sys.c. The goal is to place it in thermal folder
 and add necessary functions to use the in-kernel thermal interfaces.
 
 CC: Guenter Roeck guenter.ro...@ericsson.com
 Signed-off-by: Amit Daniel Kachhap amit.kach...@linaro.org
 Signed-off-by: Donggeun Kim dg77@samsung.com

Acked-by: Guenter Roeck guenter.ro...@ericsson.com

Some suggestions, possibly for later cleanup.

 -
 -   data = kzalloc(sizeof(struct exynos4_tmu_data), GFP_KERNEL);

If you use devm_kzalloc, you won't have to free it.

[ ...]

 -   data-mem = request_mem_region(data-mem-start,
 -   resource_size(data-mem), pdev-name);

Same here, with devm_request_mem_region.

[ ... ]

 -   data-base = ioremap(data-mem-start, resource_size(data-mem));

and devm_ioremap

[ ... ]

 -   ret = request_irq(data-irq, exynos4_tmu_irq,

and devm_request_irq

Guenter
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [lm-sensors] [PATCH 2/4] hwmon: exynos4: Move thermal sensor driver to driver/mfd directory

[Guenter Roeck]

On Sat, Mar 03, 2012 at 06:06:05AM -0500, Amit Daniel Kachhap wrote:
 This movement is needed because the hwmon entries and corresponding
 sysfs interface is a duplicate of utilities already provided by
 driver/thermal/thermal_sys.c. The goal is to place it in mfd folder
 and add necessary calls to get the temperature information.
 
 Signed-off-by: Amit Daniel Kachhap amit.kach...@linaro.org
 Signed-off-by: Donggeun Kim dg77@samsung.com
 ---
  Documentation/hwmon/exynos4_tmu |   81 --
  Documentation/mfd/exynos4_tmu   |   81 ++
  drivers/hwmon/Kconfig   |   10 -
  drivers/hwmon/Makefile  |1 -
  drivers/hwmon/exynos4_tmu.c |  514 
 ---
  drivers/mfd/Kconfig |   10 +
  drivers/mfd/Makefile|1 +
  drivers/mfd/exynos4_tmu.c   |  514 
 +++
  8 files changed, 606 insertions(+), 606 deletions(-)
  delete mode 100644 Documentation/hwmon/exynos4_tmu
  create mode 100644 Documentation/mfd/exynos4_tmu
  delete mode 100644 drivers/hwmon/exynos4_tmu.c
  create mode 100644 drivers/mfd/exynos4_tmu.c
 

You are just moving the driver from hwmon to mfd. It is still a hwmon driver
and registers itself as hwmon driver. That does not make sense. If you want it
as mfd driver, it should not register itself as hwmon driver. It might have 
sub-devices
which are thermal and/or hwmon devices in the respective trees, or it might 
just be
a thermal driver (which then registers itself as hwmon device automatically).

Guenter
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [lm-sensors] [PATCH 3/4] thermal: exynos4: Register the tmu sensor with the thermal interface layer

[Guenter Roeck]

On Sat, Mar 03, 2012 at 06:06:06AM -0500, Amit Daniel Kachhap wrote:
 Export and register information from the tmu temperature sensor to the samsung
 exynos kernel thermal framework where different cooling devices and thermal
 zone are binded. The exported information is based according to the data
 structure thermal_sensor_conf present in exynos_thermal.h. HWMON sysfs
 functions are removed as all of them are present in generic linux thermal 
 layer.
 
 Also the platform data structure is modified to pass frequency cooling
 in percentages for each thermal level.
 
 Signed-off-by: Amit Daniel Kachhap amit.kach...@linaro.org

Ah, that is what I meant. I don't think it makes sense to have this as separate 
patch.

Guenter
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html