Re: [PATCH 4/7] soundwire: qcom: deprecate qcom,din/out-ports

[Krzysztof Kozlowski <krzk@kernel.org>]

On 02/09/2025 15:13, Srinivas Kandagatla wrote:
> On 9/2/25 9:38 AM, Krzysztof Kozlowski wrote:
>> On Mon, Sep 01, 2025 at 08:50:34PM +0100, Srinivas Kandagatla wrote:
>>> Number of input and output ports can be dynamically read from the
>>> controller registers, getting this value from Device Tree is redundant
>>> and potentially lead to bugs.
>>>
>>> Remove the code parsing this property along with marking this as
>>> deprecated in device tree bindings.
>>>
>>> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@oss.qualcomm.com>
>>> ---
>>>  drivers/soundwire/qcom.c | 134 ++++++++++++++-------------------------
>>>  1 file changed, 49 insertions(+), 85 deletions(-)
>>>
>>> diff --git a/drivers/soundwire/qcom.c b/drivers/soundwire/qcom.c
>>> index 7f19ebba6137..4fa3e1c080ef 100644
>>> --- a/drivers/soundwire/qcom.c
>>> +++ b/drivers/soundwire/qcom.c
>>> @@ -128,7 +128,6 @@
>>>  #define MAX_FREQ_NUM						1
>>>  #define TIMEOUT_MS						100
>>>  #define QCOM_SWRM_MAX_RD_LEN					0x1
>>> -#define QCOM_SDW_MAX_PORTS					14
>>>  #define DEFAULT_CLK_FREQ					9600000
>>>  #define SWRM_MAX_DAIS						0xF
>>>  #define SWR_INVALID_PARAM					0xFF
>>> @@ -195,6 +194,7 @@ struct qcom_swrm_ctrl {
>>>  	int wake_irq;
>>>  	int num_din_ports;
>>>  	int num_dout_ports;
>>> +	int nports;
>>>  	int cols_index;
>>>  	int rows_index;
>>>  	unsigned long port_mask;
>>> @@ -202,7 +202,7 @@ struct qcom_swrm_ctrl {
>>>  	u8 rcmd_id;
>>>  	u8 wcmd_id;
>>>  	/* Port numbers are 1 - 14 */
>>> -	struct qcom_swrm_port_config pconfig[QCOM_SDW_MAX_PORTS + 1];
>>> +	struct qcom_swrm_port_config *pconfig;
>>>  	struct sdw_stream_runtime *sruntime[SWRM_MAX_DAIS];
>>>  	enum sdw_slave_status status[SDW_MAX_DEVICES + 1];
>>>  	int (*reg_read)(struct qcom_swrm_ctrl *ctrl, int reg, u32 *val);
>>> @@ -1157,7 +1157,7 @@ static int qcom_swrm_stream_alloc_ports(struct qcom_swrm_ctrl *ctrl,
>>>  				       struct snd_pcm_hw_params *params,
>>>  				       int direction)
>>>  {
>>> -	struct sdw_port_config pconfig[QCOM_SDW_MAX_PORTS];
>>> +	struct sdw_port_config *pconfig __free(kfree) = NULL;
>>
>> That's incorrect usage of __free(), missing constructor.
>>
>>>  	struct sdw_stream_config sconfig;
>>>  	struct sdw_master_runtime *m_rt;
>>>  	struct sdw_slave_runtime *s_rt;
>>> @@ -1167,6 +1167,10 @@ static int qcom_swrm_stream_alloc_ports(struct qcom_swrm_ctrl *ctrl,
>>>  	int maxport, pn, nports = 0, ret = 0;
>>>  	unsigned int m_port;
>>>  
>>> +	pconfig = kcalloc(ctrl->nports, sizeof(*pconfig), GFP_KERNEL);
>>
>> This almost always goes to definition and I do not see anything
>> preventing it.
> Yes, It can go to the definition.
> 
> But does this matter for a single instance of __free? I thought this was

EVERY __free() must have constructor in definition and only exceptions
are allowed if you cannot find such constructor (e.g. there are two for
the same variable).

> an issue if we have multiple __free() or guard() and __free() the order
> of definitions matter.

Yeah, for that reason this would matter, but here it is about preferred
style.

> 
> Will follow the recommendation in v2.
> 
> thanks,
> --srini
>>
>>> +	if (!pconfig)
>>> +		return -ENOMEM;
>>
>> Best regards,
>> Krzysztof
>>
> 


Best regards,
Krzysztof