[PATCH 0/2] Minor SoundWire Regmap Tweaks

[PATCH 0/2] Minor SoundWire Regmap Tweaks

[Charles Keepax]

One minor correction to a comment and updating the allowed register
sizes for the SoundWire register map, to allow support of additional
hardware.

NOTE: The second 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.

Thanks,
Charles

Charles Keepax (1):
  regmap: sdw: Update misleading comment

Lucas Tanure (1):
  regmap: sdw: Remove 8-bit value size restriction

 drivers/base/regmap/regmap-sdw.c | 41 +++++++++++++++++++-------------
 1 file changed, 25 insertions(+), 16 deletions(-)

-- 
2.30.2

[PATCH 1/2] regmap: sdw: Update misleading comment

[Charles Keepax]

In the regmap config reg_bits represents the number of address bits not
the number of value bits. Correct the misleading comment which looks a
lot like it suggests the register value itself is 32-bits wide.

Signed-off-by: Charles Keepax <ckeepax@opensource.cirrus.com>
---
 drivers/base/regmap/regmap-sdw.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/base/regmap/regmap-sdw.c b/drivers/base/regmap/regmap-sdw.c
index 81b0327f719d8..95801fd411b26 100644
--- a/drivers/base/regmap/regmap-sdw.c
+++ b/drivers/base/regmap/regmap-sdw.c
@@ -43,7 +43,7 @@ static int regmap_sdw_config_check(const struct regmap_config *config)
 	if (config->val_bits != 8)
 		return -ENOTSUPP;
 
-	/* Registers are 32 bits wide */
+	/* Register addresses are 32 bits wide */
 	if (config->reg_bits != 32)
 		return -ENOTSUPP;
 
-- 
2.30.2

[PATCH 2/2] regmap: sdw: Remove 8-bit value size restriction

[Charles Keepax]

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(-)

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;
-- 
2.30.2

Re: [PATCH 2/2] regmap: sdw: Remove 8-bit value size restriction

[Pierre-Louis Bossart]

> -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;

Re: [PATCH 2/2] regmap: sdw: Remove 8-bit value size restriction

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 889 bytes --]

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).

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 2/2] regmap: sdw: Remove 8-bit value size restriction

[Pierre-Louis Bossart]

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.

Re: [PATCH 2/2] regmap: sdw: Remove 8-bit value size restriction

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 967 bytes --]

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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 2/2] regmap: sdw: Remove 8-bit value size restriction

[Pierre-Louis Bossart]

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?

Re: [PATCH 2/2] regmap: sdw: Remove 8-bit value size restriction

[Charles Keepax]

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

Re: [PATCH 2/2] regmap: sdw: Remove 8-bit value size restriction

[Pierre-Louis Bossart]

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?

Re: [PATCH 2/2] regmap: sdw: Remove 8-bit value size restriction

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 600 bytes --]

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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 2/2] regmap: sdw: Remove 8-bit value size restriction

[Pierre-Louis Bossart]

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.

Re: [PATCH 0/2] Minor SoundWire Regmap Tweaks

[Mark Brown]

On Thu, 12 Jan 2023 17:18:38 +0000, Charles Keepax wrote:
> One minor correction to a comment and updating the allowed register
> sizes for the SoundWire register map, to allow support of additional
> hardware.
> 
> NOTE: The second 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.
> 
> [...]

Applied to

   broonie/regmap.git for-next

Thanks!

[1/2] regmap: sdw: Update misleading comment
      commit: 6466b376e927d51ea3eadc1965714305d8c3c066
[2/2] regmap: sdw: Remove 8-bit value size restriction
      commit: 522272047dc631610dd180be0c3670043bcdf42e

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark