* [PATCH v6 6/6] dt-bindings: can: can-transceiver: Document new binding
From: Faiz Abbas @ 2017-12-22 13:31 UTC (permalink / raw)
To: wg, mkl, robh+dt, mark.rutland
Cc: linux-can, netdev, devicetree, linux-kernel, faiz_abbas, nsekhar,
fcooper, robh, Wenyou.Yang, sergei.shtylyov
In-Reply-To: <1513949488-13026-1-git-send-email-faiz_abbas@ti.com>
From: Franklin S Cooper Jr <fcooper@ti.com>
Add documentation to describe usage of the new can-transceiver binding.
This new binding is applicable for any CAN device therefore it exists as
its own document.
Signed-off-by: Franklin S Cooper Jr <fcooper@ti.com>
Signed-off-by: Sekhar Nori <nsekhar@ti.com>
Acked-by: Rob Herring <robh@kernel.org>
Signed-off-by: Faiz Abbas <faiz_abbas@ti.com>
---
.../bindings/net/can/can-transceiver.txt | 24 ++++++++++++++++++++++
1 file changed, 24 insertions(+)
create mode 100644 Documentation/devicetree/bindings/net/can/can-transceiver.txt
diff --git a/Documentation/devicetree/bindings/net/can/can-transceiver.txt b/Documentation/devicetree/bindings/net/can/can-transceiver.txt
new file mode 100644
index 0000000..0011f53
--- /dev/null
+++ b/Documentation/devicetree/bindings/net/can/can-transceiver.txt
@@ -0,0 +1,24 @@
+Generic CAN transceiver Device Tree binding
+------------------------------
+
+CAN transceiver typically limits the max speed in standard CAN and CAN FD
+modes. Typically these limitations are static and the transceivers themselves
+provide no way to detect this limitation at runtime. For this situation,
+the "can-transceiver" node can be used.
+
+Required Properties:
+ max-bitrate: a positive non 0 value that determines the max
+ speed that CAN/CAN-FD can run. Any other value
+ will be ignored.
+
+Examples:
+
+Based on Texas Instrument's TCAN1042HGV CAN Transceiver
+
+m_can0 {
+ ....
+ can-transceiver {
+ max-bitrate = <5000000>;
+ };
+ ...
+};
--
2.7.4
^ permalink raw reply related
* Re: [PATCH v4] hwrng: exynos - add Samsung Exynos True RNG driver
From: Philippe Ombredanne @ 2017-12-22 13:34 UTC (permalink / raw)
To: Łukasz Stelmach
Cc: Andrew F . Davis, PrasannaKumar Muralidharan, Rob Herring,
Matt Mackall, Herbert Xu, Krzysztof Kozlowski, Kukjin Kim,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
linux-crypto, linux-samsung-soc, LKML, Marek Szyprowski,
Bartlomiej Zolnierkiewicz
In-Reply-To: <20171222132338.30054-1-l.stelmach@samsung.com>
Łukasz,
On Fri, Dec 22, 2017 at 2:23 PM, Łukasz Stelmach <l.stelmach@samsung.com> wrote:
> Add support for True Random Number Generator found in Samsung Exynos
> 5250+ SoCs.
>
> Signed-off-by: Łukasz Stelmach <l.stelmach@samsung.com>
> Reviewed-by: Krzysztof Kozlowski <krzk@kernel.org>
<snip>
> --- /dev/null
> +++ b/drivers/char/hw_random/exynos-trng.c
> @@ -0,0 +1,245 @@
> +/*
> + * RNG driver for Exynos TRNGs
> + *
> + * Author: Łukasz Stelmach <l.stelmach@samsung.com>
> + *
> + * Copyright 2017 (c) Samsung Electronics Software, Inc.
> + *
> + * Based on the Exynos PRNG driver drivers/crypto/exynos-rng by
> + * Krzysztof Kozłowski <krzk@kernel.org>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License 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?
And if you could spread the word to others in your team this would be very nice.
See also this fine article posted by Mauro on the Samsung Open Source
Group Blog [2]
Thank you!
> +MODULE_LICENSE("GPL");
Per module.h this means GPL2 or later. This is not matching your
license above which does not state any version and therefore would
mean GPL1 or later,
Please make sure you use something and common rather than this and
make sure your MODULE_LICENSE is consistent with the top level
license.
Was it this way in the code from Krzysztof?
[1] https://lkml.org/lkml/2017/12/4/934
[2] https://blogs.s-osg.org/linux-kernel-license-practices-revisited-spdx/
^ permalink raw reply
* Re: [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: Yong Deng
Cc: Maxime Ripard, Mauro Carvalho Chehab, Rob Herring, Mark Rutland,
Chen-Yu Tsai, David S. Miller, Greg Kroah-Hartman, Randy Dunlap,
Hans Verkuil, Stanimir Varbanov, Hugues Fruchet, Yannick Fertre,
Philipp Zabel, Arnd Bergmann, Benjamin Gaignard,
Ramesh Shanmugasundaram, Sakari Ailus, Rick Chang
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
^ permalink raw reply
* Re: [PATCH v4 0/2] Initial Allwinner V3s CSI Support
From: 'Ondřej Jirman' via linux-sunxi @ 2017-12-22 13:46 UTC (permalink / raw)
To: yong.deng-+3dxTMOEIRNWk0Htik3J/w, "Maxime Ripard
Cc: Mauro Carvalho Chehab, Rob Herring, Mark Rutland, Chen-Yu Tsai,
David S. Miller, Greg Kroah-Hartman, Randy Dunlap, Hans Verkuil,
Stanimir Varbanov, Hugues Fruchet, Yannick Fertre, Philipp Zabel,
Arnd Bergmann, Benjamin Gaignard, Ramesh Shanmugasundaram,
Sakari Ailus, Rick Chang, linux-media-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
linux-sunxi-/JYPxA39Uh5TLH3MbocFFw
In-Reply-To: <1513935138-35223-1-git-send-email-yong.deng-+3dxTMOEIRNWk0Htik3J/w@public.gmane.org>
[-- Attachment #1: Type: text/plain, Size: 9925 bytes --]
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
>
--
You received this message because you are subscribed to the Google Groups "linux-sunxi" group.
To unsubscribe from this group and stop receiving emails from it, send an email to linux-sunxi+unsubscribe-/JYPxA39Uh5TLH3MbocFF+G/Ez6ZCGd0@public.gmane.org
For more options, visit https://groups.google.com/d/optout.
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH 0/4] vf610-zii-dev updates
From: Russell King - ARM Linux @ 2017-12-22 14:16 UTC (permalink / raw)
To: Mark Rutland, Rob Herring, Sascha Hauer, Shawn Guo, Stefan Agner,
Andrew Lunn, Florian Fainelli, Linus Walleij, Sanchayan Maity,
Guenter Roeck
Cc: devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r
In-Reply-To: <20171220231108.GJ10595-l+eeeJia6m9URfEZ8mYm6t73F7V6hmMc@public.gmane.org>
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@4003b000 {
...
+ status = "disabled";
+ };
+ adc1: adc@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
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH v2] arm64: dts: Hi3660: Fix up psci state id
From: Vincent Guittot @ 2017-12-22 14:22 UTC (permalink / raw)
To: Leo Yan
Cc: Wei Xu, Rob Herring, Mark Rutland, Catalin Marinas, Will Deacon,
LAK, devicetree-u79uwXL29TY76Z2rM5mHXA, linux-kernel,
Daniel Lezcano, Sudeep Holla, Soby Mathew
In-Reply-To: <1513069954-22827-1-git-send-email-leo.yan-QSEj5FYQhm4dnm+yROfE0A@public.gmane.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-QSEj5FYQhm4dnm+yROfE0A@public.gmane.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-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> Cc: Daniel Lezcano <daniel.lezcano-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> Cc: Sudeep Holla <sudeep.holla-5wv7dgnIgG8@public.gmane.org>
> Cc: Soby Mathew <Soby.Mathew-5wv7dgnIgG8@public.gmane.org>
> Signed-off-by: Leo Yan <leo.yan-QSEj5FYQhm4dnm+yROfE0A@public.gmane.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
>
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* [PATCH] dt: psci: Update DT bindings to support hierarchical PSCI states
From: Ulf Hansson @ 2017-12-22 14:32 UTC (permalink / raw)
To: Rob Herring, Mark Rutland, devicetree
Cc: Ulf Hansson, Lorenzo Pieralisi, Kevin Hilman, linux-kernel,
Brendan Jackman, linux-arm-kernel, Sudeep Holla, Lina Iyer
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@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@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@0 {
+ device_type = "cpu";
+ compatible = "arm,cortex-a53", "arm,armv8";
+ reg = <0x0>;
+ enable-method = "psci";
+ power-domains = <&CPU_PD0>;
+ };
+
+ CPU1: cpu@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@0 {
+ #power-domain-cells = <0>;
+ domain-idle-states = <&CPU_PWRDN>;
+ power-domains = <&CLUSTER_PD>;
+ };
+
+ CPU_PD1: cpu-pd@1 {
+ #power-domain-cells = <0>;
+ domain-idle-states = <&CPU_PWRDN>;
+ power-domains = <&CLUSTER_PD>;
+ };
+
+ CLUSTER_PD: cluster-pd@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
* Re: [PATCH v4 01/36] asm-generic/io.h: move ioremap_nocache/ioremap_uc/ioremap_wc/ioremap_wt out of ifndef CONFIG_MMU
From: Greentime Hu @ 2017-12-22 14:38 UTC (permalink / raw)
To: kbuild test robot
Cc: kbuild-all, Greentime, Linux Kernel Mailing List, Arnd Bergmann,
linux-arch, Thomas Gleixner, Jason Cooper, Marc Zyngier,
Rob Herring, netdev, Vincent Chen, DTML, Al Viro, David Howells,
Will Deacon, Daniel Lezcano, linux-serial, Geert Uytterhoeven,
Linus Walleij, Mark Rutland, Greg
In-Reply-To: <201712201836.XDtv2JdK%fengguang.wu@intel.com>
2017-12-20 18:09 GMT+08:00 kbuild test robot <lkp@intel.com>:
> Hi Greentime,
>
> Thank you for the patch! Yet something to improve:
>
> [auto build test ERROR on tip/timers/core]
> [also build test ERROR on v4.15-rc4 next-20171220]
> [cannot apply to linus/master]
> [if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
>
> url: https://github.com/0day-ci/linux/commits/Greentime-Hu/Andes-nds32-Linux-Kernel/20171220-155937
> config: sparc-defconfig (attached as .config)
> compiler: sparc-linux-gcc (GCC) 7.2.0
> reproduce:
> wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
> chmod +x ~/bin/make.cross
> # save the attached .config to linux build tree
> make.cross ARCH=sparc
>
> All error/warnings (new ones prefixed by >>):
>
> In file included from arch/sparc/include/asm/io.h:6:0,
> from include/linux/io.h:25,
> from include/linux/irq.h:24,
> from include/asm-generic/hardirq.h:12,
> from arch/sparc/include/asm/hardirq_32.h:10,
> from arch/sparc/include/asm/hardirq.h:6,
> from include/linux/hardirq.h:8,
> from include/linux/interrupt.h:12,
> from arch/sparc/kernel/kernel.h:4,
> from arch/sparc/kernel/traps_32.c:30:
>>> arch/sparc/include/asm/io_32.h:129:15: error: conflicting types for 'ioremap'
> void __iomem *ioremap(unsigned long offset, unsigned long size);
> ^~~~~~~
> In file included from arch/sparc/include/asm/io_32.h:13:0,
> from arch/sparc/include/asm/io.h:6,
> from include/linux/io.h:25,
> from include/linux/irq.h:24,
> from include/asm-generic/hardirq.h:12,
> from arch/sparc/include/asm/hardirq_32.h:10,
> from arch/sparc/include/asm/hardirq.h:6,
> from include/linux/hardirq.h:8,
> from include/linux/interrupt.h:12,
> from arch/sparc/kernel/kernel.h:4,
> from arch/sparc/kernel/traps_32.c:30:
> include/asm-generic/io.h:864:15: note: previous declaration of 'ioremap' was here
> void __iomem *ioremap(phys_addr_t phys_addr, size_t size);
> ^~~~~~~
> In file included from arch/sparc/include/asm/io.h:6:0,
> from include/linux/io.h:25,
> from include/linux/irq.h:24,
> from include/asm-generic/hardirq.h:12,
> from arch/sparc/include/asm/hardirq_32.h:10,
> from arch/sparc/include/asm/hardirq.h:6,
> from include/linux/hardirq.h:8,
> from include/linux/interrupt.h:12,
> from arch/sparc/kernel/kernel.h:4,
> from arch/sparc/kernel/traps_32.c:30:
> arch/sparc/include/asm/io_32.h:130:0: error: "ioremap_nocache" redefined [-Werror]
> #define ioremap_nocache(X,Y) ioremap((X),(Y))
>
> In file included from arch/sparc/include/asm/io_32.h:13:0,
> from arch/sparc/include/asm/io.h:6,
> from include/linux/io.h:25,
> from include/linux/irq.h:24,
> from include/asm-generic/hardirq.h:12,
> from arch/sparc/include/asm/hardirq_32.h:10,
> from arch/sparc/include/asm/hardirq.h:6,
> from include/linux/hardirq.h:8,
> from include/linux/interrupt.h:12,
> from arch/sparc/kernel/kernel.h:4,
> from arch/sparc/kernel/traps_32.c:30:
> include/asm-generic/io.h:865:0: note: this is the location of the previous definition
> #define ioremap_nocache ioremap_nocache
>
> In file included from arch/sparc/include/asm/io.h:6:0,
> from include/linux/io.h:25,
> from include/linux/irq.h:24,
> from include/asm-generic/hardirq.h:12,
> from arch/sparc/include/asm/hardirq_32.h:10,
> from arch/sparc/include/asm/hardirq.h:6,
> from include/linux/hardirq.h:8,
> from include/linux/interrupt.h:12,
> from arch/sparc/kernel/kernel.h:4,
> from arch/sparc/kernel/traps_32.c:30:
> arch/sparc/include/asm/io_32.h:131:0: error: "ioremap_wc" redefined [-Werror]
> #define ioremap_wc(X,Y) ioremap((X),(Y))
>
> In file included from arch/sparc/include/asm/io_32.h:13:0,
> from arch/sparc/include/asm/io.h:6,
> from include/linux/io.h:25,
> from include/linux/irq.h:24,
> from include/asm-generic/hardirq.h:12,
> from arch/sparc/include/asm/hardirq_32.h:10,
> from arch/sparc/include/asm/hardirq.h:6,
> from include/linux/hardirq.h:8,
> from include/linux/interrupt.h:12,
> from arch/sparc/kernel/kernel.h:4,
> from arch/sparc/kernel/traps_32.c:30:
> include/asm-generic/io.h:881:0: note: this is the location of the previous definition
> #define ioremap_wc ioremap_wc
>
> In file included from arch/sparc/include/asm/io.h:6:0,
> from include/linux/io.h:25,
> from include/linux/irq.h:24,
> from include/asm-generic/hardirq.h:12,
> from arch/sparc/include/asm/hardirq_32.h:10,
> from arch/sparc/include/asm/hardirq.h:6,
> from include/linux/hardirq.h:8,
> from include/linux/interrupt.h:12,
> from arch/sparc/kernel/kernel.h:4,
> from arch/sparc/kernel/traps_32.c:30:
> arch/sparc/include/asm/io_32.h:132:0: error: "ioremap_wt" redefined [-Werror]
> #define ioremap_wt(X,Y) ioremap((X),(Y))
>
> In file included from arch/sparc/include/asm/io_32.h:13:0,
> from arch/sparc/include/asm/io.h:6,
> from include/linux/io.h:25,
> from include/linux/irq.h:24,
> from include/asm-generic/hardirq.h:12,
> from arch/sparc/include/asm/hardirq_32.h:10,
> from arch/sparc/include/asm/hardirq.h:6,
> from include/linux/hardirq.h:8,
> from include/linux/interrupt.h:12,
> from arch/sparc/kernel/kernel.h:4,
> from arch/sparc/kernel/traps_32.c:30:
> include/asm-generic/io.h:889:0: note: this is the location of the previous definition
> #define ioremap_wt ioremap_wt
>
> cc1: all warnings being treated as errors
> --
> In file included from arch/sparc/include/asm/io.h:6:0,
> from include/linux/io.h:25,
> from include/linux/irq.h:24,
> from include/asm-generic/hardirq.h:12,
> from arch/sparc/include/asm/hardirq_32.h:10,
> from arch/sparc/include/asm/hardirq.h:6,
> from include/linux/hardirq.h:8,
> from include/linux/interrupt.h:12,
> from include/linux/pci.h:31,
> from arch/sparc/kernel/ioport.c:36:
>>> arch/sparc/include/asm/io_32.h:129:15: error: conflicting types for 'ioremap'
> void __iomem *ioremap(unsigned long offset, unsigned long size);
> ^~~~~~~
> In file included from arch/sparc/include/asm/io_32.h:13:0,
> from arch/sparc/include/asm/io.h:6,
> from include/linux/io.h:25,
> from include/linux/irq.h:24,
> from include/asm-generic/hardirq.h:12,
> from arch/sparc/include/asm/hardirq_32.h:10,
> from arch/sparc/include/asm/hardirq.h:6,
> from include/linux/hardirq.h:8,
> from include/linux/interrupt.h:12,
> from include/linux/pci.h:31,
> from arch/sparc/kernel/ioport.c:36:
> include/asm-generic/io.h:864:15: note: previous declaration of 'ioremap' was here
> void __iomem *ioremap(phys_addr_t phys_addr, size_t size);
> ^~~~~~~
> In file included from arch/sparc/include/asm/io.h:6:0,
> from include/linux/io.h:25,
> from include/linux/irq.h:24,
> from include/asm-generic/hardirq.h:12,
> from arch/sparc/include/asm/hardirq_32.h:10,
> from arch/sparc/include/asm/hardirq.h:6,
> from include/linux/hardirq.h:8,
> from include/linux/interrupt.h:12,
> from include/linux/pci.h:31,
> from arch/sparc/kernel/ioport.c:36:
> arch/sparc/include/asm/io_32.h:130:0: error: "ioremap_nocache" redefined [-Werror]
> #define ioremap_nocache(X,Y) ioremap((X),(Y))
>
> In file included from arch/sparc/include/asm/io_32.h:13:0,
> from arch/sparc/include/asm/io.h:6,
> from include/linux/io.h:25,
> from include/linux/irq.h:24,
> from include/asm-generic/hardirq.h:12,
> from arch/sparc/include/asm/hardirq_32.h:10,
> from arch/sparc/include/asm/hardirq.h:6,
> from include/linux/hardirq.h:8,
> from include/linux/interrupt.h:12,
> from include/linux/pci.h:31,
> from arch/sparc/kernel/ioport.c:36:
> include/asm-generic/io.h:865:0: note: this is the location of the previous definition
> #define ioremap_nocache ioremap_nocache
>
> In file included from arch/sparc/include/asm/io.h:6:0,
> from include/linux/io.h:25,
> from include/linux/irq.h:24,
> from include/asm-generic/hardirq.h:12,
> from arch/sparc/include/asm/hardirq_32.h:10,
> from arch/sparc/include/asm/hardirq.h:6,
> from include/linux/hardirq.h:8,
> from include/linux/interrupt.h:12,
> from include/linux/pci.h:31,
> from arch/sparc/kernel/ioport.c:36:
> arch/sparc/include/asm/io_32.h:131:0: error: "ioremap_wc" redefined [-Werror]
> #define ioremap_wc(X,Y) ioremap((X),(Y))
>
> In file included from arch/sparc/include/asm/io_32.h:13:0,
> from arch/sparc/include/asm/io.h:6,
> from include/linux/io.h:25,
> from include/linux/irq.h:24,
> from include/asm-generic/hardirq.h:12,
> from arch/sparc/include/asm/hardirq_32.h:10,
> from arch/sparc/include/asm/hardirq.h:6,
> from include/linux/hardirq.h:8,
> from include/linux/interrupt.h:12,
> from include/linux/pci.h:31,
> from arch/sparc/kernel/ioport.c:36:
> include/asm-generic/io.h:881:0: note: this is the location of the previous definition
> #define ioremap_wc ioremap_wc
>
> In file included from arch/sparc/include/asm/io.h:6:0,
> from include/linux/io.h:25,
> from include/linux/irq.h:24,
> from include/asm-generic/hardirq.h:12,
> from arch/sparc/include/asm/hardirq_32.h:10,
> from arch/sparc/include/asm/hardirq.h:6,
> from include/linux/hardirq.h:8,
> from include/linux/interrupt.h:12,
> from include/linux/pci.h:31,
> from arch/sparc/kernel/ioport.c:36:
> arch/sparc/include/asm/io_32.h:132:0: error: "ioremap_wt" redefined [-Werror]
> #define ioremap_wt(X,Y) ioremap((X),(Y))
>
> In file included from arch/sparc/include/asm/io_32.h:13:0,
> from arch/sparc/include/asm/io.h:6,
> from include/linux/io.h:25,
> from include/linux/irq.h:24,
> from include/asm-generic/hardirq.h:12,
> from arch/sparc/include/asm/hardirq_32.h:10,
> from arch/sparc/include/asm/hardirq.h:6,
> from include/linux/hardirq.h:8,
> from include/linux/interrupt.h:12,
> from include/linux/pci.h:31,
> from arch/sparc/kernel/ioport.c:36:
> include/asm-generic/io.h:889:0: note: this is the location of the previous definition
> #define ioremap_wt ioremap_wt
>
>>> arch/sparc/kernel/ioport.c:124:15: error: conflicting types for 'ioremap'
> void __iomem *ioremap(unsigned long offset, unsigned long size)
> ^~~~~~~
> In file included from arch/sparc/include/asm/io_32.h:13:0,
> from arch/sparc/include/asm/io.h:6,
> from include/linux/io.h:25,
> from include/linux/irq.h:24,
> from include/asm-generic/hardirq.h:12,
> from arch/sparc/include/asm/hardirq_32.h:10,
> from arch/sparc/include/asm/hardirq.h:6,
> from include/linux/hardirq.h:8,
> from include/linux/interrupt.h:12,
> from include/linux/pci.h:31,
> from arch/sparc/kernel/ioport.c:36:
> include/asm-generic/io.h:864:15: note: previous declaration of 'ioremap' was here
> void __iomem *ioremap(phys_addr_t phys_addr, size_t size);
> ^~~~~~~
> In file included from include/linux/linkage.h:6:0,
> from include/linux/kernel.h:6,
> from include/linux/list.h:8,
> from include/linux/module.h:9,
> from arch/sparc/kernel/ioport.c:28:
> arch/sparc/kernel/ioport.c:131:15: error: conflicting types for 'ioremap'
> EXPORT_SYMBOL(ioremap);
> ^
> include/linux/export.h:65:21: note: in definition of macro '___EXPORT_SYMBOL'
> extern typeof(sym) sym; \
> ^~~
>>> arch/sparc/kernel/ioport.c:131:1: note: in expansion of macro 'EXPORT_SYMBOL'
> EXPORT_SYMBOL(ioremap);
> ^~~~~~~~~~~~~
> In file included from arch/sparc/include/asm/io_32.h:13:0,
> from arch/sparc/include/asm/io.h:6,
> from include/linux/io.h:25,
> from include/linux/irq.h:24,
> from include/asm-generic/hardirq.h:12,
> from arch/sparc/include/asm/hardirq_32.h:10,
> from arch/sparc/include/asm/hardirq.h:6,
> from include/linux/hardirq.h:8,
> from include/linux/interrupt.h:12,
> from include/linux/pci.h:31,
> from arch/sparc/kernel/ioport.c:36:
> include/asm-generic/io.h:864:15: note: previous declaration of 'ioremap' was here
> void __iomem *ioremap(phys_addr_t phys_addr, size_t size);
> ^~~~~~~
> cc1: all warnings being treated as errors
> --
> In file included from arch/sparc/include/asm/io.h:6:0,
> from include/linux/io.h:25,
> from include/linux/irq.h:24,
> from include/asm-generic/hardirq.h:12,
> from arch/sparc/include/asm/hardirq_32.h:10,
> from arch/sparc/include/asm/hardirq.h:6,
> from include/linux/hardirq.h:8,
> from include/linux/interrupt.h:12,
> from include/linux/pci.h:31,
> from arch/sparc//kernel/ioport.c:36:
>>> arch/sparc/include/asm/io_32.h:129:15: error: conflicting types for 'ioremap'
> void __iomem *ioremap(unsigned long offset, unsigned long size);
> ^~~~~~~
> In file included from arch/sparc/include/asm/io_32.h:13:0,
> from arch/sparc/include/asm/io.h:6,
> from include/linux/io.h:25,
> from include/linux/irq.h:24,
> from include/asm-generic/hardirq.h:12,
> from arch/sparc/include/asm/hardirq_32.h:10,
> from arch/sparc/include/asm/hardirq.h:6,
> from include/linux/hardirq.h:8,
> from include/linux/interrupt.h:12,
> from include/linux/pci.h:31,
> from arch/sparc//kernel/ioport.c:36:
> include/asm-generic/io.h:864:15: note: previous declaration of 'ioremap' was here
> void __iomem *ioremap(phys_addr_t phys_addr, size_t size);
> ^~~~~~~
> In file included from arch/sparc/include/asm/io.h:6:0,
> from include/linux/io.h:25,
> from include/linux/irq.h:24,
> from include/asm-generic/hardirq.h:12,
> from arch/sparc/include/asm/hardirq_32.h:10,
> from arch/sparc/include/asm/hardirq.h:6,
> from include/linux/hardirq.h:8,
> from include/linux/interrupt.h:12,
> from include/linux/pci.h:31,
> from arch/sparc//kernel/ioport.c:36:
> arch/sparc/include/asm/io_32.h:130:0: error: "ioremap_nocache" redefined [-Werror]
> #define ioremap_nocache(X,Y) ioremap((X),(Y))
>
> In file included from arch/sparc/include/asm/io_32.h:13:0,
> from arch/sparc/include/asm/io.h:6,
> from include/linux/io.h:25,
> from include/linux/irq.h:24,
> from include/asm-generic/hardirq.h:12,
> from arch/sparc/include/asm/hardirq_32.h:10,
> from arch/sparc/include/asm/hardirq.h:6,
> from include/linux/hardirq.h:8,
> from include/linux/interrupt.h:12,
> from include/linux/pci.h:31,
> from arch/sparc//kernel/ioport.c:36:
> include/asm-generic/io.h:865:0: note: this is the location of the previous definition
> #define ioremap_nocache ioremap_nocache
>
> In file included from arch/sparc/include/asm/io.h:6:0,
> from include/linux/io.h:25,
> from include/linux/irq.h:24,
> from include/asm-generic/hardirq.h:12,
> from arch/sparc/include/asm/hardirq_32.h:10,
> from arch/sparc/include/asm/hardirq.h:6,
> from include/linux/hardirq.h:8,
> from include/linux/interrupt.h:12,
> from include/linux/pci.h:31,
> from arch/sparc//kernel/ioport.c:36:
> arch/sparc/include/asm/io_32.h:131:0: error: "ioremap_wc" redefined [-Werror]
> #define ioremap_wc(X,Y) ioremap((X),(Y))
>
> In file included from arch/sparc/include/asm/io_32.h:13:0,
> from arch/sparc/include/asm/io.h:6,
> from include/linux/io.h:25,
> from include/linux/irq.h:24,
> from include/asm-generic/hardirq.h:12,
> from arch/sparc/include/asm/hardirq_32.h:10,
> from arch/sparc/include/asm/hardirq.h:6,
> from include/linux/hardirq.h:8,
> from include/linux/interrupt.h:12,
> from include/linux/pci.h:31,
> from arch/sparc//kernel/ioport.c:36:
> include/asm-generic/io.h:881:0: note: this is the location of the previous definition
> #define ioremap_wc ioremap_wc
>
> In file included from arch/sparc/include/asm/io.h:6:0,
> from include/linux/io.h:25,
> from include/linux/irq.h:24,
> from include/asm-generic/hardirq.h:12,
> from arch/sparc/include/asm/hardirq_32.h:10,
> from arch/sparc/include/asm/hardirq.h:6,
> from include/linux/hardirq.h:8,
> from include/linux/interrupt.h:12,
> from include/linux/pci.h:31,
> from arch/sparc//kernel/ioport.c:36:
> arch/sparc/include/asm/io_32.h:132:0: error: "ioremap_wt" redefined [-Werror]
> #define ioremap_wt(X,Y) ioremap((X),(Y))
>
> In file included from arch/sparc/include/asm/io_32.h:13:0,
> from arch/sparc/include/asm/io.h:6,
> from include/linux/io.h:25,
> from include/linux/irq.h:24,
> from include/asm-generic/hardirq.h:12,
> from arch/sparc/include/asm/hardirq_32.h:10,
> from arch/sparc/include/asm/hardirq.h:6,
> from include/linux/hardirq.h:8,
> from include/linux/interrupt.h:12,
> from include/linux/pci.h:31,
> from arch/sparc//kernel/ioport.c:36:
> include/asm-generic/io.h:889:0: note: this is the location of the previous definition
> #define ioremap_wt ioremap_wt
>
> arch/sparc//kernel/ioport.c:124:15: error: conflicting types for 'ioremap'
> void __iomem *ioremap(unsigned long offset, unsigned long size)
> ^~~~~~~
>
Hi, all:
I just tried to fix this build error.
Should I send this patch next time with nds32 patchset?
--- a/arch/sparc/include/asm/io_32.h
+++ b/arch/sparc/include/asm/io_32.h
@@ -126,12 +126,7 @@ static inline void sbus_memcpy_toio(volatile void
__iomem *dst,
* Bus number may be embedded in the higher bits of the physical address.
* This is why we have no bus number argument to ioremap().
*/
-void __iomem *ioremap(unsigned long offset, unsigned long size);
-#define ioremap_nocache(X,Y) ioremap((X),(Y))
-#define ioremap_wc(X,Y) ioremap((X),(Y))
-#define ioremap_wt(X,Y) ioremap((X),(Y))
void iounmap(volatile void __iomem *addr);
-
/* Create a virtual mapping cookie for an IO port range */
void __iomem *ioport_map(unsigned long port, unsigned int nr);
void ioport_unmap(void __iomem *);
diff --git a/arch/sparc/kernel/ioport.c b/arch/sparc/kernel/ioport.c
index 12894f2..9cdeb54 100644
--- a/arch/sparc/kernel/ioport.c
+++ b/arch/sparc/kernel/ioport.c
@@ -121,12 +121,12 @@ static void xres_free(struct xresource *xrp) {
*
* Bus type is always zero on IIep.
*/
-void __iomem *ioremap(unsigned long offset, unsigned long size)
+void __iomem *ioremap(phys_addr_t offset, size_t size)
{
char name[14];
sprintf(name, "phys_%08x", (u32)offset);
- return _sparc_alloc_io(0, offset, size, name);
+ return _sparc_alloc_io(0, (unsigned long)offset, size, name);
}
EXPORT_SYMBOL(ioremap);
^ permalink raw reply related
* Re: [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: Johan Hovold
Cc: Mark Rutland, devicetree, linux-omap, Arnd Bergmann,
Tony Lindgren, Greg Kroah-Hartman, kernel, Russell King,
linux-kernel, Rob Herring, linux-arm-kernel, Benoît Cousson,
Kevin Hilman, letux-kernel, Thierry Reding, Andreas Färber,
Jonathan Cameron
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
* Re: [PATCH v4 01/36] asm-generic/io.h: move ioremap_nocache/ioremap_uc/ioremap_wc/ioremap_wt out of ifndef CONFIG_MMU
From: Greentime Hu @ 2017-12-22 14:40 UTC (permalink / raw)
To: kbuild test robot
Cc: kbuild-all, Greentime, Linux Kernel Mailing List, Arnd Bergmann,
linux-arch, Thomas Gleixner, Jason Cooper, Marc Zyngier,
Rob Herring, netdev, Vincent Chen, DTML, Al Viro, David Howells,
Will Deacon, Daniel Lezcano, linux-serial, Geert Uytterhoeven,
Linus Walleij, Mark Rutland, Greg
In-Reply-To: <201712201748.C4nCizdE%fengguang.wu@intel.com>
2017-12-20 18:10 GMT+08:00 kbuild test robot <lkp@intel.com>:
> Hi Greentime,
>
> Thank you for the patch! Yet something to improve:
>
> [auto build test ERROR on tip/timers/core]
> [also build test ERROR on v4.15-rc4 next-20171220]
> [cannot apply to linus/master]
> [if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
>
> url: https://github.com/0day-ci/linux/commits/Greentime-Hu/Andes-nds32-Linux-Kernel/20171220-155937
> config: openrisc-or1ksim_defconfig (attached as .config)
> compiler: or1k-linux-gcc (GCC) 6.0.0 20160327 (experimental)
> reproduce:
> wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
> chmod +x ~/bin/make.cross
> # save the attached .config to linux build tree
> make.cross ARCH=openrisc
>
> All error/warnings (new ones prefixed by >>):
>
> In file included from include/linux/io.h:25:0,
> from arch/openrisc/kernel/asm-offsets.c:35:
>>> arch/openrisc/include/asm/io.h:38:29: error: conflicting types for 'ioremap'
> static inline void __iomem *ioremap(phys_addr_t offset, unsigned long size)
> ^~~~~~~
> In file included from arch/openrisc/include/asm/io.h:32:0,
> from include/linux/io.h:25,
> from arch/openrisc/kernel/asm-offsets.c:35:
> include/asm-generic/io.h:864:15: note: previous declaration of 'ioremap' was here
> void __iomem *ioremap(phys_addr_t phys_addr, size_t size);
> ^~~~~~~
> include/asm-generic/io.h:865:25: error: conflicting types for 'ioremap_nocache'
> #define ioremap_nocache ioremap_nocache
> ^
>>> arch/openrisc/include/asm/io.h:44:29: note: in expansion of macro 'ioremap_nocache'
> static inline void __iomem *ioremap_nocache(phys_addr_t offset,
> ^~~~~~~~~~~~~~~
> include/asm-generic/io.h:865:25: note: previous definition of 'ioremap_nocache' was here
> #define ioremap_nocache ioremap_nocache
> ^
> include/asm-generic/io.h:866:29: note: in expansion of macro 'ioremap_nocache'
> static inline void __iomem *ioremap_nocache(phys_addr_t offset, size_t size)
> ^~~~~~~~~~~~~~~
> make[2]: *** [arch/openrisc/kernel/asm-offsets.s] Error 1
> make[2]: Target '__build' not remade because of errors.
> make[1]: *** [prepare0] Error 2
> make[1]: Target 'prepare' not remade because of errors.
> make: *** [sub-make] Error 2
>
> vim +/ioremap +38 arch/openrisc/include/asm/io.h
>
> 58e0166a Jonas Bonn 2011-06-04 31
> 58e0166a Jonas Bonn 2011-06-04 @32 #include <asm-generic/io.h>
> 9b04ebd1 James Hogan 2012-10-23 33 #include <asm/pgtable.h>
> 58e0166a Jonas Bonn 2011-06-04 34
> 58e0166a Jonas Bonn 2011-06-04 35 extern void __iomem *__ioremap(phys_addr_t offset, unsigned long size,
> 58e0166a Jonas Bonn 2011-06-04 36 pgprot_t prot);
> 58e0166a Jonas Bonn 2011-06-04 37
> 58e0166a Jonas Bonn 2011-06-04 @38 static inline void __iomem *ioremap(phys_addr_t offset, unsigned long size)
> 58e0166a Jonas Bonn 2011-06-04 39 {
> 58e0166a Jonas Bonn 2011-06-04 40 return __ioremap(offset, size, PAGE_KERNEL);
> 58e0166a Jonas Bonn 2011-06-04 41 }
> 58e0166a Jonas Bonn 2011-06-04 42
> 58e0166a Jonas Bonn 2011-06-04 43 /* #define _PAGE_CI 0x002 */
> 58e0166a Jonas Bonn 2011-06-04 @44 static inline void __iomem *ioremap_nocache(phys_addr_t offset,
> 58e0166a Jonas Bonn 2011-06-04 45 unsigned long size)
> 58e0166a Jonas Bonn 2011-06-04 46 {
> 58e0166a Jonas Bonn 2011-06-04 47 return __ioremap(offset, size,
> 58e0166a Jonas Bonn 2011-06-04 48 __pgprot(pgprot_val(PAGE_KERNEL) | _PAGE_CI));
> 58e0166a Jonas Bonn 2011-06-04 49 }
> 58e0166a Jonas Bonn 2011-06-04 50
>
> :::::: The code at line 38 was first introduced by commit
> :::::: 58e0166a4772aaeb10c9b0f6d59f19099d2047df OpenRISC: Headers
>
> :::::: TO: Jonas Bonn <jonas@southpole.se>
> :::::: CC: Jonas Bonn <jonas@southpole.se>
>
> ---
> 0-DAY kernel test infrastructure Open Source Technology Center
> https://lists.01.org/pipermail/kbuild-all Intel Corporation
Hi, all:
I just tried to fix this build error.
Should I send this patch next time with nds32 patchset?
--- a/arch/openrisc/include/asm/io.h
+++ b/arch/openrisc/include/asm/io.h
@@ -29,13 +29,14 @@
#define PIO_OFFSET 0
#define PIO_MASK 0
+#define ioremap_nocache ioremap_nocache
#include <asm-generic/io.h>
#include <asm/pgtable.h>
extern void __iomem *__ioremap(phys_addr_t offset, unsigned long size,
pgprot_t prot);
-static inline void __iomem *ioremap(phys_addr_t offset, unsigned long size)
+static inline void __iomem *ioremap(phys_addr_t offset, size_t size)
{
return __ioremap(offset, size, PAGE_KERNEL);
}
^ permalink raw reply
* Re: [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: Johan Hovold
Cc: Rob Herring, Mark Rutland, Benoît Cousson, Tony Lindgren,
Russell King, Arnd Bergmann, Greg Kroah-Hartman, Kevin Hilman,
Andreas Färber, Thierry Reding, Jonathan Cameron, devicetree,
linux-kernel, linux-omap, letux-kernel, kernel, 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
* Re: [PATCH v11 0/6] Add support for Qualcomm A53 CPU clock
From: Georgi Djakov @ 2017-12-22 14:52 UTC (permalink / raw)
To: Stephen Boyd, jassisinghbrar
Cc: bjorn.andersson, mturquette, robh, linux-clk, linux-kernel,
linux-arm-msm, devicetree
In-Reply-To: <20171222004904.GA7997@codeaurora.org>
On 22.12.17 г. 2:49, Stephen Boyd wrote:
> On 12/05, Georgi Djakov wrote:
>> This patchset adds support for the A53 CPU clock on MSM8916 platforms
>> and allows scaling of the CPU frequency on msm8916 based platforms.
>
> Ok. I will apply just the clk ones? Patches 3,4, and 6?
>
Yes, i think this would be best. The rest should go through the mailbox tree.
Jassi, are you ok with this?
Thanks,
Georgi
^ permalink raw reply
* Re: [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: H. Nikolaus Schaller
Cc: Johan Hovold, Rob Herring, Mark Rutland, Benoît Cousson,
Tony Lindgren, Russell King, Arnd Bergmann, Kevin Hilman,
Andreas Färber, Thierry Reding, Jonathan Cameron,
devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
linux-omap-u79uwXL29TY76Z2rM5mHXA,
letux-kernel-S0jZdbWzriLCfDggNXIi3w,
kernel-Jl6IXVxNIMRxAtABVqVhTwC/G2K4zDHf,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r
In-Reply-To: <A2720858-5172-4627-8133-C677E4B4DC72-xXXSsgcRVICgSpxsJD1C4w@public.gmane.org>
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-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.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-xXXSsgcRVICgSpxsJD1C4w@public.gmane.org>
> >> ---
> >> 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
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* [PATCH 00/14] iio: triggers: add consumer support
From: Eugen Hristev @ 2017-12-22 15:07 UTC (permalink / raw)
To: nicolas.ferre-UWL1GkI3JZL3oGB3hsPCZA,
ludovic.desroches-UWL1GkI3JZL3oGB3hsPCZA,
alexandre.belloni-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8,
linux-iio-u79uwXL29TY76Z2rM5mHXA,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA, jic23-DgEjT+Ai2ygdnm+yROfE0A,
linux-input-u79uwXL29TY76Z2rM5mHXA,
dmitry.torokhov-Re5JQEeQqe8AvxtiuMwx3w
Cc: Eugen Hristev
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
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* [PATCH 01/14] MAINTAINERS: add sama5d2 resistive touchscreen
From: Eugen Hristev @ 2017-12-22 15:07 UTC (permalink / raw)
To: nicolas.ferre, ludovic.desroches, alexandre.belloni, linux-iio,
linux-arm-kernel, devicetree, linux-kernel, jic23, linux-input,
dmitry.torokhov
Cc: Eugen Hristev
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@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: nicolas.ferre, ludovic.desroches, alexandre.belloni, linux-iio,
linux-arm-kernel, devicetree, linux-kernel, jic23, linux-input,
dmitry.torokhov
Cc: Eugen Hristev
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@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: nicolas.ferre, ludovic.desroches, alexandre.belloni, linux-iio,
linux-arm-kernel, devicetree, linux-kernel, jic23, linux-input,
dmitry.torokhov
Cc: Eugen Hristev
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@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@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@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: nicolas.ferre, ludovic.desroches, alexandre.belloni, linux-iio,
linux-arm-kernel, devicetree, linux-kernel, jic23, linux-input,
dmitry.torokhov
Cc: Eugen Hristev
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: nicolas.ferre, ludovic.desroches, alexandre.belloni, linux-iio,
linux-arm-kernel, devicetree, linux-kernel, jic23, linux-input,
dmitry.torokhov
Cc: Eugen Hristev
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: nicolas.ferre, ludovic.desroches, alexandre.belloni, linux-iio,
linux-arm-kernel, devicetree, linux-kernel, jic23, linux-input,
dmitry.torokhov
Cc: Eugen Hristev
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: nicolas.ferre, ludovic.desroches, alexandre.belloni, linux-iio,
linux-arm-kernel, devicetree, linux-kernel, jic23, linux-input,
dmitry.torokhov
Cc: Eugen Hristev
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
* [PATCH 08/14] iio: triggers: add private data to pollfuncs
From: Eugen Hristev @ 2017-12-22 15:07 UTC (permalink / raw)
To: nicolas.ferre, ludovic.desroches, alexandre.belloni, linux-iio,
linux-arm-kernel, devicetree, linux-kernel, jic23, linux-input,
dmitry.torokhov
Cc: Eugen Hristev
In-Reply-To: <1513955241-10985-1-git-send-email-eugen.hristev@microchip.com>
Add a private data pointer field to pollfunc struct.
This is useful in the trigger handler to get specific data
for the driver registering the pollfunc.
Signed-off-by: Eugen Hristev <eugen.hristev@microchip.com>
---
include/linux/iio/trigger_consumer.h | 9 +++++++++
1 file changed, 9 insertions(+)
diff --git a/include/linux/iio/trigger_consumer.h b/include/linux/iio/trigger_consumer.h
index 36e2a02..13be595 100644
--- a/include/linux/iio/trigger_consumer.h
+++ b/include/linux/iio/trigger_consumer.h
@@ -29,6 +29,7 @@ struct iio_trigger;
* @timestamp: some devices need a timestamp grabbed as soon
* as possible after the trigger - hence handler
* passes it via here.
+ * @p: private data for the poll func owner.
**/
struct iio_poll_func {
struct iio_dev *indio_dev;
@@ -38,6 +39,7 @@ struct iio_poll_func {
char *name;
int irq;
s64 timestamp;
+ void *p;
};
@@ -49,6 +51,13 @@ struct iio_poll_func
const char *fmt,
...);
void iio_dealloc_pollfunc(struct iio_poll_func *pf);
+
+static inline void
+iio_pollfunc_set_private_data(struct iio_poll_func *pf, void *p)
+{
+ pf->p = p;
+}
+
irqreturn_t iio_pollfunc_store_time(int irq, void *p);
void iio_trigger_notify_done(struct iio_trigger *trig);
--
2.7.4
^ permalink raw reply related
* [PATCH 09/14] iio: inkern: triggers: create helpers for OF trigger retrieval
From: Eugen Hristev @ 2017-12-22 15:07 UTC (permalink / raw)
To: nicolas.ferre, ludovic.desroches, alexandre.belloni, linux-iio,
linux-arm-kernel, devicetree, linux-kernel, jic23, linux-input,
dmitry.torokhov
Cc: Eugen Hristev
In-Reply-To: <1513955241-10985-1-git-send-email-eugen.hristev@microchip.com>
Create helper API to get trigger information from OF regarding
trigger producer/consumer for iio triggers.
The functions will search for matching trigger by name, similar
with channel retrieval
Signed-off-by: Eugen Hristev <eugen.hristev@microchip.com>
---
drivers/iio/inkern.c | 91 ++++++++++++++++++++++++++++++++++++
include/linux/iio/trigger_consumer.h | 1 +
2 files changed, 92 insertions(+)
diff --git a/drivers/iio/inkern.c b/drivers/iio/inkern.c
index 069defc..58bd18d 100644
--- a/drivers/iio/inkern.c
+++ b/drivers/iio/inkern.c
@@ -14,9 +14,13 @@
#include <linux/iio/iio.h>
#include "iio_core.h"
+#include "iio_core_trigger.h"
#include <linux/iio/machine.h>
#include <linux/iio/driver.h>
#include <linux/iio/consumer.h>
+#include <linux/iio/trigger.h>
+#include <linux/iio/trigger_consumer.h>
+
struct iio_map_internal {
struct iio_dev *indio_dev;
@@ -262,6 +266,39 @@ static struct iio_channel *of_iio_channel_get_all(struct device *dev)
return ERR_PTR(ret);
}
+#ifdef CONFIG_IIO_TRIGGER
+
+static struct iio_trigger *of_iio_trigger_get(struct device_node *np, int index)
+{
+ struct device *idev;
+ struct iio_dev *indio_dev;
+ int err;
+ struct of_phandle_args iiospec;
+ struct iio_trigger *trig;
+
+ err = of_parse_phandle_with_args(np, "io-triggers",
+ "#io-trigger-cells",
+ index, &iiospec);
+ if (err)
+ return ERR_PTR(err);
+
+ idev = bus_find_device(&iio_bus_type, NULL, iiospec.np,
+ iio_dev_node_match);
+ of_node_put(iiospec.np);
+ if (!idev)
+ return ERR_PTR(-EPROBE_DEFER);
+
+ indio_dev = dev_to_iio_dev(idev);
+
+ trig = iio_trigger_find_from_device(indio_dev, iiospec.args[0]);
+
+ if (!trig)
+ return ERR_PTR(-ENODEV);
+
+ return trig;
+}
+#endif /* CONFIG_IIO_TRIGGER */
+
#else /* CONFIG_OF */
static inline struct iio_channel *
@@ -275,6 +312,12 @@ static inline struct iio_channel *of_iio_channel_get_all(struct device *dev)
return NULL;
}
+static inline struct iio_trigger *of_iio_trigger_get(struct device_node *np,
+ int index)
+{
+ return NULL;
+}
+
#endif /* CONFIG_OF */
static struct iio_channel *iio_channel_get_sys(const char *name,
@@ -927,3 +970,51 @@ ssize_t iio_write_channel_ext_info(struct iio_channel *chan, const char *attr,
chan->channel, buf, len);
}
EXPORT_SYMBOL_GPL(iio_write_channel_ext_info);
+
+#ifdef CONFIG_IIO_TRIGGER
+struct iio_trigger *iio_trigger_find(struct device *dev, char *name)
+{
+ struct device_node *np = dev->of_node;
+ struct iio_trigger *trig = NULL;
+
+ /* Walk up the tree of devices looking for a matching iio trigger */
+ while (np) {
+ int index = 0;
+
+ /*
+ * For named iio triggers, first look up the name in the
+ * "io-trigger-names" property. If it cannot be found, the
+ * index will be an error code, and of_iio_trigger_get()
+ * will fail.
+ */
+ if (name)
+ index = of_property_match_string(np, "io-trigger-names",
+ name);
+ trig = of_iio_trigger_get(np, index);
+ if (!IS_ERR(trig) || PTR_ERR(trig) == -EPROBE_DEFER) {
+ break;
+ } else if (name && index >= 0) {
+ pr_err("ERROR: could not get IIO trigger %pOF:%s(%i)\n",
+ np, name ? name : "", index);
+ return trig;
+ }
+
+ /*
+ * No matching IIO trigger found on this node.
+ * If the parent node has a "io-trigger-ranges" property,
+ * then we can try one of its channels.
+ */
+ np = np->parent;
+ if (np && !of_get_property(np, "io-trigger-ranges", NULL))
+ return trig;
+ }
+
+ if (!trig || (IS_ERR(trig) && PTR_ERR(trig) != -EPROBE_DEFER))
+ dev_dbg(dev, "error retrieving trigger information\n");
+ else
+ dev_dbg(dev, "trigger found: %s\n", name);
+
+ return trig;
+}
+EXPORT_SYMBOL_GPL(iio_trigger_find);
+#endif /* CONFIG_IIO_TRIGGER */
diff --git a/include/linux/iio/trigger_consumer.h b/include/linux/iio/trigger_consumer.h
index 13be595..b2fc485 100644
--- a/include/linux/iio/trigger_consumer.h
+++ b/include/linux/iio/trigger_consumer.h
@@ -62,6 +62,7 @@ irqreturn_t iio_pollfunc_store_time(int irq, void *p);
void iio_trigger_notify_done(struct iio_trigger *trig);
+struct iio_trigger *iio_trigger_find(struct device *dev, char *name);
/*
* Two functions for common case where all that happens is a pollfunc
* is attached and detached from a trigger
--
2.7.4
^ permalink raw reply related
* [PATCH 10/14] iio: adc: at91-sama5d2_adc: force trigger removal on module remove
From: Eugen Hristev @ 2017-12-22 15:07 UTC (permalink / raw)
To: nicolas.ferre-UWL1GkI3JZL3oGB3hsPCZA,
ludovic.desroches-UWL1GkI3JZL3oGB3hsPCZA,
alexandre.belloni-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8,
linux-iio-u79uwXL29TY76Z2rM5mHXA,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA, jic23-DgEjT+Ai2ygdnm+yROfE0A,
linux-input-u79uwXL29TY76Z2rM5mHXA,
dmitry.torokhov-Re5JQEeQqe8AvxtiuMwx3w
Cc: Eugen Hristev
In-Reply-To: <1513955241-10985-1-git-send-email-eugen.hristev-UWL1GkI3JZL3oGB3hsPCZA@public.gmane.org>
On module remove, if we do not call trigger remove, the trigger
stays in the subsystem, and on further module insert, we will have
multiple triggers, and the old one is not usable.
Have to call the remove function on module remove to solve this.
Signed-off-by: Eugen Hristev <eugen.hristev-UWL1GkI3JZL3oGB3hsPCZA@public.gmane.org>
---
drivers/iio/adc/at91-sama5d2_adc.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/iio/adc/at91-sama5d2_adc.c b/drivers/iio/adc/at91-sama5d2_adc.c
index 4eff835..7b9febc 100644
--- a/drivers/iio/adc/at91-sama5d2_adc.c
+++ b/drivers/iio/adc/at91-sama5d2_adc.c
@@ -1180,6 +1180,9 @@ static int at91_adc_remove(struct platform_device *pdev)
struct iio_dev *indio_dev = platform_get_drvdata(pdev);
struct at91_adc_state *st = iio_priv(indio_dev);
+ if (st->selected_trig->hw_trig)
+ devm_iio_trigger_unregister(&indio_dev->dev, st->trig);
+
iio_device_unregister(indio_dev);
at91_adc_dma_disable(pdev);
--
2.7.4
^ permalink raw reply related
* [PATCH 11/14] iio: adc: at91-sama5d2_adc: optimize scan index for diff channels
From: Eugen Hristev @ 2017-12-22 15:07 UTC (permalink / raw)
To: nicolas.ferre, ludovic.desroches, alexandre.belloni, linux-iio,
linux-arm-kernel, devicetree, linux-kernel, jic23, linux-input,
dmitry.torokhov
Cc: Eugen Hristev
In-Reply-To: <1513955241-10985-1-git-send-email-eugen.hristev@microchip.com>
Optimize the scan index for the differential channels. Before, it
was single channel count + index of the first single channel
number of the differential pair. (e.g. 11+0, +2, +4, etc.)
Divide that number by two (since it's always even), and add it up
as a scan index to have consecutive numbered channels in the
index.
Signed-off-by: Eugen Hristev <eugen.hristev@microchip.com>
---
drivers/iio/adc/at91-sama5d2_adc.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/iio/adc/at91-sama5d2_adc.c b/drivers/iio/adc/at91-sama5d2_adc.c
index 7b9febc..9610393 100644
--- a/drivers/iio/adc/at91-sama5d2_adc.c
+++ b/drivers/iio/adc/at91-sama5d2_adc.c
@@ -209,7 +209,7 @@
.channel = num, \
.channel2 = num2, \
.address = addr, \
- .scan_index = num + AT91_SAMA5D2_SINGLE_CHAN_CNT, \
+ .scan_index = (num >> 1) + AT91_SAMA5D2_SINGLE_CHAN_CNT,\
.scan_type = { \
.sign = 's', \
.realbits = 12, \
--
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