* Re: [PATCH V2 1/2] dt-bindings: leds: document new led-triggers property
From: Jacek Anaszewski @ 2017-01-21 21:42 UTC (permalink / raw)
To: Rafał Miłecki
Cc: Greg Kroah-Hartman,
linux-usb-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
open list:LED SUBSYSTEM, Richard Purdie, Pavel Machek,
devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, Rob Herring,
Mark Rutland, Rafał Miłecki
In-Reply-To: <CACna6rx-qJ5eLMOUbMcaAxaOp9avrj1sa-8zZo2m+rVvY+Kvjw-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
On 01/21/2017 05:24 PM, Rafał Miłecki wrote:
> On 20 January 2017 at 23:35, Jacek Anaszewski
> <jacek.anaszewski-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>> On 01/20/2017 10:56 PM, Rafał Miłecki wrote:
>>> From: Rafał Miłecki <rafal-g1n6cQUeyibVItvQsEIGlw@public.gmane.org>
>>>
>>> Some LEDs can be related to particular devices described in DT. This
>>> property allows specifying such relations. E.g. USB LED should usually
>>> be used to indicate some USB port(s) state.
>>>
>>> Signed-off-by: Rafał Miłecki <rafal-g1n6cQUeyibVItvQsEIGlw@public.gmane.org>
>>> ---
>>> V2: Replace "usb-ports" with "led-triggers" property which is more generic and
>>> allows specifying other devices as well.
>>>
>>> When bindings patch is related to some followup implementation, they usually go
>>> through the same tree.
>>>
>>> Greg: this patch is based on top of e64b8cc72bf9 ("DT: leds: Improve examples by
>>> adding some context") from kernel/git/j.anaszewski/linux-leds.git . Is there any
>>> way to solve this dependency issue? Or should this patch wait until 3.11 is
>>> released?
>>> ---
>>> Documentation/devicetree/bindings/leds/common.txt | 16 ++++++++++++++++
>>> 1 file changed, 16 insertions(+)
>>>
>>> diff --git a/Documentation/devicetree/bindings/leds/common.txt b/Documentation/devicetree/bindings/leds/common.txt
>>> index 24b656014089..17632a041196 100644
>>> --- a/Documentation/devicetree/bindings/leds/common.txt
>>> +++ b/Documentation/devicetree/bindings/leds/common.txt
>>> @@ -49,6 +49,17 @@ Optional properties for child nodes:
>>> - panic-indicator : This property specifies that the LED should be used,
>>> if at all possible, as a panic indicator.
>>>
>>> +- led-triggers : List of devices that should trigger this LED activity. Some
>>> + LEDs can be related to a specific device and should somehow
>>> + indicate its state. E.g. USB 2.0 LED may react to device(s) in
>>> + a USB 2.0 port(s). Another common example is switch or router
>>> + with multiple Ethernet ports each of them having its own LED
>>> + assigned (assuminled-trigger-usbportg they are not hardwired).
>>> + In such cases this property should contain phandle(s) of
>>> + related device(s). In many cases LED can be related to more
>>> + than one device (e.g. one USB LED vs. multiple USB ports) so a
>>> + list of entries can be specified.
>>> +
>>
>> This implies that it is possible to define multiple triggers for
>> a LED class device but it is not supported by LED Trigger core.
>> There is linux,default-trigger property which allows to define one
>> trigger that will be initially assigned.
>
> I think we owe some extra explanation to people not closely familiar
> with the triggers.
>
> Linux has multiple trigger drivers each supporting some *type* of
> events. Linux supports assigning only one trigger driver to LED at a
> time.
> This means you can use e.g.:
> 1) "usbport" trigger driver that supports USB events
> XOR
> 2) "timer" trigger driver that uses timer to control LED
> XOR
> 3) "cpu" trigger driver that supports CPU events
> at once.
Correct.
> With proposed "led-triggers" property one could assign different
> trigger *sources* to a LED. You could e.g. assign 2 USB ports, network
> device & CPU to a single LED. For reason explained above Linux
> couldn't support all of them at once.
>
>
>> I am aware that this is renamed usb-ports property from v1,
>> that attempts to address Rob's comment, but we can't do that this way.
>> Maybe usb-ports property could be documented in led-trigger-usbport's
>> specific bindings and a reference to it could be added next to the
>> related entry on the list of the available LED triggers (which is
>> actually missing) in Documentation/devicetree/bindings/leds/common.txt.
>
> I'm wondering if we need to care about this Linux limitation and if it
> should stop us from adding this flexible DT property. Maybe we could
> live with Linux having limitation of supporting one trigger type at a
> time?
That's what I meant. Generally I have objections to the generic
property for defining list of allowed triggers. That's why I proposed
to stay by usb-ports property that would be specific to only one
trigger: led-trigger-usbport.
led-trigger-usbport in fact is an entirely new type of LED trigger.
LED triggers is a kernel based source of events. All existing triggers
react only to a single, well defined source of events, whereas
led-trigger-usbport allows to define the scope of events (usb ports)
to notify about. Activity on each port is treated similarly and the LED
class device that listens to the trigger notifications doesn't know
which exact port triggered the notification.
>From this POV led-trigger-usbport is kind of facade, which allows
for it to fit well into the LED Trigger design and API, and usb ports
are not identical with LED triggers, but sit rather one level below.
It is led-trigger-usbport which is visible to the LED subsystem, and
not every single usb port.
Therefore it isn't logically justified to rename usb-ports property
to led-triggers. The property should be specific to this trigger.
> So e.g. if one assigns 2 USB ports + network device + CPU and
> decides to use "usport" trigger driver he'll get LED indicating status
> of USB ports only.
How would it be different from the current state? Only in limiting
the scope of triggers available for a LED class device.
> I think we could live with such limitation in Linux support for this
> generic "led-triggers" property. I think it's also not very common to
> have LED with different types of devices assigned. Personally I've
> never seen devices with LED label "USB & CPU" or whatever.
I agree. Nonetheless led-triggers seems to support such a design.
> What do you think about this? Maybe we could also document this
> limitation in trigger docs.
I'm not entirely sure if I got your point. Your reasoning seems
self contradicting to me in few places.
--
Best regards,
Jacek Anaszewski
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH v3 3/3] watchdog: zx2967: add watchdog controller driver for ZTE's zx2967 family
From: Guenter Roeck @ 2017-01-21 23:06 UTC (permalink / raw)
To: Baoyou Xie, jun.nie, wim, robh+dt, mark.rutland
Cc: linux-arm-kernel, linux-watchdog, devicetree, linux-kernel,
shawnguo, xie.baoyou, chen.chaokai, wang.qiang01
In-Reply-To: <1484917398-15604-3-git-send-email-baoyou.xie@linaro.org>
On 01/20/2017 05:03 AM, Baoyou Xie wrote:
> This patch adds watchdog controller driver for ZTE's zx2967 family.
>
> Signed-off-by: Baoyou Xie <baoyou.xie@linaro.org>
> ---
> drivers/watchdog/Kconfig | 10 ++
> drivers/watchdog/Makefile | 1 +
> drivers/watchdog/zx2967_wdt.c | 376 ++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 387 insertions(+)
> create mode 100644 drivers/watchdog/zx2967_wdt.c
>
> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
> index acb00b5..05093a2 100644
> --- a/drivers/watchdog/Kconfig
> +++ b/drivers/watchdog/Kconfig
> @@ -714,6 +714,16 @@ config ASPEED_WATCHDOG
> To compile this driver as a module, choose M here: the
> module will be called aspeed_wdt.
>
> +config ZX2967_WATCHDOG
> + tristate "ZTE zx2967 SoCs watchdog support"
> + depends on ARCH_ZX
> + select WATCHDOG_CORE
> + help
> + Say Y here to include support for the watchdog timer
> + in ZTE zx2967 SoCs.
> + To compile this driver as a module, choose M here: the
> + module will be called zx2967_wdt.
> +
> # AVR32 Architecture
>
> config AT32AP700X_WDT
> diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
> index 0c3d35e..bf2d296 100644
> --- a/drivers/watchdog/Makefile
> +++ b/drivers/watchdog/Makefile
> @@ -82,6 +82,7 @@ obj-$(CONFIG_BCM7038_WDT) += bcm7038_wdt.o
> obj-$(CONFIG_ATLAS7_WATCHDOG) += atlas7_wdt.o
> obj-$(CONFIG_RENESAS_WDT) += renesas_wdt.o
> obj-$(CONFIG_ASPEED_WATCHDOG) += aspeed_wdt.o
> +obj-$(CONFIG_ZX2967_WATCHDOG) += zx2967_wdt.o
>
> # AVR32 Architecture
> obj-$(CONFIG_AT32AP700X_WDT) += at32ap700x_wdt.o
> diff --git a/drivers/watchdog/zx2967_wdt.c b/drivers/watchdog/zx2967_wdt.c
> new file mode 100644
> index 0000000..a5656d0
> --- /dev/null
> +++ b/drivers/watchdog/zx2967_wdt.c
> @@ -0,0 +1,376 @@
> +/*
> + * watchdog driver for ZTE's zx2967 family
> + *
> + * Copyright (C) 2017 ZTE Ltd.
> + *
> + * Author: Baoyou Xie <baoyou.xie@linaro.org>
> + *
> + * License terms: GNU General Public License (GPL) version 2
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/delay.h>
> +#include <linux/io.h>
> +#include <linux/mfd/syscon.h>
> +#include <linux/module.h>
> +#include <linux/of_address.h>
> +#include <linux/platform_device.h>
> +#include <linux/reboot.h>
> +#include <linux/regmap.h>
> +#include <linux/reset.h>
> +#include <linux/watchdog.h>
> +
> +#define ZX2967_WDT_CFG_REG 0x4
> +#define ZX2967_WDT_LOAD_REG 0x8
> +#define ZX2967_WDT_REFRESH_REG 0x18
> +#define ZX2967_WDT_START_REG 0x1c
> +
> +#define ZX2967_WDT_REFRESH_MASK 0x3f
> +
> +#define ZX2967_WDT_CFG_DIV(n) ((((n) & 0xff) - 1) << 8)
> +#define ZX2967_WDT_START_EN 0x1
> +
> +#define ZX2967_WDT_WRITEKEY 0x12340000
> +
> +#define ZX2967_WDT_DIV_DEFAULT 16
> +#define ZX2967_WDT_DEFAULT_TIMEOUT 32
> +#define ZX2967_WDT_MIN_TIMEOUT 1
> +#define ZX2967_WDT_MAX_TIMEOUT 500
Is that based on a real limit or an arbitrary value ?
> +#define ZX2967_WDT_MAX_COUNT 0xffff
> +
> +#define ZX2967_WDT_FLAG_REBOOT_MON (1 << 0)
BIT ?
> +
> +struct zx2967_wdt {
> + struct device *dev;
> + struct clk *clock;
> + void __iomem *reg_base;
> + unsigned int conf;
> + unsigned int load;
> + unsigned int flags;
> + struct watchdog_device wdt_device;
> + struct notifier_block restart_handler;
> + struct notifier_block reboot_handler;
> +};
> +
> +static inline u32 zx2967_wdt_readl(struct zx2967_wdt *wdt, u16 reg)
> +{
> + return readl_relaxed(wdt->reg_base + reg);
> +}
> +
> +static inline void zx2967_wdt_writel(struct zx2967_wdt *wdt, u16 reg, u32 val)
> +{
> + writel_relaxed(val | ZX2967_WDT_WRITEKEY, wdt->reg_base + reg);
> +}
> +
> +static void zx2967_wdt_refresh(struct zx2967_wdt *wdt)
> +{
> + u32 val;
> +
> + val = zx2967_wdt_readl(wdt, ZX2967_WDT_REFRESH_REG);
> + val ^= ZX2967_WDT_REFRESH_MASK;
> + zx2967_wdt_writel(wdt, ZX2967_WDT_REFRESH_REG, val);
> +}
> +
> +static unsigned int
> +__zx2967_wdt_set_timeout(struct zx2967_wdt *wdt, unsigned int timeout)
> +{
> + unsigned int freq = clk_get_rate(wdt->clock);
The clock frequency is set to 32 kHz. It seems unnecessary to re-read it
whenever the timeout changes. Also, ...
> + unsigned int divisor = ZX2967_WDT_DIV_DEFAULT;
> + unsigned int count;
> +
> + count = timeout * freq;
> + if (count > divisor * ZX2967_WDT_MAX_COUNT)
> + divisor = DIV_ROUND_UP(count, ZX2967_WDT_MAX_COUNT);
> + count = DIV_ROUND_UP(count, divisor);
> + zx2967_wdt_writel(wdt, ZX2967_WDT_CFG_REG, ZX2967_WDT_CFG_DIV(divisor));
> + zx2967_wdt_writel(wdt, ZX2967_WDT_LOAD_REG, count);
> + zx2967_wdt_refresh(wdt);
> + wdt->load = count;
> +
> + return (count * divisor) / freq;
... if you think it can change from underneath you, you'll also need to make sure
it is not 0, to avoid a nasty surprise here. Of course, if it does change, you'll
have no idea what the actual timeout is at any given time, and the driver won't work.
> +}
> +
> +static int zx2967_wdt_set_timeout(struct watchdog_device *wdd,
> + unsigned int timeout)
> +{
> + struct zx2967_wdt *wdt = watchdog_get_drvdata(wdd);
> +
> + if (watchdog_timeout_invalid(&wdt->wdt_device, timeout)) {
> + dev_err(wdt->dev, "timeout %d is invalid\n", timeout);
> + return -EINVAL;
> + }
This function is called from the infrastructure. Let's trust the infrastructure
to check the valid range before calling this code.
> +
> + wdd->timeout = __zx2967_wdt_set_timeout(wdt, timeout);
> +
> + return 0;
> +}
> +
> +static void __zx2967_wdt_start(struct zx2967_wdt *wdt)
> +{
> + u32 val;
> +
> + val = zx2967_wdt_readl(wdt, ZX2967_WDT_START_REG);
> + val |= ZX2967_WDT_START_EN;
> + zx2967_wdt_writel(wdt, ZX2967_WDT_START_REG, val);
> +}
> +
> +static void __zx2967_wdt_stop(struct zx2967_wdt *wdt)
> +{
> + u32 val;
> +
> + val = zx2967_wdt_readl(wdt, ZX2967_WDT_START_REG);
> + val &= ~ZX2967_WDT_START_EN;
> + zx2967_wdt_writel(wdt, ZX2967_WDT_START_REG, val);
> +}
> +
> +static int zx2967_wdt_start(struct watchdog_device *wdd)
> +{
> + struct zx2967_wdt *wdt = watchdog_get_drvdata(wdd);
> +
> + __zx2967_wdt_stop(wdt);
> + zx2967_wdt_set_timeout(wdd, wdd->timeout);
This seems inconsistent. First, the watchdog should not already be
started when this function is called. Second, if it is in fact necessary
to stop the watchdog before updating its timeout, you might want to
consider stopping it in the set_timeout function, because that
function _will_ be called if the timeout is updated while the
watchdog is running.
> + __zx2967_wdt_start(wdt);
> +
> + return 0;
> +}
> +
> +static int zx2967_wdt_stop(struct watchdog_device *wdd)
> +{
> + struct zx2967_wdt *wdt = watchdog_get_drvdata(wdd);
> +
> + __zx2967_wdt_stop(wdt);
> +
> + return 0;
> +}
> +
> +static int zx2967_wdt_keepalive(struct watchdog_device *wdd)
> +{
> + struct zx2967_wdt *wdt = watchdog_get_drvdata(wdd);
> +
> + zx2967_wdt_refresh(wdt);
> +
> + return 0;
> +}
> +
> +#define ZX2967_WDT_OPTIONS \
> + (WDIOF_SETTIMEOUT | WDIOF_KEEPALIVEPING | WDIOF_MAGICCLOSE)
> +static const struct watchdog_info zx2967_wdt_ident = {
> + .options = ZX2967_WDT_OPTIONS,
> + .firmware_version = 0,
> + .identity = "zx2967 watchdog",
> +};
> +
> +static struct watchdog_ops zx2967_wdt_ops = {
> + .owner = THIS_MODULE,
> + .start = zx2967_wdt_start,
> + .stop = zx2967_wdt_stop,
> + .ping = zx2967_wdt_keepalive,
> + .set_timeout = zx2967_wdt_set_timeout,
> +};
> +
> +static void zx2967_wdt_fix_sysdown(struct zx2967_wdt *wdt)
> +{
> + __zx2967_wdt_stop(wdt);
> + __zx2967_wdt_set_timeout(wdt, 15);
> + __zx2967_wdt_start(wdt);
> +}
I am really not at all in favor of this code. It force-sets a watchdog
to 15 seconds later, if it was enabled or not.
I don't necessarily oppose the idea in general, but it would have to be
configurable and part of the infrastructure.
> +
> +static int zx2967_wdt_notify_sys(struct notifier_block *this,
> + unsigned long code, void *unused)
> +{
> + struct zx2967_wdt *wdt = container_of(this, struct zx2967_wdt,
> + reboot_handler);
> +
> + wdt->flags |= ZX2967_WDT_FLAG_REBOOT_MON;
> + switch (code) {
> + case SYS_HALT:
> + case SYS_POWER_OFF:
> + case SYS_RESTART:
> + zx2967_wdt_fix_sysdown(wdt);
> + break;
> + default:
> + break;
> + }
> +
> + return 0;
> +}
> +
> +static int zx2967_wdt_restart(struct notifier_block *this,
> + unsigned long mode, void *cmd)
> +{
> + struct zx2967_wdt *wdt;
> +
> + wdt = container_of(this, struct zx2967_wdt, restart_handler);
> +
> + zx2967_wdt_stop(&wdt->wdt_device);
> +
> + zx2967_wdt_writel(wdt, ZX2967_WDT_LOAD_REG, 0x80);
> + zx2967_wdt_refresh(wdt);
> + zx2967_wdt_writel(wdt, ZX2967_WDT_START_REG, ZX2967_WDT_START_EN);
> +
> + zx2967_wdt_start(&wdt->wdt_device);
> + /* wait for reset*/
> + mdelay(500);
> +
> + return NOTIFY_DONE;
> +}
> +
> +static void zx2967_wdt_reset_sysctrl(struct device *dev)
> +{
> + int ret;
> + struct device_node *np = NULL;
> + void __iomem *regmap;
> + unsigned int offset, mask, config;
> + struct of_phandle_args out_args;
> +
> + ret = of_parse_phandle_with_fixed_args(dev->of_node,
> + "zte,wdt-reset-sysctrl", 3, 0, &out_args);
> + if (ret) {
> + dev_info(dev, "failed to parse zte,wdt-reset-sysctrl");
Why this message ? The property is optional. There is no "failure".
Repeating the information in the devicetree description:
zte,wdt-reset-sysctrl : Directs how to reset system by the watchdog.
Given the context, and the provided implementation, I can only assume that
this is supposed to mean which action shall be taken when the watchdog triggers,
and that the bit mask provided is supposed to configure that action. If so,
that should be explained in the devicetree description, and not be hidden
in magic register values.
> + return;
> + }
> + offset = out_args.args[0];
> + config = out_args.args[1];
> + mask = out_args.args[2];
> +
> + regmap = syscon_node_to_regmap(out_args.np);
> + if (IS_ERR(regmap))
> + goto out;
> +
> + regmap_update_bits(regmap, offset, mask, config);
I don't really see the value of the local variables.
> +out:
> + of_node_put(np);
I don't really see where np is set to anything but NULL.
> +}
> +
> +static int zx2967_wdt_probe(struct platform_device *pdev)
> +{
> + struct device *dev;
> + struct zx2967_wdt *wdt;
> + struct resource *base;
> + int ret = 0;
Unnecessary initialization.
> +
> + struct reset_control *rstc;
No empty lines between variable declarations, please.
> +
> + dev = &pdev->dev;
Can be initialized above.
> +
> + wdt = devm_kzalloc(dev, sizeof(*wdt), GFP_KERNEL);
> + if (!wdt)
> + return -ENOMEM;
> +
> + platform_set_drvdata(pdev, wdt);
> +
> + wdt->dev = dev;
This is only used by an error message in the set_timeout function,
which is unnecessary.
> + wdt->wdt_device.info = &zx2967_wdt_ident;
> + wdt->wdt_device.ops = &zx2967_wdt_ops;
> + wdt->wdt_device.timeout = ZX2967_WDT_DEFAULT_TIMEOUT;
> + wdt->wdt_device.max_timeout = ZX2967_WDT_MAX_TIMEOUT;
> + wdt->wdt_device.min_timeout = ZX2967_WDT_MIN_TIMEOUT;
> + wdt->wdt_device.parent = &pdev->dev;
> +
> + base = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + wdt->reg_base = devm_ioremap_resource(dev, base);
> + if (IS_ERR(wdt->reg_base)) {
> + dev_err(dev, "ioremap failed\n");
> + return PTR_ERR(wdt->reg_base);
> + }
> +
> + zx2967_wdt_reset_sysctrl(dev);
> +
> + wdt->reboot_handler.notifier_call = zx2967_wdt_notify_sys;
> + register_reboot_notifier(&wdt->reboot_handler);
Without ever unregistering it ? Did you try to unload the driver and reboot ?
> + wdt->clock = devm_clk_get(dev, NULL);
> + if (IS_ERR(wdt->clock)) {
> + dev_err(dev, "failed to find watchdog clock source\n");
> + return PTR_ERR(wdt->clock);
> + }
> +
> + ret = clk_prepare_enable(wdt->clock);
> + if (ret < 0) {
> + dev_err(dev, "failed to enable clock\n");
> + return ret;
> + }
> + clk_set_rate(wdt->clock, 32768);
> +
> + rstc = devm_reset_control_get(dev, NULL);
> + if (IS_ERR(rstc)) {
> + dev_err(dev, "failed to get rstc");
> + ret = PTR_ERR(rstc);
> + goto fail_get_reset_control;
> + }
> +
> + reset_control_assert(rstc);
> + mdelay(10);
> + reset_control_deassert(rstc);
There is this reset, and the reset in reset_sysctrl above.
Are they both necessary ?
> + watchdog_set_drvdata(&wdt->wdt_device, wdt);
> +
> + watchdog_init_timeout(&wdt->wdt_device,
> + ZX2967_WDT_DEFAULT_TIMEOUT, dev);
What is the purpose of this call ? It sets the timeout to the default timeout,
which is already set, and it does not use the value from devicetree since the
value passed is != 0.
> + watchdog_set_nowayout(&wdt->wdt_device, WATCHDOG_NOWAYOUT);
> +
> + zx2967_wdt_stop(&wdt->wdt_device);
The watchdog was reset twice above. Is this call really necessary ?
> +
> + ret = watchdog_register_device(&wdt->wdt_device);
> + if (ret)
> + goto fail_register;
> +
> + wdt->restart_handler.notifier_call = zx2967_wdt_restart;
> + wdt->restart_handler.priority = 128;
> + ret = register_restart_handler(&wdt->restart_handler);
> + if (ret) {
> + dev_err(dev, "cannot register restart handler, %d\n", ret);
> + goto fail_restart;
> + }
> +
Why not use the infrastructure ?
> + dev_info(dev, "watchdog enabled (timeout=%d sec, nowayout=%d)",
> + wdt->wdt_device.timeout, WATCHDOG_NOWAYOUT);
> +
> + return 0;
> +
> +fail_get_reset_control:
> +fail_restart:
Please no double labels to the same code.
> + watchdog_unregister_device(&wdt->wdt_device);
> +fail_register:
> + clk_disable_unprepare(wdt->clock);
> + return ret;
> +}
> +
> +static int zx2967_wdt_remove(struct platform_device *pdev)
> +{
> + struct zx2967_wdt *wdt = platform_get_drvdata(pdev);
> +
> + unregister_restart_handler(&wdt->restart_handler);
> + watchdog_unregister_device(&wdt->wdt_device);
> + clk_disable_unprepare(wdt->clock);
> +
> + return 0;
> +}
> +
> +static void zx2967_wdt_shutdown(struct platform_device *pdev)
> +{
> + struct zx2967_wdt *wdt = platform_get_drvdata(pdev);
> +
> + if (!(wdt->flags & ZX2967_WDT_FLAG_REBOOT_MON))
> + zx2967_wdt_stop(&wdt->wdt_device);
As mentioned before, the whole reboot handling is highly unusual.
I don't really like the idea of bypassing the infrastructure
(such as watchdog_stop_on_reboot()) for driver specific special behavior.
If such behavior is useful, it should be useful for all watchdog drivers,
and be defined in the infrastructure.
> +}
> +
> +static const struct of_device_id zx2967_wdt_match[] = {
> + { .compatible = "zte,zx296718-wdt", },
> + {}
> +};
> +MODULE_DEVICE_TABLE(of, zx2967_wdt_match);
> +
> +static struct platform_driver zx2967_wdt_driver = {
> + .probe = zx2967_wdt_probe,
> + .remove = zx2967_wdt_remove,
> + .shutdown = zx2967_wdt_shutdown,
> + .driver = {
> + .name = "zx2967-wdt",
> + .of_match_table = of_match_ptr(zx2967_wdt_match),
> + },
> +};
> +module_platform_driver(zx2967_wdt_driver);
> +
> +MODULE_AUTHOR("Baoyou Xie <baoyou.xie@linaro.org>");
> +MODULE_DESCRIPTION("ZTE zx2967 Watchdog Device Driver");
> +MODULE_LICENSE("GPL v2");
>
^ permalink raw reply
* Re: [PATCH v3 1/4] Documentation: dt-bindings: add the Amlogic Meson SAR ADC documentation
From: Martin Blumenstingl @ 2017-01-21 23:10 UTC (permalink / raw)
To: Rob Herring
Cc: jic23, knaack.h, lars, pmeerw, mark.rutland, khilman, linux-iio,
devicetree, linux-amlogic, linux-clk, carlo, catalin.marinas,
will.deacon, mturquette, sboyd, narmstrong, linux-arm-kernel
In-Reply-To: <20170121205635.4qeiqyeyjrweryrh@rob-hp-laptop>
Hi Rob,
thanks for reviewing this!
On Sat, Jan 21, 2017 at 9:56 PM, Rob Herring <robh@kernel.org> wrote:
> On Thu, Jan 19, 2017 at 03:58:19PM +0100, Martin Blumenstingl wrote:
>> This adds the devicetree binding documentation for the SAR ADC found in
>> Amlogic Meson SoCs.
>> Currently only the GXBB, GXL and GXM SoCs are supported.
>>
>> Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
>> Tested-by: Neil Armstrong <narmstrong@baylibre.com>
>> ---
>> .../bindings/iio/adc/amlogic,meson-saradc.txt | 31 ++++++++++++++++++++++
>> 1 file changed, 31 insertions(+)
>> create mode 100644 Documentation/devicetree/bindings/iio/adc/amlogic,meson-saradc.txt
>>
>> diff --git a/Documentation/devicetree/bindings/iio/adc/amlogic,meson-saradc.txt b/Documentation/devicetree/bindings/iio/adc/amlogic,meson-saradc.txt
>> new file mode 100644
>> index 000000000000..9a0bec7afc63
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/iio/adc/amlogic,meson-saradc.txt
>> @@ -0,0 +1,31 @@
>> +* Amlogic Meson SAR (Successive Approximation Register) A/D converter
>> +
>> +Required properties:
>> +- compatible: depending on the SoC this should be one of:
>> + - "amlogic,meson-gxbb-saradc" for GXBB
>> + - "amlogic,meson-gxl-saradc" for GXL and GXM
>
> GXL and GXM should probably be 2 compatibles.
Amlogic's ADC driver does not differentiate between GXL and GXM
(because both SoCs are identical except the CPU and GPU cores).
I can still introduce a separate binding for GXM if you want (better
safe than sorry and takes less than 5 minutes) - just let me know
>> + along with the generic "amlogic,meson-saradc"
>> +- reg: the physical base address and length of the registers
>> +- clocks: phandle and clock identifier (see clock-names)
>> +- clock-names: mandatory clocks:
>> + - "clkin" for the reference clock (typically XTAL)
>> + - "core" for the SAR ADC core clock
>> + optional clocks:
>> + - "sana" for the analog clock
>> + - "adc_clk" for the ADC (sampling) clock
>> + - "adc_sel" for the ADC (sampling) clock mux
>> +- vref-supply: the regulator supply for the ADC reference voltage
>> +- #io-channel-cells: must be 1, see ../iio-bindings.txt
>> +
>> +Example:
>> + saradc: adc@8680 {
>> + compatible = "amlogic,meson-gxl-saradc", "amlogic,meson-saradc";
>> + #io-channel-cells = <1>;
>> + reg = <0x0 0x8680 0x0 0x34>;
>> + clocks = <&xtal>,
>> + <&clkc CLKID_SAR_ADC>,
>> + <&clkc CLKID_SANA>,
>> + <&clkc CLKID_SAR_ADC_CLK>,
>> + <&clkc CLKID_SAR_ADC_SEL>;
>> + clock-names = "clkin", "core", "sana", "adc_clk", "adc_sel";
>> + };
>> --
>> 2.11.0
>>
^ permalink raw reply
* Re: [PATCH 2/2] hwmon: (lm70) Add support for TI TMP122/124
From: Florian Fainelli @ 2017-01-21 23:11 UTC (permalink / raw)
To: Guenter Roeck, linux-kernel-u79uwXL29TY76Z2rM5mHXA
Cc: Rob Herring, Mark Rutland, Jean Delvare, Jonathan Corbet,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
open list:HARDWARE MONITORING, open list:DOCUMENTATION
In-Reply-To: <8ada15e2-2b99-02c3-e731-c469a3d6c3bf-0h96xk9xTtrk1uMJSBkQmQ@public.gmane.org>
On January 21, 2017 12:25:21 PM PST, Guenter Roeck <linux@roeck-us.net> wrote:
>On 01/21/2017 11:20 AM, Florian Fainelli wrote:
>> Add support for Texas Instruments TMP122/124 which are nearly
>identical to
>> their TMP121/123 except that they also support programmable
>temperature
>> thresholds.
>>
>> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
>
>Applied to -next.
Thanks was fast, thanks!
>
>Any plans to add support for the thresholds ?
Yes, as mentioned in the cover letter, need to get SPI_3WIRE working with the SPI controller (spi-ep93xx) to verify the thresholds do work.
Cheers
--
Florian
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH 2/2] hwmon: (lm70) Add support for TI TMP122/124
From: Guenter Roeck @ 2017-01-22 0:01 UTC (permalink / raw)
To: Florian Fainelli, linux-kernel
Cc: Rob Herring, Mark Rutland, Jean Delvare, Jonathan Corbet,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
open list:HARDWARE MONITORING, open list:DOCUMENTATION
In-Reply-To: <C0A9E7E0-D4AB-4154-BC47-656F8E1FE77E@gmail.com>
On 01/21/2017 03:11 PM, Florian Fainelli wrote:
> On January 21, 2017 12:25:21 PM PST, Guenter Roeck <linux@roeck-us.net> wrote:
>> On 01/21/2017 11:20 AM, Florian Fainelli wrote:
>>> Add support for Texas Instruments TMP122/124 which are nearly
>> identical to
>>> their TMP121/123 except that they also support programmable
>> temperature
>>> thresholds.
>>>
>>> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
>>
>> Applied to -next.
>
> Thanks was fast, thanks!
>
Lucky you, the patches caught up with me me while I was going through the backlog.
>>
>> Any plans to add support for the thresholds ?
>
> Yes, as mentioned in the cover letter, need to get SPI_3WIRE working with the SPI controller (spi-ep93xx) to verify the thresholds do work.
>
Guess the hint is that one should read the cover letter :-)
Guenter
^ permalink raw reply
* Re: [PATCH 1/2] dt-bindings: add Asus Tinker board
From: Eddie Cai(蔡枫) @ 2017-01-22 1:06 UTC (permalink / raw)
To: Rob Herring
Cc: Eddie Cai, Mark.Rutland, Stübner, linux, devicetree,
linux-arm-kernel@lists.infradead.org, linux-rockchip,
linux-kernel@vger.kernel.org
In-Reply-To: <20170121204217.y54lup7wkm7y7iwp@rob-hp-laptop>
[-- Attachment #1: Type: text/plain, Size: 1177 bytes --]
2017-01-22 4:42 GMT+08:00 Rob Herring <robh@kernel.org>:
> On Thu, Jan 19, 2017 at 10:11:58AM +0800, Eddie Cai wrote:
> > Tinker board is a credit card size develop board designed by Asus.
> > Powered by RK3288.
> >
> > Signed-off-by: Eddie Cai <eddie.cai@rock-chips.com>
> > ---
> > Documentation/devicetree/bindings/arm/rockchip.txt | 3 +++
> > 1 file changed, 3 insertions(+)
> >
> > diff --git a/Documentation/devicetree/bindings/arm/rockchip.txt
> b/Documentation/devicetree/bindings/arm/rockchip.txt
> > index cc4ace6..ce1f04a 100644
> > --- a/Documentation/devicetree/bindings/arm/rockchip.txt
> > +++ b/Documentation/devicetree/bindings/arm/rockchip.txt
> > @@ -1,5 +1,8 @@
> > Rockchip platforms device tree bindings
> > ---------------------------------------
> > +- Asus Tinker board
> > + Required root node properties:
> > + - compatible = "rockchip,rk3288-tinker", "rockchip,rk3288";
>
> This should be asus,tinker if that's specific enough. At least the board
> manufacturer should be the vendor.
>
OK, i will modify it in next version
>
> >
> > - Kylin RK3036 board:
> > Required root node properties:
> > --
> > 2.7.4
> >
>
>
>
--
Eddie
[-- Attachment #2: Type: text/html, Size: 2073 bytes --]
^ permalink raw reply
* Re: [PATCH 6/6] dt-bindings: mt8173-mtu3: add reference clock
From: Chunfeng Yun @ 2017-01-22 1:49 UTC (permalink / raw)
To: Rob Herring
Cc: Mathias Nyman, Felipe Balbi, Greg Kroah-Hartman, Matthias Brugger,
Mark Rutland, Ian Campbell, linux-kernel, linux-arm-kernel,
linux-usb, linux-mediatek, devicetree
In-Reply-To: <20170121201155.cqcihopdc3kjyfi6@rob-hp-laptop>
Hi,
On Sat, 2017-01-21 at 14:11 -0600, Rob Herring wrote:
> On Wed, Jan 18, 2017 at 02:08:27PM +0800, Chunfeng Yun wrote:
> > add a reference clock for compatibility
>
> Why? This block suddenly has 2 clocks instead of 1?
In fact, there is a reference clock which comes from 26M oscillator
directly. I ignore it because it is a fixed-clock in DTS, and always
turned on for mt8173. But later, I find that I made a mistake before
when I bring up it on a new platform whose reference clock comes from
PLL, and need control it. So here add it, no matter it is a fixed-clock
or not.
>
> >
> > Signed-off-by: Chunfeng Yun <chunfeng.yun@mediatek.com>
> > ---
> > .../devicetree/bindings/usb/mt8173-mtu3.txt | 10 +++++-----
> > 1 file changed, 5 insertions(+), 5 deletions(-)
> >
> > diff --git a/Documentation/devicetree/bindings/usb/mt8173-mtu3.txt b/Documentation/devicetree/bindings/usb/mt8173-mtu3.txt
> > index e049d19..8c976cd 100644
> > --- a/Documentation/devicetree/bindings/usb/mt8173-mtu3.txt
> > +++ b/Documentation/devicetree/bindings/usb/mt8173-mtu3.txt
> > @@ -10,7 +10,7 @@ Required properties:
> > - vusb33-supply : regulator of USB avdd3.3v
> > - clocks : a list of phandle + clock-specifier pairs, one for each
> > entry in clock-names
> > - - clock-names : must contain "sys_ck" for clock of controller;
> > + - clock-names : must contain "sys_ck" and "ref_ck" for clock of controller;
> > "wakeup_deb_p0" and "wakeup_deb_p1" are optional, they are
> > depends on "mediatek,enable-wakeup"
> > - phys : a list of phandle + phy specifier pairs
> > @@ -56,10 +56,10 @@ ssusb: usb@11271000 {
> > phys = <&phy_port0 PHY_TYPE_USB3>,
> > <&phy_port1 PHY_TYPE_USB2>;
> > power-domains = <&scpsys MT8173_POWER_DOMAIN_USB>;
> > - clocks = <&topckgen CLK_TOP_USB30_SEL>,
> > + clocks = <&topckgen CLK_TOP_USB30_SEL>, <&clk26m>,
> > <&pericfg CLK_PERI_USB0>,
> > <&pericfg CLK_PERI_USB1>;
> > - clock-names = "sys_ck",
> > + clock-names = "sys_ck", "ref_ck",
> > "wakeup_deb_p0",
> > "wakeup_deb_p1";
> > vusb33-supply = <&mt6397_vusb_reg>;
> > @@ -79,8 +79,8 @@ ssusb: usb@11271000 {
> > reg-names = "mac";
> > interrupts = <GIC_SPI 115 IRQ_TYPE_LEVEL_LOW>;
> > power-domains = <&scpsys MT8173_POWER_DOMAIN_USB>;
> > - clocks = <&topckgen CLK_TOP_USB30_SEL>;
> > - clock-names = "sys_ck";
> > + clocks = <&topckgen CLK_TOP_USB30_SEL>, <&clk26m>;
> > + clock-names = "sys_ck", "ref_ck";
> > vusb33-supply = <&mt6397_vusb_reg>;
> > status = "disabled";
> > };
> > --
> > 1.7.9.5
> >
^ permalink raw reply
* Re: [PATCH 5/6] dt-bindings: mt8173-xhci: add reference clock
From: Chunfeng Yun @ 2017-01-22 1:53 UTC (permalink / raw)
To: Rob Herring
Cc: Mark Rutland, devicetree, Mathias Nyman, Felipe Balbi,
Ian Campbell, Greg Kroah-Hartman, linux-usb, linux-kernel,
linux-mediatek, Matthias Brugger, linux-arm-kernel
In-Reply-To: <20170121201250.dpowjr3mlwuew3gx@rob-hp-laptop>
On Sat, 2017-01-21 at 14:12 -0600, Rob Herring wrote:
> On Wed, Jan 18, 2017 at 02:08:26PM +0800, Chunfeng Yun wrote:
> > add a reference clock for compatibility
>
> Same question here, too.
The reason is the same as mt8173-mtu3
thanks a lot
>
^ permalink raw reply
* RE: [PATCH v2 04/12] driver: clk: imx: Add clock driver for imx6sll
From: Jacky Bai @ 2017-01-22 2:14 UTC (permalink / raw)
To: Stephen Boyd
Cc: mark.rutland@arm.com, devicetree@vger.kernel.org,
p.zabel@pengutronix.de, mturquette@baylibre.com,
daniel.lezcano@linaro.org, linux-clk@vger.kernel.org,
linux-gpio@vger.kernel.org, robh+dt@kernel.org,
kernel@pengutronix.de, jacky.baip@gmail.com, Fabio Estevam,
tglx@linutronix.de, shawnguo@kernel.org, linus.walleij@linaro.org,
linux-arm-kernel@lists.infradead.org
In-Reply-To: <20170121010009.GC8801@codeaurora.org>
> > > > The reason why we enable these two clks here is in below commit
> > > > commit
> > > > a5120e89e7e187a91852896f586876c7a2030804
> > > > Author: Peter Chen <peter.chen@freescale.com>
> > > > Date: Fri Jan 18 10:38:05 2013 +0800
> > > > ARM i.MX6: change mxs usbphy clock usage
> > > >
> > >
> > > So can we mark these clks with CLK_IS_CRITICAL flag then instead?
> > > Or are they disabled out of the bootloader?
> > >
> >
> > Sure, using 'CLK_IS_CRITICAL' should be ok for clks_init_on clocks.
> > But for USBPHY*_GATE, it is only enabled when CONIG_USB_MXC_PHY is
> > true. And another concern is if we need to add CLK_IS_CRITICAL flag to
> clks_init_on clocks, we may need to add new wrapper function to register
> these critical clock. It is not very good.
>
> Ok so set the critical flag when CONFIG_USB_MXC_PHY is enabled?
> That isn't too hard to do.
>
Ok, thanks, I will address this in V3.
> >
> --
> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux
> Foundation Collaborative Project
^ permalink raw reply
* Re: [PATCH v11 2/8] power: add power sequence library
From: Peter Chen @ 2017-01-22 2:14 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: Rafael J. Wysocki, Peter Chen, Mark Rutland, Ulf Hansson,
Heiko Stuebner, Stephen Boyd, Linux Kernel Mailing List,
Gary Bisson, Fabio Estevam, Joshua Clayton, Arnd Bergmann,
Dmitry Eremin-Solenikov, Vaibhav Hiremath, mka, Alan Stern,
devicetree@vger.kernel.org, mail, Pawel Moll, Linux PM, Sascha
In-Reply-To: <CAJZ5v0h09A2XWLEk7v9XPe+4fP7HDr6AqZzBmvBi_V5nhLoiRw@mail.gmail.com>
On Fri, Jan 20, 2017 at 11:21:27AM +0100, Rafael J. Wysocki wrote:
> On Fri, Jan 20, 2017 at 8:52 AM, Peter Chen <hzpeterchen@gmail.com> wrote:
> > On Tue, Jan 10, 2017 at 03:02:41PM +0800, Peter Chen wrote:
> >> On Sat, Jan 07, 2017 at 10:54:56AM +0200, Krzysztof Kozlowski wrote:
> >> > On Thu, Jan 05, 2017 at 02:01:53PM +0800, Peter Chen wrote:
> >> > > We have an well-known problem that the device needs to do some power
> >> > > sequence before it can be recognized by related host, the typical
> >> > > example like hard-wired mmc devices and usb devices.
> >> > >
> >> > > This power sequence is hard to be described at device tree and handled by
> >> > > related host driver, so we have created a common power sequence
> >> > > library to cover this requirement. The core code has supplied
> >> > > some common helpers for host driver, and individual power sequence
> >> > > libraries handle kinds of power sequence for devices. The pwrseq
> >> > > librares always need to allocate extra instance for compatible
> >> > > string match.
> >> > >
> >> > > pwrseq_generic is intended for general purpose of power sequence, which
> >> > > handles gpios and clocks currently, and can cover other controls in
> >> > > future. The host driver just needs to call of_pwrseq_on/of_pwrseq_off
> >> > > if only one power sequence is needed, else call of_pwrseq_on_list
> >> > > /of_pwrseq_off_list instead (eg, USB hub driver).
> >> > >
> >> > > For new power sequence library, it can add its compatible string
> >> > > to pwrseq_of_match_table, then the pwrseq core will match it with
> >> > > DT's, and choose this library at runtime.
> >> > >
> >> > > Signed-off-by: Peter Chen <peter.chen@nxp.com>
> >> > > Tested-by: Maciej S. Szmigiero <mail@maciej.szmigiero.name>
> >> > > Tested-by Joshua Clayton <stillcompiling@gmail.com>
> >> > > Reviewed-by: Matthias Kaehlcke <mka@chromium.org>
> >> > > Tested-by: Matthias Kaehlcke <mka@chromium.org>
> >> >
> >> > Acked-by: Krzysztof Kozlowski <krzk@kernel.org>
> >> > Tested on Odroid U3 (reset sequence for LAN9730):
> >> > Tested-by: Krzysztof Kozlowski <krzk@kernel.org>
> >> >
> >>
> >> A nice ping...
> >>
> >
> > Rafael, would you please review it? This series was discussed about
> > half a year, and many people need it, I hope it can be in v4.11-rc1,
> > thanks.
>
> I'm travelling now
> (http://marc.info/?l=linux-pm&m=148410629024194&w=2) and (as stated in
> this message) I'll get to the patches when I'm back home.
>
> There is a good chance for your code to go into 4.11-rc1 if the review
> comments so far have been addressed.
>
Thanks, Rafael. I think I have addressed all your comments.
--
Best Regards,
Peter Chen
^ permalink raw reply
* Re: [PATCH v1 2/2] arm: dts: mt2701: add nor flash node
From: Guochun Mao @ 2017-01-22 2:36 UTC (permalink / raw)
To: Rob Herring, Boris Brezillon, Thomas Petazzoni, Marek Vasut,
Matthias Brugger
Cc: linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org,
Mark Rutland, devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
Richard Weinberger, Russell King,
linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
linux-mtd-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org,
linux-mediatek-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r, Cyrille Pitchen,
Brian Norris, David Woodhouse
In-Reply-To: <CAL_Jsq+S-FYy4SmjJ3fWzBugw2zCj5TQkkfUA8QVL7SLJW3E1g-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
Hi,
On Thu, 2017-01-19 at 08:18 -0600, Rob Herring wrote:
> On Thu, Jan 19, 2017 at 2:14 AM, Boris Brezillon
> > One last question and I'm done: is something like that acceptable?
> >
> > compatible = "<vendor>,<old-soc>","<vendor>,<new-soc>";
> >
> > This can happen when someone adds support for an unsupported feature
> > on a brand new SoC, and then someone else use the same driver for an
> > older SoC embedding the same IP but still wants to add a new compatible
> > just in case these 2 IPs appear to be slightly different.
>
> Yes, it's old and new compatible strings in this case and it's newest
> compatible string first.
>
> > Here the order of compat strings is no longer following a clear rule
> > like 'most-specific compatible first' or 'newest IP/SoC version first',
> > it's completely dependent on the order these IPs were supported in the
> > OS (Linux). I'm perfectly fine with that BTW, just want to make sure
> > this is authorized.
>
> I guess we should say "newest compatible for IP first" instead. There
> are some exceptions where we add fallbacks later on, but that falls
> under the most-specific part.
>
> It's order that the bindings are defined, not Linux support really,
> but in practice those are the same.
>
> Rob
Thanks for all your effort for code reviewing.
Our mt2701-nor's hardware is designed base on mt8713-nor,
even so, there would be some slight difference.
If I don't misunderstand your viewpoint in this discussion,
there's no need to drop mt2701-nor compatible.
And if not, is there any other suggestion?
Thanks.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [RESEND PATCH 6/6] dt-bindings: phy-mt65xx-usb: add support for mt2712 platform
From: Chunfeng Yun @ 2017-01-22 2:50 UTC (permalink / raw)
To: Rob Herring
Cc: Kishon Vijay Abraham I, Matthias Brugger, Felipe Balbi,
Mark Rutland, Ian Campbell, linux-kernel, linux-arm-kernel,
linux-usb, linux-mediatek, devicetree
In-Reply-To: <20170121200844.k5ivauhws43ac2s2@rob-hp-laptop>
On Sat, 2017-01-21 at 14:08 -0600, Rob Herring wrote:
> On Wed, Jan 18, 2017 at 02:00:14PM +0800, Chunfeng Yun wrote:
> > add a new compatible string for "mt2712", and a new reference clock
> > for SuperSpeed analog phy;
> >
> > Signed-off-by: Chunfeng Yun <chunfeng.yun@mediatek.com>
> > ---
> > .../devicetree/bindings/phy/phy-mt65xx-usb.txt | 81 +++++++++++++++++---
> > 1 file changed, 70 insertions(+), 11 deletions(-)
> >
> > diff --git a/Documentation/devicetree/bindings/phy/phy-mt65xx-usb.txt b/Documentation/devicetree/bindings/phy/phy-mt65xx-usb.txt
> > index 33a2b1e..8f91136 100644
> > --- a/Documentation/devicetree/bindings/phy/phy-mt65xx-usb.txt
> > +++ b/Documentation/devicetree/bindings/phy/phy-mt65xx-usb.txt
> > @@ -6,19 +6,25 @@ This binding describes a usb3.0 phy for mt65xx platforms of Medaitek SoC.
> > Required properties (controller (parent) node):
> > - compatible : should be one of
> > "mediatek,mt2701-u3phy"
> > + "mediatek,mt2712-u3phy"
> > "mediatek,mt8173-u3phy"
> > - - reg : offset and length of register for phy, exclude port's
> > - register.
> > - clocks : a list of phandle + clock-specifier pairs, one for each
> > entry in clock-names
> > - clock-names : must contain
> > - "u3phya_ref": for reference clock of usb3.0 analog phy.
> > + "u2ref_clk": 48M reference clock of HighSpeed analog phy.
> > + "u3ref_clk": 26M reference clock of SuperSpeed analog phy,
> > + sometimes is 24M, 25M or 27M, depended on platform.
>
> _clk is redundant.
remove it
>
> >
> > Required nodes : a sub-node is required for each port the controller
> > provides. Address range information including the usual
> > 'reg' property is used inside these nodes to describe
> > the controller's topology.
> >
> > +Optional properties (controller (parent) node):
> > + - reg : offset and length of register shared by multiple ports,
> > + exclude port's private register. It is needed on mt2701
> > + and mt8173, but not on mt2712.
> > +
> > Required properties (port (child) node):
> > - reg : address and length of the register set for the port.
> > - #phy-cells : should be 1 (See second example)
> > @@ -31,21 +37,27 @@ Example:
> > u3phy: usb-phy@11290000 {
> > compatible = "mediatek,mt8173-u3phy";
> > reg = <0 0x11290000 0 0x800>;
> > - clocks = <&apmixedsys CLK_APMIXED_REF2USB_TX>;
> > - clock-names = "u3phya_ref";
> > + clocks = <&apmixedsys CLK_APMIXED_REF2USB_TX>, <&clk26m>;
> > + clock-names = "u2ref_clk", "u3ref_clk";
> > #address-cells = <2>;
> > #size-cells = <2>;
> > ranges;
> > status = "okay";
> >
> > - phy_port0: port@11290800 {
> > - reg = <0 0x11290800 0 0x800>;
> > + u2port0: port@11290800 {
>
> port is for OF graph. This should be usb-phy@... instead.
Is there any problems if u2port0's name@addr is the same as its parent's
(u3phy)? as following:
u3phy: usb-phy@11290000 {
compatible = ...;
// no reg property
clocks = ...;
u2port0: usb-phy@11290000 {
reg = ...;
}
>
> > + reg = <0 0x11290800 0 0x100>;
> > + #phy-cells = <1>;
> > + status = "okay";
> > + };
> > +
> > + u3port0: port@11290900 {
> > + reg = <0 0x11290800 0 0x700>;
> > #phy-cells = <1>;
> > status = "okay";
> > };
> >
> > + ...
Thank you!
^ permalink raw reply
* Re: [PATCH v3 3/3] media: rc: add driver for IR remote receiver on MT7623 SoC
From: kbuild test robot @ 2017-01-22 5:20 UTC (permalink / raw)
Cc: kbuild-all-JC7UmRfGjtg, mchehab-JPH+aEBZ4P+UEJcrhfAQsw,
hdegoede-H+wXaHxf7aLQT0dZR+AlfA,
hkallweit1-Re5JQEeQqe8AvxtiuMwx3w, robh+dt-DgEjT+Ai2ygdnm+yROfE0A,
mark.rutland-5wv7dgnIgG8, matthias.bgg-Re5JQEeQqe8AvxtiuMwx3w,
andi.shyti-Sze3O3UU22JBDgjK7y7TUQ,
hverkuil-qWit8jRvyhVmR6Xm/wNWPw, sean-hENCXIMQXOg,
ivo.g.dimitrov.75-Re5JQEeQqe8AvxtiuMwx3w,
linux-media-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-mediatek-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
keyhaede-Re5JQEeQqe8AvxtiuMwx3w, Sean Wang
In-Reply-To: <1484292939-9454-4-git-send-email-sean.wang-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org>
[-- Attachment #1: Type: text/plain, Size: 1916 bytes --]
Hi Sean,
[auto build test ERROR on robh/for-next]
[also build test ERROR on v4.10-rc4 next-20170120]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/sean-wang-mediatek-com/Documentation-devicetree-move-shared-property-used-by-rc-into-a-common-place/20170115-052257
base: https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git for-next
config: sparc64-allyesconfig (attached as .config)
compiler: sparc64-linux-gnu-gcc (Debian 6.1.1-9) 6.1.1 20160705
reproduce:
wget https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
make.cross ARCH=sparc64
All errors (new ones prefixed by >>):
drivers/media/rc/mtk-cir.c: In function 'mtk_ir_probe':
>> drivers/media/rc/mtk-cir.c:220:11: error: too many arguments to function 'devm_rc_allocate_device'
ir->rc = devm_rc_allocate_device(dev, RC_DRIVER_IR_RAW);
^~~~~~~~~~~~~~~~~~~~~~~
In file included from drivers/media/rc/mtk-cir.c:22:0:
include/media/rc-core.h:213:16: note: declared here
struct rc_dev *devm_rc_allocate_device(struct device *dev);
^~~~~~~~~~~~~~~~~~~~~~~
vim +/devm_rc_allocate_device +220 drivers/media/rc/mtk-cir.c
214 ir->base = devm_ioremap_resource(dev, res);
215 if (IS_ERR(ir->base)) {
216 dev_err(dev, "failed to map registers\n");
217 return PTR_ERR(ir->base);
218 }
219
> 220 ir->rc = devm_rc_allocate_device(dev, RC_DRIVER_IR_RAW);
221 if (!ir->rc) {
222 dev_err(dev, "failed to allocate device\n");
223 return -ENOMEM;
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 48879 bytes --]
^ permalink raw reply
* [PATCH v4 0/8] power: bq27xxx: add support for NVRAM R/W access
From: Matt Ranostay @ 2017-01-22 7:13 UTC (permalink / raw)
To: linux-pm, devicetree; +Cc: sre, tony, Matt Ranostay
Changes from v1:
* add documentation for mWh and mAh property units
* change devicetree entries to match new property units
Changes from v2:
* split i2c changes into respective patches
* add documentation for battery information for fuel gauge
* rebased documentation patches on change on the list
* abstracted the battery configuration for the state machine
to an generic struct and platform data access function
Changes from v3:
* add "fixed-battery" compatible field to be be more consistant
with devicetree
Matt Ranostay (8):
devicetree: property-units: add mWh and mAh units
devicetree: power: add battery state machine documentation
power: power_supply: add battery information struct
power: power_supply: add battery info platform data retrieval
power: bq27xxx_battery: add BQ27425 chip id
power: bq27xxx_battery: add i2c bulk read/write functions
devicetree: power: bq27xxx: add monitored battery documentation
power: bq27xxx_battery: add initial state machine support
.../devicetree/bindings/power/supply/battery.txt | 24 ++
.../devicetree/bindings/power/supply/bq27xxx.txt | 8 +
.../devicetree/bindings/property-units.txt | 2 +
drivers/power/supply/bq27xxx_battery.c | 253 ++++++++++++++++++++-
drivers/power/supply/bq27xxx_battery_i2c.c | 64 +++++-
drivers/power/supply/power_supply_core.c | 41 ++++
include/linux/power/bq27xxx_battery.h | 7 +-
include/linux/power_supply.h | 16 ++
8 files changed, 411 insertions(+), 4 deletions(-)
create mode 100644 Documentation/devicetree/bindings/power/supply/battery.txt
--
2.10.2
^ permalink raw reply
* [PATCH v4 1/8] devicetree: property-units: add mWh and mAh units
From: Matt Ranostay @ 2017-01-22 7:13 UTC (permalink / raw)
To: linux-pm, devicetree; +Cc: sre, tony, Matt Ranostay
In-Reply-To: <20170122071404.9654-1-matt@ranostay.consulting>
Add entries for microwatt-hours and microamp-hours to property
units.
Signed-off-by: Matt Ranostay <matt@ranostay.consulting>
---
Documentation/devicetree/bindings/property-units.txt | 2 ++
1 file changed, 2 insertions(+)
diff --git a/Documentation/devicetree/bindings/property-units.txt b/Documentation/devicetree/bindings/property-units.txt
index 12278d79f6c0..5e8d220cc2b6 100644
--- a/Documentation/devicetree/bindings/property-units.txt
+++ b/Documentation/devicetree/bindings/property-units.txt
@@ -25,8 +25,10 @@ Distance
Electricity
----------------------------------------
-microamp : micro amps
+-microamp-hours : micro amp hours
-ohms : Ohms
-micro-ohms : micro Ohms
+-microwatt-hours: micro Watt hours
-microvolt : micro volts
Temperature
--
2.10.2
^ permalink raw reply related
* [PATCH v4 2/8] devicetree: power: add battery state machine documentation
From: Matt Ranostay @ 2017-01-22 7:13 UTC (permalink / raw)
To: linux-pm, devicetree; +Cc: sre, tony, Matt Ranostay, Rob Herring
In-Reply-To: <20170122071404.9654-1-matt@ranostay.consulting>
Documentation on battery properties that can be defined for
fine tuning fuel gauge state machines.
Cc: Rob Herring <robh@kernel.org>
Cc: devicetree@vger.kernel.org
Signed-off-by: Matt Ranostay <matt@ranostay.consulting>
---
.../devicetree/bindings/power/supply/battery.txt | 24 ++++++++++++++++++++++
1 file changed, 24 insertions(+)
create mode 100644 Documentation/devicetree/bindings/power/supply/battery.txt
diff --git a/Documentation/devicetree/bindings/power/supply/battery.txt b/Documentation/devicetree/bindings/power/supply/battery.txt
new file mode 100644
index 000000000000..398b4d622883
--- /dev/null
+++ b/Documentation/devicetree/bindings/power/supply/battery.txt
@@ -0,0 +1,24 @@
+Battery State Machine Support
+
+Required Properties:
+ - compatible: Must be "fixed-battery"
+
+Optional Properties:
+ - nominal-microvolt: dead battery voltage in microvolts
+ - design-microwatt-hours: battery design mWh in microwatts
+ - design-microamp-hours: battery design mAh in microamps
+
+Example:
+
+ bat: battery@0 {
+ compatible = "fixed-battery";
+ nominal-microvolt = <3700000>;
+ design-microwatt-hours = <5290000>;
+ design-microamp-hours = <1430000>;
+ };
+
+ fuel_gauge: fuel_gauge@0 {
+ ....
+ monitored-battery = <&bat>;
+ ...
+ };
--
2.10.2
^ permalink raw reply related
* [PATCH v4 3/8] power: power_supply: add battery information struct
From: Matt Ranostay @ 2017-01-22 7:13 UTC (permalink / raw)
To: linux-pm-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA
Cc: sre-DgEjT+Ai2ygdnm+yROfE0A, tony-4v6yS6AI5VpBDgjK7y7TUQ,
Matt Ranostay
In-Reply-To: <20170122071404.9654-1-matt-sk+viVC6FLCDq+mSdOJa79kegs52MxvZ@public.gmane.org>
Add power_supply_battery_info structure that is used to
enumerate battery state machine settings.
Signed-off-by: Matt Ranostay <matt-sk+viVC6FLCDq+mSdOJa79kegs52MxvZ@public.gmane.org>
---
include/linux/power_supply.h | 12 ++++++++++++
1 file changed, 12 insertions(+)
diff --git a/include/linux/power_supply.h b/include/linux/power_supply.h
index 3965503315ef..59674a257d5e 100644
--- a/include/linux/power_supply.h
+++ b/include/linux/power_supply.h
@@ -288,6 +288,18 @@ struct power_supply_info {
int use_for_apm;
};
+/*
+ * This is the recommended struct to specify static battery parameters.
+ * Power supply class itself doesn't use this, but it implements access
+ * that most platform drivers should use for consistency.
+ */
+
+struct power_supply_battery_info {
+ int energy; /* uWh */
+ int power; /* uAh */
+ int nominal_voltage; /* uV */
+};
+
extern struct atomic_notifier_head power_supply_notifier;
extern int power_supply_reg_notifier(struct notifier_block *nb);
extern void power_supply_unreg_notifier(struct notifier_block *nb);
--
2.10.2
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply related
* [PATCH v4 4/8] power: power_supply: add battery info platform data retrieval
From: Matt Ranostay @ 2017-01-22 7:14 UTC (permalink / raw)
To: linux-pm-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA
Cc: sre-DgEjT+Ai2ygdnm+yROfE0A, tony-4v6yS6AI5VpBDgjK7y7TUQ,
Matt Ranostay
In-Reply-To: <20170122071404.9654-1-matt-sk+viVC6FLCDq+mSdOJa79kegs52MxvZ@public.gmane.org>
Add power_supply_get_battery_info() to get battery platform data
Signed-off-by: Matt Ranostay <matt-sk+viVC6FLCDq+mSdOJa79kegs52MxvZ@public.gmane.org>
---
drivers/power/supply/power_supply_core.c | 41 ++++++++++++++++++++++++++++++++
include/linux/power_supply.h | 4 ++++
2 files changed, 45 insertions(+)
diff --git a/drivers/power/supply/power_supply_core.c b/drivers/power/supply/power_supply_core.c
index 1e0960b646e8..21490b5e233f 100644
--- a/drivers/power/supply/power_supply_core.c
+++ b/drivers/power/supply/power_supply_core.c
@@ -17,6 +17,7 @@
#include <linux/device.h>
#include <linux/notifier.h>
#include <linux/err.h>
+#include <linux/of.h>
#include <linux/power_supply.h>
#include <linux/thermal.h>
#include "power_supply.h"
@@ -487,6 +488,46 @@ struct power_supply *devm_power_supply_get_by_phandle(struct device *dev,
EXPORT_SYMBOL_GPL(devm_power_supply_get_by_phandle);
#endif /* CONFIG_OF */
+int power_supply_get_battery_info(struct power_supply *psy,
+ struct power_supply_battery_info *info,
+ const char *property)
+{
+ struct device_node *np = psy->of_node;
+ struct device_node *power_supply_battery_info_np;
+ struct property *p;
+ char *prefix = "fixed-battery";
+ int ret;
+
+ if (!np)
+ return -ENXIO;
+
+ power_supply_battery_info_np = of_parse_phandle(np, property, 0);
+ if (!power_supply_battery_info_np)
+ return -ENODEV;
+
+ p = of_find_property(power_supply_battery_info_np, "compatible", NULL);
+
+ if (!p || !p->value)
+ return -ENODEV;
+
+ if (strncmp(prefix, p->value, strlen(prefix)))
+ return -ENODEV;
+
+ ret = of_property_read_u32(power_supply_battery_info_np,
+ "nominal-microvolt", &info->nominal_voltage);
+ if (ret < 0)
+ return ret;
+
+ ret = of_property_read_u32(power_supply_battery_info_np,
+ "design-microwatt-hours", &info->energy);
+ if (ret < 0)
+ return ret;
+
+ return of_property_read_u32(power_supply_battery_info_np,
+ "design-microamp-hours", &info->power);
+}
+EXPORT_SYMBOL_GPL(power_supply_get_battery_info);
+
int power_supply_get_property(struct power_supply *psy,
enum power_supply_property psp,
union power_supply_propval *val)
diff --git a/include/linux/power_supply.h b/include/linux/power_supply.h
index 59674a257d5e..859d0e7f9b6a 100644
--- a/include/linux/power_supply.h
+++ b/include/linux/power_supply.h
@@ -318,6 +318,10 @@ static inline struct power_supply *
devm_power_supply_get_by_phandle(struct device *dev, const char *property)
{ return NULL; }
#endif /* CONFIG_OF */
+
+extern int power_supply_get_battery_info(struct power_supply *psy,
+ struct power_supply_battery_info *info,
+ const char *property);
extern void power_supply_changed(struct power_supply *psy);
extern int power_supply_am_i_supplied(struct power_supply *psy);
extern int power_supply_set_battery_charged(struct power_supply *psy);
--
2.10.2
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply related
* [PATCH v4 5/8] power: bq27xxx_battery: add BQ27425 chip id
From: Matt Ranostay @ 2017-01-22 7:14 UTC (permalink / raw)
To: linux-pm-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA
Cc: sre-DgEjT+Ai2ygdnm+yROfE0A, tony-4v6yS6AI5VpBDgjK7y7TUQ,
Matt Ranostay
In-Reply-To: <20170122071404.9654-1-matt-sk+viVC6FLCDq+mSdOJa79kegs52MxvZ@public.gmane.org>
Signed-off-by: Matt Ranostay <matt-sk+viVC6FLCDq+mSdOJa79kegs52MxvZ@public.gmane.org>
---
drivers/power/supply/bq27xxx_battery.c | 23 ++++++++++++++++++++++-
drivers/power/supply/bq27xxx_battery_i2c.c | 2 +-
include/linux/power/bq27xxx_battery.h | 3 ++-
3 files changed, 25 insertions(+), 3 deletions(-)
diff --git a/drivers/power/supply/bq27xxx_battery.c b/drivers/power/supply/bq27xxx_battery.c
index 398801a21b86..12ecea308186 100644
--- a/drivers/power/supply/bq27xxx_battery.c
+++ b/drivers/power/supply/bq27xxx_battery.c
@@ -417,6 +417,25 @@ static u8 bq27xxx_regs[][BQ27XXX_REG_MAX] = {
[BQ27XXX_REG_DCAP] = 0x3c,
[BQ27XXX_REG_AP] = 0x18,
},
+ [BQ27425] = {
+ [BQ27XXX_REG_CTRL] = 0x00,
+ [BQ27XXX_REG_TEMP] = 0x02,
+ [BQ27XXX_REG_INT_TEMP] = 0x1e,
+ [BQ27XXX_REG_VOLT] = 0x04,
+ [BQ27XXX_REG_AI] = 0x10,
+ [BQ27XXX_REG_FLAGS] = 0x06,
+ [BQ27XXX_REG_TTE] = INVALID_REG_ADDR,
+ [BQ27XXX_REG_TTF] = INVALID_REG_ADDR,
+ [BQ27XXX_REG_TTES] = INVALID_REG_ADDR,
+ [BQ27XXX_REG_TTECP] = INVALID_REG_ADDR,
+ [BQ27XXX_REG_NAC] = 0x08,
+ [BQ27XXX_REG_FCC] = 0x0e,
+ [BQ27XXX_REG_CYCT] = INVALID_REG_ADDR,
+ [BQ27XXX_REG_AE] = INVALID_REG_ADDR,
+ [BQ27XXX_REG_SOC] = 0x1c,
+ [BQ27XXX_REG_DCAP] = 0x3c,
+ [BQ27XXX_REG_AP] = 0x18,
+ },
};
static enum power_supply_property bq27000_battery_props[] = {
@@ -752,6 +771,7 @@ static struct {
BQ27XXX_PROP(BQ27541, bq27541_battery_props),
BQ27XXX_PROP(BQ27545, bq27545_battery_props),
BQ27XXX_PROP(BQ27421, bq27421_battery_props),
+ BQ27XXX_PROP(BQ27425, bq27421_battery_props),
};
static DEFINE_MUTEX(bq27xxx_list_lock);
@@ -1015,6 +1035,7 @@ static bool bq27xxx_battery_overtemp(struct bq27xxx_device_info *di, u16 flags)
return flags & (BQ27XXX_FLAG_OTC | BQ27XXX_FLAG_OTD);
case BQ27530:
case BQ27421:
+ case BQ27425:
return flags & BQ27XXX_FLAG_OT;
default:
return false;
@@ -1026,7 +1047,7 @@ static bool bq27xxx_battery_overtemp(struct bq27xxx_device_info *di, u16 flags)
*/
static bool bq27xxx_battery_undertemp(struct bq27xxx_device_info *di, u16 flags)
{
- if (di->chip == BQ27530 || di->chip == BQ27421)
+ if (di->chip == BQ27530 || di->chip == BQ27421 || di->chip == BQ27425)
return flags & BQ27XXX_FLAG_UT;
return false;
diff --git a/drivers/power/supply/bq27xxx_battery_i2c.c b/drivers/power/supply/bq27xxx_battery_i2c.c
index c68fbc3fe50a..2ea2d0b06948 100644
--- a/drivers/power/supply/bq27xxx_battery_i2c.c
+++ b/drivers/power/supply/bq27xxx_battery_i2c.c
@@ -167,9 +167,9 @@ static const struct i2c_device_id bq27xxx_i2c_id_table[] = {
{ "bq27742", BQ27541 },
{ "bq27545", BQ27545 },
{ "bq27421", BQ27421 },
- { "bq27425", BQ27421 },
{ "bq27441", BQ27421 },
{ "bq27621", BQ27421 },
+ { "bq27425", BQ27425 },
{},
};
MODULE_DEVICE_TABLE(i2c, bq27xxx_i2c_id_table);
diff --git a/include/linux/power/bq27xxx_battery.h b/include/linux/power/bq27xxx_battery.h
index b312bcef53da..3f265dbf11af 100644
--- a/include/linux/power/bq27xxx_battery.h
+++ b/include/linux/power/bq27xxx_battery.h
@@ -17,7 +17,8 @@ enum bq27xxx_chip {
BQ27530, /* bq27530, bq27531 */
BQ27541, /* bq27541, bq27542, bq27546, bq27742 */
BQ27545, /* bq27545 */
- BQ27421, /* bq27421, bq27425, bq27441, bq27621 */
+ BQ27421, /* bq27421, bq27441, bq27621 */
+ BQ27425, /* bq27425 */
};
/**
--
2.10.2
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply related
* [PATCH v4 6/8] power: bq27xxx_battery: add i2c bulk read/write functions
From: Matt Ranostay @ 2017-01-22 7:14 UTC (permalink / raw)
To: linux-pm, devicetree; +Cc: sre, tony, Matt Ranostay
In-Reply-To: <20170122071404.9654-1-matt@ranostay.consulting>
Signed-off-by: Matt Ranostay <matt@ranostay.consulting>
---
drivers/power/supply/bq27xxx_battery_i2c.c | 62 ++++++++++++++++++++++++++++++
include/linux/power/bq27xxx_battery.h | 4 ++
2 files changed, 66 insertions(+)
diff --git a/drivers/power/supply/bq27xxx_battery_i2c.c b/drivers/power/supply/bq27xxx_battery_i2c.c
index 2ea2d0b06948..feeb04ed88c8 100644
--- a/drivers/power/supply/bq27xxx_battery_i2c.c
+++ b/drivers/power/supply/bq27xxx_battery_i2c.c
@@ -68,6 +68,64 @@ static int bq27xxx_battery_i2c_read(struct bq27xxx_device_info *di, u8 reg,
return ret;
}
+static int bq27xxx_battery_i2c_write(struct bq27xxx_device_info *di, u8 reg,
+ int value, bool single)
+{
+ struct i2c_client *client = to_i2c_client(di->dev);
+ struct i2c_msg msg;
+ unsigned char data[4];
+
+ if (!client->adapter)
+ return -ENODEV;
+
+ data[0] = reg;
+ if (single) {
+ data[1] = (unsigned char) value;
+ msg.len = 2;
+ } else {
+ put_unaligned_le16(value, &data[1]);
+ msg.len = 3;
+ }
+
+ msg.buf = data;
+ msg.addr = client->addr;
+ msg.flags = 0;
+
+ return i2c_transfer(client->adapter, &msg, 1) == 1 ? 0 : -EINVAL;
+}
+
+static int bq27xxx_battery_i2c_bulk_read(struct bq27xxx_device_info *di, u8 reg,
+ u8 *data, int len)
+{
+ struct i2c_client *client = to_i2c_client(di->dev);
+
+ if (!client->adapter)
+ return -ENODEV;
+
+ return i2c_smbus_read_i2c_block_data(client, reg, len, data);
+}
+
+static int bq27xxx_battery_i2c_bulk_write(struct bq27xxx_device_info *di,
+ u8 reg, u8 *data, int len)
+{
+ struct i2c_client *client = to_i2c_client(di->dev);
+ struct i2c_msg msg;
+ u8 buf[33];
+
+ if (!client->adapter)
+ return -ENODEV;
+
+ buf[0] = reg;
+ memcpy(&buf[1], data, len);
+
+ msg.buf = buf;
+ msg.addr = client->addr;
+ msg.flags = 0;
+ msg.len = len + 1;
+
+ return i2c_transfer(client->adapter, &msg, 1) == 1 ? 0 : -EINVAL;
+}
+
static int bq27xxx_battery_i2c_probe(struct i2c_client *client,
const struct i2c_device_id *id)
{
@@ -95,7 +153,11 @@ static int bq27xxx_battery_i2c_probe(struct i2c_client *client,
di->dev = &client->dev;
di->chip = id->driver_data;
di->name = name;
+
di->bus.read = bq27xxx_battery_i2c_read;
+ di->bus.write = bq27xxx_battery_i2c_write;
+ di->bus.read_bulk = bq27xxx_battery_i2c_bulk_read;
+ di->bus.write_bulk = bq27xxx_battery_i2c_bulk_write;
ret = bq27xxx_battery_setup(di);
if (ret)
diff --git a/include/linux/power/bq27xxx_battery.h b/include/linux/power/bq27xxx_battery.h
index 3f265dbf11af..581402380d6e 100644
--- a/include/linux/power/bq27xxx_battery.h
+++ b/include/linux/power/bq27xxx_battery.h
@@ -41,6 +41,9 @@ struct bq27xxx_platform_data {
struct bq27xxx_device_info;
struct bq27xxx_access_methods {
int (*read)(struct bq27xxx_device_info *di, u8 reg, bool single);
+ int (*write)(struct bq27xxx_device_info *di, u8 reg, int value, bool single);
+ int (*read_bulk)(struct bq27xxx_device_info *di, u8 reg, u8 *data, int len);
+ int (*write_bulk)(struct bq27xxx_device_info *di, u8 reg, u8 *data, int len);
};
struct bq27xxx_reg_cache {
@@ -71,6 +74,7 @@ struct bq27xxx_device_info {
struct list_head list;
struct mutex lock;
u8 *regs;
+ u8 buffer[32];
};
void bq27xxx_battery_update(struct bq27xxx_device_info *di);
--
2.10.2
^ permalink raw reply related
* [PATCH v4 7/8] devicetree: power: bq27xxx: add monitored battery documentation
From: Matt Ranostay @ 2017-01-22 7:14 UTC (permalink / raw)
To: linux-pm-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA
Cc: sre-DgEjT+Ai2ygdnm+yROfE0A, tony-4v6yS6AI5VpBDgjK7y7TUQ,
Matt Ranostay, Rob Herring
In-Reply-To: <20170122071404.9654-1-matt-sk+viVC6FLCDq+mSdOJa79kegs52MxvZ@public.gmane.org>
Depends-On: http://marc.info/?l=linux-pm&m=148392292830015&w=2
Cc: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
Cc: devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
Signed-off-by: Matt Ranostay <matt-sk+viVC6FLCDq+mSdOJa79kegs52MxvZ@public.gmane.org>
---
Documentation/devicetree/bindings/power/supply/bq27xxx.txt | 8 ++++++++
1 file changed, 8 insertions(+)
diff --git a/Documentation/devicetree/bindings/power/supply/bq27xxx.txt b/Documentation/devicetree/bindings/power/supply/bq27xxx.txt
index b0c95ef63e68..0472a2db0f13 100644
--- a/Documentation/devicetree/bindings/power/supply/bq27xxx.txt
+++ b/Documentation/devicetree/bindings/power/supply/bq27xxx.txt
@@ -28,9 +28,17 @@ Required properties:
* "ti,bq27621" - BQ27621
- reg: integer, i2c address of the device.
+Optional properties:
+- monitored-battery: phandle of battery information devicetree node
+
+ See Documentation/devicetree/bindings/power/supply/battery.txt
+ If any of the referenced battery properties are set, then all must be.
+
Example:
bq27510g3 {
compatible = "ti,bq27510g3";
reg = <0x55>;
+
+ monitored-battery = <&bat>;
};
--
2.10.2
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply related
* [PATCH v4 8/8] power: bq27xxx_battery: add initial state machine support
From: Matt Ranostay @ 2017-01-22 7:14 UTC (permalink / raw)
To: linux-pm, devicetree; +Cc: sre, tony, Matt Ranostay
In-Reply-To: <20170122071404.9654-1-matt@ranostay.consulting>
Signed-off-by: Matt Ranostay <matt@ranostay.consulting>
---
drivers/power/supply/bq27xxx_battery.c | 230 ++++++++++++++++++++++++++++++++-
1 file changed, 229 insertions(+), 1 deletion(-)
diff --git a/drivers/power/supply/bq27xxx_battery.c b/drivers/power/supply/bq27xxx_battery.c
index 12ecea308186..5eb608535bb1 100644
--- a/drivers/power/supply/bq27xxx_battery.c
+++ b/drivers/power/supply/bq27xxx_battery.c
@@ -43,6 +43,7 @@
* http://www.ti.com/product/bq27621-g1
*/
+#include <linux/delay.h>
#include <linux/device.h>
#include <linux/module.h>
#include <linux/mutex.h>
@@ -777,6 +778,46 @@ static struct {
static DEFINE_MUTEX(bq27xxx_list_lock);
static LIST_HEAD(bq27xxx_battery_devices);
+#define BQ27XXX_TERM_V_MIN 2800
+#define BQ27XXX_TERM_V_MAX 3700
+
+#define BQ27XXX_BLOCK_DATA_CLASS 0x3E
+#define BQ27XXX_DATA_BLOCK 0x3F
+#define BQ27XXX_BLOCK_DATA 0x40
+#define BQ27XXX_BLOCK_DATA_CHECKSUM 0x60
+#define BQ27XXX_BLOCK_DATA_CONTROL 0x61
+#define BQ27XXX_SET_CFGUPDATE 0x13
+#define BQ27XXX_SOFT_RESET 0x42
+
+enum bq27xxx_dm_subclass_index {
+ BQ27XXX_DM_DESIGN_CAP = 0,
+ BQ27XXX_DM_DESIGN_ENERGY,
+ BQ27XXX_DM_TERMINATE_VOLTAGE,
+ BQ27XXX_NUM_IDX,
+};
+
+struct bq27xxx_dm_regs {
+ unsigned int subclass_id;
+ unsigned int offset;
+ char *name;
+};
+
+#define BQ27XXX_GAS_GAUGING_STATE_SUBCLASS 82
+
+static struct bq27xxx_dm_regs bq27425_dm_subclass_regs[] = {
+ { BQ27XXX_GAS_GAUGING_STATE_SUBCLASS, 12, "design-capacity" },
+ { BQ27XXX_GAS_GAUGING_STATE_SUBCLASS, 14, "design-energy" },
+ { BQ27XXX_GAS_GAUGING_STATE_SUBCLASS, 18, "terminate-voltage" },
+};
+
+static struct bq27xxx_dm_regs *bq27xxx_dm_subclass_regs[] = {
+ [BQ27425] = bq27425_dm_subclass_regs,
+};
+
+static unsigned int bq27xxx_unseal_keys[] = {
+ [BQ27425] = 0x04143672,
+};
+
static int poll_interval_param_set(const char *val, const struct kernel_param *kp)
{
struct bq27xxx_device_info *di;
@@ -821,6 +862,165 @@ static inline int bq27xxx_read(struct bq27xxx_device_info *di, int reg_index,
return di->bus.read(di, di->regs[reg_index], single);
}
+static int bq27xxx_battery_set_seal_state(struct bq27xxx_device_info *di,
+ bool state)
+{
+ unsigned int key = bq27xxx_unseal_keys[di->chip];
+ int ret;
+
+ if (state)
+ return di->bus.write(di, BQ27XXX_REG_CTRL, 0x20, false);
+
+ ret = di->bus.write(di, BQ27XXX_REG_CTRL, (key >> 16) & 0xffff, false);
+ if (ret < 0)
+ return ret;
+
+ return di->bus.write(di, BQ27XXX_REG_CTRL, key & 0xffff, false);
+}
+
+static int bq27xxx_battery_read_dm_block(struct bq27xxx_device_info *di,
+ int subclass)
+{
+ int ret = di->bus.write(di, BQ27XXX_REG_CTRL, 0, false);
+
+ if (ret < 0)
+ return ret;
+
+ ret = di->bus.write(di, BQ27XXX_BLOCK_DATA_CONTROL, 0, true);
+ if (ret < 0)
+ return ret;
+
+ ret = di->bus.write(di, BQ27XXX_BLOCK_DATA_CLASS, subclass, true);
+ if (ret < 0)
+ return ret;
+
+ ret = di->bus.write(di, BQ27XXX_DATA_BLOCK, 0, true);
+ if (ret < 0)
+ return ret;
+
+ usleep_range(1000, 1500);
+
+ return di->bus.read_bulk(di, BQ27XXX_BLOCK_DATA,
+ (u8 *) &di->buffer, sizeof(di->buffer));
+}
+
+static int bq27xxx_battery_print_config(struct bq27xxx_device_info *di)
+{
+ struct bq27xxx_dm_regs *reg = bq27xxx_dm_subclass_regs[di->chip];
+ int ret, i;
+
+ ret = bq27xxx_battery_read_dm_block(di,
+ BQ27XXX_GAS_GAUGING_STATE_SUBCLASS);
+ if (ret < 0)
+ return ret;
+
+ for (i = 0; i < BQ27XXX_NUM_IDX; i++) {
+ int val;
+
+ if (reg->subclass_id != BQ27XXX_GAS_GAUGING_STATE_SUBCLASS)
+ continue;
+
+ val = be16_to_cpup((u16 *) &di->buffer[reg->offset]);
+
+ dev_info(di->dev, "settings for %s set at %d\n", reg->name, val);
+
+ reg++;
+ }
+
+ return 0;
+}
+
+static bool bq27xxx_battery_update_dm_setting(struct bq27xxx_device_info *di,
+ unsigned int reg, unsigned int val)
+{
+ struct bq27xxx_dm_regs *dm_reg = &bq27xxx_dm_subclass_regs[di->chip][reg];
+ u16 *prev = (u16 *) &di->buffer[dm_reg->offset];
+
+ if (be16_to_cpup(prev) == val)
+ return false;
+
+ *prev = cpu_to_be16(val);
+
+ return true;
+}
+
+static u8 bq27xxx_battery_checksum(struct bq27xxx_device_info *di)
+{
+ u8 *data = (u8 *) &di->buffer;
+ u16 sum = 0;
+ int i;
+
+ for (i = 0; i < sizeof(di->buffer); i++) {
+ sum += data[i];
+ sum &= 0xff;
+ }
+
+ return 0xff - sum;
+}
+
+static int bq27xxx_battery_write_nvram(struct bq27xxx_device_info *di,
+ unsigned int subclass)
+{
+ int ret;
+
+ ret = di->bus.write(di, BQ27XXX_REG_CTRL, BQ27XXX_SET_CFGUPDATE, false);
+ if (ret)
+ return ret;
+
+ ret = di->bus.write(di, BQ27XXX_BLOCK_DATA_CONTROL, 0, true);
+ if (ret)
+ return ret;
+
+ ret = di->bus.write(di, BQ27XXX_BLOCK_DATA_CLASS, subclass, true);
+ if (ret)
+ return ret;
+
+ ret = di->bus.write(di, BQ27XXX_DATA_BLOCK, 0, true);
+ if (ret)
+ return ret;
+
+ ret = di->bus.write_bulk(di, BQ27XXX_BLOCK_DATA,
+ (u8 *) &di->buffer, sizeof(di->buffer));
+ if (ret < 0)
+ return ret;
+
+ usleep_range(1000, 1500);
+
+ di->bus.write(di, BQ27XXX_BLOCK_DATA_CHECKSUM,
+ bq27xxx_battery_checksum(di), true);
+
+ usleep_range(1000, 1500);
+
+ di->bus.write(di, BQ27XXX_REG_CTRL, BQ27XXX_SOFT_RESET, false);
+
+ return 0;
+}
+
+static int bq27xxx_battery_set_config(struct bq27xxx_device_info *di,
+ struct power_supply_battery_info *info)
+{
+ int ret = bq27xxx_battery_read_dm_block(di,
+ BQ27XXX_GAS_GAUGING_STATE_SUBCLASS);
+
+ if (ret < 0)
+ return ret;
+
+ ret = bq27xxx_battery_update_dm_setting(di, BQ27XXX_DM_DESIGN_CAP,
+ info->power / 1000);
+ ret |= bq27xxx_battery_update_dm_setting(di, BQ27XXX_DM_DESIGN_ENERGY,
+ info->energy / 1000);
+ ret |= bq27xxx_battery_update_dm_setting(di, BQ27XXX_DM_TERMINATE_VOLTAGE,
+ info->nominal_voltage / 1000);
+
+ if (ret) {
+ dev_info(di->dev, "updating NVM settings\n");
+ return bq27xxx_battery_write_nvram(di,
+ BQ27XXX_GAS_GAUGING_STATE_SUBCLASS);
+ }
+
+ return 0;
+}
+
/*
* Return the battery State-of-Charge
* Or < 0 if something fails.
@@ -1090,6 +1290,30 @@ static int bq27xxx_battery_read_health(struct bq27xxx_device_info *di)
return POWER_SUPPLY_HEALTH_GOOD;
}
+void bq27xxx_battery_settings(struct bq27xxx_device_info *di)
+{
+ struct power_supply_battery_info info = {};
+
+ /* functions don't exist for writing data so abort */
+ if (!di->bus.write || !di->bus.write_bulk)
+ return;
+
+ /* no settings to be set for this chipset so abort */
+ if (!bq27xxx_dm_subclass_regs[di->chip])
+ return;
+
+ bq27xxx_battery_set_seal_state(di, false);
+
+ if (power_supply_get_battery_info(di->bat, &info, "monitored-battery") < 0)
+ goto out;
+
+ bq27xxx_battery_set_config(di, &info);
+
+out:
+ bq27xxx_battery_print_config(di);
+ bq27xxx_battery_set_seal_state(di, true);
+}
+
void bq27xxx_battery_update(struct bq27xxx_device_info *di)
{
struct bq27xxx_reg_cache cache = {0, };
@@ -1372,7 +1596,10 @@ static void bq27xxx_external_power_changed(struct power_supply *psy)
int bq27xxx_battery_setup(struct bq27xxx_device_info *di)
{
struct power_supply_desc *psy_desc;
- struct power_supply_config psy_cfg = { .drv_data = di, };
+ struct power_supply_config psy_cfg;
+
+ psy_cfg.of_node = di->dev->of_node;
+ psy_cfg.drv_data = di;
INIT_DELAYED_WORK(&di->work, bq27xxx_battery_poll);
mutex_init(&di->lock);
@@ -1397,6 +1624,7 @@ int bq27xxx_battery_setup(struct bq27xxx_device_info *di)
dev_info(di->dev, "support ver. %s enabled\n", DRIVER_VERSION);
+ bq27xxx_battery_settings(di);
bq27xxx_battery_update(di);
mutex_lock(&bq27xxx_list_lock);
--
2.10.2
^ permalink raw reply related
* Re: [PATCH v3 3/3] watchdog: zx2967: add watchdog controller driver for ZTE's zx2967 family
From: Baoyou Xie @ 2017-01-22 7:50 UTC (permalink / raw)
To: Guenter Roeck
Cc: Jun Nie, wim, Rob Herring, mark.rutland, linux-arm-kernel,
linux-watchdog, devicetree, Linux Kernel Mailing List, Shawn Guo,
xie.baoyou, chen.chaokai, wang.qiang01
In-Reply-To: <cdb27525-bcf2-9ad5-08b2-d454c8d51e10@roeck-us.net>
[-- Attachment #1: Type: text/plain, Size: 17861 bytes --]
On 22 January 2017 at 07:06, Guenter Roeck <linux@roeck-us.net> wrote:
> On 01/20/2017 05:03 AM, Baoyou Xie wrote:
>
>> This patch adds watchdog controller driver for ZTE's zx2967 family.
>>
>> Signed-off-by: Baoyou Xie <baoyou.xie@linaro.org>
>> ---
>> drivers/watchdog/Kconfig | 10 ++
>> drivers/watchdog/Makefile | 1 +
>> drivers/watchdog/zx2967_wdt.c | 376 ++++++++++++++++++++++++++++++
>> ++++++++++++
>> 3 files changed, 387 insertions(+)
>> create mode 100644 drivers/watchdog/zx2967_wdt.c
>>
>> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
>> index acb00b5..05093a2 100644
>> --- a/drivers/watchdog/Kconfig
>> +++ b/drivers/watchdog/Kconfig
>> @@ -714,6 +714,16 @@ config ASPEED_WATCHDOG
>> To compile this driver as a module, choose M here: the
>> module will be called aspeed_wdt.
>>
>> +config ZX2967_WATCHDOG
>> + tristate "ZTE zx2967 SoCs watchdog support"
>> + depends on ARCH_ZX
>> + select WATCHDOG_CORE
>> + help
>> + Say Y here to include support for the watchdog timer
>> + in ZTE zx2967 SoCs.
>> + To compile this driver as a module, choose M here: the
>> + module will be called zx2967_wdt.
>> +
>> # AVR32 Architecture
>>
>> config AT32AP700X_WDT
>> diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
>> index 0c3d35e..bf2d296 100644
>> --- a/drivers/watchdog/Makefile
>> +++ b/drivers/watchdog/Makefile
>> @@ -82,6 +82,7 @@ obj-$(CONFIG_BCM7038_WDT) += bcm7038_wdt.o
>> obj-$(CONFIG_ATLAS7_WATCHDOG) += atlas7_wdt.o
>> obj-$(CONFIG_RENESAS_WDT) += renesas_wdt.o
>> obj-$(CONFIG_ASPEED_WATCHDOG) += aspeed_wdt.o
>> +obj-$(CONFIG_ZX2967_WATCHDOG) += zx2967_wdt.o
>>
>> # AVR32 Architecture
>> obj-$(CONFIG_AT32AP700X_WDT) += at32ap700x_wdt.o
>> diff --git a/drivers/watchdog/zx2967_wdt.c b/drivers/watchdog/zx2967_wdt.
>> c
>> new file mode 100644
>> index 0000000..a5656d0
>> --- /dev/null
>> +++ b/drivers/watchdog/zx2967_wdt.c
>> @@ -0,0 +1,376 @@
>> +/*
>> + * watchdog driver for ZTE's zx2967 family
>> + *
>> + * Copyright (C) 2017 ZTE Ltd.
>> + *
>> + * Author: Baoyou Xie <baoyou.xie@linaro.org>
>> + *
>> + * License terms: GNU General Public License (GPL) version 2
>> + */
>> +
>> +#include <linux/clk.h>
>> +#include <linux/delay.h>
>> +#include <linux/io.h>
>> +#include <linux/mfd/syscon.h>
>> +#include <linux/module.h>
>> +#include <linux/of_address.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/reboot.h>
>> +#include <linux/regmap.h>
>> +#include <linux/reset.h>
>> +#include <linux/watchdog.h>
>> +
>> +#define ZX2967_WDT_CFG_REG 0x4
>> +#define ZX2967_WDT_LOAD_REG 0x8
>> +#define ZX2967_WDT_REFRESH_REG 0x18
>> +#define ZX2967_WDT_START_REG 0x1c
>> +
>> +#define ZX2967_WDT_REFRESH_MASK 0x3f
>> +
>> +#define ZX2967_WDT_CFG_DIV(n) ((((n) & 0xff) - 1) << 8)
>> +#define ZX2967_WDT_START_EN 0x1
>> +
>> +#define ZX2967_WDT_WRITEKEY 0x12340000
>> +
>> +#define ZX2967_WDT_DIV_DEFAULT 16
>> +#define ZX2967_WDT_DEFAULT_TIMEOUT 32
>> +#define ZX2967_WDT_MIN_TIMEOUT 1
>> +#define ZX2967_WDT_MAX_TIMEOUT 500
>>
>
> Is that based on a real limit or an arbitrary value ?
>
> It's based on a real limit. In fact, the real limit is 524.
> +#define ZX2967_WDT_MAX_COUNT 0xffff
>> +
>> +#define ZX2967_WDT_FLAG_REBOOT_MON (1 << 0)
>>
>
> BIT ?
>
>
> +
>> +struct zx2967_wdt {
>> + struct device *dev;
>> + struct clk *clock;
>> + void __iomem *reg_base;
>> + unsigned int conf;
>> + unsigned int load;
>> + unsigned int flags;
>> + struct watchdog_device wdt_device;
>> + struct notifier_block restart_handler;
>> + struct notifier_block reboot_handler;
>> +};
>> +
>> +static inline u32 zx2967_wdt_readl(struct zx2967_wdt *wdt, u16 reg)
>> +{
>> + return readl_relaxed(wdt->reg_base + reg);
>> +}
>> +
>> +static inline void zx2967_wdt_writel(struct zx2967_wdt *wdt, u16 reg,
>> u32 val)
>> +{
>> + writel_relaxed(val | ZX2967_WDT_WRITEKEY, wdt->reg_base + reg);
>> +}
>> +
>> +static void zx2967_wdt_refresh(struct zx2967_wdt *wdt)
>> +{
>> + u32 val;
>> +
>> + val = zx2967_wdt_readl(wdt, ZX2967_WDT_REFRESH_REG);
>> + val ^= ZX2967_WDT_REFRESH_MASK;
>> + zx2967_wdt_writel(wdt, ZX2967_WDT_REFRESH_REG, val);
>> +}
>> +
>> +static unsigned int
>> +__zx2967_wdt_set_timeout(struct zx2967_wdt *wdt, unsigned int timeout)
>> +{
>> + unsigned int freq = clk_get_rate(wdt->clock);
>>
>
> The clock frequency is set to 32 kHz. It seems unnecessary to re-read it
> whenever the timeout changes. Also, ...
>
> + unsigned int divisor = ZX2967_WDT_DIV_DEFAULT;
>> + unsigned int count;
>> +
>> + count = timeout * freq;
>> + if (count > divisor * ZX2967_WDT_MAX_COUNT)
>> + divisor = DIV_ROUND_UP(count, ZX2967_WDT_MAX_COUNT);
>> + count = DIV_ROUND_UP(count, divisor);
>> + zx2967_wdt_writel(wdt, ZX2967_WDT_CFG_REG,
>> ZX2967_WDT_CFG_DIV(divisor));
>> + zx2967_wdt_writel(wdt, ZX2967_WDT_LOAD_REG, count);
>> + zx2967_wdt_refresh(wdt);
>> + wdt->load = count;
>> +
>> + return (count * divisor) / freq;
>>
>
> ... if you think it can change from underneath you, you'll also need to
> make sure
> it is not 0, to avoid a nasty surprise here. Of course, if it does change,
> you'll
> have no idea what the actual timeout is at any given time, and the driver
> won't work.
>
> +}
>> +
>> +static int zx2967_wdt_set_timeout(struct watchdog_device *wdd,
>> + unsigned int timeout)
>> +{
>> + struct zx2967_wdt *wdt = watchdog_get_drvdata(wdd);
>> +
>> + if (watchdog_timeout_invalid(&wdt->wdt_device, timeout)) {
>> + dev_err(wdt->dev, "timeout %d is invalid\n", timeout);
>> + return -EINVAL;
>> + }
>>
>
> This function is called from the infrastructure. Let's trust the
> infrastructure
> to check the valid range before calling this code.
>
> +
>> + wdd->timeout = __zx2967_wdt_set_timeout(wdt, timeout);
>> +
>> + return 0;
>> +}
>> +
>> +static void __zx2967_wdt_start(struct zx2967_wdt *wdt)
>> +{
>> + u32 val;
>> +
>> + val = zx2967_wdt_readl(wdt, ZX2967_WDT_START_REG);
>> + val |= ZX2967_WDT_START_EN;
>> + zx2967_wdt_writel(wdt, ZX2967_WDT_START_REG, val);
>> +}
>> +
>> +static void __zx2967_wdt_stop(struct zx2967_wdt *wdt)
>> +{
>> + u32 val;
>> +
>> + val = zx2967_wdt_readl(wdt, ZX2967_WDT_START_REG);
>> + val &= ~ZX2967_WDT_START_EN;
>> + zx2967_wdt_writel(wdt, ZX2967_WDT_START_REG, val);
>> +}
>> +
>> +static int zx2967_wdt_start(struct watchdog_device *wdd)
>> +{
>> + struct zx2967_wdt *wdt = watchdog_get_drvdata(wdd);
>> +
>> + __zx2967_wdt_stop(wdt);
>> + zx2967_wdt_set_timeout(wdd, wdd->timeout);
>>
>
> This seems inconsistent. First, the watchdog should not already be
> started when this function is called. Second, if it is in fact necessary
> to stop the watchdog before updating its timeout, you might want to
> consider stopping it in the set_timeout function, because that
> function _will_ be called if the timeout is updated while the
> watchdog is running.
>
>
> + __zx2967_wdt_start(wdt);
>> +
>> + return 0;
>> +}
>> +
>> +static int zx2967_wdt_stop(struct watchdog_device *wdd)
>> +{
>> + struct zx2967_wdt *wdt = watchdog_get_drvdata(wdd);
>> +
>> + __zx2967_wdt_stop(wdt);
>> +
>> + return 0;
>> +}
>> +
>> +static int zx2967_wdt_keepalive(struct watchdog_device *wdd)
>> +{
>> + struct zx2967_wdt *wdt = watchdog_get_drvdata(wdd);
>> +
>> + zx2967_wdt_refresh(wdt);
>> +
>> + return 0;
>> +}
>> +
>> +#define ZX2967_WDT_OPTIONS \
>> + (WDIOF_SETTIMEOUT | WDIOF_KEEPALIVEPING | WDIOF_MAGICCLOSE)
>> +static const struct watchdog_info zx2967_wdt_ident = {
>> + .options = ZX2967_WDT_OPTIONS,
>> + .firmware_version = 0,
>> + .identity = "zx2967 watchdog",
>> +};
>> +
>> +static struct watchdog_ops zx2967_wdt_ops = {
>> + .owner = THIS_MODULE,
>> + .start = zx2967_wdt_start,
>> + .stop = zx2967_wdt_stop,
>> + .ping = zx2967_wdt_keepalive,
>> + .set_timeout = zx2967_wdt_set_timeout,
>> +};
>> +
>> +static void zx2967_wdt_fix_sysdown(struct zx2967_wdt *wdt)
>> +{
>> + __zx2967_wdt_stop(wdt);
>> + __zx2967_wdt_set_timeout(wdt, 15);
>> + __zx2967_wdt_start(wdt);
>> +}
>>
>
> I am really not at all in favor of this code. It force-sets a watchdog
> to 15 seconds later, if it was enabled or not.
>
> I don't necessarily oppose the idea in general, but it would have to be
> configurable and part of the infrastructure.
>
> I agree you all. In fact, these codes implement the policy witch is part
of products, and don't necessarily place them here.
>
> +
>> +static int zx2967_wdt_notify_sys(struct notifier_block *this,
>> + unsigned long code, void *unused)
>> +{
>> + struct zx2967_wdt *wdt = container_of(this, struct zx2967_wdt,
>> + reboot_handler);
>> +
>> + wdt->flags |= ZX2967_WDT_FLAG_REBOOT_MON;
>> + switch (code) {
>> + case SYS_HALT:
>> + case SYS_POWER_OFF:
>> + case SYS_RESTART:
>> + zx2967_wdt_fix_sysdown(wdt);
>> + break;
>> + default:
>> + break;
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +static int zx2967_wdt_restart(struct notifier_block *this,
>> + unsigned long mode, void *cmd)
>> +{
>> + struct zx2967_wdt *wdt;
>> +
>> + wdt = container_of(this, struct zx2967_wdt, restart_handler);
>> +
>> + zx2967_wdt_stop(&wdt->wdt_device);
>> +
>> + zx2967_wdt_writel(wdt, ZX2967_WDT_LOAD_REG, 0x80);
>> + zx2967_wdt_refresh(wdt);
>> + zx2967_wdt_writel(wdt, ZX2967_WDT_START_REG, ZX2967_WDT_START_EN);
>> +
>> + zx2967_wdt_start(&wdt->wdt_device);
>> + /* wait for reset*/
>> + mdelay(500);
>> +
>> + return NOTIFY_DONE;
>> +}
>> +
>> +static void zx2967_wdt_reset_sysctrl(struct device *dev)
>> +{
>> + int ret;
>> + struct device_node *np = NULL;
>> + void __iomem *regmap;
>> + unsigned int offset, mask, config;
>> + struct of_phandle_args out_args;
>> +
>> + ret = of_parse_phandle_with_fixed_args(dev->of_node,
>> + "zte,wdt-reset-sysctrl", 3, 0, &out_args);
>> + if (ret) {
>> + dev_info(dev, "failed to parse zte,wdt-reset-sysctrl");
>>
>
> Why this message ? The property is optional. There is no "failure".
>
> Repeating the information in the devicetree description:
>
> zte,wdt-reset-sysctrl : Directs how to reset system by the
> watchdog.
>
> Given the context, and the provided implementation, I can only assume that
> this is supposed to mean which action shall be taken when the watchdog
> triggers,
> and that the bit mask provided is supposed to configure that action. If so,
> that should be explained in the devicetree description, and not be hidden
> in magic register values.
>
> The devicetree description is ok.
> + return;
>> + }
>> + offset = out_args.args[0];
>> + config = out_args.args[1];
>> + mask = out_args.args[2];
>> +
>> + regmap = syscon_node_to_regmap(out_args.np);
>> + if (IS_ERR(regmap))
>> + goto out;
>> +
>> + regmap_update_bits(regmap, offset, mask, config);
>>
>
>
> I don't really see the value of the local variables.
>
> Are you sure? I add the local variables in V3.
> +out:
>> + of_node_put(np);
>>
>
> I don't really see where np is set to anything but NULL.
>
> +}
>> +
>> +static int zx2967_wdt_probe(struct platform_device *pdev)
>> +{
>> + struct device *dev;
>> + struct zx2967_wdt *wdt;
>> + struct resource *base;
>> + int ret = 0;
>>
>
> Unnecessary initialization.
>
> +
>> + struct reset_control *rstc;
>>
>
> No empty lines between variable declarations, please.
>
> +
>> + dev = &pdev->dev;
>>
>
> Can be initialized above.
>
> +
>> + wdt = devm_kzalloc(dev, sizeof(*wdt), GFP_KERNEL);
>> + if (!wdt)
>> + return -ENOMEM;
>> +
>> + platform_set_drvdata(pdev, wdt);
>> +
>> + wdt->dev = dev;
>>
>
> This is only used by an error message in the set_timeout function,
> which is unnecessary.
>
> + wdt->wdt_device.info = &zx2967_wdt_ident;
>> + wdt->wdt_device.ops = &zx2967_wdt_ops;
>> + wdt->wdt_device.timeout = ZX2967_WDT_DEFAULT_TIMEOUT;
>> + wdt->wdt_device.max_timeout = ZX2967_WDT_MAX_TIMEOUT;
>> + wdt->wdt_device.min_timeout = ZX2967_WDT_MIN_TIMEOUT;
>> + wdt->wdt_device.parent = &pdev->dev;
>> +
>> + base = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> + wdt->reg_base = devm_ioremap_resource(dev, base);
>> + if (IS_ERR(wdt->reg_base)) {
>> + dev_err(dev, "ioremap failed\n");
>> + return PTR_ERR(wdt->reg_base);
>> + }
>> +
>> + zx2967_wdt_reset_sysctrl(dev);
>> +
>> + wdt->reboot_handler.notifier_call = zx2967_wdt_notify_sys;
>> + register_reboot_notifier(&wdt->reboot_handler);
>>
>
> Without ever unregistering it ? Did you try to unload the driver and
> reboot ?
>
> + wdt->clock = devm_clk_get(dev, NULL);
>> + if (IS_ERR(wdt->clock)) {
>> + dev_err(dev, "failed to find watchdog clock source\n");
>> + return PTR_ERR(wdt->clock);
>> + }
>> +
>> + ret = clk_prepare_enable(wdt->clock);
>> + if (ret < 0) {
>> + dev_err(dev, "failed to enable clock\n");
>> + return ret;
>> + }
>> + clk_set_rate(wdt->clock, 32768);
>> +
>> + rstc = devm_reset_control_get(dev, NULL);
>> + if (IS_ERR(rstc)) {
>> + dev_err(dev, "failed to get rstc");
>> + ret = PTR_ERR(rstc);
>> + goto fail_get_reset_control;
>> + }
>> +
>> + reset_control_assert(rstc);
>> + mdelay(10);
>> + reset_control_deassert(rstc);
>>
>
> There is this reset, and the reset in reset_sysctrl above.
> Are they both necessary ?
>
> Here, we reset watchdog itself, but reset_sysctrl is used to reset system.
> + watchdog_set_drvdata(&wdt->wdt_device, wdt);
>> +
>> + watchdog_init_timeout(&wdt->wdt_device,
>> + ZX2967_WDT_DEFAULT_TIMEOUT, dev);
>>
>
> What is the purpose of this call ? It sets the timeout to the default
> timeout,
> which is already set, and it does not use the value from devicetree since
> the
> value passed is != 0.
>
> + watchdog_set_nowayout(&wdt->wdt_device, WATCHDOG_NOWAYOUT);
>> +
>> + zx2967_wdt_stop(&wdt->wdt_device);
>>
>
> The watchdog was reset twice above. Is this call really necessary ?
>
> +
>> + ret = watchdog_register_device(&wdt->wdt_device);
>> + if (ret)
>> + goto fail_register;
>> +
>> + wdt->restart_handler.notifier_call = zx2967_wdt_restart;
>> + wdt->restart_handler.priority = 128;
>> + ret = register_restart_handler(&wdt->restart_handler);
>> + if (ret) {
>> + dev_err(dev, "cannot register restart handler, %d\n",
>> ret);
>> + goto fail_restart;
>> + }
>> +
>>
> Why not use the infrastructure ?
>
> + dev_info(dev, "watchdog enabled (timeout=%d sec, nowayout=%d)",
>> + wdt->wdt_device.timeout, WATCHDOG_NOWAYOUT);
>> +
>> + return 0;
>> +
>> +fail_get_reset_control:
>> +fail_restart:
>>
>
> Please no double labels to the same code.
>
> + watchdog_unregister_device(&wdt->wdt_device);
>> +fail_register:
>> + clk_disable_unprepare(wdt->clock);
>> + return ret;
>> +}
>> +
>> +static int zx2967_wdt_remove(struct platform_device *pdev)
>> +{
>> + struct zx2967_wdt *wdt = platform_get_drvdata(pdev);
>> +
>> + unregister_restart_handler(&wdt->restart_handler);
>> + watchdog_unregister_device(&wdt->wdt_device);
>> + clk_disable_unprepare(wdt->clock);
>> +
>> + return 0;
>> +}
>> +
>> +static void zx2967_wdt_shutdown(struct platform_device *pdev)
>> +{
>> + struct zx2967_wdt *wdt = platform_get_drvdata(pdev);
>> +
>> + if (!(wdt->flags & ZX2967_WDT_FLAG_REBOOT_MON))
>> + zx2967_wdt_stop(&wdt->wdt_device);
>>
>
> As mentioned before, the whole reboot handling is highly unusual.
>
> I don't really like the idea of bypassing the infrastructure
> (such as watchdog_stop_on_reboot()) for driver specific special behavior.
> If such behavior is useful, it should be useful for all watchdog drivers,
> and be defined in the infrastructure.
>
>
> +}
>> +
>> +static const struct of_device_id zx2967_wdt_match[] = {
>> + { .compatible = "zte,zx296718-wdt", },
>> + {}
>> +};
>> +MODULE_DEVICE_TABLE(of, zx2967_wdt_match);
>> +
>> +static struct platform_driver zx2967_wdt_driver = {
>> + .probe = zx2967_wdt_probe,
>> + .remove = zx2967_wdt_remove,
>> + .shutdown = zx2967_wdt_shutdown,
>> + .driver = {
>> + .name = "zx2967-wdt",
>> + .of_match_table = of_match_ptr(zx2967_wdt_match),
>> + },
>> +};
>> +module_platform_driver(zx2967_wdt_driver);
>> +
>> +MODULE_AUTHOR("Baoyou Xie <baoyou.xie@linaro.org>");
>> +MODULE_DESCRIPTION("ZTE zx2967 Watchdog Device Driver");
>> +MODULE_LICENSE("GPL v2");
>>
>>
>
[-- Attachment #2: Type: text/html, Size: 26369 bytes --]
^ permalink raw reply
* Re: [PATCH v9 0/4] arm64: arch_timer: Add workaround for hisilicon-161010101 erratum
From: Hanjun Guo @ 2017-01-22 7:59 UTC (permalink / raw)
To: Mark Rutland, Ding Tianhong, catalin.marinas-5wv7dgnIgG8,
will.deacon-5wv7dgnIgG8, Daniel Lezcano, Thomas Gleixner
Cc: devicetree-u79uwXL29TY76Z2rM5mHXA, marc.zyngier-5wv7dgnIgG8,
stuart.yoder-3arQi8VN3Tc, linuxarm-hv44wF8Li93QT0dZR+AlfA,
oss-fOR+EgIDQEHk1uMJSBkQmQ, shawnguo-DgEjT+Ai2ygdnm+yROfE0A,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r, Fu Wei
In-Reply-To: <20170120150428.GC22152@leverpostej>
On 2017/1/20 23:04, Mark Rutland wrote:
> On Thu, Jan 19, 2017 at 09:35:12PM +0800, Ding Tianhong wrote:
>> Ding Tianhong (4):
>> arm64: arch_timer: Add device tree binding for hisilicon-161010101
>> erratum
>> arm64: arch_timer: Introduce a generic erratum handing mechanism for
>> fsl-a008585
>> arm64: arch_timer: Work around Erratum Hisilicon-161010101
>> arm64: arch timer: Add timer erratum property for Hip05-d02 and
>> Hip06-d03
>>
>> Documentation/admin-guide/kernel-parameters.txt | 9 --
>> Documentation/arm64/silicon-errata.txt | 43 +++---
>> .../devicetree/bindings/arm/arch_timer.txt | 6 +
>> arch/arm64/boot/dts/hisilicon/hip05.dtsi | 1 +
>> arch/arm64/boot/dts/hisilicon/hip06.dtsi | 1 +
>> arch/arm64/include/asm/arch_timer.h | 38 ++----
>> drivers/clocksource/Kconfig | 18 +++
>> drivers/clocksource/arm_arch_timer.c | 150 +++++++++++++++------
>> 8 files changed, 171 insertions(+), 95 deletions(-)
>
> I've picked these up (with a few local cleanups), given that some local
> testing, and pushed the result to a branch [1] on my git repo.
>
> There are likely to be clashes with the arm64 tree (e.g. for
> silicon-errata.txt), and we're also likely to have more arch timer
> updates shortly for the GTDT stuff,
Yes, GTDT patches conflict with this patch set but it's easy to
fix.
> so I think the best bet is for both
> arm64 and the clocksource tree to pull that branch for v4.11.
>
> Alternatively, we could take this all through the arm64 tree, if the
> clocksource maintainers are happy with that.
>
> Thoughts?
GTDT patch set is adding ACPI support for arch timer, and it's
only used for ARM64 now, in order to handler the conflict easily,
I think take them all through arm64 tree is better (I was working
with Fuwei for the GTDT patch set and I hope it's not blocked by
"who will merge the code"...)
Thanks
Hanjun
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* [PATCH v4 1/3] dt: bindings: add documentation for zx2967 family watchdog controller
From: Baoyou Xie @ 2017-01-22 8:16 UTC (permalink / raw)
To: jun.nie, wim, linux, robh+dt, mark.rutland
Cc: linux-arm-kernel, linux-watchdog, devicetree, linux-kernel,
shawnguo, baoyou.xie, xie.baoyou, chen.chaokai, wang.qiang01
This patch adds dt-binding documentation for zx2967 family
watchdog controller.
Signed-off-by: Baoyou Xie <baoyou.xie@linaro.org>
---
.../bindings/watchdog/zte,zx2967-wdt.txt | 31 ++++++++++++++++++++++
1 file changed, 31 insertions(+)
create mode 100644 Documentation/devicetree/bindings/watchdog/zte,zx2967-wdt.txt
diff --git a/Documentation/devicetree/bindings/watchdog/zte,zx2967-wdt.txt b/Documentation/devicetree/bindings/watchdog/zte,zx2967-wdt.txt
new file mode 100644
index 0000000..772403a
--- /dev/null
+++ b/Documentation/devicetree/bindings/watchdog/zte,zx2967-wdt.txt
@@ -0,0 +1,31 @@
+ZTE zx2967 Watchdog timer
+
+Required properties:
+
+- compatible : should be one of the following.
+ * zte,zx296718-wdt
+- reg : Specifies base physical address and size of the registers.
+- clocks : Pairs of phandle and specifier referencing the controller's clocks.
+- resets : Reference to the reset controller controlling the watchdog
+ controller.
+
+Optional properties:
+
+- zte,wdt-reset-sysctrl : Directs how to reset system by the watchdog.
+ if we don't want to restart system when watchdog been triggered,
+ it's not required, vice versa.
+ It should include following fields.
+ * phandle of aon-sysctrl.
+ * offset of register that be written, should be 0xb0.
+ * configure value that be written to aon-sysctrl.
+ * bit mask, corresponding bits will be affected.
+
+Example:
+
+wdt: watchdog@1465000 {
+ compatible = "zte,zx296718-wdt";
+ reg = <0x1465000 0x1000>;
+ clocks = <&topcrm WDT_WCLK>;
+ resets = <&toprst 35>;
+ zte,wdt-reset-sysctrl = <&aon_sysctrl 0xb0 1 0x115>;
+};
--
2.7.4
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox