From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-2.6 required=3.0 tests=DKIM_SIGNED,DKIM_VALID, DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,SPF_HELO_NONE, SPF_PASS,URIBL_BLOCKED,USER_AGENT_MUTT autolearn=unavailable autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id D2313C28CC3 for ; Sat, 1 Jun 2019 00:09:21 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id A8DDE2705E for ; Sat, 1 Jun 2019 00:09:21 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=linaro.org header.i=@linaro.org header.b="I9FC/0ZN" Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1727161AbfFAAJV (ORCPT ); Fri, 31 May 2019 20:09:21 -0400 Received: from mail-pf1-f194.google.com ([209.85.210.194]:39843 "EHLO mail-pf1-f194.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1727141AbfFAAJU (ORCPT ); Fri, 31 May 2019 20:09:20 -0400 Received: by mail-pf1-f194.google.com with SMTP id j2so7122133pfe.6 for ; Fri, 31 May 2019 17:09:20 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to:user-agent; bh=nU7TNgR5LpcNvtYfSjIrl1EToZnQ809br5ciYzVxz7U=; b=I9FC/0ZN/P/uw2vcGA853atGEf+PUvmHuWe6siUN9uSS4JO2+5MB8qHRmZy20iRVpB YU72lxTiwZZ/cBdhfRmt7Zy9/C4xG2ZRKMpeA92Y5B2NXOz/SkEa4hL6q+pZoWboE9MU 26z1rzEZaUZj1mx/EQ2vdmwpGyEj0YQpvCPLbv3RQuaK0IL2yu6SamJAtAgubX8z8nrM O753bm2N089NIIjQ6BNnrFigotYL4rmqLenI31/X9TBd3m46slJ44Z8tN0NMky7Rrsne ojR5pFxSLUTBs1UtZx25wL/TpcPYTVC8ez1Kkd38jLrJLXcsZXXVrXufvs70Qnidi2Sw vtDQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to:user-agent; bh=nU7TNgR5LpcNvtYfSjIrl1EToZnQ809br5ciYzVxz7U=; b=TivTH/21E3BYyhiS/SNMMF4NT1i7ZR1uieVJLnWqjfTSZtiehOIDLoJMrWIEC743Et QJbo2xLq9eHIgHnCDOwF6gD3W9WLZPMt9NwQmK2a1ZDRNAH0QMOC3bI0Si687pfhJCPZ UMObT/ypNvth1BmV7OsCRnNdbsNxpPG89Va/OCsOIpxy1s8hXbRSTlRa0umIscA7Rbma XFbgCZu5T8vj6ZTLI3QHuR3zEGz2YYLcYFD9gzeTlmawPo4KUgktoLsmswzXIE3yxl4V DChbdgH+WIAIiwJ5kg91qraGBdsEjSasI+5WpOBjnmKzIfFoPZ49dZvrhu/LYzfrtINa EnTA== X-Gm-Message-State: APjAAAV5maTEjhka/jby+asEHW0ZRiGCC3KQ1PBzFZ4X2I3jY8Ub2+Yf UEhqSIl+dG8kK2aXZFP47xp/3Q== X-Google-Smtp-Source: APXvYqyy+66Tc/ezRAw9Zd8t0fwoOYr/EWkFnuNA2Mr7izQNHyFv54Hb3h8WLJ37PE1ILgdrom0QRg== X-Received: by 2002:a62:582:: with SMTP id 124mr13734526pff.209.1559347760035; Fri, 31 May 2019 17:09:20 -0700 (PDT) Received: from minitux (104-188-17-28.lightspeed.sndgca.sbcglobal.net. [104.188.17.28]) by smtp.gmail.com with ESMTPSA id e4sm6451863pgi.80.2019.05.31.17.09.18 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Fri, 31 May 2019 17:09:19 -0700 (PDT) Date: Fri, 31 May 2019 17:09:17 -0700 From: Bjorn Andersson To: Doug Anderson Cc: Andy Gross , David Brown , Rob Herring , Mark Rutland , Arun Kumar Neelakantam , Vinod Koul , linux-arm-msm , devicetree@vger.kernel.org, LKML Subject: Re: [PATCH v8 2/4] soc: qcom: Add AOSS QMP driver Message-ID: <20190601000917.GE25597@minitux> References: <20190531030057.18328-1-bjorn.andersson@linaro.org> <20190531030057.18328-3-bjorn.andersson@linaro.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.11.3 (2019-02-01) Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Fri 31 May 15:24 PDT 2019, Doug Anderson wrote: > Hi, > > On Thu, May 30, 2019 at 8:01 PM Bjorn Andersson > wrote: > > > > +/** > > + * qmp_send() - send a message to the AOSS > > + * @qmp: qmp context > > + * @data: message to be sent > > + * @len: length of the message > > + * > > + * Transmit @data to AOSS and wait for the AOSS to acknowledge the message. > > + * @len must be a multiple of 4 and not longer than the mailbox size. Access is > > + * synchronized by this implementation. > > + * > > + * Return: 0 on success, negative errno on failure > > + */ > > +static int qmp_send(struct qmp *qmp, const void *data, size_t len) > > +{ > > + int ret; > > + > > + if (WARN_ON(len + sizeof(u32) > qmp->size)) > > + return -EINVAL; > > + > > + if (WARN_ON(len % sizeof(u32))) > > + return -EINVAL; > > + > > + mutex_lock(&qmp->tx_lock); > > + > > + /* The message RAM only implements 32-bit accesses */ > > + __iowrite32_copy(qmp->msgram + qmp->offset + sizeof(u32), > > + data, len / sizeof(u32)); > > + writel(len, qmp->msgram + qmp->offset); > > + qmp_kick(qmp); > > + > > + ret = wait_event_interruptible_timeout(qmp->event, > > + qmp_message_empty(qmp), HZ); > > + if (!ret) { > > + dev_err(qmp->dev, "ucore did not ack channel\n"); > > + ret = -ETIMEDOUT; > > + > > + /* Clear message from buffer */ > > + writel(0, qmp->msgram + qmp->offset); > > + } else { > > + ret = 0; > > + } > > Just like Vinod said in in v7, the "ret = 0" is redundant. > If the condition passed to wait_event_interruptible_timeout() evaluates true the remote side has consumed the message and ret will be 1. We end up in the else block (i.e. not timeout) and we want the function to return 0, so we set ret to 0. Please let me know if I'm reading this wrong. > > > +static int qmp_qdss_clk_add(struct qmp *qmp) > > +{ > > + struct clk_init_data qdss_init = { > > + .ops = &qmp_qdss_clk_ops, > > + .name = "qdss", > > + }; > > As I mentioned in v7, there is no downside in marking qdss_init as > "static const" and it avoids the compiler inserting a memcpy() to get > this data on the stack. Using static const also reduces your stack > usage. > In which case we would just serve it from .ro, makes sense now that I read your comment again. > > > + int ret; > > + > > + qmp->qdss_clk.init = &qdss_init; > > + ret = clk_hw_register(qmp->dev, &qmp->qdss_clk); > > + if (ret < 0) { > > + dev_err(qmp->dev, "failed to register qdss clock\n"); > > + return ret; > > + } > > + > > + ret = of_clk_add_hw_provider(qmp->dev->of_node, of_clk_hw_simple_get, > > + &qmp->qdss_clk); > > I still prefer to devm-ify the whole driver, using > devm_add_action_or_reset() to handle things where there is no devm. > ...but I won't insist. > > > Above things are just nits and I won't insist. They also could be > addressed in follow-up patches. Thus: > > Reviewed-by: Douglas Anderson Thanks! Regards, Bjorn