Message ID | 20230112171840.2098463-3-ckeepax@opensource.cirrus.com |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a5d:4e01:0:0:0:0:0 with SMTP id p1csp4023112wrt; Thu, 12 Jan 2023 10:00:41 -0800 (PST) X-Google-Smtp-Source: AMrXdXveHvBh29p5UBzPeXl+5edMruwvAz61gplcvkYz78m5zyRTpTOdlS98CZ1AqbuKfAoY7Pz/ X-Received: by 2002:a17:902:eb8c:b0:193:2ed4:5623 with SMTP id q12-20020a170902eb8c00b001932ed45623mr16481741plg.26.1673546441251; Thu, 12 Jan 2023 10:00:41 -0800 (PST) ARC-Seal: i=1; a=rsa-sha256; t=1673546441; cv=none; d=google.com; s=arc-20160816; b=h5PWnZlazlg7S62ACuR9crajJiorEG9ZJvSgmCKapnUez3i3aAX8y07IihrREnRMqk Xf0aH0eXAmYLD0Q0kNfymNXP0nTzRSAh+Pa+GAANZArYGVRumSbpENVmSoux44LDBO7Q SN/TOkJ41XTPZ434vVIPm7TShchz3Cr3msOGPdV/4KE+fTtE+7fnSnZPPoP0tPDxHTb/ 6Ci5Q5GWua7Vu/aePPhjYLZSvL7y5RgvxXWdG7fkOUPjzGtZkUYMfIIhpjIckcEr3cQ6 q3EXzD3d8DecvW/dZaeuGwUvwKPOlVuVn+e+aIzD8F71dfGTmEwrhMAjKQpnr4gzjnVq tpoA== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:content-transfer-encoding:mime-version :references:in-reply-to:message-id:date:subject:cc:to:from :dkim-signature; bh=oiMd4WrK0Y9BeberYzyibU3Ja1FQD3WINBfFgZgaxeE=; b=vcQAWzQoIlioVT6Rv1fD/6a5WHSlbY48BBs61N+NekKW6MHW0R//56ZubDJBdKRuTZ 0oFSjdRYAvZzD9U5qsq3354CVLF/vn0RbrJnLSeca4YW6knxWpYL6+4YDLTc4YOYKFNC IucQtZnvvc8O3wuQj5PfygfibStlfxV1NWZzxyEgPqy+AKpBs8kRGK/E6EJ1+cURZN6s LvhTaUOAFCbHL2pE7OI+v603fauLg07X5LnJLjRoQnWcOAJcCqWPN2IHT2pI/kVwG1vq VLEMDBEu7xzkbDi0UuZ1of/X7buv3qgTgQtUHMStmYqh+YcKVG/1GixFSRpteB+Fmfud eRnQ== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@cirrus.com header.s=PODMain02222019 header.b=hQhorqXm; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=REJECT sp=REJECT dis=NONE) header.from=cirrus.com Received: from out1.vger.email (out1.vger.email. [2620:137:e000::1:20]) by mx.google.com with ESMTP id n15-20020a170902e54f00b00192f48dfd30si19209523plf.327.2023.01.12.10.00.27; Thu, 12 Jan 2023 10:00:41 -0800 (PST) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20; Authentication-Results: mx.google.com; dkim=pass header.i=@cirrus.com header.s=PODMain02222019 header.b=hQhorqXm; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=REJECT sp=REJECT dis=NONE) header.from=cirrus.com Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S232421AbjALSAG (ORCPT <rfc822;zhuangel570@gmail.com> + 99 others); Thu, 12 Jan 2023 13:00:06 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:39700 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S230385AbjALR7M (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Thu, 12 Jan 2023 12:59:12 -0500 Received: from mx0b-001ae601.pphosted.com (mx0b-001ae601.pphosted.com [67.231.152.168]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 772E57DE0D for <linux-kernel@vger.kernel.org>; Thu, 12 Jan 2023 09:18:55 -0800 (PST) Received: from pps.filterd (m0077474.ppops.net [127.0.0.1]) by mx0b-001ae601.pphosted.com (8.17.1.19/8.17.1.19) with ESMTP id 30CF7JfD026944; Thu, 12 Jan 2023 11:18:43 -0600 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=cirrus.com; h=from : to : cc : subject : date : message-id : in-reply-to : references : mime-version : content-transfer-encoding : content-type; s=PODMain02222019; bh=oiMd4WrK0Y9BeberYzyibU3Ja1FQD3WINBfFgZgaxeE=; b=hQhorqXm15mCNMA3UraK2h9I9pVxSLcVkmgluVBfdPWDFtaTXb+GWdeSLsyU0epP2HVw Cbo6dyWjjbQwzcuQmSOTpY7nKhHpI5s9CIsGGKSIFE6N8jUq6tA17pdZ0ek4bUsTM7zb mfhuV2keLrrojSYBj53gk7ollAYJ4wTtLFeJeeZtxFQHud3liLDzyZ3cqcbpzUDRqZnY dMknC0SBjUjD4cjCvzDapI4I6Gg8Y81JhxLmKhboG+QVC1BA3j6rbt9cjUKSA2tc3TWO cxeIGP0hV7jqjTLGvTiILY2qSUk7/Nkj935OsycEVIoea0OoccfBpA/FeQ7VOdVEIecs qQ== Received: from ediex02.ad.cirrus.com ([84.19.233.68]) by mx0b-001ae601.pphosted.com (PPS) with ESMTPS id 3n1k582d2g-3 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Thu, 12 Jan 2023 11:18:43 -0600 Received: from ediex01.ad.cirrus.com (198.61.84.80) by ediex02.ad.cirrus.com (198.61.84.81) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.2.1118.21; Thu, 12 Jan 2023 11:18:40 -0600 Received: from ediswmail.ad.cirrus.com (198.61.86.93) by ediex01.ad.cirrus.com (198.61.84.80) with Microsoft SMTP Server id 15.2.1118.21 via Frontend Transport; Thu, 12 Jan 2023 11:18:40 -0600 Received: from algalon.ad.cirrus.com (algalon.ad.cirrus.com [198.90.251.122]) by ediswmail.ad.cirrus.com (Postfix) with ESMTP id 87D2611CB; Thu, 12 Jan 2023 17:18:40 +0000 (UTC) From: Charles Keepax <ckeepax@opensource.cirrus.com> To: <broonie@kernel.org>, <vkoul@kernel.org> CC: <yung-chuan.liao@linux.intel.com>, <pierre-louis.bossart@linux.intel.com>, <sanyog.r.kale@intel.com>, <linux-kernel@vger.kernel.org>, <patches@opensource.cirrus.com> Subject: [PATCH 2/2] regmap: sdw: Remove 8-bit value size restriction Date: Thu, 12 Jan 2023 17:18:40 +0000 Message-ID: <20230112171840.2098463-3-ckeepax@opensource.cirrus.com> X-Mailer: git-send-email 2.30.2 In-Reply-To: <20230112171840.2098463-1-ckeepax@opensource.cirrus.com> References: <20230112171840.2098463-1-ckeepax@opensource.cirrus.com> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit Content-Type: text/plain X-Proofpoint-ORIG-GUID: PIqSdmW65vTh42YKbBI4N4XajQ5pJHZt X-Proofpoint-GUID: PIqSdmW65vTh42YKbBI4N4XajQ5pJHZt X-Proofpoint-Spam-Reason: safe X-Spam-Status: No, score=-2.7 required=5.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_EF,RCVD_IN_DNSWL_LOW,SPF_HELO_NONE,SPF_PASS autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on lindbergh.monkeyblade.net Precedence: bulk List-ID: <linux-kernel.vger.kernel.org> X-Mailing-List: linux-kernel@vger.kernel.org X-getmail-retrieved-from-mailbox: =?utf-8?q?INBOX?= X-GMAIL-THRID: =?utf-8?q?1754840633319951751?= X-GMAIL-MSGID: =?utf-8?q?1754840633319951751?= |
Series |
Minor SoundWire Regmap Tweaks
|
|
Commit Message
Charles Keepax
Jan. 12, 2023, 5:18 p.m. UTC
From: Lucas Tanure <tanureal@opensource.cirrus.com> Some SoundWire devices have larger width device specific register maps, in addition to the standard SoundWire 8-bit map. Update the helpers to allow accessing arbitrarily sized register values and remove the explicit 8-bit restriction from regmap_sdw_config_check. Signed-off-by: Lucas Tanure <tanureal@opensource.cirrus.com> Signed-off-by: Charles Keepax <ckeepax@opensource.cirrus.com> --- This patch requires the patch commit 62dc9f3f2fd0 ("soundwire: bus: export sdw_nwrite_no_pm and sdw_nread_no_pm functions") from Vinod's SoundWire tree to build, so not sure if we want to push these patches through his tree or merge his tree across. drivers/base/regmap/regmap-sdw.c | 39 ++++++++++++++++++++------------ 1 file changed, 24 insertions(+), 15 deletions(-)
Comments
> -static int regmap_sdw_write(void *context, unsigned int reg, unsigned int val) > +static int regmap_sdw_write(void *context, const void *val_buf, size_t val_size) > { > struct device *dev = context; > struct sdw_slave *slave = dev_to_sdw_dev(dev); > + /* First word of buffer contains the destination address */ > + u32 addr = le32_to_cpu(*(const __le32 *)val_buf); > + const u8 *val = val_buf; > > - return sdw_write_no_pm(slave, reg, val); > + return sdw_nwrite_no_pm(slave, addr, val_size - sizeof(addr), val + sizeof(addr)); > } > > -static int regmap_sdw_read(void *context, unsigned int reg, unsigned int *val) > +static int regmap_sdw_gather_write(void *context, > + const void *reg_buf, size_t reg_size, > + const void *val_buf, size_t val_size) > { > struct device *dev = context; > struct sdw_slave *slave = dev_to_sdw_dev(dev); > - int read; > + u32 addr = le32_to_cpu(*(const __le32 *)reg_buf); what's the difference between regmap_sdw_write() and regmap_sdw_gather_write()? Seems to me that it's the same functionality of writing at consecutive addresses. It's not a true 'gather' in the sense that only the first address is used? > > - read = sdw_read_no_pm(slave, reg); > - if (read < 0) > - return read; > + return sdw_nwrite_no_pm(slave, addr, val_size, val_buf); > +} > > - *val = read; > - return 0;
On Thu, Jan 12, 2023 at 11:38:38AM -0600, Pierre-Louis Bossart wrote: > > +static int regmap_sdw_gather_write(void *context, > > + const void *reg_buf, size_t reg_size, > > + const void *val_buf, size_t val_size) > > { > > struct device *dev = context; > > struct sdw_slave *slave = dev_to_sdw_dev(dev); > > - int read; > > + u32 addr = le32_to_cpu(*(const __le32 *)reg_buf); > what's the difference between regmap_sdw_write() and > regmap_sdw_gather_write()? Seems to me that it's the same functionality > of writing at consecutive addresses. It's not a true 'gather' in the > sense that only the first address is used? The regmap gather_write() operation allows the bus to take two buffers, one for the register and one for the value, rather than requiring the core combine everything into a single buffer (mainly useful for large transfers like firmware downloads).
On 1/12/23 12:14, Mark Brown wrote: > On Thu, Jan 12, 2023 at 11:38:38AM -0600, Pierre-Louis Bossart wrote: > >>> +static int regmap_sdw_gather_write(void *context, >>> + const void *reg_buf, size_t reg_size, >>> + const void *val_buf, size_t val_size) >>> { >>> struct device *dev = context; >>> struct sdw_slave *slave = dev_to_sdw_dev(dev); >>> - int read; >>> + u32 addr = le32_to_cpu(*(const __le32 *)reg_buf); > >> what's the difference between regmap_sdw_write() and >> regmap_sdw_gather_write()? Seems to me that it's the same functionality >> of writing at consecutive addresses. It's not a true 'gather' in the >> sense that only the first address is used? > > The regmap gather_write() operation allows the bus to take two buffers, > one for the register and one for the value, rather than requiring the > core combine everything into a single buffer (mainly useful for large > transfers like firmware downloads). Right, but that's not supported in SoundWire. sdw_nwrite() will only work with consecutive addresses - and the auto-increment is handled in software, not hardware. What's suggested here is to use the first element of reg_buf, which begs the question how different this is from a regular write. If there was a discontinuity in reg_buf then this wouldn't work at all.
On Thu, Jan 12, 2023 at 12:43:46PM -0600, Pierre-Louis Bossart wrote: > On 1/12/23 12:14, Mark Brown wrote: > > The regmap gather_write() operation allows the bus to take two buffers, > > one for the register and one for the value, rather than requiring the > > core combine everything into a single buffer (mainly useful for large > > transfers like firmware downloads). > Right, but that's not supported in SoundWire. sdw_nwrite() will only > work with consecutive addresses - and the auto-increment is handled in > software, not hardware. No, that's exactly what this is for. It's for the *register address* being in a separate buffer, the data is then a sequence of consecutive register values. > What's suggested here is to use the first element of reg_buf, which begs > the question how different this is from a regular write. If there was a > discontinuity in reg_buf then this wouldn't work at all. reg_buf contains the address of exactly one register.
On 1/12/23 13:50, Mark Brown wrote: > On Thu, Jan 12, 2023 at 12:43:46PM -0600, Pierre-Louis Bossart wrote: >> On 1/12/23 12:14, Mark Brown wrote: > >>> The regmap gather_write() operation allows the bus to take two buffers, >>> one for the register and one for the value, rather than requiring the >>> core combine everything into a single buffer (mainly useful for large >>> transfers like firmware downloads). > >> Right, but that's not supported in SoundWire. sdw_nwrite() will only >> work with consecutive addresses - and the auto-increment is handled in >> software, not hardware. > > No, that's exactly what this is for. It's for the *register address* > being in a separate buffer, the data is then a sequence of consecutive > register values.> >> What's suggested here is to use the first element of reg_buf, which begs >> the question how different this is from a regular write. If there was a >> discontinuity in reg_buf then this wouldn't work at all. > > reg_buf contains the address of exactly one register. So what's the difference with a plain write() of N data?
On Thu, Jan 12, 2023 at 02:19:29PM -0600, Pierre-Louis Bossart wrote: > On 1/12/23 13:50, Mark Brown wrote: > > On Thu, Jan 12, 2023 at 12:43:46PM -0600, Pierre-Louis Bossart wrote: > >> On 1/12/23 12:14, Mark Brown wrote: > > > >>> The regmap gather_write() operation allows the bus to take two buffers, > >>> one for the register and one for the value, rather than requiring the > >>> core combine everything into a single buffer (mainly useful for large > >>> transfers like firmware downloads). > > > >> Right, but that's not supported in SoundWire. sdw_nwrite() will only > >> work with consecutive addresses - and the auto-increment is handled in > >> software, not hardware. > > > > No, that's exactly what this is for. It's for the *register address* > > being in a separate buffer, the data is then a sequence of consecutive > > register values.> > >> What's suggested here is to use the first element of reg_buf, which begs > >> the question how different this is from a regular write. If there was a > >> discontinuity in reg_buf then this wouldn't work at all. > > > > reg_buf contains the address of exactly one register. > > So what's the difference with a plain write() of N data? There are two back end interfaces in regmap, the reg_write/read and the plain write/read. Both have currently have some limitations when dealing with SoundWire. The reg_write/reg_read can only deal with a single register at a time, which is really far from ideal, since it means all transactions will be broken up into individual registers at the regmap level, mostly depriving the SoundWire side of the opportunity to do things like a BRA transfer if it deems that suitable. And denying users the ability to use the regmap_raw_read/write API at all. The write/read interface allows us to pass the full transaction through, but does have the downside it copies the address around a bit more and does some pointless endian swaps on big endian systems. This interface is generally used by buses like I2C/SPI where there is no actual concept of a register address only a buffer of bytes to be sent/read, thus prefers to pass a single working buffer if it sensibly can. I went with this solution because it enables all the functionality and the downside is fairly minimal, apart from looking a little clunky as you note. I guess ideally from SoundWire's point of view expanding the first interface to allow multi-register transactions would make the most sense, but that a fairly invasive change to regmap and one I am a little hesitant to get into right now. The half-way house would be the current CODEC i am working on doesn't currently need any BRA/raw transfers so I could stick to the reg_write/reg_read functions, but still allow larger than 8-bit registers. It would get me what I need for now, look a little cleaner, and we can deal with the other issues when they come up. Thanks, Charles
On 1/13/23 05:02, Charles Keepax wrote: > On Thu, Jan 12, 2023 at 02:19:29PM -0600, Pierre-Louis Bossart wrote: >> On 1/12/23 13:50, Mark Brown wrote: >>> On Thu, Jan 12, 2023 at 12:43:46PM -0600, Pierre-Louis Bossart wrote: >>>> On 1/12/23 12:14, Mark Brown wrote: >>> >>>>> The regmap gather_write() operation allows the bus to take two buffers, >>>>> one for the register and one for the value, rather than requiring the >>>>> core combine everything into a single buffer (mainly useful for large >>>>> transfers like firmware downloads). >>> >>>> Right, but that's not supported in SoundWire. sdw_nwrite() will only >>>> work with consecutive addresses - and the auto-increment is handled in >>>> software, not hardware. >>> >>> No, that's exactly what this is for. It's for the *register address* >>> being in a separate buffer, the data is then a sequence of consecutive >>> register values.> >>>> What's suggested here is to use the first element of reg_buf, which begs >>>> the question how different this is from a regular write. If there was a >>>> discontinuity in reg_buf then this wouldn't work at all. >>> >>> reg_buf contains the address of exactly one register. >> >> So what's the difference with a plain write() of N data? > > There are two back end interfaces in regmap, the reg_write/read > and the plain write/read. Both have currently have some > limitations when dealing with SoundWire. > > The reg_write/reg_read can only deal with a single register > at a time, which is really far from ideal, since it means > all transactions will be broken up into individual registers > at the regmap level, mostly depriving the SoundWire side > of the opportunity to do things like a BRA transfer if it > deems that suitable. And denying users the ability to use the > regmap_raw_read/write API at all. > > The write/read interface allows us to pass the full transaction > through, but does have the downside it copies the address around > a bit more and does some pointless endian swaps on big endian > systems. This interface is generally used by buses like I2C/SPI > where there is no actual concept of a register address only a > buffer of bytes to be sent/read, thus prefers to pass a single > working buffer if it sensibly can. I went with this solution > because it enables all the functionality and the downside is > fairly minimal, apart from looking a little clunky as you note. The change from reg_write/read_reg to write/read seems ok, what I was asking about was the gather_write. + .write = regmap_sdw_write, + .gather_write = regmap_sdw_gather_write, + .read = regmap_sdw_read, what happens if you only have .write and .read? What does the .gather_write help with if you only use only address?
On Fri, Jan 13, 2023 at 10:57:39AM -0600, Pierre-Louis Bossart wrote: > The change from reg_write/read_reg to write/read seems ok, what I was > asking about was the gather_write. > + .write = regmap_sdw_write, > + .gather_write = regmap_sdw_gather_write, > + .read = regmap_sdw_read, > what happens if you only have .write and .read? What does the > .gather_write help with if you only use only address? Like I said before it means that the core doesn't have to put the register in a linear buffer with the values, meaning it can avoid copying already formatted data around or allocating memory.
On 1/13/23 11:11, Mark Brown wrote: > On Fri, Jan 13, 2023 at 10:57:39AM -0600, Pierre-Louis Bossart wrote: > >> The change from reg_write/read_reg to write/read seems ok, what I was >> asking about was the gather_write. > >> + .write = regmap_sdw_write, >> + .gather_write = regmap_sdw_gather_write, >> + .read = regmap_sdw_read, > >> what happens if you only have .write and .read? What does the >> .gather_write help with if you only use only address? > > Like I said before it means that the core doesn't have to put the > register in a linear buffer with the values, meaning it can avoid > copying already formatted data around or allocating memory. Ah ok, I read sideways and missed the pointer arithmetic in the write implementation return sdw_nwrite_no_pm(slave, addr, val_size - sizeof(addr), val + sizeof(addr)); Thanks for the clarification.
diff --git a/drivers/base/regmap/regmap-sdw.c b/drivers/base/regmap/regmap-sdw.c index 95801fd411b26..09899ae99fc19 100644 --- a/drivers/base/regmap/regmap-sdw.c +++ b/drivers/base/regmap/regmap-sdw.c @@ -6,43 +6,52 @@ #include <linux/module.h> #include <linux/regmap.h> #include <linux/soundwire/sdw.h> +#include <linux/types.h> #include "internal.h" -static int regmap_sdw_write(void *context, unsigned int reg, unsigned int val) +static int regmap_sdw_write(void *context, const void *val_buf, size_t val_size) { struct device *dev = context; struct sdw_slave *slave = dev_to_sdw_dev(dev); + /* First word of buffer contains the destination address */ + u32 addr = le32_to_cpu(*(const __le32 *)val_buf); + const u8 *val = val_buf; - return sdw_write_no_pm(slave, reg, val); + return sdw_nwrite_no_pm(slave, addr, val_size - sizeof(addr), val + sizeof(addr)); } -static int regmap_sdw_read(void *context, unsigned int reg, unsigned int *val) +static int regmap_sdw_gather_write(void *context, + const void *reg_buf, size_t reg_size, + const void *val_buf, size_t val_size) { struct device *dev = context; struct sdw_slave *slave = dev_to_sdw_dev(dev); - int read; + u32 addr = le32_to_cpu(*(const __le32 *)reg_buf); - read = sdw_read_no_pm(slave, reg); - if (read < 0) - return read; + return sdw_nwrite_no_pm(slave, addr, val_size, val_buf); +} - *val = read; - return 0; +static int regmap_sdw_read(void *context, + const void *reg_buf, size_t reg_size, + void *val_buf, size_t val_size) +{ + struct device *dev = context; + struct sdw_slave *slave = dev_to_sdw_dev(dev); + u32 addr = le32_to_cpu(*(const __le32 *)reg_buf); + + return sdw_nread_no_pm(slave, addr, val_size, val_buf); } static const struct regmap_bus regmap_sdw = { - .reg_read = regmap_sdw_read, - .reg_write = regmap_sdw_write, + .write = regmap_sdw_write, + .gather_write = regmap_sdw_gather_write, + .read = regmap_sdw_read, .reg_format_endian_default = REGMAP_ENDIAN_LITTLE, .val_format_endian_default = REGMAP_ENDIAN_LITTLE, }; static int regmap_sdw_config_check(const struct regmap_config *config) { - /* All register are 8-bits wide as per MIPI Soundwire 1.0 Spec */ - if (config->val_bits != 8) - return -ENOTSUPP; - /* Register addresses are 32 bits wide */ if (config->reg_bits != 32) return -ENOTSUPP;