* [PATCH v3 09/11] clk: sunxi-ng: add support for Allwinner A64 DE2 CCU
From: Icenowy Zheng @ 2017-12-22 12:22 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20171222122243.25735-1-icenowy@aosc.io>
Allwinner A64's DE2 needs to claim a section of SRAM (SRAM C) to work.
Add support for it.
Signed-off-by: Icenowy Zheng <icenowy@aosc.io>
---
drivers/clk/sunxi-ng/ccu-sun8i-de2.c | 32 ++++++++++++++++++++++++--------
1 file changed, 24 insertions(+), 8 deletions(-)
diff --git a/drivers/clk/sunxi-ng/ccu-sun8i-de2.c b/drivers/clk/sunxi-ng/ccu-sun8i-de2.c
index 468d1abaf0ee..38b029b7bb5a 100644
--- a/drivers/clk/sunxi-ng/ccu-sun8i-de2.c
+++ b/drivers/clk/sunxi-ng/ccu-sun8i-de2.c
@@ -17,6 +17,7 @@
#include <linux/of_platform.h>
#include <linux/platform_device.h>
#include <linux/reset.h>
+#include <linux/soc/sunxi/sunxi_sram.h>
#include "ccu_common.h"
#include "ccu_div.h"
@@ -196,6 +197,11 @@ static const struct sunxi_ccu_desc sun8i_v3s_de2_clk_desc = {
.num_resets = ARRAY_SIZE(sun8i_a83t_de2_resets),
};
+static bool sunxi_de2_clk_has_sram(const struct device_node *node)
+{
+ return of_device_is_compatible(node, "allwinner,sun50i-a64-de2-clk");
+}
+
static int sunxi_de2_clk_probe(struct platform_device *pdev)
{
struct resource *res;
@@ -239,11 +245,20 @@ static int sunxi_de2_clk_probe(struct platform_device *pdev)
return ret;
}
+ if (sunxi_de2_clk_has_sram(pdev->dev.of_node)) {
+ ret = sunxi_sram_claim(&pdev->dev);
+ if (ret) {
+ dev_err(&pdev->dev,
+ "Error couldn't map SRAM to device\n");
+ return ret;
+ }
+ }
+
/* The clocks need to be enabled for us to access the registers */
ret = clk_prepare_enable(bus_clk);
if (ret) {
dev_err(&pdev->dev, "Couldn't enable bus clk: %d\n", ret);
- return ret;
+ goto err_release_sram;
}
ret = clk_prepare_enable(mod_clk);
@@ -272,6 +287,10 @@ static int sunxi_de2_clk_probe(struct platform_device *pdev)
clk_disable_unprepare(mod_clk);
err_disable_bus_clk:
clk_disable_unprepare(bus_clk);
+err_release_sram:
+ if (sunxi_de2_clk_has_sram(pdev->dev.of_node))
+ sunxi_sram_release(&pdev->dev);
+
return ret;
}
@@ -288,17 +307,14 @@ static const struct of_device_id sunxi_de2_clk_ids[] = {
.compatible = "allwinner,sun8i-v3s-de2-clk",
.data = &sun8i_v3s_de2_clk_desc,
},
+ {
+ .compatible = "allwinner,sun50i-a64-de2-clk",
+ .data = &sun50i_a64_de2_clk_desc,
+ },
{
.compatible = "allwinner,sun50i-h5-de2-clk",
.data = &sun50i_a64_de2_clk_desc,
},
- /*
- * The Allwinner A64 SoC needs some bit to be poke in syscon to make
- * DE2 really working.
- * So there's currently no A64 compatible here.
- * H5 shares the same reset line with A64, so here H5 is using the
- * clock description of A64.
- */
{ }
};
--
2.14.2
^ permalink raw reply related
* [PATCH v3 10/11] arm64: allwinner: a64: add DE2 CCU for A64 SoC
From: Icenowy Zheng @ 2017-12-22 12:22 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20171222122243.25735-1-icenowy@aosc.io>
The A64 SoC features a DE2 CCU like the one in H5, but needs to claim a
section of SRAM (SRAM C) to be accessed.
Adds the device tree nodes for the SRAM controller and the DE2 CCU.
Signed-off-by: Icenowy Zheng <icenowy@aosc.io>
---
Changes in v3:
- Fixed the alliwnner,sram property (the 1 after SRAM phadle is missing
in v2).
arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi | 34 +++++++++++++++++++++++++++
1 file changed, 34 insertions(+)
diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi b/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
index d783d164b9c3..fb8ea7c414e1 100644
--- a/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
+++ b/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
@@ -130,6 +130,40 @@
#size-cells = <1>;
ranges;
+ display_clocks: clock at 1000000 {
+ compatible = "allwinner,sun50i-a64-de2-clk";
+ reg = <0x01000000 0x100000>;
+ clocks = <&ccu CLK_DE>,
+ <&ccu CLK_BUS_DE>;
+ clock-names = "mod",
+ "bus";
+ resets = <&ccu RST_BUS_DE>;
+ allwinner,sram = <&de2_sram 1>;
+ #clock-cells = <1>;
+ #reset-cells = <1>;
+ };
+
+ sram-controller at 1c00000 {
+ compatible = "allwinner,sun50i-a64-sram-controller";
+ reg = <0x01c00000 0x1000>;
+ #address-cells = <1>;
+ #size-cells = <1>;
+ ranges;
+
+ sram_c: sram at 18000 {
+ compatible = "mmio-sram";
+ reg = <0x00018000 0x28000>;
+ #address-cells = <1>;
+ #size-cells = <1>;
+ ranges = <0 0x00018000 0x28000>;
+
+ de2_sram: sram-section at 0 {
+ compatible = "allwinner,sun50i-a64-sram-c";
+ reg = <0x0000 0x28000>;
+ };
+ };
+ };
+
syscon: syscon at 1c00000 {
compatible = "allwinner,sun50i-a64-system-controller",
"syscon";
--
2.14.2
^ permalink raw reply related
* [PATCH v3 11/11] arm64: allwinner: a64: add simplefb for A64 SoC
From: Icenowy Zheng @ 2017-12-22 12:22 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20171222122243.25735-1-icenowy@aosc.io>
The A64 SoC features two display pipelines, one has a LCD output, the
other has a HDMI output.
Add support for simplefb for these pipelines on A64 SoC.
Signed-off-by: Icenowy Zheng <icenowy@aosc.io>
---
arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi | 31 +++++++++++++++++++++++++++
1 file changed, 31 insertions(+)
diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi b/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
index fb8ea7c414e1..993f5df73e8d 100644
--- a/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
+++ b/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
@@ -42,9 +42,11 @@
* OTHER DEALINGS IN THE SOFTWARE.
*/
+#include <dt-bindings/clock/sun8i-de2.h>
#include <dt-bindings/clock/sun50i-a64-ccu.h>
#include <dt-bindings/clock/sun8i-r-ccu.h>
#include <dt-bindings/interrupt-controller/arm-gic.h>
+#include <dt-bindings/reset/sun8i-de2.h>
#include <dt-bindings/reset/sun50i-a64-ccu.h>
/ {
@@ -52,6 +54,35 @@
#address-cells = <1>;
#size-cells = <1>;
+ chosen {
+ #address-cells = <1>;
+ #size-cells = <1>;
+ ranges;
+
+ framebuffer-lcd {
+ compatible = "allwinner,simple-framebuffer",
+ "simple-framebuffer";
+ allwinner,pipeline = "mixer0-lcd0";
+ clocks = <&display_clocks CLK_BUS_MIXER0>,
+ <&ccu CLK_BUS_TCON0>, <&ccu CLK_BUS_TCON0>,
+ <&display_clocks CLK_MIXER0>,
+ <&ccu CLK_TCON0>;
+ status = "disabled";
+ };
+
+ framebuffer-hdmi {
+ compatible = "allwinner,simple-framebuffer",
+ "simple-framebuffer";
+ allwinner,pipeline = "mixer1-lcd1-hdmi";
+ clocks = <&display_clocks CLK_BUS_MIXER1>,
+ <&ccu CLK_BUS_TCON1>, <&ccu CLK_BUS_HDMI>,
+ <&display_clocks CLK_MIXER1>,
+ <&ccu CLK_TCON1>, <&ccu CLK_HDMI>,
+ <&ccu CLK_HDMI_DDC>;
+ status = "disabled";
+ };
+ };
+
cpus {
#address-cells = <1>;
#size-cells = <0>;
--
2.14.2
^ permalink raw reply related
* [PATCH] ARM: make ARCH_S3C24XX select USE_OF and clean-up boot/dts/Makefile
From: Krzysztof Kozlowski @ 2017-12-22 12:41 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1511749163-11057-1-git-send-email-yamada.masahiro@socionext.com>
On Mon, Nov 27, 2017 at 3:19 AM, Masahiro Yamada
<yamada.masahiro@socionext.com> wrote:
> ARCH_S3C24XX is a very exceptional platform that some DT files in
> arch/arm/boot/dts/, but does not select USE_OF.
Not entirely. The platform does select USE_OF - when MACH_S3C2416_DT
is chosen. For other boards USE_OF is not necessary because they do
not use DT. Why you need to select it for entire arch?
Best regards,
Krzysztof
>
> All the other platforms with DT files correctly select USE_OF
> directly or indirectly (Most of them are either ARCH_MULTIPLATFORM
> or ARM_SINGLE_ARMV7M).
>
> With ARCH_S3C24XX fixed, "ifeq ($(CONFIG_OF),y)" in DT Makefile
> can be deleted.
>
> Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
> ---
>
> arch/arm/Kconfig | 1 +
> arch/arm/boot/dts/Makefile | 3 ---
> 2 files changed, 1 insertion(+), 3 deletions(-)
>
> diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
> index 51c8df5..5604497 100644
> --- a/arch/arm/Kconfig
> +++ b/arch/arm/Kconfig
> @@ -595,6 +595,7 @@ config ARCH_S3C24XX
> select MULTI_IRQ_HANDLER
> select NEED_MACH_IO_H
> select SAMSUNG_ATAGS
> + select USE_OF
> help
> Samsung S3C2410, S3C2412, S3C2413, S3C2416, S3C2440, S3C2442, S3C2443
> and S3C2450 SoCs based systems, such as the Simtec Electronics BAST
> diff --git a/arch/arm/boot/dts/Makefile b/arch/arm/boot/dts/Makefile
> index d0381e9..6f7f25d 100644
> --- a/arch/arm/boot/dts/Makefile
> +++ b/arch/arm/boot/dts/Makefile
> @@ -1,6 +1,4 @@
> # SPDX-License-Identifier: GPL-2.0
> -ifeq ($(CONFIG_OF),y)
> -
> dtb-$(CONFIG_ARCH_ALPINE) += \
> alpine-db.dtb
> dtb-$(CONFIG_MACH_ARTPEC6) += \
> @@ -1104,4 +1102,3 @@ dtb-$(CONFIG_ARCH_ZX) += zx296702-ad1.dtb
> dtb-$(CONFIG_ARCH_ASPEED) += aspeed-bmc-opp-palmetto.dtb \
> aspeed-bmc-opp-romulus.dtb \
> aspeed-ast2500-evb.dtb
> -endif
> --
> 2.7.4
>
> --
> To unsubscribe from this list: send the line "unsubscribe devicetree" in
> the body of a message to majordomo at vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* [PATCH v5 3/5] misc serdev: Add w2sg0004 (gps receiver) power control driver
From: Johan Hovold @ 2017-12-22 12:44 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <5494ad34b39a6c62601e3747440268dfb3be7d5a.1512114576.git.hns@goldelico.com>
On Fri, Dec 01, 2017 at 08:49:36AM +0100, H. Nikolaus Schaller wrote:
> Add driver for Wi2Wi W2SG0004/84 GPS module connected to some SoC UART.
>
> It uses serdev API hooks to monitor and forward the UART traffic to /dev/ttyGPSn
> and turn on/off the module. It also detects if the module is turned on (sends data)
> but should be off, e.g. if it was already turned on during boot or power-on-reset.
>
> Additionally, rfkill block/unblock can be used to control an external LNA
> (and power down the module if not needed).
>
> The driver concept is based on code developed by Neil Brown <neilb@suse.de>
> but simplified and adapted to use the new serdev API introduced in v4.11.
I'm sorry (and I know this discussion has been going on for a long
time), but this still feels like too much of a hack.
You're registering a tty driver to allow user space to continue treat
this as a tty device, but you provide no means of actually modifying
anything (line settings, etc). It's essentially just a character device
with common tty ioctls as noops from a device PoV (well, plus the ldisc
buffering and processing).
This will probably require someone to first implement a generic gps
framework with a properly defined interface which you may then teach
gpsd to use (e.g. to avoid all its autodetection functionality) instead.
Or some entirely different approach, for example, where you manage
everything from user space.
I'd suggest reiterating the problem you're trying to solve and
enumerating the previously discussed potential solutions in order to
find a proper abstraction level for this (before getting lost in
implementation details).
That being said, I'm still pointing some bugs and issue below that you
can consider for future versions of this (and other drivers) below.
> Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
> ---
> drivers/misc/Kconfig | 10 +
> drivers/misc/Makefile | 1 +
> drivers/misc/w2sg0004.c | 553 ++++++++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 564 insertions(+)
> create mode 100644 drivers/misc/w2sg0004.c
>
> diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
> index f1a5c2357b14..a3b11016ed2b 100644
> --- a/drivers/misc/Kconfig
> +++ b/drivers/misc/Kconfig
> @@ -508,4 +508,14 @@ source "drivers/misc/mic/Kconfig"
> source "drivers/misc/genwqe/Kconfig"
> source "drivers/misc/echo/Kconfig"
> source "drivers/misc/cxl/Kconfig"
> +
> +config W2SG0004
> + tristate "W2SG00x4 on/off control"
Please provide a better summary for what this driver does.
> + depends on GPIOLIB && SERIAL_DEV_BUS
> + help
> + Enable on/off control of W2SG00x4 GPS moduled connected
Some whitespace issue here.
> + to some SoC UART to allow powering up/down if the /dev/ttyGPSn
> + is opened/closed.
> + It also provides a rfkill gps name to control the LNA power.
> +
> endmenu
> diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile
> index 5ca5f64df478..d9d824b3d20a 100644
> --- a/drivers/misc/Makefile
> +++ b/drivers/misc/Makefile
> @@ -50,6 +50,7 @@ obj-$(CONFIG_SRAM_EXEC) += sram-exec.o
> obj-y += mic/
> obj-$(CONFIG_GENWQE) += genwqe/
> obj-$(CONFIG_ECHO) += echo/
> +obj-$(CONFIG_W2SG0004) += w2sg0004.o
> obj-$(CONFIG_VEXPRESS_SYSCFG) += vexpress-syscfg.o
> obj-$(CONFIG_CXL_BASE) += cxl/
> obj-$(CONFIG_ASPEED_LPC_CTRL) += aspeed-lpc-ctrl.o
> diff --git a/drivers/misc/w2sg0004.c b/drivers/misc/w2sg0004.c
> new file mode 100644
> index 000000000000..6bfd12eb8e02
> --- /dev/null
> +++ b/drivers/misc/w2sg0004.c
> @@ -0,0 +1,553 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Driver for power controlling the w2sg0004/w2sg0084 GPS receiver.
> + *
> + * Copyright (C) 2013 Neil Brown <neilb@suse.de>
> + * Copyright (C) 2015-2017 H. Nikolaus Schaller <hns@goldelico.com>,
> + * Golden Delicious Computers
> + *
> + * This receiver has an ON/OFF pin which must be toggled to
> + * turn the device 'on' of 'off'. A high->low->high toggle
s/of/or/
> + * will switch the device on if it is off, and off if it is on.
> + *
> + * To enable receiving on/off requests we register with the
> + * UART power management notifications.
No, the UART (serial device) would be the grandparent of your serdev
device (for which you register PM callbacks).
> + *
> + * It is not possible to directly detect the state of the device.
Didn't the 0084 version have a pin for this?
> + * However when it is on it will send characters on a UART line
> + * regularly.
> + *
> + * To detect that the power state is out of sync (e.g. if GPS
> + * was enabled before a reboot), we register for UART data received
> + * notifications.
> + *
> + * In addition we register as a rfkill client so that we can
> + * control the LNA power.
> + *
> + */
> +
> +#include <linux/delay.h>
> +#include <linux/err.h>
> +#include <linux/interrupt.h>
> +#include <linux/irq.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_irq.h>
> +#include <linux/of_gpio.h>
> +#include <linux/platform_device.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/rfkill.h>
> +#include <linux/serdev.h>
> +#include <linux/sched.h>
> +#include <linux/slab.h>
> +#include <linux/tty.h>
> +#include <linux/tty_flip.h>
> +#include <linux/workqueue.h>
> +
> +/*
> + * There seems to be restrictions on how quickly we can toggle the
> + * on/off line. data sheets says "two rtc ticks", whatever that means.
> + * If we do it too soon it doesn't work.
> + * So we have a state machine which uses the common work queue to ensure
> + * clean transitions.
> + * When a change is requested we record that request and only act on it
> + * once the previous change has completed.
> + * A change involves a 10ms low pulse, and a 990ms raised level, so only
> + * one change per second.
> + */
> +
> +enum w2sg_state {
> + W2SG_IDLE, /* is not changing state */
> + W2SG_PULSE, /* activate on/off impulse */
> + W2SG_NOPULSE /* deactivate on/off impulse */
> +};
> +
> +struct w2sg_data {
> + struct rfkill *rf_kill;
> + struct regulator *lna_regulator;
> + int lna_blocked; /* rfkill block gps active */
> + int lna_is_off; /* LNA is currently off */
> + int is_on; /* current state (0/1) */
These look like flags; use a bitfield rather than an int.
> + unsigned long last_toggle;
> + unsigned long backoff; /* time to wait since last_toggle */
> + int on_off_gpio; /* the on-off gpio number */
> + struct serdev_device *uart; /* uart connected to the chip */
> + struct tty_driver *tty_drv; /* this is the user space tty */
This does not belong in the device data as someone (Arnd?) already
pointed out to you in a comment to an earlier version. More below.
> + struct device *dev; /* from tty_port_register_device() */
> + struct tty_port port;
> + int open_count; /* how often we were opened */
> + enum w2sg_state state;
> + int requested; /* requested state (0/1) */
> + int suspended;
> + struct delayed_work work;
> + int discard_count;
> +};
You seem to have completely ignored locking. These flags and resources
are accessed from multiple contexts, and it all looks very susceptible
to races (e.g. the work queue can race with the rfkill callback and
leave the regulator enabled when it should be off).
> +/* should become w2sg_by_minor[n] if we want to support multiple devices */
> +static struct w2sg_data *w2sg_by_minor;
> +
> +static int w2sg_set_lna_power(struct w2sg_data *data)
> +{
> + int ret = 0;
> + int off = data->suspended || !data->requested || data->lna_blocked;
> +
> + pr_debug("%s: %s\n", __func__, off ? "off" : "on");
This and other pr_xxx should be replaced with dev_dbg and friends.
> +
> + if (off != data->lna_is_off) {
> + data->lna_is_off = off;
> + if (!IS_ERR_OR_NULL(data->lna_regulator)) {
> + if (off)
> + regulator_disable(data->lna_regulator);
> + else
> + ret = regulator_enable(data->lna_regulator);
> + }
> + }
> +
> + return ret;
> +}
> +
> +static void w2sg_set_power(void *pdata, int val)
Don't pass around void pointers like this.
> +{
> + struct w2sg_data *data = (struct w2sg_data *) pdata;
> +
> + pr_debug("%s to state=%d (requested=%d)\n", __func__, val,
> + data->requested);
> +
> + if (val && !data->requested) {
> + data->requested = true;
> + } else if (!val && data->requested) {
> + data->backoff = HZ;
> + data->requested = false;
> + } else
> + return;
Missing brackets.
> +
> + if (!data->suspended)
> + schedule_delayed_work(&data->work, 0);
> +}
> +
> +/* called each time data is received by the UART (i.e. sent by the w2sg0004) */
> +static int w2sg_uart_receive_buf(struct serdev_device *serdev,
> + const unsigned char *rxdata,
> + size_t count)
> +{
> + struct w2sg_data *data =
> + (struct w2sg_data *) serdev_device_get_drvdata(serdev);
> +
> + if (!data->requested && !data->is_on) {
> + /*
> + * we have received characters while the w2sg
> + * should have been be turned off
> + */
> + data->discard_count += count;
> + if ((data->state == W2SG_IDLE) &&
> + time_after(jiffies,
> + data->last_toggle + data->backoff)) {
> + /* Should be off by now, time to toggle again */
> + pr_debug("w2sg00x4 has sent %d characters data although it should be off!\n",
> + data->discard_count);
> +
> + data->discard_count = 0;
> +
> + data->is_on = true;
> + data->backoff *= 2;
> + if (!data->suspended)
> + schedule_delayed_work(&data->work, 0);
> + }
> + } else if (data->open_count > 0) {
> + int n;
> +
> + pr_debug("w2sg00x4: push %lu chars to tty port\n",
> + (unsigned long) count);
> +
> + /* pass to user-space */
> + n = tty_insert_flip_string(&data->port, rxdata, count);
> + if (n != count)
> + pr_err("w2sg00x4: did loose %lu characters\n",
> + (unsigned long) (count - n));
> + tty_flip_buffer_push(&data->port);
> + return n;
> + }
> +
> + /* assume we have processed everything */
> + return count;
> +}
> +
> +/* try to toggle the power state by sending a pulse to the on-off GPIO */
> +static void toggle_work(struct work_struct *work)
> +{
> + struct w2sg_data *data = container_of(work, struct w2sg_data,
> + work.work);
> +
> + switch (data->state) {
> + case W2SG_IDLE:
> + if (data->requested == data->is_on)
> + return;
> +
> + w2sg_set_lna_power(data); /* update LNA power state */
> + gpio_set_value_cansleep(data->on_off_gpio, 0);
> + data->state = W2SG_PULSE;
> +
> + pr_debug("w2sg: power gpio ON\n");
> +
> + schedule_delayed_work(&data->work,
> + msecs_to_jiffies(10));
> + break;
> +
> + case W2SG_PULSE:
> + gpio_set_value_cansleep(data->on_off_gpio, 1);
> + data->last_toggle = jiffies;
> + data->state = W2SG_NOPULSE;
> + data->is_on = !data->is_on;
> +
> + pr_debug("w2sg: power gpio OFF\n");
> +
> + schedule_delayed_work(&data->work,
> + msecs_to_jiffies(10));
> + break;
> +
> + case W2SG_NOPULSE:
> + data->state = W2SG_IDLE;
> +
> + pr_debug("w2sg: idle\n");
> +
> + break;
> +
> + }
> +}
> +
> +static int w2sg_rfkill_set_block(void *pdata, bool blocked)
> +{
> + struct w2sg_data *data = pdata;
> +
> + pr_debug("%s: blocked: %d\n", __func__, blocked);
> +
> + data->lna_blocked = blocked;
> +
> + return w2sg_set_lna_power(data);
> +}
> +
> +static struct rfkill_ops w2sg0004_rfkill_ops = {
> + .set_block = w2sg_rfkill_set_block,
> +};
> +
> +static struct serdev_device_ops serdev_ops = {
> + .receive_buf = w2sg_uart_receive_buf,
> +};
> +
> +/*
> + * we are a man-in the middle between the user-space visible tty port
> + * and the serdev tty where the chip is connected.
> + * This allows us to recognise when the device should be powered on
> + * or off and handle the "false" state that data arrives while no
> + * users-space tty client exists.
> + */
> +
> +static struct w2sg_data *w2sg_get_by_minor(unsigned int minor)
> +{
> + BUG_ON(minor >= 1);
Never use BUG_ON in driver code.
> + return w2sg_by_minor;
> +}
> +
> +static int w2sg_tty_install(struct tty_driver *driver, struct tty_struct *tty)
> +{
> + struct w2sg_data *data;
> + int retval;
> +
> + pr_debug("%s() tty = %p\n", __func__, tty);
> +
> + data = w2sg_get_by_minor(tty->index);
> +
> + if (!data)
> + return -ENODEV;
> +
> + retval = tty_standard_install(driver, tty);
> + if (retval)
> + goto error_init_termios;
> +
> + tty->driver_data = data;
> +
> + return 0;
> +
> +error_init_termios:
> + tty_port_put(&data->port);
Where's the corresponding get?
> + return retval;
> +}
> +
> +static int w2sg_tty_open(struct tty_struct *tty, struct file *file)
> +{
> + struct w2sg_data *data = tty->driver_data;
> +
> + pr_debug("%s() data = %p open_count = ++%d\n", __func__,
> + data, data->open_count);
> +
> + w2sg_set_power(data, ++data->open_count > 0);
> +
> + return tty_port_open(&data->port, tty, file);
You'd leave the open count incremented on errors here.
> +}
> +
> +static void w2sg_tty_close(struct tty_struct *tty, struct file *file)
> +{
> + struct w2sg_data *data = tty->driver_data;
> +
> + pr_debug("%s()\n", __func__);
> +
> + w2sg_set_power(data, --data->open_count > 0);
> +
> + tty_port_close(&data->port, tty, file);
> +}
> +
> +static int w2sg_tty_write(struct tty_struct *tty,
> + const unsigned char *buffer, int count)
> +{
> + struct w2sg_data *data = tty->driver_data;
> +
> + /* simply pass down to UART */
> + return serdev_device_write_buf(data->uart, buffer, count);
> +}
> +
> +static const struct tty_operations w2sg_serial_ops = {
> + .install = w2sg_tty_install,
> + .open = w2sg_tty_open,
> + .close = w2sg_tty_close,
> + .write = w2sg_tty_write,
> +};
> +
> +static const struct tty_port_operations w2sg_port_ops = {
> + /* none defined, but we need the struct */
> +};
> +
> +static int w2sg_probe(struct serdev_device *serdev)
> +{
> + struct w2sg_data *data;
> + struct rfkill *rf_kill;
> + int err;
> + int minor;
> + enum of_gpio_flags flags;
> +
> + pr_debug("%s()\n", __func__);
> +
> + /*
> + * For future consideration:
> + * for multiple such GPS receivers in one system
> + * we need a mechanism to define distinct minor values
> + * and search for an unused one.
> + */
> + minor = 0;
> + if (w2sg_get_by_minor(minor)) {
> + pr_err("w2sg minor is already in use!\n");
> + return -ENODEV;
> + }
> +
> + data = devm_kzalloc(&serdev->dev, sizeof(*data), GFP_KERNEL);
> + if (data == NULL)
> + return -ENOMEM;
> +
> + serdev_device_set_drvdata(serdev, data);
> +
> + data->on_off_gpio = of_get_named_gpio_flags(serdev->dev.of_node,
> + "enable-gpios", 0,
> + &flags);
You should be using gpio descriptors and not the legacy interface.
> + /* defer until we have all gpios */
> + if (data->on_off_gpio == -EPROBE_DEFER)
> + return -EPROBE_DEFER;
> +
> + data->lna_regulator = devm_regulator_get_optional(&serdev->dev,
> + "lna");
> + if (IS_ERR(data->lna_regulator)) {
> + /* defer until we can get the regulator */
> + if (PTR_ERR(data->lna_regulator) == -EPROBE_DEFER)
> + return -EPROBE_DEFER;
> +
> + data->lna_regulator = NULL;
> + }
> + pr_debug("%s() lna_regulator = %p\n", __func__, data->lna_regulator);
> +
> + data->lna_blocked = true;
> + data->lna_is_off = true;
> +
> + data->is_on = false;
> + data->requested = false;
> + data->state = W2SG_IDLE;
> + data->last_toggle = jiffies;
> + data->backoff = HZ;
> +
> + data->uart = serdev;
> +
> + INIT_DELAYED_WORK(&data->work, toggle_work);
> +
> + err = devm_gpio_request(&serdev->dev, data->on_off_gpio,
> + "w2sg0004-on-off");
> + if (err < 0)
> + goto out;
Just return unless you're actually undwinding something.
> +
> + gpio_direction_output(data->on_off_gpio, false);
> +
> + serdev_device_set_client_ops(data->uart, &serdev_ops);
> + serdev_device_open(data->uart);
Missing error handling.
And always keeping the port open would in most cases prevent the serial
controller from runtime suspending.
> +
> + serdev_device_set_baudrate(data->uart, 9600);
> + serdev_device_set_flow_control(data->uart, false);
So you hardcode these settings and provide no means for user space to
change them. This may make sense for this GPS, but it looks like at
least some of the wi2wi 0084 modules use 4800 baud ("i"-versions?).
> +
> + rf_kill = rfkill_alloc("GPS", &serdev->dev, RFKILL_TYPE_GPS,
> + &w2sg0004_rfkill_ops, data);
> + if (rf_kill == NULL) {
> + err = -ENOMEM;
> + goto err_rfkill;
Name error labels after what they do (not where you jump from).
That may have prevented the NULL deref you'd trigger in the error path
here.
> + }
> +
> + err = rfkill_register(rf_kill);
> + if (err) {
> + dev_err(&serdev->dev, "Cannot register rfkill device\n");
> + goto err_rfkill;
> + }
> +
> + data->rf_kill = rf_kill;
> +
> + /* allocate the tty driver */
> + data->tty_drv = alloc_tty_driver(1);
As was already pointed out by Arnd in a review of an previous version of
this series, this must not be done at probe (do it at module init). We
don't want separate tty drivers for every device.
> + if (!data->tty_drv)
> + return -ENOMEM;
Here you'd leak the registered rfkill structure, and leave the port
open.
> +
> + /* initialize the tty driver */
> + data->tty_drv->owner = THIS_MODULE;
> + data->tty_drv->driver_name = "w2sg0004";
> + data->tty_drv->name = "ttyGPS";
I don't think you should be claiming this generic namespace for
something as specific as this.
> + data->tty_drv->major = 0;
> + data->tty_drv->minor_start = 0;
> + data->tty_drv->type = TTY_DRIVER_TYPE_SERIAL;
> + data->tty_drv->subtype = SERIAL_TYPE_NORMAL;
> + data->tty_drv->flags = TTY_DRIVER_REAL_RAW | TTY_DRIVER_DYNAMIC_DEV;
> + data->tty_drv->init_termios = tty_std_termios;
> + data->tty_drv->init_termios.c_cflag = B9600 | CS8 | CREAD |
> + HUPCL | CLOCAL;
> + /*
> + * optional:
> + * tty_termios_encode_baud_rate(&data->tty_drv->init_termios,
> + 115200, 115200);
> + * w2sg_tty_termios(&data->tty_drv->init_termios);
> + */
Why is this here?
> + tty_set_operations(data->tty_drv, &w2sg_serial_ops);
> +
> + /* register the tty driver */
> + err = tty_register_driver(data->tty_drv);
> + if (err) {
> + pr_err("%s - tty_register_driver failed(%d)\n",
> + __func__, err);
> + put_tty_driver(data->tty_drv);
> + goto err_rfkill;
> + }
> +
> + /* minor (0) is now in use */
> + w2sg_by_minor = data;
> +
> + tty_port_init(&data->port);
> + data->port.ops = &w2sg_port_ops;
> +
> + data->dev = tty_port_register_device(&data->port,
> + data->tty_drv, minor, &serdev->dev);
Missing error handling.
> +
> + /* keep off until user space requests the device */
> + w2sg_set_power(data, false);
> +
> + return 0;
> +
> +err_rfkill:
> + rfkill_destroy(rf_kill);
> + serdev_device_close(data->uart);
> +out:
> + return err;
> +}
> +
> +static void w2sg_remove(struct serdev_device *serdev)
> +{
> + struct w2sg_data *data = serdev_device_get_drvdata(serdev);
> + int minor;
> +
> + cancel_delayed_work_sync(&data->work);
> +
> + /* what is the right sequence to avoid problems? */
Clearly that's something you need to determine.
> + serdev_device_close(data->uart);
> +
> + /* we should lookup in w2sg_by_minor */
> + minor = 0;
> + tty_unregister_device(data->tty_drv, minor);
> +
> + tty_unregister_driver(data->tty_drv);
> +}
> +
> +static int __maybe_unused w2sg_suspend(struct device *dev)
> +{
> + struct w2sg_data *data = dev_get_drvdata(dev);
> +
> + data->suspended = true;
> +
> + cancel_delayed_work_sync(&data->work);
> +
> + w2sg_set_lna_power(data); /* shuts down if needed */
> +
> + if (data->state == W2SG_PULSE) {
> + msleep(10);
> + gpio_set_value_cansleep(data->on_off_gpio, 1);
> + data->last_toggle = jiffies;
> + data->is_on = !data->is_on;
> + data->state = W2SG_NOPULSE;
> + }
> +
> + if (data->state == W2SG_NOPULSE) {
> + msleep(10);
> + data->state = W2SG_IDLE;
> + }
> +
> + if (data->is_on) {
> + pr_info("GPS off for suspend %d %d %d\n", data->requested,
> + data->is_on, data->lna_is_off);
> +
> + gpio_set_value_cansleep(data->on_off_gpio, 0);
> + msleep(10);
> + gpio_set_value_cansleep(data->on_off_gpio, 1);
> + data->is_on = 0;
> + }
> +
> + return 0;
> +}
> +
> +static int __maybe_unused w2sg_resume(struct device *dev)
> +{
> + struct w2sg_data *data = dev_get_drvdata(dev);
> +
> + data->suspended = false;
> +
> + pr_info("GPS resuming %d %d %d\n", data->requested,
> + data->is_on, data->lna_is_off);
> +
> + schedule_delayed_work(&data->work, 0); /* enables LNA if needed */
> +
> + return 0;
> +}
> +
> +static const struct of_device_id w2sg0004_of_match[] = {
> + { .compatible = "wi2wi,w2sg0004" },
> + { .compatible = "wi2wi,w2sg0084" },
> + {},
> +};
> +MODULE_DEVICE_TABLE(of, w2sg0004_of_match);
> +
> +SIMPLE_DEV_PM_OPS(w2sg_pm_ops, w2sg_suspend, w2sg_resume);
> +
> +static struct serdev_device_driver w2sg_driver = {
> + .probe = w2sg_probe,
> + .remove = w2sg_remove,
> + .driver = {
> + .name = "w2sg0004",
> + .owner = THIS_MODULE,
> + .pm = &w2sg_pm_ops,
> + .of_match_table = of_match_ptr(w2sg0004_of_match)
> + },
> +};
> +
> +module_serdev_device_driver(w2sg_driver);
> +
> +MODULE_AUTHOR("NeilBrown <neilb@suse.de>");
> +MODULE_AUTHOR("H. Nikolaus Schaller <hns@goldelico.com>");
> +MODULE_DESCRIPTION("w2sg0004 GPS power management driver");
> +MODULE_LICENSE("GPL v2");
Johan
^ permalink raw reply
* [RFC PATCH V1 1/2] clk: use atomic runtime pm api in clk_core_is_enabled
From: Ulf Hansson @ 2017-12-22 12:51 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1513935965-12909-1-git-send-email-aisheng.dong@nxp.com>
On 22 December 2017 at 10:46, Dong Aisheng <aisheng.dong@nxp.com> wrote:
> Current clk_pm_runtime_put is using pm_runtime_put_sync which
> is not safe to be called in clk_core_is_enabled as it should
> be able to run in atomic context.
>
> Thus use pm_runtime_put instead which is atomic safe.
>
> Cc: Stephen Boyd <sboyd@codeaurora.org>
> Cc: Michael Turquette <mturquette@baylibre.com>
> Cc: Ulf Hansson <ulf.hansson@linaro.org>
> Cc: Marek Szyprowski <m.szyprowski@samsung.com>
> Fixes: 9a34b45397e5 ("clk: Add support for runtime PM")
> Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>
Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org>
Kind regards
Uffe
> ---
> drivers/clk/clk.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> index 5ec5809..e24968f 100644
> --- a/drivers/clk/clk.c
> +++ b/drivers/clk/clk.c
> @@ -227,7 +227,8 @@ static bool clk_core_is_enabled(struct clk_core *core)
>
> ret = core->ops->is_enabled(core->hw);
> done:
> - clk_pm_runtime_put(core);
> + if (core->dev)
> + pm_runtime_put(core->dev);
>
> return ret;
> }
> --
> 2.7.4
>
^ permalink raw reply
* [PATCH v5 5/5] misc serdev: w2sg0004: add debugging code and Kconfig
From: Johan Hovold @ 2017-12-22 12:51 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <5816bfb9e7cab68591c133e20696d6188ebe70de.1512114577.git.hns@goldelico.com>
On Fri, Dec 01, 2017 at 08:49:38AM +0100, H. Nikolaus Schaller wrote:
> This allows to set CONFIG_W2SG0004_DEBUG which will
> make the driver report more activities and it will turn on the
> GPS module during boot while the driver assumes that it
> is off. This can be used to debug the correct functioning of
> the hardware. Therefore we add it as an option to the driver
> because we think it is of general use (and a little tricky to
> add by system testers).
>
> Normally it should be off.
>
> Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
> ---
> drivers/misc/Kconfig | 8 ++++++++
> drivers/misc/w2sg0004.c | 37 +++++++++++++++++++++++++++++++++++++
> 2 files changed, 45 insertions(+)
I'd say say this does not belong in the kernel at all. And even if the
power-state test were to be allowed, most of the pr_debugs would
need to go. You really should be using dev_dbg and friends, which can
already be enabled selectively at run time using dynamic debugging.
Johan
^ permalink raw reply
* [linux-sunxi] [PATCH v4 2/2] media: V3s: Add support for Allwinner CSI.
From: Philippe Ombredanne @ 2017-12-22 13:40 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20171222102156.cfemen6ouxxxbrem@plaes.org>
Yong,
On Fri, Dec 22, 2017 at 11:21 AM, Priit Laes <plaes@plaes.org> wrote:
> On Fri, Dec 22, 2017 at 05:47:00PM +0800, Yong Deng wrote:
>> Allwinner V3s SoC have two CSI module. CSI0 is used for MIPI interface
>> and CSI1 is used for parallel interface. This is not documented in
>> datasheet but by testing and guess.
>>
>> This patch implement a v4l2 framework driver for it.
>>
>> Currently, the driver only support the parallel interface. MIPI-CSI2,
>> ISP's support are not included in this patch.
>>
>> Signed-off-by: Yong Deng <yong.deng@magewell.com>
<snip>
>> --- /dev/null
>> +++ b/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi.c
>> @@ -0,0 +1,878 @@
>> +/*
>> + * Copyright (c) 2017 Magewell Electronics Co., Ltd. (Nanjing).
>> + * All rights reserved.
>> + * Author: Yong Deng <yong.deng@magewell.com>
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License version 2 as
>> + * published by the Free Software Foundation.
>> + *
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
>> + * GNU General Public License for more details.
>> + */
Would you mind using the new SPDX tags documented in Thomas patch set
[1] rather than this fine but longer legalese?
>> +MODULE_LICENSE("GPL v2");
Per module.h this means GPL2 only. This is not matching your top
license above which is GPL2 or later.
Please make sure your MODULE_LICENSE is consistent with the top level license.
[1] https://lkml.org/lkml/2017/12/4/934
--
Cordially
Philippe Ombredanne
^ permalink raw reply
* [linux-sunxi] [PATCH v4 0/2] Initial Allwinner V3s CSI Support
From: Ondřej Jirman @ 2017-12-22 13:46 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1513935138-35223-1-git-send-email-yong.deng@magewell.com>
Hello,
Yong Deng p??e v P? 22. 12. 2017 v 17:32 +0800:
> This patchset add initial support for Allwinner V3s CSI.
>
> Allwinner V3s SoC have two CSI module. CSI0 is used for MIPI interface
> and CSI1 is used for parallel interface. This is not documented in
> datasheet but by testing and guess.
>
> This patchset implement a v4l2 framework driver and add a binding
> documentation for it.
>
> Currently, the driver only support the parallel interface. And has been
> tested with a BT1120 signal which generating from FPGA. The following
> fetures are not support with this patchset:
> - ISP
> - MIPI-CSI2
> - Master clock for camera sensor
> - Power regulator for the front end IC
>
> Thanks for Ond?ej Jirman's help.
>
> Changes in v4:
> * Deal with the CSI 'INNER QUEUE'.
> CSI will lookup the next dma buffer for next frame before the
> the current frame done IRQ triggered. This is not documented
> but reported by Ond?ej Jirman.
> The BSP code has workaround for this too. It skip to mark the
> first buffer as frame done for VB2 and pass the second buffer
> to CSI in the first frame done ISR call. Then in second frame
> done ISR call, it mark the first buffer as frame done for VB2
> and pass the third buffer to CSI. And so on. The bad thing is
> that the first buffer will be written twice and the first frame
> is dropped even the queued buffer is sufficient.
> So, I make some improvement here. Pass the next buffer to CSI
> just follow starting the CSI. In this case, the first frame
> will be stored in first buffer, second frame in second buffer.
> This mothed is used to avoid dropping the first frame, it
> would also drop frame when lacking of queued buffer.
> * Fix: using a wrong mbus_code when getting the supported formats
> * Change all fourcc to pixformat
> * Change some function names
>
> Changes in v3:
> * Get rid of struct sun6i_csi_ops
> * Move sun6i-csi to new directory drivers/media/platform/sunxi
> * Merge sun6i_csi.c and sun6i_csi_v3s.c into sun6i_csi.c
> * Use generic fwnode endpoints parser
> * Only support a single subdev to make things simple
> * Many complaintion fix
>
> Changes in v2:
> * Change sunxi-csi to sun6i-csi
> * Rebase to media_tree master branch
>
> Following is the 'v4l2-compliance -s -f' output, I have test this
> with both interlaced and progressive signal:
>
> # ./v4l2-compliance -s -f
> v4l2-compliance SHA : 6049ea8bd64f9d78ef87ef0c2b3dc9b5de1ca4a1
>
> Driver Info:
> Driver name : sun6i-video
> Card type : sun6i-csi
> Bus info : platform:csi
> Driver version: 4.15.0
> Capabilities : 0x84200001
> Video Capture
> Streaming
> Extended Pix Format
> Device Capabilities
> Device Caps : 0x04200001
> Video Capture
> Streaming
> Extended Pix Format
>
> Compliance test for device /dev/video0 (not using libv4l2):
>
> Required ioctls:
> test VIDIOC_QUERYCAP: OK
>
> Allow for multiple opens:
> test second video open: OK
> test VIDIOC_QUERYCAP: OK
> test VIDIOC_G/S_PRIORITY: OK
> test for unlimited opens: OK
>
> Debug ioctls:
> test VIDIOC_DBG_G/S_REGISTER: OK (Not Supported)
> test VIDIOC_LOG_STATUS: OK (Not Supported)
>
> Input ioctls:
> test VIDIOC_G/S_TUNER/ENUM_FREQ_BANDS: OK (Not Supported)
> test VIDIOC_G/S_FREQUENCY: OK (Not Supported)
> test VIDIOC_S_HW_FREQ_SEEK: OK (Not Supported)
> test VIDIOC_ENUMAUDIO: OK (Not Supported)
> test VIDIOC_G/S/ENUMINPUT: OK
> test VIDIOC_G/S_AUDIO: OK (Not Supported)
> Inputs: 1 Audio Inputs: 0 Tuners: 0
>
> Output ioctls:
> test VIDIOC_G/S_MODULATOR: OK (Not Supported)
> test VIDIOC_G/S_FREQUENCY: OK (Not Supported)
> test VIDIOC_ENUMAUDOUT: OK (Not Supported)
> test VIDIOC_G/S/ENUMOUTPUT: OK (Not Supported)
> test VIDIOC_G/S_AUDOUT: OK (Not Supported)
> Outputs: 0 Audio Outputs: 0 Modulators: 0
>
> Input/Output configuration ioctls:
> test VIDIOC_ENUM/G/S/QUERY_STD: OK (Not Supported)
> test VIDIOC_ENUM/G/S/QUERY_DV_TIMINGS: OK (Not Supported)
> test VIDIOC_DV_TIMINGS_CAP: OK (Not Supported)
> test VIDIOC_G/S_EDID: OK (Not Supported)
>
> Test input 0:
>
> Control ioctls:
> test VIDIOC_QUERY_EXT_CTRL/QUERYMENU: OK (Not Supported)
> test VIDIOC_QUERYCTRL: OK (Not Supported)
> test VIDIOC_G/S_CTRL: OK (Not Supported)
> test VIDIOC_G/S/TRY_EXT_CTRLS: OK (Not Supported)
> test VIDIOC_(UN)SUBSCRIBE_EVENT/DQEVENT: OK (Not Supported)
> test VIDIOC_G/S_JPEGCOMP: OK (Not Supported)
> Standard Controls: 0 Private Controls: 0
I'm not sure if your driver passes control queries to the subdev. It
did not originally, and I'm not sure you picked up the change from my
version of the driver. "Not supported" here seems to indicate that it
does not.
I'd be interested what's the recommended practice here. It sure helps
with some apps that expect to be able to modify various input controls
directly on the /dev/video# device. These are then supported out of the
box.
It's a one-line change. See:
https://www.kernel.org/doc/html/latest/media/kapi/v4l2-controls.html#in
heriting-controls
> Format ioctls:
> test VIDIOC_ENUM_FMT/FRAMESIZES/FRAMEINTERVALS: OK
> test VIDIOC_G/S_PARM: OK (Not Supported)
> test VIDIOC_G_FBUF: OK (Not Supported)
> test VIDIOC_G_FMT: OK
> test VIDIOC_TRY_FMT: OK
> test VIDIOC_S_FMT: OK
> test VIDIOC_G_SLICED_VBI_CAP: OK (Not Supported)
> test Cropping: OK (Not Supported)
> test Composing: OK (Not Supported)
> test Scaling: OK (Not Supported)
>
> Codec ioctls:
> test VIDIOC_(TRY_)ENCODER_CMD: OK (Not Supported)
> test VIDIOC_G_ENC_INDEX: OK (Not Supported)
> test VIDIOC_(TRY_)DECODER_CMD: OK (Not Supported)
>
> Buffer ioctls:
> test VIDIOC_REQBUFS/CREATE_BUFS/QUERYBUF: OK
> test VIDIOC_EXPBUF: OK
>
> Test input 0:
>
> Streaming ioctls:
> test read/write: OK (Not Supported)
> test MMAP: OK
> test USERPTR: OK (Not Supported)
> test DMABUF: Cannot test, specify --expbuf-device
>
> Stream using all formats:
> test MMAP for Format HM12, Frame Size 1280x720:
> Stride 1920, Field None: OK
> test MMAP for Format NV12, Frame Size 1280x720:
> Stride 1920, Field None: OK
> test MMAP for Format NV21, Frame Size 1280x720:
> Stride 1920, Field None: OK
> test MMAP for Format YU12, Frame Size 1280x720:
> Stride 1920, Field None: OK
> test MMAP for Format YV12, Frame Size 1280x720:
> Stride 1920, Field None: OK
> test MMAP for Format NV16, Frame Size 1280x720:
> Stride 2560, Field None: OK
> test MMAP for Format NV61, Frame Size 1280x720:
> Stride 2560, Field None: OK
> test MMAP for Format 422P, Frame Size 1280x720:
> Stride 2560, Field None: OK
>
> Total: 54, Succeeded: 54, Failed: 0, Warnings: 0
>
> Yong Deng (2):
> dt-bindings: media: Add Allwinner V3s Camera Sensor Interface (CSI)
> media: V3s: Add support for Allwinner CSI.
>
> .../devicetree/bindings/media/sun6i-csi.txt | 51 ++
> MAINTAINERS | 8 +
> drivers/media/platform/Kconfig | 1 +
> drivers/media/platform/Makefile | 2 +
> drivers/media/platform/sunxi/sun6i-csi/Kconfig | 9 +
> drivers/media/platform/sunxi/sun6i-csi/Makefile | 3 +
> drivers/media/platform/sunxi/sun6i-csi/sun6i_csi.c | 878 +++++++++++++++++++++
> drivers/media/platform/sunxi/sun6i-csi/sun6i_csi.h | 147 ++++
> .../media/platform/sunxi/sun6i-csi/sun6i_csi_reg.h | 203 +++++
> .../media/platform/sunxi/sun6i-csi/sun6i_video.c | 752 ++++++++++++++++++
> .../media/platform/sunxi/sun6i-csi/sun6i_video.h | 60 ++
> 11 files changed, 2114 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/media/sun6i-csi.txt
> create mode 100644 drivers/media/platform/sunxi/sun6i-csi/Kconfig
> create mode 100644 drivers/media/platform/sunxi/sun6i-csi/Makefile
> create mode 100644 drivers/media/platform/sunxi/sun6i-csi/sun6i_csi.c
> create mode 100644 drivers/media/platform/sunxi/sun6i-csi/sun6i_csi.h
> create mode 100644 drivers/media/platform/sunxi/sun6i-csi/sun6i_csi_reg.h
> create mode 100644 drivers/media/platform/sunxi/sun6i-csi/sun6i_video.c
> create mode 100644 drivers/media/platform/sunxi/sun6i-csi/sun6i_video.h
>
> --
> 1.8.3.1
>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: This is a digitally signed message part
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20171222/b8debf28/attachment-0001.sig>
^ permalink raw reply
* [PATCH] clk: divider: fix incorrect usage of container_of
From: Sylvain Lemieux @ 2017-12-22 13:51 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20171222103419.GB18255@piout.net>
For lpc32xx:
Acked-by: Sylvain Lemieux <slemieux.tyco@gmail.com>
On Fri, Dec 22, 2017 at 5:34 AM, Alexandre Belloni
<alexandre.belloni@free-electrons.com> wrote:
> On 21/12/2017 at 17:30:54 +0100, Jerome Brunet wrote:
>> divider_recalc_rate() is an helper function used by clock divider of
>> different types, so the structure containing the 'hw' pointer is not
>> always a 'struct clk_divider'
>>
>> At the following line:
>> > div = _get_div(table, val, flags, divider->width);
>>
>> in several cases, the value of 'divider->width' is garbage as the actual
>> structure behind this memory is not a 'struct clk_divider'
>>
>> Fortunately, this width value is used by _get_val() only when
>> CLK_DIVIDER_MAX_AT_ZERO flag is set. This has never been the case so
>> far when the structure is not a 'struct clk_divider'. This is probably
>> why we did not notice this bug before
>>
>> Fixes: afe76c8fd030 ("clk: allow a clk divider with max divisor when zero")
>> Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
>
> For RTC:
> Acked-by: Alexandre Belloni <alexandre.belloni@free-electrons.com>
>
>> ---
>> Hi Stephen, Mike,
>>
>> In addition to clock, this patch also touch the rtc and drm directories.
>> As it is changing the API of the helper function, I have this fix in a
>> single commit to avoid breaking bisect.
>>
>> Please let me know if you prefer to do differently.
>>
>> Cheers
>> Jerome
>>
>> drivers/clk/clk-divider.c | 7 +++----
>> drivers/clk/hisilicon/clkdivider-hi6220.c | 2 +-
>> drivers/clk/nxp/clk-lpc32xx.c | 2 +-
>> drivers/clk/qcom/clk-regmap-divider.c | 2 +-
>> drivers/clk/sunxi-ng/ccu_div.c | 2 +-
>> drivers/gpu/drm/msm/dsi/pll/dsi_pll_14nm.c | 2 +-
>> drivers/rtc/rtc-ac100.c | 6 ++++--
>> include/linux/clk-provider.h | 2 +-
>> 8 files changed, 13 insertions(+), 12 deletions(-)
>>
>> diff --git a/drivers/clk/clk-divider.c b/drivers/clk/clk-divider.c
>> index 4ed516cb7276..b49942b9fe50 100644
>> --- a/drivers/clk/clk-divider.c
>> +++ b/drivers/clk/clk-divider.c
>> @@ -118,12 +118,11 @@ static unsigned int _get_val(const struct clk_div_table *table,
>> unsigned long divider_recalc_rate(struct clk_hw *hw, unsigned long parent_rate,
>> unsigned int val,
>> const struct clk_div_table *table,
>> - unsigned long flags)
>> + unsigned long flags, unsigned long width)
>> {
>> - struct clk_divider *divider = to_clk_divider(hw);
>> unsigned int div;
>>
>> - div = _get_div(table, val, flags, divider->width);
>> + div = _get_div(table, val, flags, width);
>> if (!div) {
>> WARN(!(flags & CLK_DIVIDER_ALLOW_ZERO),
>> "%s: Zero divisor and CLK_DIVIDER_ALLOW_ZERO not set\n",
>> @@ -145,7 +144,7 @@ static unsigned long clk_divider_recalc_rate(struct clk_hw *hw,
>> val &= div_mask(divider->width);
>>
>> return divider_recalc_rate(hw, parent_rate, val, divider->table,
>> - divider->flags);
>> + divider->flags, divider->width);
>> }
>>
>> static bool _is_valid_table_div(const struct clk_div_table *table,
>> diff --git a/drivers/clk/hisilicon/clkdivider-hi6220.c b/drivers/clk/hisilicon/clkdivider-hi6220.c
>> index a1c1f684ad58..9f46cf9dcc65 100644
>> --- a/drivers/clk/hisilicon/clkdivider-hi6220.c
>> +++ b/drivers/clk/hisilicon/clkdivider-hi6220.c
>> @@ -56,7 +56,7 @@ static unsigned long hi6220_clkdiv_recalc_rate(struct clk_hw *hw,
>> val &= div_mask(dclk->width);
>>
>> return divider_recalc_rate(hw, parent_rate, val, dclk->table,
>> - CLK_DIVIDER_ROUND_CLOSEST);
>> + CLK_DIVIDER_ROUND_CLOSEST, dclk->width);
>> }
>>
>> static long hi6220_clkdiv_round_rate(struct clk_hw *hw, unsigned long rate,
>> diff --git a/drivers/clk/nxp/clk-lpc32xx.c b/drivers/clk/nxp/clk-lpc32xx.c
>> index b669a5c10fee..f5d815f577e0 100644
>> --- a/drivers/clk/nxp/clk-lpc32xx.c
>> +++ b/drivers/clk/nxp/clk-lpc32xx.c
>> @@ -956,7 +956,7 @@ static unsigned long clk_divider_recalc_rate(struct clk_hw *hw,
>> val &= div_mask(divider->width);
>>
>> return divider_recalc_rate(hw, parent_rate, val, divider->table,
>> - divider->flags);
>> + divider->flags, divider->width);
>> }
>>
>> static long clk_divider_round_rate(struct clk_hw *hw, unsigned long rate,
>> diff --git a/drivers/clk/qcom/clk-regmap-divider.c b/drivers/clk/qcom/clk-regmap-divider.c
>> index 53484912301e..928fcc16ee27 100644
>> --- a/drivers/clk/qcom/clk-regmap-divider.c
>> +++ b/drivers/clk/qcom/clk-regmap-divider.c
>> @@ -59,7 +59,7 @@ static unsigned long div_recalc_rate(struct clk_hw *hw,
>> div &= BIT(divider->width) - 1;
>>
>> return divider_recalc_rate(hw, parent_rate, div, NULL,
>> - CLK_DIVIDER_ROUND_CLOSEST);
>> + CLK_DIVIDER_ROUND_CLOSEST, divider->width);
>> }
>>
>> const struct clk_ops clk_regmap_div_ops = {
>> diff --git a/drivers/clk/sunxi-ng/ccu_div.c b/drivers/clk/sunxi-ng/ccu_div.c
>> index baa3cf96507b..302a18efd39f 100644
>> --- a/drivers/clk/sunxi-ng/ccu_div.c
>> +++ b/drivers/clk/sunxi-ng/ccu_div.c
>> @@ -71,7 +71,7 @@ static unsigned long ccu_div_recalc_rate(struct clk_hw *hw,
>> parent_rate);
>>
>> val = divider_recalc_rate(hw, parent_rate, val, cd->div.table,
>> - cd->div.flags);
>> + cd->div.flags, cd->div.width);
>>
>> if (cd->common.features & CCU_FEATURE_FIXED_POSTDIV)
>> val /= cd->fixed_post_div;
>> diff --git a/drivers/gpu/drm/msm/dsi/pll/dsi_pll_14nm.c b/drivers/gpu/drm/msm/dsi/pll/dsi_pll_14nm.c
>> index fe15aa64086f..71fe60e5f01f 100644
>> --- a/drivers/gpu/drm/msm/dsi/pll/dsi_pll_14nm.c
>> +++ b/drivers/gpu/drm/msm/dsi/pll/dsi_pll_14nm.c
>> @@ -698,7 +698,7 @@ static unsigned long dsi_pll_14nm_postdiv_recalc_rate(struct clk_hw *hw,
>> val &= div_mask(width);
>>
>> return divider_recalc_rate(hw, parent_rate, val, NULL,
>> - postdiv->flags);
>> + postdiv->flags, width);
>> }
>>
>> static long dsi_pll_14nm_postdiv_round_rate(struct clk_hw *hw,
>> diff --git a/drivers/rtc/rtc-ac100.c b/drivers/rtc/rtc-ac100.c
>> index 9e336184491c..0282ccc6181c 100644
>> --- a/drivers/rtc/rtc-ac100.c
>> +++ b/drivers/rtc/rtc-ac100.c
>> @@ -137,13 +137,15 @@ static unsigned long ac100_clkout_recalc_rate(struct clk_hw *hw,
>> div = (reg >> AC100_CLKOUT_PRE_DIV_SHIFT) &
>> ((1 << AC100_CLKOUT_PRE_DIV_WIDTH) - 1);
>> prate = divider_recalc_rate(hw, prate, div,
>> - ac100_clkout_prediv, 0);
>> + ac100_clkout_prediv, 0,
>> + AC100_CLKOUT_PRE_DIV_WIDTH);
>> }
>>
>> div = (reg >> AC100_CLKOUT_DIV_SHIFT) &
>> (BIT(AC100_CLKOUT_DIV_WIDTH) - 1);
>> return divider_recalc_rate(hw, prate, div, NULL,
>> - CLK_DIVIDER_POWER_OF_TWO);
>> + CLK_DIVIDER_POWER_OF_TWO,
>> + AC100_CLKOUT_DIV_WIDTH);
>> }
>>
>> static long ac100_clkout_round_rate(struct clk_hw *hw, unsigned long rate,
>> diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h
>> index 73ac87f34df9..4c4001086447 100644
>> --- a/include/linux/clk-provider.h
>> +++ b/include/linux/clk-provider.h
>> @@ -412,7 +412,7 @@ extern const struct clk_ops clk_divider_ro_ops;
>>
>> unsigned long divider_recalc_rate(struct clk_hw *hw, unsigned long parent_rate,
>> unsigned int val, const struct clk_div_table *table,
>> - unsigned long flags);
>> + unsigned long flags, unsigned long width);
>> long divider_round_rate_parent(struct clk_hw *hw, struct clk_hw *parent,
>> unsigned long rate, unsigned long *prate,
>> const struct clk_div_table *table,
>> --
>> 2.14.3
>>
>
> --
> Alexandre Belloni, Free Electrons
> Embedded Linux and Kernel engineering
> http://free-electrons.com
^ permalink raw reply
* [PATCH 0/4] vf610-zii-dev updates
From: Russell King - ARM Linux @ 2017-12-22 14:16 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20171220231108.GJ10595@n2100.armlinux.org.uk>
On Wed, Dec 20, 2017 at 11:11:08PM +0000, Russell King - ARM Linux wrote:
> Hi,
>
> These patches update the DT for the ZII VF610 boards.
>
> The first patch fixes complaints at boot about missing DMAs on rev C
> boards, particularly for the SPI interface. This is because edma1 is
> not enabled. This seems to be a regression from the 4.10 era.
>
> The second patch fixes an interrupt storm during boot on rev B boards,
> which causes boot to take 80+ seconds - this seems to be a long
> standing issue since the DT description was first added. The PTB28
> pin is definitely GPIO 98, and GPIO 98 is definitely part of the
> gpio3 block, not the gpio2 block. Since GPIO 66 (which is the
> corresponding GPIO in gpio2) is low, and the IRQ trigger is level-low,
> this causes an interrupt storm.
>
> The last two patches add an explicit description of the PHYs that are
> actually connected to the switch - the 88e1545 is a quad PHY, and
> without describing the MDIO bus, DSA assumes that any PHYs it can
> discover are present for the switch. As only the first three PHYs
> are connected, this leads the 4th port to believe it is connected to
> the 4th PHY when the fixed-link definition is (eventually) removed.
>
> Head this off by providing the proper descriptions, and as we have
> them, also describe the interrupts for these PHYs.
>
> Note, however, that the interrupt description is not quite correct -
> the 88e1545 PHYs all share one interrupt line, and there is a register
> in the PHY which can be used to demux the interrupt to the specific
> PHY. However, in this description, we ignore the demux register, and
> just share the interrupt between the PHYs. That much is fine, but
> the pinmuxing becomes problematical - if we describe the same pinmux
> settings for each PHY for the interrupt line, the 2nd/3rd PHYs fail.
> This has no known solution. Suggestions welcome.
>
> arch/arm/boot/dts/vf610-zii-dev-rev-b.dts | 34 ++++++++++++++++++++++++++++++-
> arch/arm/boot/dts/vf610-zii-dev.dtsi | 4 ++++
> 2 files changed, 37 insertions(+), 1 deletion(-)
There's more stuff that isn't correct in the Vybrid DTS files...
When the SoC was first added, the vfxxx.dtsi had:
+ adc0: adc at 4003b000 {
...
+ status = "disabled";
+ };
+ adc1: adc at 400bb000 {
...
+ status = "disabled";
+ };
This default status remains today.
IIO hwmon support was added later:
+ iio-hwmon {
+ compatible = "iio-hwmon";
+ io-channels = <&adc0 16>, <&adc1 16>;
+ };
which is all fine and dandy, but iio-hwmon fails to probe unless it can
find _all_ the io-channels specified.
Given that the two ADC channels referenced by iio-hwmon default to being
disabled, it makes no sense for iio-hwmon to default to being enabled.
What's more is that if, say, adc0 is enabled by a board, the iio-hwmon
device still fails to be probed because it fails to get adc1.
As I see it, there's two possible solutions to this:
1. remove the default disabled status of the ADCs so both are always
available, or
2. default iio-hwmon to disabled, and provide a label for this device
so that a correct io-channels specification can be suppled for the
board in question, and for iio-hwmon to be enabled where appropriate.
--
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 8.8Mbps down 630kbps up
According to speedtest.net: 8.21Mbps down 510kbps up
^ permalink raw reply
* [PATCH v2] arm64: dts: Hi3660: Fix up psci state id
From: Vincent Guittot @ 2017-12-22 14:22 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1513069954-22827-1-git-send-email-leo.yan@linaro.org>
Hi Leo,
Sorry for jumping late in the discussion but should we also remove
the NAP state from the property cpu-idle-states of the CPUs because
this state not supported by the platform at least for now and may be
not in a near future ?
Then, I have another question regarding the update of the
psci-suspend-parameter. These changes implies an update of the psci
firmawre which means that we will now have 2 different firmware
version compatible with 2 different dt.
Is there any way to check that the ATF on the board is the one that
compatible with the parameter with something like a version ? I
currently use the previous firmware which works fine with current
kernel and dt binding once the NAP state is removed from the table.
When moving on recent kernel, I will have to take care of updating the
firmware and if i need to go back on a previous kernel, i will have to
make sure that i have the right ATF version. This make a lot of chance
of having the wrong configuration
Regards,
Vincent
On 12 December 2017 at 10:12, Leo Yan <leo.yan@linaro.org> wrote:
> Thanks a lot for Vincent Guittot careful work to find bug for 'CPU_NAP'
> idle state. From ftrace log we can observe CA73 CPUs can be easily
> waken up from 'CPU_NAP' state but the 'waken up' CPUs doesn't handle
> anything and sleep again; so there have tons of trace events for CA73
> CPUs entering and exiting idle state.
>
> On Hi3660 CA73 has retention state 'CPU_NAP' for CPU idle, this state we
> set its psci parameter as '0x0000001' and from this parameter it can
> calculate state id is 1. Unfortunately ARM trusted firmware (ARM-TF)
> takes 1 as a invalid value for state id, so the CPU cannot enter idle
> state and directly bail out to kernel.
>
> We want to create good practice for psci parameters platform definition,
> so review the psci specification. The spec "ARM Power State Coordination
> Interface - Platform Design Document (ARM DEN 0022D)" recommends state
> ID in chapter "6.5 Recommended StateID Encoding". The recommended power
> state IDs can be presented by below listed values; and it divides into
> three fields, every field can use 4 bits to present power states
> corresponding to core level, cluster level and system level:
> 0: Run
> 1: Standby
> 2: Retention
> 3: Powerdown
>
> This commit changes psci parameter to compliance with the suggested
> state ID in the doc. Except we change 'CPU_NAP' state psci parameter
> to '0x0000002', this commit also changes 'CPU_SLEEP' and 'CLUSTER_SLEEP'
> state parameters to '0x0010003' and '0x1010033' respectively.
>
> Credits to Daniel, Sudeep and Soby for suggestion and consolidation.
>
> Cc: Vincent Guittot <vincent.guittot@linaro.org>
> Cc: Daniel Lezcano <daniel.lezcano@linaro.org>
> Cc: Sudeep Holla <sudeep.holla@arm.com>
> Cc: Soby Mathew <Soby.Mathew@arm.com>
> Signed-off-by: Leo Yan <leo.yan@linaro.org>
> ---
> arch/arm64/boot/dts/hisilicon/hi3660.dtsi | 8 ++++----
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/arch/arm64/boot/dts/hisilicon/hi3660.dtsi b/arch/arm64/boot/dts/hisilicon/hi3660.dtsi
> index ab0b95b..99d5a46 100644
> --- a/arch/arm64/boot/dts/hisilicon/hi3660.dtsi
> +++ b/arch/arm64/boot/dts/hisilicon/hi3660.dtsi
> @@ -147,7 +147,7 @@
>
> CPU_NAP: cpu-nap {
> compatible = "arm,idle-state";
> - arm,psci-suspend-param = <0x0000001>;
> + arm,psci-suspend-param = <0x0000002>;
> entry-latency-us = <7>;
> exit-latency-us = <2>;
> min-residency-us = <15>;
> @@ -156,7 +156,7 @@
> CPU_SLEEP: cpu-sleep {
> compatible = "arm,idle-state";
> local-timer-stop;
> - arm,psci-suspend-param = <0x0010000>;
> + arm,psci-suspend-param = <0x0010003>;
> entry-latency-us = <40>;
> exit-latency-us = <70>;
> min-residency-us = <3000>;
> @@ -165,7 +165,7 @@
> CLUSTER_SLEEP_0: cluster-sleep-0 {
> compatible = "arm,idle-state";
> local-timer-stop;
> - arm,psci-suspend-param = <0x1010000>;
> + arm,psci-suspend-param = <0x1010033>;
> entry-latency-us = <500>;
> exit-latency-us = <5000>;
> min-residency-us = <20000>;
> @@ -174,7 +174,7 @@
> CLUSTER_SLEEP_1: cluster-sleep-1 {
> compatible = "arm,idle-state";
> local-timer-stop;
> - arm,psci-suspend-param = <0x1010000>;
> + arm,psci-suspend-param = <0x1010033>;
> entry-latency-us = <1000>;
> exit-latency-us = <5000>;
> min-residency-us = <20000>;
> --
> 2.7.4
>
^ permalink raw reply
* [PATCH] dt: psci: Update DT bindings to support hierarchical PSCI states
From: Ulf Hansson @ 2017-12-22 14:32 UTC (permalink / raw)
To: linux-arm-kernel
From: Lina Iyer <lina.iyer@linaro.org>
Update DT bindings to represent hierarchical CPU and CPU domain idle states
for PSCI. Also update the PSCI examples to clearly show how flattened and
hierarchical idle states can be represented in DT.
Signed-off-by: Lina Iyer <lina.iyer@linaro.org>
Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
For your information, I have picked up the work from Lina Iyer around the so
called CPU cluster idling series [1,2] and I working on new versions. However,
I decided to post the updates to the PSCI DT bindings first, as they will be
needed to be agreed upon before further changes can be done to the PSCI firmware
driver.
Note, these bindings have been discussed over and over again, at LKML, but
especially also at various Linux conferences, like LPC and Linaro Connect. We
finally came to a conclusion and the changes we agreed upon, should be reflected
in this update.
Of course, it's a while ago since the latest discussions, but hopefully people
don't have too hard time to remember.
Kind regards
Uffe
[1]
https://www.spinics.net/lists/arm-kernel/msg566200.html
[2]
https://lwn.net/Articles/716300/
---
Documentation/devicetree/bindings/arm/psci.txt | 152 +++++++++++++++++++++++++
1 file changed, 152 insertions(+)
diff --git a/Documentation/devicetree/bindings/arm/psci.txt b/Documentation/devicetree/bindings/arm/psci.txt
index a2c4f1d..5a8f11b 100644
--- a/Documentation/devicetree/bindings/arm/psci.txt
+++ b/Documentation/devicetree/bindings/arm/psci.txt
@@ -105,7 +105,159 @@ Case 3: PSCI v0.2 and PSCI v0.1.
...
};
+PSCI v1.0 onwards, supports OS-Initiated mode for powering off CPUs and CPU
+clusters from the firmware. For such topologies the PSCI firmware driver acts
+as pseudo-controller, which may be specified in the psci DT node. The
+definitions of the CPU and the CPU cluster topology, must conform to the domain
+idle state specification [3]. The domain idle states themselves, must be
+compatible with the defined 'domain-idle-state' binding [1], and also need to
+specify the arm,psci-suspend-param property for each idle state.
+
+DT allows representing CPU and CPU cluster idle states in two different ways -
+
+The flattened model as given in Example 1, lists CPU's idle states followed by
+the domain idle state that the CPUs may choose. This is the general practice
+followed in PSCI firmwares that support Platform Coordinated mode. Note that
+the idle states are all compatible with "arm,idle-state".
+
+Example 2 represents the hierarchical model of CPU and domain idle states.
+CPUs define their domain provider in their DT node. The domain controls the
+power to the CPU and possibly other h/w blocks that would be powered off when
+the CPU is powered off. The CPU's idle states may therefore be considered as
+the domain's idle states and have the compatible "arm,idle-state". Such domains
+may be embedded within another domain that represents common h/w blocks between
+these CPUs viz. the cluster. The idle states of the cluster would be
+represented as the domain's idle states. In order to use OS-Initiated mode of
+PSCI in the firmware, the hierarchical representation must be used.
+
+Example 1: Flattened representation of CPU and domain idle states
+ cpus {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ CPU0: cpu at 0 {
+ device_type = "cpu";
+ compatible = "arm,cortex-a53", "arm,armv8";
+ reg = <0x0>;
+ enable-method = "psci";
+ cpu-idle-states = <&CPU_PWRDN>, <&CLUSTER_RET>,
+ <&CLUSTER_PWR_DWN>;
+ };
+
+ CPU1: cpu at 1 {
+ device_type = "cpu";
+ compatible = "arm,cortex-a57", "arm,armv8";
+ reg = <0x100>;
+ enable-method = "psci";
+ cpu-idle-states = <&CPU_PWRDN>, <&CLUSTER_RET>,
+ <&CLUSTER_PWR_DWN>;
+ };
+
+ idle-states {
+ CPU_PWRDN: cpu_power_down{
+ compatible = "arm,idle-state";
+ arm,psci-suspend-param = <0x000001>;
+ entry-latency-us = <10>;
+ exit-latency-us = <10>;
+ min-residency-us = <100>;
+ };
+
+ CLUSTER_RET: domain_ret {
+ compatible = "arm,idle-state";
+ arm,psci-suspend-param = <0x1000010>;
+ entry-latency-us = <500>;
+ exit-latency-us = <500>;
+ min-residency-us = <2000>;
+ };
+
+ CLUSTER_PWR_DWN: domain_off {
+ compatible = "arm,idle-state";
+ arm,psci-suspend-param = <0x1000030>;
+ entry-latency-us = <2000>;
+ exit-latency-us = <2000>;
+ min-residency-us = <6000>;
+ };
+ };
+
+ psci {
+ compatible = "arm,psci-0.2";
+ method = "smc";
+ };
+
+Example 2: Hierarchical representation of CPU and domain idle states
+
+ cpus {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ CPU0: cpu at 0 {
+ device_type = "cpu";
+ compatible = "arm,cortex-a53", "arm,armv8";
+ reg = <0x0>;
+ enable-method = "psci";
+ power-domains = <&CPU_PD0>;
+ };
+
+ CPU1: cpu at 1 {
+ device_type = "cpu";
+ compatible = "arm,cortex-a57", "arm,armv8";
+ reg = <0x100>;
+ enable-method = "psci";
+ power-domains = <&CPU_PD1>;
+ };
+
+ idle-states {
+ CPU_PWRDN: cpu_power_down{
+ compatible = "arm,idle-state";
+ arm,psci-suspend-param = <0x000001>;
+ entry-latency-us = <10>;
+ exit-latency-us = <10>;
+ min-residency-us = <100>;
+ };
+
+ CLUSTER_RET: domain_ret {
+ compatible = "domain-idle-state";
+ arm,psci-suspend-param = <0x1000010>;
+ entry-latency-us = <500>;
+ exit-latency-us = <500>;
+ min-residency-us = <2000>;
+ };
+
+ CLUSTER_PWR_DWN: domain_off {
+ compatible = "domain-idle-state";
+ arm,psci-suspend-param = <0x1000030>;
+ entry-latency-us = <2000>;
+ exit-latency-us = <2000>;
+ min-residency-us = <6000>;
+ };
+ };
+ };
+
+ psci {
+ compatible = "arm,psci-1.0";
+ method = "smc";
+
+ CPU_PD0: cpu-pd at 0 {
+ #power-domain-cells = <0>;
+ domain-idle-states = <&CPU_PWRDN>;
+ power-domains = <&CLUSTER_PD>;
+ };
+
+ CPU_PD1: cpu-pd at 1 {
+ #power-domain-cells = <0>;
+ domain-idle-states = <&CPU_PWRDN>;
+ power-domains = <&CLUSTER_PD>;
+ };
+
+ CLUSTER_PD: cluster-pd at 0 {
+ #power-domain-cells = <0>;
+ domain-idle-states = <&CLUSTER_RET>, <&CLUSTER_PWR_DWN>;
+ };
+ };
+
[1] Kernel documentation - ARM idle states bindings
Documentation/devicetree/bindings/arm/idle-states.txt
[2] Power State Coordination Interface (PSCI) specification
http://infocenter.arm.com/help/topic/com.arm.doc.den0022c/DEN0022C_Power_State_Coordination_Interface.pdf
+[3]. PM Domains description
+ Documentation/devicetree/bindings/power/power_domain.txt
--
2.7.4
^ permalink raw reply related
* [PATCH v5 3/5] misc serdev: Add w2sg0004 (gps receiver) power control driver
From: H. Nikolaus Schaller @ 2017-12-22 14:40 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20171222124427.GI3374@localhost>
Hi Johan,
> Am 22.12.2017 um 13:44 schrieb Johan Hovold <johan@kernel.org>:
>
> On Fri, Dec 01, 2017 at 08:49:36AM +0100, H. Nikolaus Schaller wrote:
>> Add driver for Wi2Wi W2SG0004/84 GPS module connected to some SoC UART.
>>
>> It uses serdev API hooks to monitor and forward the UART traffic to /dev/ttyGPSn
>> and turn on/off the module. It also detects if the module is turned on (sends data)
>> but should be off, e.g. if it was already turned on during boot or power-on-reset.
>>
>> Additionally, rfkill block/unblock can be used to control an external LNA
>> (and power down the module if not needed).
>>
>> The driver concept is based on code developed by Neil Brown <neilb@suse.de>
>> but simplified and adapted to use the new serdev API introduced in v4.11.
>
> I'm sorry (and I know this discussion has been going on for a long
> time),
I'd say: already too much time.
> but this still feels like too much of a hack.
Well, to me it feels like review process is trying to get things 200% right
in the first step and therefore delays proposals that may already be useful
to real world users. Review is of course important to protect against severe
bugs and security issues.
My concern is that in 5 more years this chip is obsolete and then we never
had a working solution in distributions that base directly on kernel.org...
What I am lacking in this discussion is the spirit of starting with something
that basically works and permanently enhancing things. Like Linux started
25 years ago and many drivers look very different in v4.15 than v2.6.32.
Instead, we are sent back every time to rework it completely and at some point
our enthusiasm (and time budget) has boiled down to 0 because we see no more
reward for working on this.
>
> You're registering a tty driver to allow user space to continue treat
> this as a tty device, but you provide no means of actually modifying
> anything (line settings, etc).
That was planned for a second step but is not important for pure GPS
reception.
> It's essentially just a character device
> with common tty ioctls as noops from a device PoV (well, plus the ldisc
> buffering and processing).
Yes. The buffering is the more important aspect and as far as I understand
we would have to implement our own buffer for a chardev driver. Buffering is
perfectly solved for tty devices.
Another aspect is that the same chip connected through an USB or Bluetooth
connection is presented as a tty device and not a char dev.
> This will probably require someone to first implement a generic gps
> framework with a properly defined interface which you may then teach
> gpsd to use (e.g. to avoid all its autodetection functionality) instead.
Yes, that is what I dream of.
But in past 5 years there was no "someone" to pop up and my time to work
on this topic is too limited. It is not even my main focus of efforts.
So I prefer solutions that can be done today with today's means and not
dreams (even if we share them).
> Or some entirely different approach, for example, where you manage
> everything from user space.
>
> I'd suggest reiterating the problem you're trying to solve and
> enumerating the previously discussed potential solutions in order to
> find a proper abstraction level for this (before getting lost in
> implementation details).
Yes, please feel free to write patches that implement it that way.
>
> That being said, I'm still pointing some bugs and issue below that you
> can consider for future versions of this (and other drivers) below.
Thanks!
>
>> Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
>> ---
>> drivers/misc/Kconfig | 10 +
>> drivers/misc/Makefile | 1 +
>> drivers/misc/w2sg0004.c | 553 ++++++++++++++++++++++++++++++++++++++++++++++++
>> 3 files changed, 564 insertions(+)
>> create mode 100644 drivers/misc/w2sg0004.c
>>
>> diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
>> index f1a5c2357b14..a3b11016ed2b 100644
>> --- a/drivers/misc/Kconfig
>> +++ b/drivers/misc/Kconfig
>> @@ -508,4 +508,14 @@ source "drivers/misc/mic/Kconfig"
>> source "drivers/misc/genwqe/Kconfig"
>> source "drivers/misc/echo/Kconfig"
>> source "drivers/misc/cxl/Kconfig"
>> +
>> +config W2SG0004
>> + tristate "W2SG00x4 on/off control"
>
> Please provide a better summary for what this driver does.
Ok.
>
>> + depends on GPIOLIB && SERIAL_DEV_BUS
>> + help
>> + Enable on/off control of W2SG00x4 GPS moduled connected
s/moduled/module/
>
> Some whitespace issue here.
Ok.
>
>> + to some SoC UART to allow powering up/down if the /dev/ttyGPSn
>> + is opened/closed.
>> + It also provides a rfkill gps name to control the LNA power.
>> +
>> endmenu
>> diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile
>> index 5ca5f64df478..d9d824b3d20a 100644
>> --- a/drivers/misc/Makefile
>> +++ b/drivers/misc/Makefile
>> @@ -50,6 +50,7 @@ obj-$(CONFIG_SRAM_EXEC) += sram-exec.o
>> obj-y += mic/
>> obj-$(CONFIG_GENWQE) += genwqe/
>> obj-$(CONFIG_ECHO) += echo/
>> +obj-$(CONFIG_W2SG0004) += w2sg0004.o
>> obj-$(CONFIG_VEXPRESS_SYSCFG) += vexpress-syscfg.o
>> obj-$(CONFIG_CXL_BASE) += cxl/
>> obj-$(CONFIG_ASPEED_LPC_CTRL) += aspeed-lpc-ctrl.o
>> diff --git a/drivers/misc/w2sg0004.c b/drivers/misc/w2sg0004.c
>> new file mode 100644
>> index 000000000000..6bfd12eb8e02
>> --- /dev/null
>> +++ b/drivers/misc/w2sg0004.c
>> @@ -0,0 +1,553 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Driver for power controlling the w2sg0004/w2sg0084 GPS receiver.
>> + *
>> + * Copyright (C) 2013 Neil Brown <neilb@suse.de>
>> + * Copyright (C) 2015-2017 H. Nikolaus Schaller <hns@goldelico.com>,
>> + * Golden Delicious Computers
>> + *
>> + * This receiver has an ON/OFF pin which must be toggled to
>> + * turn the device 'on' of 'off'. A high->low->high toggle
>
> s/of/or/
Ok.
>
>> + * will switch the device on if it is off, and off if it is on.
>> + *
>> + * To enable receiving on/off requests we register with the
>> + * UART power management notifications.
>
> No, the UART (serial device) would be the grandparent of your serdev
> device (for which you register PM callbacks).
Ah, thanks. This came from original description 5 years ago and if
you work too long on something you do no longer read what is really
written.
>
>> + *
>> + * It is not possible to directly detect the state of the device.
>
> Didn't the 0084 version have a pin for this?
AFAIR no and even if it had, it would not help for the 0004 device
(80% of the GTA04 devices have been built with the 0004 and there is no known
mechanism to detect the chip variant during runtime).
>
>> + * However when it is on it will send characters on a UART line
>> + * regularly.
>> + *
>> + * To detect that the power state is out of sync (e.g. if GPS
>> + * was enabled before a reboot), we register for UART data received
>> + * notifications.
>> + *
>> + * In addition we register as a rfkill client so that we can
>> + * control the LNA power.
>> + *
>> + */
>> +
>> +#include <linux/delay.h>
>> +#include <linux/err.h>
>> +#include <linux/interrupt.h>
>> +#include <linux/irq.h>
>> +#include <linux/module.h>
>> +#include <linux/of.h>
>> +#include <linux/of_irq.h>
>> +#include <linux/of_gpio.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/regulator/consumer.h>
>> +#include <linux/rfkill.h>
>> +#include <linux/serdev.h>
>> +#include <linux/sched.h>
>> +#include <linux/slab.h>
>> +#include <linux/tty.h>
>> +#include <linux/tty_flip.h>
>> +#include <linux/workqueue.h>
>> +
>> +/*
>> + * There seems to be restrictions on how quickly we can toggle the
>> + * on/off line. data sheets says "two rtc ticks", whatever that means.
>> + * If we do it too soon it doesn't work.
>> + * So we have a state machine which uses the common work queue to ensure
>> + * clean transitions.
>> + * When a change is requested we record that request and only act on it
>> + * once the previous change has completed.
>> + * A change involves a 10ms low pulse, and a 990ms raised level, so only
>> + * one change per second.
>> + */
>> +
>> +enum w2sg_state {
>> + W2SG_IDLE, /* is not changing state */
>> + W2SG_PULSE, /* activate on/off impulse */
>> + W2SG_NOPULSE /* deactivate on/off impulse */
>> +};
>> +
>> +struct w2sg_data {
>> + struct rfkill *rf_kill;
>> + struct regulator *lna_regulator;
>> + int lna_blocked; /* rfkill block gps active */
>> + int lna_is_off; /* LNA is currently off */
>> + int is_on; /* current state (0/1) */
>
> These look like flags; use a bitfield rather than an int.
Well, yes. But what does it save? 8 bytes in memory?
If we use char it would not even save anything.
>
>> + unsigned long last_toggle;
>> + unsigned long backoff; /* time to wait since last_toggle */
>> + int on_off_gpio; /* the on-off gpio number */
>> + struct serdev_device *uart; /* uart connected to the chip */
>> + struct tty_driver *tty_drv; /* this is the user space tty */
>
> This does not belong in the device data as someone (Arnd?) already
> pointed out to you in a comment to an earlier version. More below.
>
>> + struct device *dev; /* from tty_port_register_device() */
>> + struct tty_port port;
>> + int open_count; /* how often we were opened */
>> + enum w2sg_state state;
>> + int requested; /* requested state (0/1) */
>> + int suspended;
>> + struct delayed_work work;
>> + int discard_count;
>> +};
>
> You seem to have completely ignored locking. These flags and resources
> are accessed from multiple contexts, and it all looks very susceptible
> to races (e.g. the work queue can race with the rfkill callback and
> leave the regulator enabled when it should be off).
Has never been observed in practise, but you are right to ask to consider.
>
>> +/* should become w2sg_by_minor[n] if we want to support multiple devices */
>> +static struct w2sg_data *w2sg_by_minor;
>> +
>> +static int w2sg_set_lna_power(struct w2sg_data *data)
>> +{
>> + int ret = 0;
>> + int off = data->suspended || !data->requested || data->lna_blocked;
>> +
>> + pr_debug("%s: %s\n", __func__, off ? "off" : "on");
>
> This and other pr_xxx should be replaced with dev_dbg and friends.
Ok.
>
>> +
>> + if (off != data->lna_is_off) {
>> + data->lna_is_off = off;
>> + if (!IS_ERR_OR_NULL(data->lna_regulator)) {
>> + if (off)
>> + regulator_disable(data->lna_regulator);
>> + else
>> + ret = regulator_enable(data->lna_regulator);
>> + }
>> + }
>> +
>> + return ret;
>> +}
>> +
>> +static void w2sg_set_power(void *pdata, int val)
>
> Don't pass around void pointers like this.
Agreed. There might have been a reason years ago...
Maybe it was originally a callback handler with some opaque user-data pointer.
>
>> +{
>> + struct w2sg_data *data = (struct w2sg_data *) pdata;
>> +
>> + pr_debug("%s to state=%d (requested=%d)\n", __func__, val,
>> + data->requested);
>> +
>> + if (val && !data->requested) {
>> + data->requested = true;
>> + } else if (!val && data->requested) {
>> + data->backoff = HZ;
>> + data->requested = false;
>> + } else
>> + return;
>
> Missing brackets.
Ok.
Interestingly there is no warning from checkpatch.
>
>> +
>> + if (!data->suspended)
>> + schedule_delayed_work(&data->work, 0);
>> +}
>> +
>> +/* called each time data is received by the UART (i.e. sent by the w2sg0004) */
>> +static int w2sg_uart_receive_buf(struct serdev_device *serdev,
>> + const unsigned char *rxdata,
>> + size_t count)
>> +{
>> + struct w2sg_data *data =
>> + (struct w2sg_data *) serdev_device_get_drvdata(serdev);
>> +
>> + if (!data->requested && !data->is_on) {
>> + /*
>> + * we have received characters while the w2sg
>> + * should have been be turned off
>> + */
>> + data->discard_count += count;
>> + if ((data->state == W2SG_IDLE) &&
>> + time_after(jiffies,
>> + data->last_toggle + data->backoff)) {
>> + /* Should be off by now, time to toggle again */
>> + pr_debug("w2sg00x4 has sent %d characters data although it should be off!\n",
>> + data->discard_count);
>> +
>> + data->discard_count = 0;
>> +
>> + data->is_on = true;
>> + data->backoff *= 2;
>> + if (!data->suspended)
>> + schedule_delayed_work(&data->work, 0);
>> + }
>> + } else if (data->open_count > 0) {
>> + int n;
>> +
>> + pr_debug("w2sg00x4: push %lu chars to tty port\n",
>> + (unsigned long) count);
>> +
>> + /* pass to user-space */
>> + n = tty_insert_flip_string(&data->port, rxdata, count);
>> + if (n != count)
>> + pr_err("w2sg00x4: did loose %lu characters\n",
>> + (unsigned long) (count - n));
>> + tty_flip_buffer_push(&data->port);
>> + return n;
>> + }
>> +
>> + /* assume we have processed everything */
>> + return count;
>> +}
>> +
>> +/* try to toggle the power state by sending a pulse to the on-off GPIO */
>> +static void toggle_work(struct work_struct *work)
>> +{
>> + struct w2sg_data *data = container_of(work, struct w2sg_data,
>> + work.work);
>> +
>> + switch (data->state) {
>> + case W2SG_IDLE:
>> + if (data->requested == data->is_on)
>> + return;
>> +
>> + w2sg_set_lna_power(data); /* update LNA power state */
>> + gpio_set_value_cansleep(data->on_off_gpio, 0);
>> + data->state = W2SG_PULSE;
>> +
>> + pr_debug("w2sg: power gpio ON\n");
>> +
>> + schedule_delayed_work(&data->work,
>> + msecs_to_jiffies(10));
>> + break;
>> +
>> + case W2SG_PULSE:
>> + gpio_set_value_cansleep(data->on_off_gpio, 1);
>> + data->last_toggle = jiffies;
>> + data->state = W2SG_NOPULSE;
>> + data->is_on = !data->is_on;
>> +
>> + pr_debug("w2sg: power gpio OFF\n");
>> +
>> + schedule_delayed_work(&data->work,
>> + msecs_to_jiffies(10));
>> + break;
>> +
>> + case W2SG_NOPULSE:
>> + data->state = W2SG_IDLE;
>> +
>> + pr_debug("w2sg: idle\n");
>> +
>> + break;
>> +
>> + }
>> +}
>> +
>> +static int w2sg_rfkill_set_block(void *pdata, bool blocked)
>> +{
>> + struct w2sg_data *data = pdata;
>> +
>> + pr_debug("%s: blocked: %d\n", __func__, blocked);
>> +
>> + data->lna_blocked = blocked;
>> +
>> + return w2sg_set_lna_power(data);
>> +}
>> +
>> +static struct rfkill_ops w2sg0004_rfkill_ops = {
>> + .set_block = w2sg_rfkill_set_block,
>> +};
>> +
>> +static struct serdev_device_ops serdev_ops = {
>> + .receive_buf = w2sg_uart_receive_buf,
>> +};
>> +
>> +/*
>> + * we are a man-in the middle between the user-space visible tty port
>> + * and the serdev tty where the chip is connected.
>> + * This allows us to recognise when the device should be powered on
>> + * or off and handle the "false" state that data arrives while no
>> + * users-space tty client exists.
>> + */
>> +
>> +static struct w2sg_data *w2sg_get_by_minor(unsigned int minor)
>> +{
>> + BUG_ON(minor >= 1);
>
> Never use BUG_ON in driver code.
Ok.
>
>> + return w2sg_by_minor;
>> +}
>> +
>> +static int w2sg_tty_install(struct tty_driver *driver, struct tty_struct *tty)
>> +{
>> + struct w2sg_data *data;
>> + int retval;
>> +
>> + pr_debug("%s() tty = %p\n", __func__, tty);
>> +
>> + data = w2sg_get_by_minor(tty->index);
>> +
>> + if (!data)
>> + return -ENODEV;
>> +
>> + retval = tty_standard_install(driver, tty);
>> + if (retval)
>> + goto error_init_termios;
>> +
>> + tty->driver_data = data;
>> +
>> + return 0;
>> +
>> +error_init_termios:
>> + tty_port_put(&data->port);
>
> Where's the corresponding get?
I think we copied gb_tty_install or something alike
http://elixir.free-electrons.com/linux/v4.15-rc4/source/drivers/staging/greybus/uart.c#L393
but that might be buggy or we didn't copy all (tty_port-get) what is needed to make it correct.
>
>> + return retval;
>> +}
>> +
>> +static int w2sg_tty_open(struct tty_struct *tty, struct file *file)
>> +{
>> + struct w2sg_data *data = tty->driver_data;
>> +
>> + pr_debug("%s() data = %p open_count = ++%d\n", __func__,
>> + data, data->open_count);
>> +
>> + w2sg_set_power(data, ++data->open_count > 0);
>> +
>> + return tty_port_open(&data->port, tty, file);
>
> You'd leave the open count incremented on errors here.
Ok.
>
>> +}
>> +
>> +static void w2sg_tty_close(struct tty_struct *tty, struct file *file)
>> +{
>> + struct w2sg_data *data = tty->driver_data;
>> +
>> + pr_debug("%s()\n", __func__);
>> +
>> + w2sg_set_power(data, --data->open_count > 0);
>> +
>> + tty_port_close(&data->port, tty, file);
>> +}
>> +
>> +static int w2sg_tty_write(struct tty_struct *tty,
>> + const unsigned char *buffer, int count)
>> +{
>> + struct w2sg_data *data = tty->driver_data;
>> +
>> + /* simply pass down to UART */
>> + return serdev_device_write_buf(data->uart, buffer, count);
>> +}
>> +
>> +static const struct tty_operations w2sg_serial_ops = {
>> + .install = w2sg_tty_install,
>> + .open = w2sg_tty_open,
>> + .close = w2sg_tty_close,
>> + .write = w2sg_tty_write,
>> +};
>> +
>> +static const struct tty_port_operations w2sg_port_ops = {
>> + /* none defined, but we need the struct */
>> +};
>> +
>> +static int w2sg_probe(struct serdev_device *serdev)
>> +{
>> + struct w2sg_data *data;
>> + struct rfkill *rf_kill;
>> + int err;
>> + int minor;
>> + enum of_gpio_flags flags;
>> +
>> + pr_debug("%s()\n", __func__);
>> +
>> + /*
>> + * For future consideration:
>> + * for multiple such GPS receivers in one system
>> + * we need a mechanism to define distinct minor values
>> + * and search for an unused one.
>> + */
>> + minor = 0;
>> + if (w2sg_get_by_minor(minor)) {
>> + pr_err("w2sg minor is already in use!\n");
>> + return -ENODEV;
>> + }
>> +
>> + data = devm_kzalloc(&serdev->dev, sizeof(*data), GFP_KERNEL);
>> + if (data == NULL)
>> + return -ENOMEM;
>> +
>> + serdev_device_set_drvdata(serdev, data);
>> +
>> + data->on_off_gpio = of_get_named_gpio_flags(serdev->dev.of_node,
>> + "enable-gpios", 0,
>> + &flags);
>
> You should be using gpio descriptors and not the legacy interface.
Ok.
>
>> + /* defer until we have all gpios */
>> + if (data->on_off_gpio == -EPROBE_DEFER)
>> + return -EPROBE_DEFER;
>> +
>> + data->lna_regulator = devm_regulator_get_optional(&serdev->dev,
>> + "lna");
>> + if (IS_ERR(data->lna_regulator)) {
>> + /* defer until we can get the regulator */
>> + if (PTR_ERR(data->lna_regulator) == -EPROBE_DEFER)
>> + return -EPROBE_DEFER;
>> +
>> + data->lna_regulator = NULL;
>> + }
>> + pr_debug("%s() lna_regulator = %p\n", __func__, data->lna_regulator);
>> +
>> + data->lna_blocked = true;
>> + data->lna_is_off = true;
>> +
>> + data->is_on = false;
>> + data->requested = false;
>> + data->state = W2SG_IDLE;
>> + data->last_toggle = jiffies;
>> + data->backoff = HZ;
>> +
>> + data->uart = serdev;
>> +
>> + INIT_DELAYED_WORK(&data->work, toggle_work);
>> +
>> + err = devm_gpio_request(&serdev->dev, data->on_off_gpio,
>> + "w2sg0004-on-off");
>> + if (err < 0)
>> + goto out;
>
> Just return unless you're actually undwinding something.
>
>> +
>> + gpio_direction_output(data->on_off_gpio, false);
>> +
>> + serdev_device_set_client_ops(data->uart, &serdev_ops);
>> + serdev_device_open(data->uart);
>
> Missing error handling.
>
> And always keeping the port open would in most cases prevent the serial
> controller from runtime suspending.
Ok.
>
>> +
>> + serdev_device_set_baudrate(data->uart, 9600);
>> + serdev_device_set_flow_control(data->uart, false);
>
> So you hardcode these settings and provide no means for user space to
> change them. This may make sense for this GPS, but it looks like at
> least some of the wi2wi 0084 modules use 4800 baud ("i"-versions?).
That is new to me that there is an i-version (we don't have it in use).
If you have such a device, please feel free to integrate a dynamic
setting depending on chip variant (e.g. based on compatible string).
>
>> +
>> + rf_kill = rfkill_alloc("GPS", &serdev->dev, RFKILL_TYPE_GPS,
>> + &w2sg0004_rfkill_ops, data);
>> + if (rf_kill == NULL) {
>> + err = -ENOMEM;
>> + goto err_rfkill;
>
> Name error labels after what they do (not where you jump from).
Ok.
>
> That may have prevented the NULL deref you'd trigger in the error path
> here.
>
>> + }
>> +
>> + err = rfkill_register(rf_kill);
>> + if (err) {
>> + dev_err(&serdev->dev, "Cannot register rfkill device\n");
>> + goto err_rfkill;
>> + }
>> +
>> + data->rf_kill = rf_kill;
>> +
>> + /* allocate the tty driver */
>> + data->tty_drv = alloc_tty_driver(1);
>
> As was already pointed out by Arnd in a review of an previous version of
> this series, this must not be done at probe (do it at module init). We
> don't want separate tty drivers for every device.
Looks as if we should use tty_alloc_driver() in addition.
>
>> + if (!data->tty_drv)
>> + return -ENOMEM;
>
> Here you'd leak the registered rfkill structure, and leave the port
> open.
Ok.
>
>> +
>> + /* initialize the tty driver */
>> + data->tty_drv->owner = THIS_MODULE;
>> + data->tty_drv->driver_name = "w2sg0004";
>> + data->tty_drv->name = "ttyGPS";
>
> I don't think you should be claiming this generic namespace for
> something as specific as this.
Suggestion?
>
>> + data->tty_drv->major = 0;
>> + data->tty_drv->minor_start = 0;
>> + data->tty_drv->type = TTY_DRIVER_TYPE_SERIAL;
>> + data->tty_drv->subtype = SERIAL_TYPE_NORMAL;
>> + data->tty_drv->flags = TTY_DRIVER_REAL_RAW | TTY_DRIVER_DYNAMIC_DEV;
>> + data->tty_drv->init_termios = tty_std_termios;
>> + data->tty_drv->init_termios.c_cflag = B9600 | CS8 | CREAD |
>> + HUPCL | CLOCAL;
>> + /*
>> + * optional:
>> + * tty_termios_encode_baud_rate(&data->tty_drv->init_termios,
>> + 115200, 115200);
>> + * w2sg_tty_termios(&data->tty_drv->init_termios);
>> + */
>
> Why is this here?
Was planned to integrate in a second step.
>
>> + tty_set_operations(data->tty_drv, &w2sg_serial_ops);
>> +
>> + /* register the tty driver */
>> + err = tty_register_driver(data->tty_drv);
>> + if (err) {
>> + pr_err("%s - tty_register_driver failed(%d)\n",
>> + __func__, err);
>> + put_tty_driver(data->tty_drv);
>> + goto err_rfkill;
>> + }
>> +
>> + /* minor (0) is now in use */
>> + w2sg_by_minor = data;
>> +
>> + tty_port_init(&data->port);
>> + data->port.ops = &w2sg_port_ops;
>> +
>> + data->dev = tty_port_register_device(&data->port,
>> + data->tty_drv, minor, &serdev->dev);
>
> Missing error handling.
Ok.
>
>> +
>> + /* keep off until user space requests the device */
>> + w2sg_set_power(data, false);
>> +
>> + return 0;
>> +
>> +err_rfkill:
>> + rfkill_destroy(rf_kill);
>> + serdev_device_close(data->uart);
>> +out:
>> + return err;
>> +}
>> +
>> +static void w2sg_remove(struct serdev_device *serdev)
>> +{
>> + struct w2sg_data *data = serdev_device_get_drvdata(serdev);
>> + int minor;
>> +
>> + cancel_delayed_work_sync(&data->work);
>> +
>> + /* what is the right sequence to avoid problems? */
>
> Clearly that's something you need to determine.
Well, I had hoped that the review process helps us here.
I do not have the same deep understanding of tty and serdev as all reviewers.
And there isn't much documentation beyond the code.
So in other words: I have no idea how to determine.
>
>> + serdev_device_close(data->uart);
>> +
>> + /* we should lookup in w2sg_by_minor */
>> + minor = 0;
>> + tty_unregister_device(data->tty_drv, minor);
>> +
>> + tty_unregister_driver(data->tty_drv);
>> +}
>> +
>> +static int __maybe_unused w2sg_suspend(struct device *dev)
>> +{
>> + struct w2sg_data *data = dev_get_drvdata(dev);
>> +
>> + data->suspended = true;
>> +
>> + cancel_delayed_work_sync(&data->work);
>> +
>> + w2sg_set_lna_power(data); /* shuts down if needed */
>> +
>> + if (data->state == W2SG_PULSE) {
>> + msleep(10);
>> + gpio_set_value_cansleep(data->on_off_gpio, 1);
>> + data->last_toggle = jiffies;
>> + data->is_on = !data->is_on;
>> + data->state = W2SG_NOPULSE;
>> + }
>> +
>> + if (data->state == W2SG_NOPULSE) {
>> + msleep(10);
>> + data->state = W2SG_IDLE;
>> + }
>> +
>> + if (data->is_on) {
>> + pr_info("GPS off for suspend %d %d %d\n", data->requested,
>> + data->is_on, data->lna_is_off);
>> +
>> + gpio_set_value_cansleep(data->on_off_gpio, 0);
>> + msleep(10);
>> + gpio_set_value_cansleep(data->on_off_gpio, 1);
>> + data->is_on = 0;
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +static int __maybe_unused w2sg_resume(struct device *dev)
>> +{
>> + struct w2sg_data *data = dev_get_drvdata(dev);
>> +
>> + data->suspended = false;
>> +
>> + pr_info("GPS resuming %d %d %d\n", data->requested,
>> + data->is_on, data->lna_is_off);
>> +
>> + schedule_delayed_work(&data->work, 0); /* enables LNA if needed */
>> +
>> + return 0;
>> +}
>> +
>> +static const struct of_device_id w2sg0004_of_match[] = {
>> + { .compatible = "wi2wi,w2sg0004" },
>> + { .compatible = "wi2wi,w2sg0084" },
>> + {},
>> +};
>> +MODULE_DEVICE_TABLE(of, w2sg0004_of_match);
>> +
>> +SIMPLE_DEV_PM_OPS(w2sg_pm_ops, w2sg_suspend, w2sg_resume);
>> +
>> +static struct serdev_device_driver w2sg_driver = {
>> + .probe = w2sg_probe,
>> + .remove = w2sg_remove,
>> + .driver = {
>> + .name = "w2sg0004",
>> + .owner = THIS_MODULE,
>> + .pm = &w2sg_pm_ops,
>> + .of_match_table = of_match_ptr(w2sg0004_of_match)
>> + },
>> +};
>> +
>> +module_serdev_device_driver(w2sg_driver);
>> +
>> +MODULE_AUTHOR("NeilBrown <neilb@suse.de>");
>> +MODULE_AUTHOR("H. Nikolaus Schaller <hns@goldelico.com>");
>> +MODULE_DESCRIPTION("w2sg0004 GPS power management driver");
>> +MODULE_LICENSE("GPL v2");
>
> Johan
It looks as if you understand much better than us how this driver
should be written.
So, I would be happy if you take over development based on what we
suggest.
Please feel free to modify it as you think it is right. I can then
test your version on our hardware and report issues.
BR and thanks,
Nikolaus Schaller
^ permalink raw reply
* [PATCH v5 5/5] misc serdev: w2sg0004: add debugging code and Kconfig
From: H. Nikolaus Schaller @ 2017-12-22 14:41 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20171222125150.GJ3374@localhost>
> Am 22.12.2017 um 13:51 schrieb Johan Hovold <johan@kernel.org>:
>
> On Fri, Dec 01, 2017 at 08:49:38AM +0100, H. Nikolaus Schaller wrote:
>> This allows to set CONFIG_W2SG0004_DEBUG which will
>> make the driver report more activities and it will turn on the
>> GPS module during boot while the driver assumes that it
>> is off. This can be used to debug the correct functioning of
>> the hardware. Therefore we add it as an option to the driver
>> because we think it is of general use (and a little tricky to
>> add by system testers).
>>
>> Normally it should be off.
>>
>> Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
>> ---
>> drivers/misc/Kconfig | 8 ++++++++
>> drivers/misc/w2sg0004.c | 37 +++++++++++++++++++++++++++++++++++++
>> 2 files changed, 45 insertions(+)
>
> I'd say say this does not belong in the kernel at all.
There are other examples of such DEBUG options for drivers.
E.g. CONFIG_LIBERTAS_DEBUG.
It helps the hardware developer to test things and should of course
be disabled in a production kernel.
During hardware bringup I am always happy if someone else has shared
such tools instead of omitting them. So that we do not have to invent
them (again) and hack into our own kernel.
Of course, this should be discussed and you are open to take it
or leave it.
> And even if the
> power-state test were to be allowed, most of the pr_debugs would
> need to go.
I see.
"/* If you are writing a driver, please use dev_dbg instead */"
Well, nobody actively reads this comment in the header file...
Maybe checkpatch could be teached to warn?
> You really should be using dev_dbg and friends, which can
> already be enabled selectively at run time using dynamic debugging.
Well, in this case it is not needed to dynamically turn it on/off.
BR and thanks,
Nikolaus Schaller
^ permalink raw reply
* [PATCH v5 5/5] misc serdev: w2sg0004: add debugging code and Kconfig
From: Greg Kroah-Hartman @ 2017-12-22 14:58 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <A2720858-5172-4627-8133-C677E4B4DC72@goldelico.com>
On Fri, Dec 22, 2017 at 03:41:24PM +0100, H. Nikolaus Schaller wrote:
>
> > Am 22.12.2017 um 13:51 schrieb Johan Hovold <johan@kernel.org>:
> >
> > On Fri, Dec 01, 2017 at 08:49:38AM +0100, H. Nikolaus Schaller wrote:
> >> This allows to set CONFIG_W2SG0004_DEBUG which will
> >> make the driver report more activities and it will turn on the
> >> GPS module during boot while the driver assumes that it
> >> is off. This can be used to debug the correct functioning of
> >> the hardware. Therefore we add it as an option to the driver
> >> because we think it is of general use (and a little tricky to
> >> add by system testers).
> >>
> >> Normally it should be off.
> >>
> >> Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
> >> ---
> >> drivers/misc/Kconfig | 8 ++++++++
> >> drivers/misc/w2sg0004.c | 37 +++++++++++++++++++++++++++++++++++++
> >> 2 files changed, 45 insertions(+)
> >
> > I'd say say this does not belong in the kernel at all.
>
> There are other examples of such DEBUG options for drivers.
> E.g. CONFIG_LIBERTAS_DEBUG.
And that is wrong and should be removed.
> It helps the hardware developer to test things and should of course
> be disabled in a production kernel.
dev_dbg() should be used instead. And you can always hard-code #define DEBUG
at the top of your file when doing bringup.
thanks,
greg k-h
^ permalink raw reply
* [PATCH 06/10] arm64: handle 52-bit physical addresses in page table entries
From: Catalin Marinas @ 2017-12-22 15:04 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <a0a81fd9-e11c-bd85-5484-48b2e638c65a@arm.com>
On Mon, Dec 18, 2017 at 04:36:46PM +0000, Suzuki K. Poulose wrote:
> On 13/12/17 17:07, Kristina Martsenko wrote:
> > @@ -427,7 +451,7 @@ static inline phys_addr_t pmd_page_paddr(pmd_t pmd)
> > #define pte_set_fixmap_offset(pmd, addr) pte_set_fixmap(pte_offset_phys(pmd, addr))
> > #define pte_clear_fixmap() clear_fixmap(FIX_PTE)
> > -#define pmd_page(pmd) pfn_to_page(__phys_to_pfn(pmd_val(pmd) & PHYS_MASK))
> > +#define pmd_page(pmd) pfn_to_page(__phys_to_pfn(__pmd_to_phys(pmd)))
>
> You could simplify the above to :
>
> #define pmd_page(pmd) pfn_to_page(pmd_pfn(pmd))
>
> ? similarly for pud_page.
Just for the record as we already discussed: pmd_pfn(pmd) gives a
different result from __phys_to_pfn(__pmd_to_phys(pmd)). Weird but
pmd_pfn() is used with huge page stuff and it ensures that the pfn
corresponds to a PMD_SIZE-aligned page while pmd_page() is used to
retrieve the page of the pte pointed at by the pmd, so only PAGE_SIZE
aligned.
--
Catalin
^ permalink raw reply
* [PATCH 00/14] iio: triggers: add consumer support
From: Eugen Hristev @ 2017-12-22 15:07 UTC (permalink / raw)
To: linux-arm-kernel
Hello,
I would like to introduce his patch series which implements:
- new iio channel IIO_POSITION
- new inkern API to expose iio triggers to consumer drivers
- new bindings for trigger consumer drivers in device tree
- new input touchscreen driver for SAMA5D2
- implementation of POSITION and PRESSURE channels in sama5d2_adc driver
- devicetree changes for sama5d2 soc w.r.t. resistive touchscreen.
The SAMA5D2 SOC contains the ADC IP which has support
for a resistive touchscreen inside, using channels 0,1,2,3 from the ADC.
As discussed earlier on the mailing list, here is an implementation based
on the fact that the IIO device can expose the trigger and the required
channels to another driver (in our case the input touchscreen driver).
This touchscreen driver will consume the trigger and the values ( by
feeding them to the input subsystem).
Design decisions: The touchscreen driver will do a successful probe at
all times. Only on touch open and close, it will connect to the ADC device.
This is done to avoid a hard dependency between loading the ADC module and
the touchscreen one. So probe will always succeed and the dependency
is only soft.
Below it's an explanation of each patch in the series. Some changes were
required in the inkern API and the implementation was done in the ADC driver
and the touchscreen driver.
1: Added maintainers entry for new driver. This is done in commit:
MAINTAINERS: add sama5d2 resistive touchscreen
2: First we need a new set of channels named Position to report from the IIO
device the position on the touchpad. This is done in commit :
iio: Add channel for Position
3: we need device tree connection between the trigger producer and the
trigger consumer. I made new bindings very similar with the ones for the IIO
channels. This is done in the commit:
dt-bindings: iio: add binding support for iio trigger
provider/consumer
4: we need bindings for the input touchscreen driver, named sama5d2_rts
(resistive touch screen). This driver will be the consumer for the IIO
trigger and channels. This is done in the commit:
dt-bindings: input: touchscreen: sama5d2_rts: create bindings
5: we need some helper functions to get the trigger from the iio device
from inkern API. This is done in the commit:
iio: triggers: add helper function to retrieve trigger
6: we need helper functions to attach and detach a pollfunction to and
from the trigger itself. The trigger is registered by an IIO device, and
is still connected to it, but we need to be able to register a different
pollfunc from another driver. Thus I exposed this API to be inkern API.
This is done in commit:
iio: triggers: expose attach and detach helpers for pollfuncs
7: we need to be able to attach a pollfunc to a trigger, even if the
pollfunc is not coming from an iio device. Thus, register the pollfunc using
the iio_dev of the trigger (in case it's coming as NULL from the pollfunc).
This is done in the commit:
iio: triggers: on pollfunc attach, complete iio_dev if NULL
8: we need a private data on the pollfunc, such that the consumer
driver has a pointer to it's private data when the handler is called.
This is done in the commit:
iio: triggers: add private data to pollfuncs
9: we need to have inkern API to be able to look inside the devicetree
and find from the properties, which trigger(s) need to be connected.
Created some API that will look through the properties and return found
triggers. This is done in the commit:
iio: inkern: triggers: create helpers for OF trigger retrieval
10: in at91-sama5d2_adc we need to remove trigger on module remove in
order to not have stray triggers that are unusable after the module is
removed and reinserted. If such triggers stay registered, the code that
looks through the list of triggers will not work as intended.
This is done in commit:
iio: adc: at91-sama5d2_adc: force trigger removal on module remove
11: we need consecutive indexes for the channels provided by at91-sama5d2_adc
so that we optimize the index values and easier to connect the
touchscreen driver to the channels as a consumer. This is done in the commit:
iio: adc: at91-sama5d2_adc: optimize scan index for diff channels
12: the implementation of the channels and the support for position
conversion and pressure is done in the at91-sama5d2_adc driver.
The implementation provides a trigger named "touch" which can be
connected to a consumer driver.
Once a driver connects and attaches a pollfunc to this trigger, the
configure trigger callback is called, and then the ADC driver will
initialize pad measurement.
First step is to enable touchscreen 4wire support and enable
pen detect IRQ.
Once a pen is detected, a periodic trigger is setup to trigger every 2 ms
(e.g.) and sample the resistive touchscreen values. The trigger poll
is called, and the consumer driver is then woke up, and it can read the
respective channels for the values : X, and Y for position and pressure
channel.
Because only one trigger can be active in hardware in the same time,
while touching the pad, the ADC will block any attempt to use the
triggered buffer. Same, conversions using the software trigger are also
impossible (since the periodic trigger is setup).
If some driver wants to attach while the trigger is in use, it will also fail.
Once the pen is not detected anymore, the trigger is free for use (hardware
or software trigger, with or without DMA).
Channels 0,1,2 and 3 are unavailable if a touchscreen is enabled.
This is done in the commit:
iio: adc: at91-sama5d2_adc: support for position and pressure channels
13: the implementation of the basic touchscreen driver, which is an IIO
consumer.
The driver registers an input device and connects to the given IIO device from
devicetree. It requires an IIO trigger (acting as a consumer) and three IIO
channels : one for X position, one for Y position and one for pressure.
It the reports the values to the input subsystem.
This is done in the commit:
input: touchscreen: sama5d2_rts: SAMA5D2 Resistive touchscreen driver
14: Finally, we need to have the node in the sama5d2.dtsi,
as it's part of the SoC. This is done in the commit:
ARM: dts: at91: sama5d2: Add resistive touch device
This series is based on the discussion in mailing list and probably requires
some patching up, but hopefully the idea and the implementation of the inkern
trigger consumers and the splitting between the ADC driver and the input
driver is ok like this.
The implementation avoids using a MFD driver, which means to make three
drivers in fact, one for ADC, one for touch, with a shared IRQ, and a general
MFD driver that will be a placeholder for the two.
This implementation is based on patches initially written by
Swamy Bandaru Venkateshwara and Mohamed Jamsheeth Hajanajubudeen
Eugen Hristev (14):
MAINTAINERS: add sama5d2 resistive touchscreen
iio: Add channel for Position
dt-bindings: iio: add binding support for iio trigger
provider/consumer
dt-bindings: input: touchscreen: sama5d2_rts: create bindings
iio: triggers: add helper function to retrieve trigger
iio: triggers: expose attach and detach helpers for pollfuncs
iio: triggers: on pollfunc attach, complete iio_dev if NULL
iio: triggers: add private data to pollfuncs
iio: inkern: triggers: create helpers for OF trigger retrieval
iio: adc: at91-sama5d2_adc: force trigger removal on module remove
iio: adc: at91-sama5d2_adc: optimize scan index for diff channels
iio: adc: at91-sama5d2_adc: support for position and pressure channels
input: touchscreen: sama5d2_rts: SAMA5D2 Resistive touchscreen driver
ARM: dts: at91: sama5d2: Add resistive touch device
Documentation/ABI/testing/sysfs-bus-iio | 11 +
.../devicetree/bindings/iio/iio-bindings.txt | 52 ++-
.../bindings/input/touchscreen/sama5d2_rts.txt | 31 ++
MAINTAINERS | 6 +
arch/arm/boot/dts/sama5d2.dtsi | 12 +-
drivers/iio/adc/at91-sama5d2_adc.c | 460 ++++++++++++++++++++-
drivers/iio/iio_core_trigger.h | 21 +
drivers/iio/industrialio-core.c | 1 +
drivers/iio/industrialio-trigger.c | 42 +-
drivers/iio/inkern.c | 91 ++++
drivers/input/touchscreen/Kconfig | 13 +
drivers/input/touchscreen/Makefile | 1 +
drivers/input/touchscreen/sama5d2_rts.c | 287 +++++++++++++
include/linux/iio/trigger_consumer.h | 21 +
include/uapi/linux/iio/types.h | 1 +
tools/iio/iio_event_monitor.c | 2 +
16 files changed, 1036 insertions(+), 16 deletions(-)
create mode 100644 Documentation/devicetree/bindings/input/touchscreen/sama5d2_rts.txt
create mode 100644 drivers/input/touchscreen/sama5d2_rts.c
--
2.7.4
^ permalink raw reply
* [PATCH 01/14] MAINTAINERS: add sama5d2 resistive touchscreen
From: Eugen Hristev @ 2017-12-22 15:07 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1513955241-10985-1-git-send-email-eugen.hristev@microchip.com>
Add MAINTAINERS entry for sama5d2 resistive touchscreen driver
Signed-off-by: Eugen Hristev <eugen.hristev@microchip.com>
---
MAINTAINERS | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/MAINTAINERS b/MAINTAINERS
index 650aa0e..8beec28 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -8957,6 +8957,12 @@ F: drivers/media/platform/atmel/atmel-isc.c
F: drivers/media/platform/atmel/atmel-isc-regs.h
F: devicetree/bindings/media/atmel-isc.txt
+MICROCHIP / ATMEL SAMA5D2 RESISTIVE TOUCHSCREEN DRIVER
+M: Eugen Hristev <eugen.hristev@microchip.com>
+L: linux-input at vger.kernel.org
+S: Maintained
+F: drivers/input/touchscreen/sama5d2_rts.c
+
MICROCHIP KSZ SERIES ETHERNET SWITCH DRIVER
M: Woojung Huh <Woojung.Huh@microchip.com>
M: Microchip Linux Driver Support <UNGLinuxDriver@microchip.com>
--
2.7.4
^ permalink raw reply related
* [PATCH 02/14] iio: Add channel for Position
From: Eugen Hristev @ 2017-12-22 15:07 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1513955241-10985-1-git-send-email-eugen.hristev@microchip.com>
Add new channel type for position on a pad.
These type of analog sensor represents the position of a pen
on a touchpad, and is represented as a voltage, which can be
converted to a position on X and Y axis on the pad.
The channel can then be consumed by a touchscreen driver or
read as-is for a raw indication of the touchpen on a touchpad.
Signed-off-by: Eugen Hristev <eugen.hristev@microchip.com>
---
Documentation/ABI/testing/sysfs-bus-iio | 11 +++++++++++
drivers/iio/industrialio-core.c | 1 +
include/uapi/linux/iio/types.h | 1 +
tools/iio/iio_event_monitor.c | 2 ++
4 files changed, 15 insertions(+)
diff --git a/Documentation/ABI/testing/sysfs-bus-iio b/Documentation/ABI/testing/sysfs-bus-iio
index a478740..d2b9e2f 100644
--- a/Documentation/ABI/testing/sysfs-bus-iio
+++ b/Documentation/ABI/testing/sysfs-bus-iio
@@ -190,6 +190,17 @@ Description:
but should match other such assignments on device).
Units after application of scale and offset are m/s^2.
+What: /sys/bus/iio/devices/iio:deviceX/in_position_x_raw
+What: /sys/bus/iio/devices/iio:deviceX/in_position_y_raw
+KernelVersion: 4.16
+Contact: linux-iio at vger.kernel.org
+Description:
+ Position in direction x or y on a pad (may be arbitrarily
+ assigned but should match other such assignments on device).
+ Units after application of scale and offset are millipercents
+ from the pad's size in both directions. Should be calibrated by
+ the consumer.
+
What: /sys/bus/iio/devices/iio:deviceX/in_anglvel_x_raw
What: /sys/bus/iio/devices/iio:deviceX/in_anglvel_y_raw
What: /sys/bus/iio/devices/iio:deviceX/in_anglvel_z_raw
diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
index 2e8e36f..a4fa49b 100644
--- a/drivers/iio/industrialio-core.c
+++ b/drivers/iio/industrialio-core.c
@@ -85,6 +85,7 @@ static const char * const iio_chan_type_name_spec[] = {
[IIO_COUNT] = "count",
[IIO_INDEX] = "index",
[IIO_GRAVITY] = "gravity",
+ [IIO_POSITION] = "position",
};
static const char * const iio_modifier_names[] = {
diff --git a/include/uapi/linux/iio/types.h b/include/uapi/linux/iio/types.h
index 4213cdf..35e17da 100644
--- a/include/uapi/linux/iio/types.h
+++ b/include/uapi/linux/iio/types.h
@@ -44,6 +44,7 @@ enum iio_chan_type {
IIO_COUNT,
IIO_INDEX,
IIO_GRAVITY,
+ IIO_POSITION,
};
enum iio_modifier {
diff --git a/tools/iio/iio_event_monitor.c b/tools/iio/iio_event_monitor.c
index b61245e..0c2b317 100644
--- a/tools/iio/iio_event_monitor.c
+++ b/tools/iio/iio_event_monitor.c
@@ -58,6 +58,7 @@ static const char * const iio_chan_type_name_spec[] = {
[IIO_PH] = "ph",
[IIO_UVINDEX] = "uvindex",
[IIO_GRAVITY] = "gravity",
+ [IIO_POSITION] = "position",
};
static const char * const iio_ev_type_text[] = {
@@ -151,6 +152,7 @@ static bool event_is_known(struct iio_event_data *event)
case IIO_PH:
case IIO_UVINDEX:
case IIO_GRAVITY:
+ case IIO_POSITION:
break;
default:
return false;
--
2.7.4
^ permalink raw reply related
* [PATCH 03/14] dt-bindings: iio: add binding support for iio trigger provider/consumer
From: Eugen Hristev @ 2017-12-22 15:07 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1513955241-10985-1-git-send-email-eugen.hristev@microchip.com>
Add bindings for producer/consumer for iio triggers.
Similar with iio channels, the iio triggers can be connected between drivers:
one driver will be a producer by registering iio triggers, and another driver
will connect as a consumer.
Signed-off-by: Eugen Hristev <eugen.hristev@microchip.com>
---
.../devicetree/bindings/iio/iio-bindings.txt | 52 +++++++++++++++++++++-
1 file changed, 51 insertions(+), 1 deletion(-)
diff --git a/Documentation/devicetree/bindings/iio/iio-bindings.txt b/Documentation/devicetree/bindings/iio/iio-bindings.txt
index 68d6f8c..d861f0df 100644
--- a/Documentation/devicetree/bindings/iio/iio-bindings.txt
+++ b/Documentation/devicetree/bindings/iio/iio-bindings.txt
@@ -11,6 +11,10 @@ value of a #io-channel-cells property in the IIO provider node.
[1] http://marc.info/?l=linux-iio&m=135902119507483&w=2
+Moreover, the provider can have a set of triggers that can be attached to
+from the consumer drivers.
+
+
==IIO providers==
Required properties:
@@ -18,6 +22,11 @@ Required properties:
with a single IIO output and 1 for nodes with multiple
IIO outputs.
+Optional properties:
+#io-trigger-cells: Number of cells for the IIO trigger specifier. Typically 0
+ for nodes with a single IIO trigger and 1 for nodes with
+ multiple IIO triggers.
+
Example for a simple configuration with no trigger:
adc: voltage-sensor at 35 {
@@ -26,7 +35,7 @@ Example for a simple configuration with no trigger:
#io-channel-cells = <1>;
};
-Example for a configuration with trigger:
+Example for a configuration with channels provided by trigger:
adc at 35 {
compatible = "some-vendor,some-adc";
@@ -42,6 +51,17 @@ Example for a configuration with trigger:
};
};
+Example for a configuration for a trigger provider:
+
+ adc: sensor-with-trigger at 35 {
+ compatible = "some-vendor,some-adc";
+ reg = <0x35>;
+ #io-channel-cells = <1>;
+ #io-trigger-cells = <1>;
+ /* other properties */
+ };
+
+
==IIO consumers==
Required properties:
@@ -61,16 +81,38 @@ io-channel-ranges:
IIO channels from this node. Useful for bus nodes to provide
and IIO channel to their children.
+io-triggers: List of phandle and IIO specifier pairs, one pair
+ for each trigger input to the device. Note: if the
+ IIO trigger provider specifies '0' for #io-trigger-cells,
+ then only the phandle portion of the pair will appear.
+
+io-trigger-names:
+ List of IIO trigger input name strings sorted in the same
+ order as the io-triggers property. Consumers drivers
+ will use io-trigger-names to match IIO trigger input names
+ with IIO specifiers.
+
+io-trigger-ranges:
+ Empty property indicating that child nodes can inherit named
+ IIO triggers from this node. Useful for bus nodes to provide
+ IIO triggers to their children.
+
For example:
device {
io-channels = <&adc 1>, <&ref 0>;
io-channel-names = "vcc", "vdd";
+ io-triggers = <&adc 0>, <&adc 1>;
+ io-trigger-names = "continuous", "external";
};
This represents a device with two IIO inputs, named "vcc" and "vdd".
The vcc channel is connected to output 1 of the &adc device, and the
vdd channel is connected to output 0 of the &ref device.
+The device also connects to two IIO trigger inputs, named "continuous" and
+"external". The continuous trigger is connected to the trigger output 0 of the
+&adc device, and the external trigger is connected to the trigger output 1 of
+the same &adc device.
==Example==
@@ -78,6 +120,7 @@ vdd channel is connected to output 0 of the &ref device.
compatible = "maxim,max1139";
reg = <0x35>;
#io-channel-cells = <1>;
+ #io-trigger-cells = <1>;
};
...
@@ -94,4 +137,11 @@ vdd channel is connected to output 0 of the &ref device.
compatible = "some-consumer";
io-channels = <&adc 10>, <&adc 11>;
io-channel-names = "adc1", "adc2";
+ io-triggers = <&adc 0>;
+ };
+
+ some_other_consumer {
+ compatible = "some-other-consumer";
+ io-triggers = <&adc 1>, <&adc 2>;
+ io-trigger-names = "edge-triggered", "pwm-generated";
};
--
2.7.4
^ permalink raw reply related
* [PATCH 04/14] dt-bindings: input: touchscreen: sama5d2_rts: create bindings
From: Eugen Hristev @ 2017-12-22 15:07 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1513955241-10985-1-git-send-email-eugen.hristev@microchip.com>
Added bindings for Microchip SAMA5D2 resistive touchscreen.
Signed-off-by: Eugen Hristev <eugen.hristev@microchip.com>
---
.../bindings/input/touchscreen/sama5d2_rts.txt | 31 ++++++++++++++++++++++
1 file changed, 31 insertions(+)
create mode 100644 Documentation/devicetree/bindings/input/touchscreen/sama5d2_rts.txt
diff --git a/Documentation/devicetree/bindings/input/touchscreen/sama5d2_rts.txt b/Documentation/devicetree/bindings/input/touchscreen/sama5d2_rts.txt
new file mode 100644
index 0000000..ea52a5c
--- /dev/null
+++ b/Documentation/devicetree/bindings/input/touchscreen/sama5d2_rts.txt
@@ -0,0 +1,31 @@
+Microchip SAMA5D2 resistive touchscreen
+
+Required properties:
+
+ - compatible: microchip,sama5d2-resistive-touch
+
+Optional properties:
+
+The device must be connected to an IIO device that provides channels for
+position and pressure measurement, and a trigger output that will periodically
+trigger when the touch is pressed.
+Refer to ../iio/iio-bindings.txt for details
+
+ - io-channels: can have two or three channels connected to an IIO device.
+These should correspond to the channels exposed by the IIO device and should
+have the right index as the ADC registers them.
+ - io-channel-names: must have the two or three channels' names :
+"x", "y", or "x", "y" and "pressure"
+ - io-triggers: can have a trigger input connected to an IIO device.
+ - microchip,pressure-threshold: a pressure threshold for the touchscreen.
+ Represented by an integer value.
+
+Example:
+
+ resistive_touch: resistive-touch {
+ compatible = "microchip,sama5d2-resistive-touch";
+ microchip,pressure-threshold = <3000>;
+ io-channels = <&adc 19>, <&adc 20>, <&adc 21>;
+ io-channel-names = "x", "y", "pressure";
+ io-triggers = <&adc 1>;
+ };
--
2.7.4
^ permalink raw reply related
* [PATCH 05/14] iio: triggers: add helper function to retrieve trigger
From: Eugen Hristev @ 2017-12-22 15:07 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1513955241-10985-1-git-send-email-eugen.hristev@microchip.com>
Add a helper function that will retrieve a trigger by index
from a device. This is intended to be used in trigger consumer drivers.
Signed-off-by: Eugen Hristev <eugen.hristev@microchip.com>
---
drivers/iio/iio_core_trigger.h | 21 +++++++++++++++++++++
drivers/iio/industrialio-trigger.c | 23 +++++++++++++++++++++++
2 files changed, 44 insertions(+)
diff --git a/drivers/iio/iio_core_trigger.h b/drivers/iio/iio_core_trigger.h
index 1fdb1e4..14c4253 100644
--- a/drivers/iio/iio_core_trigger.h
+++ b/drivers/iio/iio_core_trigger.h
@@ -21,6 +21,15 @@ void iio_device_register_trigger_consumer(struct iio_dev *indio_dev);
**/
void iio_device_unregister_trigger_consumer(struct iio_dev *indio_dev);
+/**
+ * iio_trigger_find_from_device() - get the trigger from the device having the
+ * index given.
+ * @indio_dev: iio_dev where to get the trigger from
+ * @index: the index of the trigger to retrieve
+ **/
+__maybe_unused struct iio_trigger *
+iio_trigger_find_from_device(struct iio_dev *indio_dev, u32 index);
+
#else
/**
@@ -40,4 +49,16 @@ static void iio_device_unregister_trigger_consumer(struct iio_dev *indio_dev)
{
}
+/**
+ * iio_trigger_find_from_device() - get the trigger from the device having the
+ * index given.
+ * @indio_dev: iio_dev where to get the trigger from
+ * @index: the index of the trigger to retrieve
+ **/
+static __maybe_unused struct iio_trigger *
+iio_trigger_find_from_device(struct iio_dev *indio_dev, u32 index)
+{
+ return NULL;
+}
+
#endif /* CONFIG_TRIGGER_CONSUMER */
diff --git a/drivers/iio/industrialio-trigger.c b/drivers/iio/industrialio-trigger.c
index ce66699..cbaa079 100644
--- a/drivers/iio/industrialio-trigger.c
+++ b/drivers/iio/industrialio-trigger.c
@@ -134,6 +134,29 @@ int iio_trigger_set_immutable(struct iio_dev *indio_dev, struct iio_trigger *tri
}
EXPORT_SYMBOL(iio_trigger_set_immutable);
+/* Find the trigger from the given device corresponding to given index */
+struct __maybe_unused iio_trigger *
+iio_trigger_find_from_device(struct iio_dev *indio_dev, u32 index)
+{
+ struct iio_trigger *iter, *trig = NULL;
+ u32 i = 0;
+
+ mutex_lock(&iio_trigger_list_lock);
+
+ list_for_each_entry(iter, &iio_trigger_list, list) {
+ if (!iio_trigger_validate_own_device(iter, indio_dev)) {
+ if (i == index) {
+ trig = iter;
+ break;
+ }
+ i++;
+ }
+ }
+ mutex_unlock(&iio_trigger_list_lock);
+
+ return trig;
+}
+
/* Search for trigger by name, assuming iio_trigger_list_lock held */
static struct iio_trigger *__iio_trigger_find_by_name(const char *name)
{
--
2.7.4
^ permalink raw reply related
* [PATCH 06/14] iio: triggers: expose attach and detach helpers for pollfuncs
From: Eugen Hristev @ 2017-12-22 15:07 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1513955241-10985-1-git-send-email-eugen.hristev@microchip.com>
Make attach and detach functions for pollfuncs available as symbols.
These functions are required in the trigger consumer drivers in order
to register their own pollfunctions to iio devices.
Signed-off-by: Eugen Hristev <eugen.hristev@microchip.com>
---
drivers/iio/industrialio-trigger.c | 10 ++++++----
include/linux/iio/trigger_consumer.h | 9 +++++++++
2 files changed, 15 insertions(+), 4 deletions(-)
diff --git a/drivers/iio/industrialio-trigger.c b/drivers/iio/industrialio-trigger.c
index cbaa079..8565c92 100644
--- a/drivers/iio/industrialio-trigger.c
+++ b/drivers/iio/industrialio-trigger.c
@@ -265,8 +265,8 @@ static void iio_trigger_put_irq(struct iio_trigger *trig, int irq)
* the relevant function is in there may be the best option.
*/
/* Worth protecting against double additions? */
-static int iio_trigger_attach_poll_func(struct iio_trigger *trig,
- struct iio_poll_func *pf)
+int iio_trigger_attach_poll_func(struct iio_trigger *trig,
+ struct iio_poll_func *pf)
{
int ret = 0;
bool notinuse
@@ -312,9 +312,10 @@ static int iio_trigger_attach_poll_func(struct iio_trigger *trig,
module_put(pf->indio_dev->driver_module);
return ret;
}
+EXPORT_SYMBOL(iio_trigger_attach_poll_func);
-static int iio_trigger_detach_poll_func(struct iio_trigger *trig,
- struct iio_poll_func *pf)
+int iio_trigger_detach_poll_func(struct iio_trigger *trig,
+ struct iio_poll_func *pf)
{
int ret = 0;
bool no_other_users
@@ -334,6 +335,7 @@ static int iio_trigger_detach_poll_func(struct iio_trigger *trig,
return ret;
}
+EXPORT_SYMBOL(iio_trigger_detach_poll_func);
irqreturn_t iio_pollfunc_store_time(int irq, void *p)
{
diff --git a/include/linux/iio/trigger_consumer.h b/include/linux/iio/trigger_consumer.h
index c4f8c74..aeefcdb 100644
--- a/include/linux/iio/trigger_consumer.h
+++ b/include/linux/iio/trigger_consumer.h
@@ -60,4 +60,13 @@ void iio_trigger_notify_done(struct iio_trigger *trig);
int iio_triggered_buffer_postenable(struct iio_dev *indio_dev);
int iio_triggered_buffer_predisable(struct iio_dev *indio_dev);
+/*
+ * Two functions for the uncommon case when we need to attach or detach
+ * a specific pollfunc to and from a trigger
+ */
+int iio_trigger_attach_poll_func(struct iio_trigger *trig,
+ struct iio_poll_func *pf);
+
+int iio_trigger_detach_poll_func(struct iio_trigger *trig,
+ struct iio_poll_func *pf);
#endif
--
2.7.4
^ permalink raw reply related
* [PATCH 07/14] iio: triggers: on pollfunc attach, complete iio_dev if NULL
From: Eugen Hristev @ 2017-12-22 15:07 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1513955241-10985-1-git-send-email-eugen.hristev@microchip.com>
When attaching a pollfunc to a trigger, if the pollfunc does not
have an associated iio_dev pointer, just use the private data
iio_dev pointer from the trigger to fill in the poll func required
iio_dev reference.
Signed-off-by: Eugen Hristev <eugen.hristev@microchip.com>
---
drivers/iio/industrialio-trigger.c | 9 +++++++++
include/linux/iio/trigger_consumer.h | 2 ++
2 files changed, 11 insertions(+)
diff --git a/drivers/iio/industrialio-trigger.c b/drivers/iio/industrialio-trigger.c
index 8565c92..ab180bd 100644
--- a/drivers/iio/industrialio-trigger.c
+++ b/drivers/iio/industrialio-trigger.c
@@ -272,6 +272,15 @@ int iio_trigger_attach_poll_func(struct iio_trigger *trig,
bool notinuse
= bitmap_empty(trig->pool, CONFIG_IIO_CONSUMERS_PER_TRIGGER);
+ /*
+ * If we did not get a iio_dev in the poll func, attempt to
+ * obtain the trigger's owner's device struct
+ */
+ if (!pf->indio_dev)
+ pf->indio_dev = iio_trigger_get_drvdata(trig);
+ if (!pf->indio_dev)
+ return -EINVAL;
+
/* Prevent the module from being removed whilst attached to a trigger */
__module_get(pf->indio_dev->driver_module);
diff --git a/include/linux/iio/trigger_consumer.h b/include/linux/iio/trigger_consumer.h
index aeefcdb..36e2a02 100644
--- a/include/linux/iio/trigger_consumer.h
+++ b/include/linux/iio/trigger_consumer.h
@@ -63,6 +63,8 @@ int iio_triggered_buffer_predisable(struct iio_dev *indio_dev);
/*
* Two functions for the uncommon case when we need to attach or detach
* a specific pollfunc to and from a trigger
+ * If the pollfunc has a NULL iio_dev pointer, it will be filled from the
+ * trigger struct.
*/
int iio_trigger_attach_poll_func(struct iio_trigger *trig,
struct iio_poll_func *pf);
--
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