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

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

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