Re: [PATCH v3 2/7] ASoC: codecs: wcd937x: add wcd937x codec driver

[Mohammad Rafi Shaik <quic_mohs@xxxxxxxxxxx>]

On 4/25/2024 7:13 PM, Pierre-Louis Bossart wrote:
> > +struct wcd937x_priv {
> > +	struct sdw_slave *tx_sdw_dev;
> > +	struct sdw_slave *rx_sdw_dev;

> Does this mean that the codec has 2 SoundWire interfaces?

Yes, Similar to WCD938x 
(https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/sound/soc/codecs/wcd938x.c?h=next-20240507), 
it has two soundwire interfaces.

All read/write operations associated with WCD937x goes over tx_sdw.

rx_sdw_dev is unused in this driver. I will remove it.

> If yes, aren't there merits in splitting the implementation in two
> separate drivers, one for each interface and probing on the relevant partID?
>
> This is how the RT713 was handled. The mic function was exposed as the
> RT1713.
>
> By representing the device as a single entity, things could be fun
> because the two interfaces are really independent. things like clock
> stop are handled at the interface level.
>
> The code in this driver is difficult to review, for example in the probe
> you wait for the TX part to complete the enumeration/initialization, but
> there's nothing mentioned or stated on the RX part, and there's really
> nothing related to the detection of this device. I don't actually see a
> sdw_driver at all, it's a platform driver.
wcd937x waits for TX part to complete as it is needed for register 
read/write operations.

Please refer patch 3/7 for slave driver.

> Would you mind adding a paragraph on how the SoundWire interfaces are
> handled and how the SoundWire bus is involved if there's no sdw_driver?
>
> Thanks!

Sure, will add details in the commit message.

Soundwire master driver is already upstreamed and is available at
https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/drivers/soundwire/qcom.c?h=next-20240507

Thanks & Regards,
Rafi.