* Re: [PATCH V3 RESEND] staging: sm750: Fix lynxfb_ops_imageblit() if image->depth != 1
From: Sudip Mukherjee @ 2015-05-08 9:55 UTC (permalink / raw)
To: linux-fbdev
In-Reply-To: <1431075868-15746-1-git-send-email-chenhc@lemote.com>
On Fri, May 08, 2015 at 05:04:28PM +0800, Huacai Chen wrote:
> If image->depth != 1, lynxfb_ops_imageblit() should fallback to call
> cfb_imageblit(), not return directly. Otherwise it can't display the
> boot logo.
why resending?
regards
sudip
^ permalink raw reply
* Re: [PATCHv6 05/10] ARM: mxs: fix in tree users of ssd1306
From: Maxime Ripard @ 2015-05-08 13:31 UTC (permalink / raw)
To: Shawn Guo
Cc: Tomi Valkeinen, Thomas Niederprüm, Sascha Hauer, plagnioj,
robh+dt, linux-fbdev, linux-kernel
In-Reply-To: <20150507112759.GC3162@dragon>
[-- Attachment #1: Type: text/plain, Size: 1472 bytes --]
Hi Shawn,
On Thu, May 07, 2015 at 07:28:01PM +0800, Shawn Guo wrote:
> On Thu, May 07, 2015 at 01:55:35PM +0300, Tomi Valkeinen wrote:
> > Hi,
> >
> > On 31/03/15 21:27, Thomas Niederprüm wrote:
> > > This patch updates the in tree-users of the SSD1306 controller for using
> > > the newly introduced DT properties.
> > >
> > > Signed-off-by: Thomas Niederprüm <niederp@physik.uni-kl.de>
> > > ---
> > > arch/arm/boot/dts/imx28-cfa10036.dts | 3 +++
> > > 1 file changed, 3 insertions(+)
> > >
> > > diff --git a/arch/arm/boot/dts/imx28-cfa10036.dts b/arch/arm/boot/dts/imx28-cfa10036.dts
> > > index b04b6b8..570aa33 100644
> > > --- a/arch/arm/boot/dts/imx28-cfa10036.dts
> > > +++ b/arch/arm/boot/dts/imx28-cfa10036.dts
> > > @@ -99,6 +99,9 @@
> > > solomon,height = <32>;
> > > solomon,width = <128>;
> > > solomon,page-offset = <0>;
> > > + solomon,com-lrremap;
> > > + solomon,com-invdir;
> > > + solomon,com-offset = <32>;
> > > };
> > > };
> >
> > Should this patch be merged separately via proper tree? I can take the
> > rest of the patches via fbdev tree. I can, of course, take this via
> > fbdev also, if I get an ack.
>
> @Maxime
>
> I assume you're the person, right?
I don't have this board right now to actually test, but the changes
look good.
Maxime
--
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* Re: [PATCH v4] staging: sm750fb: use arch_phys_wc_add() and ioremap_wc()
From: Greg KH @ 2015-05-10 13:09 UTC (permalink / raw)
To: Luis R. Rodriguez
Cc: sudipm.mukherjee, teddy.wang, devel, Juergen Gross, linux-fbdev,
Antonino Daplas, Daniel Vetter, Luis R. Rodriguez, linux-kernel,
luto, Tomi Valkeinen, Thomas Gleixner, Suresh Siddha, Dave Airlie,
Ingo Molnar, Jean-Christophe Plagniol-Villard, cocci
In-Reply-To: <1430784951-16505-1-git-send-email-mcgrof@do-not-panic.com>
On Mon, May 04, 2015 at 05:15:51PM -0700, Luis R. Rodriguez wrote:
> From: "Luis R. Rodriguez" <mcgrof@suse.com>
>
> The same area used for ioremap() is used for the MTRR area.
> Convert the driver from using the x86 specific MTRR code to
> the architecture agnostic arch_phys_wc_add(). arch_phys_wc_add()
> will avoid MTRR if write-combining is available, in order to
> take advantage of that also ensure the ioremap'd area is requested
> as write-combining.
>
> There are a few motivations for this:
>
> a) Take advantage of PAT when available
>
> b) Help bury MTRR code away, MTRR is architecture specific and on
> x86 its replaced by PAT
>
> c) Help with the goal of eventually using _PAGE_CACHE_UC over
> _PAGE_CACHE_UC_MINUS on x86 on ioremap_nocache() (see commit
> de33c442e titled "x86 PAT: fix performance drop for glx,
> use UC minus for ioremap(), ioremap_nocache() and
> pci_mmap_page_range()")
>
> The conversion done is expressed by the following Coccinelle
> SmPL patch, it additionally required manual intervention to
> address all the #ifdery and removal of redundant things which
> arch_phys_wc_add() already addresses such as verbose message
> about when MTRR fails and doing nothing when we didn't get
> an MTRR.
>
> @ mtrr_found @
> expression index, base, size;
> @@
>
> -index = mtrr_add(base, size, MTRR_TYPE_WRCOMB, 1);
> +index = arch_phys_wc_add(base, size);
>
> @ mtrr_rm depends on mtrr_found @
> expression mtrr_found.index, mtrr_found.base, mtrr_found.size;
> @@
>
> -mtrr_del(index, base, size);
> +arch_phys_wc_del(index);
>
> @ mtrr_rm_zero_arg depends on mtrr_found @
> expression mtrr_found.index;
> @@
>
> -mtrr_del(index, 0, 0);
> +arch_phys_wc_del(index);
>
> @ mtrr_rm_fb_info depends on mtrr_found @
> struct fb_info *info;
> expression mtrr_found.index;
> @@
>
> -mtrr_del(index, info->fix.smem_start, info->fix.smem_len);
> +arch_phys_wc_del(index);
>
> @ ioremap_replace_nocache depends on mtrr_found @
> struct fb_info *info;
> expression base, size;
> @@
>
> -info->screen_base = ioremap_nocache(base, size);
> +info->screen_base = ioremap_wc(base, size);
>
> @ ioremap_replace_default depends on mtrr_found @
> struct fb_info *info;
> expression base, size;
> @@
>
> -info->screen_base = ioremap(base, size);
> +info->screen_base = ioremap_wc(base, size);
>
> Generated-by: Coccinelle SmPL
> Cc: Sudip Mukherjee <sudipm.mukherjee@gmail.com>
> Cc: Teddy Wang <teddy.wang@siliconmotion.com>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: Suresh Siddha <sbsiddha@gmail.com>
> Cc: Ingo Molnar <mingo@elte.hu>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Juergen Gross <jgross@suse.com>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: Andy Lutomirski <luto@amacapital.net>
> Cc: Dave Airlie <airlied@redhat.com>
> Cc: Antonino Daplas <adaplas@gmail.com>
> Cc: Jean-Christophe Plagniol-Villard <plagnioj@jcrosoft.com>
> Cc: Tomi Valkeinen <tomi.valkeinen@ti.com>
> Cc: devel@driverdev.osuosl.org
> Cc: linux-fbdev@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org
> Signed-off-by: Luis R. Rodriguez <mcgrof@suse.com>
> ---
> drivers/staging/sm750fb/sm750.c | 36 ++++--------------------------------
> drivers/staging/sm750fb/sm750.h | 3 ---
> drivers/staging/sm750fb/sm750_hw.c | 3 +--
> 3 files changed, 5 insertions(+), 37 deletions(-)
Still doesn't apply to my tree:
checking file drivers/staging/sm750fb/sm750.c
Hunk #4 FAILED at 1150.
1 out of 6 hunks FAILED
checking file drivers/staging/sm750fb/sm750.h
checking file drivers/staging/sm750fb/sm750_hw.c
^ permalink raw reply
* Re: [PATCH v3 1/2] staging: sm750fb: cleanup white space
From: Greg KH @ 2015-05-10 13:09 UTC (permalink / raw)
To: Charles Rose
Cc: sudipm.mukherjee, teddy.wang, linux-fbdev, devel, linux-kernel
In-Reply-To: <1430881664-13727-1-git-send-email-charles.rose.linux@gmail.com>
On Tue, May 05, 2015 at 11:07:43PM -0400, Charles Rose wrote:
> This patch fixes "space prohibited" errors reported by checkpatch.pl.
>
> Signed-off-by: Charles Rose <charles.rose.linux@gmail.com>
> ---
> drivers/staging/sm750fb/ddk750_chip.c | 12 ++++++------
> drivers/staging/sm750fb/ddk750_power.c | 8 ++++----
> drivers/staging/sm750fb/sm750_accel.c | 2 +-
> drivers/staging/sm750fb/sm750_help.h | 2 +-
> 4 files changed, 12 insertions(+), 12 deletions(-)
Doesn't apply to my tree :(
^ permalink raw reply
* Re: [PATCH V3 RESEND] staging: sm750: Fix lynxfb_ops_imageblit() if image->depth != 1
From: Greg Kroah-Hartman @ 2015-05-10 13:17 UTC (permalink / raw)
To: linux-fbdev
In-Reply-To: <1431075868-15746-1-git-send-email-chenhc@lemote.com>
On Fri, May 08, 2015 at 05:04:28PM +0800, Huacai Chen wrote:
> If image->depth != 1, lynxfb_ops_imageblit() should fallback to call
> cfb_imageblit(), not return directly. Otherwise it can't display the
> boot logo.
>
> Cc: Teddy Wang <teddy.wang@siliconmotion.com>
> Acked-by: Sudip Mukherjee <sudipm.mukherjee@gmail.com>
> Signed-off-by: Huacai Chen <chenhc@lemote.com>
> ---
> V2: Coding style ajustment.
> V3: Add TODO comments.
>
> drivers/staging/sm750fb/TODO | 1 +
> drivers/staging/sm750fb/sm750.c | 3 +++
> 2 files changed, 4 insertions(+), 0 deletions(-)
Doesn't apply without fuzz, so something isn't synced up properly here.
Please refresh it against my tree and resend.
thanks,
greg k-h
^ permalink raw reply
* Re: [PATCH v2 0/3] add cursor blink interval terminal escape sequence
From: Greg Kroah-Hartman @ 2015-05-10 13:32 UTC (permalink / raw)
To: Pavel Machek
Cc: Scot Doyle, Michael Kerrisk, Jiri Slaby, Tomi Valkeinen,
Jean-Christophe Plagniol-Villard, Geert Uytterhoeven,
linux-kernel, linux-fbdev, linux-man, linux-api
In-Reply-To: <20150328075443.GD5248@xo-6d-61-c0.localdomain>
On Sat, Mar 28, 2015 at 08:54:43AM +0100, Pavel Machek wrote:
> On Thu 2015-03-26 13:51:04, Scot Doyle wrote:
> > v2: Add documentation to console_codes man page (man-pages repo)
> >
> > This patch series adds an escape sequence to specify the current console's
> > cursor blink interval. The default interval is set to fbcon's currently
> > hardcoded 200 msecs.
>
> Actually... Greg, can you import console_codes.4 into kernel tree somehow?
Is that file part of the manpages project? If so, it's fine where it
is, otherwise feel free to send a patch.
thanks,
greg k-h
^ permalink raw reply
* [PATCH V4] staging: sm750: Fix lynxfb_ops_imageblit() if image->depth != 1
From: Huacai Chen @ 2015-05-11 3:08 UTC (permalink / raw)
To: linux-fbdev
If image->depth != 1, lynxfb_ops_imageblit() should fallback to call
cfb_imageblit(), not return directly. Otherwise it can't display the
boot logo.
Cc: Teddy Wang <teddy.wang@siliconmotion.com>
Acked-by: Sudip Mukherjee <sudipm.mukherjee@gmail.com>
Signed-off-by: Huacai Chen <chenhc@lemote.com>
---
V2: Coding style ajustment.
V3: Add TODO comments.
V4: Rebase on latest code.
drivers/staging/sm750fb/TODO | 1 +
drivers/staging/sm750fb/sm750.c | 3 +++
2 files changed, 4 insertions(+), 0 deletions(-)
diff --git a/drivers/staging/sm750fb/TODO b/drivers/staging/sm750fb/TODO
index bc16172..f8698498 100644
--- a/drivers/staging/sm750fb/TODO
+++ b/drivers/staging/sm750fb/TODO
@@ -2,6 +2,7 @@ TODO:
- lots of checkpatch cleanup
- use kernel coding style
- refine the code and remove unused code
+- Implement hardware acceleration for imageblit if image->depth > 1
- check on hardware effects of removal of USE_HW_I2C and USE_DVICHIP (these two
are supposed to be sample code which is given here if someone wants to
use those functionalities)
diff --git a/drivers/staging/sm750fb/sm750.c b/drivers/staging/sm750fb/sm750.c
index 3c7ea95..6f78de3 100644
--- a/drivers/staging/sm750fb/sm750.c
+++ b/drivers/staging/sm750fb/sm750.c
@@ -279,7 +279,10 @@ static void lynxfb_ops_imageblit(struct fb_info *info,
}
goto _do_work;
}
+ /* TODO: Implement hardware acceleration for image->depth > 1 */
+ cfb_imageblit(info, image);
return;
+
_do_work:
/*
* If not use spin_lock, system will die if user load driver
--
1.7.7.3
^ permalink raw reply related
* Re: [PATCHv3 0/4] add devm_of_phy_get_by_index and update platform drivers
From: Kishon Vijay Abraham I @ 2015-05-11 13:56 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <Pine.LNX.4.44L0.1504231031170.1529-100000@iolanthe.rowland.org>
Hi,
On Thursday 23 April 2015 08:01 PM, Alan Stern wrote:
> On Wed, 22 Apr 2015, Arun Ramamurthy wrote:
>
>> This patch set adds a new API to get phy by index when multiple
>> phys are present. This patch is based on discussion with Arnd Bergmann
>> about dt bindings for multiple phys.
>>
>> History:
>> v1:
>> - Removed null pointers on Dmitry's suggestion
>> - Improved documentation in commit messages
>> - Exported new phy api
>> v2:
>> - EHCI and OHCI platform Kconfigs select Generic Phy
>> to fix build errors in certain configs.
>> v3:
>> - Made GENERIC_PHY an invisible option so
>> that other configs can select it
>> - Added stubs for devm_of_phy_get_by_index
>> - Reformated code
>>
>> Arun Ramamurthy (4):
>> phy: phy-core: Make GENERIC_PHY an invisible option
>> phy: core: Add devm_of_phy_get_by_index to phy-core
>> usb: ehci-platform: Use devm_of_phy_get_by_index
>> usb: ohci-platform: Use devm_of_phy_get_by_index
>
> For patches 3 and 4:
>
> Acked-by: Alan Stern <stern@rowland.harvard.edu>
merged this to linux-phy.
Thanks
Kishon
^ permalink raw reply
* Re: [PATCH v3 2/3] Phy: DT binding documentation for Broadcom Cygnus USB PHY driver
From: Kishon Vijay Abraham I @ 2015-05-11 14:23 UTC (permalink / raw)
To: Arun Ramamurthy, Rob Herring, Pawel Moll, Mark Rutland,
Ian Campbell, Kumar Gala, Russell King, Gregory CLEMENT,
Gabriel FERNANDEZ, Jason Cooper, Thomas Petazzoni,
Greg Kroah-Hartman, Arnd Bergmann
Cc: devicetree, linux-kernel, linux-fbdev, Dmitry Torokhov,
Anatol Pomazau, Jonathan Richardson, Scott Branden, Ray Jui,
bcm-kernel-feedback-list
In-Reply-To: <1429744479-10410-3-git-send-email-arun.ramamurthy@broadcom.com>
Hi,
On Thursday 23 April 2015 04:44 AM, Arun Ramamurthy wrote:
> Broadcom's Cygnus chip has a USB 2.0 host controller connected to
> three separate phys. One of the phs (port 2) is also connectd to
> a usb 2.0 device controller
>
> Signed-off-by: Arun Ramamurthy <arun.ramamurthy@broadcom.com>
> Reviewed-by: Ray Jui <rjui@broadcom.com>
> Reviewed-by: Scott Branden <sbranden@broadcom.com>
> ---
> .../bindings/phy/brcm,cygnus-usb-phy.txt | 69 ++++++++++++++++++++++
> include/dt-bindings/phy/phy.h | 2 +
> 2 files changed, 71 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/phy/brcm,cygnus-usb-phy.txt
>
> diff --git a/Documentation/devicetree/bindings/phy/brcm,cygnus-usb-phy.txt b/Documentation/devicetree/bindings/phy/brcm,cygnus-usb-phy.txt
> new file mode 100644
> index 0000000..ec62044
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/phy/brcm,cygnus-usb-phy.txt
> @@ -0,0 +1,69 @@
> +BROADCOM CYGNUS USB PHY
> +
> +Required Properties:
> + - compatible: brcm,cygnus-usb-phy
> + - reg : usbphy_regs - Base address of phy registers
> + usb2h_idm_regs - Base address of host idm registers
> + usb2d_idm_regs - Base address of device idm registers
> + - phy-cells - must be 1 for each port declared. The node
> + that uses the phy must provide either PHY_CONFIG_DEVICE for device
> + or PHY_CONFIG_HOST for host
> +
> +NOTE: port 0 and port 1 are host only and port 2 can be configured for host or
> +device.
> +
> +Example of phy :
> + usbphy0: usbphy@0x0301c000 {
> + compatible = "brcm,cygnus-usb-phy";
> + reg = <0x0301c000 0x2000>,
> + <0x18115000 0x1000>,
> + <0x18111000 0x1000>;
> + status = "okay";
> +
> + #address-cells = <1>;
> + #size-cells = <0>;
> + usbphy0_0: usbphy0@0 {
> + #phy-cells = <1>;
> + reg = <0>;
> + status = "okay";
> + phy-supply = <&vbus_p0>;
> + };
> +
> + usbphy0_1: usbphy0@1 {
> + #phy-cells = <1>;
> + reg = <1>;
> + status = "okay";
> + };
> +
> + usbphy0_2: usbphy0@2 {
> + #phy-cells = <1>;
> + reg = <2>;
> + status = "okay";
> + phy-supply = <&vbus_p2>;
> + };
> + };
> +
> +Example of node using the phy:
> +
> + /* This nodes declares all three ports as host */
> +
> + ehci0: usb@0x18048000 {
> + compatible = "generic-ehci";
> + reg = <0x18048000 0x100>;
> + interrupts = <GIC_SPI 72 IRQ_TYPE_LEVEL_HIGH>;
> + phys = <&usbphy0_0 PHY_CONFIG_HOST &usbphy0_1 PHY_CONFIG_HOST &usbphy0_2 PHY_CONFIG_HOST>;
> + status = "okay";
> + };
> +
> + /*
> + * This node declares port 2 phy
> + * and configures it for device
> + */
> +
> + usbd_udc_dwc1: usbd_udc_dwc@0x1804c000 {
> + compatible = "iproc-udc";
> + reg = <0x1804c000 0x2000>;
> + interrupts = <GIC_SPI 123 IRQ_TYPE_LEVEL_HIGH>;
> + phys = <&usbphy0_2 PHY_CONFIG_DEVICE>;
> + phy-names = "usb";
> + };
> diff --git a/include/dt-bindings/phy/phy.h b/include/dt-bindings/phy/phy.h
> index 6c90193..3f6b1ac 100644
> --- a/include/dt-bindings/phy/phy.h
> +++ b/include/dt-bindings/phy/phy.h
> @@ -15,5 +15,7 @@
> #define PHY_TYPE_PCIE 2
> #define PHY_TYPE_USB2 3
> #define PHY_TYPE_USB3 4
> +#define PHY_CONFIG_HOST 1
> +#define PHY_CONFIG_DEVICE 0
'0' and '1' are already defined for "PHY_NONE" and "PHY_TYPE_SATA". Is this for
USB2 or for USB3?
Thanks
Kishon
^ permalink raw reply
* Re: [PATCH v3 3/3] phy: cygnus-usbphy: Add Broadcom Cygnus USB phy driver
From: Kishon Vijay Abraham I @ 2015-05-11 14:49 UTC (permalink / raw)
To: Arun Ramamurthy, Rob Herring, Pawel Moll, Mark Rutland,
Ian Campbell, Kumar Gala, Russell King, Gregory CLEMENT,
Gabriel FERNANDEZ, Jason Cooper, Thomas Petazzoni,
Greg Kroah-Hartman, Arnd Bergmann
Cc: devicetree, linux-kernel, linux-fbdev, Dmitry Torokhov,
Anatol Pomazau, Jonathan Richardson, Scott Branden, Ray Jui,
bcm-kernel-feedback-list, Dmitry Torokhov
In-Reply-To: <1429744479-10410-4-git-send-email-arun.ramamurthy@broadcom.com>
Hi,
On Thursday 23 April 2015 04:44 AM, Arun Ramamurthy wrote:
> This driver adds support for USB 2.0 host and device phy for
> Broadcom's Cygnus chipset. The host controller is connected to
> three separate phys and one of the phys (port 2) is connected to
> the device controller
>
> Signed-off-by: Arun Ramamurthy <arun.ramamurthy@broadcom.com>
> Signed-off-by: Dmitry Torokhov <dtor@chromium.org>
> Reviewed-by: Ray Jui <rjui@broadcom.com>
> Reviewed-by: Scott Branden <sbranden@broadcom.com>
> ---
> drivers/phy/Kconfig | 12 +
> drivers/phy/Makefile | 1 +
> drivers/phy/phy-bcm-cygnus-usb.c | 504 +++++++++++++++++++++++++++++++++++++++
> 3 files changed, 517 insertions(+)
> create mode 100644 drivers/phy/phy-bcm-cygnus-usb.c
>
> diff --git a/drivers/phy/Kconfig b/drivers/phy/Kconfig
> index 2962de2..0aa62dc 100644
> --- a/drivers/phy/Kconfig
> +++ b/drivers/phy/Kconfig
> @@ -291,4 +291,16 @@ config PHY_QCOM_UFS
> help
> Support for UFS PHY on QCOM chipsets.
>
> +config PHY_BCM_CYGNUS_USB
> + tristate "Broadcom Cygnus USB PHY support"
> + depends on OF
> + depends on ARCH_BCM_CYGNUS || COMPILE_TEST
> + select GENERIC_PHY
> + default ARCH_BCM_CYGNUS
> + help
> + Enable this to support the USB PHY in Broadcom's Cygnus chip.
> + The phys are capable of supporting host mode for all ports
> + and device mode for port 2. The host or device configuration is
> + read from the device tree.
> +
> endmenu
> diff --git a/drivers/phy/Makefile b/drivers/phy/Makefile
> index f080e1b..47a37fa 100644
> --- a/drivers/phy/Makefile
> +++ b/drivers/phy/Makefile
> @@ -6,6 +6,7 @@ obj-$(CONFIG_GENERIC_PHY) += phy-core.o
> obj-$(CONFIG_PHY_BERLIN_USB) += phy-berlin-usb.o
> obj-$(CONFIG_PHY_BERLIN_SATA) += phy-berlin-sata.o
> obj-$(CONFIG_ARMADA375_USBCLUSTER_PHY) += phy-armada375-usb2.o
> +obj-$(CONFIG_PHY_BCM_CYGNUS_USB) += phy-bcm-cygnus-usb.o
> obj-$(CONFIG_BCM_KONA_USB2_PHY) += phy-bcm-kona-usb2.o
> obj-$(CONFIG_PHY_EXYNOS_DP_VIDEO) += phy-exynos-dp-video.o
> obj-$(CONFIG_PHY_EXYNOS_MIPI_VIDEO) += phy-exynos-mipi-video.o
> diff --git a/drivers/phy/phy-bcm-cygnus-usb.c b/drivers/phy/phy-bcm-cygnus-usb.c
> new file mode 100644
> index 0000000..e718404
> --- /dev/null
> +++ b/drivers/phy/phy-bcm-cygnus-usb.c
> @@ -0,0 +1,504 @@
> +/*
> + * Copyright (C) 2015 Broadcom Corporation
> + *
> + * 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 version 2.
> + *
> + * This program is distributed "as is" WITHOUT ANY WARRANTY of any
> + * kind, whether express or implied; without even the implied warranty
> + * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + */
> +
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/io.h>
> +#include <linux/of.h>
> +#include <linux/of_address.h>
> +#include <linux/phy/phy.h>
> +#include <linux/delay.h>
> +
> +#define CDRU_USBPHY_CLK_RST_SEL_OFFSET 0x11b4
> +#define CDRU_USBPHY2_HOST_DEV_SEL_OFFSET 0x11b8
> +#define CDRU_SPARE_REG_0_OFFSET 0x1238
> +#define CRMU_USB_PHY_AON_CTRL_OFFSET 0x00028
> +#define CDRU_USB_DEV_SUSPEND_RESUME_CTRL_OFFSET 0x1210
> +#define CDRU_USBPHY_P0_STATUS_OFFSET 0x11D0
> +#define CDRU_USBPHY_P1_STATUS_OFFSET 0x11E8
> +#define CDRU_USBPHY_P2_STATUS_OFFSET 0x1200
> +#define CDRU_USBPHY_USBPHY_ILDO_ON_FLAG 1
> +#define CDRU_USBPHY_USBPHY_PLL_LOCK 0
> +#define CDRU_USB_DEV_SUSPEND_RESUME_CTRL_DISABLE 0
> +#define PHY2_DEV_HOST_CTRL_SEL_HOST 1
> +
> +#define CRMU_USBPHY_P0_AFE_CORERDY_VDDC 1
> +#define CRMU_USBPHY_P0_RESETB 2
> +#define CRMU_USBPHY_P1_AFE_CORERDY_VDDC 9
> +#define CRMU_USBPHY_P1_RESETB 10
> +#define CRMU_USBPHY_P2_AFE_CORERDY_VDDC 17
> +#define CRMU_USBPHY_P2_RESETB 18
> +
> +#define USB2_IDM_IDM_IO_CONTROL_DIRECT_OFFSET 0x0408
> +#define USB2_IDM_IDM_IO_CONTROL_DIRECT__clk_enable 0
> +#define USB2_IDM_IDM_RESET_CONTROL_OFFSET 0x0800
> +#define USB2_IDM_IDM_RESET_CONTROL__RESET 0
> +
> +#define PLL_LOCK_RETRY_COUNT 1000
> +#define MAX_PHY_PORTS 3
> +
> +static int power_bit[] = {CRMU_USBPHY_P0_AFE_CORERDY_VDDC,
> + CRMU_USBPHY_P1_AFE_CORERDY_VDDC,
> + CRMU_USBPHY_P2_AFE_CORERDY_VDDC};
> +static int reset_bit[] = {CRMU_USBPHY_P0_RESETB,
> + CRMU_USBPHY_P1_RESETB,
> + CRMU_USBPHY_P2_RESETB};
> +static int status_reg[] = {CDRU_USBPHY_P0_STATUS_OFFSET,
> + CDRU_USBPHY_P1_STATUS_OFFSET,
> + CDRU_USBPHY_P2_STATUS_OFFSET};
> +
> +struct bcm_phy_instance;
> +
> +struct bcm_phy_driver {
> + void __iomem *usbphy_regs;
> + void __iomem *usb2h_idm_regs;
> + void __iomem *usb2d_idm_regs;
> + struct bcm_phy_instance *ports[MAX_PHY_PORTS];
> + spinlock_t lock;
> + bool idm_host_enabled;
> +};
> +
> +struct bcm_phy_instance {
> + struct bcm_phy_driver *driver;
> + struct phy *generic_phy;
> + int port_no;
> + bool host_mode; /* true - Host, false - device */
> + bool power;
> +};
> +
> +static inline int bcm_phy_cdru_usbphy_status_wait(u32 usb_reg, int reg_bit,
> + struct bcm_phy_driver *phy_driver)
> +{
> + /* Wait for the PLL lock status */
> + int retry = PLL_LOCK_RETRY_COUNT;
> + u32 reg_val;
> +
> + do {
> + udelay(1);
> + reg_val = readl(phy_driver->usbphy_regs +
> + usb_reg);
> + if (reg_val & (1 << reg_bit))
> + return 0;
> + } while (--retry > 0);
> +
> + return -EBUSY;
> +
> +}
> +
> +static struct phy *bcm_usb_phy_xlate(struct device *dev,
> + struct of_phandle_args *args)
> +{
> + struct bcm_phy_driver *phy_driver = dev_get_drvdata(dev);
> + struct bcm_phy_instance *port = NULL;
> + int i;
> +
> + if (!phy_driver)
> + return ERR_PTR(-ENODEV);
> +
> + if (WARN_ON(args->args_count != 1))
> + return ERR_PTR(-ENODEV);
> +
> + if (WARN_ON(args->args[0] < 0 || args->args[0] > 1))
> + return ERR_PTR(-ENODEV);
> +
> + for (i = 0; i < ARRAY_SIZE(phy_driver->ports); i++) {
> + struct bcm_phy_instance *p = phy_driver->ports[i];
> +
> + if (p && p->generic_phy->dev.of_node = args->np) {
> + port = p;
> + break;
> + }
> + }
> +
> + if (!port) {
> + dev_err(dev, "Failed to locate phy %s\n", args->np->name);
> + return ERR_PTR(-EINVAL);
> + }
> +
> + port->host_mode = args->args[0];
> +
> + return port->generic_phy;
> +}
> +
> +static int bcm_phy_init(struct phy *generic_phy)
> +{
> + struct bcm_phy_instance *port = phy_get_drvdata(generic_phy);
> + struct bcm_phy_driver *phy_driver = port->driver;
> + unsigned long flags;
> + u32 reg_val;
> +
> + spin_lock_irqsave(&phy_driver->lock, flags);
> +
> + /*
> + * Only PORT 2 is capable of being device and host
> + * Default setting is device, check if it is set to host.
> + */
> + if (port->port_no != 2) {
can be a goto here with a label in the end.
> + spin_unlock_irqrestore(&phy_driver->lock, flags);
> + return 0;
> + }
> +
> + if (port->host_mode) {
> + writel(PHY2_DEV_HOST_CTRL_SEL_HOST,
> + phy_driver->usbphy_regs +
> + CDRU_USBPHY2_HOST_DEV_SEL_OFFSET);
> + } else {
> + /*
> + * Disable suspend/resume signals to device controller
> + * when a port is in device mode.
> + */
> + reg_val = readl(phy_driver->usbphy_regs +
> + CDRU_USB_DEV_SUSPEND_RESUME_CTRL_OFFSET);
> + reg_val |> + 1 << CDRU_USB_DEV_SUSPEND_RESUME_CTRL_DISABLE;
> + writel(reg_val, phy_driver->usbphy_regs +
> + CDRU_USB_DEV_SUSPEND_RESUME_CTRL_OFFSET);
> + }
> +
> +
^^spurious blank line.
> + spin_unlock_irqrestore(&phy_driver->lock, flags);
> + return 0;
> +}
> +
> +static int bcm_phy_shutdown(struct phy *generic_phy)
> +{
> + struct bcm_phy_instance *port = phy_get_drvdata(generic_phy);
> + struct bcm_phy_driver *phy_driver = port->driver;
> + unsigned long flags;
> + int i;
> + u32 reg_val, powered_on_phy;
> + bool power_off_flag = true;
> +
> + spin_lock_irqsave(&phy_driver->lock, flags);
> +
> + /* power down the phy */
> + reg_val = readl(phy_driver->usbphy_regs +
> + CRMU_USB_PHY_AON_CTRL_OFFSET);
> + reg_val &= ~(1 << power_bit[port->port_no]);
> + reg_val &= ~(1 << reset_bit[port->port_no]);
> + writel(reg_val, phy_driver->usbphy_regs +
> + CRMU_USB_PHY_AON_CTRL_OFFSET);
> + port->power = false;
> +
> + /*
> + * If a port is configured to device and it is being shutdown,
> + * turn off the clocks to the usb device controller.
> + */
> + if (port->port_no = 2 && !port->host_mode) {
> + reg_val = readl(phy_driver->usb2d_idm_regs +
> + USB2_IDM_IDM_IO_CONTROL_DIRECT_OFFSET);
> + reg_val &= ~(1 << USB2_IDM_IDM_IO_CONTROL_DIRECT__clk_enable);
please avoid using camel case..
> + writel(reg_val, phy_driver->usb2d_idm_regs +
> + USB2_IDM_IDM_IO_CONTROL_DIRECT_OFFSET);
> +
> + reg_val = readl(phy_driver->usb2d_idm_regs +
> + USB2_IDM_IDM_RESET_CONTROL_OFFSET);
> + reg_val |= 1 << USB2_IDM_IDM_RESET_CONTROL__RESET;
> + writel(reg_val, phy_driver->usb2d_idm_regs +
> + USB2_IDM_IDM_RESET_CONTROL_OFFSET);
> +
> + spin_unlock_irqrestore(&phy_driver->lock, flags);
> + return 0;
> + }
> +
> + /*
> + * If the phy being shutdown provides clock and reset to
> + * the host controller, change it do a different powered on phy
> + * If all phys are powered off, shut of the host controller
> + */
> + reg_val = readl(phy_driver->usbphy_regs +
> + CDRU_USBPHY_CLK_RST_SEL_OFFSET);
> + powered_on_phy = reg_val;
The above statement is useless.
> + if (reg_val = port->port_no) {
> + for (i = 0; i < ARRAY_SIZE(phy_driver->ports); i++) {
> + if (phy_driver->ports[i] &&
> + phy_driver->ports[i]->power &&
> + phy_driver->ports[i]->host_mode) {
> + power_off_flag = false;
> + powered_on_phy = i;
> + break;
> + }
> + }
> + }
> +
> + if (power_off_flag) {
> + /*
> + * Put the host controller into reset state and
> + * disable clock.
> + */
> + reg_val = readl(phy_driver->usb2h_idm_regs +
> + USB2_IDM_IDM_IO_CONTROL_DIRECT_OFFSET);
> + reg_val &> + ~(1 << USB2_IDM_IDM_IO_CONTROL_DIRECT__clk_enable);
> + writel(reg_val, phy_driver->usb2h_idm_regs +
> + USB2_IDM_IDM_IO_CONTROL_DIRECT_OFFSET);
> +
> + reg_val = readl(phy_driver->usb2h_idm_regs +
> + USB2_IDM_IDM_RESET_CONTROL_OFFSET);
> + reg_val |= (1 << USB2_IDM_IDM_RESET_CONTROL__RESET);
> + writel(reg_val, phy_driver->usb2h_idm_regs +
> + USB2_IDM_IDM_RESET_CONTROL_OFFSET);
> + phy_driver->idm_host_enabled = 0;
> + } else {
> + writel(powered_on_phy, phy_driver->usbphy_regs +
> + CDRU_USBPHY_CLK_RST_SEL_OFFSET);
> + }
> +
> + spin_unlock_irqrestore(&phy_driver->lock, flags);
> +
> + return 0;
> +}
> +
> +static int bcm_phy_poweron(struct phy *generic_phy)
> +{
> + struct bcm_phy_instance *port = phy_get_drvdata(generic_phy);
> + struct bcm_phy_driver *phy_driver = port->driver;
> + unsigned long flags;
> + int ret;
> + u32 reg_val;
> + bool clock_reset_flag = true;
> +
> + spin_lock_irqsave(&phy_driver->lock, flags);
> +
> + /* Bring the AFE block out of reset to start powering up the PHY */
> + reg_val = readl(phy_driver->usbphy_regs + CRMU_USB_PHY_AON_CTRL_OFFSET);
> + reg_val |= 1 << power_bit[port->port_no];
> + writel(reg_val, phy_driver->usbphy_regs + CRMU_USB_PHY_AON_CTRL_OFFSET);
> +
> + /* Check for power on and PLL lock */
> + ret = bcm_phy_cdru_usbphy_status_wait(status_reg[port->port_no],
> + CDRU_USBPHY_USBPHY_ILDO_ON_FLAG, phy_driver);
> + if (ret < 0) {
> + dev_err(&generic_phy->dev,
> + "Timed out waiting for USBPHY_ILDO_ON_FLAG on port %d",
> + port->port_no);
> + goto err_shutdown;
> + }
> + ret = bcm_phy_cdru_usbphy_status_wait(status_reg[port->port_no],
> + CDRU_USBPHY_USBPHY_PLL_LOCK, phy_driver);
> + if (ret < 0) {
> + dev_err(&generic_phy->dev,
> + "Timed out waiting for USBPHY_PLL_LOCK on port %d",
> + port->port_no);
> + goto err_shutdown;
> + }
> + port->power = true;
> +
> + /* Check if the port 2 is configured for device */
> + if (port->port_no = 2 && !port->host_mode) {
> + /*
> + * Enable clock to USB device and get the USB device
> + * out of reset.
> + */
> + reg_val = readl(phy_driver->usb2d_idm_regs +
> + USB2_IDM_IDM_IO_CONTROL_DIRECT_OFFSET);
> + reg_val |= 1 << USB2_IDM_IDM_IO_CONTROL_DIRECT__clk_enable;
> + writel(reg_val, phy_driver->usb2d_idm_regs +
> + USB2_IDM_IDM_IO_CONTROL_DIRECT_OFFSET);
> +
> + reg_val = readl(phy_driver->usb2d_idm_regs +
> + USB2_IDM_IDM_RESET_CONTROL_OFFSET);
> + reg_val &= ~(1 << USB2_IDM_IDM_RESET_CONTROL__RESET);
> + writel(reg_val, phy_driver->usb2d_idm_regs +
> + USB2_IDM_IDM_RESET_CONTROL_OFFSET);
> +
> + } else {
> + reg_val = readl(phy_driver->usbphy_regs +
> + CDRU_USBPHY_CLK_RST_SEL_OFFSET);
> +
> + /*
> + * Check if the phy that is configured to provide clock
> + * and reset is powered on.
> + */
> + if (reg_val >= 0 && reg_val < ARRAY_SIZE(phy_driver->ports))
> + if (phy_driver->ports[reg_val])
> + if (phy_driver->ports[reg_val]->power)
> + clock_reset_flag = false;
> +
> + /* if not set the current phy */
> + if (clock_reset_flag) {
> + reg_val = port->port_no;
> + writel(reg_val, phy_driver->usbphy_regs +
> + CDRU_USBPHY_CLK_RST_SEL_OFFSET);
> + }
> + }
> +
> + if (!phy_driver->idm_host_enabled) {
> + /* Enable clock to USB and get the USB out of reset */
> + reg_val = readl(phy_driver->usb2h_idm_regs +
> + USB2_IDM_IDM_IO_CONTROL_DIRECT_OFFSET);
> + reg_val |= 1 << USB2_IDM_IDM_IO_CONTROL_DIRECT__clk_enable;
> + writel(reg_val, phy_driver->usb2h_idm_regs +
> + USB2_IDM_IDM_IO_CONTROL_DIRECT_OFFSET);
> +
> + reg_val = readl(phy_driver->usb2h_idm_regs +
> + USB2_IDM_IDM_RESET_CONTROL_OFFSET);
> + reg_val &= ~(1 << USB2_IDM_IDM_RESET_CONTROL__RESET);
> + writel(reg_val, phy_driver->usb2h_idm_regs +
> + USB2_IDM_IDM_RESET_CONTROL_OFFSET);
> + phy_driver->idm_host_enabled = true;
> + }
> +
> + spin_unlock_irqrestore(&phy_driver->lock, flags);
> + return 0;
> +
> +err_shutdown:
> + spin_unlock_irqrestore(&phy_driver->lock, flags);
> + bcm_phy_shutdown(generic_phy);
Instead of calling the phy_shutdown, you can just power off the PHY.
> + return ret;
> +}
> +
> +static struct phy_ops ops = {
> + .init = bcm_phy_init,
> + .power_on = bcm_phy_poweron,
> + .power_off = bcm_phy_shutdown,
> +};
> +
> +static const struct of_device_id bcm_phy_dt_ids[] = {
> + { .compatible = "brcm,cygnus-usb-phy", },
> + { }
> +};
> +
> +static int bcm_phy_probe(struct platform_device *pdev)
> +{
> + struct device *dev = &pdev->dev;
> + struct bcm_phy_driver *phy_driver;
> + struct phy_provider *phy_provider;
> + struct device_node *child;
> + struct resource *res;
> + int error;
> + u32 reg_val;
> +
> + phy_driver = devm_kzalloc(dev, sizeof(struct bcm_phy_driver),
> + GFP_KERNEL);
> + if (!phy_driver)
> + return -ENOMEM;
> +
> + spin_lock_init(&phy_driver->lock);
> +
> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + if (!res) {
> + dev_err(&pdev->dev, "missing memory resource usbphy_regs\n");
> + return -EINVAL;
> + }
> + phy_driver->usbphy_regs = devm_ioremap_nocache(dev, res->start,
> + resource_size(res));
> + if (!phy_driver->usbphy_regs) {
> + dev_err(dev, "Failed to remap usbphy_regs\n");
> + return -ENOMEM;
> + }
> +
> + res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
> + if (!res) {
> + dev_err(&pdev->dev, "missing memory resource ubs2h_idm_regs\n");
> + return -EINVAL;
> + }
> + phy_driver->usb2h_idm_regs = devm_ioremap_nocache(dev, res->start,
> + resource_size(res));
> + if (!phy_driver->usb2h_idm_regs) {
> + dev_err(dev, "Failed to remap usb2h_idm_regs\n");
> + return -ENOMEM;
> + }
> +
> + res = platform_get_resource(pdev, IORESOURCE_MEM, 2);
> + if (!res) {
> + dev_err(&pdev->dev, "missing memory resource ubs2d_idm_regs\n");
> + return -EINVAL;
> + }
> + phy_driver->usb2d_idm_regs = devm_ioremap_nocache(dev, res->start,
> + resource_size(res));
> + if (!phy_driver->usb2d_idm_regs) {
> + dev_err(dev, "Failed to remap usb2d_idm_regs\n");
> + return -ENOMEM;
> + }
> +
> + dev_set_drvdata(dev, phy_driver);
> +
> + phy_driver->idm_host_enabled = false;
> +
> + /* Shut down all ports. They can be powered up as required */
> + reg_val = readl(phy_driver->usbphy_regs +
> + CRMU_USB_PHY_AON_CTRL_OFFSET);
> + reg_val &= ~(1 << CRMU_USBPHY_P0_AFE_CORERDY_VDDC);
> + reg_val &= ~(1 << CRMU_USBPHY_P0_RESETB);
> + reg_val &= ~(1 << CRMU_USBPHY_P1_AFE_CORERDY_VDDC);
> + reg_val &= ~(1 << CRMU_USBPHY_P1_RESETB);
> + reg_val &= ~(1 << CRMU_USBPHY_P2_AFE_CORERDY_VDDC);
> + reg_val &= ~(1 << CRMU_USBPHY_P2_RESETB);
> + writel(reg_val, phy_driver->usbphy_regs +
> + CRMU_USB_PHY_AON_CTRL_OFFSET);
> +
> + for_each_available_child_of_node(dev->of_node, child) {
> + struct bcm_phy_instance *port;
> + u32 port_no;
> +
> + if (of_property_read_u32(child, "reg", &port_no)) {
> + dev_err(dev, "missing reg property in node %s\n",
> + child->name);
> + return -EINVAL;
> + }
> +
> + if (port_no >= ARRAY_SIZE(phy_driver->ports)) {
> + dev_err(dev, "invalid reg in node %s: %u\n",
> + child->name, port_no);
> + return -EINVAL;
> + }
> +
> + port = devm_kzalloc(dev, sizeof(*port), GFP_KERNEL);
> + if (!port)
> + return -ENOMEM;
> +
> + port->generic_phy = devm_phy_create(dev, child, &ops);
> + if (IS_ERR(port->generic_phy)) {
> + error = PTR_ERR(port->generic_phy);
> + dev_err(dev, "Failed to create phy %u: %d",
> + port_no, error);
> + return error;
> + }
> +
> + port->port_no = port_no;
> + port->driver = phy_driver;
> + phy_set_drvdata(port->generic_phy, port);
> +
> + phy_driver->ports[port_no] = port;
> + }
> +
> + phy_provider = devm_of_phy_provider_register(dev, bcm_usb_phy_xlate);
> + if (IS_ERR(phy_provider)) {
> + error = PTR_ERR(phy_provider);
> + dev_err(dev, "Failed to register as phy provider: %d\n",
> + error);
> + return error;
> + }
> +
> + dev_set_drvdata(pdev, phy_driver);
dev_set_drvdata is being done multiple times.
Please also run checkpatch and fix all the errors and warnings before posting
the next version.
Thanks
Kishon
^ permalink raw reply
* Re: [PATCHv3 2/4] phy: core: Add devm_of_phy_get_by_index to phy-core
From: Kishon Vijay Abraham I @ 2015-05-11 15:21 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1429743853-10254-3-git-send-email-arun.ramamurthy@broadcom.com>
Hi,
On Thursday 23 April 2015 04:34 AM, Arun Ramamurthy wrote:
> Some generic drivers, such as ehci, may use multiple phys and for such
> drivers referencing phy(s) by name(s) does not make sense. Instead of
> inventing new naming schemes and using custom code to iterate through them,
> such drivers are better of using nameless phy bindings and using this newly
> introduced API to iterate through them.
>
> Signed-off-by: Arun Ramamurthy <arun.ramamurthy@broadcom.com>
> Reviewed-by: Ray Jui <rjui@broadcom.com>
> Reviewed-by: Scott Branden <sbranden@broadcom.com>
> ---
> Documentation/phy.txt | 7 ++++++-
> drivers/phy/phy-core.c | 32 ++++++++++++++++++++++++++++++++
> include/linux/phy/phy.h | 8 ++++++++
> 3 files changed, 46 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/phy.txt b/Documentation/phy.txt
> index 371361c..b388c5a 100644
> --- a/Documentation/phy.txt
> +++ b/Documentation/phy.txt
> @@ -76,6 +76,8 @@ struct phy *phy_get(struct device *dev, const char *string);
> struct phy *phy_optional_get(struct device *dev, const char *string);
> struct phy *devm_phy_get(struct device *dev, const char *string);
> struct phy *devm_phy_optional_get(struct device *dev, const char *string);
> +struct phy *devm_of_phy_get_by_index(struct device *dev, struct device_node *np,
> + int index);
>
> phy_get, phy_optional_get, devm_phy_get and devm_phy_optional_get can
> be used to get the PHY. In the case of dt boot, the string arguments
> @@ -86,7 +88,10 @@ successful PHY get. On driver detach, release function is invoked on
> the the devres data and devres data is freed. phy_optional_get and
> devm_phy_optional_get should be used when the phy is optional. These
> two functions will never return -ENODEV, but instead returns NULL when
> -the phy cannot be found.
> +the phy cannot be found.Some generic drivers, such as ehci, may use multiple
> +phys and for such drivers referencing phy(s) by name(s) does not make sense. In
> +this case, devm_of_phy_get_by_index can be used to get a phy reference based on
> +the index.
>
> It should be noted that NULL is a valid phy reference. All phy
> consumer calls on the NULL phy become NOPs. That is the release calls,
> diff --git a/drivers/phy/phy-core.c b/drivers/phy/phy-core.c
> index 3791838..964a84d 100644
> --- a/drivers/phy/phy-core.c
> +++ b/drivers/phy/phy-core.c
> @@ -623,6 +623,38 @@ struct phy *devm_of_phy_get(struct device *dev, struct device_node *np,
> EXPORT_SYMBOL_GPL(devm_of_phy_get);
>
> /**
> + * devm_of_phy_get_by_index() - lookup and obtain a reference to a phy by index.
> + * @dev: device that requests this phy
> + * @np: node containing the phy
> + * @index: index of the phy
> + *
> + * Gets the phy using _of_phy_get(), and associates a device with it using
> + * devres. On driver detach, release function is invoked on the devres data,
> + * then, devres data is freed.
> + *
> + */
> +struct phy *devm_of_phy_get_by_index(struct device *dev, struct device_node *np,
> + int index)
> +{
> + struct phy **ptr, *phy;
> +
> + ptr = devres_alloc(devm_phy_release, sizeof(*ptr), GFP_KERNEL);
> + if (!ptr)
> + return ERR_PTR(-ENOMEM);
> +
> + phy = _of_phy_get(np, index);
> + if (!IS_ERR(phy)) {
> + *ptr = phy;
> + devres_add(dev, ptr);
> + } else {
> + devres_free(ptr);
> + }
> +
> + return phy;
> +}
> +EXPORT_SYMBOL_GPL(devm_of_phy_get_by_index);
> +
> +/**
> * phy_create() - create a new phy
> * @dev: device that is creating the new phy
> * @node: device node of the phy
> diff --git a/include/linux/phy/phy.h b/include/linux/phy/phy.h
> index a0197fa..978d5af 100644
> --- a/include/linux/phy/phy.h
> +++ b/include/linux/phy/phy.h
> @@ -133,6 +133,8 @@ struct phy *devm_phy_get(struct device *dev, const char *string);
> struct phy *devm_phy_optional_get(struct device *dev, const char *string);
> struct phy *devm_of_phy_get(struct device *dev, struct device_node *np,
> const char *con_id);
> +struct phy *devm_of_phy_get_by_index(struct device *dev, struct device_node *np,
> + int index);
> void phy_put(struct phy *phy);
> void devm_phy_put(struct device *dev, struct phy *phy);
> struct phy *of_phy_get(struct device_node *np, const char *con_id);
> @@ -261,6 +263,12 @@ static inline struct phy *devm_of_phy_get(struct device *dev,
> return ERR_PTR(-ENOSYS);
> }
>
> +struct phy *devm_of_phy_get_by_index(struct device *dev, struct device_node *np,
> + int index);
Fixed the compilation error because of the ';' in the end and merged it to
linux-phy.
Thanks
Kishon
^ permalink raw reply
* [PATCH] backlight/lp855x: Don't clear level on suspend/blank
From: Sean Paul @ 2015-05-11 20:32 UTC (permalink / raw)
To: linux-fbdev
Don't clear the backlight level when we're going into suspend or
blanking. Instead, just temporarily set the level to 0 so we retain
the value when we resume.
Reported-by: Benson Leung <bleung@chromium.org>
Tested-by: Stephen Barber <smbarber@chromium.org>
Signed-off-by: Sean Paul <seanpaul@chromium.org>
---
drivers/video/backlight/lp855x_bl.c | 18 ++++++------------
1 file changed, 6 insertions(+), 12 deletions(-)
diff --git a/drivers/video/backlight/lp855x_bl.c b/drivers/video/backlight/lp855x_bl.c
index 08ae72f..74daf7c 100644
--- a/drivers/video/backlight/lp855x_bl.c
+++ b/drivers/video/backlight/lp855x_bl.c
@@ -257,21 +257,15 @@ static void lp855x_pwm_ctrl(struct lp855x *lp, int br, int max_br)
static int lp855x_bl_update_status(struct backlight_device *bl)
{
struct lp855x *lp = bl_get_data(bl);
+ int brightness = bl->props.brightness;
if (bl->props.state & (BL_CORE_SUSPENDED | BL_CORE_FBBLANK))
- bl->props.brightness = 0;
+ brightness = 0;
- if (lp->mode = PWM_BASED) {
- int br = bl->props.brightness;
- int max_br = bl->props.max_brightness;
-
- lp855x_pwm_ctrl(lp, br, max_br);
-
- } else if (lp->mode = REGISTER_BASED) {
- u8 val = bl->props.brightness;
-
- lp855x_write_byte(lp, lp->cfg->reg_brightness, val);
- }
+ if (lp->mode = PWM_BASED)
+ lp855x_pwm_ctrl(lp, brightness, bl->props.max_brightness);
+ else if (lp->mode = REGISTER_BASED)
+ lp855x_write_byte(lp, lp->cfg->reg_brightness, (u8)brightness);
return 0;
}
--
2.2.0.rc0.207.ga3a616c
^ permalink raw reply related
* Re: [PATCH] backlight/lp855x: Don't clear level on suspend/blank
From: Kim, Milo @ 2015-05-11 22:48 UTC (permalink / raw)
To: linux-fbdev
In-Reply-To: <1431376325-25741-1-git-send-email-seanpaul@chromium.org>
Hi Sean,
On 5/12/2015 5:32 AM, Sean Paul wrote:
> Don't clear the backlight level when we're going into suspend or
> blanking. Instead, just temporarily set the level to 0 so we retain
> the value when we resume.
Could you describe what the problem is? I wrote same code in other
backlight drivers. So maybe I need to look into them as well.
> ---
> drivers/video/backlight/lp855x_bl.c | 18 ++++++------------
> 1 file changed, 6 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/video/backlight/lp855x_bl.c b/drivers/video/backlight/lp855x_bl.c
> index 08ae72f..74daf7c 100644
> --- a/drivers/video/backlight/lp855x_bl.c
> +++ b/drivers/video/backlight/lp855x_bl.c
> @@ -257,21 +257,15 @@ static void lp855x_pwm_ctrl(struct lp855x *lp, int br, int max_br)
> static int lp855x_bl_update_status(struct backlight_device *bl)
> {
> struct lp855x *lp = bl_get_data(bl);
> + int brightness = bl->props.brightness;
>
> if (bl->props.state & (BL_CORE_SUSPENDED | BL_CORE_FBBLANK))
> - bl->props.brightness = 0;
> + brightness = 0;
After that, this driver can't show exact brightness value when the
application tries to read 'brightness' through the sysfs.
The backlight is off but read value of 'brightness' is non-zero.
https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/drivers/video/backlight/backlight.c?id=refs/tags/v4.1-rc3#n159
Best regards,
Milo
^ permalink raw reply
* Re: simple framebuffer slower by factor of 20, on socfpga (arm) platform
From: Pavel Machek @ 2015-05-12 8:52 UTC (permalink / raw)
To: Russell King - ARM Linux
Cc: Nicolas Pitre, Geert Uytterhoeven, Tomi Valkeinen, Archit Taneja,
Marek Vasut, kernel list, Dinh Nguyen,
Jean-Christophe PLAGNIOL-VILLARD, Grant Likely, Rob Herring,
Jingoo Han, Rob Clark, Linux Fbdev development list,
devicetree@vger.kernel.org, Alexander Shiyan, H Hartley Sweeten
In-Reply-To: <20150506104504.GM2067@n2100.arm.linux.org.uk>
On Wed 2015-05-06 11:45:04, Russell King - ARM Linux wrote:
> On Tue, Apr 28, 2015 at 11:28:53AM -0400, Nicolas Pitre wrote:
> > On Tue, 28 Apr 2015, Russell King - ARM Linux wrote:
> >
> > > On Fri, Apr 24, 2015 at 03:46:56PM +0200, Geert Uytterhoeven wrote:
> > > > So please optimize ARM's _memcpy_fromio(), _memcpy_toio(), and _memset_io().
> > > > That will benefit other drivers on ARM, too.
> > >
> > > That's not going to happen.
> > >
> > > I've had a patch which does that, but people are concerned that it changes
> > > the behaviour of the functions by changing the access size, which could
> > > cause regressions. It seems people are far too worried about that to even
> > > consider trying. :(
> >
> > What about making the optimized implementation available via kconfig?
>
> I'd prefer not to. My personal feeling is to put the patch in and just be
> done with it - these functions are supposed to be used on IO areas which
> don't care about access size (in other words, are memory-like rather than
> being register-like.) Here's the rather old patch:
>
> From: Russell King <rmk+kernel@arm.linux.org.uk>
> Subject: [PATCH] ARM: optimize memset_io()/memcpy_fromio()/memcpy_toio()
>
> If we are building for a LE platform, and we haven't overriden the
> MMIO ops, then we can optimize the mem*io operations using the
> standard string functions.
>
> Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
Tested-by: Pavel Machek <pavel@ucw.cz>
Acked-by: Pavel Machek <pavel@ucw.cz>
Works for me, framebuffer performance is back in "too fast to measure"
range.
When this is merged, should 981409b25e2a99409b26daa67293ca1cfd5ea0a0
be reverted in -stable?
Thanks,
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
^ permalink raw reply
* Re: [PATCH v2 08/17] gpio: locomo: implement per-pin irq handling
From: Linus Walleij @ 2015-05-12 11:15 UTC (permalink / raw)
To: Dmitry Eremin-Solenikov
Cc: Russell King, Daniel Mack, Robert Jarzmik, Alexandre Courbot,
Wolfram Sang, Dmitry Torokhov, Bryan Wu, Richard Purdie,
Samuel Ortiz, Lee Jones, Mark Brown, Jingoo Han,
Jean-Christophe Plagniol-Villard, Tomi Valkeinen, Liam Girdwood,
Andrea Adami, linux-arm-kernel@lists.infradead.org,
linux-gpio@vger.kernel.org, linux-i2c@vger.kernel.org,
Linux Input, linux-leds
In-Reply-To: <CALT56yNJutWTW5zsos+=Uh+BDvO4o_tqBEj+H621TncVK6eD_Q@mail.gmail.com>
On Wed, May 6, 2015 at 6:42 PM, Dmitry Eremin-Solenikov
<dbaryshkov@gmail.com> wrote:
> 2015-05-06 17:15 GMT+03:00 Linus Walleij <linus.walleij@linaro.org>:
>> On Tue, Apr 28, 2015 at 1:55 AM, Dmitry Eremin-Solenikov
>> <dbaryshkov@gmail.com> wrote:
>>
>>> LoCoMo has a possibility to generate per-GPIO edge irqs. Support for
>>> that was there in old locomo driver, got 'cleaned up' during old driver
>>> IRQ cascading cleanup and is now reimplemented. It is expected that
>>> SL-5500 (collie) will use locomo gpio irqs for mmc detection irq.
>>>
>>> Signed-off-by: Dmitry Eremin-Solenikov <dbaryshkov@gmail.com>
>>
>> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
>
> Thanks for review of the patches.
>
>> Assume this will also go through MFD.
>>
>>> + irq_set_chained_handler(lg->irq, NULL);
>>> + irq_set_handler_data(lg->irq, NULL);
>>
>> Why is this needed? If the GPIOLIB_IRQCHIP code in
>> gpiolib.c is not doing this then maybe it's the core code that
>> needs fixing rather than having this in the driver.
>
> I have skimmed through the rest of drivers using
> gpiochip_set_chained_irqchip(). Indeed none of the drivers NULL
> the chained handled and handler data. However I couldn't locate
> the code where they would be cleared.
>
> Should I still send the patch fixing the GPIOLIB_IRQCHIP?
Yes I guess you need to save parent_irq in struct gpio_chip and
remove it in gpiochip_irqchip_remove() if it's != 0.
Yours,
Linus Walleij
^ permalink raw reply
* Re: [PATCH v2 02/17] leds: port locomo leds driver to new locomo core
From: Dmitry Eremin-Solenikov @ 2015-05-12 15:35 UTC (permalink / raw)
To: Jacek Anaszewski
Cc: Russell King, Daniel Mack, Robert Jarzmik, Linus Walleij,
Alexandre Courbot, Wolfram Sang, Dmitry Torokhov, Bryan Wu,
Richard Purdie, Samuel Ortiz, Lee Jones, Mark Brown, Jingoo Han,
Jean-Christophe Plagniol-Villard, Tomi Valkeinen, Liam Girdwood,
Andrea Adami, linux-arm-kernel, linux-gpio@vger.kernel.org,
linux-i2c, linux-input, linux-leds
In-Reply-To: <554A2DB4.3020000@samsung.com>
2015-05-06 18:05 GMT+03:00 Jacek Anaszewski <j.anaszewski@samsung.com>:
> On 04/28/2015 01:55 AM, Dmitry Eremin-Solenikov wrote:
>>
>> Adapt locomo leds driver to new locomo core setup.
>>
>> Signed-off-by: Dmitry Eremin-Solenikov <dbaryshkov@gmail.com>
>> ---
>> drivers/leds/Kconfig | 1 -
>> drivers/leds/leds-locomo.c | 119
>> +++++++++++++++++++++++----------------------
>> 2 files changed, 61 insertions(+), 59 deletions(-)
>>
>> diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
>> index 966b960..4b4650b 100644
>> --- a/drivers/leds/Kconfig
>> +++ b/drivers/leds/Kconfig
>> @@ -79,7 +79,6 @@ config LEDS_LM3642
>> config LEDS_LOCOMO
>> tristate "LED Support for Locomo device"
>> depends on LEDS_CLASS
>> - depends on SHARP_LOCOMO
>
>
> Why do you remove this dependency?
Because SHARP_LOCOMO is a Kconfig symbol for the old driver. New driver
uses MFD_LOCOMO Kconfig entry. Also the driver now uses generic platform
device and regmap interfaces, so there is no direct dependency on main
LoCoMo driver. And the policy (IIRC) was not to have such dependencies.
>
>> help
>> This option enables support for the LEDs on Sharp Locomo.
>> Zaurus models SL-5500 and SL-5600.
>> diff --git a/drivers/leds/leds-locomo.c b/drivers/leds/leds-locomo.c
>> index 80ba048..7fde812 100644
>> --- a/drivers/leds/leds-locomo.c
>> +++ b/drivers/leds/leds-locomo.c
>> @@ -9,89 +9,92 @@
>> */
>>
>> #include <linux/kernel.h>
>> -#include <linux/init.h>
>> -#include <linux/module.h>
>> -#include <linux/device.h>
>> #include <linux/leds.h>
>> -
>> -#include <mach/hardware.h>
>> -#include <asm/hardware/locomo.h>
>> +#include <linux/module.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/regmap.h>
>> +#include <linux/mfd/locomo.h>
>
>
> Please keep alphabetical order.
Ack
>
>> +
>> +struct locomo_led {
>> + struct led_classdev led;
>> + struct regmap *regmap;
>> + unsigned int reg;
>> +};
>>
>> static void locomoled_brightness_set(struct led_classdev *led_cdev,
>> - enum led_brightness value, int offset)
>> + enum led_brightness value)
>> {
>> - struct locomo_dev *locomo_dev = LOCOMO_DEV(led_cdev->dev->parent);
>> - unsigned long flags;
>> + struct locomo_led *led = container_of(led_cdev, struct locomo_led,
>> led);
>>
>> - local_irq_save(flags);
>> - if (value)
>> - locomo_writel(LOCOMO_LPT_TOFH, locomo_dev->mapbase +
>> offset);
>> - else
>> - locomo_writel(LOCOMO_LPT_TOFL, locomo_dev->mapbase +
>> offset);
>> - local_irq_restore(flags);
>> + regmap_write(led->regmap, led->reg,
>> + value ? LOCOMO_LPT_TOFH : LOCOMO_LPT_TOFL);
>> }
>
>
> Please use work queue for setting brightness. This is required for the
> driver to be compatible with led triggers. You can refer to the
> existing LED drivers on how to implement this.
Hmm. Why? The regmap here uses MMIO access, so it is atomic operation
and doesn't need to be wrapped into work queue, does it?
[skipped]
--
With best wishes
Dmitry
^ permalink raw reply
* Re: [PATCH v2 10/17] i2c: add locomo i2c driver
From: Wolfram Sang @ 2015-05-12 19:24 UTC (permalink / raw)
To: Dmitry Eremin-Solenikov
Cc: Russell King, Daniel Mack, Robert Jarzmik, Linus Walleij,
Alexandre Courbot, Dmitry Torokhov, Bryan Wu, Richard Purdie,
Samuel Ortiz, Lee Jones, Mark Brown, Jingoo Han,
Jean-Christophe Plagniol-Villard, Tomi Valkeinen, Liam Girdwood,
Andrea Adami, linux-arm-kernel, linux-gpio, linux-i2c,
linux-input, linux-leds, linux-spi, linux-fbdev, alsa-devel
In-Reply-To: <1430178954-11138-11-git-send-email-dbaryshkov@gmail.com>
[-- Attachment #1: Type: text/plain, Size: 830 bytes --]
On Tue, Apr 28, 2015 at 02:55:47AM +0300, Dmitry Eremin-Solenikov wrote:
> LoCoMo chip contains a tiny i2c controller destined to control
> M62332 DAC. Provide a separate I2C driver for this cell.
>
> Signed-off-by: Dmitry Eremin-Solenikov <dbaryshkov@gmail.com>
Thanks for the submission!
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/regmap.h>
> +#include <linux/slab.h>
> +#include <linux/mfd/locomo.h>
> +
> +#include <linux/i2c.h>
> +#include <linux/i2c-algo-bit.h>
To avoid duplicates, I prefer the includes to be sorted. No need to
resend because of that, though.
Kudos for brushing up old code! Do you want the drivers go via the
subsystem trees? Or is there a dependency so it should go in via some
other tree in one go?
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply
* Re: [PATCH v2 10/17] i2c: add locomo i2c driver
From: Dmitry Eremin-Solenikov @ 2015-05-12 19:27 UTC (permalink / raw)
To: Wolfram Sang
Cc: Russell King, Daniel Mack, Robert Jarzmik, Linus Walleij,
Alexandre Courbot, Dmitry Torokhov, Bryan Wu, Richard Purdie,
Samuel Ortiz, Lee Jones, Mark Brown, Jingoo Han,
Jean-Christophe Plagniol-Villard, Tomi Valkeinen, Liam Girdwood,
Andrea Adami, linux-arm-kernel, linux-gpio@vger.kernel.org,
linux-i2c, linux-input, linux-leds, linux-spi
In-Reply-To: <20150512192413.GE4449@schokonusskuchen.bad>
2015-05-12 22:24 GMT+03:00 Wolfram Sang <wsa@the-dreams.de>:
> On Tue, Apr 28, 2015 at 02:55:47AM +0300, Dmitry Eremin-Solenikov wrote:
>> LoCoMo chip contains a tiny i2c controller destined to control
>> M62332 DAC. Provide a separate I2C driver for this cell.
>>
>> Signed-off-by: Dmitry Eremin-Solenikov <dbaryshkov@gmail.com>
>
> Thanks for the submission!
>
>> +#include <linux/kernel.h>
>> +#include <linux/module.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/regmap.h>
>> +#include <linux/slab.h>
>> +#include <linux/mfd/locomo.h>
>> +
>> +#include <linux/i2c.h>
>> +#include <linux/i2c-algo-bit.h>
>
> To avoid duplicates, I prefer the includes to be sorted. No need to
> resend because of that, though.
>
> Kudos for brushing up old code! Do you want the drivers go via the
> subsystem trees? Or is there a dependency so it should go in via some
> other tree in one go?
There is a header dependency -- <linux/mfd/locomo.h> is a new header.
I'd prefer for these patches to go in via one of MFD, arm-soc or ARM trees
as a whole series (if that would be ok with respective maintainers).
--
With best wishes
Dmitry
^ permalink raw reply
* Re: [PATCH v2 10/17] i2c: add locomo i2c driver
From: Wolfram Sang @ 2015-05-12 19:28 UTC (permalink / raw)
To: Dmitry Eremin-Solenikov
Cc: Russell King, Daniel Mack, Robert Jarzmik, Linus Walleij,
Alexandre Courbot, Dmitry Torokhov, Bryan Wu, Richard Purdie,
Samuel Ortiz, Lee Jones, Mark Brown, Jingoo Han,
Jean-Christophe Plagniol-Villard, Tomi Valkeinen, Liam Girdwood,
Andrea Adami, linux-arm-kernel, linux-gpio@vger.kernel.org,
linux-i2c, linux-input, linux-leds, linux-spi
In-Reply-To: <CALT56yNHvJ1Swj2pp2JrBaJBensSP703JXQKfvivFrJOYf85QA@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 1339 bytes --]
On Tue, May 12, 2015 at 10:27:07PM +0300, Dmitry Eremin-Solenikov wrote:
> 2015-05-12 22:24 GMT+03:00 Wolfram Sang <wsa@the-dreams.de>:
> > On Tue, Apr 28, 2015 at 02:55:47AM +0300, Dmitry Eremin-Solenikov wrote:
> >> LoCoMo chip contains a tiny i2c controller destined to control
> >> M62332 DAC. Provide a separate I2C driver for this cell.
> >>
> >> Signed-off-by: Dmitry Eremin-Solenikov <dbaryshkov@gmail.com>
> >
> > Thanks for the submission!
> >
> >> +#include <linux/kernel.h>
> >> +#include <linux/module.h>
> >> +#include <linux/platform_device.h>
> >> +#include <linux/regmap.h>
> >> +#include <linux/slab.h>
> >> +#include <linux/mfd/locomo.h>
> >> +
> >> +#include <linux/i2c.h>
> >> +#include <linux/i2c-algo-bit.h>
> >
> > To avoid duplicates, I prefer the includes to be sorted. No need to
> > resend because of that, though.
> >
> > Kudos for brushing up old code! Do you want the drivers go via the
> > subsystem trees? Or is there a dependency so it should go in via some
> > other tree in one go?
>
> There is a header dependency -- <linux/mfd/locomo.h> is a new header.
> I'd prefer for these patches to go in via one of MFD, arm-soc or ARM trees
> as a whole series (if that would be ok with respective maintainers).
Fine with me.
Reviewed-by: Wolfram Sang <wsa@the-dreams.de>
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply
* Re: [PATCH v2 04/17] input: locomokbd: provide an Alt-SysRQ combination
From: Dmitry Torokhov @ 2015-05-12 20:12 UTC (permalink / raw)
To: Dmitry Eremin-Solenikov
Cc: Russell King, Daniel Mack, Robert Jarzmik, Linus Walleij,
Alexandre Courbot, Wolfram Sang, Bryan Wu, Richard Purdie,
Samuel Ortiz, Lee Jones, Mark Brown, Jingoo Han,
Jean-Christophe Plagniol-Villard, Tomi Valkeinen, Liam Girdwood,
Andrea Adami, linux-arm-kernel, linux-gpio, linux-i2c,
linux-input, linux-leds, linux-spi, linux-fbdev, alsa-devel
In-Reply-To: <1430178954-11138-5-git-send-email-dbaryshkov@gmail.com>
Hi Dmitry,
On Tue, Apr 28, 2015 at 02:55:41AM +0300, Dmitry Eremin-Solenikov wrote:
> It is usefull sometimes to have an Alt-SysRQ combo on the keyboard to be
> able to trigger sysrq functions directly. Add an option providing sysrq
> mapping for Contact-Home keys.
No, if you want to remap the keys please do it from usepspace; locomokbd
supports it (EVIOCSKEYCODE ioctl).
Thanks.
>
> Signed-off-by: Dmitry Eremin-Solenikov <dbaryshkov@gmail.com>
> ---
> drivers/input/keyboard/Kconfig | 7 +++++++
> drivers/input/keyboard/locomokbd.c | 14 +++++++++++---
> 2 files changed, 18 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/input/keyboard/Kconfig b/drivers/input/keyboard/Kconfig
> index 0a3d875..eb8b11b 100644
> --- a/drivers/input/keyboard/Kconfig
> +++ b/drivers/input/keyboard/Kconfig
> @@ -343,6 +343,13 @@ config KEYBOARD_LOCOMO
> To compile this driver as a module, choose M here: the
> module will be called locomokbd.
>
> +config KEYBOARD_LOCOMO_SYSRQ
> + bool "Provide SysRQ key on LoCoMo keyboard"
> + depends on KEYBOARD_LOCOMO
> + help
> + Say Y here to be able to use Contact-Home as Alt-Sysrq combo.
> + Say N if you want to use them as usual keys.
> +
> config KEYBOARD_LPC32XX
> tristate "LPC32XX matrix key scanner support"
> depends on ARCH_LPC32XX && OF
> diff --git a/drivers/input/keyboard/locomokbd.c b/drivers/input/keyboard/locomokbd.c
> index eed0a94..eb8dbe2 100644
> --- a/drivers/input/keyboard/locomokbd.c
> +++ b/drivers/input/keyboard/locomokbd.c
> @@ -40,6 +40,14 @@
> #define KEY_CONTACT KEY_F18
> #define KEY_CENTER KEY_F15
>
> +#ifdef CONFIG_KEYBOARD_LOCOMOKBD_SYSRQ
> +#define LOCOMO_KEY_CONTACT KEY_LEFTALT
> +#define LOCOMO_KEY_HOME KEY_SYSRQ
> +#else
> +#define LOCOMO_KEY_CONTACT KEY_CONTACT
> +#define LOCOMO_KEY_HOME KEY_HOME
> +#endif
> +
> #define KB_ROWS 16
> #define KB_COLS 8
> #define LOCOMOKBD_NUMKEYS (KB_ROWS * KB_COLS)
> @@ -48,7 +56,7 @@
> static const unsigned char
> locomokbd_keycode[LOCOMOKBD_NUMKEYS] = {
> 0, KEY_ESC, KEY_ACTIVITY, 0, 0, 0, 0, 0, 0, 0, /* 0 - 9 */
> - 0, 0, 0, 0, 0, 0, 0, KEY_MENU, 0, KEY_CONTACT, /* 10 - 19 */
> + 0, 0, 0, 0, 0, 0, 0, KEY_MENU, 0, LOCOMO_KEY_CONTACT, /* 10 - 19 */
> 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, /* 20 - 29 */
> 0, 0, 0, KEY_CENTER, 0, KEY_MAIL, 0, 0, 0, 0, /* 30 - 39 */
> 0, 0, 0, 0, 0, 0, 0, 0, 0, KEY_RIGHT, /* 40 - 49 */
> @@ -280,9 +288,9 @@ static int locomokbd_probe(struct platform_device *dev)
> sizeof(locomokbd->keycode));
>
> if (machine_is_collie())
> - locomokbd->keycode[18] = KEY_HOME;
> + locomokbd->keycode[18] = LOCOMO_KEY_HOME;
> else
> - locomokbd->keycode[3] = KEY_HOME;
> + locomokbd->keycode[3] = LOCOMO_KEY_HOME;
>
> for (i = 0; i < LOCOMOKBD_NUMKEYS; i++)
> input_set_capability(input_dev, EV_KEY, locomokbd->keycode[i]);
> --
> 2.1.4
>
--
Dmitry
^ permalink raw reply
* Re: [PATCH v2 03/17] input: convert LoCoMo keyboard driver to use new locomo core
From: Dmitry Torokhov @ 2015-05-12 20:21 UTC (permalink / raw)
To: Dmitry Eremin-Solenikov
Cc: Russell King, Daniel Mack, Robert Jarzmik, Linus Walleij,
Alexandre Courbot, Wolfram Sang, Bryan Wu, Richard Purdie,
Samuel Ortiz, Lee Jones, Mark Brown, Jingoo Han,
Jean-Christophe Plagniol-Villard, Tomi Valkeinen, Liam Girdwood,
Andrea Adami, linux-arm-kernel, linux-gpio, linux-i2c,
linux-input, linux-leds, linux-spi, linux-fbdev, alsa-devel
In-Reply-To: <1430178954-11138-4-git-send-email-dbaryshkov@gmail.com>
Hi Dmitry,
On Tue, Apr 28, 2015 at 02:55:40AM +0300, Dmitry Eremin-Solenikov wrote:
> As LoCoMo is switching to new device model, adapt keyboard driver to
> support new locomo core driver.
>
> Signed-off-by: Dmitry Eremin-Solenikov <dbaryshkov@gmail.com>
> ---
> drivers/input/keyboard/Kconfig | 1 -
> drivers/input/keyboard/locomokbd.c | 271 +++++++++++++++++++------------------
> 2 files changed, 143 insertions(+), 129 deletions(-)
>
> diff --git a/drivers/input/keyboard/Kconfig b/drivers/input/keyboard/Kconfig
> index 106fbac..0a3d875 100644
> --- a/drivers/input/keyboard/Kconfig
> +++ b/drivers/input/keyboard/Kconfig
> @@ -337,7 +337,6 @@ config KEYBOARD_LM8333
>
> config KEYBOARD_LOCOMO
> tristate "LoCoMo Keyboard Support"
> - depends on SHARP_LOCOMO
> help
> Say Y here if you are running Linux on a Sharp Zaurus Collie or Poodle based PDA
>
> diff --git a/drivers/input/keyboard/locomokbd.c b/drivers/input/keyboard/locomokbd.c
> index c94d610..eed0a94 100644
> --- a/drivers/input/keyboard/locomokbd.c
> +++ b/drivers/input/keyboard/locomokbd.c
> @@ -23,37 +23,37 @@
> *
> */
>
> -#include <linux/slab.h>
> -#include <linux/module.h>
> +#include <linux/delay.h>
> #include <linux/init.h>
> #include <linux/input.h>
> -#include <linux/delay.h>
> -#include <linux/device.h>
> #include <linux/interrupt.h>
> -#include <linux/ioport.h>
> -
> -#include <asm/hardware/locomo.h>
> -#include <asm/irq.h>
> -
> -MODULE_AUTHOR("John Lenz <lenz@cs.wisc.edu>");
> -MODULE_DESCRIPTION("LoCoMo keyboard driver");
> -MODULE_LICENSE("GPL");
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/regmap.h>
> +#include <linux/slab.h>
> +#include <linux/mfd/locomo.h>
>
> -#define LOCOMOKBD_NUMKEYS 128
> +/* There is one minor difference between mappings on poodle and collie */
> +#include <asm/mach-types.h>
>
> #define KEY_ACTIVITY KEY_F16
> #define KEY_CONTACT KEY_F18
> #define KEY_CENTER KEY_F15
>
> +#define KB_ROWS 16
> +#define KB_COLS 8
> +#define LOCOMOKBD_NUMKEYS (KB_ROWS * KB_COLS)
> +#define SCANCODE(c, r) (((c)<<4) + (r) + 1)
> +
> static const unsigned char
> locomokbd_keycode[LOCOMOKBD_NUMKEYS] = {
> 0, KEY_ESC, KEY_ACTIVITY, 0, 0, 0, 0, 0, 0, 0, /* 0 - 9 */
> - 0, 0, 0, 0, 0, 0, 0, KEY_MENU, KEY_HOME, KEY_CONTACT, /* 10 - 19 */
> + 0, 0, 0, 0, 0, 0, 0, KEY_MENU, 0, KEY_CONTACT, /* 10 - 19 */
> 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, /* 20 - 29 */
> 0, 0, 0, KEY_CENTER, 0, KEY_MAIL, 0, 0, 0, 0, /* 30 - 39 */
> 0, 0, 0, 0, 0, 0, 0, 0, 0, KEY_RIGHT, /* 40 - 49 */
> KEY_UP, KEY_LEFT, 0, 0, KEY_P, 0, KEY_O, KEY_I, KEY_Y, KEY_T, /* 50 - 59 */
> - KEY_E, KEY_W, 0, 0, 0, 0, KEY_DOWN, KEY_ENTER, 0, 0, /* 60 - 69 */
> + KEY_E, KEY_W, 0, 0, 0, 0, KEY_DOWN, KEY_KPENTER, 0, 0, /* 60 - 69 */
> KEY_BACKSPACE, 0, KEY_L, KEY_U, KEY_H, KEY_R, KEY_D, KEY_Q, 0, 0, /* 70 - 79 */
> 0, 0, 0, 0, 0, 0, KEY_ENTER, KEY_RIGHTSHIFT, KEY_K, KEY_J, /* 80 - 89 */
> KEY_G, KEY_F, KEY_X, KEY_S, 0, 0, 0, 0, 0, 0, /* 90 - 99 */
> @@ -62,20 +62,14 @@ locomokbd_keycode[LOCOMOKBD_NUMKEYS] = {
> KEY_M, KEY_SPACE, KEY_V, KEY_APOSTROPHE, KEY_SLASH, 0, 0, 0 /* 120 - 128 */
> };
>
> -#define KB_ROWS 16
> -#define KB_COLS 8
> -#define KB_ROWMASK(r) (1 << (r))
> -#define SCANCODE(c,r) ( ((c)<<4) + (r) + 1 )
> -
> #define KB_DELAY 8
> -#define SCAN_INTERVAL (HZ/10)
>
> struct locomokbd {
> unsigned char keycode[LOCOMOKBD_NUMKEYS];
> struct input_dev *input;
> - char phys[32];
>
> - unsigned long base;
> + struct regmap *regmap;
> + int irq;
> spinlock_t lock;
>
> struct timer_list timer;
> @@ -84,37 +78,33 @@ struct locomokbd {
> };
>
> /* helper functions for reading the keyboard matrix */
> -static inline void locomokbd_charge_all(unsigned long membase)
> +static inline void locomokbd_charge_all(struct locomokbd *locomokbd)
> {
> - locomo_writel(0x00FF, membase + LOCOMO_KSC);
> + regmap_write(locomokbd->regmap, LOCOMO_KSC, 0x00ff);
> }
>
> -static inline void locomokbd_activate_all(unsigned long membase)
> +static inline void locomokbd_activate_all(struct locomokbd *locomokbd)
Drop "inline"s from the .c file please.
> {
> - unsigned long r;
> -
> - locomo_writel(0, membase + LOCOMO_KSC);
> - r = locomo_readl(membase + LOCOMO_KIC);
> - r &= 0xFEFF;
> - locomo_writel(r, membase + LOCOMO_KIC);
> + regmap_write(locomokbd->regmap, LOCOMO_KSC, 0);
> + regmap_update_bits(locomokbd->regmap, LOCOMO_KIC, 0x100, 0);
> }
>
> -static inline void locomokbd_activate_col(unsigned long membase, int col)
> +static inline void locomokbd_activate_col(struct locomokbd *locomokbd, int col)
> {
> unsigned short nset;
> unsigned short nbset;
>
> - nset = 0xFF & ~(1 << col);
> + nset = 0xFF & ~BIT(col);
> nbset = (nset << 8) + nset;
> - locomo_writel(nbset, membase + LOCOMO_KSC);
> + regmap_write(locomokbd->regmap, LOCOMO_KSC, nbset);
> }
>
> -static inline void locomokbd_reset_col(unsigned long membase, int col)
> +static inline void locomokbd_reset_col(struct locomokbd *locomokbd, int col)
> {
> unsigned short nbset;
>
> - nbset = ((0xFF & ~(1 << col)) << 8) + 0xFF;
> - locomo_writel(nbset, membase + LOCOMO_KSC);
> + nbset = ((0xFF & ~BIT(col)) << 8) + 0xFF;
> + regmap_write(locomokbd->regmap, LOCOMO_KSC, nbset);
> }
>
> /*
> @@ -129,24 +119,25 @@ static void locomokbd_scankeyboard(struct locomokbd *locomokbd)
> unsigned int row, col, rowd;
> unsigned long flags;
> unsigned int num_pressed;
> - unsigned long membase = locomokbd->base;
> + bool esc_pressed = false;
>
> spin_lock_irqsave(&locomokbd->lock, flags);
>
> - locomokbd_charge_all(membase);
> + locomokbd_charge_all(locomokbd);
>
> num_pressed = 0;
> for (col = 0; col < KB_COLS; col++) {
> -
> - locomokbd_activate_col(membase, col);
> + udelay(KB_DELAY);
> + locomokbd_activate_col(locomokbd, col);
> udelay(KB_DELAY);
>
> - rowd = ~locomo_readl(membase + LOCOMO_KIB);
> + regmap_read(locomokbd->regmap, LOCOMO_KIB, &rowd);
> + rowd = ~rowd;
> for (row = 0; row < KB_ROWS; row++) {
> unsigned int scancode, pressed, key;
>
> scancode = SCANCODE(col, row);
> - pressed = rowd & KB_ROWMASK(row);
> + pressed = rowd & BIT(row);
> key = locomokbd->keycode[scancode];
>
> input_report_key(locomokbd->input, key, pressed);
> @@ -158,29 +149,30 @@ static void locomokbd_scankeyboard(struct locomokbd *locomokbd)
> /* The "Cancel/ESC" key is labeled "On/Off" on
> * Collie and Poodle and should suspend the device
> * if it was pressed for more than a second. */
> - if (unlikely(key = KEY_ESC)) {
> - if (!time_after(jiffies,
> - locomokbd->suspend_jiffies + HZ))
> - continue;
> - if (locomokbd->count_cancel++
> - != (HZ/SCAN_INTERVAL + 1))
> - continue;
> - input_event(locomokbd->input, EV_PWR,
> - KEY_SUSPEND, 1);
> - locomokbd->suspend_jiffies = jiffies;
> - } else
> - locomokbd->count_cancel = 0;
> + if (unlikely(key = KEY_ESC))
> + esc_pressed = true;
> }
> - locomokbd_reset_col(membase, col);
> + locomokbd_reset_col(locomokbd, col);
> }
> - locomokbd_activate_all(membase);
> + locomokbd_activate_all(locomokbd);
>
> input_sync(locomokbd->input);
>
> /* if any keys are pressed, enable the timer */
> if (num_pressed)
> - mod_timer(&locomokbd->timer, jiffies + SCAN_INTERVAL);
> + mod_timer(&locomokbd->timer, jiffies + msecs_to_jiffies(100));
> else
> + regmap_update_bits(locomokbd->regmap, LOCOMO_KIC, 0x10, 0x10);
> +
> +
> + if (esc_pressed && time_after(jiffies,
> + locomokbd->suspend_jiffies + msecs_to_jiffies(1000))) {
> + if (locomokbd->count_cancel++ > (20)) {
Why parentheses around 20?
> + input_event(locomokbd->input, EV_PWR,
> + KEY_SUSPEND, 1);
> + locomokbd->suspend_jiffies = jiffies;
> + }
> + } else
> locomokbd->count_cancel = 0;
If one branch has curly braces the other should have them too.
>
> spin_unlock_irqrestore(&locomokbd->lock, flags);
> @@ -192,18 +184,18 @@ static void locomokbd_scankeyboard(struct locomokbd *locomokbd)
> static irqreturn_t locomokbd_interrupt(int irq, void *dev_id)
> {
> struct locomokbd *locomokbd = dev_id;
> - u16 r;
> + unsigned int r;
>
> - r = locomo_readl(locomokbd->base + LOCOMO_KIC);
> +
> + regmap_read(locomokbd->regmap, LOCOMO_KIC, &r);
> if ((r & 0x0001) = 0)
> return IRQ_HANDLED;
>
> - locomo_writel(r & ~0x0100, locomokbd->base + LOCOMO_KIC); /* Ack */
> + /* Mask and Ack */
> + regmap_write(locomokbd->regmap, LOCOMO_KIC, r & ~0x110);
>
> - /** wait chattering delay **/
> - udelay(100);
> + mod_timer(&locomokbd->timer, jiffies + msecs_to_jiffies(1));
>
> - locomokbd_scankeyboard(locomokbd);
> return IRQ_HANDLED;
> }
>
> @@ -220,47 +212,37 @@ static void locomokbd_timer_callback(unsigned long data)
> static int locomokbd_open(struct input_dev *dev)
> {
> struct locomokbd *locomokbd = input_get_drvdata(dev);
> - u16 r;
> -
> - r = locomo_readl(locomokbd->base + LOCOMO_KIC) | 0x0010;
> - locomo_writel(r, locomokbd->base + LOCOMO_KIC);
> - return 0;
> +
> + return regmap_update_bits(locomokbd->regmap, LOCOMO_KIC, 0x10, 0x10);
> }
>
> static void locomokbd_close(struct input_dev *dev)
> {
> struct locomokbd *locomokbd = input_get_drvdata(dev);
> - u16 r;
> -
> - r = locomo_readl(locomokbd->base + LOCOMO_KIC) & ~0x0010;
> - locomo_writel(r, locomokbd->base + LOCOMO_KIC);
> +
> + regmap_update_bits(locomokbd->regmap, LOCOMO_KIC, 0x10, 0x0);
> }
>
> -static int locomokbd_probe(struct locomo_dev *dev)
> +static int locomokbd_probe(struct platform_device *dev)
> {
> struct locomokbd *locomokbd;
> struct input_dev *input_dev;
> int i, err;
>
> - locomokbd = kzalloc(sizeof(struct locomokbd), GFP_KERNEL);
> - input_dev = input_allocate_device();
> - if (!locomokbd || !input_dev) {
> - err = -ENOMEM;
> - goto err_free_mem;
> - }
> + locomokbd = devm_kzalloc(&dev->dev, sizeof(struct locomokbd),
> + GFP_KERNEL);
> + if (!locomokbd)
> + return -ENOMEM;
>
> - /* try and claim memory region */
> - if (!request_mem_region((unsigned long) dev->mapbase,
> - dev->length,
> - LOCOMO_DRIVER_NAME(dev))) {
> - err = -EBUSY;
> - printk(KERN_ERR "locomokbd: Can't acquire access to io memory for keyboard\n");
> - goto err_free_mem;
> - }
> + locomokbd->regmap = dev_get_regmap(dev->dev.parent, NULL);
> + if (!locomokbd->regmap)
> + return -EINVAL;
>
> - locomo_set_drvdata(dev, locomokbd);
> + locomokbd->irq = platform_get_irq(dev, 0);
> + if (locomokbd->irq < 0)
> + return -ENXIO;
>
> - locomokbd->base = (unsigned long) dev->mapbase;
> + platform_set_drvdata(dev, locomokbd);
>
> spin_lock_init(&locomokbd->lock);
>
> @@ -270,11 +252,13 @@ static int locomokbd_probe(struct locomo_dev *dev)
>
> locomokbd->suspend_jiffies = jiffies;
>
> - locomokbd->input = input_dev;
> - strcpy(locomokbd->phys, "locomokbd/input0");
> + input_dev = input_allocate_device();
devm_input_allocate_device()?
> + if (!input_dev)
> + return -ENOMEM;
>
> + locomokbd->input = input_dev;
> input_dev->name = "LoCoMo keyboard";
> - input_dev->phys = locomokbd->phys;
> + input_dev->phys = "locomokbd/input0";
> input_dev->id.bustype = BUS_HOST;
> input_dev->id.vendor = 0x0001;
> input_dev->id.product = 0x0001;
> @@ -291,16 +275,30 @@ static int locomokbd_probe(struct locomo_dev *dev)
>
> input_set_drvdata(input_dev, locomokbd);
>
> - memcpy(locomokbd->keycode, locomokbd_keycode, sizeof(locomokbd->keycode));
> + memcpy(locomokbd->keycode,
> + locomokbd_keycode,
> + sizeof(locomokbd->keycode));
> +
> + if (machine_is_collie())
> + locomokbd->keycode[18] = KEY_HOME;
> + else
> + locomokbd->keycode[3] = KEY_HOME;
This seems like a new addition. Ideally keymap twiddling shoudl be done
from userspace.
> +
> for (i = 0; i < LOCOMOKBD_NUMKEYS; i++)
> - set_bit(locomokbd->keycode[i], input_dev->keybit);
> - clear_bit(0, input_dev->keybit);
> + input_set_capability(input_dev, EV_KEY, locomokbd->keycode[i]);
> + input_set_capability(input_dev, EV_PWR, KEY_SUSPEND);
> + __set_bit(EV_REP, input_dev->evbit);
> +
> + regmap_write(locomokbd->regmap, LOCOMO_KCMD, 1);
> + regmap_write(locomokbd->regmap, LOCOMO_KSC, 0x0);
> + regmap_write(locomokbd->regmap, LOCOMO_KIC, 0x0);
>
> /* attempt to get the interrupt */
> - err = request_irq(dev->irq[0], locomokbd_interrupt, 0, "locomokbd", locomokbd);
> + err = request_irq(locomokbd->irq, locomokbd_interrupt, 0,
> + "locomokbd", locomokbd);
devm_request_irq()?
> if (err) {
> - printk(KERN_ERR "locomokbd: Can't get irq for keyboard\n");
> - goto err_release_region;
> + dev_err(&dev->dev, "locomokbd: Can't get irq for keyboard\n");
> + goto err_free_mem;
> }
>
> err = input_register_device(locomokbd->input);
> @@ -309,54 +307,71 @@ static int locomokbd_probe(struct locomo_dev *dev)
>
> return 0;
>
> - err_free_irq:
> - free_irq(dev->irq[0], locomokbd);
> - err_release_region:
> - release_mem_region((unsigned long) dev->mapbase, dev->length);
> - locomo_set_drvdata(dev, NULL);
> - err_free_mem:
> +err_free_irq:
> + free_irq(locomokbd->irq, locomokbd);
> +err_free_mem:
> input_free_device(input_dev);
> - kfree(locomokbd);
>
> return err;
> }
>
> -static int locomokbd_remove(struct locomo_dev *dev)
> +static int locomokbd_remove(struct platform_device *dev)
> {
> - struct locomokbd *locomokbd = locomo_get_drvdata(dev);
> + struct locomokbd *locomokbd = platform_get_drvdata(dev);
>
> - free_irq(dev->irq[0], locomokbd);
> + free_irq(locomokbd->irq, locomokbd);
Is not needed with devm.
>
> del_timer_sync(&locomokbd->timer);
Should likely to go into close().
>
> input_unregister_device(locomokbd->input);
Is not needed with devm.
> - locomo_set_drvdata(dev, NULL);
>
> - release_mem_region((unsigned long) dev->mapbase, dev->length);
> + return 0;
> +}
> +
> +#ifdef CONFIG_PM_SLEEP
> +static int locomokbd_suspend(struct device *dev)
Mark as __maybe_unused instead of giarding with CONFIG_PM_SLEEP.
> +{
> + struct locomokbd *locomokbd = dev_get_drvdata(dev);
> +
> + regmap_update_bits(locomokbd->regmap, LOCOMO_KIC, 0x10, 0x0);
>
> - kfree(locomokbd);
> + del_timer_sync(&locomokbd->timer);
>
> return 0;
> }
>
> -static struct locomo_driver keyboard_driver = {
> - .drv = {
> - .name = "locomokbd"
> +static int locomokbd_resume(struct device *dev)
__maybe_unused as well.
> +{
> + struct locomokbd *locomokbd = dev_get_drvdata(dev);
> +
> + regmap_write(locomokbd->regmap, LOCOMO_KCMD, 1);
> + regmap_write(locomokbd->regmap, LOCOMO_KSC, 0);
> + regmap_update_bits(locomokbd->regmap, LOCOMO_KIC, 0x100, 0);
> + regmap_update_bits(locomokbd->regmap, LOCOMO_KIC, 0x10, 0x10);
> +
> + locomokbd_scankeyboard(locomokbd);
> +
> + return 0;
> +}
> +
> +static SIMPLE_DEV_PM_OPS(locomo_kbd_pm, locomokbd_suspend, locomokbd_resume);
> +#define LOCOMO_KBD_PM (&locomo_kbd_pm)
> +#else
> +#define LOCOMO_KBD_PM NULL
> +#endif
Just do
static SIMPLE_DEV_PM_OPS(locomo_kbd_pm,
locomokbd_suspend, locomokbd_resume);
outside of #ifdef, it will produce the right thing (an empty structure).
> +
> +static struct platform_driver locomokbd_driver = {
> + .driver = {
> + .name = "locomo-kbd",
> + .pm = LOCOMO_KBD_PM,
.pm = &locomo_kbd_pm;
> },
> - .devid = LOCOMO_DEVID_KEYBOARD,
> .probe = locomokbd_probe,
> .remove = locomokbd_remove,
> };
>
> -static int __init locomokbd_init(void)
> -{
> - return locomo_driver_register(&keyboard_driver);
> -}
> -
> -static void __exit locomokbd_exit(void)
> -{
> - locomo_driver_unregister(&keyboard_driver);
> -}
> +module_platform_driver(locomokbd_driver);
>
> -module_init(locomokbd_init);
> -module_exit(locomokbd_exit);
> +MODULE_AUTHOR("John Lenz <lenz@cs.wisc.edu>");
> +MODULE_DESCRIPTION("LoCoMo keyboard driver");
> +MODULE_LICENSE("GPL");
> +MODULE_ALIAS("platform:locomo-kbd");
> --
> 2.1.4
>
Thanks.
--
Dmitry
^ permalink raw reply
* Re: [PATCH v2 01/17] mfd: add new driver for Sharp LoCoMo
From: Dmitry Eremin-Solenikov @ 2015-05-12 20:39 UTC (permalink / raw)
To: Lee Jones
Cc: Russell King, Daniel Mack, Robert Jarzmik, Linus Walleij,
Alexandre Courbot, Wolfram Sang, Dmitry Torokhov, Bryan Wu,
Richard Purdie, Samuel Ortiz, Mark Brown, Jingoo Han,
Jean-Christophe Plagniol-Villard, Tomi Valkeinen, Liam Girdwood,
Andrea Adami, linux-arm-kernel, linux-gpio@vger.kernel.org,
linux-i2c, linux-input, linux-leds, linux-spi
In-Reply-To: <20150428184525.GJ9169@x1>
2015-04-28 21:45 GMT+03:00 Lee Jones <lee.jones@linaro.org>:
> On Tue, 28 Apr 2015, Dmitry Eremin-Solenikov wrote:
>
>> LoCoMo is a GA used on Sharp Zaurus SL-5x00. Current driver does has
>> several design issues (special bus instead of platform bus, doesn't use
>> mfd-core, etc).
>>
>> Implement 'core' parts of locomo support as an mfd driver.
>>
>> Signed-off-by: Dmitry Eremin-Solenikov <dbaryshkov@gmail.com>
>> ---
Thanks for the review. I agree (and have implemented) with most of
your comments.
However I have few questions. See below.
>
>> +/* the following is the overall data for the locomo chip */
>> +struct locomo {
>> + struct device *dev;
>> + unsigned int irq;
>> + spinlock_t lock;
>> + struct irq_domain *domain;
>> + struct regmap *regmap;
>> +};
>> +
>> +static struct resource locomo_kbd_resources[] = {
>> + DEFINE_RES_IRQ(IRQ_LOCOMO_KEY),
>> +};
>> +
>> +static struct resource locomo_gpio_resources[] = {
>> + DEFINE_RES_IRQ(IRQ_LOCOMO_GPIO),
>> +};
>> +
>> +/* Filled in locomo_probe() function. */
>> +static struct locomo_gpio_platform_data locomo_gpio_pdata;
>
> I'd prefer you didn't use globals for this.
Just for platform data, or for all the structures?
>> +static struct resource locomo_lt_resources[] = {
>> + DEFINE_RES_IRQ(IRQ_LOCOMO_LT),
>> +};
>> +
>> +static struct resource locomo_spi_resources[] = {
>> + DEFINE_RES_IRQ(IRQ_LOCOMO_SPI),
>> +};
>> +
>> +/* Filled in locomo_probe() function. */
>> +static struct locomo_lcd_platform_data locomo_lcd_pdata;
>> +
>> +static struct mfd_cell locomo_cells[] = {
>> + {
>> + .name = "locomo-kbd",
>> + .resources = locomo_kbd_resources,
>> + .num_resources = ARRAY_SIZE(locomo_kbd_resources),
>> + },
>> + {
>> + .name = "locomo-gpio",
>> + .resources = locomo_gpio_resources,
>> + .num_resources = ARRAY_SIZE(locomo_gpio_resources),
>> + .platform_data = &locomo_gpio_pdata,
>> + .pdata_size = sizeof(locomo_gpio_pdata),
>> + },
>> + {
>> + .name = "locomo-lt", /* Long time timer */
>> + .resources = locomo_lt_resources,
>> + .num_resources = ARRAY_SIZE(locomo_lt_resources),
>> + },
>> + {
>> + .name = "locomo-spi",
>> + .resources = locomo_spi_resources,
>> + .num_resources = ARRAY_SIZE(locomo_spi_resources),
>> + },
>> + {
>> + .name = "locomo-led",
>> + },
>> + {
>> + .name = "locomo-backlight",
>> + },
>
> Please make these:
>
>> + { .name = "locomo-led" },
>> + { .name = "locomo-backlight" },
>
> ... and put them at the bottom.
They will be populated by of_compatible lines, so it makes little sense
to me. What about adding of compatibility lines to this patch?
>> + while (1) {
>> + regmap_read(lchip->regmap, LOCOMO_ICR, &req);
>> + req &= 0x0f00;
>
> What is this magic number? Please #define it.
Adding comments to this function instead.
>
>> + if (!req)
>> + break;
>> +
>> + irq = ffs(req) - 9;
>
> Minus another random number? Either define it or enter a comment.
>
>> +#ifdef CONFIG_PM_SLEEP
>> +static int locomo_suspend(struct device *dev)
>> +{
>> + struct locomo *lchip = dev_get_drvdata(dev);
>> +
>> + /* AUDIO */
>
> WHY ARE YOU SHOUTING? Ironic eh? ;)
>
>> + regmap_write(lchip->regmap, LOCOMO_PAIF, 0x00);
>> +
>> + /*
>> + * Original code disabled the clock depending on leds settings
>> + * However we disable leds before suspend, thus it's safe
>> + * to just assume this setting.
>> + */
>> + /* CLK32 off */
>> + regmap_write(lchip->regmap, LOCOMO_C32K, 0x00);
>> +
>> + /* 22MHz/24MHz clock off */
>> + regmap_write(lchip->regmap, LOCOMO_ACC, 0x00);
>> +
>> + return 0;
>> +}
>> +
>> +static int locomo_resume(struct device *dev)
>> +{
>> + struct locomo *lchip = dev_get_drvdata(dev);
>
> Do audio and clk sort themselves out?
PAIF and ACC registers are used only by audio parts of the device. However
there is no current Linux driver for those parts. The registers are cleared
in case the firmware has set something in them, but in future it will
be the task
of the audio driver to properly clear and restore them.
>> + regmap_write(lchip->regmap, LOCOMO_C32K, 0x00);
>> +
>> + return 0;
>> +}
>> +
[skipped]
>> +
>> + if (pdata) {
>> + locomo_gpio_pdata.gpio_base = pdata->gpio_base;
>> + locomo_lcd_pdata.comadj = pdata->comadj;
>> + } else {
>> + locomo_gpio_pdata.gpio_base = -1;
>> + locomo_lcd_pdata.comadj = 128;
>> + }
>
> struct locomo_gpio_platform_data locomo_gpio_pdata;
>
> locomo_gpio_pdata = devm_kzalloc(<blah>);
>
> locomo_cells[GPIO].platform_data = locomo_gpio_pdata;
I do not quite agree with you at this place. The passed platform_data
will be kmemdup()'ed inside platform core. So the whole struct will be
duplicated twice inside kmallocate'd memory. Ideally I'd like to drop
the whole platform_data busyness, but that requires switching to DTS
first.
>> diff --git a/include/linux/mfd/locomo.h b/include/linux/mfd/locomo.h
>> new file mode 100644
>> index 0000000..6729767
>> --- /dev/null
>> +++ b/include/linux/mfd/locomo.h
>> +/* MCS decoder for boot selecting */
>> +#define LOCOMO_MCSX0 0x10
>> +#define LOCOMO_MCSX1 0x14
>> +#define LOCOMO_MCSX2 0x18
>> +#define LOCOMO_MCSX3 0x1c
>
> These are pretty cryptic. Any way of making them easier to identify.
No way. The names are based on old Sharp code. The drivers do not use
them, but I'd like to still keep the registers for the reference purposes.
>> +struct locomo_gpio_platform_data {
>> + unsigned int gpio_base;
>> +};
>
> A struct for a single int seems overkill.
>
>> +struct locomo_lcd_platform_data {
>> + u8 comadj;
>> +};
>> +
>> +struct locomo_platform_data {
>> + unsigned int gpio_base;
>> + u8 comadj;
>> +};
>
> Why do you need to pass gpio_base twice?
First: machine file -> core driver
Second: core driver -> gpio driver
The other way to do the same would be:
struct locomo_gpio_platform_data {
unsigned int gpio_base;
};
struct locomo_lcd_platform_data {
u8 comadj;
};
struct locomo_platform_data {
struct locomo_gpio_platform_data gpio_pdata;
struct locomo_lcd_platform_data lcd_pdata;
};
And to assign pointers to the passed data in the mfd_cells
during locomo_probe. Does that look better to you?
--
With best wishes
Dmitry
^ permalink raw reply
* Re: [PATCH v2 04/17] input: locomokbd: provide an Alt-SysRQ combination
From: Dmitry Eremin-Solenikov @ 2015-05-12 20:40 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: Russell King, Daniel Mack, Robert Jarzmik, Linus Walleij,
Alexandre Courbot, Wolfram Sang, Bryan Wu, Richard Purdie,
Samuel Ortiz, Lee Jones, Mark Brown, Jingoo Han,
Jean-Christophe Plagniol-Villard, Tomi Valkeinen, Liam Girdwood,
Andrea Adami, linux-arm-kernel,
linux-gpio-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
linux-i2c-u79uwXL29TY76Z2rM5mHXA, linux-input, linux-leds,
linux-spi
In-Reply-To: <20150512201215.GD20725@dtor-ws>
2015-05-12 23:12 GMT+03:00 Dmitry Torokhov <dmitry.torokhov@gmail.com>:
> Hi Dmitry,
>
> On Tue, Apr 28, 2015 at 02:55:41AM +0300, Dmitry Eremin-Solenikov wrote:
>> It is usefull sometimes to have an Alt-SysRQ combo on the keyboard to be
>> able to trigger sysrq functions directly. Add an option providing sysrq
>> mapping for Contact-Home keys.
>
> No, if you want to remap the keys please do it from usepspace; locomokbd
> supports it (EVIOCSKEYCODE ioctl).
OK, this was useful to boot kernels with minimal to no userspace (tiny
initramfs)
and still have the SysRQ at hand. I'll keep this as a local/OE patch instead.
--
With best wishes
Dmitry
^ permalink raw reply
* Re: [PATCH v2 03/17] input: convert LoCoMo keyboard driver to use new locomo core
From: Dmitry Eremin-Solenikov @ 2015-05-12 21:01 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: Russell King, Daniel Mack, Robert Jarzmik, Linus Walleij,
Alexandre Courbot, Wolfram Sang, Bryan Wu, Richard Purdie,
Samuel Ortiz, Lee Jones, Mark Brown, Jingoo Han,
Jean-Christophe Plagniol-Villard, Tomi Valkeinen, Liam Girdwood,
Andrea Adami, linux-arm-kernel, linux-gpio@vger.kernel.org,
linux-i2c, linux-input, linux-leds, linux-spi
In-Reply-To: <20150512202106.GE20725@dtor-ws>
Hello,
2015-05-12 23:21 GMT+03:00 Dmitry Torokhov <dmitry.torokhov@gmail.com>:
> Hi Dmitry,
>
> On Tue, Apr 28, 2015 at 02:55:40AM +0300, Dmitry Eremin-Solenikov wrote:
>> As LoCoMo is switching to new device model, adapt keyboard driver to
>> support new locomo core driver.
>>
>> Signed-off-by: Dmitry Eremin-Solenikov <dbaryshkov@gmail.com>
>> ---
Thanks for the review.
>> /* helper functions for reading the keyboard matrix */
>> -static inline void locomokbd_charge_all(unsigned long membase)
>> +static inline void locomokbd_charge_all(struct locomokbd *locomokbd)
>> {
>> - locomo_writel(0x00FF, membase + LOCOMO_KSC);
>> + regmap_write(locomokbd->regmap, LOCOMO_KSC, 0x00ff);
>> }
>>
>> -static inline void locomokbd_activate_all(unsigned long membase)
>> +static inline void locomokbd_activate_all(struct locomokbd *locomokbd)
>
> Drop "inline"s from the .c file please.
Why?
>
>> {
>> - unsigned long r;
>> -
>> - locomo_writel(0, membase + LOCOMO_KSC);
>> - r = locomo_readl(membase + LOCOMO_KIC);
>> - r &= 0xFEFF;
>> - locomo_writel(r, membase + LOCOMO_KIC);
>> + regmap_write(locomokbd->regmap, LOCOMO_KSC, 0);
>> + regmap_update_bits(locomokbd->regmap, LOCOMO_KIC, 0x100, 0);
>> }
>>
[skipped]
>> @@ -291,16 +275,30 @@ static int locomokbd_probe(struct locomo_dev *dev)
>>
>> input_set_drvdata(input_dev, locomokbd);
>>
>> - memcpy(locomokbd->keycode, locomokbd_keycode, sizeof(locomokbd->keycode));
>> + memcpy(locomokbd->keycode,
>> + locomokbd_keycode,
>> + sizeof(locomokbd->keycode));
>> +
>> + if (machine_is_collie())
>> + locomokbd->keycode[18] = KEY_HOME;
>> + else
>> + locomokbd->keycode[3] = KEY_HOME;
>
> This seems like a new addition. Ideally keymap twiddling shoudl be done
> from userspace.
This fixes a hardware issue. Home key is wired differently on two
devices using this driver.
I'd prefer to have such setting in board file or in DTS in future,
however that looks like an
overkill. What would be your suggestion?
>> /* attempt to get the interrupt */
>> - err = request_irq(dev->irq[0], locomokbd_interrupt, 0, "locomokbd", locomokbd);
>> + err = request_irq(locomokbd->irq, locomokbd_interrupt, 0,
>> + "locomokbd", locomokbd);
>
> devm_request_irq()?
>
[skipped]
>> -static int locomokbd_remove(struct locomo_dev *dev)
>> +static int locomokbd_remove(struct platform_device *dev)
>> {
>> - struct locomokbd *locomokbd = locomo_get_drvdata(dev);
>> + struct locomokbd *locomokbd = platform_get_drvdata(dev);
>>
>> - free_irq(dev->irq[0], locomokbd);
>> + free_irq(locomokbd->irq, locomokbd);
>
> Is not needed with devm.
Not quite. There will be a possibility for the IRQ to happen after deleting
a timer in locomokbd_remove() and before freeing the IRQ through the devres
core. Oops.
>
>>
>> del_timer_sync(&locomokbd->timer);
>
> Should likely to go into close().
Hmm. I will rethink this part, thank you.
>> +
>> +#ifdef CONFIG_PM_SLEEP
>> +static int locomokbd_suspend(struct device *dev)
>
> Mark as __maybe_unused instead of giarding with CONFIG_PM_SLEEP.
Fine, however I thought that #ifdef's here are a typical pattern.
[skipped the rest]
--
With best wishes
Dmitry
^ permalink raw reply
* Re: [PATCH v2 03/17] input: convert LoCoMo keyboard driver to use new locomo core
From: Dmitry Torokhov @ 2015-05-12 21:13 UTC (permalink / raw)
To: Dmitry Eremin-Solenikov
Cc: Russell King, Daniel Mack, Robert Jarzmik, Linus Walleij,
Alexandre Courbot, Wolfram Sang, Bryan Wu, Richard Purdie,
Samuel Ortiz, Lee Jones, Mark Brown, Jingoo Han,
Jean-Christophe Plagniol-Villard, Tomi Valkeinen, Liam Girdwood,
Andrea Adami, linux-arm-kernel, linux-gpio@vger.kernel.org,
linux-i2c, linux-input, linux-leds, linux-spi
In-Reply-To: <CALT56yOjiBWHH3Uuo82Hqh8Va8V_KCUaaLL7RFtPUjWmfOFbGA@mail.gmail.com>
On Wed, May 13, 2015 at 12:01:23AM +0300, Dmitry Eremin-Solenikov wrote:
> Hello,
>
> 2015-05-12 23:21 GMT+03:00 Dmitry Torokhov <dmitry.torokhov@gmail.com>:
> > Hi Dmitry,
> >
> > On Tue, Apr 28, 2015 at 02:55:40AM +0300, Dmitry Eremin-Solenikov wrote:
> >> As LoCoMo is switching to new device model, adapt keyboard driver to
> >> support new locomo core driver.
> >>
> >> Signed-off-by: Dmitry Eremin-Solenikov <dbaryshkov@gmail.com>
> >> ---
>
> Thanks for the review.
>
> >> /* helper functions for reading the keyboard matrix */
> >> -static inline void locomokbd_charge_all(unsigned long membase)
> >> +static inline void locomokbd_charge_all(struct locomokbd *locomokbd)
> >> {
> >> - locomo_writel(0x00FF, membase + LOCOMO_KSC);
> >> + regmap_write(locomokbd->regmap, LOCOMO_KSC, 0x00ff);
> >> }
> >>
> >> -static inline void locomokbd_activate_all(unsigned long membase)
> >> +static inline void locomokbd_activate_all(struct locomokbd *locomokbd)
> >
> > Drop "inline"s from the .c file please.
>
> Why?
Because compiler usually knows better whether a function should be
inlined or not as it keeps track of available registers, so leave the
decision to it.
>
> >
> >> {
> >> - unsigned long r;
> >> -
> >> - locomo_writel(0, membase + LOCOMO_KSC);
> >> - r = locomo_readl(membase + LOCOMO_KIC);
> >> - r &= 0xFEFF;
> >> - locomo_writel(r, membase + LOCOMO_KIC);
> >> + regmap_write(locomokbd->regmap, LOCOMO_KSC, 0);
> >> + regmap_update_bits(locomokbd->regmap, LOCOMO_KIC, 0x100, 0);
> >> }
> >>
>
> [skipped]
>
> >> @@ -291,16 +275,30 @@ static int locomokbd_probe(struct locomo_dev *dev)
> >>
> >> input_set_drvdata(input_dev, locomokbd);
> >>
> >> - memcpy(locomokbd->keycode, locomokbd_keycode, sizeof(locomokbd->keycode));
> >> + memcpy(locomokbd->keycode,
> >> + locomokbd_keycode,
> >> + sizeof(locomokbd->keycode));
> >> +
> >> + if (machine_is_collie())
> >> + locomokbd->keycode[18] = KEY_HOME;
> >> + else
> >> + locomokbd->keycode[3] = KEY_HOME;
> >
> > This seems like a new addition. Ideally keymap twiddling shoudl be done
> > from userspace.
>
> This fixes a hardware issue. Home key is wired differently on two
> devices using this driver.
> I'd prefer to have such setting in board file or in DTS in future,
> however that looks like an
> overkill. What would be your suggestion?
I am OK with doing this in driver, just as a separate patch please.
>
> >> /* attempt to get the interrupt */
> >> - err = request_irq(dev->irq[0], locomokbd_interrupt, 0, "locomokbd", locomokbd);
> >> + err = request_irq(locomokbd->irq, locomokbd_interrupt, 0,
> >> + "locomokbd", locomokbd);
> >
> > devm_request_irq()?
> >
> [skipped]
>
> >> -static int locomokbd_remove(struct locomo_dev *dev)
> >> +static int locomokbd_remove(struct platform_device *dev)
> >> {
> >> - struct locomokbd *locomokbd = locomo_get_drvdata(dev);
> >> + struct locomokbd *locomokbd = platform_get_drvdata(dev);
> >>
> >> - free_irq(dev->irq[0], locomokbd);
> >> + free_irq(locomokbd->irq, locomokbd);
> >
> > Is not needed with devm.
>
> Not quite. There will be a possibility for the IRQ to happen after deleting
> a timer in locomokbd_remove() and before freeing the IRQ through the devres
> core. Oops.
Right, but if you make sure that device does not generate interrupts in
probe() until open() is called and do the same in close(), then it
should be OK.
>
> >
> >>
> >> del_timer_sync(&locomokbd->timer);
> >
> > Should likely to go into close().
>
> Hmm. I will rethink this part, thank you.
>
> >> +
> >> +#ifdef CONFIG_PM_SLEEP
> >> +static int locomokbd_suspend(struct device *dev)
> >
> > Mark as __maybe_unused instead of giarding with CONFIG_PM_SLEEP.
>
> Fine, however I thought that #ifdef's here are a typical pattern.
It is up to subsystems, __maybe_unused provides better compile coverage.
Thanks.
--
Dmitry
^ permalink raw reply
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