Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752605AbbGMW1h (ORCPT ); Mon, 13 Jul 2015 18:27:37 -0400 Received: from seldrel01.sonyericsson.com ([37.139.156.2]:9885 "EHLO seldrel01.sonyericsson.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751510AbbGMW1f (ORCPT ); Mon, 13 Jul 2015 18:27:35 -0400 Date: Mon, 13 Jul 2015 15:27:29 -0700 From: Bjorn Andersson To: Georgi Djakov CC: , Kumar Gala , Andy Gross , David Brown , , , Jeffrey Hugo , Subject: Re: [PATCH v2 04/11] soc: qcom: Add Shared Memory Driver Message-ID: <20150713222729.GC15178@usrtlx11787.corpusers.net> References: <1435355419-23602-1-git-send-email-bjorn.andersson@sonymobile.com> <1435355419-23602-5-git-send-email-bjorn.andersson@sonymobile.com> <559BD809.3060003@linaro.org> MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Disposition: inline In-Reply-To: <559BD809.3060003@linaro.org> User-Agent: Mutt/1.5.22 (2013-10-16) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 5612 Lines: 177 On Tue 07 Jul 06:45 PDT 2015, Georgi Djakov wrote: > Hi Bjorn, > Thank you for this patchset! Some nits and a question below. > Thank you! > On 06/27/2015 12:50 AM, bjorn@kryo.se wrote: > > From: Bjorn Andersson > > > > This adds the Qualcomm Shared Memory Driver (SMD) providing > > communication channels to remote processors, ontop of SMEM. > > > > Signed-off-by: Bjorn Andersson > > --- > > drivers/soc/qcom/Kconfig | 8 + > > drivers/soc/qcom/Makefile | 1 + > > drivers/soc/qcom/smd.c | 1324 ++++++++++++++++++++++++++++++++++++++++++ > > include/linux/soc/qcom/smd.h | 46 ++ > > 4 files changed, 1379 insertions(+) > > create mode 100644 drivers/soc/qcom/smd.c > > create mode 100644 include/linux/soc/qcom/smd.h > > > [...] > > --- /dev/null > > +++ b/drivers/soc/qcom/smd.c > > @@ -0,0 +1,1324 @@ > > +/* > > + * Copyright (c) 2015, Sony Mobile Communications AB. > > + * Copyright (c) 2012-2013, The Linux Foundation. All rights reserved. > > + * > > + * This program is free software; you can redistribute it and/or modify > > + * it under the terms of the GNU General Public License version 2 and > > + * only version 2 as published by the Free Software Foundation. > > + * > > + * This program is distributed in the hope that it will be useful, > > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > > + * GNU General Public License for more details. > > + */ > > + > > +#include > > unused? > Right, used to just delay on the tx buffer being full, will drop. > [...] > > + > > +#define GET_RX_CHANNEL_INFO(channel, param) \ > > + (channel->rx_info_word ? \ > > + channel->rx_info_word->param : \ > > + channel->rx_info->param) > > + > > +#define SET_RX_CHANNEL_INFO(channel, param, value) \ > > + (channel->rx_info_word ? \ > > + (channel->rx_info_word->param = value) : \ > > + (channel->rx_info->param = value)) > > + > > +#define GET_TX_CHANNEL_INFO(channel, param) \ > > + (channel->rx_info_word ? \ > > Maybe this should be tx_info_word? > There's no practical difference, but I'll update it. > > + channel->tx_info_word->param : \ > > + channel->tx_info->param) > > + > > +#define SET_TX_CHANNEL_INFO(channel, param, value) \ > > + (channel->rx_info_word ? \ > > ditto? > ditto... > > + (channel->tx_info_word->param = value) : \ > > + (channel->tx_info->param = value)) > > + > [...] > > + ret = qcom_smem_get(edge->edge_id, smem_fifo_item, &fifo_base, &fifo_size); > > + if (ret) > > + goto free_name_and_channel; > > + > > + /* The channel consist of a rx and tx fifo of equal size */ > > + fifo_size /= 2; > > + > > + dev_dbg(smd->dev, "new channel '%s' info-size: %d fifo-size: %zu\n", > > %zu for info-size? > Hmm, the compiler only complained about the fifo_size, but you're of course right. > > + name, info_size, fifo_size); > > + > > [...] > > +static int __init qcom_smd_init(void) > > +{ > > + int ret; > > + > > + ret = bus_register(&qcom_smd_bus); > > + if (ret) { > > + pr_err("failed to register smd bus: %d\n", ret); > > + return ret; > > + } > > + > > + return platform_driver_register(&qcom_smd_driver); > > +} > > +arch_initcall(qcom_smd_init); > > + > > +static void __exit qcom_smd_exit(void) > > +{ > > + platform_driver_unregister(&qcom_smd_driver); > > + bus_unregister(&qcom_smd_bus); > > +} > > +module_exit(qcom_smd_exit); > > + > [...] > > +/** > > + * struct qcom_smd_driver - smd driver struct > > + * @driver: underlying device driver > > + * @probe: invoked when the smd channel is found > > + * @remove: invoked when the smd channel is closed > > + * @callback: invoked when an inbound message is received on the channel, > > + * should return 0 on success or -EBUSY if the data cannot be > > + * consumed at this time > > + */ > > +struct qcom_smd_driver { > > + struct device_driver driver; > > + int (*probe)(struct qcom_smd_device *dev); > > + void (*remove)(struct qcom_smd_device *dev); > > + int (*callback)(struct qcom_smd_device *, const void *, size_t); > > +}; > > + > > +int qcom_smd_driver_register(struct qcom_smd_driver *drv); > > +void qcom_smd_driver_unregister(struct qcom_smd_driver *drv); > > + > > +#define module_qcom_smd_driver(__smd_driver) \ > > + module_driver(__smd_driver, qcom_smd_driver_register, \ > > + qcom_smd_driver_unregister) > > + > > This comment is mostly related to your RPM over SMD driver patch, as > i have a RPM clock driver based on it. The RPM clock driver registers > some fundamental stuff like XO and i had to hack smd-rpm to probe > earlier, so that most other drivers can initialize. So i was wondering, > what if we register the drivers on the bus earlier? What do you think? > My only concern would be that if we're calling qcom_smd_driver_register() before the smd arch_initcall has registered the bus it will fail. Part of this I see no problem with modifying the rpm driver to register earlier - and it would be good to have those regulators earlier as well... I've intentionally not done anything about this, because it's helped to smoke out a bunch of EPROBE_DEFER issues for me already, but longer term it's not okay for all our drivers to fail 2-3 times before the regulators are up... Regards, Bjorn -- 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/