Re: [PATCH 6/7] soundwire: debugfs: add interface to read/write commands

[Vinod Koul <vkoul@xxxxxxxxxx>]

On 05-04-24, 10:12, Pierre-Louis Bossart wrote:
> On 4/5/24 06:45, Vinod Koul wrote:
> > On 26-03-24, 09:01, Bard Liao wrote:
> >> From: Pierre-Louis Bossart <pierre-louis.bossart@xxxxxxxxxxxxxxx>
> >>
> >> We have an existing debugfs files to read standard registers
> >> (DP0/SCP/DPn).
> >>
> >> This patch provides a more generic interface to ANY set of read/write
> >> contiguous registers in a peripheral device. In follow-up patches,
> >> this interface will be extended to use BRA transfers.
> >>
> >> The sequence is to use the following files added under the existing
> >> debugsfs directory for each peripheral device:
> >>
> >> command (write 0, read 1)
> >> num_bytes
> >> start_address
> >> firmware_file (only for writes)
> >> read_buffer (only for reads)
> >>
> >> Example for a read command - this checks the 6 bytes used for
> >> enumeration.
> >>
> >> cd /sys/kernel/debug/soundwire/master-0-0/sdw\:0\:025d\:0711\:01/
> >> echo 1 > command
> >> echo 6 > num_bytes
> >> echo 0x50 > start_address
> >> echo 1 > go
> > 
> > can we have a simpler interface? i am not a big fan of this kind of
> > structure for debugging.
> > 
> > How about two files read_bytes and write_bytes where you read/write
> > bytes.
> > 
> > echo 0x50 6 > read_bytes
> > cat read_bytes
> > 
> > in this format I would like to see addr and values (not need to print
> > address /value words (regmap does that too)
> > 
> > For write
> > 
> > echo start_addr N byte0 byte 1 ... byte N > write_bytes
> 
> I think you missed the required extension where we will add a new
> 'command_type' to specify which of the regular or BTP/BRA accesses is used.
> 
> Also the bytes can come from a firmware file, it would be very odd to
> have a command line with 32k value, wouldn't it?

ofc no one should expect that... it should be written directly from the firmware file

> I share your concern about making the interface as simple as possible,
> but I don't see how it can be made simpler really. We have to specify
> - read/write
> - access type (BRA or regular)
> - start address
> - number of bytes
> - a firmware file for writes
> - a means to see the read data.
> 
> And I personally prefer one 1:1 mapping between setting and debugfs
> file, rather than a M:1 mapping that require users to think about the
> syntax and which value maps to what setting. At my age I have to define
> things that I will remember on the next Monday.

Exactly, you won't remember all the files to write to, my idea was a
simple format or addr, N and data.. a TLV kind of structure..

What would happen if you issue go, but missed writing one of the files.
Also I would expect you to do error checking of inputs...

Thanks
-- 
~Vinod