linux-kernel - Re: [PATCH 4/5] ASoC: codecs: Add WCD939x Soundwire slave driver

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <a7725504-89fd-4f62-b8d0-6ec863bd059a@linaro.org>
Date:   Sat, 25 Nov 2023 12:55:07 +0100
From:   Konrad Dybcio <konrad.dybcio@...aro.org>
To:     Neil Armstrong <neil.armstrong@...aro.org>,
        Srinivas Kandagatla <srinivas.kandagatla@...aro.org>,
        Banajit Goswami <bgoswami@...cinc.com>,
        Andy Gross <agross@...nel.org>,
        Bjorn Andersson <andersson@...nel.org>,
        Liam Girdwood <lgirdwood@...il.com>,
        Mark Brown <broonie@...nel.org>,
        Rob Herring <robh+dt@...nel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        Conor Dooley <conor+dt@...nel.org>,
        Jaroslav Kysela <perex@...ex.cz>, Takashi Iwai <tiwai@...e.com>
Cc:     linux-arm-msm@...r.kernel.org, alsa-devel@...a-project.org,
        linux-sound@...r.kernel.org, devicetree@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH 4/5] ASoC: codecs: Add WCD939x Soundwire slave driver

On 23.11.2023 15:49, Neil Armstrong wrote:
> Add Soundwire Slave driver for the WCD9390/WCD9395 Audio Codec.
> 
> The WCD9390/WCD9395 Soundwire Slaves will be used by the
> main WCD9390/WCD9395 Audio Codec driver to access registers
> and configure Soundwire RX and TX ports.
> 
> Signed-off-by: Neil Armstrong <neil.armstrong@...aro.org>
> ---
[...]


> +static struct wcd939x_sdw_ch_info wcd939x_sdw_tx_ch_info[] = {
> +	WCD_SDW_CH(WCD939X_ADC1, WCD939X_ADC_1_4_PORT, BIT(0)),
> +	WCD_SDW_CH(WCD939X_ADC2, WCD939X_ADC_1_4_PORT, BIT(1)),
> +	WCD_SDW_CH(WCD939X_ADC3, WCD939X_ADC_1_4_PORT, BIT(2)),
> +	WCD_SDW_CH(WCD939X_ADC4, WCD939X_ADC_1_4_PORT, BIT(3)),
> +	// TOFIX support ADC3/4 & DMIC0/1 on port 2
Well, fix it or drop it :D

> +	//WCD_SDW_CH(WCD939X_ADC3, WCD939X_ADC_DMIC_1_2_PORT, BIT(0)),
> +	//WCD_SDW_CH(WCD939X_ADC4, WCD939X_ADC_DMIC_1_2_PORT, BIT(1)),
> +	//WCD_SDW_CH(WCD939X_DMIC0, WCD939X_ADC_DMIC_1_2_PORT, BIT(2)),
> +	//WCD_SDW_CH(WCD939X_DMIC1, WCD939X_ADC_DMIC_1_2_PORT, BIT(3)),
> +	WCD_SDW_CH(WCD939X_DMIC0, WCD939X_DMIC_0_3_MBHC_PORT, BIT(0)),
> +	WCD_SDW_CH(WCD939X_DMIC1, WCD939X_DMIC_0_3_MBHC_PORT, BIT(1)),
> +	WCD_SDW_CH(WCD939X_MBHC, WCD939X_DMIC_0_3_MBHC_PORT, BIT(2)),
> +	WCD_SDW_CH(WCD939X_DMIC2, WCD939X_DMIC_0_3_MBHC_PORT, BIT(2)),
> +	WCD_SDW_CH(WCD939X_DMIC3, WCD939X_DMIC_0_3_MBHC_PORT, BIT(3)),
> +	WCD_SDW_CH(WCD939X_DMIC4, WCD939X_DMIC_3_7_PORT, BIT(0)),
> +	WCD_SDW_CH(WCD939X_DMIC5, WCD939X_DMIC_3_7_PORT, BIT(1)),
> +	WCD_SDW_CH(WCD939X_DMIC6, WCD939X_DMIC_3_7_PORT, BIT(2)),
> +	WCD_SDW_CH(WCD939X_DMIC7, WCD939X_DMIC_3_7_PORT, BIT(3)),
> +};
[...]

> +
> +int wcd939x_swr_get_current_bank(struct sdw_slave *sdev)
> +{
> +	int bank;
> +
> +	bank = sdw_read(sdev, SDW_SCP_CTRL);
> +
> +	return ((bank & 0x40) ? 1 : 0);
bool conversion?

Also, 0x40 == BIT(6), can you look up what it means and #define it?

[...]

> +
> +static int wcd9390_bus_config(struct sdw_slave *slave,
> +			      struct sdw_bus_params *params)
> +{
> +	sdw_write(slave, SWRS_SCP_HOST_CLK_DIV2_CTL_BANK(params->next_bank),
> +		  0x01);
similar, BIT(0)
[...]

> +	{ WCD939X_EAR_STATUS_REG_2, 0x08 },
> +	{ WCD939X_FLYBACK_NEW_CTRL_2, 0x00 }, //??
> +	{ WCD939X_FLYBACK_NEW_CTRL_3, 0x00 }, //??
> +	{ WCD939X_FLYBACK_NEW_CTRL_4, 0x44 }, //??
drop //s
[...]

> +static bool wcd939x_volatile_register(struct device *dev, unsigned int reg)
> +{
> +	if (reg <= WCD939X_BASE)
> +		return false;
Maybe move this check to readonly_register
> +
> +	if (wcd939x_readonly_register(dev, reg))
> +		return true;
and call readonly for .volatile_reg as well?
[...]

> +	/**
> +	 * Port map index starts with 0, however the data port for this codec
> +	 * are from index 1
> +	 */
This is not kerneldoc

> +	if (of_property_read_bool(dev->of_node, "qcom,tx-port-mapping")) {
> +		wcd->is_tx = true;
> +		ret = of_property_read_u32_array(dev->of_node,
> +						 "qcom,tx-port-mapping",
> +						 &pdev->m_port_map[1],
> +						 WCD939X_MAX_TX_SWR_PORTS);
> +	} else {
> +		ret = of_property_read_u32_array(dev->of_node,
> +						 "qcom,rx-port-mapping",
> +						 &pdev->m_port_map[1],
> +						 WCD939X_MAX_RX_SWR_PORTS);
> +	}
This is used in wcd9380 and will be used in wcd9370 when that happens some
day, maybe it'd be worth to commonize it as qcom_{rx/tx}_portmap_get?
[...]

> +static const struct sdw_device_id wcd9390_slave_id[] = {
> +	SDW_SLAVE_ENTRY(0x0217, 0x10e, 0),
0x10e - WCD9380 or 9385 slave? an inline comment at the end of the line
would be cool!

Konrad