[PATCH v9 1/3] i2c: iProc: define Broadcom iProc I2C binding
Document the I2C device tree binding for Broadcom iProc family of
SoCs
Signed-off-by: Ray Jui
Reviewed-by: Scott Branden
Reviewed-by: Kevin Cernekee
---
.../devicetree/bindings/i2c/brcm,iproc-i2c.txt | 37
1 file changed, 37 insertions(+)
create mode 100644 Documentation/devicetree/bindings/i2c/brcm,iproc-i2c.txt
diff --git a/Documentation/devicetree/bindings/i2c/brcm,iproc-i2c.txt
b/Documentation/devicetree/bindings/i2c/brcm,iproc-i2c.txt
new file mode 100644
index 000..81f982c
--- /dev/null
+++ b/Documentation/devicetree/bindings/i2c/brcm,iproc-i2c.txt
@@ -0,0 +1,37 @@
+Broadcom iProc I2C controller
+
+Required properties:
+
+- compatible:
+Must be "brcm,iproc-i2c"
+
+- reg:
+Define the base and range of the I/O address space that contain the iProc
+I2C controller registers
+
+- interrupts:
+Should contain the I2C interrupt
+
+- clock-frequency:
+This is the I2C bus clock. Need to be either 10 or 40
+
+- #address-cells:
+Always 1 (for I2C addresses)
+
+- #size-cells:
+Always 0
+
+Example:
+ i2c0: i2c@18008000 {
+ compatible = "brcm,iproc-i2c";
+ reg = <0x18008000 0x100>;
+ #address-cells = <1>;
+ #size-cells = <0>;
+ interrupts = ;
+ clock-frequency = <10>;
+
+ codec: wm8750@1a {
+ compatible = "wlf,wm8750";
+ reg = <0x1a>;
+ };
+ };
--
1.7.9.5
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v9 2/3] i2c: iproc: Add Broadcom iProc I2C Driver
Add initial support to the Broadcom iProc I2C controller found in the
iProc family of SoCs.
The iProc I2C controller has separate internal TX and RX FIFOs, each has
a size of 64 bytes. The iProc I2C controller supports two bus speeds
including standard mode (100kHz) and fast mode (400kHz)
Signed-off-by: Ray Jui
Reviewed-by: Scott Branden
Reviewed-by: Kevin Cernekee
---
drivers/i2c/busses/Kconfig | 10 +
drivers/i2c/busses/Makefile|1 +
drivers/i2c/busses/i2c-bcm-iproc.c | 461
3 files changed, 472 insertions(+)
create mode 100644 drivers/i2c/busses/i2c-bcm-iproc.c
diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
index ab838d9..3d08731 100644
--- a/drivers/i2c/busses/Kconfig
+++ b/drivers/i2c/busses/Kconfig
@@ -372,6 +372,16 @@ config I2C_BCM2835
This support is also available as a module. If so, the module
will be called i2c-bcm2835.
+config I2C_BCM_IPROC
+ tristate "Broadcom iProc I2C controller"
+ depends on ARCH_BCM_IPROC || COMPILE_TEST
+ default ARCH_BCM_IPROC
+ help
+ If you say yes to this option, support will be included for the
+ Broadcom iProc I2C controller.
+
+ If you don't know what to do here, say N.
+
config I2C_BCM_KONA
tristate "BCM Kona I2C adapter"
depends on ARCH_BCM_MOBILE
diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile
index 56388f6..d93b509 100644
--- a/drivers/i2c/busses/Makefile
+++ b/drivers/i2c/busses/Makefile
@@ -33,6 +33,7 @@ obj-$(CONFIG_I2C_AT91)+= i2c-at91.o
obj-$(CONFIG_I2C_AU1550) += i2c-au1550.o
obj-$(CONFIG_I2C_AXXIA)+= i2c-axxia.o
obj-$(CONFIG_I2C_BCM2835) += i2c-bcm2835.o
+obj-$(CONFIG_I2C_BCM_IPROC)+= i2c-bcm-iproc.o
obj-$(CONFIG_I2C_BLACKFIN_TWI) += i2c-bfin-twi.o
obj-$(CONFIG_I2C_CADENCE) += i2c-cadence.o
obj-$(CONFIG_I2C_CBUS_GPIO)+= i2c-cbus-gpio.o
diff --git a/drivers/i2c/busses/i2c-bcm-iproc.c
b/drivers/i2c/busses/i2c-bcm-iproc.c
new file mode 100644
index 000..d3c8915
--- /dev/null
+++ b/drivers/i2c/busses/i2c-bcm-iproc.c
@@ -0,0 +1,461 @@
+/*
+ * Copyright (C) 2014 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
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+
+#define CFG_OFFSET 0x00
+#define CFG_RESET_SHIFT 31
+#define CFG_EN_SHIFT 30
+#define CFG_M_RETRY_CNT_SHIFT16
+#define CFG_M_RETRY_CNT_MASK 0x0f
+
+#define TIM_CFG_OFFSET 0x04
+#define TIM_CFG_MODE_400_SHIFT 31
+
+#define M_FIFO_CTRL_OFFSET 0x0c
+#define M_FIFO_RX_FLUSH_SHIFT31
+#define M_FIFO_TX_FLUSH_SHIFT30
+#define M_FIFO_RX_CNT_SHIFT 16
+#define M_FIFO_RX_CNT_MASK 0x7f
+#define M_FIFO_RX_THLD_SHIFT 8
+#define M_FIFO_RX_THLD_MASK 0x3f
+
+#define M_CMD_OFFSET 0x30
+#define M_CMD_START_BUSY_SHIFT 31
+#define M_CMD_STATUS_SHIFT 25
+#define M_CMD_STATUS_MASK0x07
+#define M_CMD_STATUS_SUCCESS 0x0
+#define M_CMD_STATUS_LOST_ARB0x1
+#define M_CMD_STATUS_NACK_ADDR 0x2
+#define M_CMD_STATUS_NACK_DATA 0x3
+#define M_CMD_STATUS_TIMEOUT 0x4
+#define M_CMD_PROTOCOL_SHIFT 9
+#define M_CMD_PROTOCOL_MASK 0xf
+#define M_CMD_PROTOCOL_BLK_WR0x7
+#define M_CMD_PROTOCOL_BLK_RD0x8
+#define M_CMD_PEC_SHIFT 8
+#define M_CMD_RD_CNT_SHIFT 0
+#define M_CMD_RD_CNT_MASK0xff
+
+#define IE_OFFSET0x38
+#define IE_M_RX_FIFO_FULL_SHIFT 31
+#define IE_M_RX_THLD_SHIFT 30
+#define IE_M_START_BUSY_SHIFT28
+
+#define IS_OFFSET0x3c
+#define IS_M_RX_FIFO_FULL_SHIFT 31
+#define IS_M_RX_THLD_SHIFT 30
+#define IS_M_START_BUSY_SHIFT28
+
+#define M_TX_OFFSET 0x40
+#define M_TX_WR_STATUS_SHIFT 31
+#define M_TX_DATA_SHIFT 0
+#define M_TX_DATA_MASK 0xff
+
+#define M_RX_OFFSET 0x44
+#define M_RX_STATUS_SHIFT30
+#define M_RX_STATUS_MASK 0x03
+#define M_RX_PEC_ERR_SHIFT 29
+#define M_RX_DATA_SHIFT 0
+#define M_RX_DATA_MASK 0xff
+
+#define I2C_TIMEOUT_MESC 100
+#define M_TX_RX_FIFO_SIZE64
+
+enum bus_speed_index {
+ I2C_SPD_100K = 0,
+ I2C_SPD_400K,
+};
+
+struct bcm_iproc_i2c_dev {
+
[PATCH v9 3/3] ARM: dts: add I2C device nodes for Broadcom Cygnus
Add I2C device nodes and its properties in bcm-cygnus.dtsi but keep
them disabled there. Individual I2C devices can be enabled in board
specific dts file when I2C slave devices are enabled in the future
Signed-off-by: Ray Jui
Reviewed-by: Scott Branden
Reviewed-by: Kevin Cernekee
---
arch/arm/boot/dts/bcm-cygnus.dtsi | 20
1 file changed, 20 insertions(+)
diff --git a/arch/arm/boot/dts/bcm-cygnus.dtsi
b/arch/arm/boot/dts/bcm-cygnus.dtsi
index 5126f9e..ff5fb6a 100644
--- a/arch/arm/boot/dts/bcm-cygnus.dtsi
+++ b/arch/arm/boot/dts/bcm-cygnus.dtsi
@@ -70,6 +70,26 @@
};
};
+ i2c0: i2c@18008000 {
+ compatible = "brcm,cygnus-iproc-i2c", "brcm,iproc-i2c";
+ reg = <0x18008000 0x100>;
+ #address-cells = <1>;
+ #size-cells = <0>;
+ interrupts = ;
+ clock-frequency = <10>;
+ status = "disabled";
+ };
+
+ i2c1: i2c@1800b000 {
+ compatible = "brcm,cygnus-iproc-i2c", "brcm,iproc-i2c";
+ reg = <0x1800b000 0x100>;
+ #address-cells = <1>;
+ #size-cells = <0>;
+ interrupts = ;
+ clock-frequency = <10>;
+ status = "disabled";
+ };
+
uart0: serial@1802 {
compatible = "snps,dw-apb-uart";
reg = <0x1802 0x100>;
--
1.7.9.5
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v9 0/3] Add I2C support to Broadcom iProc
This patchset contains the initial I2C support for Broadcom iProc family of SoCs. The iProc I2C controller has separate internal TX and RX FIFOs, each has a size of 64 bytes. The iProc I2C controller supports two bus speeds including standard mode (100 kHz) and fast mode (400 kHz) Synced code base to Linux 3.19-rc7 Changes from v8: - Sort header includes in alphabetical order - Use correct error code - Get rid of redundant functions and combine functions to make driver more slim - Get rid of redundant debugging prints that are already available from the I2C framework - Other minor improvements Changes from v7: - Remove redundant 10-bit address check in the driver - Fix the driver that accidentally emits 1-byte of data with zero content in the case of SMBUS quick command - Improve debugging prints in the driver - Other minor improvements Changes from v6: - Get rid of unnecessary atomic variable usage in the driver - Improve the "waiting for transaction to complete" logic further by making sure there's no pending/ongoing interrupt by the time when flag 'xfer_is_done' is checked - After disabling interrupt with 'writel', add 'readl' to the same register to flush the bus to ensure the write has gone through Changes from v5: - Improve the "waiting for transaction to be complete" logic to take care of the corner case when an interrupt fires after wait_for_completion_timeout times out - Improve the logic to disable I2C interrupt in the remove function. Make it more generic so it works for both dedicated and shared interrupt Changes from v4: - Remove redundant header file includes - Change the logic that waits for the host controller to be idle to simply return -EBUSY - Use proper print level and error codes in the driver - Allow zero length message in the driver to support I2C_SMBUS_QUICK - Change back to use devm_request_irq. Disable interrupt in the remove function so there's no outstanding I2C interrupt when the driver is being removed from the framework - Other minor miscellaneous improvements and fixes Changes from v3: - Add config dependency to COMPILE_TEST to allow the driver to be build tested by other platforms - Improve CPU utilization efficiency in the loop of waiting for bus to idle - Add more comment in the driver to clarify the way how the "start busy" interrupt is triggered from the I2C controller - Fix inconsistent coding style and format - Improve the bus speed validation logic in the driver - Add code to free the interrupt line in driver's remove function. Also change to use non-devm API to request the interrupt line - Other miscellaneous improvements and fixes Changes from v2: - Have the I2C driver default to y so it does not need to be selected from ARCH_BCM_IPROC. This also helps to get rid of one patch. The driver still depends on ARCH_BCM_IPROC - Get rid of redundant check on resource returned by platform_get_resource Changes from v1: - Fix function argument parenthesis - Get rid of redundant driver owner field Ray Jui (3): i2c: iProc: define Broadcom iProc I2C binding i2c: iproc: Add Broadcom iProc I2C Driver ARM: dts: add I2C device nodes for Broadcom Cygnus .../devicetree/bindings/i2c/brcm,iproc-i2c.txt | 37 ++ arch/arm/boot/dts/bcm-cygnus.dtsi | 20 + drivers/i2c/busses/Kconfig | 10 + drivers/i2c/busses/Makefile|1 + drivers/i2c/busses/i2c-bcm-iproc.c | 461 5 files changed, 529 insertions(+) create mode 100644 Documentation/devicetree/bindings/i2c/brcm,iproc-i2c.txt create mode 100644 drivers/i2c/busses/i2c-bcm-iproc.c -- 1.7.9.5 -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v8 2/3] i2c: iproc: Add Broadcom iProc I2C Driver
On 2/7/2015 9:50 AM, Wolfram Sang wrote:
> Hi Ray,
>
> On Fri, Feb 06, 2015 at 05:28:26PM -0800, Ray Jui wrote:
>> Add initial support to the Broadcom iProc I2C controller found in the
>> iProc family of SoCs.
>>
>> The iProc I2C controller has separate internal TX and RX FIFOs, each has
>> a size of 64 bytes. The iProc I2C controller supports two bus speeds
>> including standard mode (100kHz) and fast mode (400kHz)
>
> Mostly looking good.
>
>> +#include
>> +#include
>> +#include
>> +#include
>> +#include
>> +#include
>> +#include
>> +#include
>
> Please sort the includes.
>
Will do.
>> +static bool bcm_iproc_i2c_bus_busy(struct bcm_iproc_i2c_dev *iproc_i2c)
>> +{
>> +if (readl(iproc_i2c->base + M_CMD_OFFSET) &
>> +(1 << M_CMD_START_BUSY_SHIFT))
>> +return true;
>> +else
>> +return false;
>> +}
>
> Minor: return !!(readl(...))? You decide.
>
Okay will do that. Will also remove this function since now it becomes
one line and is used only once.
>> +
>> +static int bcm_iproc_i2c_format_addr(struct bcm_iproc_i2c_dev *iproc_i2c,
>> + struct i2c_msg *msg, u8 *addr)
>> +{
>> +*addr = msg->addr << 1;
>> +
>> +if (msg->flags & I2C_M_RD)
>> +*addr |= 1;
>> +
>> +return 0;
>> +}
>
> I'd suggest a oneliner.
>
> *addr = msg->addr << 1 | (msg->flags & I2C_M_RD ? 1 : 0)
>
> Or use !! like above.
>
> Don't do an extra function for that. It is only used once and it also
> doesn't need to be int since it can't fail anyhow.
>
> (Note to self: I should make a macro for that in i2c.h)
>
Yes will change. Thanks.
>> +/* need to reserve one byte in the FIFO for the slave address */
>> +if (msg->len > M_TX_RX_FIFO_SIZE - 1) {
>> +dev_err(iproc_i2c->device,
>> +"only support data length up to %u bytes\n",
>> +M_TX_RX_FIFO_SIZE - 1);
>> +return -EINVAL;
>
> -EOPNOTSUPP
>
> Is it really a HW limitation? Could the driver later be extended to
> continue filling the FIFO if a certain threshold is reached?
>
Will return -EOPNOTSUPP. This really depends on whether or not we expect
one sequence of START + SLV ADDR + DATA + STOP per i2c message. I can
later extend the driver to refill/re-drain the FIFO for data size >= 64
bytes, if one sequence of SATRT...STOP per message is not a requirement.
>> +dev_dbg(iproc_i2c->device, "xfer %c, addr=0x%02x, len=%d\n",
>> +(msg->flags & I2C_M_RD) ? 'R' : 'W', msg->addr,
>> +msg->len);
>> +dev_dbg(iproc_i2c->device, "*** data: %*ph\n", msg->len, msg->buf);
>
> Not really needed. We have tracing for that.
>
Will remove.
>> +if (bus_speed < 10) {
>> +dev_err(iproc_i2c->device, "%d Hz bus speed not supported\n",
>> +bus_speed);
>> +dev_err(iproc_i2c->device,
>> +"valid speeds are 100khz and 400khz\n");
>> +return -EINVAL;
>> +} else if (bus_speed < 40) {
>> +bus_speed = 10;
>> +speed_bit = 0;
>> +} else {
>> +bus_speed = 40;
>> +speed_bit = 1;
>> +}
>> +
>> +val = readl(iproc_i2c->base + TIM_CFG_OFFSET);
>> +val &= ~(1 << TIM_CFG_MODE_400_SHIFT);
>> +val |= speed_bit << TIM_CFG_MODE_400_SHIFT;
>
> val |= (bus_speed == 40) ...
>
> and skip speed_bit? You decide.
>
Okay, I'll get rid of speed_bit.
>> +static void bcm_iproc_i2c_enable(struct bcm_iproc_i2c_dev *iproc_i2c)
>> +{
>> +u32 val;
>> +
>> +val = readl(iproc_i2c->base + CFG_OFFSET);
>> +val |= 1 << CFG_EN_SHIFT;
>> +writel(val, iproc_i2c->base + CFG_OFFSET);
>> +}
>> +
>> +static void bcm_iproc_i2c_disable(struct bcm_iproc_i2c_dev *iproc_i2c)
>> +{
>> +u32 val;
>> +
>> +val = readl(iproc_i2c->base + CFG_OFFSET);
>> +val &= ~(1 << CFG_EN_SHIFT);
>> +writel(val, iproc_i2c->base + CFG_OFFSET);
>> +}
>
> Extra functions? They are self explaining and only used once. You
> decide.
In fact I'll keep the function, since it will likely be needed later
when we add suspend/resume support to the driver. But I'll combine the
two functions and make it a single function called
bcm_iproc_i2c_enable_disable.
>
> Rest looks fine, thanks!
>
Thanks for the review!
Ray
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH/RFC 0/4] Probe deferral for IOMMU DT integration
Laura Abbott hat am 6. Februar 2015 um 01:31 geschrieben: > > The requirement for this is based on a previous patch to add clock > support to the ARM SMMU driver[2]. Once we have clock support, it's > possible that the driver itself may need to be defered which breaks > a bunch of assumptions about how SMMU probing is supposed to work. Hi Laura, I was hoping that we would not need this, and instead treat the iommu in the same way as timers and SMP initialization, both of which need to be run early at boot time but may rely on clock controllers to be initialized first. Is there a specific requirement that makes this impossible here, or is your intention to solve the problem more nicely by allowing deferred probing over forcing the input clocks of the iommu to be early? Arnd -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/3] clk: hi6220: Clock driver support for Hisilicon hi6220 SoC
Hi Bintian, On 7 February 2015 at 10:05, Brent Wang wrote: > Hello Tyler, > > Thank you very much for helping test this patchset! Not a problem. > > 2015-02-07 2:10 GMT+08:00 Tyler Baker : >> Hi Bintian, >> >> This patch applied to next-20150204 is producing build failures on >> various ARM defconfigs[1]. I received the following error in all >> cases: >> >> drivers/built-in.o: In function `hi6220_clk_register_divider': >> :(.init.text+0x1a84c): undefined reference to `hi6220_register_clkdiv' >> Makefile:925: recipe for target 'vmlinux' failed >> make: *** [vmlinux] Error 1 > It's my fault, I just test on ARM64, The following patch can fix this error: > = > diff --git a/drivers/clk/hisilicon/Makefile b/drivers/clk/hisilicon/Makefile > index bbf0539..48f0116 100644 > --- a/drivers/clk/hisilicon/Makefile > +++ b/drivers/clk/hisilicon/Makefile > @@ -2,9 +2,9 @@ > # Hisilicon Clock specific Makefile > # > > -obj-y += clk.o clkgate-separated.o > +obj-y += clk.o clkgate-separated.o clkdivider-hi6220.o > > obj-$(CONFIG_ARCH_HI3xxx) += clk-hi3620.o > obj-$(CONFIG_ARCH_HIP04) += clk-hip04.o > obj-$(CONFIG_ARCH_HIX5HD2) += clk-hix5hd2.o > -obj-$(CONFIG_COMMON_CLK_HI6220)+= clkdivider-hi6220.o clk-hi6220.o > +obj-$(CONFIG_COMMON_CLK_HI6220)+= clk-hi6220.o > == > > I will fix this problem in next version. Great, thank you. I quickly tested your change above, and it gets the ARM defconfigs building again. > > Thanks, > > Bintian > > >> >> On 5 February 2015 at 01:24, Bintian Wang wrote: >>> Add clock drivers for hi6220 SoC, this driver controls the SoC >>> registers to supply different clocks to different IPs in the SoC. >>> >>> We add one divider clock for hi6220 because the divider in hi6220 >>> also has a mask bit but it doesnot obey the rule defined by flag >>> "CLK_DIVIDER_HIWORD_MASK", we can not get index of the mask bit by >>> left shift fixed bits (e.g. 16 bits), so we add this divider clock >>> to handle it. >>> >>> This patch also enables this clock driver for ARCH_HISI and document >>> devicetree bindings. >>> >>> Signed-off-by: Bintian Wang >>> Reviewed-by: Haojian Zhuang >>> Reviewed-by: Zhangfei Gao >>> --- >>> .../devicetree/bindings/clock/hi6220-clock.txt | 30 +++ >>> arch/arm64/Kconfig |1 + >>> drivers/clk/Kconfig|2 + >>> drivers/clk/Makefile |4 +- >>> drivers/clk/hisilicon/Kconfig |5 + >>> drivers/clk/hisilicon/Makefile |1 + >>> drivers/clk/hisilicon/clk-hi6220.c | 284 >>> >>> drivers/clk/hisilicon/clk.c| 29 ++ >>> drivers/clk/hisilicon/clk.h| 17 ++ >>> drivers/clk/hisilicon/clkdivider-hi6220.c | 273 >>> +++ >>> include/dt-bindings/clock/hi6220-clock.h | 172 >>> 11 files changed, 815 insertions(+), 3 deletions(-) >>> create mode 100644 Documentation/devicetree/bindings/clock/hi6220-clock.txt >>> create mode 100644 drivers/clk/hisilicon/Kconfig >>> create mode 100644 drivers/clk/hisilicon/clk-hi6220.c >>> create mode 100644 drivers/clk/hisilicon/clkdivider-hi6220.c >>> create mode 100644 include/dt-bindings/clock/hi6220-clock.h >>> >>> diff --git a/Documentation/devicetree/bindings/clock/hi6220-clock.txt >>> b/Documentation/devicetree/bindings/clock/hi6220-clock.txt >>> new file mode 100644 >>> index 000..a3ddda1 >>> --- /dev/null >>> +++ b/Documentation/devicetree/bindings/clock/hi6220-clock.txt >>> @@ -0,0 +1,30 @@ >>> +* Hisilicon Hi6220 Clock Controller >>> + >>> +The hi6220 clock controller generates and supplies clock to various >>> +controllers within the hi6220 SoC. >>> + >>> +Required Properties: >>> + >>> +- compatible: should be one of the following: >>> + - "hisilicon,hi6220-clock-ao" - controller for those clocks under SoC >>> + power always on(AO) domain, it is the sub node of SoC power AO >>> + controller in dts file. >>> + - "hisilicon,hi6220-clock-sys" - controller for those clocks under SoC >>> + system control domain, it is the sub node of SoC system controller >>> + in dts file. >>> + - "hisilicon,hi6220-clock-media" - controller for those clocks under >>> + SoC media control domain, it is the sub node of SoC media controller >>> + in dts file. >>> + - "hisilicon,hi6220-clock-power" - controller for those clocks under >>> + SoC power control domain, it is the sub node of SoC power controller >>> + in dts file. >>> + >>> +- reg: physical base address of the controller and length of memory mapped >>> + region. >>> + >>> +- #clock-cells: should be 1. >>> + >>> +Each clock is assigned an identifier and client nodes use this identifier >>> +to specify the clock which they consume. >>> + >>> +All these identifier could be found in . >>> diff --gi
Re: [PATCH 0/2] GPIO joystick driver
Hi Hans, On Sat, Feb 07, 2015 at 07:35:46AM +, Holmberg, Hans wrote: > Hi Dmitry, > > > Why do we need this new driver when we have gpio-keys driver that can be > > configured to emit ABS_* events? > > > > As far as I can tell, there is no way to specify values for ABS-"keys" in the > device tree binding. It may mot be present in device tree binding, but the driver does support it, so I would rather extend the binding than have a brand new driver. > > While both the device tree binding and the gpio-keys driver could possibly be > adapted to > support this, it makes sense to me to describe joysticks in a separate > binding and keep > the driver implementations apart. Why? From the input core perspective there is no difference. It is just a device that emits set of events. > > In addition to this, auto-repeat and the sys fs disable interface for > individual keys > does not make sense for joysticks. You do not need to use it when you describe the device (BTW autorepeat for trigger might be a good thing). Thanks. -- Dmitry -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/1] arm: Fix unavailable MTD userland devices on Excito B3 boards
On Sat, Feb 07, 2015 at 08:59:46PM +0100, Gordon Bos wrote: > Hi Andrew, > > There is a difference, and apparently it somewhere got solved: > > From running kernel 3.17.1 > > ~ # dmesg | grep spi > > Running an upgraded kernel 3.18.5 with the same DT blob > > ~ # dmesg | grep spi >[0.928832] m25p80 spi0.0: found m25p16, expected m25p80 >[0.934124] m25p80 spi0.0: m25p16 (2048 Kbytes) So this is with your patch applied. You can see the driver is complaining the DT says there should be a m25p80, but it actually found a m25p16. If you remove your change, this warning will go away. > You may therefore consider the MTD issue as solved. Great. Andrew -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/1] arm: Fix unavailable MTD userland devices on Excito B3 boards
On Sat, Feb 07, 2015 at 07:22:13PM +0100, Gordon Bos wrote: > Hi Andrew, > > I may be mistaken here, but this here references the driver that is > to be used, or not? > > |compatible = "st,m25p16"; Nope, that is the device, not the driver. The driver has a list of devices it is capable of driving. The kernel uses that list to find the right driver. > In the kernels I tried, m25p80 driver is simply not happy with > m25p16 being defined in DT. Please define "not happy". Please show me the kernel log of it being not happy. We need to compare and contrast my kernel log, where it is happy with "st,m25p16" against your kernel log where it is unhappy. Then we might have some clues to understand what is going on. Thanks Andrew -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v8 2/3] i2c: iproc: Add Broadcom iProc I2C Driver
Hi Ray,
On Fri, Feb 06, 2015 at 05:28:26PM -0800, Ray Jui wrote:
> Add initial support to the Broadcom iProc I2C controller found in the
> iProc family of SoCs.
>
> The iProc I2C controller has separate internal TX and RX FIFOs, each has
> a size of 64 bytes. The iProc I2C controller supports two bus speeds
> including standard mode (100kHz) and fast mode (400kHz)
Mostly looking good.
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
Please sort the includes.
> +static bool bcm_iproc_i2c_bus_busy(struct bcm_iproc_i2c_dev *iproc_i2c)
> +{
> + if (readl(iproc_i2c->base + M_CMD_OFFSET) &
> + (1 << M_CMD_START_BUSY_SHIFT))
> + return true;
> + else
> + return false;
> +}
Minor: return !!(readl(...))? You decide.
> +
> +static int bcm_iproc_i2c_format_addr(struct bcm_iproc_i2c_dev *iproc_i2c,
> + struct i2c_msg *msg, u8 *addr)
> +{
> + *addr = msg->addr << 1;
> +
> + if (msg->flags & I2C_M_RD)
> + *addr |= 1;
> +
> + return 0;
> +}
I'd suggest a oneliner.
*addr = msg->addr << 1 | (msg->flags & I2C_M_RD ? 1 : 0)
Or use !! like above.
Don't do an extra function for that. It is only used once and it also
doesn't need to be int since it can't fail anyhow.
(Note to self: I should make a macro for that in i2c.h)
> + /* need to reserve one byte in the FIFO for the slave address */
> + if (msg->len > M_TX_RX_FIFO_SIZE - 1) {
> + dev_err(iproc_i2c->device,
> + "only support data length up to %u bytes\n",
> + M_TX_RX_FIFO_SIZE - 1);
> + return -EINVAL;
-EOPNOTSUPP
Is it really a HW limitation? Could the driver later be extended to
continue filling the FIFO if a certain threshold is reached?
> + dev_dbg(iproc_i2c->device, "xfer %c, addr=0x%02x, len=%d\n",
> + (msg->flags & I2C_M_RD) ? 'R' : 'W', msg->addr,
> + msg->len);
> + dev_dbg(iproc_i2c->device, "*** data: %*ph\n", msg->len, msg->buf);
Not really needed. We have tracing for that.
> + if (bus_speed < 10) {
> + dev_err(iproc_i2c->device, "%d Hz bus speed not supported\n",
> + bus_speed);
> + dev_err(iproc_i2c->device,
> + "valid speeds are 100khz and 400khz\n");
> + return -EINVAL;
> + } else if (bus_speed < 40) {
> + bus_speed = 10;
> + speed_bit = 0;
> + } else {
> + bus_speed = 40;
> + speed_bit = 1;
> + }
> +
> + val = readl(iproc_i2c->base + TIM_CFG_OFFSET);
> + val &= ~(1 << TIM_CFG_MODE_400_SHIFT);
> + val |= speed_bit << TIM_CFG_MODE_400_SHIFT;
val |= (bus_speed == 40) ...
and skip speed_bit? You decide.
> +static void bcm_iproc_i2c_enable(struct bcm_iproc_i2c_dev *iproc_i2c)
> +{
> + u32 val;
> +
> + val = readl(iproc_i2c->base + CFG_OFFSET);
> + val |= 1 << CFG_EN_SHIFT;
> + writel(val, iproc_i2c->base + CFG_OFFSET);
> +}
> +
> +static void bcm_iproc_i2c_disable(struct bcm_iproc_i2c_dev *iproc_i2c)
> +{
> + u32 val;
> +
> + val = readl(iproc_i2c->base + CFG_OFFSET);
> + val &= ~(1 << CFG_EN_SHIFT);
> + writel(val, iproc_i2c->base + CFG_OFFSET);
> +}
Extra functions? They are self explaining and only used once. You
decide.
Rest looks fine, thanks!
signature.asc
Description: Digital signature
Re: [PATCH 1/1] arm: Fix unavailable MTD userland devices on Excito B3 boards
On Sat, Feb 07, 2015 at 06:15:12PM +0100, Gordon Bos wrote:
> Hi Andrew,
>
> Unsure how it can work for you, but this is the corresponding entry
> from the Excito release:
>
> ---
> |/*
> * 2048KB SPI Flash on Boot Device (Numonyx MP25P16)
> /
>
> static struct mtd_partition bubba3_flash_parts[] = {
> {
> .name = "u-boot",
> .size = SZ_512K+SZ_256K,
> .offset = 0,
> },
> {
> .name = "env",
> .size = SZ_128K,
> .offset = MTDPART_OFS_NXTBLK,
> },
> {
> .name = "data",
> .size = MTDPART_SIZ_FULL,
> .offset = MTDPART_OFS_NXTBLK,
> },
> };
>
> static const struct flash_platform_data bubba3_flash = {
> .type = "m25p16",
So the device is a m25p16.
> .name = "spi_flash",
> .parts = bubba3_flash_parts,
> .nr_parts = ARRAY_SIZE(bubba3_flash_parts),
> };
>
> static struct spi_board_info __initdata bubba3_spi_slave_info[] = {
> {
> .modalias = "m25p80",
And the driver is m25p80.
This is consistent with the current device tree description.
So what actually happens when you use the mainline DT on your device?
I'm not going to accept any changes until we understand what problems
you have and why it works for me.
Thanks
Andrew
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 2/5] ARM: dts: pxa: add clocks
Hello.
On 02/07/2015 04:16 PM, Robert Jarzmik wrote:
+ stuart: uart@4070 {
+ clocks = <&pxa2xx_clks CLK_STUART>;
+ };
+
The ePAPR standard tells to call such nodes "serial", not "uart".
Good to know, but not for this patch.
The naming is coming from pxa2xx.dtsi.
Ah, I was wondering why the node descriptions included only one prop.
> And you're very welcome to post a patch to fix that ;)
In my copious free time... :-)
Cheers.
WBR, Sergei
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/1] arm: Fix unavailable MTD userland devices on Excito B3 boards
On Sat, Feb 07, 2015 at 04:04:59PM +0100, Gordon Bos wrote:
> Hi,
>
> I've had some further discussion on this with another B3 owner.
> According to an old patch that Excito corporation built for kernel
> version 2.6 the flash memory is in fact a ||Numonyx MP25P16
> The driver however must be m25p80.
Hi Gordon
Here is what my B3 says during boot:
m25p80 spi0.0: m25p16 (2048 Kbytes)
3 ofpart partitions found on MTD device spi0.0
Creating 3 MTD partitions on "spi0.0":
0x-0x000c : "u-boot"
0x000c-0x000e : "u-boot env"
0x000e-0x0020 : "data"
m25p80 is the driver being used. The device is an m25p16.
> m25p16@0 {
> #address-cells = <1>;
> #size-cells = <1>;
> - compatible = "st,m25p16";
> + compatible = "st,m25p80";
Here, compatibility indicates what the device is. The kernel will then
find a driver for that device. If you look in
drivers/mtd/devices/m25p80.c you see a long list of devices this
driver supports, of which m25p16.
Also, if you specify the wrong device here, the kernel will complain
when it asks the device to identify itself and find it is different:
http://lxr.free-electrons.com/source/drivers/mtd/spi-nor/spi-nor.c#L915
it will print a message: "found %s, expected %s\n"
Are you seeing such a message? Maybe your B3 has a different device
than mine?
Thanks
Andrew
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/1] arm: Fix unavailable MTD userland devices on Excito B3 boards
On Sat, Feb 07, 2015 at 04:04:59PM +0100, Gordon Bos wrote: > Hi, > > I've had some further discussion on this with another B3 owner. > According to an old patch that Excito corporation built for kernel > version 2.6 the flash memory is in fact a ||Numonyx MP25P16 > The driver however must be m25p80. Hi Gordon I need to reboot my B3 in order to see what it says. However, with the current DT i do have access to the MTD devices. What problems are you actually having? Thanks Andrew -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 0/5] ARM: sun9i: Add USB host controller support for A80
On Tue, Feb 03, 2015 at 06:22:00AM +0800, Chen-Yu Tsai wrote: > Hi everyone, > > This is v4 of the sun9i A80 USB host support series. > > Changes since v3: > > - Dropped patches merged. > > - Moved reg_usb3_vbus into the optimus board dts > > - Dropped ohci1 from A80 dtsi. > > Cover letter from v3: > > This series adds USB host controller (EHCI/OHCI) support for the Allwinner > A80 SoC. The A80 has 3 pairs of host controllers and USB PHYs. The PHYs, > unlike in previous SoCs, do not have low level control registers anymore. > > As such, this series forgoes the original phy-sun4i-usb driver, and adds > a new, simpler driver for the USB PHYs. It may be possible to merge the > two, but given that work is being done on the OTG front for the earlier > SoCs, it may be better to merge them after support is complete. > > EHCI/OHCI0 corresponds to USB1 DP/DM pins; EHCI1 only has HSIC support; > EHCI2/OHCI/2 is USB2 DP/DM externally. External pins labeled USB0 are > for the USB 3.0 OTG controller. Applied 2-5. Thanks! Maxime -- Maxime Ripard, Free Electrons Embedded Linux, Kernel and Android engineering http://free-electrons.com signature.asc Description: Digital signature
Re: [PATCH v3 2/5] ARM: dts: pxa: add clocks
Sergei Shtylyov writes:
> Hello.
>
> On 2/7/2015 3:39 PM, Robert Jarzmik wrote:
>> +stuart: uart@4070 {
>> +clocks = <&pxa2xx_clks CLK_STUART>;
>> +};
>> +
>
>The ePAPR standard tells to call such nodes "serial", not "uart".
Good to know, but not for this patch.
The naming is coming from pxa2xx.dtsi. And you're very welcome to post a patch
to fix that ;)
Cheers.
--
Robert
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 2/5] ARM: dts: pxa: add clocks
Hello.
On 2/7/2015 3:39 PM, Robert Jarzmik wrote:
Add clocks to the IPs already described in the pxa device-tree
files. There are more clocks in the clock tree than IPs described in the
current pxa device-tree.
This patch ensures that :
- the current description is correct
- the clocks are actually claimed, so that clock framework doesn't
disable them automatically (unused clocks shutdown)
Signed-off-by: Robert Jarzmik
---
arch/arm/boot/dts/pxa27x.dtsi | 31 ---
1 file changed, 28 insertions(+), 3 deletions(-)
diff --git a/arch/arm/boot/dts/pxa27x.dtsi b/arch/arm/boot/dts/pxa27x.dtsi
index 98b560e..e8d5097 100644
--- a/arch/arm/boot/dts/pxa27x.dtsi
+++ b/arch/arm/boot/dts/pxa27x.dtsi
[...]
@@ -12,36 +12,62 @@
marvell,intc-nr-irqs = <34>;
};
+ gpio: gpio@40e0 {
+ compatible = "intel,pxa27x-gpio";
+ clocks = <&pxa2xx_clks CLK_NONE>;
+ };
+
+ ffuart: uart@4010 {
+ clocks = <&pxa2xx_clks CLK_FFUART>;
+ };
+
+ btuart: uart@4020 {
+ clocks = <&pxa2xx_clks CLK_BTUART>;
+ };
+
+ stuart: uart@4070 {
+ clocks = <&pxa2xx_clks CLK_STUART>;
+ };
+
The ePAPR standard tells to call such nodes "serial", not "uart".
[...]
WBR, Sergei
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v3 3/5] ARM: dts: pxa: add pxa27x-udc to pxa27x
Each pxa27x has an embedded usb udc controller. Add it in the pxa27x
device-tree description.
Signed-off-by: Robert Jarzmik
---
arch/arm/boot/dts/pxa27x.dtsi | 8
1 file changed, 8 insertions(+)
diff --git a/arch/arm/boot/dts/pxa27x.dtsi b/arch/arm/boot/dts/pxa27x.dtsi
index e8d5097..ddb6982 100644
--- a/arch/arm/boot/dts/pxa27x.dtsi
+++ b/arch/arm/boot/dts/pxa27x.dtsi
@@ -68,6 +68,14 @@
pxai2c1: i2c@40301680 {
clocks = <&pxa2xx_clks CLK_I2C>;
};
+
+ pxa27x_udc: udc@4060 {
+ compatible = "marvell,pxa270-udc";
+ reg = <0x4060 0x1>;
+ interrupts = <11>;
+ clocks = <&pxa2xx_clks CLK_USB>;
+ status = "disabled";
+ };
};
clocks {
--
2.1.0
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v3 4/5] ARM: dts: pxa: add pxa27x-keypad to pxa27x
Each pxa27x has an embedded keypad controller. Add it in the pxa27x
device-tree description.
Signed-off-by: Robert Jarzmik
---
arch/arm/boot/dts/pxa27x.dtsi | 8
1 file changed, 8 insertions(+)
diff --git a/arch/arm/boot/dts/pxa27x.dtsi b/arch/arm/boot/dts/pxa27x.dtsi
index ddb6982..6ecf1a6 100644
--- a/arch/arm/boot/dts/pxa27x.dtsi
+++ b/arch/arm/boot/dts/pxa27x.dtsi
@@ -76,6 +76,14 @@
clocks = <&pxa2xx_clks CLK_USB>;
status = "disabled";
};
+
+ keypad: keypad@4150 {
+ compatible = "marvell,pxa27x-keypad";
+ reg = <0x4150 0x4c>;
+ interrupts = <4>;
+ clocks = <&pxa2xx_clks CLK_KEYPAD>;
+ status = "disabled";
+ };
};
clocks {
--
2.1.0
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v3 2/5] ARM: dts: pxa: add clocks
Add clocks to the IPs already described in the pxa device-tree
files. There are more clocks in the clock tree than IPs described in the
current pxa device-tree.
This patch ensures that :
- the current description is correct
- the clocks are actually claimed, so that clock framework doesn't
disable them automatically (unused clocks shutdown)
Signed-off-by: Robert Jarzmik
---
arch/arm/boot/dts/pxa27x.dtsi | 31 ---
1 file changed, 28 insertions(+), 3 deletions(-)
diff --git a/arch/arm/boot/dts/pxa27x.dtsi b/arch/arm/boot/dts/pxa27x.dtsi
index 98b560e..e8d5097 100644
--- a/arch/arm/boot/dts/pxa27x.dtsi
+++ b/arch/arm/boot/dts/pxa27x.dtsi
@@ -1,6 +1,6 @@
/* The pxa3xx skeleton simply augments the 2xx version */
#include "pxa2xx.dtsi"
-#include "dt-bindings/clock/pxa2xx-clock.h"
+#include "dt-bindings/clock/pxa-clock.h"
/ {
model = "Marvell PXA27x familiy SoC";
@@ -12,36 +12,62 @@
marvell,intc-nr-irqs = <34>;
};
+ gpio: gpio@40e0 {
+ compatible = "intel,pxa27x-gpio";
+ clocks = <&pxa2xx_clks CLK_NONE>;
+ };
+
+ ffuart: uart@4010 {
+ clocks = <&pxa2xx_clks CLK_FFUART>;
+ };
+
+ btuart: uart@4020 {
+ clocks = <&pxa2xx_clks CLK_BTUART>;
+ };
+
+ stuart: uart@4070 {
+ clocks = <&pxa2xx_clks CLK_STUART>;
+ };
+
pwm0: pwm@40b0 {
compatible = "marvell,pxa270-pwm", "marvell,pxa250-pwm";
reg = <0x40b0 0x10>;
#pwm-cells = <1>;
+ clocks = <&pxa2xx_clks CLK_PWM0>;
};
pwm1: pwm@40b00010 {
compatible = "marvell,pxa270-pwm", "marvell,pxa250-pwm";
reg = <0x40b00010 0x10>;
#pwm-cells = <1>;
+ clocks = <&pxa2xx_clks CLK_PWM1>;
};
pwm2: pwm@40c0 {
compatible = "marvell,pxa270-pwm", "marvell,pxa250-pwm";
reg = <0x40c0 0x10>;
#pwm-cells = <1>;
+ clocks = <&pxa2xx_clks CLK_PWM0>;
};
pwm3: pwm@40c00010 {
compatible = "marvell,pxa270-pwm", "marvell,pxa250-pwm";
reg = <0x40c00010 0x10>;
#pwm-cells = <1>;
+ clocks = <&pxa2xx_clks CLK_PWM1>;
};
pwri2c: i2c@40f000180 {
compatible = "mrvl,pxa-i2c";
reg = <0x40f00180 0x24>;
interrupts = <6>;
+ clocks = <&pxa2xx_clks CLK_PWRI2C>;
status = "disabled";
};
+
+ pxai2c1: i2c@40301680 {
+ clocks = <&pxa2xx_clks CLK_I2C>;
+ };
};
clocks {
@@ -54,10 +80,9 @@
ranges;
pxa2xx_clks: pxa2xx_clks@4134 {
- compatible = "marvell,pxa-clocks";
+ compatible = "marvell,pxa270-clocks";
#clock-cells = <1>;
status = "okay";
};
};
-
};
--
2.1.0
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v3 5/5] ARM: dts: pxa: add pxa-timer to pxa27x
Each pxa has an embedded OS Timers IP. The kernel cannot work without a
valid clocksource, and this adds the OS Timers to the pxa device-tree
description.
Signed-off-by: Robert Jarzmik
---
Since v1: removed clocksource node, pxa-timer being directly under
pxabus (Rob's comment).
Since v2: renamed pxa-timer to timer (Sergei's comment)
---
arch/arm/boot/dts/pxa27x.dtsi | 8
1 file changed, 8 insertions(+)
diff --git a/arch/arm/boot/dts/pxa27x.dtsi b/arch/arm/boot/dts/pxa27x.dtsi
index 6ecf1a6..fb0abad 100644
--- a/arch/arm/boot/dts/pxa27x.dtsi
+++ b/arch/arm/boot/dts/pxa27x.dtsi
@@ -101,4 +101,12 @@
status = "okay";
};
};
+
+ timer@40a0 {
+ compatible = "marvell,pxa-timer";
+ reg = <0x40a0 0x20>;
+ interrupts = <26>;
+ clocks = <&pxa2xx_clks CLK_OSTIMER>;
+ status = "okay";
+ };
};
--
2.1.0
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v3 1/5] ARM: dts: pxa: add pwri2c to pxa device-tree
pxa27x variant has 2 I2C busses on the SoC :
- the casual I2C
- the power I2C, normally driving power regulators, and capable of
receiving orders on core frequency modifications
Add the missing pwri2c to pxa27x description.
Signed-off-by: Robert Jarzmik
---
Since v2: as Dmitry pointed out, no pwri2c on pxa25x, only pxa27x
---
arch/arm/boot/dts/pxa27x.dtsi | 7 +++
1 file changed, 7 insertions(+)
diff --git a/arch/arm/boot/dts/pxa27x.dtsi b/arch/arm/boot/dts/pxa27x.dtsi
index 80fc5d7..98b560e 100644
--- a/arch/arm/boot/dts/pxa27x.dtsi
+++ b/arch/arm/boot/dts/pxa27x.dtsi
@@ -35,6 +35,13 @@
reg = <0x40c00010 0x10>;
#pwm-cells = <1>;
};
+
+ pwri2c: i2c@40f000180 {
+ compatible = "mrvl,pxa-i2c";
+ reg = <0x40f00180 0x24>;
+ interrupts = <6>;
+ status = "disabled";
+ };
};
clocks {
--
2.1.0
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 3/4] ARM: dts: exynos3250: use pmureg device node to enable mipi phy
This patch removes mipi phy relevant properties from dsim device node and makes the pmureg device node to be used instead to enable mipi phy. Signed-off-by: Inki Dae --- arch/arm/boot/dts/exynos3250.dtsi |3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/arch/arm/boot/dts/exynos3250.dtsi b/arch/arm/boot/dts/exynos3250.dtsi index 2246549..eb80802 100644 --- a/arch/arm/boot/dts/exynos3250.dtsi +++ b/arch/arm/boot/dts/exynos3250.dtsi @@ -246,8 +246,7 @@ interrupts = <0 83 0>; samsung,phy-type = <0>; samsung,power-domain = <&pd_lcd0>; - phys = <&mipi_phy 1>; - phy-names = "dsim"; + samsung,pmureg = <&pmu_system_controller>; clocks = <&cmu CLK_DSIM0>, <&cmu CLK_SCLK_MIPI0>; clock-names = "bus_clk", "pll_clk"; #address-cells = <1>; -- 1.7.9.5 -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 4/4] ARM: dts: exynos5420: use pmureg device node to enable mipi phy
This patch removes mipi phy relevant properties from dsim device node and makes the pmureg device node to be used instead to enable mipi phy. Signed-off-by: Inki Dae --- arch/arm/boot/dts/exynos5420.dtsi |3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/arch/arm/boot/dts/exynos5420.dtsi b/arch/arm/boot/dts/exynos5420.dtsi index 6d38f8b..b8b8826 100644 --- a/arch/arm/boot/dts/exynos5420.dtsi +++ b/arch/arm/boot/dts/exynos5420.dtsi @@ -525,8 +525,7 @@ compatible = "samsung,exynos5410-mipi-dsi"; reg = <0x1450 0x1>; interrupts = <0 82 0>; - phys = <&mipi_phy 1>; - phy-names = "dsim"; + samsung,pmureg = <&pmu_system_controller>; clocks = <&clock CLK_DSIM1>, <&clock CLK_SCLK_MIPI1>; clock-names = "bus_clk", "pll_clk"; #address-cells = <1>; -- 1.7.9.5 -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 1/4] drm/exynos: dsim: fix to control mipi phy register
This patch fixes the issue that the try to get a phy object is failed
to enable mipi phy.
System and power management unit registers should be controlled by
syscon framework. So this patch removes existing phy framework based
codes and adds syscon support instead. However, we should support
legacy device tree binding so consider the legacy binding for compatibility.
In addition, we need to remove below device node and relevant properties,
mipi_phy: video-phy@10020710 {
compatible = "samsung,s5pv210-mipi-video-phy";
reg = <0x10020710 8>;
#phy-cells = <1>;
};
Now camera device node uses mipi_phy node relevant properties like below,
camera {
...
csis_0: csis@1188 {
...
phys = <&mipi_phy 0>;
phy-names = "csis";
...
};
csis_1: csis@1189 {
...
phys = <&mipi_phy 2>;
phy-names = "csis";
...
};
...
};
With above, we will find below message while booting,
can't request region for resource [mem 0x10020710-0x10020717]
Signed-off-by: Inki Dae
---
.../devicetree/bindings/video/exynos_dsim.txt |9 ++--
drivers/gpu/drm/exynos/exynos_drm_dsi.c| 54 ++--
2 files changed, 56 insertions(+), 7 deletions(-)
diff --git a/Documentation/devicetree/bindings/video/exynos_dsim.txt
b/Documentation/devicetree/bindings/video/exynos_dsim.txt
index ca2b4aa..dec3b55 100644
--- a/Documentation/devicetree/bindings/video/exynos_dsim.txt
+++ b/Documentation/devicetree/bindings/video/exynos_dsim.txt
@@ -11,15 +11,18 @@ Required properties:
- clocks: list of clock specifiers, must contain an entry for each required
entry in clock-names
- clock-names: should include "bus_clk"and "pll_clk" entries
- - phys: list of phy specifiers, must contain an entry for each required
-entry in phy-names
- - phy-names: should include "dsim" entry
+ - samsung,pmureg: handle to syscon used to control the PMU registers
- vddcore-supply: MIPI DSIM Core voltage supply (e.g. 1.1V)
- vddio-supply: MIPI DSIM I/O and PLL voltage supply (e.g. 1.8V)
- samsung,pll-clock-frequency: specifies frequency of the "pll_clk" clock
- #address-cells, #size-cells: should be set respectively to <1> and <0>
according to DSI host bindings (see MIPI DSI bindings [1])
+Deprecated properties for MIPI DSI Master:
+ - phys: list of phy specifiers, must contain an entry for each required
+entry in phy-names (Deprecated)
+ - phy-names: should include "dsim" entry (Deprecated)
+
Optional properties:
- samsung,power-domain: a phandle to DSIM power domain node
diff --git a/drivers/gpu/drm/exynos/exynos_drm_dsi.c
b/drivers/gpu/drm/exynos/exynos_drm_dsi.c
index 05fe93d..38b025e 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_dsi.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_dsi.c
@@ -23,6 +23,8 @@
#include
#include
#include
+#include
+#include
#include
#include
@@ -263,6 +265,8 @@ struct exynos_dsi_transfer {
struct exynos_dsi_driver_data {
unsigned int plltmr_reg;
+ unsigned int mipi_phy_offset;
+ unsigned int mipi_phy_en_shift;
unsigned int has_freqband:1;
unsigned int has_clklane_stop:1;
};
@@ -277,6 +281,7 @@ struct exynos_dsi {
void __iomem *reg_base;
struct phy *phy;
+ struct regmap *pmureg;
struct clk *pll_clk;
struct clk *bus_clk;
struct regulator_bulk_data supplies[2];
@@ -313,21 +318,29 @@ static struct exynos_dsi_driver_data
exynos3_dsi_driver_data = {
.plltmr_reg = 0x50,
.has_freqband = 1,
.has_clklane_stop = 1,
+ .mipi_phy_offset = 0x710,
+ .mipi_phy_en_shift = 0,
};
static struct exynos_dsi_driver_data exynos4_dsi_driver_data = {
.plltmr_reg = 0x50,
.has_freqband = 1,
.has_clklane_stop = 1,
+ .mipi_phy_offset = 0x710,
+ .mipi_phy_en_shift = 0,
};
static struct exynos_dsi_driver_data exynos4415_dsi_driver_data = {
.plltmr_reg = 0x58,
.has_clklane_stop = 1,
+ .mipi_phy_offset = 0x710,
+ .mipi_phy_en_shift = 0,
};
static struct exynos_dsi_driver_data exynos5_dsi_driver_data = {
.plltmr_reg = 0x58,
+ .mipi_phy_offset = 0x714,
+ .mipi_phy_en_shift = 0,
};
static struct of_device_id exynos_dsi_of_match[] = {
@@ -1294,6 +1307,7 @@ static const struct mipi_dsi_host_ops exynos_dsi_ops = {
static int exynos_dsi_poweron(struct exynos_dsi *dsi)
{
+ struct exynos_dsi_driver_data *driver_data = dsi->driver_data;
int ret;
ret = regulator_bulk_enable(ARRAY_SIZE(dsi->supplies), dsi->supplies);
@@ -1314,7 +1328,14 @@ static int exynos_dsi_poweron(struct exynos_dsi *dsi)
[PATCH 2/4] ARM: dts: exynos4: use pmureg device node to enable mipi phy
This patch removes mipi phy relevant properties from dsim device node and makes the pmureg device node to be used instead to enable mipi phy. Signed-off-by: Inki Dae --- arch/arm/boot/dts/exynos4.dtsi |3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/arch/arm/boot/dts/exynos4.dtsi b/arch/arm/boot/dts/exynos4.dtsi index b8168f1..42dfdcf 100644 --- a/arch/arm/boot/dts/exynos4.dtsi +++ b/arch/arm/boot/dts/exynos4.dtsi @@ -148,8 +148,7 @@ reg = <0x11C8 0x1>; interrupts = <0 79 0>; samsung,power-domain = <&pd_lcd0>; - phys = <&mipi_phy 1>; - phy-names = "dsim"; + samsung,pmureg = <&pmu_system_controller>; clocks = <&clock CLK_DSIM0>, <&clock CLK_SCLK_MIPI0>; clock-names = "bus_clk", "pll_clk"; status = "disabled"; -- 1.7.9.5 -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 0/4] drm/exynos: use syscon framework to enable mipi phy
This patch series makes syscon framework to be used instead of phy framework. For this, I adds syscon support to mipi dsi driver and the relevant device tree properties to each dtsi files, Exynos4, Exynos3250 and Exynos5420. Inki Dae (4): drm/exynos: dsim: fix to control mipi phy register ARM: dts: exynos4: use pmureg device node to enable mipi phy ARM: dts: exynos3250: use pmureg device node to enable mipi phy ARM: dts: exynos5420: use pmureg device node to enable mipi phy .../devicetree/bindings/video/exynos_dsim.txt |9 ++-- arch/arm/boot/dts/exynos3250.dtsi |3 +- arch/arm/boot/dts/exynos4.dtsi |3 +- arch/arm/boot/dts/exynos5420.dtsi |3 +- drivers/gpu/drm/exynos/exynos_drm_dsi.c| 54 ++-- 5 files changed, 59 insertions(+), 13 deletions(-) -- 1.7.9.5 -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 1/3] irqchip: vf610-mscm-ir: add support for MSCM interrupt router
On Fri, 2015-02-06 at 20:51 +0100, Stefan Agner wrote: > This adds support for Vybrid's interrupt router. On VF6xx models, > almost all peripherals can be used by either of the two CPU's, > the Cortex-A5 or the Cortex-M4. The interrupt router routes the > peripheral interrupts to the configured CPU. > > This IRQ chip driver configures the interrupt router to route > the requested interrupt to the CPU the kernel is running on. > The driver makes use of the irqdomain hierarchy support. The > parent is given by the device tree. This should be one of the > two possible parents either ARM GIC or the ARM NVIC interrupt > controller. The latter is currently not yet supported. > > Note that there is no resource control mechnism implemented to > avoid concurrent access of the same peripheral. The user needs > to make sure to use device trees which assign the peripherals > orthogonally. However, this driver warns the user in case the > interrupt is already configured for the other CPU. This provides > a poor man's resource controller. > > Acked-by: Marc Zyngier > Signed-off-by: Stefan Agner > --- > arch/arm/mach-imx/Kconfig | 1 + > drivers/irqchip/Kconfig | 11 ++ > drivers/irqchip/Makefile| 1 + > drivers/irqchip/irq-vf610-mscm-ir.c | 206 > > 4 files changed, 219 insertions(+) > create mode 100644 drivers/irqchip/irq-vf610-mscm-ir.c > > diff --git a/arch/arm/mach-imx/Kconfig b/arch/arm/mach-imx/Kconfig > index e8627e0..bf91a59 100644 > --- a/arch/arm/mach-imx/Kconfig > +++ b/arch/arm/mach-imx/Kconfig > @@ -631,6 +631,7 @@ config SOC_IMX6SX > > config SOC_VF610 > bool "Vybrid Family VF610 support" > + select VF610_MSCM_IR > select ARM_GIC > select PINCTRL_VF610 > select PL310_ERRATA_769419 if CACHE_L2X0 > diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig > index cc79d2a..9c13d81 100644 > --- a/drivers/irqchip/Kconfig > +++ b/drivers/irqchip/Kconfig > @@ -136,6 +136,17 @@ config IRQ_CROSSBAR > a free irq and configures the IP. Thus the peripheral interrupts are > routed to one of the free irqchip interrupt lines. > > +config VF610_MSCM_IR > + bool > + help > + Support for MSCM interrupt router available on Vybrid SoC's. The > + interrupt router is between the CPU's interrupt controller and the > + peripheral. The router allows to route the peripheral interrupts to > + one of the two available CPU's on Vybrid VF6xx SoC's (Cortex-A5 or > + Cortex-M4). The router will be configured transparently on a IRQ > + request. > + select IRQ_DOMAIN_HIERARCHY > + As far as I can tell this new Kconfig symbol operates in lockstep with SOC_VF610: if SOC_VF610 is set, this will also be set, and if SOC_VF610 is not set, this won't be set either. Is a separate symbol needed? If you decide to keep it, I have two minor nits. 1) Make the help text the last option of the Kconfig entry. It's legal to put Kconfig options in any order that you'd like. But with very few exceptions, the help text is always the last. Please use that pattern. 2) This Kconfig entry has no prompt, so I'm not aware of a way that people ever can read this help text when running "make *configure". So this help text is basically a comment. You might as well format it as a comment then. > config KEYSTONE_IRQ > tristate "Keystone 2 IRQ controller IP" > depends on ARCH_KEYSTONE Thanks, Paul Bolle -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] ARM: dts: exynos4412-trats2: set display clock correctly
This patch sets display clock correctly.
If Display clock isn't set correctly then you would find below messages
and Display controller doesn't work correctly since a patch[1]
exynos-drm: No connectors reported connected with modes
[drm] Cannot find any crtc or sizes - going 1024x768
[1] commit abc0b1447d49 ("drm: Perform basic sanity checks on probed modes")
Signed-off-by: Inki Dae
---
arch/arm/boot/dts/exynos4412-trats2.dts |2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/arm/boot/dts/exynos4412-trats2.dts
b/arch/arm/boot/dts/exynos4412-trats2.dts
index 29231b4..1ec4d33 100644
--- a/arch/arm/boot/dts/exynos4412-trats2.dts
+++ b/arch/arm/boot/dts/exynos4412-trats2.dts
@@ -691,7 +691,7 @@
display-timings {
timing-0 {
- clock-frequency = <0>;
+ clock-frequency = <57153600>;
hactive = <720>;
vactive = <1280>;
hfront-porch = <5>;
--
1.7.9.5
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv6 4/5] edac: altera: Add Altera L2 Cache and OCRAM EDAC Support
On Thu, Jan 08, 2015 at 08:53:55PM -0600, ttha...@opensource.altera.com wrote:
> +static int altr_edac_device_probe(struct platform_device *pdev)
> +{
> + struct edac_device_ctl_info *dci;
> + struct altr_edac_device_dev *drvdata;
> + struct resource *r;
> + int res = 0;
> + struct device_node *np = pdev->dev.of_node;
> + char *ecc_name = (char *)np->name;
> + static int dev_instance;
> +
> + if (!devres_open_group(&pdev->dev, NULL, GFP_KERNEL)) {
> + edac_printk(KERN_ERR, EDAC_DEVICE,
> + "Unable to open devm\n");
> + return -ENOMEM;
> + }
Why are you opening a devres group here? The device model already opens
a devres group ready for the ->probe callback, which is released when
the device is unbound... hmm, see below though...
> +
> + r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + if (!r) {
> + edac_printk(KERN_ERR, EDAC_DEVICE,
> + "Unable to get mem resource\n");
> + return -EPROBE_DEFER;
Why EPROBE_DEFER for a missing resource?
> + }
> +
> + if (!devm_request_mem_region(&pdev->dev, r->start, resource_size(r),
> + dev_name(&pdev->dev))) {
> + edac_printk(KERN_ERR, EDAC_DEVICE,
> + "%s:Error requesting mem region\n", ecc_name);
> + return -EBUSY;
> + }
> +
> + dci = edac_device_alloc_ctl_info(sizeof(*drvdata), ecc_name,
> + 1, ecc_name, 1, 0, NULL, 0,
> + dev_instance++);
> +
> + if (!dci) {
> + edac_printk(KERN_ERR, EDAC_DEVICE,
> + "%s: Unable to allocate EDAC device\n", ecc_name);
> + return -ENOMEM;
> + }
> +
> + drvdata = dci->pvt_info;
> + dci->dev = &pdev->dev;
> + platform_set_drvdata(pdev, dci);
> + drvdata->edac_dev_name = ecc_name;
> +
> + drvdata->base = devm_ioremap(&pdev->dev, r->start, resource_size(r));
> + if (!drvdata->base)
> + goto err;
You could use devm_ioremap_resource() to simplify the mapping, resource
allocation and resource error handling.
> +
> + /* Get driver specific data for this EDAC device */
> + drvdata->data = of_match_node(altr_edac_device_of_match, np)->data;
> +
> + /* Check specific dependencies for the module */
> + if (drvdata->data->setup) {
> + res = drvdata->data->setup(pdev, drvdata->base);
> + if (res < 0)
> + goto err;
> + }
> +
> + drvdata->sb_irq = platform_get_irq(pdev, 0);
> + res = devm_request_irq(&pdev->dev, drvdata->sb_irq,
> +altr_edac_device_handler,
> +0, dev_name(&pdev->dev), dci);
> + if (res < 0)
> + goto err;
> +
> + drvdata->db_irq = platform_get_irq(pdev, 1);
> + res = devm_request_irq(&pdev->dev, drvdata->db_irq,
> +altr_edac_device_handler,
> +0, dev_name(&pdev->dev), dci);
> + if (res < 0)
> + goto err;
> +
> + dci->mod_name = "Altera EDAC Memory";
> + dci->dev_name = drvdata->edac_dev_name;
> +
> + altr_set_sysfs_attr(dci, drvdata->data);
> +
> + if (edac_device_add_device(dci))
> + goto err;
> +
> + devres_close_group(&pdev->dev, NULL);
> +
> + return 0;
> +err:
> + edac_printk(KERN_ERR, EDAC_DEVICE,
> + "%s:Error setting up EDAC device: %d\n", ecc_name, res);
> + devres_release_group(&pdev->dev, NULL);
> + edac_device_free_ctl_info(dci);
Hmm, I guess this is why you're using devm groups - it seems like
edac allocation/freeing needs to be improved to work cooperatively
with devm_* APIs rather than having to add devm group complexity
into drivers.
--
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [alsa-devel] [PATCH v2 3/3] ASoC: add generic dt-card support
On Tue, Feb 03, 2015 at 08:31:30PM +0100, Jean-Francois Moine wrote: > Mark Brown wrote: > > So how does the simple controller interact with a more complex one given > > that it's somehow picking some controller node to start from? > A way to solve this problem could be to create only one card builder. > This creation could be explicit (created by the first active audio > controller) or implicit by the audio subsystem on the first controller or > CODEC creation. > Then, the card builder could scan all the DT looking for the audio > ports and create one or more cards according to the graph > connectivity. How is this going to work with dynamically instantiated hardware like DT overlays? > > If you have a device with any sort of speaker or microphone, or any sort > > of external connector for interfacing with an external device like a > > headphone jack, then you have something that could be a widget. > I know what are the widgets and routes, I was just wondering why they > (especially the widgets) need to appear at the card level instead of > just being declared in the DAIs (from the platform or the DT). As previously and repeatedly discussed DAIs have no special place in a general audio system and we can't base the entire system off them. Which DAI should have the headphone jack connected to the analogue only headphone driver in my system (there may not even be a way to route digital audio to it)? How does this work for off-SoC audio hubs where there is a device with multiple DAIs connected to both one or more other digital devices and the analogue? Please go and research this if you're intending to work on generic bindings, it gets extremely repetitive to have to go over this again and again. We already have simple-card to provide a binding for trivial systems and don't want to end up with a never ending series of slightly more complicated bindings which each cover slightly different sets of systems in ways that users struggle to differentiate between. > And the same question may also be raised about the audio formats, clocks, > tdm's... Similar things here - which of the two or more devices on a digital audio link (yes, they're buses not point to point links) has the configuration and how do we stitch them together? How do we figure out when and how to do runtime reconfiguration of the clock tree (which is needed by some systems)? Again, please do some research on this. If you are trying to define generic device tree bindings it is really important that you understand what you are trying to model with those bindings. signature.asc Description: Digital signature
Re: [Patch V4 00/10] ASoC: QCOM: Add support for ipq806x SOC
On Thu, Feb 05, 2015 at 12:53:36PM -0800, Kenneth Westfield wrote: > This patch series adds support for I2S audio playback on the Qualcomm > Technologies ipq806x SOC. > The ipq806x SOC has audio-related hardware blocks in its low-power audio > subsystem (or LPASS). One of the relevant blocks in the LPASS is its > low-power > audio interface (or LPAIF). This contains an MI2S port, which is what these > drivers are configured to use. The LPAIF also contains a DMA engine that is > dedicated to moving audio samples into the transmit FIFO of the MI2S port. In > addition, there is also low-power memory (LPM) within the audio subsystem, > which > is used for buffering the audio samples. This is implementing an AP centric audio system design where the AP directly programs all the audio hardware. Given that pretty much all public Qualcomm systems use a DSP centric model where the AP interacts only with a DSP which deals with DMA and the physical interfaces it seems reasonable to suppose that this system also has a DSP which at some future point people are likely to want to use. I'd really like to see some discussion as to how this is all supposed to be handled - how will these direct hardware access drivers and device trees work when someone does want to use the DSP (without causing problems), and how will we transition from one to the other. This is particularly pressing if there are use cases where people will want to switch between the two modes at runtime. What I'm trying to avoid here is being in a situation where we have existing stable DT bindings which we have to support but which conflict with the way that people want to use the systems. signature.asc Description: Digital signature
Re: [Patch V4 05/10] ASoC: ipq806x: add LPASS header files
On Thu, Feb 05, 2015 at 12:53:41PM -0800, Kenneth Westfield wrote:
> +#define __LPASS_H__
> +
> +#define LPASS_AHBIX_CLOCK_FREQUENCY 131072
> +
> +/* Both the CPU DAI driver and platform driver will access this data */
> +struct lpass_data {
> +
> + /* clocks inside the low-power audio subsystem (LPASS) domain */
> + struct clk *ahbix_clk;
This uses struct clk so it needs at least a forward declaration of it if
not just a straight inclusion of linux/clk.h. There's several other
types and annotations that are referenced without an include to ensure
the compiler knows about them, the general idea is to avoid implicit
dependencies and thee surprising build breaks they cause.
signature.asc
Description: Digital signature
Re: [Patch V4 07/10] ASoC: ipq806x: Add LPASS platform driver
On Thu, Feb 05, 2015 at 12:53:43PM -0800, Kenneth Westfield wrote:
> + irqreturn_t ret = IRQ_NONE;
> +
> + interrupts = ioread32(drvdata->lpaif + status_offset) & chan_bitmask;
Elsewhere we're using regmap... why not here and how does this play
with regmap?
> + if (likely(interrupts & period_bitmask)) {
In general it's better not to use likely() and unlikely() annotations
unless you've got some evidence that they actually improve things (which
should be explained somewhere). They make the code that bit noisier and
there's some potential for them to cause the compiler to do unhelpful
things.
> +static int lpass_platform_alloc_buffer(struct snd_pcm_substream *substream,
> + struct snd_soc_pcm_runtime *soc_runtime)
> +{
> + struct snd_dma_buffer *buf = &substream->dma_buffer;
> + struct lpass_data *drvdata =
> + snd_soc_platform_get_drvdata(soc_runtime->platform);
> +
> + if (atomic_cmpxchg(&drvdata->lpm_lock, 0, 1)) {
cmpxchg() and atomics are both among the more error prone
synchronization primitives the kernel has. In cases where it is
appropriate to use them it is very important that the code be entirely
clear about what is going on so that not only are we clear that the
concurrency handling is safe but we can also modify the code safely in
the future.
This driver has no documentation at all for this variable so we've at
least got a problem with the clarity side here. Looking at the code
it's not entirely clear to me wat exactly is being protected or why
we're not using a simpler mechanism like variable protected by a mutex.
> + return devm_snd_soc_register_platform(&pdev->dev,
> + &lpass_platform_driver);
> +}
> +EXPORT_SYMBOL(asoc_qcom_lpass_platform_register);
ASoC APIs are all exported EXPORT_SYMBOL_GPL, their users should be too.
signature.asc
Description: Digital signature
Re: [Patch V4 03/10] ASoC: qcom: Document LPASS CPU bindings
On Thu, Feb 05, 2015 at 12:53:39PM -0800, Kenneth Westfield wrote: > +- qcom,system-clock-shift: Add this bool property if the default > + frequency of the system clock needs to > + be reduced. > +- qcom,system-clock-shift-compare: A numerical value used to right-shift > + the default system clock frequency for > + comparison with the target bit clock > + frequency. > +- qcom,system-clock-shift-amount : A numerical value used to right-shift > + the default system clock frequency. > +- qcom,alternate-sysclk : Add this bool property if the > default > + frequency of the system clock cannot > + divide down to the target bit clock > + frequency. > +- qcom,alternate-sysclk-bitwidth : A numerical value representing the > + sample bitwidth which requires use of > + the alternate system clock frequency. > +- qcom,alternate-sysclk-frequency: A numerical value representing the new > + system clock frequency to use. None of these seem like they are appropriate for device tree properties, they appear to be choosing a specific clocking configuration which is something that would normally be done as part of the system integration in the machine driver rather than in the DAI driver. This binding won't work in cases where the clocks are being changed at runtime and would limit systems where that becomes possible in future. Further, the interface seems too low level - it's specifying individual dividers and so on which would normally be things that can trivially be calculated or inferred given the input and target clock rates. signature.asc Description: Digital signature
Re: [PATCH 7/7] spi: spi-fsl-dspi: Add support for TCFQ transfer mode
On Thu, Jan 29, 2015 at 07:10:31PM +0100, Stefan Agner wrote: > It would be interesting to know what the authors original motivation > implementing this feature was. Reading the email of the original > patchset indicates that there are platforms where only TCF is supported: > > For adopting of different platform, either of them is a way of DSPI > transfer data. > > However, I don't know which platform that would be. Also, it seems that > Chao Fu's email is not valid anymore. It's not clear to me if the above means that this is due to hardware differences or due to tuning for the platform. signature.asc Description: Digital signature
Re: [Patch V4 08/10] ASoC: qcom: Add ability to build QCOM drivers
On Thu, Feb 05, 2015 at 12:53:44PM -0800, Kenneth Westfield wrote: > +config SND_SOC_IPQ806X > + tristate "SoC Audio support for IPQ806x based platforms" > + depends on SND_SOC_QCOM || ARCH_QCOM || COMPILE_TEST > +select SND_SIMPLE_CARD > + select SND_SOC_LPASS_CPU > + select SND_SOC_LPASS_PLATFORM > +select SND_SOC_MAX98357A No, we don't have blocks like this selecting simple-card for any other platforms using simple-card so we shouldn't do that here either. Just let all the drivers be individually selectable. You've also got a mix of tabs and spaces going on. signature.asc Description: Digital signature
Re: [Patch V4 02/10] ASoC: max98357a: Document MAX98357A bindings
On Thu, Feb 05, 2015 at 12:53:38PM -0800, Kenneth Westfield wrote: > From: Kenneth Westfield > > Add documentation to the sound directory of the > device-tree bindings for the Maxim MAX98357A audio > DAC. Applied, thanks. signature.asc Description: Digital signature
Re: [Patch V4 06/10] ASoC: ipq806x: Add LPASS CPU DAI driver
On Thu, Feb 05, 2015 at 12:53:42PM -0800, Kenneth Westfield wrote:
> + int bitwidth, ret;
> + bitwidth = snd_pcm_format_width(format);
> + if (bitwidth < 0) {
> + dev_err(dai->dev, "%s() invalid bit width given\n", __func__);
Print the error code.
> + regval = 0;
> + regval |= LPAIF_I2SCTL_LOOPBACK_DISABLE;
> + regval |= LPAIF_I2SCTL_WSSRC_INTERNAL;
Why not just write a single assignment statement?
> + default:
> + dev_err(dai->dev, "%s() invalid bitwidth given: %u\n",
> + __func__, bitwidth);
bitwidth is a signed type but you are using an unsigned format specifier
here.
> + reg = LPAIF_I2SCTL_REG(LPAIF_I2S_PORT_MI2S);
> + mask = LPAIF_I2SCTL_SPKEN_MASK;
> + val = LPAIF_I2SCTL_SPKEN_ENABLE;
> + ret = regmap_update_bits(drvdata->lpaif_map, reg, mask, val);
None of these intermediate variables seem to be doing a lot, why not
just specify the constants directly as arguments (that's the more normal
style)? A similar thing applies in several other places in this file.
> +#ifdef CONFIG_OF
> +static const struct of_device_id lpass_cpu_device_id[] = {
> + {.compatible = "qcom," DRV_NAME},
> + {}
> +};
> +#endif
Using DRV_NAME in the compatible like this makes it impossible to grep
for the driver which isn't helpful. In general I prefer not to use
DRV_NAME at all (exactly how often do we change the driver name?) but in
this case it's actively harmful.
signature.asc
Description: Digital signature
Re: [Patch V4 04/10] ASoC: codec: Add MAX98357A codec driver
On Thu, Feb 05, 2015 at 12:53:40PM -0800, Kenneth Westfield wrote: > From: Kenneth Westfield > > Add codec driver for the Maxim MAX98357A DAC. Applied, thanks. signature.asc Description: Digital signature
