* Re: [PATCH 2/2] ARM: dts: duovero-parlor: Add HDMI output
From: Tomi Valkeinen @ 2014-02-26 12:44 UTC (permalink / raw)
To: Russell King - ARM Linux
Cc: Laurent Pinchart, Sebastian Reichel, Javier Martinez Canillas,
Florian Vaussard, Benoît Cousson, Tony Lindgren,
devicetree@vger.kernel.org, linux-omap@vger.kernel.org,
Ash Charles, linux-arm-kernel@lists.infradead.org, Arnd Bergmann
In-Reply-To: <20140226120359.GA27282@n2100.arm.linux.org.uk>
[-- Attachment #1: Type: text/plain, Size: 4490 bytes --]
On 26/02/14 14:03, Russell King - ARM Linux wrote:
> On Wed, Feb 26, 2014 at 01:14:18PM +0200, Tomi Valkeinen wrote:
>> We have a bunch of panels and encoders used on OMAP boards, and we have
>> separate, omapdss specific, drivers for those. My plan is to continue
>> improving those drivers until they can be platform independent. This
>> would be the Common Display Framework that's been discussed (or a
>> precursor to it).
>
> I believe CDF has been knocked on the head.
I refuse to believe that we can't have common drivers for display
components. Maybe CDF as it's been proposed in its current form is not
good (although I haven't seen any explanation why), we need something
like it. So the "CDF" I speak of is not any particular set of patches
already sent, but a framework that would allow us to have generic
display drivers.
I'd be very glad if someone can point me to the discussions where CDF
has been knocked on the head.
> Also - DRM is not going to ever support hotplugging components - this
> was discussed at kernel summit last year and David Airlie was quite
Ok. Very odd stance. Maybe there's a reason for it that I just don't see.
> adamant about that. So, any "framework" which forces hotplugging of
> components on subsystems isn't going to fly.
CDF doesn't force hotplugging.
Although without hotplugging (hot-unplug not needed), or at least some
minimal form of it, the system is a bit crippled. Leave one kernel
module out, or have one driver probe fail, the whole display subsystem
fails, even if some display pipelines would work fine.
On OMAP4 SDP board, for example, this would mean that if the user
doesn't compile PicoDLP driver, or the driver fails to probe, the two
LCDs and the analog TV out would also fail. Many of the boards don't
even have a PicoDLP module installed, so a fail there somewhere is
guaranteed.
> This is why we now have the component helpers in the driver model -
> to allow devices to be collected together into one logical subsystem
> group, and bound/unbound as a group.
Yep, it's a good start. The component helpers could well be used with CDF.
But if I'm not mistaken, it suffers from the problems above, when there
are multiple independent pipelines (simultaneous or non-simultaneous)
handled by the same IPs.
And, while I may be mistaken, it sounds that the component helpers leave
mostly everything up to the display drivers. Everyone devising their own
way to describe the hardware in DT, and the connections between the
components. Of course, the core component system shouldn't define
anything DT related, as it doesn't. But that part is still needed, which
is where CDF comes in.
In my opinion, the component helpers or similar would be used with CDF
in the beginning, because DRM doesn't support hotplug. Eventually we
should get some kind of basic hotplug support, so that we could add
display pipelines when they are ready.
I need to ask Dave why he is so strongly opposed to hotplugging components.
> Remember that "hotplugging" in this context does not mean that the
> user can physically do something with the hardware. It means that
> they're separate devices which can be probed/removed at will. Every
> device in Linux can be bound and unbound from its driver at any time
> by userspace, and that is something which is expected to be handled
> gracefully.
Hmm, sorry, can you rephrase?
My use of hotplug in this context means roughly "add a new display, and
whatever is related to that, when the drivers required have been probed".
So with hotplug, a new fbdev or a combination of drm crtcs, encoders,
etc, could appear even after the initial probe of the display controller.
But all this is, I think, a bit aside the original point. The original
point was about DT bindings. What kind of framework we have in the
kernel side to handle the bindings is an interesting and very important
topic, but they are not strictly tied together.
Even if CDF is the worst thing ever, and needs to die quickly, I think
the proposed DT bindings are still valid. They describe the hardware as
precisely and as future-proofly as we've been able to come up with.
People can use component helpers with them if they see that's a good
approach. Or if on some beautiful day we get an agreement on CDF or
something similar, and we can support hotplugging components, we already
have proper DT bindings for it.
Tomi
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 901 bytes --]
^ permalink raw reply
* RE: [PATCH v4] can: xilinx CAN controller support.
From: Appana Durga Kedareswara Rao @ 2014-02-26 13:07 UTC (permalink / raw)
To: Marc Kleine-Budde, wg@grandegger.com, Michal Simek,
grant.likely@linaro.org, robh+dt@kernel.org,
linux-can@vger.kernel.org
Cc: netdev@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
linux-kernel@vger.kernel.org, devicetree@vger.kernel.org
In-Reply-To: <530D0A9C.5090402@pengutronix.de>
Hi Marc,
> -----Original Message-----
> From: Marc Kleine-Budde [mailto:mkl@pengutronix.de]
> Sent: Wednesday, February 26, 2014 2:57 AM
> To: Appana Durga Kedareswara Rao; wg@grandegger.com; Michal Simek;
> grant.likely@linaro.org; robh+dt@kernel.org; linux-can@vger.kernel.org
> Cc: netdev@vger.kernel.org; linux-arm-kernel@lists.infradead.org; linux-
> kernel@vger.kernel.org; devicetree@vger.kernel.org; Appana Durga
> Kedareswara Rao
> Subject: Re: [PATCH v4] can: xilinx CAN controller support.
>
> On 02/24/2014 05:25 AM, Kedareswara rao Appana wrote:
> > This patch adds xilinx CAN controller support.
> > This driver supports both ZYNQ CANPS and Soft IP AXI CAN controller.
> >
> > Signed-off-by: Kedareswara rao Appana <appanad@xilinx.com>
>
> Looks quite good. Just the tx_complete routine is a bit fishy. Comments
> inline.
>
> > ---
> > This patch is rebased on the 3.14 rc4 kernel.
> > Changes for v4:
> > - Added check for the tx fifo full interrupt condition in
> > Tx interrupt routine.
> > - Added be io helper functions.
>
> Which combinations of endianess have you tested (Kernel/Hardware)?
>
I tested with the little endian versions only(kernel and Hardware)
> > - Moved the clock enable/disable to probe/remove because of
> > Added big endian support for AXI CAN controller case(reading
> > a register during the probe for that we need to enable clock).
>
> Just disable the clock in the end of probe(). See inline comments
>
Ok
> > Changes for v3:
> > - Updated the driver with review comments.
> > - Modified the transmit logic as per Marc suggestion.
> > - Enabling the clocks when the interface is up to reduce the
> > Power consumption.
> > Changes for v2:
> > - Updated with the review comments.
> > - Removed the unnecessary debug prints.
> > - include tx,rx fifo depths in ZYNQ CANPS case also.
> > ---
> > .../devicetree/bindings/net/can/xilinx_can.txt | 45 +
> > drivers/net/can/Kconfig | 7 +
> > drivers/net/can/Makefile | 1 +
> > drivers/net/can/xilinx_can.c | 1162 ++++++++++++++++++++
> > 4 files changed, 1215 insertions(+), 0 deletions(-) create mode
> > 100644 Documentation/devicetree/bindings/net/can/xilinx_can.txt
> > create mode 100644 drivers/net/can/xilinx_can.c
> >
> > diff --git a/Documentation/devicetree/bindings/net/can/xilinx_can.txt
> > b/Documentation/devicetree/bindings/net/can/xilinx_can.txt
> > new file mode 100644
> > index 0000000..0e57103
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/net/can/xilinx_can.txt
> > @@ -0,0 +1,45 @@
> > +Xilinx Axi CAN/Zynq CANPS controller Device Tree Bindings
> > +---------------------------------------------------------
> > +
> > +Required properties:
> > +- compatible : Should be "xlnx,zynq-can-1.00.a" for Zynq
> CAN
> > + controllers and "xlnx,axi-can-1.00.a" for Axi CAN
> > + controllers.
> > +- reg : Physical base address and size of the Axi CAN/Zynq
> > + CANPS registers map.
> > +- interrupts : Property with a value describing the interrupt
> > + number.
> > +- interrupt-parent : Must be core interrupt controller
> > +- clock-names : List of input clock names - "ref_clk",
> "aper_clk"
> > + (See clock bindings for details. Two clocks are
> > + required for Zynq CAN. For Axi CAN
> > + case it is one(ref_clk)).
> > +- clocks : Clock phandles (see clock bindings for details).
> > +- tx-fifo-depth : Can Tx fifo depth.
> > +- rx-fifo-depth : Can Rx fifo depth.
> > +
> > +
> > +Example:
> > +
> > +For Zynq CANPS Dts file:
> > + zynq_can_0: zynq-can@e0008000 {
> > + compatible = "xlnx,zynq-can-1.00.a";
> > + clocks = <&clkc 19>, <&clkc 36>;
> > + clock-names = "ref_clk", "aper_clk";
> > + reg = <0xe0008000 0x1000>;
> > + interrupts = <0 28 4>;
> > + interrupt-parent = <&intc>;
> > + tx-fifo-depth = <0x40>;
> > + rx-fifo-depth = <0x40>;
> > + };
> > +For Axi CAN Dts file:
> > + axi_can_0: axi-can@40000000 {
> > + compatible = "xlnx,axi-can-1.00.a";
> > + clocks = <&clkc 0>;
> > + clock-names = "ref_clk" ;
> > + reg = <0x40000000 0x10000>;
> > + interrupt-parent = <&intc>;
> > + interrupts = <0 59 1>;
> > + tx-fifo-depth = <0x40>;
> > + rx-fifo-depth = <0x40>;
> > + };
> > diff --git a/drivers/net/can/Kconfig b/drivers/net/can/Kconfig index
> > 9e7d95d..b180239 100644
> > --- a/drivers/net/can/Kconfig
> > +++ b/drivers/net/can/Kconfig
> > @@ -125,6 +125,13 @@ config CAN_GRCAN
> > endian syntheses of the cores would need some modifications on
> > the hardware level to work.
> >
> > +config CAN_XILINXCAN
> > + tristate "Xilinx CAN"
> > + depends on ARCH_ZYNQ || MICROBLAZE
> > + ---help---
> > + Xilinx CAN driver. This driver supports both soft AXI CAN IP and
> > + Zynq CANPS IP.
> > +
> > source "drivers/net/can/mscan/Kconfig"
> >
> > source "drivers/net/can/sja1000/Kconfig"
> > diff --git a/drivers/net/can/Makefile b/drivers/net/can/Makefile index
> > c744039..0b8e11e 100644
> > --- a/drivers/net/can/Makefile
> > +++ b/drivers/net/can/Makefile
> > @@ -25,5 +25,6 @@ obj-$(CONFIG_CAN_JANZ_ICAN3) += janz-
> ican3.o
> > obj-$(CONFIG_CAN_FLEXCAN) += flexcan.o
> > obj-$(CONFIG_PCH_CAN) += pch_can.o
> > obj-$(CONFIG_CAN_GRCAN) += grcan.o
> > +obj-$(CONFIG_CAN_XILINXCAN) += xilinx_can.o
> >
> > ccflags-$(CONFIG_CAN_DEBUG_DEVICES) := -DDEBUG diff --git
> > a/drivers/net/can/xilinx_can.c b/drivers/net/can/xilinx_can.c new file
> > mode 100644 index 0000000..7564bef
> > --- /dev/null
> > +++ b/drivers/net/can/xilinx_can.c
> > @@ -0,0 +1,1162 @@
> > +/* Xilinx CAN device driver
> > + *
> > + * Copyright (C) 2012 - 2014 Xilinx, Inc.
> > + * Copyright (C) 2009 PetaLogix. All rights reserved.
> > + *
> > + * Description:
> > + * This driver is developed for Axi CAN IP and for Zynq CANPS Controller.
> > + * 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, either version 2 of the License, or
> > + * (at your option) any later version.
> > + *
> > + * This program is distributed in the hope that it will be useful,
> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> > + * GNU General Public License for more details.
> > + *
> > + * You should have received a copy of the GNU General Public License
> > + * along with this program. If not, see <http://www.gnu.org/licenses/>.
> > + */
> > +
> > +#include <linux/clk.h>
> > +#include <linux/errno.h>
> > +#include <linux/init.h>
> > +#include <linux/interrupt.h>
> > +#include <linux/io.h>
> > +#include <linux/kernel.h>
> > +#include <linux/module.h>
> > +#include <linux/netdevice.h>
> > +#include <linux/of.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/skbuff.h>
> > +#include <linux/string.h>
> > +#include <linux/types.h>
> > +#include <linux/can/dev.h>
> > +#include <linux/can/error.h>
> > +#include <linux/can/led.h>
> > +
> > +#define DRIVER_NAME "XILINX_CAN"
> > +
> > +/* CAN registers set */
> > +enum xcan_reg {
> > + XCAN_SRR_OFFSET = 0x00, /* Software reset */
> > + XCAN_MSR_OFFSET = 0x04, /* Mode select */
> > + XCAN_BRPR_OFFSET = 0x08, /* Baud rate prescaler */
> > + XCAN_BTR_OFFSET = 0x0C, /* Bit timing */
> > + XCAN_ECR_OFFSET = 0x10, /* Error counter */
> > + XCAN_ESR_OFFSET = 0x14, /* Error status */
> > + XCAN_SR_OFFSET = 0x18, /* Status */
> > + XCAN_ISR_OFFSET = 0x1C, /* Interrupt status */
> > + XCAN_IER_OFFSET = 0x20, /* Interrupt enable */
> > + XCAN_ICR_OFFSET = 0x24, /* Interrupt clear */
> > + XCAN_TXFIFO_ID_OFFSET = 0x30,/* TX FIFO ID */
> > + XCAN_TXFIFO_DLC_OFFSET = 0x34, /* TX FIFO DLC */
> > + XCAN_TXFIFO_DW1_OFFSET = 0x38, /* TX FIFO Data Word 1 */
> > + XCAN_TXFIFO_DW2_OFFSET = 0x3C, /* TX FIFO Data Word 2 */
> > + XCAN_RXFIFO_ID_OFFSET = 0x50, /* RX FIFO ID */
> > + XCAN_RXFIFO_DLC_OFFSET = 0x54, /* RX FIFO DLC */
> > + XCAN_RXFIFO_DW1_OFFSET = 0x58, /* RX FIFO Data Word 1 */
> > + XCAN_RXFIFO_DW2_OFFSET = 0x5C, /* RX FIFO Data Word 2 */
> > +};
> > +
> > +/* CAN register bit masks - XCAN_<REG>_<BIT>_MASK */
> > +#define XCAN_SRR_CEN_MASK 0x00000002 /* CAN enable */
> > +#define XCAN_SRR_RESET_MASK 0x00000001 /* Soft Reset the
> CAN core */
> > +#define XCAN_MSR_LBACK_MASK 0x00000002 /* Loop back
> mode select */
> > +#define XCAN_MSR_SLEEP_MASK 0x00000001 /* Sleep mode
> select */
> > +#define XCAN_BRPR_BRP_MASK 0x000000FF /* Baud rate
> prescaler */
> > +#define XCAN_BTR_SJW_MASK 0x00000180 /* Synchronous
> jump width */
> > +#define XCAN_BTR_TS2_MASK 0x00000070 /* Time segment
> 2 */
> > +#define XCAN_BTR_TS1_MASK 0x0000000F /* Time segment
> 1 */
> > +#define XCAN_ECR_REC_MASK 0x0000FF00 /* Receive error
> counter */
> > +#define XCAN_ECR_TEC_MASK 0x000000FF /* Transmit error
> counter */
> > +#define XCAN_ESR_ACKER_MASK 0x00000010 /* ACK error */
> > +#define XCAN_ESR_BERR_MASK 0x00000008 /* Bit error */
> > +#define XCAN_ESR_STER_MASK 0x00000004 /* Stuff error */
> > +#define XCAN_ESR_FMER_MASK 0x00000002 /* Form error */
> > +#define XCAN_ESR_CRCER_MASK 0x00000001 /* CRC error */
> > +#define XCAN_SR_TXFLL_MASK 0x00000400 /* TX FIFO is full
> */
> > +#define XCAN_SR_ESTAT_MASK 0x00000180 /* Error status */
> > +#define XCAN_SR_ERRWRN_MASK 0x00000040 /* Error warning
> */
> > +#define XCAN_SR_NORMAL_MASK 0x00000008 /* Normal mode
> */
> > +#define XCAN_SR_LBACK_MASK 0x00000002 /* Loop back
> mode */
> > +#define XCAN_SR_CONFIG_MASK 0x00000001 /* Configuration
> mode */
> > +#define XCAN_IXR_TXFEMP_MASK 0x00004000 /* TX FIFO Empty
> */
> > +#define XCAN_IXR_WKUP_MASK 0x00000800 /* Wake up
> interrupt */
> > +#define XCAN_IXR_SLP_MASK 0x00000400 /* Sleep
> interrupt */
> > +#define XCAN_IXR_BSOFF_MASK 0x00000200 /* Bus off
> interrupt */
> > +#define XCAN_IXR_ERROR_MASK 0x00000100 /* Error interrupt
> */
> > +#define XCAN_IXR_RXNEMP_MASK 0x00000080 /* RX FIFO
> NotEmpty intr */
> > +#define XCAN_IXR_RXOFLW_MASK 0x00000040 /* RX FIFO
> Overflow intr */
> > +#define XCAN_IXR_RXOK_MASK 0x00000010 /* Message
> received intr */
> > +#define XCAN_IXR_TXFLL_MASK 0x00000004 /* Tx FIFO Full
> intr */
> > +#define XCAN_IXR_TXOK_MASK 0x00000002 /* TX successful
> intr */
> > +#define XCAN_IXR_ARBLST_MASK 0x00000001 /* Arbitration
> lost intr */
> > +#define XCAN_IDR_ID1_MASK 0xFFE00000 /* Standard msg
> identifier */
> > +#define XCAN_IDR_SRR_MASK 0x00100000 /* Substitute
> remote TXreq */
> > +#define XCAN_IDR_IDE_MASK 0x00080000 /* Identifier
> extension */
> > +#define XCAN_IDR_ID2_MASK 0x0007FFFE /* Extended
> message ident */
> > +#define XCAN_IDR_RTR_MASK 0x00000001 /* Remote TX
> request */
> > +#define XCAN_DLCR_DLC_MASK 0xF0000000 /* Data length
> code */
> > +
> > +#define XCAN_INTR_ALL (XCAN_IXR_TXOK_MASK |
> XCAN_IXR_BSOFF_MASK |\
> > + XCAN_IXR_WKUP_MASK |
> XCAN_IXR_SLP_MASK | \
> > + XCAN_IXR_RXNEMP_MASK |
> XCAN_IXR_ERROR_MASK | \
> > + XCAN_IXR_ARBLST_MASK |
> XCAN_IXR_RXOK_MASK)
> > +
> > +/* CAN register bit shift - XCAN_<REG>_<BIT>_SHIFT */
> > +#define XCAN_BTR_SJW_SHIFT 7 /* Synchronous jump width
> */
> > +#define XCAN_BTR_TS2_SHIFT 4 /* Time segment 2 */
> > +#define XCAN_IDR_ID1_SHIFT 21 /* Standard Messg
> Identifier */
> > +#define XCAN_IDR_ID2_SHIFT 1 /* Extended Message
> Identifier */
> > +#define XCAN_DLCR_DLC_SHIFT 28 /* Data length code */
> > +#define XCAN_ESR_REC_SHIFT 8 /* Rx Error Count */
> > +
> > +/* CAN frame length constants */
> > +#define XCAN_ECHO_SKB_MAX 64
> > +#define XCAN_FRAME_MAX_DATA_LEN 8
> > +#define XCAN_TIMEOUT (1 * HZ)
> > +
> > +/**
> > + * struct xcan_priv - This definition define CAN driver instance
> > + * @can: CAN private data structure.
> > + * @tx_head: Tx CAN packets ready to send on the
> queue
> > + * @tx_tail: Tx CAN packets successfully sended on the
> queue
> > + * @xcan_echo_skb_max_tx: Maximum number packets the driver
> can send
> > + * @xcan_echo_skb_max_rx: Maximum number packets the driver
> can receive
> > + * @napi: NAPI structure
> > + * @read_reg: For reading data from CAN registers
> > + * @write_reg: For writing data to CAN registers
> > + * @dev: Network device data structure
> > + * @reg_base: Ioremapped address to registers
> > + * @irq_flags: For request_irq()
> > + * @aperclk: Pointer to struct clk
> > + * @devclk: Pointer to struct clk
> > + */
> > +struct xcan_priv {
> > + struct can_priv can;
> > + unsigned int tx_head;
> > + unsigned int tx_tail;
> > + u32 xcan_echo_skb_max_tx;
>
> To make the nameing more uniform, you can rename it into "tx_max".
>
Ok
> > + u32 xcan_echo_skb_max_rx;
>
> I think the _rx value is only needed inside the probe function, right?
> Then you can remove it from the priv struct.
>
Yes it is only need in the probe.
Will remove.
> > + struct napi_struct napi;
> > + u32 (*read_reg)(const struct xcan_priv *priv, enum xcan_reg reg);
> > + void (*write_reg)(const struct xcan_priv *priv, enum xcan_reg reg,
> > + u32 val);
> > + struct net_device *dev;
> > + void __iomem *reg_base;
> > + unsigned long irq_flags;
> > + struct clk *aperclk;
> > + struct clk *devclk;
> > +};
> > +
> > +/* CAN Bittiming constants as per Xilinx CAN specs */ static const
> > +struct can_bittiming_const xcan_bittiming_const = {
> > + .name = DRIVER_NAME,
> > + .tseg1_min = 1,
> > + .tseg1_max = 16,
> > + .tseg2_min = 1,
> > + .tseg2_max = 8,
> > + .sjw_max = 4,
> > + .brp_min = 1,
> > + .brp_max = 256,
> > + .brp_inc = 1,
> > +};
> > +
> > +/**
> > + * xcan_write_reg_le - Write a value to the device register little endian
> > + * @priv: Driver private data structure
> > + * @reg: Register offset
> > + * @val: Value to write at the Register offset
> > + *
> > + * Write data to the paricular CAN register */ static void
> > +xcan_write_reg_le(const struct xcan_priv *priv, enum xcan_reg reg,
> > + u32 val)
> > +{
> > + iowrite32(val, priv->reg_base + reg); }
> > +
> > +/**
> > + * xcan_read_reg_le - Read a value from the device register little endian
> > + * @priv: Driver private data structure
> > + * @reg: Register offset
> > + *
> > + * Read data from the particular CAN register
> > + * Return: value read from the CAN register */ static u32
> > +xcan_read_reg_le(const struct xcan_priv *priv, enum xcan_reg reg) {
> > + return ioread32(priv->reg_base + reg); }
> > +
> > +/**
> > + * xcan_write_reg_be - Write a value to the device register big endian
> > + * @priv: Driver private data structure
> > + * @reg: Register offset
> > + * @val: Value to write at the Register offset
> > + *
> > + * Write data to the paricular CAN register */ static void
> > +xcan_write_reg_be(const struct xcan_priv *priv, enum xcan_reg reg,
> > + u32 val)
> > +{
> > + iowrite32be(val, priv->reg_base + reg); }
> > +
> > +/**
> > + * xcan_read_reg_be - Read a value from the device register big endian
> > + * @priv: Driver private data structure
> > + * @reg: Register offset
> > + *
> p> + * Read data from the particular CAN register
> > + * Return: value read from the CAN register */ static u32
> > +xcan_read_reg_be(const struct xcan_priv *priv, enum xcan_reg reg) {
> > + return ioread32be(priv->reg_base + reg); }
> > +
> > +/**
> > + * set_reset_mode - Resets the CAN device mode
> > + * @ndev: Pointer to net_device structure
> > + *
> > + * This is the driver reset mode routine.The driver
> > + * enters into configuration mode.
> > + *
> > + * Return: 0 on success and failure value on error */ static int
> > +set_reset_mode(struct net_device *ndev) {
> > + struct xcan_priv *priv = netdev_priv(ndev);
> > + unsigned long timeout;
> > +
> > + priv->can.state = CAN_STATE_STOPPED;
> > +
> > + timeout = jiffies + XCAN_TIMEOUT;
> > + while (!(priv->read_reg(priv, XCAN_SR_OFFSET) &
> XCAN_SR_CONFIG_MASK)) {
> > + if (time_after(jiffies, timeout)) {
> > + netdev_warn(ndev, "timedout waiting for config
> mode\n");
> > + return -ETIMEDOUT;
> > + }
> > + usleep_range(500, 10000);
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +/**
> > + * xcan_set_bittiming - CAN set bit timing routine
> > + * @ndev: Pointer to net_device structure
> > + *
> > + * This is the driver set bittiming routine.
> > + * Return: 0 on success and failure value on error */ static int
> > +xcan_set_bittiming(struct net_device *ndev) {
> > + struct xcan_priv *priv = netdev_priv(ndev);
> > + struct can_bittiming *bt = &priv->can.bittiming;
> > + u32 btr0, btr1;
> > + u32 is_config_mode;
> > +
> > + /* Check whether Xilinx CAN is in configuration mode.
> > + * It cannot set bit timing if Xilinx CAN is not in configuration mode.
> > + */
> > + is_config_mode = priv->read_reg(priv, XCAN_SR_OFFSET) &
> > + XCAN_SR_CONFIG_MASK;
> > + if (!is_config_mode) {
> > + netdev_alert(ndev,
> > + "Cannot set bittiming can is not in config mode\n");
> > + return -EPERM;
> > + }
> > +
> > + /* Setting Baud Rate prescalar value in BRPR Register */
> > + btr0 = (bt->brp - 1) & XCAN_BRPR_BRP_MASK;
> > +
> > + /* Setting Time Segment 1 in BTR Register */
> > + btr1 = (bt->prop_seg + bt->phase_seg1 - 1) & XCAN_BTR_TS1_MASK;
>
> Both masks should not be needed as the values are inside the range you
> specified. If you cut the values, the bit timing will not be correct anyway.
>
Ok
> > +
> > + /* Setting Time Segment 2 in BTR Register */
> > + btr1 |= (bt->phase_seg2 - 1) << XCAN_BTR_TS2_SHIFT;
> > +
> > + /* Setting Synchronous jump width in BTR Register */
> > + btr1 |= (bt->sjw - 1) << XCAN_BTR_SJW_SHIFT;
> > +
> > + priv->write_reg(priv, XCAN_BRPR_OFFSET, btr0);
> > + priv->write_reg(priv, XCAN_BTR_OFFSET, btr1);
> > +
> > + netdev_dbg(ndev, "BRPR=0x%08x, BTR=0x%08x\n",
> > + priv->read_reg(priv, XCAN_BRPR_OFFSET),
> > + priv->read_reg(priv, XCAN_BTR_OFFSET));
> > +
> > + return 0;
> > +}
> > +
> > +/**
> > + * xcan_chip_start - This the drivers start routine
> > + * @ndev: Pointer to net_device structure
> > + *
> > + * This is the drivers start routine.
> > + * Based on the State of the CAN device it puts
> > + * the CAN device into a proper mode.
> > + *
> > + * Return: 0 on success and failure value on error */ static int
> > +xcan_chip_start(struct net_device *ndev) {
> > + struct xcan_priv *priv = netdev_priv(ndev);
> > + u32 err;
> > + unsigned long timeout;
> > +
> > + /* Check if it is in reset mode */
> > + err = set_reset_mode(ndev);
> > + if (err < 0)
> > + return err;
> > +
> > + /* Enable interrupts */
> > + priv->write_reg(priv, XCAN_IER_OFFSET, XCAN_INTR_ALL);
> > +
> > + /* Check whether it is loopback mode or normal mode */
> > + if (priv->can.ctrlmode & CAN_CTRLMODE_LOOPBACK)
> > + /* Put device into loopback mode */
> > + priv->write_reg(priv, XCAN_MSR_OFFSET,
> XCAN_MSR_LBACK_MASK);
> > + else
> > + /* The device is in normal mode */
> > + priv->write_reg(priv, XCAN_MSR_OFFSET, 0);
> > +
> > + if (priv->can.state == CAN_STATE_STOPPED) {
> > + /* Enable Xilinx CAN */
> > + priv->write_reg(priv, XCAN_SRR_OFFSET,
> XCAN_SRR_CEN_MASK);
> > + priv->can.state = CAN_STATE_ERROR_ACTIVE;
> > + timeout = jiffies + XCAN_TIMEOUT;
> > + if (priv->can.ctrlmode & CAN_CTRLMODE_LOOPBACK) {
> > + while ((priv->read_reg(priv, XCAN_SR_OFFSET)
> > + & XCAN_SR_LBACK_MASK) == 0) {
> > + if (time_after(jiffies, timeout)) {
> > + netdev_warn(ndev,
> > + "timedout for loopback
> mode\n");
> > + return -ETIMEDOUT;
> > + }
> > + usleep_range(500, 10000);
> > + }
> > + } else {
> > + while ((priv->read_reg(priv, XCAN_SR_OFFSET)
> > + & XCAN_SR_NORMAL_MASK) == 0) {
> > + if (time_after(jiffies, timeout)) {
> > + netdev_warn(ndev,
> > + "timedout for normal
> mode\n");
> > + return -ETIMEDOUT;
> > + }
> > + usleep_range(500, 10000);
> > + }
> > + }
> > + netdev_dbg(ndev, "status:#x%08x\n",
> > + priv->read_reg(priv, XCAN_SR_OFFSET));
> > + }
> > + priv->can.state = CAN_STATE_ERROR_ACTIVE;
> > + return 0;
> > +}
> > +
> > +/**
> > + * xcan_do_set_mode - This sets the mode of the driver
> > + * @ndev: Pointer to net_device structure
> > + * @mode: Tells the mode of the driver
> > + *
> > + * This check the drivers state and calls the
> > + * the corresponding modes to set.
> > + *
> > + * Return: 0 on success and failure value on error */ static int
> > +xcan_do_set_mode(struct net_device *ndev, enum can_mode mode) {
> > + int ret;
> > +
> > + switch (mode) {
> > + case CAN_MODE_START:
> > + ret = xcan_chip_start(ndev);
> > + if (ret < 0)
> > + netdev_err(ndev, "xcan_chip_start failed!\n");
> > + netif_wake_queue(ndev);
> > + break;
> > + default:
> > + ret = -EOPNOTSUPP;
> > + break;
> > + }
> > +
> > + return ret;
> > +}
> > +
> > +/**
> > + * xcan_start_xmit - Starts the transmission
> > + * @skb: sk_buff pointer that contains data to be Txed
> > + * @ndev: Pointer to net_device structure
> > + *
> > + * This function is invoked from upper layers to initiate
> > +transmission. This
> > + * function uses the next available free txbuff and populates their
> > +fields to
> > + * start the transmission.
> > + *
> > + * Return: 0 on success and failure value on error */ static int
> > +xcan_start_xmit(struct sk_buff *skb, struct net_device *ndev) {
> > + struct xcan_priv *priv = netdev_priv(ndev);
> > + struct net_device_stats *stats = &ndev->stats;
> > + struct can_frame *cf = (struct can_frame *)skb->data;
> > + u32 id, dlc, data[2] = {0, 0};
> > +
> > + if (can_dropped_invalid_skb(ndev, skb))
> > + return NETDEV_TX_OK;
> > +
> > + /* Check if the TX buffer is full */
> > + if (unlikely(priv->read_reg(priv, XCAN_SR_OFFSET) &
> > + XCAN_SR_TXFLL_MASK)) {
> > + netif_stop_queue(ndev);
> > + netdev_err(ndev, "BUG!, TX FIFO full when queue
> awake!\n");
> > + return NETDEV_TX_BUSY;
> > + }
> > +
> > + /* Watch carefully on the bit sequence */
> > + if (cf->can_id & CAN_EFF_FLAG) {
> > + /* Extended CAN ID format */
> > + id = ((cf->can_id & CAN_EFF_MASK) << XCAN_IDR_ID2_SHIFT)
> &
> > + XCAN_IDR_ID2_MASK;
> > + id |= (((cf->can_id & CAN_EFF_MASK) >>
> > + (CAN_EFF_ID_BITS-CAN_SFF_ID_BITS)) <<
> > + XCAN_IDR_ID1_SHIFT) & XCAN_IDR_ID1_MASK;
> > +
> > + /* The substibute remote TX request bit should be "1"
> > + * for extended frames as in the Xilinx CAN datasheet
> > + */
> > + id |= XCAN_IDR_IDE_MASK | XCAN_IDR_SRR_MASK;
> > +
> > + if (cf->can_id & CAN_RTR_FLAG)
> > + /* Extended frames remote TX request */
> > + id |= XCAN_IDR_RTR_MASK;
> > + } else {
> > + /* Standard CAN ID format */
> > + id = ((cf->can_id & CAN_SFF_MASK) << XCAN_IDR_ID1_SHIFT)
> &
> > + XCAN_IDR_ID1_MASK;
> > +
> > + if (cf->can_id & CAN_RTR_FLAG)
> > + /* Extended frames remote TX request */
>
> Copy/paste error in the coment
>
Ok will change
> > + id |= XCAN_IDR_SRR_MASK;
> > + }
> > +
> > + dlc = cf->can_dlc << XCAN_DLCR_DLC_SHIFT;
> > +
> > + if (cf->can_dlc > 0)
> > + data[0] = be32_to_cpup((__be32 *)(cf->data + 0));
> > + if (cf->can_dlc > 4)
> > + data[1] = be32_to_cpup((__be32 *)(cf->data + 4));
> > +
> > + can_put_echo_skb(skb, ndev, priv->tx_head % priv-
> >xcan_echo_skb_max_tx);
> > + priv->tx_head++;
> > +
> > + /* Write the Frame to Xilinx CAN TX FIFO */
> > + priv->write_reg(priv, XCAN_TXFIFO_ID_OFFSET, id);
> > + /* If the CAN frame is RTR frame this write triggers tranmission */
> > + priv->write_reg(priv, XCAN_TXFIFO_DLC_OFFSET, dlc);
> > + if (!(cf->can_id & CAN_RTR_FLAG)) {
> > + priv->write_reg(priv, XCAN_TXFIFO_DW1_OFFSET, data[0]);
> > + /* If the CAN frame is Standard/Extended frame this
> > + * write triggers tranmission
> > + */
> > + priv->write_reg(priv, XCAN_TXFIFO_DW2_OFFSET, data[1]);
> > + stats->tx_bytes += cf->can_dlc;
> > + }
> > +
> > + /* Check if the TX buffer is full */
> > + if ((priv->tx_head - priv->tx_tail) == priv->xcan_echo_skb_max_tx)
> > + netif_stop_queue(ndev);
> > +
> > + return NETDEV_TX_OK;
> > +}
> > +
> > +/**
> > + * xcan_rx - Is called from CAN isr to complete the received
> > + * frame processing
> > + * @ndev: Pointer to net_device structure
> > + *
> > + * This function is invoked from the CAN isr(poll) to process the Rx
> > +frames. It
> > + * does minimal processing and invokes "netif_receive_skb" to
> > +complete further
> > + * processing.
> > + * Return: 0 on success and negative error value on error */ static
> > +int xcan_rx(struct net_device *ndev) {
> > + struct xcan_priv *priv = netdev_priv(ndev);
> > + struct net_device_stats *stats = &ndev->stats;
> > + struct can_frame *cf;
> > + struct sk_buff *skb;
> > + u32 id_xcan, dlc, data[2] = {0, 0};
> > +
> > + skb = alloc_can_skb(ndev, &cf);
> > + if (!skb)
> > + return -ENOMEM;
> > +
> > + /* Read a frame from Xilinx zynq CANPS */
> > + id_xcan = priv->read_reg(priv, XCAN_RXFIFO_ID_OFFSET);
> > + dlc = priv->read_reg(priv, XCAN_RXFIFO_DLC_OFFSET) >>
> > + XCAN_DLCR_DLC_SHIFT;
> > +
> > + /* Change Xilinx CAN data length format to socketCAN data format
> */
> > + cf->can_dlc = get_can_dlc(dlc);
> > +
> > + /* Change Xilinx CAN ID format to socketCAN ID format */
> > + if (id_xcan & XCAN_IDR_IDE_MASK) {
> > + /* The received frame is an Extended format frame */
> > + cf->can_id = (id_xcan & XCAN_IDR_ID1_MASK) >> 3;
> > + cf->can_id |= (id_xcan & XCAN_IDR_ID2_MASK) >>
> > + XCAN_IDR_ID2_SHIFT;
> > + cf->can_id |= CAN_EFF_FLAG;
> > + if (id_xcan & XCAN_IDR_RTR_MASK)
> > + cf->can_id |= CAN_RTR_FLAG;
> > + } else {
> > + /* The received frame is a standard format frame */
> > + cf->can_id = (id_xcan & XCAN_IDR_ID1_MASK) >>
> > + XCAN_IDR_ID1_SHIFT;
> > + if (id_xcan & XCAN_IDR_RTR_MASK)
> > + cf->can_id |= CAN_RTR_FLAG;
> > + }
> > +
> > + if (!(id_xcan & XCAN_IDR_RTR_MASK)) {
> > + data[0] = priv->read_reg(priv, XCAN_RXFIFO_DW1_OFFSET);
> > + data[1] = priv->read_reg(priv, XCAN_RXFIFO_DW2_OFFSET);
> > +
> > + /* Change Xilinx CAN data format to socketCAN data format
> */
> > + *(__be32 *)(cf->data) = cpu_to_be32(data[0]);
> > + if (cf->can_dlc > 4)
> > + *(__be32 *)(cf->data + 4) = cpu_to_be32(data[1]);
> > + }
> > + can_led_event(ndev, CAN_LED_EVENT_RX);
> > +
> > + netif_receive_skb(skb);
> > +
> > + stats->rx_bytes += cf->can_dlc;
> > + stats->rx_packets++;
> > + return 0;
> > +}
> > +
> > +/**
> > + * xcan_err_interrupt - error frame Isr
> > + * @ndev: net_device pointer
> > + * @isr: interrupt status register value
> > + *
> > + * This is the CAN error interrupt and it will
> > + * check the the type of error and forward the error
> > + * frame to upper layers.
> > + */
> > +static void xcan_err_interrupt(struct net_device *ndev, u32 isr) {
> > + struct xcan_priv *priv = netdev_priv(ndev);
> > + struct net_device_stats *stats = &ndev->stats;
> > + struct can_frame *cf;
> > + struct sk_buff *skb;
> > + u32 err_status, status;
> > +
> > + skb = alloc_can_err_skb(ndev, &cf);
> > + if (!skb) {
> > + netdev_err(ndev, "alloc_can_err_skb() failed!\n");
> > + return;
> > + }
> > +
> > + err_status = priv->read_reg(priv, XCAN_ESR_OFFSET);
> > + priv->write_reg(priv, XCAN_ESR_OFFSET, err_status);
> > + status = priv->read_reg(priv, XCAN_SR_OFFSET);
> > +
> > + if (isr & XCAN_IXR_BSOFF_MASK) {
> > + priv->can.state = CAN_STATE_BUS_OFF;
> > + cf->can_id |= CAN_ERR_BUSOFF;
> > + priv->can.can_stats.bus_off++;
> > + /* Leave device in Config Mode in bus-off state */
> > + priv->write_reg(priv, XCAN_SRR_OFFSET,
> XCAN_SRR_RESET_MASK);
> > + can_bus_off(ndev);
> > + } else if ((status & XCAN_SR_ESTAT_MASK) ==
> XCAN_SR_ESTAT_MASK) {
> > + cf->can_id |= CAN_ERR_CRTL;
> > + priv->can.state = CAN_STATE_ERROR_PASSIVE;
> > + priv->can.can_stats.error_passive++;
> > + cf->data[1] |= CAN_ERR_CRTL_RX_PASSIVE |
> > + CAN_ERR_CRTL_TX_PASSIVE;
> > + } else if (status & XCAN_SR_ERRWRN_MASK) {
> > + cf->can_id |= CAN_ERR_CRTL;
> > + priv->can.state = CAN_STATE_ERROR_WARNING;
> > + priv->can.can_stats.error_warning++;
> > + cf->data[1] |= CAN_ERR_CRTL_RX_WARNING |
> > + CAN_ERR_CRTL_TX_WARNING;
> > + }
> > +
> > + /* Check for Arbitration lost interrupt */
> > + if (isr & XCAN_IXR_ARBLST_MASK) {
> > + cf->can_id |= CAN_ERR_LOSTARB;
> > + cf->data[0] = CAN_ERR_LOSTARB_UNSPEC;
> > + priv->can.can_stats.arbitration_lost++;
> > + }
> > +
> > + /* Check for RX FIFO Overflow interrupt */
> > + if (isr & XCAN_IXR_RXOFLW_MASK) {
> > + cf->can_id |= CAN_ERR_CRTL;
> > + cf->data[1] |= CAN_ERR_CRTL_RX_OVERFLOW;
> > + stats->rx_over_errors++;
> > + stats->rx_errors++;
> > + priv->write_reg(priv, XCAN_SRR_OFFSET,
> XCAN_SRR_RESET_MASK);
> > + }
> > +
> > + /* Check for error interrupt */
> > + if (isr & XCAN_IXR_ERROR_MASK) {
> > + cf->can_id |= CAN_ERR_PROT | CAN_ERR_BUSERROR;
> > + cf->data[2] |= CAN_ERR_PROT_UNSPEC;
> > +
> > + /* Check for Ack error interrupt */
> > + if (err_status & XCAN_ESR_ACKER_MASK) {
> > + cf->can_id |= CAN_ERR_ACK;
> > + cf->data[3] |= CAN_ERR_PROT_LOC_ACK;
> > + stats->tx_errors++;
> > + }
> > +
> > + /* Check for Bit error interrupt */
> > + if (err_status & XCAN_ESR_BERR_MASK) {
> > + cf->can_id |= CAN_ERR_PROT;
> > + cf->data[2] = CAN_ERR_PROT_BIT;
> > + stats->tx_errors++;
> > + }
> > +
> > + /* Check for Stuff error interrupt */
> > + if (err_status & XCAN_ESR_STER_MASK) {
> > + cf->can_id |= CAN_ERR_PROT;
> > + cf->data[2] = CAN_ERR_PROT_STUFF;
> > + stats->rx_errors++;
> > + }
> > +
> > + /* Check for Form error interrupt */
> > + if (err_status & XCAN_ESR_FMER_MASK) {
> > + cf->can_id |= CAN_ERR_PROT;
> > + cf->data[2] = CAN_ERR_PROT_FORM;
> > + stats->rx_errors++;
> > + }
> > +
> > + /* Check for CRC error interrupt */
> > + if (err_status & XCAN_ESR_CRCER_MASK) {
> > + cf->can_id |= CAN_ERR_PROT;
> > + cf->data[3] = CAN_ERR_PROT_LOC_CRC_SEQ |
> > + CAN_ERR_PROT_LOC_CRC_DEL;
> > + stats->rx_errors++;
> > + }
> > + priv->can.can_stats.bus_error++;
> > + }
> > +
> > + netif_rx(skb);
> > + stats->rx_packets++;
> > + stats->rx_bytes += cf->can_dlc;
> > +
> > + netdev_dbg(ndev, "%s: error status register:0x%x\n",
> > + __func__, priv->read_reg(priv, XCAN_ESR_OFFSET));
> }
> > +
> > +/**
> > + * xcan_state_interrupt - It will check the state of the CAN device
> > + * @ndev: net_device pointer
> > + * @isr: interrupt status register value
> > + *
> > + * This will checks the state of the CAN device
> > + * and puts the device into appropriate state.
> > + */
> > +static void xcan_state_interrupt(struct net_device *ndev, u32 isr) {
> > + struct xcan_priv *priv = netdev_priv(ndev);
> > +
> > + /* Check for Sleep interrupt if set put CAN device in sleep state */
> > + if (isr & XCAN_IXR_SLP_MASK)
> > + priv->can.state = CAN_STATE_SLEEPING;
> > +
> > + /* Check for Wake up interrupt if set put CAN device in Active state
> */
> > + if (isr & XCAN_IXR_WKUP_MASK)
> > + priv->can.state = CAN_STATE_ERROR_ACTIVE; }
> > +
> > +/**
> > + * xcan_rx_poll - Poll routine for rx packets (NAPI)
> > + * @napi: napi structure pointer
> > + * @quota: Max number of rx packets to be processed.
> > + *
> > + * This is the poll routine for rx part.
> > + * It will process the packets maximux quota value.
> > + *
> > + * Return: number of packets received */ static int
> > +xcan_rx_poll(struct napi_struct *napi, int quota) {
> > + struct net_device *ndev = napi->dev;
> > + struct xcan_priv *priv = netdev_priv(ndev);
> > + u32 isr, ier;
> > + int work_done = 0;
> > +
> > + isr = priv->read_reg(priv, XCAN_ISR_OFFSET);
> > + while ((isr & XCAN_IXR_RXNEMP_MASK) && (work_done < quota)) {
> > + if (isr & XCAN_IXR_RXOK_MASK) {
> > + priv->write_reg(priv, XCAN_ICR_OFFSET,
> > + XCAN_IXR_RXOK_MASK);
> > + if (xcan_rx(ndev) < 0)
> > + return work_done;
> > + work_done++;
> > + } else {
> > + priv->write_reg(priv, XCAN_ICR_OFFSET,
> > + XCAN_IXR_RXNEMP_MASK);
> > + break;
> > + }
> > + priv->write_reg(priv, XCAN_ICR_OFFSET,
> XCAN_IXR_RXNEMP_MASK);
> > + isr = priv->read_reg(priv, XCAN_ISR_OFFSET);
> > + }
> > +
> > + if (work_done < quota) {
> > + napi_complete(napi);
> > + ier = priv->read_reg(priv, XCAN_IER_OFFSET);
> > + ier |= (XCAN_IXR_RXOK_MASK |
> XCAN_IXR_RXNEMP_MASK);
> > + priv->write_reg(priv, XCAN_IER_OFFSET, ier);
> > + }
> > + return work_done;
> > +}
> > +
> > +/**
> > + * xcan_tx_interrupt - Tx Done Isr
> > + * @ndev: net_device pointer
> > + * @isr: Interrupt status register value
> > + */
> > +static void xcan_tx_interrupt(struct net_device *ndev, u32 isr) {
> > + struct xcan_priv *priv = netdev_priv(ndev);
> > + struct net_device_stats *stats = &ndev->stats;
> > +
> > + stats->tx_packets++;
> Your tx_packats is only incremented once, even if there is more than one
> frame transmitted.
>
> This loop looks broken. Can you explain how it works.
>
> What it shoud do is:
> We have put (priv->tx_head - priv->tx_tail) CAN frames into the FIFO.
> This means at maximum there could be this amount of CAN frames which
> have been successfully transmitted. For every cycle in this while loop you
> should:
> a) check if a CAN frame has successfully been transmitted
> (as this CAN core uses a FIFO it should be "oldest")
> A read_reg() of some kind is missing in your loop.
> b) if needed, remove this event from the FIFO or
> mark the interrupt as done. Whatever you hardware needs.
> c) update your statistics
> d) Use can_get_echo_skb to push this frame into the networking stack
> e) As a CAN frame has been transmitted successfully, wake the tx_queue.
>
> > + while (priv->tx_head - priv->tx_tail > 0) {
> > + if (isr & XCAN_IXR_TXFLL_MASK) {
> > + priv->write_reg(priv, XCAN_ICR_OFFSET,
> > + XCAN_IXR_TXFLL_MASK);
> > + netif_stop_queue(ndev);
>
> Why do you stop the queue here? A CAN frame has successfully been
> transmitted, there should be room in the FIFO.
>
> > + break;
> > + }
> > + can_get_echo_skb(ndev, priv->tx_tail %
> > + priv->xcan_echo_skb_max_tx);
> > + priv->tx_tail++;
> > + }
> > +
The below are the bit fields available for the Transmit FIFO.
1) In the ISR(interrupt status register) Tx Ok interrupt and Tx fifo full interrupt.
2) in the SR(Status Register) Tx fifo full condition.
I am modifying the entire tx interrupt logic to like below.
static void xcan_tx_interrupt(struct net_device *ndev, u32 isr)
{
struct xcan_priv *priv = netdev_priv(ndev);
struct net_device_stats *stats = &ndev->stats;
while (priv->tx_head - priv->tx_tail > 0) {
if (isr & XCAN_IXR_TXFLL_MASK) {
priv->write_reg(priv, XCAN_ICR_OFFSET,
XCAN_IXR_TXFLL_MASK);
break;
}
can_get_echo_skb(ndev, priv->tx_tail %
priv->xcan_echo_skb_max_tx);
priv->tx_tail++;
stats->tx_packets++;
netif_wake_queue(ndev);
can_led_event(ndev, CAN_LED_EVENT_TX);
}
}
Are you Ok with the above logic?
> > + netif_wake_queue(ndev);
> > + can_led_event(ndev, CAN_LED_EVENT_TX); }
> > +
> > +/**
> > + * xcan_interrupt - CAN Isr
> > + * @irq: irq number
> > + * @dev_id: device id poniter
> > + *
> > + * This is the xilinx CAN Isr. It checks for the type of interrupt
> > + * and invokes the corresponding ISR.
> > + *
> > + * Return:
> > + * IRQ_NONE - If CAN device is in sleep mode, IRQ_HANDLED otherwise
> > +*/ static irqreturn_t xcan_interrupt(int irq, void *dev_id) {
> > + struct net_device *ndev = (struct net_device *)dev_id;
> > + struct xcan_priv *priv = netdev_priv(ndev);
> > + u32 isr, ier;
> > +
> > + /* Get the interrupt status from Xilinx CAN */
> > + isr = priv->read_reg(priv, XCAN_ISR_OFFSET);
> > + if (!isr)
> > + return IRQ_NONE;
> > +
> > + netdev_dbg(ndev, "%s: isr:#x%08x, err:#x%08x\n", __func__,
> > + isr, priv->read_reg(priv, XCAN_ESR_OFFSET));
> > +
> > + /* Check for the type of interrupt and Processing it */
> > + if (isr & (XCAN_IXR_SLP_MASK | XCAN_IXR_WKUP_MASK)) {
> > + priv->write_reg(priv, XCAN_ICR_OFFSET,
> (XCAN_IXR_SLP_MASK |
> > + XCAN_IXR_WKUP_MASK));
> > + xcan_state_interrupt(ndev, isr);
> > + }
> > +
> > + /* Check for Tx interrupt and Processing it */
> > + if (isr & XCAN_IXR_TXOK_MASK) {
> > + priv->write_reg(priv, XCAN_ICR_OFFSET,
> XCAN_IXR_TXOK_MASK);
> > + xcan_tx_interrupt(ndev, isr);
> > + }
> > +
> > + /* Check for the type of error interrupt and Processing it */
> > + if (isr & (XCAN_IXR_ERROR_MASK | XCAN_IXR_RXOFLW_MASK |
> > + XCAN_IXR_BSOFF_MASK |
> XCAN_IXR_ARBLST_MASK)) {
> > + priv->write_reg(priv, XCAN_ICR_OFFSET,
> (XCAN_IXR_ERROR_MASK |
> > + XCAN_IXR_RXOFLW_MASK |
> XCAN_IXR_BSOFF_MASK |
> > + XCAN_IXR_ARBLST_MASK));
> > + xcan_err_interrupt(ndev, isr);
> > + }
> > +
> > + /* Check for the type of receive interrupt and Processing it */
> > + if (isr & (XCAN_IXR_RXNEMP_MASK | XCAN_IXR_RXOK_MASK)) {
> > + ier = priv->read_reg(priv, XCAN_IER_OFFSET);
> > + ier &= ~(XCAN_IXR_RXNEMP_MASK |
> XCAN_IXR_RXOK_MASK);
> > + priv->write_reg(priv, XCAN_IER_OFFSET, ier);
> > + napi_schedule(&priv->napi);
> > + }
> > + return IRQ_HANDLED;
> > +}
> > +
> > +/**
> > + * xcan_stop - Driver stop routine
> > + * @ndev: Pointer to net_device structure
> > + *
> > + * This is the drivers stop routine. It will disable the
> > + * interrupts and put the device into configuration mode.
> > + */
> > +static void xcan_stop(struct net_device *ndev)
>
> please name it chip_stop as it's the inverse of chip_start
Ok
> > +{
> > + struct xcan_priv *priv = netdev_priv(ndev);
> > + u32 ier;
> > +
> > + /* Disable interrupts and leave the can in configuration mode */
> > + ier = priv->read_reg(priv, XCAN_IER_OFFSET);
> > + ier &= ~XCAN_INTR_ALL;
> > + priv->write_reg(priv, XCAN_IER_OFFSET, ier);
> > + priv->write_reg(priv, XCAN_SRR_OFFSET, XCAN_SRR_RESET_MASK);
> > + priv->can.state = CAN_STATE_STOPPED; }
> > +
> > +/**
> > + * xcan_open - Driver open routine
> > + * @ndev: Pointer to net_device structure
> > + *
> > + * This is the driver open routine.
> > + * Return: 0 on success and failure value on error */ static int
> > +xcan_open(struct net_device *ndev) {
> > + struct xcan_priv *priv = netdev_priv(ndev);
> > + int ret;
> > +
>
> enable clocks
Ok
>
> > + ret = request_irq(ndev->irq, xcan_interrupt, priv->irq_flags,
> > + ndev->name, (void *)ndev);
>
> nitpick: the (void *) cast is not needed.
>
Ok
> > + if (ret < 0) {
> > + netdev_err(ndev, "Irq allocation for CAN failed\n");
> > + return ret;
> > + }
> > +
> > + /* Set chip into reset mode */
> > + ret = set_reset_mode(ndev);
> > + if (ret < 0) {
> > + netdev_err(ndev, "mode resetting failed failed!\n");
> > + free_irq(ndev->irq, ndev);
> > + return ret;
> > + }
> > +
> > + /* Common open */
> > + ret = open_candev(ndev);
> > + if (ret) {
> > + free_irq(ndev->irq, ndev);
> > + return ret;
> > + }
> > +
> > + ret = xcan_chip_start(ndev);
> > + if (ret < 0)
> free irq missing....better introduce a label at the end of the function and use
> goto to clean up.
Ok
> > + netdev_err(ndev, "xcan_chip_start failed!\n");
> > +
> > +
> > + can_led_event(ndev, CAN_LED_EVENT_OPEN);
> > + napi_enable(&priv->napi);
> > + netif_start_queue(ndev);
> > +
> > + return 0;
> > +}
> > +
> > +/**
> > + * xcan_close - Driver close routine
> > + * @ndev: Pointer to net_device structure
> > + *
> > + * Return: 0 always
> > + */
> > +static int xcan_close(struct net_device *ndev) {
> > + struct xcan_priv *priv = netdev_priv(ndev);
> > +
> > + netif_stop_queue(ndev);
> > + napi_disable(&priv->napi);
> > + xcan_stop(ndev);
> > + free_irq(ndev->irq, ndev);
> > + close_candev(ndev);
> > +
>
> add stop clocks somewhere here
>
Ok
> > + can_led_event(ndev, CAN_LED_EVENT_STOP);
> > +
> > + return 0;
> > +}
> > +
> > +/**
> > + * xcan_get_berr_counter - error counter routine
> > + * @ndev: Pointer to net_device structure
> > + * @bec: Pointer to can_berr_counter structure
> > + *
> > + * This is the driver error counter routine.
> > + * Return: 0 always
> > + */
> > +static int xcan_get_berr_counter(const struct net_device *ndev,
> > + struct can_berr_counter *bec)
> > +{
> > + struct xcan_priv *priv = netdev_priv(ndev);
>
> If you leave the clock switched off after probe (see below), you probalby
> have to enable the clocks here.
Ok
> > +
> > + bec->txerr = priv->read_reg(priv, XCAN_ECR_OFFSET) &
> XCAN_ECR_TEC_MASK;
> > + bec->rxerr = ((priv->read_reg(priv, XCAN_ECR_OFFSET) &
> > + XCAN_ECR_REC_MASK) >> XCAN_ESR_REC_SHIFT);
> > +
>
> and disable here again.
Ok
>
> > + return 0;
> > +}
> > +
> > +static const struct net_device_ops xcan_netdev_ops = {
> > + .ndo_open = xcan_open,
> > + .ndo_stop = xcan_close,
> > + .ndo_start_xmit = xcan_start_xmit,
> > +};
> > +
> > +#ifdef CONFIG_PM_SLEEP
> > +/**
> > + * xcan_suspend - Suspend method for the driver
> > + * @_dev: Address of the platform_device structure
> > + *
> > + * Put the driver into low power mode.
> > + * Return: 0 always
> > + */
> > +static int xcan_suspend(struct device *_dev) {
> > + struct platform_device *pdev = container_of(_dev,
> > + struct platform_device, dev);
> > + struct net_device *ndev = platform_get_drvdata(pdev);
> > + struct xcan_priv *priv = netdev_priv(ndev);
> > +
> > + if (netif_running(ndev)) {
> > + netif_stop_queue(ndev);
> > + netif_device_detach(ndev);
> > + }
> > +
> > + priv->write_reg(priv, XCAN_MSR_OFFSET,
> XCAN_MSR_SLEEP_MASK);
> > + priv->can.state = CAN_STATE_SLEEPING;
> > +
> > + clk_disable(priv->aperclk);
> > + clk_disable(priv->devclk);
> > +
> > + return 0;
> > +}
> > +
> > +/**
> > + * xcan_resume - Resume from suspend
> > + * @dev: Address of the platformdevice structure
> > + *
> > + * Resume operation after suspend.
> > + * Return: 0 on success and failure value on error */ static int
> > +xcan_resume(struct device *dev) {
> > + struct platform_device *pdev = container_of(dev,
> > + struct platform_device, dev);
> > + struct net_device *ndev = platform_get_drvdata(pdev);
> > + struct xcan_priv *priv = netdev_priv(ndev);
> > + int ret;
> > +
> > + ret = clk_enable(priv->aperclk);
> > + if (ret) {
> > + dev_err(dev, "Cannot enable clock.\n");
> > + return ret;
> > + }
> > + ret = clk_enable(priv->devclk);
> > + if (ret) {
> > + dev_err(dev, "Cannot enable clock.\n");
> please disable aperclk
Ok
> > + return ret;
> > + }
> > +
> > + priv->write_reg(priv, XCAN_MSR_OFFSET, 0);
> > + priv->write_reg(priv, XCAN_SRR_OFFSET, XCAN_SRR_CEN_MASK);
> > + priv->can.state = CAN_STATE_ERROR_ACTIVE;
> > +
> > + if (netif_running(ndev)) {
> > + netif_device_attach(ndev);
> > + netif_start_queue(ndev);
> > + }
> > +
> > + return 0;
> > +}
> > +#endif
> > +
> > +static SIMPLE_DEV_PM_OPS(xcan_dev_pm_ops, xcan_suspend,
> xcan_resume);
> > +
> > +/**
> > + * xcan_probe - Platform registration call
> > + * @pdev: Handle to the platform device structure
> > + *
> > + * This function does all the memory allocation and registration for
> > +the CAN
> > + * device.
> > + *
> > + * Return: 0 on success and failure value on error */ static int
> > +xcan_probe(struct platform_device *pdev) {
> > + struct resource *res; /* IO mem resources */
> > + struct net_device *ndev;
> > + struct xcan_priv *priv;
> > + int ret;
> > +
> > + /* Create a CAN device instance */
> > + ndev = alloc_candev(sizeof(struct xcan_priv),
> XCAN_ECHO_SKB_MAX);
> > + if (!ndev)
> > + return -ENOMEM;
> > +
> > + priv = netdev_priv(ndev);
> > + priv->dev = ndev;
> > + priv->can.bittiming_const = &xcan_bittiming_const;
> > + priv->can.do_set_bittiming = xcan_set_bittiming;
> > + priv->can.do_set_mode = xcan_do_set_mode;
> > + priv->can.do_get_berr_counter = xcan_get_berr_counter;
> > + priv->can.ctrlmode_supported = CAN_CTRLMODE_LOOPBACK |
> > + CAN_CTRLMODE_BERR_REPORTING;
> > +
> > + /* Get IRQ for the device */
> > + ndev->irq = platform_get_irq(pdev, 0);
> > + ndev->flags |= IFF_ECHO; /* We support local echo */
> > +
> > + platform_set_drvdata(pdev, ndev);
> > + SET_NETDEV_DEV(ndev, &pdev->dev);
> > + ndev->netdev_ops = &xcan_netdev_ops;
> > +
> > + /* Get the virtual base address for the device */
> > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > + priv->reg_base = devm_ioremap_resource(&pdev->dev, res);
> > + if (IS_ERR(priv->reg_base)) {
> > + ret = PTR_ERR(priv->reg_base);
> > + goto err_free;
> > + }
> > + ndev->mem_start = res->start;
> > + ndev->mem_end = res->end;
>
> Please don't assign mem_start and _end.
Ok
>
> > +
> > + ret = of_property_read_u32(pdev->dev.of_node, "tx-fifo-depth",
> > + &priv->xcan_echo_skb_max_tx);
> > + if (ret < 0)
> > + goto err_free;
> > +
> > + ret = of_property_read_u32(pdev->dev.of_node, "rx-fifo-depth",
> > + &priv->xcan_echo_skb_max_rx);
> > + if (ret < 0)
> > + goto err_free;
> > +
> > + /* Getting the CAN devclk info */
> > + priv->devclk = devm_clk_get(&pdev->dev, "ref_clk");
> > + if (IS_ERR(priv->devclk)) {
> > + dev_err(&pdev->dev, "Device clock not found.\n");
> > + ret = PTR_ERR(priv->devclk);
> > + goto err_free;
> > + }
> > +
> > + /* Check for type of CAN device */
> > + if (of_device_is_compatible(pdev->dev.of_node,
> > + "xlnx,zynq-can-1.00.a")) {
> > + priv->aperclk = devm_clk_get(&pdev->dev, "aper_clk");
> > + if (IS_ERR(priv->aperclk)) {
> > + dev_err(&pdev->dev, "aper clock not found\n");
> > + ret = PTR_ERR(priv->aperclk);
> > + goto err_free;
> > + }
> > + } else {
> > + priv->aperclk = priv->devclk;
> > + }
> > +
> > + ret = clk_prepare_enable(priv->devclk);
> > + if (ret) {
> > + dev_err(&pdev->dev, "unable to enable device clock\n");
> > + goto err_free;
> > + }
> > +
> > + ret = clk_prepare_enable(priv->aperclk);
> > + if (ret) {
> > + dev_err(&pdev->dev, "unable to enable aper clock\n");
> > + goto err_unprepar_disabledev;
> > + }
> > +
> > + priv->write_reg = xcan_write_reg_le;
> > + priv->read_reg = xcan_read_reg_le;
> > +
> > + if (priv->read_reg(priv, XCAN_SR_OFFSET) !=
> XCAN_SR_CONFIG_MASK) {
> > + priv->write_reg = xcan_write_reg_be;
> > + priv->read_reg = xcan_read_reg_be;
> > + }
> > +
> > + priv->can.clock.freq = clk_get_rate(priv->devclk);
> > +
> > + netif_napi_add(ndev, &priv->napi, xcan_rx_poll,
> > + priv->xcan_echo_skb_max_rx);
>
> please align with the opening bracket
Ok
>
> > +
> > + ret = register_candev(ndev);
> > + if (ret) {
> > + dev_err(&pdev->dev, "fail to register failed (err=%d)\n",
> ret);
> > + goto err_unprepar_disableaper;
> > + }
> > +
> > + devm_can_led_init(ndev);
> > + dev_info(&pdev->dev,
> > + "reg_base=0x%p irq=%d clock=%d, tx fifo
> depth:%d\n",
> > + priv->reg_base, ndev->irq, priv->can.clock.freq,
> > + priv->xcan_echo_skb_max_tx);
>
> please use netdev_dbg
>
Ok
> please disable the clocks here, as they are not used until open().
>
Ok
Regards,
Kedar.
> > +
> > + return 0;
> > +
> > +err_unprepar_disableaper:
> > + clk_disable_unprepare(priv->aperclk);
> > +err_unprepar_disabledev:
> > + clk_disable_unprepare(priv->devclk);
> > +err_free:
> > + free_candev(ndev);
> > +
> > + return ret;
> > +}
> > +
> > +/**
> > + * xcan_remove - Unregister the device after releasing the resources
> > + * @pdev: Handle to the platform device structure
> > + *
> > + * This function frees all the resources allocated to the device.
> > + * Return: 0 always
> > + */
> > +static int xcan_remove(struct platform_device *pdev) {
> > + struct net_device *ndev = platform_get_drvdata(pdev);
> > + struct xcan_priv *priv = netdev_priv(ndev);
> > +
> > + if (set_reset_mode(ndev) < 0)
> > + netdev_err(ndev, "mode resetting failed!\n");
> > +
> > + unregister_candev(ndev);
> > + netif_napi_del(&priv->napi);
> > + clk_disable_unprepare(priv->aperclk);
> > + clk_disable_unprepare(priv->devclk);
> > + free_candev(ndev);
> > +
> > + return 0;
> > +}
> > +
> > +/* Match table for OF platform binding */ static struct of_device_id
> > +xcan_of_match[] = {
> > + { .compatible = "xlnx,zynq-can-1.00.a", },
> > + { .compatible = "xlnx,axi-can-1.00.a", },
> > + { /* end of list */ },
> > +};
> > +MODULE_DEVICE_TABLE(of, xcan_of_match);
> > +
> > +static struct platform_driver xcan_driver = {
> > + .probe = xcan_probe,
> > + .remove = xcan_remove,
> > + .driver = {
> > + .owner = THIS_MODULE,
> > + .name = DRIVER_NAME,
> > + .pm = &xcan_dev_pm_ops,
> > + .of_match_table = xcan_of_match,
> > + },
> > +};
> > +
> > +module_platform_driver(xcan_driver);
> > +
> > +MODULE_LICENSE("GPL");
> > +MODULE_AUTHOR("Xilinx Inc");
> > +MODULE_DESCRIPTION("Xilinx CAN interface");
> >
>
> Marc
>
> --
> Pengutronix e.K. | Marc Kleine-Budde |
> Industrial Linux Solutions | Phone: +49-231-2826-924 |
> Vertretung West/Dortmund | Fax: +49-5121-206917-5555 |
> Amtsgericht Hildesheim, HRA 2686 | http://www.pengutronix.de |
This email and any attachments are intended for the sole use of the named recipient(s) and contain(s) confidential information that may be proprietary, privileged or copyrighted under applicable law. If you are not the intended recipient, do not read, copy, or forward this email message or any attachments. Delete this email message and any attachments immediately.
^ permalink raw reply
* Re: [PATCH v4 3/3] Documentation: of: Document graph bindings
From: Tomi Valkeinen @ 2014-02-26 13:14 UTC (permalink / raw)
To: Philipp Zabel
Cc: Russell King - ARM Linux, Mauro Carvalho Chehab, Grant Likely,
Rob Herring, Sylwester Nawrocki, Laurent Pinchart, Kyungmin Park,
linux-kernel, linux-media, devicetree, Guennadi Liakhovetski
In-Reply-To: <1393340304-19005-4-git-send-email-p.zabel@pengutronix.de>
[-- Attachment #1: Type: text/plain, Size: 692 bytes --]
On 25/02/14 16:58, Philipp Zabel wrote:
> +Optional endpoint properties
> +----------------------------
> +
> +- remote-endpoint: phandle to an 'endpoint' subnode of a remote device node.
Why is that optional? What use is an endpoint, if it's not connected to
something?
Also, if this is being worked on, I'd like to propose the addition of
simpler single-endpoint cases which I've been using with OMAP DSS. So if
there's just a single endpoint for the device, which is very common, you
can have just:
device {
...
endpoint { ... };
};
However, I guess that the patch just keeps growing and growing, so maybe
it's better to add such things later =).
Tomi
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 901 bytes --]
^ permalink raw reply
* Re: [PATCH v4] can: xilinx CAN controller support.
From: Marc Kleine-Budde @ 2014-02-26 13:22 UTC (permalink / raw)
To: Appana Durga Kedareswara Rao, wg@grandegger.com, Michal Simek,
grant.likely@linaro.org, robh+dt@kernel.org,
linux-can@vger.kernel.org
Cc: netdev@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
linux-kernel@vger.kernel.org, devicetree@vger.kernel.org
In-Reply-To: <cd00fb2d-c8a7-423d-8aa0-2b413e9ac101@CO9EHSMHS017.ehs.local>
[-- Attachment #1: Type: text/plain, Size: 3028 bytes --]
On 02/26/2014 02:07 PM, Appana Durga Kedareswara Rao wrote:
>> This loop looks broken. Can you explain how it works.
>>
>> What it shoud do is:
>> We have put (priv->tx_head - priv->tx_tail) CAN frames into the FIFO.
>> This means at maximum there could be this amount of CAN frames which
>> have been successfully transmitted. For every cycle in this while loop you
>> should:
>> a) check if a CAN frame has successfully been transmitted
>> (as this CAN core uses a FIFO it should be "oldest")
>> A read_reg() of some kind is missing in your loop.
>> b) if needed, remove this event from the FIFO or
>> mark the interrupt as done. Whatever you hardware needs.
>> c) update your statistics
>> d) Use can_get_echo_skb to push this frame into the networking stack
>> e) As a CAN frame has been transmitted successfully, wake the tx_queue.
>>
>>> + while (priv->tx_head - priv->tx_tail > 0) {
>>> + if (isr & XCAN_IXR_TXFLL_MASK) {
>>> + priv->write_reg(priv, XCAN_ICR_OFFSET,
>>> + XCAN_IXR_TXFLL_MASK);
>>> + netif_stop_queue(ndev);
>>
>> Why do you stop the queue here? A CAN frame has successfully been
>> transmitted, there should be room in the FIFO.
>>
>>> + break;
>>> + }
>>> + can_get_echo_skb(ndev, priv->tx_tail %
>>> + priv->xcan_echo_skb_max_tx);
>>> + priv->tx_tail++;
>>> + }
>>> +
>
> The below are the bit fields available for the Transmit FIFO.
> 1) In the ISR(interrupt status register) Tx Ok interrupt and Tx fifo full interrupt.
> 2) in the SR(Status Register) Tx fifo full condition.
>
>
> I am modifying the entire tx interrupt logic to like below.
>
> static void xcan_tx_interrupt(struct net_device *ndev, u32 isr)
> {
> struct xcan_priv *priv = netdev_priv(ndev);
> struct net_device_stats *stats = &ndev->stats;
>
> while (priv->tx_head - priv->tx_tail > 0) {
> if (isr & XCAN_IXR_TXFLL_MASK) {
> priv->write_reg(priv, XCAN_ICR_OFFSET,
> XCAN_IXR_TXFLL_MASK);
> break;
> }
> can_get_echo_skb(ndev, priv->tx_tail %
> priv->xcan_echo_skb_max_tx);
> priv->tx_tail++;
> stats->tx_packets++;
> netif_wake_queue(ndev);
> can_led_event(ndev, CAN_LED_EVENT_TX);
>
> }
You just need to wake the queue once.
> }
>
>
> Are you Ok with the above logic?
No, how can you tell how many frames have been transmitted?
Marc
--
Pengutronix e.K. | Marc Kleine-Budde |
Industrial Linux Solutions | Phone: +49-231-2826-924 |
Vertretung West/Dortmund | Fax: +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686 | http://www.pengutronix.de |
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 242 bytes --]
^ permalink raw reply
* Re: [PATCH 2/2] ARM: dts: duovero-parlor: Add HDMI output
From: Russell King - ARM Linux @ 2014-02-26 13:28 UTC (permalink / raw)
To: Tomi Valkeinen
Cc: Laurent Pinchart, Sebastian Reichel, Javier Martinez Canillas,
Florian Vaussard, Benoît Cousson, Tony Lindgren,
devicetree@vger.kernel.org, linux-omap@vger.kernel.org,
Ash Charles, linux-arm-kernel@lists.infradead.org, Arnd Bergmann
In-Reply-To: <530DE192.9030506@ti.com>
On Wed, Feb 26, 2014 at 02:44:02PM +0200, Tomi Valkeinen wrote:
> > Also - DRM is not going to ever support hotplugging components - this
> > was discussed at kernel summit last year and David Airlie was quite
>
> Ok. Very odd stance. Maybe there's a reason for it that I just don't see.
DRM is like ALSA - it's a card level subsystem. All components have
to be present before the card level is brought up for the subsystem to
function correctly.
> > adamant about that. So, any "framework" which forces hotplugging of
> > components on subsystems isn't going to fly.
>
> CDF doesn't force hotplugging.
>
> Although without hotplugging (hot-unplug not needed), or at least some
> minimal form of it, the system is a bit crippled. Leave one kernel
> module out, or have one driver probe fail, the whole display subsystem
> fails, even if some display pipelines would work fine.
That is, unfortunately, one of the side effects of the policy - but that's
not a policy that's going to change any time soon. As I said, that was
made very clear at the last kernel summit - we had a /specific/ session
on the issues around multi-device DRM chaired by David.
There are some DRM drivers which have tried to do this, but they're all
buggy in some regard, whether that be to the point of oopsing the kernel
if things don't quite go to plan, or whether it's races between different
parts.
> > This is why we now have the component helpers in the driver model -
> > to allow devices to be collected together into one logical subsystem
> > group, and bound/unbound as a group.
>
> Yep, it's a good start. The component helpers could well be used with CDF.
>
> But if I'm not mistaken, it suffers from the problems above, when there
> are multiple independent pipelines (simultaneous or non-simultaneous)
> handled by the same IPs.
It may "suffer from the problems above" that you've raised, but that's
by explicit design of it - because that's what subsystems like DRM and
ALSA require, and this is _precisely_ the problem it's solving.
It's solving the "subsystem requires a stable view of hardware components,
but we have multiple devices and drivers which need probing" problem.
> And, while I may be mistaken, it sounds that the component helpers leave
> mostly everything up to the display drivers. Everyone devising their own
> way to describe the hardware in DT, and the connections between the
> components. Of course, the core component system shouldn't define
> anything DT related, as it doesn't. But that part is still needed, which
> is where CDF comes in.
Sigh. It's very easy for people to get the wrong end of the stick.
What the component helpers do is provide a _subsystem_ _independent_
method of collecting a set of devices together and binding them to the
drivers at an appropriate time, in a way that is _completely_ independent
of whether you're using platform data, DT, ACPI, or whatever other
hardware description language comes along.
It's up to the users of this to define how components are grouped
together, whether that be at the subsystem level or at the driver
level - whatever is appropriate.
If a subsystem (eg, a display subsystem) wants to define "this is how
you define in DT the bindings between all components" and provide its
own hook for the "add_components" callback which does this, then it's
at liberty to do that.
If we can come up with a generic way to describe how all the components
in a display subsystem should be connected together, then great - but
that needs to happen very quickly. Philipp Zabel is working on replacing
the imx-drm binding method right now for 3.15, and is probably completely
unaware of anything that's been talked about here. I need to sort out
Armada DRM at some point to use the component stuff, which includes
sorting out TDA998x for DT - which again needs to be done in such a way
that it follows a common theme.
> I need to ask Dave why he is so strongly opposed to hotplugging components.
I suspect one reason for it is because it means rewriting the XF86
backend to Xorg to cope with it...
> > Remember that "hotplugging" in this context does not mean that the
> > user can physically do something with the hardware. It means that
> > they're separate devices which can be probed/removed at will. Every
> > device in Linux can be bound and unbound from its driver at any time
> > by userspace, and that is something which is expected to be handled
> > gracefully.
>
> Hmm, sorry, can you rephrase?
I'll do better than that. Try running this script with the
/sys/bus/.../drivers/whatever for the drivers you wish to test:
#!/bin/sh
for driver in "$@"; do
if [ -f "${driver}/unbind" ]; then
for device in "${driver}"/*; do
if [ -d "${device}" ]; then
devname="$(basename "${device}")"
echo "$devname" > "${driver}/unbind"
echo "$devname" > "${driver}/bind"
fi
done
fi
done
The system should survive that.
> So with hotplug, a new fbdev or a combination of drm crtcs, encoders,
> etc, could appear even after the initial probe of the display controller.
This is the exact situation that David is opposed to. DRM, like ALSA,
whats to have a stable view of hardware - once the drm_device has been
created and probed, no further changes to it are allowed.
Certainly no changes to the CRTCs will _ever_ be permitted, because it
completely destroys the user API for referecing which encoders can be
associated with which CRTCs - not only at the kernel level, but also the
Xorg and Xrandr level too. That's done via a bitmask of allowable CRTCs,
where bit 0 refers to the first CRTC, bit 1 to the second and so on.
That's propagated all the way through to userspace, right through the Xorg
interfaces to applications.
Connectors and encoders are fixed at the moment after initial probe time
in DRM due to the way the fbdev emulation layer works. There's also issues
there concerning bitmasks for which connectors can be cloned onto other
connectors which follows the same pattern as above - and again, that
propagates all the way through userspace.
So, if this is going to get fixed, there has to be a desire to break
userspace quite badly, and there is no such desire to do that.
For instance, let's say that Xorg is up and running, and you have the
gnome applet for configuring your display open, and you have two CRTCs.
Then the first CRTC is removed from the system, resulting in CRTC 1
becoming CRTC 0 in the kernel. What happens...
Think the same thing through for a system with three connectors, A, B, C
numbered 0, 1, and 2 respectively. A and be cloned onto B. Now connector
A is removed, meaning B and C appear to become numbers 0 and 1 in the
kernel...
--
FTTC broadband for 0.8mile line: now at 9.7Mbps down 460kbps up... slowly
improving, and getting towards what was expected from it.
^ permalink raw reply
* Re: [PATCH RESEND v10 0/7] mmc: omap_hsmmc: pbias dt and cleanup
From: Florian Vaussard @ 2014-02-26 14:04 UTC (permalink / raw)
To: Balaji T K, linux-omap, bcousson, devicetree, linux-mmc, chris,
tony
Cc: broonie
In-Reply-To: <1392821801-29673-1-git-send-email-balajitk@ti.com>
Hi,
On 02/19/2014 03:56 PM, Balaji T K wrote:
> Few cleanups to reduce code indent,
> Add pbias_regulator support and adapt omap_hsmmc to use pbias regulator
> to configure required voltage on mmc1 pad(SD card) i/o rails on OMAP SoCs.
>
I tested on both OMAP3630 (Overo Storm) and OMAP4430 (DuoVero). The
rootfs is mounted on mmc1 and works as usual. Here is what I can see:
- pbias-supply is parsed from DT
- VMMC1 is set to 3V
- According to the debugfs entry, we are working at 3V signaling
- By dumping CONTROL_PBIASLITE, bit MMC1_PBIASLITE_VMODE
(PBIASLITEVMODE0 on OMAP3) is set to 1 (-> 3V)
Do you see any other tests that I could run to validate your patches?
Otherwise:
Tested-by: Florian Vaussard <florian.vaussard@epfl.ch>
> Balaji T K (7):
> mmc: omap_hsmmc: use devm_regulator API
> mmc: omap_hsmmc: handle vcc and vcc_aux independently
> regulator: add pbias regulator support
> mmc: omap_hsmmc: adapt hsmmc to use pbias regulator
> ARM: dts: add pbias dt node
> ARM: OMAP: enable SYSCON and REGULATOR_PBIAS in omap2plus_defconfig
> mmc: omap_hsmmc: remove pbias workaround
>
> .../bindings/regulator/pbias-regulator.txt | 27 ++
> arch/arm/boot/dts/dra7.dtsi | 17 ++
> arch/arm/boot/dts/omap2430.dtsi | 17 ++
> arch/arm/boot/dts/omap3.dtsi | 17 ++
> arch/arm/boot/dts/omap4.dtsi | 17 ++
> arch/arm/boot/dts/omap5.dtsi | 17 ++
> arch/arm/configs/omap2plus_defconfig | 2 +
> drivers/mmc/host/omap_hsmmc.c | 111 +++++----
> drivers/regulator/Kconfig | 9 +
> drivers/regulator/Makefile | 1 +
> drivers/regulator/pbias-regulator.c | 255 ++++++++++++++++++++
> 11 files changed, 441 insertions(+), 49 deletions(-)
> create mode 100644 Documentation/devicetree/bindings/regulator/pbias-regulator.txt
> create mode 100644 drivers/regulator/pbias-regulator.c
>
^ permalink raw reply
* Re: [PATCH v3 5/6] pwm: enable TI PWMSS if the IIO tiecap driver is selected
From: Thierry Reding @ 2014-02-26 14:15 UTC (permalink / raw)
To: Matt Porter
Cc: Jonathan Cameron, Grant Likely, Rob Herring, Benoît Cousson,
Tony Lindgren, Pawel Moll, Mark Rutland, Ian Campbell, Kumar Gala,
Linux IIO List, Linux Kernel Mailing List, Devicetree List,
Linux PWM List, Linux OMAP List, Linux ARM Kernel List
In-Reply-To: <1391626901-31684-6-git-send-email-mporter-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
[-- Attachment #1: Type: text/plain, Size: 922 bytes --]
On Wed, Feb 05, 2014 at 02:01:40PM -0500, Matt Porter wrote:
> The IIO TI ECAP driver depends on the TI PWMSS management
> driver in this subsystem. Enable PWMSS when the IIO TI ECAP
> driver is selected.
>
> Signed-off-by: Matt Porter <mporter-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> ---
> drivers/pwm/Kconfig | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> index 22f2f28..bd3cc65 100644
> --- a/drivers/pwm/Kconfig
> +++ b/drivers/pwm/Kconfig
> @@ -219,7 +219,7 @@ config PWM_TIEHRPWM
>
> config PWM_TIPWMSS
> bool
> - default y if SOC_AM33XX && (PWM_TIECAP || PWM_TIEHRPWM)
> + default y if SOC_AM33XX && (IIO_TIECAP || PWM_TIECAP || PWM_TIEHRPWM)
> help
> PWM Subsystem driver support for AM33xx SOC.
Perhaps this module should move out of drivers/pwm if it's no longer a
PWM specific module.
Thierry
[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply
* Re: [PATCH v3 1/3] dma: Support multiple interleaved frames with non-contiguous memory
From: Srikanth Thokala @ 2014-02-26 14:21 UTC (permalink / raw)
To: Jassi Brar
Cc: Srikanth Thokala, Williams, Dan J, Koul, Vinod, Michal Simek,
Grant Likely, Rob Herring, devicetree, Levente Kurusa,
Lars-Peter Clausen, lkml, dmaengine, Andy Shevchenko,
linux-arm-kernel@lists.infradead.org
In-Reply-To: <CAJe_ZhfSknzwMUo3nu14Hz5sr_hA6KojB=u4BvyT3byh5ViwXA@mail.gmail.com>
On Mon, Feb 24, 2014 at 7:39 AM, Jassi Brar <jaswinder.singh@linaro.org> wrote:
> On 21 February 2014 23:37, Srikanth Thokala <sthokal@xilinx.com> wrote:
>> On Thu, Feb 20, 2014 at 3:23 PM, Jassi Brar <jaswinder.singh@linaro.org>
>> wrote:
>>> On 20 February 2014 14:54, Srikanth Thokala <sthokal@xilinx.com> wrote:
>>>> On Wed, Feb 19, 2014 at 12:33 AM, Jassi Brar <jaswinder.singh@linaro.org>
>>>> wrote:
>>>>> On 18 February 2014 23:16, Srikanth Thokala <sthokal@xilinx.com> wrote:
>>>>>> On Tue, Feb 18, 2014 at 10:20 PM, Jassi Brar
>>>>>> <jaswinder.singh@linaro.org> wrote:
>>>>>>> On 18 February 2014 16:58, Srikanth Thokala <sthokal@xilinx.com>
>>>>>>> wrote:
>>>>>>>> On Mon, Feb 17, 2014 at 3:27 PM, Jassi Brar
>>>>>>>> <jaswinder.singh@linaro.org> wrote:
>>>>>>>>> On 15 February 2014 17:30, Srikanth Thokala <sthokal@xilinx.com>
>>>>>>>>> wrote:
>>>>>>>>>> The current implementation of interleaved DMA API support multiple
>>>>>>>>>> frames only when the memory is contiguous by incrementing
>>>>>>>>>> src_start/
>>>>>>>>>> dst_start members of interleaved template.
>>>>>>>>>>
>>>>>>>>>> But, when the memory is non-contiguous it will restrict slave
>>>>>>>>>> device
>>>>>>>>>> to not submit multiple frames in a batch. This patch handles this
>>>>>>>>>> issue by allowing the slave device to send array of interleaved dma
>>>>>>>>>> templates each having a different memory location.
>>>>>>>>>>
>>>>>>>>> How fragmented could be memory in your case? Is it inefficient to
>>>>>>>>> submit separate transfers for each segment/frame?
>>>>>>>>> It will help if you could give a typical example (chunk size and gap
>>>>>>>>> in bytes) of what you worry about.
>>>>>>>>
>>>>>>>> With scatter-gather engine feature in the hardware, submitting
>>>>>>>> separate
>>>>>>>> transfers for each frame look inefficient. As an example, our DMA
>>>>>>>> engine
>>>>>>>> supports up to 16 video frames, with each frame (a typical video
>>>>>>>> frame
>>>>>>>> size) being contiguous in memory but frames are scattered into
>>>>>>>> different
>>>>>>>> locations. We could not definitely submit frame by frame as it would
>>>>>>>> be
>>>>>>>> software overhead (HW interrupting for each frame) resulting in video
>>>>>>>> lags.
>>>>>>>>
>>>>>>> IIUIC, it is 30fps and one dma interrupt per frame ... it doesn't seem
>>>>>>> inefficient at all. Even poor-latency audio would generate a higher
>>>>>>> interrupt-rate. So the "inefficiency concern" doesn't seem valid to
>>>>>>> me.
>>>>>>>
>>>>>>> Not to mean we shouldn't strive to reduce the interrupt-rate further.
>>>>>>> Another option is to emulate the ring-buffer scheme of ALSA.... which
>>>>>>> should be possible since for a session of video playback the frame
>>>>>>> buffers' locations wouldn't change.
>>>>>>>
>>>>>>> Yet another option is to use the full potential of the
>>>>>>> interleaved-xfer api as such. It seems you confuse a 'video frame'
>>>>>>> with the interleaved-xfer api's 'frame'. They are different.
>>>>>>>
>>>>>>> Assuming your one video frame is F bytes long and Gk is the gap in
>>>>>>> bytes between end of frame [k] and start of frame [k+1] and Gi != Gj
>>>>>>> for i!=j
>>>>>>> In the context of interleaved-xfer api, you have just 1 Frame of 16
>>>>>>> chunks. Each chunk is Fbytes and the inter-chunk-gap(ICG) is Gk where
>>>>>>> 0<=k<15
>>>>>>> So for your use-case .....
>>>>>>> dma_interleaved_template.numf = 1 /* just 1 frame */
>>>>>>> dma_interleaved_template.frame_size = 16 /* containing 16 chunks */
>>>>>>> ...... //other parameters
>>>>>>>
>>>>>>> You have 3 options to choose from and all should work just as fine.
>>>>>>> Otherwise please state your problem in real numbers (video-frames'
>>>>>>> size, count & gap in bytes).
>>>>>>
>>>>>> Initially I interpreted interleaved template the same. But, Lars
>>>>>> corrected me
>>>>>> in the subsequent discussion and let me put it here briefly,
>>>>>>
>>>>>> In the interleaved template, each frame represents a line of size
>>>>>> denoted by
>>>>>> chunk.size and the stride by icg. 'numf' represent number of frames
>>>>>> i.e.
>>>>>> number of lines.
>>>>>>
>>>>>> In video frame context,
>>>>>> chunk.size -> hsize
>>>>>> chunk.icg -> stride
>>>>>> numf -> vsize
>>>>>> and frame_size is always 1 as it will have only one chunk in a line.
>>>>>>
>>>>> But you said in your last post
>>>>> "with each frame (a typical video frame size) being contiguous in
>>>>> memory"
>>>>> ... which is not true from what you write above. Anyways, my first 2
>>>>> suggestions still hold.
>>>>
>>>> Yes, each video frame is contiguous and they can be scattered.
>>>>
>>> I assume by contiguous frame you mean as in framebuffer? Which is an
>>> array of bytes.
>>> If yes, then you should do as I suggest first, frame_size=16 and numf=1.
>>
>> I think am confusing you. I would like to explain with an example. Lets
>> say
>> each video frame is 4k size starting at address 0x10004000 (-0x10005000) and
>> other frame at 0x20002000 (-0x20003000), and so on.
>>
> As I said plz dont confuse video frame with DMA frame.... in video
> frame the stride is constant(zero or not) whereas in DMA context the
> stride must be zero for the frame to be called contiguous.
>
>> So, the frames are
>> scattered in memory and as the template doesnt allow multiple src_start/
>> dst_start we could not use single template to fill the HW descriptors (of
>> frames). So, I feel your suggestion might not work if the frames are
>> scattered.
>> Also, how could we get 'vsize' value in your approach?
>>
> The client driver(video driver) should know the frame parameters.
> Practically you'll have to populate 16 transfer templates (frame
> attributes and locations won't change for a session) and submit to be
> transferred _cyclically_.
>
>> More importantly,
>> we are overriding the semantics of interleaved template members.
>>
> Not at all. Interleaved-dma isn't meant for only constant stride/icg
> transfers. Rather it's for identical frames with random strides.
>
>>
>>>
>>> If no, then it seems you are already doing the right thing.... the
>>> ring-buffer scheme. Please share some stats how the current api is
>>> causing you overhead because that is a very common case (many
>>> controllers support LLI) and you have 467ms (@30fps with 16-frames
>>> ring-buffer) to queue in before you see any frame drop.
>>
>> As I mentioned earlier in the thread, our hardware has a SG engine where by
>> we could send multiple frames in a batch. Using the original implementation
>> of interleaved API, we have three options to transfer.
>>
>> One is to send frame by frame to the hardware. We get a async_tx desc for
>> each frame and then we submit it to hardware triggering it to transfer this
>> BD.
>> We queue the next descriptor on the pending queue and whenever there is
>> a completion interrupt we submit the next BD on this queue to hardware. In
>> this implementation we are not efficiently using the SG engine in the
>> hardware
>> as we transferring frame by frame even HW allows us to transfer multiple
>> frames.
>>
> Sending one frame at a time will likely cause jitters. That shouldn't be done.
>
>> The second option is to queue all the BDs until the maximum frames that HW
>> is
>> capable to transfer in a batch and then submit to SG engine in the HW. With
>> this approach I feel there will be additional software overhead to track the
>> number
>> of maximum transfers and few additional cycles to release the cookie of each
>> desc.
>> Here, each desc represents a frame.
>>
> APIs are written for the gcd of h/w. We should optimize only for
> majority and here isn't even anything gained after changing the api.
> What you want to 'optimize' has been there since ever. Nobody
> considers that overhead. BTW you don't even want to spend a few 'extra
> cycles' but what about every other platform that doesn't support this
> and will have to scan for such transfer requests?
>
>> The last option is the current implementation of the driver along with this
>> change in
>> API. It will allow us to send array of interleaved templates wherein we
>> could allocate
>> a single async desc which will handle multiple frames (or segments) and just
>> submit
>> this desc to HW. Then we program the current to first frame, tail to last
>> frame and
>> HW will complete this transfer. Here, each desc represents multiple frames.
>>
>> My point here is the driver should use hardware resources efficiently and I
>> feel the
>> driver will be inefficient if we dont use them.
>>
> I am afraid you are getting carried away by the 'awesomeness' of your
> hardware. RingBuffers/Cyclic transfers are meant for cases just like
> yours.
> Consider the following ... if you queue 16 frames and don't care to
> track before all are transmitted, you'll have very high latency. Video
> seeks will take unacceptably long and give the impression of a slow
> system. Whereas if you get callbacks for each frame rendered, you
> could updates frames from the next one thereby having very quick
> response time.
Not at all, at least in our hardware, when we submit transfers it is most
likely to be completed. There are few errors, but mostly are recoverable
and it doesnt stop the transfer unless there is a critical error which needs
a reset to the whole system. So, at least in my use case there will be
no latency. I am not saying hardware is great, but it is the IP implementation.
Regarding this API change, I had earlier explained my use case in my
v2 thread.
Lars and Vinod came up with this resolution to allow array of
interleaved templates.
I also feel this is reasonable change for the subsystem.
Lars/Vinod, could you also comment on this thread for an early resolution?
Based on everyone's opinion I can take a call for my v4 patch.
Thanks
Srikanth
>
> Anyways there are already ways to achieve what you want (however a bad
> idea that might be). So I am not convinced the change to api is any
> useful (your hardware won't run any more efficiently with the change).
>
> Regards,
> Jassi
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
^ permalink raw reply
* Re: [PATCH 2/2] ARM: dts: duovero-parlor: Add HDMI output
From: Tomi Valkeinen @ 2014-02-26 14:35 UTC (permalink / raw)
To: Russell King - ARM Linux
Cc: linux-arm-kernel@lists.infradead.org, devicetree@vger.kernel.org,
Arnd Bergmann, Tony Lindgren, Sebastian Reichel, Laurent Pinchart,
Benoît Cousson, Javier Martinez Canillas,
linux-omap@vger.kernel.org, Florian Vaussard, Ash Charles
In-Reply-To: <20140226132856.GB27282@n2100.arm.linux.org.uk>
[-- Attachment #1.1: Type: text/plain, Size: 7137 bytes --]
On 26/02/14 15:28, Russell King - ARM Linux wrote:
> On Wed, Feb 26, 2014 at 02:44:02PM +0200, Tomi Valkeinen wrote:
>>> Also - DRM is not going to ever support hotplugging components - this
>>> was discussed at kernel summit last year and David Airlie was quite
>>
>> Ok. Very odd stance. Maybe there's a reason for it that I just don't see.
>
> DRM is like ALSA - it's a card level subsystem. All components have
> to be present before the card level is brought up for the subsystem to
> function correctly.
Well, yes, at the moment. I don't know what his message was, but if it
was "DRM won't get hotplug, even if someone would do it properly", that
sounds just silly.
>> But if I'm not mistaken, it suffers from the problems above, when there
>> are multiple independent pipelines (simultaneous or non-simultaneous)
>> handled by the same IPs.
>
> It may "suffer from the problems above" that you've raised, but that's
> by explicit design of it - because that's what subsystems like DRM and
> ALSA require, and this is _precisely_ the problem it's solving.
>
> It's solving the "subsystem requires a stable view of hardware components,
> but we have multiple devices and drivers which need probing" problem.
And that's good. What I'd like to avoid is developers using the
component helpers, and designing the DT just for that use case,
preventing the use of a possible future framework.
The example in your component helper commit:
imx-drm {
compatible = "fsl,drm";
crtcs = <&ipu1>;
connectors = <&hdmi>;
};
How would that be extended if one imx board has an external HDMI
encoder? Or maybe the board has an external HDMI encoder, and also a
separate level-shifter/ESD chip like some OMAP boards have. Or maybe a
board has two displays connected to one imx LCD output, and a GPIO is
used to switch between the used display.
Rephrasing: How would those DT bindings be extended to allow arbitrarily
long or complex display pipelines?
The proposed OMAP DSS and CDF DT bindings try to allow all the above cases.
So in my opinion, using component helpers is good, but it'd be important
to make sure the DT bindings for all platforms are future proof, and
also compatible so that we can share encoder/panel drivers.
>> And, while I may be mistaken, it sounds that the component helpers leave
>> mostly everything up to the display drivers. Everyone devising their own
>> way to describe the hardware in DT, and the connections between the
>> components. Of course, the core component system shouldn't define
>> anything DT related, as it doesn't. But that part is still needed, which
>> is where CDF comes in.
>
> Sigh. It's very easy for people to get the wrong end of the stick.
>
> What the component helpers do is provide a _subsystem_ _independent_
> method of collecting a set of devices together and binding them to the
> drivers at an appropriate time, in a way that is _completely_ independent
> of whether you're using platform data, DT, ACPI, or whatever other
> hardware description language comes along.
Yep, that's what I meant with "Of course, the core component system
shouldn't define anything DT related, as it doesn't.". Maybe that's not
even English, so my bad =).
> It's up to the users of this to define how components are grouped
> together, whether that be at the subsystem level or at the driver
> level - whatever is appropriate.
>
> If a subsystem (eg, a display subsystem) wants to define "this is how
> you define in DT the bindings between all components" and provide its
> own hook for the "add_components" callback which does this, then it's
> at liberty to do that.
>
> If we can come up with a generic way to describe how all the components
> in a display subsystem should be connected together, then great - but
> that needs to happen very quickly. Philipp Zabel is working on replacing
> the imx-drm binding method right now for 3.15, and is probably completely
> unaware of anything that's been talked about here. I need to sort out
Yes, I just pinged him a few hours ago about this. I've been ill for a
few weeks, so I'm catching up on emails, but I want to sync with him
asap to see if the OMAP DSS side and his imx series have things in common.
> Armada DRM at some point to use the component stuff, which includes
> sorting out TDA998x for DT - which again needs to be done in such a way
> that it follows a common theme.
BeagleBoneBlack has TDA998x, so I'm also very interested in that.
>> So with hotplug, a new fbdev or a combination of drm crtcs, encoders,
>> etc, could appear even after the initial probe of the display controller.
>
> This is the exact situation that David is opposed to. DRM, like ALSA,
> whats to have a stable view of hardware - once the drm_device has been
> created and probed, no further changes to it are allowed.
>
> Certainly no changes to the CRTCs will _ever_ be permitted, because it
> completely destroys the user API for referecing which encoders can be
> associated with which CRTCs - not only at the kernel level, but also the
> Xorg and Xrandr level too. That's done via a bitmask of allowable CRTCs,
> where bit 0 refers to the first CRTC, bit 1 to the second and so on.
> That's propagated all the way through to userspace, right through the Xorg
> interfaces to applications.
>
> Connectors and encoders are fixed at the moment after initial probe time
> in DRM due to the way the fbdev emulation layer works. There's also issues
> there concerning bitmasks for which connectors can be cloned onto other
> connectors which follows the same pattern as above - and again, that
> propagates all the way through userspace.
>
> So, if this is going to get fixed, there has to be a desire to break
> userspace quite badly, and there is no such desire to do that.
>
> For instance, let's say that Xorg is up and running, and you have the
> gnome applet for configuring your display open, and you have two CRTCs.
> Then the first CRTC is removed from the system, resulting in CRTC 1
> becoming CRTC 0 in the kernel. What happens...
>
> Think the same thing through for a system with three connectors, A, B, C
> numbered 0, 1, and 2 respectively. A and be cloned onto B. Now connector
> A is removed, meaning B and C appear to become numbers 0 and 1 in the
> kernel...
I specifically said "hot-unplug not needed". Fbs, crtcs, etc. could only
appear, never to be removed individually. I don't see much benefit in
supporting hot-unplug, but I see much benefit with hot-plug. And I'm
sure there could be problems with only hot-plug, but I'd bet they are
much simpler if we never remove the components individually.
Usually, all the hot-plugging would happen before the rootfs is mounted.
However, it'd still be possible to load a display driver as a module
later, as long as any user (say, X) would be loaded after that.
Do you see that model as overly problematic, possibly breaking the
userspace API?
Tomi
[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 901 bytes --]
[-- Attachment #2: Type: text/plain, Size: 176 bytes --]
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH RESEND v10 0/7] mmc: omap_hsmmc: pbias dt and cleanup
From: Balaji T K @ 2014-02-26 14:43 UTC (permalink / raw)
To: florian.vaussard
Cc: linux-omap, bcousson, devicetree, linux-mmc, chris, tony, broonie,
Stefan Roese
In-Reply-To: <530DF453.40201@epfl.ch>
On Wednesday 26 February 2014 07:34 PM, Florian Vaussard wrote:
> Hi,
>
> On 02/19/2014 03:56 PM, Balaji T K wrote:
>> Few cleanups to reduce code indent,
>> Add pbias_regulator support and adapt omap_hsmmc to use pbias regulator
>> to configure required voltage on mmc1 pad(SD card) i/o rails on OMAP SoCs.
>>
>
> I tested on both OMAP3630 (Overo Storm) and OMAP4430 (DuoVero). The
> rootfs is mounted on mmc1 and works as usual. Here is what I can see:
>
> - pbias-supply is parsed from DT
> - VMMC1 is set to 3V
> - According to the debugfs entry, we are working at 3V signaling
> - By dumping CONTROL_PBIASLITE, bit MMC1_PBIASLITE_VMODE
> (PBIASLITEVMODE0 on OMAP3) is set to 1 (-> 3V)
>
> Do you see any other tests that I could run to validate your patches?
Nope, Stefan has tested the other use case of detecting sd card at kernel
without any sd card/pbias activity at u-boot.
> Otherwise:
>
> Tested-by: Florian Vaussard <florian.vaussard@epfl.ch>
>
Thanks Florian, Stefan for Testing.
^ permalink raw reply
* RE: [PATCH v4] can: xilinx CAN controller support.
From: Appana Durga Kedareswara Rao @ 2014-02-26 14:46 UTC (permalink / raw)
To: Marc Kleine-Budde, wg-5Yr1BZd7O62+XT7JhA+gdA@public.gmane.org,
Michal Simek,
grant.likely-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org,
robh+dt-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org,
linux-can-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
Cc: netdev-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org,
linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
In-Reply-To: <530DEA7C.6010609-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
Hi Marc,
> -----Original Message-----
> From: Marc Kleine-Budde [mailto:mkl@pengutronix.de]
> Sent: Wednesday, February 26, 2014 6:52 PM
> To: Appana Durga Kedareswara Rao; wg@grandegger.com; Michal Simek;
> grant.likely@linaro.org; robh+dt@kernel.org; linux-can@vger.kernel.org
> Cc: netdev@vger.kernel.org; linux-arm-kernel@lists.infradead.org; linux-
> kernel@vger.kernel.org; devicetree@vger.kernel.org
> Subject: Re: [PATCH v4] can: xilinx CAN controller support.
>
> On 02/26/2014 02:07 PM, Appana Durga Kedareswara Rao wrote:
> >> This loop looks broken. Can you explain how it works.
> >>
> >> What it shoud do is:
> >> We have put (priv->tx_head - priv->tx_tail) CAN frames into the FIFO.
> >> This means at maximum there could be this amount of CAN frames which
> >> have been successfully transmitted. For every cycle in this while
> >> loop you
> >> should:
> >> a) check if a CAN frame has successfully been transmitted
> >> (as this CAN core uses a FIFO it should be "oldest")
> >> A read_reg() of some kind is missing in your loop.
> >> b) if needed, remove this event from the FIFO or
> >> mark the interrupt as done. Whatever you hardware needs.
> >> c) update your statistics
> >> d) Use can_get_echo_skb to push this frame into the networking stack
> >> e) As a CAN frame has been transmitted successfully, wake the tx_queue.
> >>
> >>> + while (priv->tx_head - priv->tx_tail > 0) {
> >>> + if (isr & XCAN_IXR_TXFLL_MASK) {
> >>> + priv->write_reg(priv, XCAN_ICR_OFFSET,
> >>> + XCAN_IXR_TXFLL_MASK);
> >>> + netif_stop_queue(ndev);
> >>
> >> Why do you stop the queue here? A CAN frame has successfully been
> >> transmitted, there should be room in the FIFO.
> >>
> >>> + break;
> >>> + }
> >>> + can_get_echo_skb(ndev, priv->tx_tail %
> >>> + priv->xcan_echo_skb_max_tx);
> >>> + priv->tx_tail++;
> >>> + }
> >>> +
> >
> > The below are the bit fields available for the Transmit FIFO.
> > 1) In the ISR(interrupt status register) Tx Ok interrupt and Tx fifo full
> interrupt.
> > 2) in the SR(Status Register) Tx fifo full condition.
> >
> >
> > I am modifying the entire tx interrupt logic to like below.
> >
> > static void xcan_tx_interrupt(struct net_device *ndev, u32 isr) {
> > struct xcan_priv *priv = netdev_priv(ndev);
> > struct net_device_stats *stats = &ndev->stats;
> >
> > while (priv->tx_head - priv->tx_tail > 0) {
> > if (isr & XCAN_IXR_TXFLL_MASK) {
> > priv->write_reg(priv, XCAN_ICR_OFFSET,
> > XCAN_IXR_TXFLL_MASK);
> > break;
> > }
> > can_get_echo_skb(ndev, priv->tx_tail %
> > priv->xcan_echo_skb_max_tx);
> > priv->tx_tail++;
> > stats->tx_packets++;
> > netif_wake_queue(ndev);
> > can_led_event(ndev, CAN_LED_EVENT_TX);
> >
> > }
>
> You just need to wake the queue once.
Ok
>
> > }
> >
> >
> > Are you Ok with the above logic?
>
> No, how can you tell how many frames have been transmitted?
There is no register to read how many can frames are transmitted.
The only way to know Is by reading this parameter (stats->tx_packets++;) through ip command
ip -d -s link show can
Or using ifconfig command.
At the h/w level it can transmit Max upto 64 packets (Max fifo depth)
We need to monitor the Tx fifo full bit in the ISR(Interrupt Status Register) or Tx fifo full bit in the SR(Status Register) and if it is full we need to stop the queue that is
I am doing in the _xmit by reading the status register before proceeding the packet transmission.
Regards,
Kedar.
>
> Marc
>
> --
> Pengutronix e.K. | Marc Kleine-Budde |
> Industrial Linux Solutions | Phone: +49-231-2826-924 |
> Vertretung West/Dortmund | Fax: +49-5121-206917-5555 |
> Amtsgericht Hildesheim, HRA 2686 | http://www.pengutronix.de |
This email and any attachments are intended for the sole use of the named recipient(s) and contain(s) confidential information that may be proprietary, privileged or copyrighted under applicable law. If you are not the intended recipient, do not read, copy, or forward this email message or any attachments. Delete this email message and any attachments immediately.
^ permalink raw reply
* Re: [RFC PATCH] [media]: of: move graph helpers from drivers/media/v4l2-core to drivers/of
From: Philipp Zabel @ 2014-02-26 14:48 UTC (permalink / raw)
To: Grant Likely
Cc: Sascha Hauer, Rob Herring, Russell King - ARM Linux,
Mauro Carvalho Chehab, Rob Herring, Sylwester Nawrocki,
Laurent Pinchart, Tomi Valkeinen, Kyungmin Park,
linux-kernel@vger.kernel.org, linux-media@vger.kernel.org,
devicetree@vger.kernel.org, Philipp Zabel
In-Reply-To: <20140226110114.CF2C7C40A89@trevor.secretlab.ca>
Hi Grant,
thank you for the comments.
Am Mittwoch, den 26.02.2014, 11:01 +0000 schrieb Grant Likely:
> On Mon, 24 Feb 2014 18:36:29 +0100, Philipp Zabel <p.zabel@pengutronix.de> wrote:
> > Am Dienstag, den 18.02.2014, 16:26 +0000 schrieb Grant Likely:
> > > >
> > > > You can find it under Documentation/devicetree/bindings/media/video-interfaces.txt
> > >
> > > Okay, I think I'm okay with moving the helpers, but I will make one
> > > requirement. I would like to have a short binding document describing
> > > the pattern being used. The document above talks a lot about video
> > > specific issues, but the helpers appear to be specifically about the
> > > tree walking properties of the API.
> >
> > Reusing the non-video-secific parts of video-interfaces.txt, how about
> > the following:
>
> This is good, but I have some comments. This document describes itself
> as the common way for doing a device graph within the device tree, but
> there is already a well established pattern for device graphs that is
> used by the interrupts-extended, clocks and other bindings. Those are
> all domain-specific bindings, but the core concept is one device uses
> a resource provided by another device. The resource references construct
> a graph independent from the natural FDT node graph. (ie. the interrupts
> binding forms the interrupt graph. Same for the clock binding).
>
> So, while this binding does describe a pattern for separate device
> graphs, it is by no means the only common way of doing so.
>
> I would like the document to acknowledge the difference from the
> phandle+args pattern used elsewhere and a description of when it would
> be appropriate to use this instead of a simpler binding.
Alright. The main point of this binding is that the devices may have
multiple distinct ports that each can be connected to other devices.
So, contrary to the other graph bindings, devices are not simple nodes
in a directed graph. For example a single capture device with two
separate inputs connected to camera sensors:
,--------.
| cam [port]-. ,-----------------.
`--------´ `-[port@0] capture |
,---------. ,-[port@1] interface |
| cam [port]-´ `-----------------´
`---------´
Or two separate display controllers with two parallel outputs each, a
4-port multiplexer, and an encoder (e.g. lvds):
,------------------.
| display [port@0]--. ,-------------.
| controller [port@1]-. `-[port@0] |
`------------------´ `--[port@1] mux |
,------------------. ,--[port@2] | ,-------------.
| display [port@0]-´ ,-[port@3] [port@4]--[port] encoder |
| controller [port@1]--´ `-------------´ `-------------´
`------------------´
Optionally, the same with the multiplexer integrated into the
encoder device:
,------------------.
| display [port@0]--. ,----------------.
| controller [port@1]-. `-[port@0] |
`------------------´ `--[port@1] encoder |
,------------------. ,--[port@2] with mux |
| display [port@0]-´ ,-[port@3] |
| controller [port@1]--´ `----------------´
`------------------´
Or display controller, encoder, and panel:
,--------. ,---------.
| dc [port]--[port@0] |
`--------´ | encoder | ,-----------.
| [port@1]--[port] panel |
`---------´ `-----------´
> > "Common bindings for device graphs"
> >
> > General concept
> > ---------------
> >
> > The hierarchical organisation of the device tree is well suited to describe
> > control flow to devices, but data flow between devices that work together to
> > form a logical compound device can follow arbitrarily complex graphs.
>
> I would argue that this pattern isn't necessarily restricted to data
> flow descriptions. It wants to describe linkage between devices that are
> sufficiently complex that the simple binding doesn't do the job.
Ok. In principle it would be possible to use the same scheme for other
connections than data links.
> > The device tree graph bindings allow to describe data bus connections between
> > individual devices, that can't be inferred from device tree parent-child
> > relationships. The common bindings do not contain any information about the
> > direction or type of data flow, they just map connections. Specific properties
> > of the connections can be set depending on the type of connection. To see
> > how this binding applies to video pipelines, see for example
> > Documentation/device-tree/bindings/media/video-interfaces.txt.
>
> Even if you don't want to declare the direction of data flow, there does
> need to be some guidance as to how the binding is constructed. Does
> device A point to device B? Or the other way around? Why would someone
> choose one over the other? I don't want to see a situation where A & B
> point to each other. Things get complex if the graph is allowed to be
> cyclical.
According to video-interfaces.txt, it is expected that endpoints contain
phandles pointing to the remote endpoint on both sides. I'd like to
leave this up to the more specialized bindings, but I can see that this
makes enumerating the connections starting from each device tree node
easier, for example at probe time.
If the back links are not provided in the device tree, a device at the
receiving end of a remote-endpoint phandle can only know about connected
remote ports after the kernel parsed the whole graph and created the
back links itself.
> > Each endpoint can contain a 'remote-endpoint' phandle property that points to
> > the corresponding endpoint in the port of the remote device. Two 'endpoint'
> > nodes are linked with each other through their 'remote-endpoint' phandles.
>
> I really don't like this aspect. It is far too easy to get wrong.
On the other hand it is really easy to test for and warn about missing
or misdirected backlinks.
> Graphs should be one direction only.
But this is not what the current binding in video-interfaces.txt
describes. I don't think it is a good idea to explicitly forbid
backlinks in this binding.
regards
Philipp
^ permalink raw reply
* Re: [PATCH v4 3/3] Documentation: of: Document graph bindings
From: Tomi Valkeinen @ 2014-02-26 14:50 UTC (permalink / raw)
To: Philipp Zabel
Cc: Russell King - ARM Linux, Mauro Carvalho Chehab, Grant Likely,
Rob Herring, Sylwester Nawrocki, Laurent Pinchart, Kyungmin Park,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
linux-media-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA, Guennadi Liakhovetski
In-Reply-To: <1393426623.3248.70.camel-+qGW7pzALmz7o/J7KWpOmN53zsg1cpMQ@public.gmane.org>
[-- Attachment #1: Type: text/plain, Size: 2079 bytes --]
On 26/02/14 16:57, Philipp Zabel wrote:
> Hi Tomi,
>
> Am Mittwoch, den 26.02.2014, 15:14 +0200 schrieb Tomi Valkeinen:
>> On 25/02/14 16:58, Philipp Zabel wrote:
>>
>>> +Optional endpoint properties
>>> +----------------------------
>>> +
>>> +- remote-endpoint: phandle to an 'endpoint' subnode of a remote device node.
>>
>> Why is that optional? What use is an endpoint, if it's not connected to
>> something?
>
> This allows to include the an empty endpoint template in a SoC dtsi for
> the convenience of board dts writers. Also, the same property is
> currently listed as optional in video-interfaces.txt.
>
> soc.dtsi:
> display-controller {
> port {
> disp0: endpoint { };
> };
> };
>
> board.dts:
> #include "soc.dtsi"
> &disp0 {
> remote-endpoint = <&panel_input>;
> };
> panel {
> port {
> panel_in: endpoint {
> remote-endpoint = <&disp0>;
> };
> };
> };
>
> Any board not using that port can just leave the endpoint disconnected.
Hmm I see. I'm against that.
I think the SoC dtsi should not contain endpoint node, or even port node
(at least usually). It doesn't know how many endpoints, if any, a
particular board has. That part should be up to the board dts.
I've done this with OMAP as (much simplified):
SoC.dtsi:
dss: dss@58000000 {
status = "disabled";
};
Nothing else (relevant here). The binding documentation states that dss
has one port, and information what data is needed for the port and endpoint.
board.dts:
&dss {
status = "ok";
pinctrl-names = "default";
pinctrl-0 = <&dss_dpi_pins>;
dpi_out: endpoint {
remote-endpoint = <&tfp410_in>;
data-lines = <24>;
};
};
That's using the shortened version without port node.
Of course, it's up to the developer how his dts looks like. But to me it
makes sense to require the remote-endpoint property, as the endpoint, or
even the port, doesn't make much sense if there's nothing to connect to.
Tomi
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 901 bytes --]
^ permalink raw reply
* Re: [PATCH 00/10] pinctrl: mvebu: remove hard-coded addresses from Dove pinctrl
From: Jason Cooper @ 2014-02-26 14:53 UTC (permalink / raw)
To: Linus Walleij
Cc: Mark Rutland, Andrew Lunn, Russell King, Pawel Moll, Ian Campbell,
linux-doc-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, Rob Herring,
Rob Landley, Kumar Gala, Gregory Clement,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org,
Sebastian Hesselbarth
In-Reply-To: <CACRpkdYQa2BG_qxf5zY=7rks6O31ZF-BX0af3_u1MOgwfwxFSQ-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
On Wed, Feb 26, 2014 at 10:43:45AM +0100, Linus Walleij wrote:
> On Wed, Feb 26, 2014 at 1:09 AM, Jason Cooper <jason-NLaQJdtUoK4Be96aLqz0jA@public.gmane.org> wrote:
>
> > Sebastian has now re-organized the branches as I asked, and I confirmed
> > that the final result is the exact same as mine (diff is null).
>
> Okay!
>
> > Usually when I submit pull requests to arm-soc, they like to see the
> > branches. That way if there is an error in one of them, they just drop
> > the one branch and the others remain.
> >
> > Would you like them the same way? If so, I'll send the pulls to you
> > tomorrow.
>
> Send me one big branch with everything on it.
Sure.
> Actually, I'd prefer to pull it in, rebase and sign off each patch
> individually in my tree if that is not causing you problems.
Actually, that would mess us up pretty badly. :( One of the reasons we
take the effort to base off of -rc1 and create stable topic branches is
so that the commit IDs don't change. This way, all the patches needed
to boot the new mvebu SoCs (code intended for v3.15) can be boot tested
from the mvebu/for-next branch, which is merge-tested and randconfig
tested in linux-next. This all happens _before_ the merge window for
v3.15.
There have been huge benefits since we started doing this. In fact,
just yesterday I committed three patches to fix issues discovered as a
result of this process:
edd9d3cffc90 watchdog: orion_wdt: Use %pa to print 'phys_addr_t'
1b82af4f1749 ARM: kirkwood: select dtbs based on SoC
a02dd0271d01 ARM: mvebu: select dtbs from MACH_ARMADA_*
The first was discovered by Olof's autobuilder, and the last two were
discovered by Kevin's boot farm.
If you rebase the branch, I'll have to drop it from our for-next tree to
prevent conflicts in linux-next with your -next branch. Which means no
one will be able to boot test the new SoCs without going through a lot
of hunting to re-collect all the branches.
The advantage of having mvebu/for-next in addition to linux-next is that
should there be a boot failure, we can quickly determine whether it is a
result of the mvebu code (mvebu/for-next fails) or something outside of
mvebu (only linux-next fails).
By keeping the commit IDs the same, the same branch can be in multiple
trees all getting merged into linux-next. Currently, mvebu does this
with arm-soc, clk, and irqchip. In those cases, those maintainers are
the ones who send the pull requests to Linus, not us.
> That way it is visible that the patches were funneled through pin
> control.
I'm a little confused by this. Once you merge the branch into one of
yours, that merge commit is a part of the history. In fact, the branch
is still intact for eternity. So by merging the branch, adding other
patches, and signing the tip of the result, it should be clear it came
through pinctrl, no?
thx,
Jason.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH v4 3/3] Documentation: of: Document graph bindings
From: Philipp Zabel @ 2014-02-26 14:57 UTC (permalink / raw)
To: Tomi Valkeinen
Cc: Russell King - ARM Linux, Mauro Carvalho Chehab, Grant Likely,
Rob Herring, Sylwester Nawrocki, Laurent Pinchart, Kyungmin Park,
linux-kernel, linux-media, devicetree, Guennadi Liakhovetski
In-Reply-To: <530DE8A9.9050809@ti.com>
Hi Tomi,
Am Mittwoch, den 26.02.2014, 15:14 +0200 schrieb Tomi Valkeinen:
> On 25/02/14 16:58, Philipp Zabel wrote:
>
> > +Optional endpoint properties
> > +----------------------------
> > +
> > +- remote-endpoint: phandle to an 'endpoint' subnode of a remote device node.
>
> Why is that optional? What use is an endpoint, if it's not connected to
> something?
This allows to include the an empty endpoint template in a SoC dtsi for
the convenience of board dts writers. Also, the same property is
currently listed as optional in video-interfaces.txt.
soc.dtsi:
display-controller {
port {
disp0: endpoint { };
};
};
board.dts:
#include "soc.dtsi"
&disp0 {
remote-endpoint = <&panel_input>;
};
panel {
port {
panel_in: endpoint {
remote-endpoint = <&disp0>;
};
};
};
Any board not using that port can just leave the endpoint disconnected.
On the other hand, the same could be achieved with Heiko Stübner's
conditional nodes dtc patch:
soc.dtsi:
display-controller {
port {
/delete-unreferenced/ disp0: endpoint { };
};
};
> Also, if this is being worked on, I'd like to propose the addition of
> simpler single-endpoint cases which I've been using with OMAP DSS. So if
> there's just a single endpoint for the device, which is very common, you
> can have just:
>
> device {
> ...
> endpoint { ... };
> };
>
> However, I guess that the patch just keeps growing and growing, so maybe
> it's better to add such things later =).
Yes, that looks good. I'd be happy if we could add this in a second step
as a backwards compatible simplification.
regards
Philipp
^ permalink raw reply
* Re: [PATCH v3 1/3] dma: Support multiple interleaved frames with non-contiguous memory
From: Jassi Brar @ 2014-02-26 15:02 UTC (permalink / raw)
To: Srikanth Thokala
Cc: Williams, Dan J, Koul, Vinod, Michal Simek, Grant Likely,
Rob Herring, devicetree-u79uwXL29TY76Z2rM5mHXA, Levente Kurusa,
Lars-Peter Clausen, lkml, dmaengine-u79uwXL29TY76Z2rM5mHXA,
Andy Shevchenko,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org
In-Reply-To: <CA+mB=1KHuZb14zrb8K9Hs4qQMmYdWEuHpAnwE0FMYOQYv=g5-w-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
On 26 February 2014 23:21, Srikanth Thokala <sthokal-gjFFaj9aHVfQT0dZR+AlfA@public.gmane.org> wrote:
> On Mon, Feb 24, 2014 at 7:39 AM, Jassi Brar <jaswinder.singh-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
>> On 21 February 2014 23:37, Srikanth Thokala <sthokal-gjFFaj9aHVfQT0dZR+AlfA@public.gmane.org> wrote:
>>> On Thu, Feb 20, 2014 at 3:23 PM, Jassi Brar <jaswinder.singh-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
>>> wrote:
>>>> On 20 February 2014 14:54, Srikanth Thokala <sthokal-gjFFaj9aHVfQT0dZR+AlfA@public.gmane.org> wrote:
>>>>> On Wed, Feb 19, 2014 at 12:33 AM, Jassi Brar <jaswinder.singh-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
>>>>> wrote:
>>>>>> On 18 February 2014 23:16, Srikanth Thokala <sthokal-gjFFaj9aHVfQT0dZR+AlfA@public.gmane.org> wrote:
>>>>>>> On Tue, Feb 18, 2014 at 10:20 PM, Jassi Brar
>>>>>>> <jaswinder.singh-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
>>>>>>>> On 18 February 2014 16:58, Srikanth Thokala <sthokal-gjFFaj9aHVfQT0dZR+AlfA@public.gmane.org>
>>>>>>>> wrote:
>>>>>>>>> On Mon, Feb 17, 2014 at 3:27 PM, Jassi Brar
>>>>>>>>> <jaswinder.singh-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
>>>>>>>>>> On 15 February 2014 17:30, Srikanth Thokala <sthokal-gjFFaj9aHVfQT0dZR+AlfA@public.gmane.org>
>>>>>>>>>> wrote:
>>>>>>>>>>> The current implementation of interleaved DMA API support multiple
>>>>>>>>>>> frames only when the memory is contiguous by incrementing
>>>>>>>>>>> src_start/
>>>>>>>>>>> dst_start members of interleaved template.
>>>>>>>>>>>
>>>>>>>>>>> But, when the memory is non-contiguous it will restrict slave
>>>>>>>>>>> device
>>>>>>>>>>> to not submit multiple frames in a batch. This patch handles this
>>>>>>>>>>> issue by allowing the slave device to send array of interleaved dma
>>>>>>>>>>> templates each having a different memory location.
>>>>>>>>>>>
>>>>>>>>>> How fragmented could be memory in your case? Is it inefficient to
>>>>>>>>>> submit separate transfers for each segment/frame?
>>>>>>>>>> It will help if you could give a typical example (chunk size and gap
>>>>>>>>>> in bytes) of what you worry about.
>>>>>>>>>
>>>>>>>>> With scatter-gather engine feature in the hardware, submitting
>>>>>>>>> separate
>>>>>>>>> transfers for each frame look inefficient. As an example, our DMA
>>>>>>>>> engine
>>>>>>>>> supports up to 16 video frames, with each frame (a typical video
>>>>>>>>> frame
>>>>>>>>> size) being contiguous in memory but frames are scattered into
>>>>>>>>> different
>>>>>>>>> locations. We could not definitely submit frame by frame as it would
>>>>>>>>> be
>>>>>>>>> software overhead (HW interrupting for each frame) resulting in video
>>>>>>>>> lags.
>>>>>>>>>
>>>>>>>> IIUIC, it is 30fps and one dma interrupt per frame ... it doesn't seem
>>>>>>>> inefficient at all. Even poor-latency audio would generate a higher
>>>>>>>> interrupt-rate. So the "inefficiency concern" doesn't seem valid to
>>>>>>>> me.
>>>>>>>>
>>>>>>>> Not to mean we shouldn't strive to reduce the interrupt-rate further.
>>>>>>>> Another option is to emulate the ring-buffer scheme of ALSA.... which
>>>>>>>> should be possible since for a session of video playback the frame
>>>>>>>> buffers' locations wouldn't change.
>>>>>>>>
>>>>>>>> Yet another option is to use the full potential of the
>>>>>>>> interleaved-xfer api as such. It seems you confuse a 'video frame'
>>>>>>>> with the interleaved-xfer api's 'frame'. They are different.
>>>>>>>>
>>>>>>>> Assuming your one video frame is F bytes long and Gk is the gap in
>>>>>>>> bytes between end of frame [k] and start of frame [k+1] and Gi != Gj
>>>>>>>> for i!=j
>>>>>>>> In the context of interleaved-xfer api, you have just 1 Frame of 16
>>>>>>>> chunks. Each chunk is Fbytes and the inter-chunk-gap(ICG) is Gk where
>>>>>>>> 0<=k<15
>>>>>>>> So for your use-case .....
>>>>>>>> dma_interleaved_template.numf = 1 /* just 1 frame */
>>>>>>>> dma_interleaved_template.frame_size = 16 /* containing 16 chunks */
>>>>>>>> ...... //other parameters
>>>>>>>>
>>>>>>>> You have 3 options to choose from and all should work just as fine.
>>>>>>>> Otherwise please state your problem in real numbers (video-frames'
>>>>>>>> size, count & gap in bytes).
>>>>>>>
>>>>>>> Initially I interpreted interleaved template the same. But, Lars
>>>>>>> corrected me
>>>>>>> in the subsequent discussion and let me put it here briefly,
>>>>>>>
>>>>>>> In the interleaved template, each frame represents a line of size
>>>>>>> denoted by
>>>>>>> chunk.size and the stride by icg. 'numf' represent number of frames
>>>>>>> i.e.
>>>>>>> number of lines.
>>>>>>>
>>>>>>> In video frame context,
>>>>>>> chunk.size -> hsize
>>>>>>> chunk.icg -> stride
>>>>>>> numf -> vsize
>>>>>>> and frame_size is always 1 as it will have only one chunk in a line.
>>>>>>>
>>>>>> But you said in your last post
>>>>>> "with each frame (a typical video frame size) being contiguous in
>>>>>> memory"
>>>>>> ... which is not true from what you write above. Anyways, my first 2
>>>>>> suggestions still hold.
>>>>>
>>>>> Yes, each video frame is contiguous and they can be scattered.
>>>>>
>>>> I assume by contiguous frame you mean as in framebuffer? Which is an
>>>> array of bytes.
>>>> If yes, then you should do as I suggest first, frame_size=16 and numf=1.
>>>
>>> I think am confusing you. I would like to explain with an example. Lets
>>> say
>>> each video frame is 4k size starting at address 0x10004000 (-0x10005000) and
>>> other frame at 0x20002000 (-0x20003000), and so on.
>>>
>> As I said plz dont confuse video frame with DMA frame.... in video
>> frame the stride is constant(zero or not) whereas in DMA context the
>> stride must be zero for the frame to be called contiguous.
>>
>>> So, the frames are
>>> scattered in memory and as the template doesnt allow multiple src_start/
>>> dst_start we could not use single template to fill the HW descriptors (of
>>> frames). So, I feel your suggestion might not work if the frames are
>>> scattered.
>>> Also, how could we get 'vsize' value in your approach?
>>>
>> The client driver(video driver) should know the frame parameters.
>> Practically you'll have to populate 16 transfer templates (frame
>> attributes and locations won't change for a session) and submit to be
>> transferred _cyclically_.
>>
>>> More importantly,
>>> we are overriding the semantics of interleaved template members.
>>>
>> Not at all. Interleaved-dma isn't meant for only constant stride/icg
>> transfers. Rather it's for identical frames with random strides.
>>
>>>
>>>>
>>>> If no, then it seems you are already doing the right thing.... the
>>>> ring-buffer scheme. Please share some stats how the current api is
>>>> causing you overhead because that is a very common case (many
>>>> controllers support LLI) and you have 467ms (@30fps with 16-frames
>>>> ring-buffer) to queue in before you see any frame drop.
>>>
>>> As I mentioned earlier in the thread, our hardware has a SG engine where by
>>> we could send multiple frames in a batch. Using the original implementation
>>> of interleaved API, we have three options to transfer.
>>>
>>> One is to send frame by frame to the hardware. We get a async_tx desc for
>>> each frame and then we submit it to hardware triggering it to transfer this
>>> BD.
>>> We queue the next descriptor on the pending queue and whenever there is
>>> a completion interrupt we submit the next BD on this queue to hardware. In
>>> this implementation we are not efficiently using the SG engine in the
>>> hardware
>>> as we transferring frame by frame even HW allows us to transfer multiple
>>> frames.
>>>
>> Sending one frame at a time will likely cause jitters. That shouldn't be done.
>>
>>> The second option is to queue all the BDs until the maximum frames that HW
>>> is
>>> capable to transfer in a batch and then submit to SG engine in the HW. With
>>> this approach I feel there will be additional software overhead to track the
>>> number
>>> of maximum transfers and few additional cycles to release the cookie of each
>>> desc.
>>> Here, each desc represents a frame.
>>>
>> APIs are written for the gcd of h/w. We should optimize only for
>> majority and here isn't even anything gained after changing the api.
>> What you want to 'optimize' has been there since ever. Nobody
>> considers that overhead. BTW you don't even want to spend a few 'extra
>> cycles' but what about every other platform that doesn't support this
>> and will have to scan for such transfer requests?
>>
>>> The last option is the current implementation of the driver along with this
>>> change in
>>> API. It will allow us to send array of interleaved templates wherein we
>>> could allocate
>>> a single async desc which will handle multiple frames (or segments) and just
>>> submit
>>> this desc to HW. Then we program the current to first frame, tail to last
>>> frame and
>>> HW will complete this transfer. Here, each desc represents multiple frames.
>>>
>>> My point here is the driver should use hardware resources efficiently and I
>>> feel the
>>> driver will be inefficient if we dont use them.
>>>
>> I am afraid you are getting carried away by the 'awesomeness' of your
>> hardware. RingBuffers/Cyclic transfers are meant for cases just like
>> yours.
>> Consider the following ... if you queue 16 frames and don't care to
>> track before all are transmitted, you'll have very high latency. Video
>> seeks will take unacceptably long and give the impression of a slow
>> system. Whereas if you get callbacks for each frame rendered, you
>> could updates frames from the next one thereby having very quick
>> response time.
>
> Not at all, at least in our hardware, when we submit transfers it is most
> likely to be completed. There are few errors, but mostly are recoverable
> and it doesnt stop the transfer unless there is a critical error which needs
> a reset to the whole system. So, at least in my use case there will be
> no latency. I am not saying hardware is great, but it is the IP implementation.
>
I am not talking about the error cases.
Apply your patch locally so that you queue 16frames and not get
notified upon each frame 'rendered'... now click on 'seek bar' of the
video player. See how slow it is to jump to play from the new
location. Or if the frames are to be encoded after dma transfer...
see the 'padding' that would need to be done at the end. These
concerns are common with audio subsystem using ring-buffers.
Anyways that is just FYI, I don't care how you implement your platform.
> Regarding this API change, I had earlier explained my use case in my
> v2 thread.
> Lars and Vinod came up with this resolution to allow array of
> interleaved templates.
> I also feel this is reasonable change for the subsystem.
>
I don't see you getting any better performance from your hardware and
I certainly don't find your usecase anything new (many dma controllers
support LLI and they work just fine as such). And I have already
explained how you could do, whatever you want, without this change.
So a polite NAK from me.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH v3 1/4] ARM: sunxi: Add driver for sunxi usb phy
From: Chen-Yu Tsai @ 2014-02-26 15:12 UTC (permalink / raw)
To: linux-sunxi
Cc: Kishon Vijay Abraham I, Maxime Ripard, linux-arm-kernel,
devicetree, Hans de Goede
In-Reply-To: <1393157352-21104-2-git-send-email-hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
Hi,
On Sun, Feb 23, 2014 at 8:09 PM, Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
> The Allwinner A1x / A2x SoCs have 2 or 3 usb phys which are all accessed
> through a single set of registers. Besides this there are also some other
> phy related bits which need poking, which are per phy, but shared between the
> ohci and ehci controllers, so these are also controlled from this new phy
> driver.
>
> Signed-off-by: Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> ---
> .../devicetree/bindings/phy/sun4i-usb-phy.txt | 26 ++
> drivers/phy/Kconfig | 11 +
> drivers/phy/Makefile | 1 +
> drivers/phy/phy-sun4i-usb.c | 329 +++++++++++++++++++++
> 4 files changed, 367 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/phy/sun4i-usb-phy.txt
> create mode 100644 drivers/phy/phy-sun4i-usb.c
>
[..]
> diff --git a/drivers/phy/phy-sun4i-usb.c b/drivers/phy/phy-sun4i-usb.c
> new file mode 100644
> index 0000000..31c4611
> --- /dev/null
> +++ b/drivers/phy/phy-sun4i-usb.c
[..]
> +static int sun4i_usb_phy_probe(struct platform_device *pdev)
> +{
> + struct sun4i_usb_phy_data *data;
> + struct device *dev = &pdev->dev;
> + struct device_node *np = dev->of_node;
> + void __iomem *pmu = NULL;
> + struct phy_provider *phy_provider;
> + struct reset_control *reset;
> + struct regulator *vbus;
> + struct resource *res;
> + struct phy *phy;
> + char name[16];
> + int i;
> +
> + data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
> + if (!data)
> + return -ENOMEM;
> +
> + mutex_init(&data->mutex);
> +
> + if (of_device_is_compatible(np, "allwinner,sun5i-a13-usb-phy"))
> + data->num_phys = 2;
> + else
> + data->num_phys = 3;
> +
> + if (of_device_is_compatible(np, "allwinner,sun4i-a10-usb-phy"))
> + data->disc_thresh = 3;
> + else
> + data->disc_thresh = 2;
> +
> + res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "phy_ctrl");
> + data->base = devm_ioremap_resource(dev, res);
> + if (IS_ERR(data->base))
> + return PTR_ERR(data->base);
> +
> + data->clk = devm_clk_get(dev, "usb_phy");
> + if (IS_ERR(data->clk)) {
> + dev_err(dev, "could not get usb_phy clock\n");
> + return PTR_ERR(data->clk);
> + }
> +
> + /* Skip 0, 0 is the phy for otg which is not yet supported. */
> + for (i = 1; i < data->num_phys; i++) {
> + snprintf(name, sizeof(name), "usb%d_vbus", i);
> + vbus = devm_regulator_get_optional(dev, name);
> + if (IS_ERR(vbus)) {
> + if (PTR_ERR(vbus) == -EPROBE_DEFER)
> + return -EPROBE_DEFER;
> + vbus = NULL;
> + }
> +
> + snprintf(name, sizeof(name), "usb%d_reset", i);
> + reset = devm_reset_control_get(dev, name);
> + if (IS_ERR(reset)) {
I got around to rebasing my musb tree. I see you fixed the reset
control error path. You got one.
> + dev_err(dev, "failed to get reset %s\n", name);
> + return PTR_ERR(phy);
But you missed one here. And phy is uninitialized until later,
so this will break big time.
Cheers
ChenYu
> + }
> +
> + if (i) { /* No pmu for usbc0 */
> + snprintf(name, sizeof(name), "pmu%d", i);
> + res = platform_get_resource_byname(pdev,
> + IORESOURCE_MEM, name);
> + pmu = devm_ioremap_resource(dev, res);
> + if (IS_ERR(pmu))
> + return PTR_ERR(pmu);
> + }
> +
> + phy = devm_phy_create(dev, &sun4i_usb_phy_ops, NULL);
> + if (IS_ERR(phy)) {
> + dev_err(dev, "failed to create PHY %d\n", i);
> + return PTR_ERR(phy);
> + }
> +
> + data->phys[i].phy = phy;
> + data->phys[i].pmu = pmu;
> + data->phys[i].vbus = vbus;
> + data->phys[i].reset = reset;
> + data->phys[i].index = i;
> + phy_set_drvdata(phy, &data->phys[i]);
> + }
> +
> + dev_set_drvdata(dev, data);
> + phy_provider = devm_of_phy_provider_register(dev, sun4i_usb_phy_xlate);
> + if (IS_ERR(phy_provider))
> + return PTR_ERR(phy_provider);
> +
> + return 0;
> +}
> +
> +static const struct of_device_id sun4i_usb_phy_of_match[] = {
> + { .compatible = "allwinner,sun4i-a10-usb-phy" },
> + { .compatible = "allwinner,sun5i-a13-usb-phy" },
> + { .compatible = "allwinner,sun7i-a20-usb-phy" },
> + { },
> +};
> +MODULE_DEVICE_TABLE(of, sun4i_usb_phy_of_match);
> +
> +static struct platform_driver sun4i_usb_phy_driver = {
> + .probe = sun4i_usb_phy_probe,
> + .driver = {
> + .of_match_table = sun4i_usb_phy_of_match,
> + .name = "sun4i-usb-phy",
> + .owner = THIS_MODULE,
> + }
> +};
> +module_platform_driver(sun4i_usb_phy_driver);
> +
> +MODULE_DESCRIPTION("Allwinner sun4i USB phy driver");
> +MODULE_AUTHOR("Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>");
> +MODULE_LICENSE("GPL v2");
> --
> 1.9.0
>
> --
> You received this message because you are subscribed to the Google Groups "linux-sunxi" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to linux-sunxi+unsubscribe-/JYPxA39Uh5TLH3MbocFF+G/Ez6ZCGd0@public.gmane.org
> For more options, visit https://groups.google.com/groups/opt_out.
^ permalink raw reply
* Re: [PATCH v4 1/3] [media] of: move graph helpers from drivers/media/v4l2-core to drivers/of
From: Philipp Zabel @ 2014-02-26 15:24 UTC (permalink / raw)
To: Grant Likely
Cc: Russell King - ARM Linux, Mauro Carvalho Chehab, Rob Herring,
Sylwester Nawrocki, Laurent Pinchart, Tomi Valkeinen,
Kyungmin Park, linux-kernel, linux-media, devicetree,
Guennadi Liakhovetski, Philipp Zabel
In-Reply-To: <20140226113729.A9D5AC40A89@trevor.secretlab.ca>
Hi Grant,
Am Mittwoch, den 26.02.2014, 11:37 +0000 schrieb Grant Likely:
[...]
> > drivers/media/v4l2-core/v4l2-of.c | 117 ----------------------
> > drivers/of/Makefile | 1 +
> > drivers/of/of_graph.c | 134 ++++++++++++++++++++++++++
>
> Nah. Just put it into drivers/of/base.c. This isn't a separate subsystem
> and the functions are pretty basic.
Ok.
[...]
> > +struct device_node *of_graph_get_next_endpoint(const struct device_node *parent,
> > + struct device_node *prev)
> > +{
> > + struct device_node *endpoint;
> > + struct device_node *port = NULL;
> > +
> > + if (!parent)
> > + return NULL;
> > +
> > + if (!prev) {
> > + struct device_node *node;
> > + /*
> > + * It's the first call, we have to find a port subnode
> > + * within this node or within an optional 'ports' node.
> > + */
> > + node = of_get_child_by_name(parent, "ports");
> > + if (node)
> > + parent = node;
> > +
> > + port = of_get_child_by_name(parent, "port");
>
> If you've got a "ports" node, then I would expect every single child to
> be a port. Should not need the _by_name variant.
The 'ports' node is optional. It is only needed if the parent node has
its own #address-cells and #size-cells properties. If the ports are
direct children of the device node, there might be other nodes than
ports:
device {
#address-cells = <1>;
#size-cells = <0>;
port@0 {
endpoint { ... };
};
port@1 {
endpoint { ... };
};
some-other-child { ... };
};
device {
#address-cells = <x>;
#size-cells = <y>;
ports {
#address-cells = <1>;
#size-cells = <0>;
port@0 {
endpoint { ... };
};
port@1 {
endpoint { ... };
};
};
some-other-child { ... };
};
The helper should find the two endpoints in both cases.
Tomi suggests an even more compact form for devices with just one port:
device {
endpoint { ... };
some-other-child { ... };
};
> It seems that this function is merely a helper to get all grandchildren
> of a node (with some very minor constraints). That could be generalized
> and simplified. If the function takes the "ports" node as an argument
> instead of the parent, then there is a greater likelyhood that other
> code can make use of it...
>
> Thinking further. I think the semantics of this whole feature basically
> boil down to this:
>
> #define for_each_grandchild_of_node(parent, child, grandchild) \
> for_each_child_of_node(parent, child) \
> for_each_child_of_node(child, grandchild)
>
> Correct? Or in this specific case:
>
> parent = of_get_child_by_name(np, "ports")
> for_each_grandchild_of_node(parent, child, grandchild) {
> ...
> }
Hmm, that would indeed be a bit more generic, but it doesn't handle the
optional 'ports' subnode and doesn't allow for other child nodes in the
device node.
> Finally, looking at the actual patch, is any of this actually needed.
> All of the users updated by this patch only ever handle a single
> endpoint. Have I read it correctly? Are there any users supporting
> multiple endpoints?
Yes, mainline currently only contains simple cases. I have posted i.MX6
patches that use this scheme for the output path:
http://www.spinics.net/lists/arm-kernel/msg310817.html
http://www.spinics.net/lists/arm-kernel/msg310821.html
> > +
> > + if (port) {
> > + /* Found a port, get an endpoint. */
> > + endpoint = of_get_next_child(port, NULL);
> > + of_node_put(port);
> > + } else {
> > + endpoint = NULL;
> > + }
> > +
> > + if (!endpoint)
> > + pr_err("%s(): no endpoint nodes specified for %s\n",
> > + __func__, parent->full_name);
> > + of_node_put(node);
>
> If you 'return endpoint' here, then the else block can go down a level.
Note that this patch is a straight move of existing code.
I can follow up with code beautification and ...
> > + } else {
> > + port = of_get_parent(prev);
> > + if (!port)
> > + /* Hm, has someone given us the root node ?... */
> > + return NULL;
>
> WARN_ONCE(). That's a very definite coding failure if that happens.
... with a fix for this.
> > +
> > + /* Avoid dropping prev node refcount to 0. */
> > + of_node_get(prev);
> > + endpoint = of_get_next_child(port, prev);
> > + if (endpoint) {
> > + of_node_put(port);
> > + return endpoint;
> > + }
> > +
> > + /* No more endpoints under this port, try the next one. */
> > + do {
> > + port = of_get_next_child(parent, port);
> > + if (!port)
> > + return NULL;
> > + } while (of_node_cmp(port->name, "port"));
> > +
> > + /* Pick up the first endpoint in this port. */
> > + endpoint = of_get_next_child(port, NULL);
> > + of_node_put(port);
> > + }
> > +
> > + return endpoint;
> > +}
> > +EXPORT_SYMBOL(of_graph_get_next_endpoint);
> > +
> > +/**
> > + * of_graph_get_remote_port_parent() - get remote port's parent node
> > + * @node: pointer to a local endpoint device_node
> > + *
> > + * Return: Remote device node associated with remote endpoint node linked
> > + * to @node. Use of_node_put() on it when done.
> > + */
> > +struct device_node *of_graph_get_remote_port_parent(
> > + const struct device_node *node)
> > +{
> > + struct device_node *np;
> > + unsigned int depth;
> > +
> > + /* Get remote endpoint node. */
> > + np = of_parse_phandle(node, "remote-endpoint", 0);
> > +
> > + /* Walk 3 levels up only if there is 'ports' node. */
>
> This needs a some explaining. My reading of the binding pattern is that
> it will always be a fixed number of levels. Why is this test fuzzy?
[...]
See above. The ports subnode level is optional. In most cases, the port
nodes will be direct children of the device node.
Walking up 3 levels from the endpoint node will return the device if
there was a ports node. If there is no ports node, we only have to walk
up two levels.
regards
Philipp
^ permalink raw reply
* [PATCH v2 0/8] Input: pixcir_i2c_ts: Add Type-B Multi-touch and DT support
From: Roger Quadros @ 2014-02-26 15:27 UTC (permalink / raw)
To: dmitry.torokhov
Cc: rydberg, jcbian, balbi, dmurphy, mugunthanvnm, linux-input,
linux-kernel, devicetree, Roger Quadros
Hi,
This series does the following
- use devres managed resource allocations
- convert to Type-B multi touch protocol
- support upto 5 fingers with hardware supplied tracking IDs
- device tree support
Changelog:
v2:
- Addressed review comments and re-arranged patch order
v1:
- http://article.gmane.org/gmane.linux.kernel/1616417
cheers,
-roger
---
Roger Quadros (8):
Input: pixcir_i2c_ts: Use devres managed resource allocations
Input: pixcir_i2c_ts: Initialize interrupt mode and power mode
Input: pixcir_i2c_ts: Get rid of pdata->attb_read_val()
Input: pixcir_i2c_ts: Use Type-B Multi-Touch protocol
Input: pixcir_i2c_ts: support upto 5 fingers and hardware provided
tracking IDs
Input: pixcir_i2c_ts: Implement wakeup from suspend
Input: pixcir_i2c_ts: Add device tree support
ARM: dts: am43x-epos-evm: Correct Touch controller info
.../bindings/input/touchscreen/pixcir_i2c_ts.txt | 26 ++
.../devicetree/bindings/vendor-prefixes.txt | 1 +
arch/arm/boot/dts/am43x-epos-evm.dts | 13 +-
drivers/input/touchscreen/pixcir_i2c_ts.c | 510 ++++++++++++++++++---
include/linux/input/pixcir_ts.h | 56 ++-
5 files changed, 545 insertions(+), 61 deletions(-)
create mode 100644 Documentation/devicetree/bindings/input/touchscreen/pixcir_i2c_ts.txt
--
1.8.3.2
^ permalink raw reply
* [PATCH v2 1/8] Input: pixcir_i2c_ts: Use devres managed resource allocations
From: Roger Quadros @ 2014-02-26 15:27 UTC (permalink / raw)
To: dmitry.torokhov
Cc: rydberg, jcbian, balbi, dmurphy, mugunthanvnm, linux-input,
linux-kernel, devicetree, Roger Quadros
In-Reply-To: <1393428486-15001-1-git-send-email-rogerq@ti.com>
Use devm_() and friends for allocating memory, input device
and IRQ.
Signed-off-by: Roger Quadros <rogerq@ti.com>
Acked-by: Mugunthan V N <mugunthanvnm@ti.com>
---
drivers/input/touchscreen/pixcir_i2c_ts.c | 34 ++++++++++++-------------------
1 file changed, 13 insertions(+), 21 deletions(-)
diff --git a/drivers/input/touchscreen/pixcir_i2c_ts.c b/drivers/input/touchscreen/pixcir_i2c_ts.c
index 02392d2..38e83a2 100644
--- a/drivers/input/touchscreen/pixcir_i2c_ts.c
+++ b/drivers/input/touchscreen/pixcir_i2c_ts.c
@@ -130,6 +130,7 @@ static int pixcir_i2c_ts_probe(struct i2c_client *client,
{
const struct pixcir_ts_platform_data *pdata =
dev_get_platdata(&client->dev);
+ struct device *dev = &client->dev;
struct pixcir_i2c_ts_data *tsdata;
struct input_dev *input;
int error;
@@ -139,12 +140,14 @@ static int pixcir_i2c_ts_probe(struct i2c_client *client,
return -EINVAL;
}
- tsdata = kzalloc(sizeof(*tsdata), GFP_KERNEL);
- input = input_allocate_device();
- if (!tsdata || !input) {
- dev_err(&client->dev, "Failed to allocate driver data!\n");
- error = -ENOMEM;
- goto err_free_mem;
+ tsdata = devm_kzalloc(dev, sizeof(*tsdata), GFP_KERNEL);
+ if (!tsdata)
+ return -ENOMEM;
+
+ input = devm_input_allocate_device(dev);
+ if (!input) {
+ dev_err(&client->dev, "Failed to allocate input device\n");
+ return -ENOMEM;
}
tsdata->client = client;
@@ -165,29 +168,22 @@ static int pixcir_i2c_ts_probe(struct i2c_client *client,
input_set_drvdata(input, tsdata);
- error = request_threaded_irq(client->irq, NULL, pixcir_ts_isr,
+ error = devm_request_threaded_irq(dev, client->irq, NULL, pixcir_ts_isr,
IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
client->name, tsdata);
if (error) {
- dev_err(&client->dev, "Unable to request touchscreen IRQ.\n");
- goto err_free_mem;
+ dev_err(dev, "failed to request irq %d\n", client->irq);
+ return error;
}
error = input_register_device(input);
if (error)
- goto err_free_irq;
+ return error;
i2c_set_clientdata(client, tsdata);
device_init_wakeup(&client->dev, 1);
return 0;
-
-err_free_irq:
- free_irq(client->irq, tsdata);
-err_free_mem:
- input_free_device(input);
- kfree(tsdata);
- return error;
}
static int pixcir_i2c_ts_remove(struct i2c_client *client)
@@ -198,10 +194,6 @@ static int pixcir_i2c_ts_remove(struct i2c_client *client)
tsdata->exiting = true;
mb();
- free_irq(client->irq, tsdata);
-
- input_unregister_device(tsdata->input);
- kfree(tsdata);
return 0;
}
--
1.8.3.2
^ permalink raw reply related
* [PATCH v2 2/8] Input: pixcir_i2c_ts: Initialize interrupt mode and power mode
From: Roger Quadros @ 2014-02-26 15:28 UTC (permalink / raw)
To: dmitry.torokhov
Cc: rydberg, jcbian, balbi, dmurphy, mugunthanvnm, linux-input,
linux-kernel, devicetree, Roger Quadros
In-Reply-To: <1393428486-15001-1-git-send-email-rogerq@ti.com>
Introduce helper functions to configure power and interrupt
registers. Default to IDLE mode on probe as device supports
auto wakeup to ACVIE mode on detecting finger touch.
Configure interrupt mode and polarity on start up.
Power down on device closure or module removal.
Signed-off-by: Roger Quadros <rogerq@ti.com>
Acked-by: Mugunthan V N <mugunthanvnm@ti.com>
---
drivers/input/touchscreen/pixcir_i2c_ts.c | 173 +++++++++++++++++++++++++++++-
include/linux/input/pixcir_ts.h | 42 ++++++++
2 files changed, 213 insertions(+), 2 deletions(-)
diff --git a/drivers/input/touchscreen/pixcir_i2c_ts.c b/drivers/input/touchscreen/pixcir_i2c_ts.c
index 38e83a2..cce3740 100644
--- a/drivers/input/touchscreen/pixcir_i2c_ts.c
+++ b/drivers/input/touchscreen/pixcir_i2c_ts.c
@@ -100,6 +100,161 @@ static irqreturn_t pixcir_ts_isr(int irq, void *dev_id)
return IRQ_HANDLED;
}
+static int pixcir_set_power_mode(struct pixcir_i2c_ts_data *ts,
+ enum pixcir_power_mode mode)
+{
+ struct device *dev = &ts->client->dev;
+ int ret;
+
+ ret = i2c_smbus_read_byte_data(ts->client, PIXCIR_REG_POWER_MODE);
+ if (ret < 0) {
+ dev_err(dev, "%s: can't read reg 0x%x : %d\n",
+ __func__, PIXCIR_REG_POWER_MODE, ret);
+ return ret;
+ }
+
+ ret &= ~PIXCIR_POWER_MODE_MASK;
+ ret |= mode;
+
+ /* Always AUTO_IDLE */
+ ret |= PIXCIR_POWER_ALLOW_IDLE;
+
+ ret = i2c_smbus_write_byte_data(ts->client, PIXCIR_REG_POWER_MODE, ret);
+ if (ret < 0) {
+ dev_err(dev, "%s: can't write reg 0x%x : %d\n",
+ __func__, PIXCIR_REG_POWER_MODE, ret);
+ return ret;
+ }
+
+ return 0;
+}
+
+/*
+ * Set the interrupt mode for the device i.e. ATTB line behaviour
+ *
+ * @polarity : 1 for active high, 0 for active low.
+ */
+static int pixcir_set_int_mode(struct pixcir_i2c_ts_data *ts,
+ enum pixcir_int_mode mode,
+ bool polarity)
+{
+ struct device *dev = &ts->client->dev;
+ int ret;
+
+ ret = i2c_smbus_read_byte_data(ts->client, PIXCIR_REG_INT_MODE);
+ if (ret < 0) {
+ dev_err(dev, "%s: can't read reg 0x%x : %d\n",
+ __func__, PIXCIR_REG_INT_MODE, ret);
+ return ret;
+ }
+
+ ret &= ~PIXCIR_INT_MODE_MASK;
+ ret |= mode;
+
+ if (polarity)
+ ret |= PIXCIR_INT_POL_HIGH;
+ else
+ ret &= ~PIXCIR_INT_POL_HIGH;
+
+ ret = i2c_smbus_write_byte_data(ts->client, PIXCIR_REG_INT_MODE, ret);
+ if (ret < 0) {
+ dev_err(dev, "%s: can't write reg 0x%x : %d\n",
+ __func__, PIXCIR_REG_INT_MODE, ret);
+ return ret;
+ }
+
+ return 0;
+}
+
+/*
+ * Enable/disable interrupt generation
+ */
+static int pixcir_int_enable(struct pixcir_i2c_ts_data *ts, bool enable)
+{
+ struct device *dev = &ts->client->dev;
+ int ret;
+
+ ret = i2c_smbus_read_byte_data(ts->client, PIXCIR_REG_INT_MODE);
+ if (ret < 0) {
+ dev_err(dev, "%s: can't read reg 0x%x : %d\n",
+ __func__, PIXCIR_REG_INT_MODE, ret);
+ return ret;
+ }
+
+ if (enable)
+ ret |= PIXCIR_INT_ENABLE;
+ else
+ ret &= ~PIXCIR_INT_ENABLE;
+
+ ret = i2c_smbus_write_byte_data(ts->client, PIXCIR_REG_INT_MODE, ret);
+ if (ret < 0) {
+ dev_err(dev, "%s: can't write reg 0x%x : %d\n",
+ __func__, PIXCIR_REG_INT_MODE, ret);
+ return ret;
+ }
+
+ return 0;
+}
+
+static int pixcir_start(struct pixcir_i2c_ts_data *ts)
+{
+ struct device *dev = &ts->client->dev;
+ int ret;
+
+ /* LEVEL_TOUCH interrupt with active low polarity */
+ ret = pixcir_set_int_mode(ts, PIXCIR_INT_LEVEL_TOUCH, 0);
+ if (ret) {
+ dev_err(dev, "Failed to set interrupt mode\n");
+ return ret;
+ }
+
+ ts->exiting = false;
+ mb(); /* Update status before IRQ can fire */
+
+ /* enable interrupt generation */
+ ret = pixcir_int_enable(ts, 1);
+ if (ret) {
+ dev_err(dev, "Failed to enable interrupt generation\n");
+ return ret;
+ }
+
+ return 0;
+}
+
+static int pixcir_stop(struct pixcir_i2c_ts_data *ts)
+{
+ struct device *dev = &ts->client->dev;
+ int ret;
+
+ /* disable interrupt generation */
+ ret = pixcir_int_enable(ts, 0);
+ if (ret) {
+ dev_err(dev, "Failed to disable interrupt generation\n");
+ return ret;
+ }
+
+ synchronize_irq(ts->client->irq);
+ mb(); /* Update status after pending IRQ is complete */
+ ts->exiting = true;
+
+ return 0;
+}
+
+static int pixcir_input_open(struct input_dev *dev)
+{
+ struct pixcir_i2c_ts_data *ts = input_get_drvdata(dev);
+
+ return pixcir_start(ts);
+}
+
+static void pixcir_input_close(struct input_dev *dev)
+{
+ struct pixcir_i2c_ts_data *ts = input_get_drvdata(dev);
+
+ pixcir_stop(ts);
+}
+
+
#ifdef CONFIG_PM_SLEEP
static int pixcir_i2c_ts_suspend(struct device *dev)
{
@@ -157,6 +312,8 @@ static int pixcir_i2c_ts_probe(struct i2c_client *client,
input->name = client->name;
input->id.bustype = BUS_I2C;
input->dev.parent = &client->dev;
+ input->open = pixcir_input_open;
+ input->close = pixcir_input_close;
__set_bit(EV_KEY, input->evbit);
__set_bit(EV_ABS, input->evbit);
@@ -176,6 +333,18 @@ static int pixcir_i2c_ts_probe(struct i2c_client *client,
return error;
}
+ /* Always be in IDLE mode to save power, device supports auto wake */
+ error = pixcir_set_power_mode(tsdata, PIXCIR_POWER_IDLE);
+ if (error) {
+ dev_err(dev, "Failed to set IDLE mode\n");
+ return error;
+ }
+
+ /* Stop device till opened */
+ error = pixcir_stop(tsdata);
+ if (error)
+ return error;
+
error = input_register_device(input);
if (error)
return error;
@@ -192,8 +361,8 @@ static int pixcir_i2c_ts_remove(struct i2c_client *client)
device_init_wakeup(&client->dev, 0);
- tsdata->exiting = true;
- mb();
+ if (!tsdata->exiting)
+ pixcir_stop(tsdata);
return 0;
}
diff --git a/include/linux/input/pixcir_ts.h b/include/linux/input/pixcir_ts.h
index 7163d91..7942804 100644
--- a/include/linux/input/pixcir_ts.h
+++ b/include/linux/input/pixcir_ts.h
@@ -1,6 +1,48 @@
#ifndef _PIXCIR_I2C_TS_H
#define _PIXCIR_I2C_TS_H
+/*
+ * Register map
+ */
+#define PIXCIR_REG_POWER_MODE 51
+#define PIXCIR_REG_INT_MODE 52
+
+/*
+ * Power modes:
+ * active: max scan speed
+ * idle: lower scan speed with automatic transition to active on touch
+ * halt: datasheet says sleep but this is more like halt as the chip
+ * clocks are cut and it can only be brought out of this mode
+ * using the RESET pin.
+ */
+enum pixcir_power_mode {
+ PIXCIR_POWER_ACTIVE,
+ PIXCIR_POWER_IDLE,
+ PIXCIR_POWER_HALT,
+};
+
+#define PIXCIR_POWER_MODE_MASK 0x03
+#define PIXCIR_POWER_ALLOW_IDLE (1UL << 2)
+
+/*
+ * Interrupt modes:
+ * periodical: interrupt is asserted periodicaly
+ * diff coordinates: interrupt is asserted when coordinates change
+ * level on touch: interrupt level asserted during touch
+ * pulse on touch: interrupt pulse asserted druing touch
+ *
+ */
+enum pixcir_int_mode {
+ PIXCIR_INT_PERIODICAL,
+ PIXCIR_INT_DIFF_COORD,
+ PIXCIR_INT_LEVEL_TOUCH,
+ PIXCIR_INT_PULSE_TOUCH,
+};
+
+#define PIXCIR_INT_MODE_MASK 0x03
+#define PIXCIR_INT_ENABLE (1UL << 3)
+#define PIXCIR_INT_POL_HIGH (1UL << 2)
+
struct pixcir_ts_platform_data {
int (*attb_read_val)(void);
int x_max;
--
1.8.3.2
^ permalink raw reply related
* [PATCH v2 3/8] Input: pixcir_i2c_ts: Get rid of pdata->attb_read_val()
From: Roger Quadros @ 2014-02-26 15:28 UTC (permalink / raw)
To: dmitry.torokhov
Cc: rydberg, jcbian, balbi, dmurphy, mugunthanvnm, linux-input,
linux-kernel, devicetree, Roger Quadros
In-Reply-To: <1393428486-15001-1-git-send-email-rogerq@ti.com>
Get rid of the attb_read_val() platform hook. Instead,
read the ATTB gpio directly from the driver.
Fail if valid ATTB gpio is not provided by patform data.
Signed-off-by: Roger Quadros <rogerq@ti.com>
Acked-by: Mugunthan V N <mugunthanvnm@ti.com>
---
drivers/input/touchscreen/pixcir_i2c_ts.c | 16 +++++++++++++++-
include/linux/input/pixcir_ts.h | 2 +-
2 files changed, 16 insertions(+), 2 deletions(-)
diff --git a/drivers/input/touchscreen/pixcir_i2c_ts.c b/drivers/input/touchscreen/pixcir_i2c_ts.c
index cce3740..fe17b41 100644
--- a/drivers/input/touchscreen/pixcir_i2c_ts.c
+++ b/drivers/input/touchscreen/pixcir_i2c_ts.c
@@ -24,6 +24,7 @@
#include <linux/i2c.h>
#include <linux/input.h>
#include <linux/input/pixcir_ts.h>
+#include <linux/gpio.h>
struct pixcir_i2c_ts_data {
struct i2c_client *client;
@@ -87,11 +88,12 @@ static void pixcir_ts_poscheck(struct pixcir_i2c_ts_data *data)
static irqreturn_t pixcir_ts_isr(int irq, void *dev_id)
{
struct pixcir_i2c_ts_data *tsdata = dev_id;
+ const struct pixcir_ts_platform_data *pdata = tsdata->chip;
while (!tsdata->exiting) {
pixcir_ts_poscheck(tsdata);
- if (tsdata->chip->attb_read_val())
+ if (gpio_get_value(pdata->gpio_attb))
break;
msleep(20);
@@ -293,6 +295,11 @@ static int pixcir_i2c_ts_probe(struct i2c_client *client,
if (!pdata) {
dev_err(&client->dev, "platform data not defined\n");
return -EINVAL;
+ } else {
+ if (!gpio_is_valid(pdata->gpio_attb)) {
+ dev_err(dev, "Invalid gpio_attb in pdata\n");
+ return -EINVAL;
+ }
}
tsdata = devm_kzalloc(dev, sizeof(*tsdata), GFP_KERNEL);
@@ -325,6 +332,13 @@ static int pixcir_i2c_ts_probe(struct i2c_client *client,
input_set_drvdata(input, tsdata);
+ error = devm_gpio_request_one(dev, pdata->gpio_attb,
+ GPIOF_DIR_IN, "pixcir_i2c_attb");
+ if (error) {
+ dev_err(dev, "Failed to request ATTB gpio\n");
+ return error;
+ }
+
error = devm_request_threaded_irq(dev, client->irq, NULL, pixcir_ts_isr,
IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
client->name, tsdata);
diff --git a/include/linux/input/pixcir_ts.h b/include/linux/input/pixcir_ts.h
index 7942804..160cf35 100644
--- a/include/linux/input/pixcir_ts.h
+++ b/include/linux/input/pixcir_ts.h
@@ -44,9 +44,9 @@ enum pixcir_int_mode {
#define PIXCIR_INT_POL_HIGH (1UL << 2)
struct pixcir_ts_platform_data {
- int (*attb_read_val)(void);
int x_max;
int y_max;
+ int gpio_attb; /* GPIO connected to ATTB line */
};
#endif
--
1.8.3.2
^ permalink raw reply related
* [PATCH v2 4/8] Input: pixcir_i2c_ts: Use Type-B Multi-Touch protocol
From: Roger Quadros @ 2014-02-26 15:28 UTC (permalink / raw)
To: dmitry.torokhov-Re5JQEeQqe8AvxtiuMwx3w
Cc: rydberg-Hk7bIW8heu4wFerOooGFRg, jcbian-mY6CKx1T+M6Pt1CcHtbs0g,
balbi-l0cyMroinI0, dmurphy-l0cyMroinI0, mugunthanvnm-l0cyMroinI0,
linux-input-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA, Roger Quadros
In-Reply-To: <1393428486-15001-1-git-send-email-rogerq-l0cyMroinI0@public.gmane.org>
Switch to using the Type-B Multi-Touch protocol.
Signed-off-by: Roger Quadros <rogerq-l0cyMroinI0@public.gmane.org>
---
drivers/input/touchscreen/pixcir_i2c_ts.c | 125 ++++++++++++++++++++++--------
1 file changed, 94 insertions(+), 31 deletions(-)
diff --git a/drivers/input/touchscreen/pixcir_i2c_ts.c b/drivers/input/touchscreen/pixcir_i2c_ts.c
index fe17b41..8736f71 100644
--- a/drivers/input/touchscreen/pixcir_i2c_ts.c
+++ b/drivers/input/touchscreen/pixcir_i2c_ts.c
@@ -23,9 +23,12 @@
#include <linux/slab.h>
#include <linux/i2c.h>
#include <linux/input.h>
+#include <linux/input/mt.h>
#include <linux/input/pixcir_ts.h>
#include <linux/gpio.h>
+#define PIXCIR_MAX_SLOTS 2
+
struct pixcir_i2c_ts_data {
struct i2c_client *client;
struct input_dev *input;
@@ -33,12 +36,25 @@ struct pixcir_i2c_ts_data {
bool exiting;
};
-static void pixcir_ts_poscheck(struct pixcir_i2c_ts_data *data)
+struct pixcir_touch {
+ int x;
+ int y;
+};
+
+struct pixcir_report_data {
+ int num_touches;
+ struct pixcir_touch touches[PIXCIR_MAX_SLOTS];
+};
+
+static void pixcir_ts_parse(struct pixcir_i2c_ts_data *tsdata,
+ struct pixcir_report_data *report)
{
- struct pixcir_i2c_ts_data *tsdata = data;
u8 rdbuf[10], wrbuf[1] = { 0 };
+ u8 *bufptr;
u8 touch;
- int ret;
+ int ret, i;
+
+ memset(report, 0, sizeof(struct pixcir_report_data));
ret = i2c_master_send(tsdata->client, wrbuf, sizeof(wrbuf));
if (ret != sizeof(wrbuf)) {
@@ -56,45 +72,85 @@ static void pixcir_ts_poscheck(struct pixcir_i2c_ts_data *data)
return;
}
- touch = rdbuf[0];
- if (touch) {
- u16 posx1 = (rdbuf[3] << 8) | rdbuf[2];
- u16 posy1 = (rdbuf[5] << 8) | rdbuf[4];
- u16 posx2 = (rdbuf[7] << 8) | rdbuf[6];
- u16 posy2 = (rdbuf[9] << 8) | rdbuf[8];
-
- input_report_key(tsdata->input, BTN_TOUCH, 1);
- input_report_abs(tsdata->input, ABS_X, posx1);
- input_report_abs(tsdata->input, ABS_Y, posy1);
-
- input_report_abs(tsdata->input, ABS_MT_POSITION_X, posx1);
- input_report_abs(tsdata->input, ABS_MT_POSITION_Y, posy1);
- input_mt_sync(tsdata->input);
-
- if (touch == 2) {
- input_report_abs(tsdata->input,
- ABS_MT_POSITION_X, posx2);
- input_report_abs(tsdata->input,
- ABS_MT_POSITION_Y, posy2);
- input_mt_sync(tsdata->input);
- }
- } else {
- input_report_key(tsdata->input, BTN_TOUCH, 0);
+ touch = rdbuf[0] & 0x7;
+ if (touch > PIXCIR_MAX_SLOTS)
+ touch = PIXCIR_MAX_SLOTS;
+
+ report->num_touches = touch;
+ bufptr = &rdbuf[2];
+
+ for (i = 0; i < touch; i++) {
+ report->touches[i].x = (bufptr[1] << 8) | bufptr[0];
+ report->touches[i].y = (bufptr[3] << 8) | bufptr[2];
+
+ bufptr = &bufptr[4];
}
+}
+
+static void pixcir_ts_report(struct pixcir_i2c_ts_data *ts,
+ struct pixcir_report_data *report)
+{
+ struct input_mt_pos pos[PIXCIR_MAX_SLOTS];
+ int slots[PIXCIR_MAX_SLOTS];
+ struct pixcir_touch *touch;
+ int n, i, slot;
+ struct device *dev = &ts->client->dev;
- input_sync(tsdata->input);
+ n = report->num_touches;
+ if (n > PIXCIR_MAX_SLOTS)
+ n = PIXCIR_MAX_SLOTS;
+
+ for (i = 0; i < n; i++) {
+ touch = &report->touches[i];
+ pos[i].x = touch->x;
+ pos[i].y = touch->y;
+ }
+
+ input_mt_assign_slots(ts->input, slots, pos, n);
+
+ for (i = 0; i < n; i++) {
+ touch = &report->touches[i];
+ slot = slots[i];
+
+ input_mt_slot(ts->input, slot);
+ input_mt_report_slot_state(ts->input,
+ MT_TOOL_FINGER, true);
+
+ input_event(ts->input, EV_ABS, ABS_MT_POSITION_X, touch->x);
+ input_event(ts->input, EV_ABS, ABS_MT_POSITION_Y, touch->y);
+
+ dev_dbg(dev, "%d: slot %d, x %d, y %d\n",
+ i, slot, touch->x, touch->y);
+ }
+
+ input_mt_sync_frame(ts->input);
+ input_sync(ts->input);
}
static irqreturn_t pixcir_ts_isr(int irq, void *dev_id)
{
struct pixcir_i2c_ts_data *tsdata = dev_id;
const struct pixcir_ts_platform_data *pdata = tsdata->chip;
+ struct pixcir_report_data report;
while (!tsdata->exiting) {
- pixcir_ts_poscheck(tsdata);
-
- if (gpio_get_value(pdata->gpio_attb))
+ /* parse packet */
+ pixcir_ts_parse(tsdata, &report);
+
+ /* report it */
+ pixcir_ts_report(tsdata, &report);
+
+ if (gpio_get_value(pdata->gpio_attb)) {
+ if (report.num_touches) {
+ /*
+ * Last report with no finger up?
+ * Do it now then.
+ */
+ input_mt_sync_frame(tsdata->input);
+ input_sync(tsdata->input);
+ }
break;
+ }
msleep(20);
}
@@ -330,6 +386,13 @@ static int pixcir_i2c_ts_probe(struct i2c_client *client,
input_set_abs_params(input, ABS_MT_POSITION_X, 0, pdata->x_max, 0, 0);
input_set_abs_params(input, ABS_MT_POSITION_Y, 0, pdata->y_max, 0, 0);
+ error = input_mt_init_slots(input, PIXCIR_MAX_SLOTS,
+ INPUT_MT_DIRECT | INPUT_MT_DROP_UNUSED);
+ if (error) {
+ dev_err(dev, "Error initializing Multi-Touch slots\n");
+ return error;
+ }
+
input_set_drvdata(input, tsdata);
error = devm_gpio_request_one(dev, pdata->gpio_attb,
--
1.8.3.2
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply related
* [PATCH v2 5/8] Input: pixcir_i2c_ts: support upto 5 fingers and hardware provided tracking IDs
From: Roger Quadros @ 2014-02-26 15:28 UTC (permalink / raw)
To: dmitry.torokhov
Cc: rydberg, jcbian, balbi, dmurphy, mugunthanvnm, linux-input,
linux-kernel, devicetree, Roger Quadros
In-Reply-To: <1393428486-15001-1-git-send-email-rogerq@ti.com>
Some variants of the Pixcir touch controller support upto 5
simultaneous fingers and hardware tracking IDs. Prepare the driver
for that.
Signed-off-by: Roger Quadros <rogerq@ti.com>
---
drivers/input/touchscreen/pixcir_i2c_ts.c | 74 ++++++++++++++++++++++++-------
include/linux/input/pixcir_ts.h | 12 +++++
2 files changed, 69 insertions(+), 17 deletions(-)
diff --git a/drivers/input/touchscreen/pixcir_i2c_ts.c b/drivers/input/touchscreen/pixcir_i2c_ts.c
index 8736f71..b1e92f6 100644
--- a/drivers/input/touchscreen/pixcir_i2c_ts.c
+++ b/drivers/input/touchscreen/pixcir_i2c_ts.c
@@ -27,18 +27,20 @@
#include <linux/input/pixcir_ts.h>
#include <linux/gpio.h>
-#define PIXCIR_MAX_SLOTS 2
+#define PIXCIR_MAX_SLOTS 5 /* Max fingers supported by driver */
struct pixcir_i2c_ts_data {
struct i2c_client *client;
struct input_dev *input;
- const struct pixcir_ts_platform_data *chip;
+ const struct pixcir_ts_platform_data *pdata;
bool exiting;
+ int max_fingers; /* Max fingers supported in this instance */
};
struct pixcir_touch {
int x;
int y;
+ int id;
};
struct pixcir_report_data {
@@ -49,13 +51,21 @@ struct pixcir_report_data {
static void pixcir_ts_parse(struct pixcir_i2c_ts_data *tsdata,
struct pixcir_report_data *report)
{
- u8 rdbuf[10], wrbuf[1] = { 0 };
+ u8 rdbuf[2 + PIXCIR_MAX_SLOTS * 5];
+ u8 wrbuf[1] = { 0 };
u8 *bufptr;
u8 touch;
int ret, i;
+ int readsize;
+ const struct pixcir_i2c_chip_data *chip = &tsdata->pdata->chip;
memset(report, 0, sizeof(struct pixcir_report_data));
+ i = chip->has_hw_ids ? 1 : 0;
+ readsize = 2 + tsdata->max_fingers * (4 + i);
+ if (readsize > sizeof(rdbuf))
+ readsize = sizeof(rdbuf);
+
ret = i2c_master_send(tsdata->client, wrbuf, sizeof(wrbuf));
if (ret != sizeof(wrbuf)) {
dev_err(&tsdata->client->dev,
@@ -64,7 +74,7 @@ static void pixcir_ts_parse(struct pixcir_i2c_ts_data *tsdata,
return;
}
- ret = i2c_master_recv(tsdata->client, rdbuf, sizeof(rdbuf));
+ ret = i2c_master_recv(tsdata->client, rdbuf, readsize);
if (ret != sizeof(rdbuf)) {
dev_err(&tsdata->client->dev,
"%s: i2c_master_recv failed(), ret=%d\n",
@@ -73,8 +83,8 @@ static void pixcir_ts_parse(struct pixcir_i2c_ts_data *tsdata,
}
touch = rdbuf[0] & 0x7;
- if (touch > PIXCIR_MAX_SLOTS)
- touch = PIXCIR_MAX_SLOTS;
+ if (touch > tsdata->max_fingers)
+ touch = tsdata->max_fingers;
report->num_touches = touch;
bufptr = &rdbuf[2];
@@ -83,7 +93,12 @@ static void pixcir_ts_parse(struct pixcir_i2c_ts_data *tsdata,
report->touches[i].x = (bufptr[1] << 8) | bufptr[0];
report->touches[i].y = (bufptr[3] << 8) | bufptr[2];
- bufptr = &bufptr[4];
+ if (chip->has_hw_ids) {
+ report->touches[i].id = bufptr[4];
+ bufptr = &bufptr[5];
+ } else {
+ bufptr = &bufptr[4];
+ }
}
}
@@ -95,22 +110,35 @@ static void pixcir_ts_report(struct pixcir_i2c_ts_data *ts,
struct pixcir_touch *touch;
int n, i, slot;
struct device *dev = &ts->client->dev;
+ const struct pixcir_i2c_chip_data *chip = &ts->pdata->chip;
n = report->num_touches;
if (n > PIXCIR_MAX_SLOTS)
n = PIXCIR_MAX_SLOTS;
- for (i = 0; i < n; i++) {
- touch = &report->touches[i];
- pos[i].x = touch->x;
- pos[i].y = touch->y;
- }
+ if (!chip->has_hw_ids) {
+ for (i = 0; i < n; i++) {
+ touch = &report->touches[i];
+ pos[i].x = touch->x;
+ pos[i].y = touch->y;
+ }
- input_mt_assign_slots(ts->input, slots, pos, n);
+ input_mt_assign_slots(ts->input, slots, pos, n);
+ }
for (i = 0; i < n; i++) {
touch = &report->touches[i];
- slot = slots[i];
+
+ if (chip->has_hw_ids) {
+ slot = input_mt_get_slot_by_key(ts->input, touch->id);
+ if (slot < 0) {
+ dev_dbg(dev, "no free slot for id 0x%x\n",
+ touch->id);
+ continue;
+ }
+ } else {
+ slot = slots[i];
+ }
input_mt_slot(ts->input, slot);
input_mt_report_slot_state(ts->input,
@@ -130,7 +158,7 @@ static void pixcir_ts_report(struct pixcir_i2c_ts_data *ts,
static irqreturn_t pixcir_ts_isr(int irq, void *dev_id)
{
struct pixcir_i2c_ts_data *tsdata = dev_id;
- const struct pixcir_ts_platform_data *pdata = tsdata->chip;
+ const struct pixcir_ts_platform_data *pdata = tsdata->pdata;
struct pixcir_report_data report;
while (!tsdata->exiting) {
@@ -356,6 +384,11 @@ static int pixcir_i2c_ts_probe(struct i2c_client *client,
dev_err(dev, "Invalid gpio_attb in pdata\n");
return -EINVAL;
}
+
+ if (pdata->chip.max_fingers <= 0) {
+ dev_err(dev, "Invalid max_fingers in pdata\n");
+ return -EINVAL;
+ }
}
tsdata = devm_kzalloc(dev, sizeof(*tsdata), GFP_KERNEL);
@@ -370,7 +403,7 @@ static int pixcir_i2c_ts_probe(struct i2c_client *client,
tsdata->client = client;
tsdata->input = input;
- tsdata->chip = pdata;
+ tsdata->pdata = pdata;
input->name = client->name;
input->id.bustype = BUS_I2C;
@@ -386,7 +419,14 @@ static int pixcir_i2c_ts_probe(struct i2c_client *client,
input_set_abs_params(input, ABS_MT_POSITION_X, 0, pdata->x_max, 0, 0);
input_set_abs_params(input, ABS_MT_POSITION_Y, 0, pdata->y_max, 0, 0);
- error = input_mt_init_slots(input, PIXCIR_MAX_SLOTS,
+ tsdata->max_fingers = tsdata->pdata->chip.max_fingers;
+ if (tsdata->max_fingers > PIXCIR_MAX_SLOTS) {
+ tsdata->max_fingers = PIXCIR_MAX_SLOTS;
+ dev_info(dev, "Limiting maximum fingers to %d\n",
+ tsdata->max_fingers);
+ }
+
+ error = input_mt_init_slots(input, tsdata->max_fingers,
INPUT_MT_DIRECT | INPUT_MT_DROP_UNUSED);
if (error) {
dev_err(dev, "Error initializing Multi-Touch slots\n");
diff --git a/include/linux/input/pixcir_ts.h b/include/linux/input/pixcir_ts.h
index 160cf35..7bae83b 100644
--- a/include/linux/input/pixcir_ts.h
+++ b/include/linux/input/pixcir_ts.h
@@ -43,10 +43,22 @@ enum pixcir_int_mode {
#define PIXCIR_INT_ENABLE (1UL << 3)
#define PIXCIR_INT_POL_HIGH (1UL << 2)
+/**
+ * struct pixcir_irc_chip_data - chip related data
+ * @max_fingers: Max number of fingers reported simultaneously by h/w
+ * @has_hw_ids: Hardware supports finger tracking IDs
+ *
+ */
+struct pixcir_i2c_chip_data {
+ u8 max_fingers;
+ bool has_hw_ids;
+};
+
struct pixcir_ts_platform_data {
int x_max;
int y_max;
int gpio_attb; /* GPIO connected to ATTB line */
+ struct pixcir_i2c_chip_data chip;
};
#endif
--
1.8.3.2
^ permalink raw reply related
* [PATCH v2 6/8] Input: pixcir_i2c_ts: Implement wakeup from suspend
From: Roger Quadros @ 2014-02-26 15:28 UTC (permalink / raw)
To: dmitry.torokhov
Cc: rydberg, jcbian, balbi, dmurphy, mugunthanvnm, linux-input,
linux-kernel, devicetree, Roger Quadros
In-Reply-To: <1393428486-15001-1-git-send-email-rogerq@ti.com>
Improve the suspend and resume handlers to allow the device
to wakeup the system from suspend.
Signed-off-by: Roger Quadros <rogerq@ti.com>
Acked-by: Mugunthan V N <mugunthanvnm@ti.com>
---
drivers/input/touchscreen/pixcir_i2c_ts.c | 46 ++++++++++++++++++++++++++++---
1 file changed, 42 insertions(+), 4 deletions(-)
diff --git a/drivers/input/touchscreen/pixcir_i2c_ts.c b/drivers/input/touchscreen/pixcir_i2c_ts.c
index b1e92f6..8ca258b 100644
--- a/drivers/input/touchscreen/pixcir_i2c_ts.c
+++ b/drivers/input/touchscreen/pixcir_i2c_ts.c
@@ -345,21 +345,59 @@ static void pixcir_input_close(struct input_dev *dev)
static int pixcir_i2c_ts_suspend(struct device *dev)
{
struct i2c_client *client = to_i2c_client(dev);
+ struct pixcir_i2c_ts_data *ts = i2c_get_clientdata(client);
+ struct input_dev *input = ts->input;
+ int ret = 0;
+
+ mutex_lock(&input->mutex);
+
+ if (device_may_wakeup(&client->dev)) {
+ /* need to start device if not open, to be wakeup source */
+ if (!input->users) {
+ ret = pixcir_start(ts);
+ if (ret)
+ goto unlock;
+ }
- if (device_may_wakeup(&client->dev))
enable_irq_wake(client->irq);
- return 0;
+ } else if (input->users) {
+ ret = pixcir_stop(ts);
+ }
+
+unlock:
+ mutex_unlock(&input->mutex);
+
+ return ret;
}
static int pixcir_i2c_ts_resume(struct device *dev)
{
struct i2c_client *client = to_i2c_client(dev);
+ struct pixcir_i2c_ts_data *ts = i2c_get_clientdata(client);
+ struct input_dev *input = ts->input;
+ int ret = 0;
+
+ mutex_lock(&input->mutex);
- if (device_may_wakeup(&client->dev))
+ if (device_may_wakeup(&client->dev)) {
disable_irq_wake(client->irq);
- return 0;
+ /* need to stop device if it was not open on suspend */
+ if (!input->users) {
+ ret = pixcir_stop(ts);
+ if (ret)
+ goto unlock;
+ }
+
+ } else if (input->users) {
+ ret = pixcir_start(ts);
+ }
+
+unlock:
+ mutex_unlock(&input->mutex);
+
+ return ret;
}
#endif
--
1.8.3.2
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox