Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753840AbcC1Hl1 (ORCPT ); Mon, 28 Mar 2016 03:41:27 -0400 Received: from regular1.263xmail.com ([211.150.99.137]:45057 "EHLO regular1.263xmail.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753416AbcC1Hl0 (ORCPT ); Mon, 28 Mar 2016 03:41:26 -0400 X-263anti-spam: KSV:0; X-MAIL-GRAY: 0 X-MAIL-DELIVERY: 1 X-KSVirus-check: 0 X-ABS-CHECKED: 4 X-ADDR-CHECKED: 0 X-MAE-MAIL_UID: 0056f8e00c0a1bce@maes@fc5a8a0b175a0581 X-MAE-ID: 223.6.254.61 X-RL-SENDER: andy.yan@rock-chips.com X-LOGIN-NAME: andy.yan@rock-chips.com X-SENDER-IP: 121.15.173.1 Subject: Re: [PATCH v6 2/4] power: reset: add reboot mode driver To: Krzysztof Kozlowski 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> <56F8D078.6040503@samsung.com> 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 From: Andy Yan Message-ID: <56F8DFED.50905@rock-chips.com> Date: Mon, 28 Mar 2016 15:40:29 +0800 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.5.1 MIME-Version: 1.0 In-Reply-To: <56F8D078.6040503@samsung.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 5037 Lines: 148 Hi Krzysztof : On 2016年03月28日 14:34, Krzysztof Kozlowski wrote: > 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 seems that the value '0' cannot be used. >>> 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'? What I mean is set info->mode as a pointer point to prop->name + len struct mode_info { char *mode; .......... ......... } info->mode = prop->name + len > >>>> + 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 > > > > >