On 24.03.2016 17:03, Andy Yan wrote:

Hi Krzystof:

(...)

quoted

quoted

+static int get_reboot_mode_magic(struct reboot_mode_driver *reboot,
+                                const char *cmd)
+{
+       const char *normal = "normal";
+       int magic = 0;
+       struct mode_info *info;
+
+       if (!cmd)
+               cmd = normal;
+
+       list_for_each_entry(info, &reboot->head, list) {
+               if (!strcmp(info->mode, cmd)) {
+                       magic = info->magic;
+                       break;
+               }
+       }
+
+       return magic;

In absence of 'normal' mode (it is not described as required property)
the magic will be '0'. It would be nice to document that in bindings.
Imagine someone forgets about this and will wonder why 0x0 is written
to his precious register on normal reboot...

    If the magic value is '0', we won't touch the register, please see
reboot_mode_notify bellow.

Ah, indeed... so we cannot use value of '0' for magic (e.g. to clear any
existing value for normal reboot)?

quoted

It would be nice to document that 'mode-normal' has a special
(hard-coded) meaning.

quoted

+}
+
+static int reboot_mode_notify(struct notifier_block *this,
+                             unsigned long mode, void *cmd)
+{
+       struct reboot_mode_driver *reboot;
+       int magic;
+
+       reboot = container_of(this, struct reboot_mode_driver,
reboot_notifier);
+       magic = get_reboot_mode_magic(reboot, cmd);
+       if (magic)
+               reboot->write(magic);
+
+       return NOTIFY_DONE;
+}
+
+int reboot_mode_register(struct device *dev, int (*write)(int))
+{
+       struct reboot_mode_driver *reboot;
+       struct mode_info *info;
+       struct property *prop;
+       size_t len = strlen(PREFIX);
+       int ret;
+
+       reboot = devm_kzalloc(dev, sizeof(*reboot), GFP_KERNEL);
+       if (!reboot)
+               return -ENOMEM;
+
+       reboot->write = write;
+       INIT_LIST_HEAD(&reboot->head);
+       for_each_property_of_node(dev->of_node, prop) {
+               if (len > strlen(prop->name) || strncmp(prop->name,
PREFIX, len))
+                       continue;

New line please for readability.

    Okay

quoted

quoted

+               info = devm_kzalloc(dev, sizeof(*info), GFP_KERNEL);
+               if (!info)
+                       return -ENOMEM;

Ditto.

    Okay

quoted

quoted

+               strcpy(info->mode, prop->name + len);

Ehm, and how do you protect that name of mode is shorter than 32
characters?

    How about info->mode = prop->name + len ?

I don't get your answer.
As fair as I read the code, the prop->name can be very long and you are
copying it from 5 character. If the name of the mode has bazillion
characters then again my question: how do you protect that it will fit
in 32 bytes of 'mode'?

quoted

quoted

+               if (of_property_read_u32(dev->of_node, prop->name,
&info->magic)) {
+                       dev_err(dev, "reboot mode %s without magic
number\n",
+                               info->mode);
+                       devm_kfree(dev, info);
+                       continue;
+               }
+               list_add_tail(&info->list, &reboot->head);
+       }
+       reboot->reboot_notifier.notifier_call = reboot_mode_notify;
+       ret = register_reboot_notifier(&reboot->reboot_notifier);
+       if (ret)
+               dev_err(dev, "can't register reboot notifier\n");
+
+       return ret;
+}
+EXPORT_SYMBOL_GPL(reboot_mode_register);
+
+MODULE_AUTHOR("Andy Yan <andy.yan@rock-chips.com");
+MODULE_DESCRIPTION("System reboot mode driver");
+MODULE_LICENSE("GPL v2");

diff --git a/drivers/power/reset/reboot-mode.h

b/drivers/power/reset/reboot-mode.h
new file mode 100644
index 0000000..44ed34f

--- /dev/null
+++ b/drivers/power/reset/reboot-mode.h

@@ -0,0 +1,6 @@
+#ifndef __REBOOT_MODE_H__
+#define __REBOOT_MODE_H__
+
+int reboot_mode_register(struct device *dev, int (*write)(int));

Documentation would be appreciated. Although it is local header but
you decoupled them and you are exporting the function.

    I think I should remove the EXPORT_SYMBOL_GPL.

You need to export symbols which are located in modules. Is it located
in module?

Best regards,
Krzysztof