From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755445AbbE2FoS (ORCPT ); Fri, 29 May 2015 01:44:18 -0400 Received: from mail-by2on0095.outbound.protection.outlook.com ([207.46.100.95]:64885 "EHLO na01-by2-obe.outbound.protection.outlook.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1752833AbbE2FoN (ORCPT ); Fri, 29 May 2015 01:44:13 -0400 Authentication-Results: spf=pass (sender IP is 149.199.60.100) smtp.mailfrom=xilinx.com; ettus.com; dkim=none (message not signed) header.d=none; Message-ID: <5567FCA0.70906@xilinx.com> Date: Fri, 29 May 2015 07:44:00 +0200 From: Michal Simek User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.6.0 MIME-Version: 1.0 To: Moritz Fischer , Michal Simek CC: Jassi Brar , , , , , , Kumar Gala , =?UTF-8?B?U8O2cmVuIEJyaW5rbWFubg==?= , , Greg KH , , Arnd Bergmann , , Jingoo Han , , Subject: Re: [PATCHv2 2/2] mailbox: Adding driver for Xilinx LogiCORE IP mailbox. References: <1432751745-17473-1-git-send-email-moritz.fischer@ettus.com> <1432751745-17473-3-git-send-email-moritz.fischer@ettus.com> <5566AB68.8000108@xilinx.com> In-Reply-To: Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 7bit X-RCIS-Action: ALLOW X-TM-AS-Product-Ver: IMSS-7.1.0.1224-8.0.0.1202-21574.005 X-TM-AS-User-Approved-Sender: Yes;Yes X-EOPAttributedMessage: 0 X-Microsoft-Exchange-Diagnostics: 1;BN1BFFO11OLC003;1:ijmsL9uijTXgnCvC8Z/TIcPrNN0qAcLIxewO9TzV7G5eUj9TQ0Og11AC7ICgpKQ5qqwVm34p+K1TmEiTt0/dNnVGmEEUiG220vtK4WrwtTkxOV9uR6Ze4AFom+o8P7Lhxc2WHsRtWYKlpZYR250Z1SRpZ2Rba7gnI/ftoItVRUPg0/enAhguS0GvEbv/lcz8FgPiy0LSOCWX7pFKD1oP0+nURXrsiYkpF3uSQbNfmShZPpBLF2i7CmUsIFD12jZjbb6aNEF9M15Et/P5iuueVw== X-Forefront-Antispam-Report: CIP:149.199.60.100;CTRY:US;IPV:NLI;EFV:NLI;SFV:NSPM;SFS:(10009020)(6009001)(438002)(199003)(24454002)(377454003)(189002)(164054003)(51704005)(479174004)(86362001)(50986999)(76176999)(33656002)(47776003)(64126003)(54356999)(575784001)(93886004)(65816999)(77096005)(64706001)(63266004)(5001960100002)(5001770100001)(83506001)(2950100001)(65956001)(87936001)(6806004)(23676002)(65806001)(50466002)(46102003)(4001350100001)(62966003)(19580405001)(19580395003)(81156007)(4001540100001)(36386004)(189998001)(92566002)(5001860100001)(5001830100001)(36756003)(77156002)(106466001)(2004002)(107986001)(5001870100001)(4001450100001);DIR:OUT;SFP:1101;SCL:1;SRVR:BN1BFFO11HUB052;H:xsj-pvapsmtpgw02;FPR:;SPF:Pass;PTR:unknown-60-100.xilinx.com,xapps1.xilinx.com;A:1;MX:1;LANG:en; X-Microsoft-Exchange-Diagnostics: 1;BN1BFFO11HUB052;2:ar828FnS62sreo6vqeyLP5H4TVS8E9lnyDrjGGCp09niijz73dj63XV5sRYhjYOC;2:ZeIqon9ekMwOkwima+fz/az71wh/tINbJK9eA6HPm2pMULOVaRGsWb870y8p6HYqIbQaYjJ2BK23xu5r1YDaayarM2MnuwK6zhGYae9rfOELjscCJn9e/bb4TgfpzsXrC0n4BZ81Wu8bDeYZ3o1/m680puYdoDBu3NSjgjXJgrzRpNPM27kU/5AlnQPO7XUqtLIU467cmRE6h/In4lS24mOGgBn0+QynVUXHPxFgKBTd8AX6Z6Pq4mVTS2ISg1SO;6:ILdcybP+UJt5WYyB2a9NgcmO2FlhQJneuAi7NuTefn7oVsrdeuyhIAPB6h4GcL3ngMNLwDTAqtOUvaJ70o5lfxGxhCOgtwLLRc/cmNCRDh4aon3/kKtxOewo8QCU7jjpMfAPVzosg7GBmAO7v/kvQAKk7UJEqQp12bMH4GU1oy/Sclk//cLmXhB9Na4I6W1ZIOwkItML7r3A9wHfqXg0vfUiCeEQsC0n28PyIJczB/+HgilIJRI4F/MqRgW0W4z43jPOSnss60CC54SPKSDhVeAaV1Jh32c5mTh2/DhuRAULvZkLlO0wWwyRUqZXkHV6MlzUkRICD5Ck4ovcd4D+yeToVhFRjSfISUAC3lILj3SWPmJkIrgyWAqobnD49BQAn9+jG+ZqGpGO6v1tsZsrouotHjI+4+IHYsqSnlf02lgumTVn9obHChVkRpdMqQOU3n9oM8kYs8pp01yP8yj3EG8vSlRkPEIPhu5uy5L360FPtaQ1h4y5SHv59Zsgzh71 X-Microsoft-Antispam: UriScan:;BCL:0;PCL:0;RULEID:;SRVR:BN1BFFO11HUB052; X-Microsoft-Antispam-PRVS: X-Exchange-Antispam-Report-Test: UriScan:; X-Exchange-Antispam-Report-CFA-Test: BCL:0;PCL:0;RULEID:(601004)(5005006)(520003)(3002001);SRVR:BN1BFFO11HUB052;BCL:0;PCL:0;RULEID:;SRVR:BN1BFFO11HUB052; X-Microsoft-Exchange-Diagnostics: 1;BN1BFFO11HUB052;3:eJ1ZPb29avPwYjSQa6KAWNT617jou7Ys5lj9miS6Sg+rkG39r3Cu6gBNRrqsOu3zkEmiu3duzy0UsW45GsJTHTdTVlJd+W6mOIeKLFJ+So9mi3nrciL+Uu4v5ODFyBO6PGNdRt2qQLXqP9nU3lWqZDEgajbMh+2ZSJd+mXmYtGT2KmbNDoYuUL984XBWynWGbpdQzC3lMh4jXSGXSg6fqhDaNJcCgpTIJ9Vnx73QwKJrfcvsLC3CQFNmuPBVCnmbwtrsb8u1BSAFBjw/xH4VPm2juTZ4A7Cs+2/9hiugUS5G331PK/QQrlR1xKGRetN6 X-Forefront-PRVS: 059185FE08 X-Microsoft-Exchange-Diagnostics: =?utf-8?B?MTtCTjFCRkZPMTFIVUIwNTI7OTpOWDRsTWpScnIvTjhpbUNzTXM0REYyNHl4?= =?utf-8?B?TVlwZEZjU3BUMDA2MlpjVlZaZU94VkEvRnVreFA2dkViaEU2Y1FDY3dNZFJt?= =?utf-8?B?MWZVaUpneG1KeGREellFdzUvTFdTdTQrTzdaVVJtV1pOZmVYOUhobnRMcTRY?= =?utf-8?B?Y00zYWNJazBwZUtqMHNlQ3I4Zy9yUW1nRmlIaHhTRWtNbzZUTHhhZ3NEd2lK?= =?utf-8?B?TFNmVENFUW1zdTlKaUF1c1J1R0RDUzZLSGhwK3FjemxxbTJ6dCs0djJra0dR?= =?utf-8?B?bmdydzZoUUNzY04xTVB0akhMRDNpdUlReXpjWEV5SWhJRjRtMzBOY21qSXNE?= =?utf-8?B?c1IrOVlBWUdwK3ZDSTM0MCtPVSs5NUNQNTlCVjEyVTJqS0dEUzBEMzl4ZHJK?= =?utf-8?B?MWJiV2NuMEpJdHA0NXRBeGJlRjRBVnl3VGcwaEhmL0wrU3VoZlNNMkxKVTVy?= =?utf-8?B?TzY3SVJNUmJSS0czWndvT0NVTnltTi9tOFFsdG5aVzROQmFJUDZPejhoQ1o1?= =?utf-8?B?VVJneG5VUVNmWFU0Z1U4Y3dTbEFkQW8wVWtSQ0x6UWh5Z2ZTYkRhNTBIaUtH?= =?utf-8?B?ZnU0WSs3MUVoMUNIZitVUUtod1lGTDByRWorODAwY1pQR2kzNHNKVGJra3dU?= =?utf-8?B?MXEyWFpQbG8wOERIeU1ONWJUQlVQVXNHdEVrMjdYb2RzMjVOV1RRdk00MlRq?= =?utf-8?B?STlEeDhRMUMxdGJvSXNTZkt6eUV5SW5kaFNZK1ZaOEE1OVg1NFpxUFJTbndT?= =?utf-8?B?alI0MVYxNGEvaCtUb3pkN3BrYk1qZ2dsY0VVOXFjWDFmTWlYVGxmNGlmNm0w?= =?utf-8?B?aWJRM214M2xwUGd4dnF1bml5Zy9aUFJYVkhBRDZUYVJ1MWQzUGRHOHZOSFVw?= =?utf-8?B?T3JQNFVDU3FGWnlDNGgxUThYUjZTYXMyOWFCR2o4anBmdnF6djVwVWhsRWZm?= =?utf-8?B?QVZscGdLajlEZmRzRkU3ZnF2dW5ubm5LSUhZdGFoRTN2cU1jT3lMNy9weFE1?= =?utf-8?B?VEU5Ulc2ZFNKczF5NFZJeFVJbExGMk1lZngvSW52elp0b1gyRE9VcFAvenVp?= =?utf-8?B?NTc2NytUNWFWVkk5SmhCUWErSk1ldnVDMUlqSjRGTWhkcVR5TytucTZ3YVJn?= =?utf-8?B?UFlUODZTNXZLZ0NXTHN0dUdkWTNINVdHRG5ScGdHTC80cWdwUmhZeHJUVFp1?= =?utf-8?B?c0pMVzRSRnZLSG9GVWhNV05nM3dxR1AreUR6TlpDUXFpbjN2cEN3bFNPMERu?= =?utf-8?B?TjVYUzFqajNLSTkyY2JIcnRGZHA1V3VOZVV0U2h5dW9lbEhYYlZ6QVNhRm9R?= =?utf-8?B?aDliVU1EL0FtSm9ORUxNUzkxR1NkMGl2NnhENnM3bEg0VUFjeWk0Uk9ZYjVF?= =?utf-8?B?Y1FlSmJvbWdGNEFRdEVtalB6NWg1cHhZMTlSUFZxK00xZVJTOFNXQ092MlFw?= =?utf-8?B?QWxyMGh0aEFSYlZpNkxTenY4L0dPUW5ySzBENnVObFhYYnBPakFRY0x3TDFF?= =?utf-8?B?VkFJdHVuVlNNTGF3MjJQeUlyaE5EWDlNWmRzeHVYWk0rT2F4WTc2OW1RVkho?= =?utf-8?B?MWR5MEcrWk9vcy8yRGpYZ0owdUVidnZGdDNIOExuZGVSRDRWUGcyRG9VUlFF?= =?utf-8?B?M2dnYUQrazdEam5NblFkczdlS3MrZklMMmdLOTBxNTVVcVJTMW9OdVZTQmVK?= =?utf-8?B?YlgwVVBiclhWVjFRY2cvVGJNR2NBSk9nbEprZEpHdlVsa25LaGxJQW1yc3ZX?= =?utf-8?B?MUFhZEw4MWJUTWo0cEtrU2FmdHFJWjgrV2lYVDMyUEpMdDgxUkhsa2Z0SzdZ?= =?utf-8?Q?rT6fid8z56mnrk?= X-Microsoft-Exchange-Diagnostics: 1;BN1BFFO11HUB052;3:0CHsojPDuwo/Ww4S+65LUaya/pg175qMOh/+GaKeu8WmPzMKwI+lfYhGWQnfRCPGDJVQxLzE6dSDJujf1aqQSk9pYo9Ka8SieXmd9kV5GIPyaL9zOakM7mMTOjOCDqvpugl/pYyPqBaRRUJjPAOqBQ==;10:OqHng8/Ygqv4ltSnVH9LaSTbRJe3avWMTcFeiDZyCLrhTr/ZKIbqMmSbaWqAaKKQc2JB6BbQozTfw5rI9srx/nRjMCnExJdlxIAqe1KYKkw= X-OriginatorOrg: xilinx.com X-MS-Exchange-CrossTenant-OriginalArrivalTime: 29 May 2015 05:44:10.0946 (UTC) X-MS-Exchange-CrossTenant-Id: 657af505-d5df-48d0-8300-c31994686c5c X-MS-Exchange-CrossTenant-OriginalAttributedTenantConnectingIp: TenantId=657af505-d5df-48d0-8300-c31994686c5c;Ip=[149.199.60.100];Helo=[xsj-pvapsmtpgw02] X-MS-Exchange-CrossTenant-FromEntityHeader: HybridOnPrem X-MS-Exchange-Transport-CrossTenantHeadersStamped: BN1BFFO11HUB052 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 05/28/2015 07:35 PM, Moritz Fischer wrote: > On Wed, May 27, 2015 at 10:45 PM, Michal Simek wrote: >> On 05/27/2015 08:35 PM, Moritz Fischer wrote: >>> The Xilinx LogiCORE IP mailbox is a FPGA core that allows for >>> interprocessor communication via AXI4 memory mapped / AXI4 stream >>> interfaces. >>> >>> It is single channel per core and allows for transmit and receive. >>> >>> Changes from v1: >>> - Added common clock framework support >>> - Deal with IRQs that happend before driver load, >>> since HW will not let us know about them when we enable IRQs >>> >>> Changes from v0: >>> - Several stylistic issues >>> - Dropped superfluous intr_mode member >>> - Really masking the IRQs on mailbox_shutdown >>> - No longer using polling by accident in non-IRQ mode >>> - Swapped doc and driver commits >>> >>> Signed-off-by: Moritz Fischer >>> --- >>> MAINTAINERS | 7 + >>> drivers/mailbox/Kconfig | 8 + >>> drivers/mailbox/Makefile | 2 + >>> drivers/mailbox/mailbox-xilinx.c | 349 ++++++++++++++++++++++++++++++++++ >>> 4 files changed, 366 insertions(+) >>> >>> diff --git a/MAINTAINERS b/MAINTAINERS >>> index f8e0afb..f1f0d10 100644 >>> --- a/MAINTAINERS >>> +++ b/MAINTAINERS >>> @@ -10986,6 +10986,13 @@ M: John Linn >>> S: Maintained >>> F: drivers/net/ethernet/xilinx/xilinx_axienet* >>> >>> +XILINX MAILBOX DRIVER >>> +M: Moritz Fischer >>> +L: linux-kernel@vger.kernel.org >>> +S: Maintained >>> +F: drivers/mailbox/mailbox-xilinx.c >>> +F: Documentation/devicetree/bindings/mailbox/xilinx-mailbox.txt >>> + >>> XILINX UARTLITE SERIAL DRIVER >>> M: Peter Korsgaard >>> L: linux-serial@vger.kernel.org >>> diff --git a/drivers/mailbox/Kconfig b/drivers/mailbox/Kconfig >>> index 84b0a2d..e11e4b2 100644 >>> --- a/drivers/mailbox/Kconfig >>> +++ b/drivers/mailbox/Kconfig >>> @@ -60,4 +60,12 @@ config ALTERA_MBOX >>> An implementation of the Altera Mailbox soft core. It is used >>> to send message between processors. Say Y here if you want to use the >>> Altera mailbox support. >>> + >>> +config XILINX_MBOX >>> + tristate "Xilinx Mailbox" >>> + help >>> + An implementation of the Xilinx Mailbox soft core. It is used >>> + to send message between processors. Say Y here if you want to use the >>> + Xilinx mailbox support. >>> + >>> endif >>> diff --git a/drivers/mailbox/Makefile b/drivers/mailbox/Makefile >>> index b18201e..d28a028 100644 >>> --- a/drivers/mailbox/Makefile >>> +++ b/drivers/mailbox/Makefile >>> @@ -11,3 +11,5 @@ obj-$(CONFIG_OMAP2PLUS_MBOX) += omap-mailbox.o >>> obj-$(CONFIG_PCC) += pcc.o >>> >>> obj-$(CONFIG_ALTERA_MBOX) += mailbox-altera.o >>> + >>> +obj-$(CONFIG_XILINX_MBOX) += mailbox-xilinx.o >>> diff --git a/drivers/mailbox/mailbox-xilinx.c b/drivers/mailbox/mailbox-xilinx.c >>> new file mode 100644 >>> index 0000000..fd1cdf2 >>> --- /dev/null >>> +++ b/drivers/mailbox/mailbox-xilinx.c >>> @@ -0,0 +1,349 @@ >>> +/* >>> + * Copyright (c) 2015, National Instruments Corp. All rights reserved. >>> + * >>> + * Driver for the Xilinx LogiCORE mailbox IP block >>> + * >>> + * This program is free software; you can redistribute it and/or modify >>> + * it under the terms of the GNU General Public License as published by >>> + * the Free Software Foundation; version 2 of the License. >>> + * >>> + * 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. >>> + */ >>> + >>> +#include >>> +#include >>> +#include >>> +#include >>> +#include >>> +#include >>> +#include >>> +#include >>> +#include >>> + >>> +#define DRIVER_NAME "xilinx-mailbox" >>> + >>> +/* register offsets */ >>> +#define MAILBOX_REG_WRDATA 0x00 >>> +#define MAILBOX_REG_RDDATA 0x08 >>> +#define MAILBOX_REG_STATUS 0x10 >>> +#define MAILBOX_REG_ERROR 0x14 >>> +#define MAILBOX_REG_SIT 0x18 >>> +#define MAILBOX_REG_RIT 0x1c >>> +#define MAILBOX_REG_IS 0x20 >>> +#define MAILBOX_REG_IE 0x24 >>> +#define MAILBOX_REG_IP 0x28 >>> + >>> +/* status register */ >>> +#define STS_RTA BIT(3) >>> +#define STS_STA BIT(2) >>> +#define STS_FULL BIT(1) >>> +#define STS_EMPTY BIT(0) >>> + >>> +/* error register */ >>> +#define ERR_FULL BIT(1) >>> +#define ERR_EMPTY BIT(0) >>> + >>> +/* mailbox interrupt status register */ >>> +#define INT_STATUS_ERR BIT(2) >>> +#define INT_STATUS_RTI BIT(1) >>> +#define INT_STATUS_STI BIT(0) >>> + >>> +/* mailbox interrupt enable register */ >>> +#define INT_ENABLE_ERR BIT(2) >>> +#define INT_ENABLE_RTI BIT(1) >>> +#define INT_ENABLE_STI BIT(0) >>> + >>> +#define MBOX_POLLING_MS 5 /* polling interval 5ms */ >>> + >>> +struct xilinx_mbox { >>> + int irq; >>> + void __iomem *mbox_base; >>> + struct clk *clk; >>> + struct device *dev; >>> + struct mbox_controller controller; >>> + >>> + /* if the controller supports only RX polling mode */ >>> + struct timer_list rxpoll_timer; >>> +}; >>> + >>> +static struct xilinx_mbox *mbox_chan_to_xilinx_mbox(struct mbox_chan *chan) >>> +{ >>> + if (!chan || !chan->con_priv) >>> + return NULL; >>> + >>> + return (struct xilinx_mbox *)chan->con_priv; >>> +} >>> + >>> +static inline bool xilinx_mbox_full(struct xilinx_mbox *mbox) >>> +{ >>> + u32 status; >>> + >>> + status = readl_relaxed(mbox->mbox_base + MAILBOX_REG_STATUS); >>> + >>> + return status & STS_FULL; >>> +} >>> + >>> +static inline bool xilinx_mbox_pending(struct xilinx_mbox *mbox) >>> +{ >>> + u32 status; >>> + >>> + status = readl_relaxed(mbox->mbox_base + MAILBOX_REG_STATUS); >>> + >>> + return !(status & STS_EMPTY); >>> +} >>> + >>> +static void xilinx_mbox_intmask(struct xilinx_mbox *mbox, u32 mask, bool enable) >>> +{ >>> + u32 mask_reg; >>> + >>> + mask_reg = readl_relaxed(mbox->mbox_base + MAILBOX_REG_IE); >>> + if (enable) >>> + mask_reg |= mask; >>> + else >>> + mask_reg &= ~mask; >>> + writel_relaxed(mask_reg, mbox->mbox_base + MAILBOX_REG_IE); >>> +} >>> + >>> + >>> +static inline void xilinx_mbox_rx_intmask(struct xilinx_mbox *mbox, bool enable) >>> +{ >>> + xilinx_mbox_intmask(mbox, INT_ENABLE_RTI, enable); >>> +} >>> + >>> +static inline void xilinx_mbox_tx_intmask(struct xilinx_mbox *mbox, bool enable) >>> +{ >>> + xilinx_mbox_intmask(mbox, INT_ENABLE_STI, enable); >>> +} >>> + >>> +static void xilinx_mbox_rx_data(struct mbox_chan *chan) >>> +{ >>> + struct xilinx_mbox *mbox = mbox_chan_to_xilinx_mbox(chan); >>> + u32 data; >>> + >>> + if (xilinx_mbox_pending(mbox)) { >>> + data = readl_relaxed(mbox->mbox_base + MAILBOX_REG_RDDATA); >>> + mbox_chan_received_data(chan, (void *)data); >>> + } >>> +} >>> + >>> +static void xilinx_mbox_poll_rx(unsigned long data) >>> +{ >>> + struct mbox_chan *chan = (struct mbox_chan *)data; >>> + struct xilinx_mbox *mbox = mbox_chan_to_xilinx_mbox(chan); >>> + >>> + xilinx_mbox_rx_data(chan); >>> + >>> + mod_timer(&mbox->rxpoll_timer, >>> + jiffies + msecs_to_jiffies(MBOX_POLLING_MS)); >>> +} >>> + >>> +static irqreturn_t xilinx_mbox_interrupt(int irq, void *p) >>> +{ >>> + u32 mask; >>> + struct mbox_chan *chan = (struct mbox_chan *)p; >>> + struct xilinx_mbox *mbox = mbox_chan_to_xilinx_mbox(chan); >>> + >>> + mask = readl_relaxed(mbox->mbox_base + MAILBOX_REG_IS); >>> + if (mask & INT_STATUS_RTI) >>> + xilinx_mbox_rx_data(chan); >>> + >>> + /* mask irqs *before* notifying done, require tx_block=true */ >>> + if (mask & INT_STATUS_STI) { >>> + xilinx_mbox_tx_intmask(mbox, false); >>> + mbox_chan_txdone(chan, 0); >>> + } >>> + >>> + writel_relaxed(mask, mbox->mbox_base + MAILBOX_REG_IS); >> >> empty line here please. > > Will do >>> + return IRQ_HANDLED; >>> +} >>> + >>> +static int xilinx_mbox_startup(struct mbox_chan *chan) >>> +{ >>> + int ret; >>> + struct xilinx_mbox *mbox = mbox_chan_to_xilinx_mbox(chan); >>> + >>> + if (mbox->irq >= 0) { >>> + ret = request_irq(mbox->irq, xilinx_mbox_interrupt, 0, >>> + dev_name(mbox->dev), chan); >>> + if (unlikely(ret)) { >>> + dev_err(mbox->dev, >>> + "failed to register mailbox interrupt:%d\n", >>> + ret); >>> + goto polling; /* use polling if failed */ >> >> This is the problematic path. For case that request_irq failed - you are >> using polling mode but in shutdown below mbox_irq > 0 and you will >> free_irq not call del_timer_sync. >> I understand that you want to be smart here but it should be enough just >> to return error here and you are done. But up2you I expect that this >> path wasn't tested. > > Yeah, trying to be smart will likely go wrong. I'm fine with returning ret here. >>> + } >>> + >>> + xilinx_mbox_rx_intmask(mbox, true); >>> + >>> + /* if fifo was full already, we didn't get an interrupt */ >>> + while (xilinx_mbox_pending(mbox)) >>> + xilinx_mbox_rx_data(chan); >>> + >>> + return 0; >>> + } >>> + >>> +polling: >>> + /* setup polling timer */ >>> + setup_timer(&mbox->rxpoll_timer, xilinx_mbox_poll_rx, >>> + (unsigned long)chan); >>> + mod_timer(&mbox->rxpoll_timer, >>> + jiffies + msecs_to_jiffies(MBOX_POLLING_MS)); >>> + >>> + return 0; >>> +} >>> + >>> +static int xilinx_mbox_send_data(struct mbox_chan *chan, void *data) >>> +{ >>> + struct xilinx_mbox *mbox = mbox_chan_to_xilinx_mbox(chan); >>> + u32 *udata = (u32 *)data; >>> + >>> + if (!mbox || !data) >>> + return -EINVAL; >>> + >>> + if (xilinx_mbox_full(mbox)) >>> + return -EBUSY; >>> + >>> + /* enable interrupt before send */ >>> + if (mbox->irq >= 0) >>> + xilinx_mbox_tx_intmask(mbox, true); >>> + >>> + writel_relaxed(*udata, mbox->mbox_base + MAILBOX_REG_WRDATA); >>> + >>> + return 0; >>> +} >>> + >>> +static bool xilinx_mbox_last_tx_done(struct mbox_chan *chan) >>> +{ >>> + struct xilinx_mbox *mbox = mbox_chan_to_xilinx_mbox(chan); >>> + >>> + /* return false if mailbox is full */ >>> + return !xilinx_mbox_full(mbox); >>> +} >>> + >>> +static bool xilinx_mbox_peek_data(struct mbox_chan *chan) >>> +{ >>> + struct xilinx_mbox *mbox = mbox_chan_to_xilinx_mbox(chan); >>> + >>> + return xilinx_mbox_pending(mbox); >>> +} >>> + >>> +static void xilinx_mbox_shutdown(struct mbox_chan *chan) >>> +{ >>> + struct xilinx_mbox *mbox = mbox_chan_to_xilinx_mbox(chan); >>> + >>> + if (mbox->irq >= 0) { >> >> look below. >> >>> + /* mask all interrupts */ >>> + writel_relaxed(0, mbox->mbox_base + MAILBOX_REG_IE); >>> + free_irq(mbox->irq, chan); >>> + } else { >>> + del_timer_sync(&mbox->rxpoll_timer); >>> + } >>> +} >>> + >>> +static struct mbox_chan_ops xilinx_mbox_ops = { >>> + .send_data = xilinx_mbox_send_data, >>> + .startup = xilinx_mbox_startup, >>> + .shutdown = xilinx_mbox_shutdown, >>> + .last_tx_done = xilinx_mbox_last_tx_done, >>> + .peek_data = xilinx_mbox_peek_data, >>> +}; >>> + >>> +static int xilinx_mbox_probe(struct platform_device *pdev) >>> +{ >>> + struct xilinx_mbox *mbox; >>> + struct resource *regs; >>> + struct mbox_chan *chans; >>> + int ret; >>> + >>> + mbox = devm_kzalloc(&pdev->dev, sizeof(*mbox), GFP_KERNEL); >>> + if (!mbox) >>> + return -ENOMEM; >>> + >>> + /* get clk and enable */ >>> + mbox->clk = devm_clk_get(&pdev->dev, "mbox"); >>> + clk_prepare_enable(mbox->clk); >> >> huh you should probably check error first before enabling. >> Also if you call enable there, you should also call >> clk_disable_unprepare when any error here happen. >> Probably make sense to move this clock enabling below. > > Ouch, good catch. The problem with moving the enable further down is, > that reading the regs requires the clock to already be up and running. > Suggestions? just put it to the best suitable location and handle error path correctly. Thanks, Michal