From mboxrd@z Thu Jan 1 00:00:00 1970 From: Krzysztof Kozlowski Subject: Re: [PATCH v6 2/4] power: reset: add reboot mode driver Date: Mon, 28 Mar 2016 15:34:32 +0900 Message-ID: <56F8D078.6040503@samsung.com> References: <1458646525-491-1-git-send-email-andy.yan@rock-chips.com> <1458646642-591-1-git-send-email-andy.yan@rock-chips.com> <56F39F36.5050702@rock-chips.com> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit Return-path: In-reply-to: <56F39F36.5050702@rock-chips.com> Sender: linux-pm-owner@vger.kernel.org To: Andy Yan Cc: robh+dt@kernel.org, sre@kernel.org, heiko@sntech.de, john.stultz@linaro.org, arnd@arndb.de, galak@codeaurora.org, ijc+devicetree@hellion.org.uk, catalin.marinas@arm.com, olof@lixom.net, alexandre.belloni@free-electrons.com, dbaryshkov@gmail.com, jun.nie@linaro.org, pawel.moll@arm.com, will.deacon@arm.com, linux-rockchip@lists.infradead.org, matthias.bgg@gmail.com, devicetree@vger.kernel.org, linux-pm@vger.kernel.org, f.fainelli@gmail.com, linux@arm.linux.org.uk, mbrugger@suse.com, linux-arm-kernel@lists.infradead.org, lorenzo.pieralisi@arm.com, moritz.fischer@ettus.com, linux-kernel@vger.kernel.org, wxt@rock-chips.com, dwmw2@infradead.org, mark.rutland@arm.com List-Id: linux-rockchip.vger.kernel.org On 24.03.2016 17:03, Andy Yan wrote: > Hi Krzystof: (...) >>> +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)? >> It would be nice to document that 'mode-normal' has a special >> (hard-coded) meaning. >> >>> +} >>> + >>> +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 >> >>> + info = devm_kzalloc(dev, sizeof(*info), GFP_KERNEL); >>> + if (!info) >>> + return -ENOMEM; >> Ditto. > > Okay >> >>> + 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'? >> >>> + 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 >> +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