RE: [PATCH 1/2] ASoC: max98363: add soundwire amplifier driver

[Lee, RyanS]

> -----Original Message-----
> From: Pierre-Louis Bossart <pierre-louis.bossart@xxxxxxxxxxxxxxx>
> Sent: Monday, February 27, 2023 3:38 PM
> To: Lee, RyanS <RyanS.Lee@xxxxxxxxxx>; Mark Brown
> <broonie@xxxxxxxxxx>
> Cc: “Ryan <ryan.lee.analog@xxxxxxxxx>; lgirdwood@xxxxxxxxx;
> tiwai@xxxxxxxx; krzysztof.kozlowski@xxxxxxxxxx; rf@xxxxxxxxxxxxxxxxxxxxx;
> ckeepax@xxxxxxxxxxxxxxxxxxxxx; herve.codina@xxxxxxxxxxx;
> wangweidong.a@xxxxxxxxxx; james.schulman@xxxxxxxxxx;
> ajye_huang@xxxxxxxxxxxxxxxxxxxxxxxxxxxxxx; shumingf@xxxxxxxxxxx;
> povik+lin@xxxxxxxxxxx; flatmax@xxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx;
> alsa-devel@xxxxxxxxxxxxxxxx; robh+dt@xxxxxxxxxx;
> devicetree@xxxxxxxxxxxxxxx
> Subject: Re: [PATCH 1/2] ASoC: max98363: add soundwire amplifier driver
> 
> [External]
> 
> 
> >>> Put differently, SoundWire codec drivers should only deal with
> >>> non-standard vendor-specific registers.
> >>
> >> OK, it'd be good to be clear about what the issue is when reviewing things.
> >> The registers *are* in the device's register map but the driver
> >> shouldn't be referencing them at all and should instead be going via
> >> the SoundWire core for anything in there.
> >
> > Thanks for the comment.
> > The only reason I added standard SoundWire registers to the amp driver
> > is to check the values for the debugging purpose because these
> > registers values are important to understand the device status, but it
> > is not visible from the regmap debugfs if those registers are not included
> on the regmap table of the driver.
> > The driver never controls the standard SoundWire registers by itself.
> > Do you recommend removing the standard SoundWire registers from the
> > driver or keeping it non-volatile?
> > (The reg_default values in the table are all amp reset values and
> > those registers are treated as volatile. I shall clear 'unique ID'
> > field because it is determined by the hardware pin connection.)
> 
> We already have debugfs support for those registers, see
> sdw_slave_reg_show() in drivers/soundwire/debugfs.c
> 
> It's not the same file as regmap debugfs but the information is already there,
> see e.g. an example on the SOF CI devices:
> 
> cd /sys/kernel/debug/soundwire/master-0-1/sdw:1:025d:0700:00
> more registers
> 
> Register  Value
> 
> DP0
>   0	 0
>   1	 0
>   2	 0
>   3	 0
>   4	 0
>   5	 1
> Bank0
>  20	 0
>  22	 0
>  23	 0
>  24	 0
>  25	 0
>  26	 0
>  27	XX
>  28	XX
> Bank1
>  30	 0
>  32	 0
>  33	 0
>  34	 0
>  35	 0
>  36	 0
>  37	XX
>  38	XX
> 
> SCP
>  40	 0
>  41	 7
>  42	 0
>  43	 0
>  44	20
>  45	 9
>  46	 4
>  47	XX
>  48	XX
>  49	XX
>  4a	XX
>  4b	XX
>  50	10
>  51	 2
>  52	5d
>  53	 7
>  54	 0
>  55	 0
> 
> DP1
> 100	 0
> 101	 0
> 102	 0

Thank you for the useful information. Then, there is no reason to keep
standard registers in the driver. I shall remove stand registers from
the amp register map.