Re: [PATCH v2 3/4] dmaengine: ti: k3-udma-glue: Add function to request TX channel by ID

[Siddharth Vadapalli]

Hello Péter,

On 14/12/23 21:11, Péter Ujfalusi wrote:
> 
> 
> On 12/12/2023 13:10, Siddharth Vadapalli wrote:
>> The existing function k3_udma_glue_request_tx_chn() supports requesting
>> a TX DMA channel by its name. Add support to request TX channel by ID in
>> the form of a new function k3_udma_glue_request_tx_chn_by_id() and export
>> it for use by drivers which are probed by alternate methods (non
>> device-tree) but still wish to make use of the existing DMA APIs. Such
>> drivers could be informed about the TX channel to use by RPMsg for example.
>>
>> Since the implementation of k3_udma_glue_request_tx_chn_by_id() reuses
>> most of the code in k3_udma_glue_request_tx_chn(), create a new function
>> for the common code named as k3_udma_glue_request_tx_chn_common().
>>
>> Signed-off-by: Siddharth Vadapalli <s-vadapalli@xxxxxx>
>> ---
>> Changes since v1:
>> - Updated commit message indicating the use-case for which the patch is
>>   being added.
>>
>>  drivers/dma/ti/k3-udma-glue.c    | 101 +++++++++++++++++++++++--------
>>  include/linux/dma/k3-udma-glue.h |   4 ++
>>  2 files changed, 79 insertions(+), 26 deletions(-)
>>
>> diff --git a/drivers/dma/ti/k3-udma-glue.c b/drivers/dma/ti/k3-udma-glue.c
>> index eff1ae3d3efe..ea5119a5e8eb 100644
>> --- a/drivers/dma/ti/k3-udma-glue.c
>> +++ b/drivers/dma/ti/k3-udma-glue.c
>> @@ -274,29 +274,13 @@ static int k3_udma_glue_cfg_tx_chn(struct k3_udma_glue_tx_channel *tx_chn)
>>  	return tisci_rm->tisci_udmap_ops->tx_ch_cfg(tisci_rm->tisci, &req);
>>  }
>>  

...

>>  
>>  err:
>> @@ -395,6 +410,40 @@ struct k3_udma_glue_tx_channel *k3_udma_glue_request_tx_chn(struct device *dev,
>>  }
>>  EXPORT_SYMBOL_GPL(k3_udma_glue_request_tx_chn);
>>  
>> +struct k3_udma_glue_tx_channel *
>> +k3_udma_glue_request_tx_chn_by_id(struct device *dev, struct k3_udma_glue_tx_channel_cfg *cfg,
>> +				  struct device_node *udmax_np, u32 thread_id)
> 
> udmax_np is not dev->of_node ?

I am not sure I fully understand the question. If you meant to ask if the driver
which uses this API will not have its device's of_node set to udmax_np, then yes
that's correct.

The driver shall be probed over RPMsg-bus, due to which its device's of_node
will not be udmax_np. Additionally, the udmax_np is the device-tree node of one
of the DMA Controllers described in the device-tree. The driver shall obtain the
reference to the udmax_np node using the API:
of_find_compatible_node()
with the compatible to be passed to the above API being a part of the driver's
data. Thus, it is possible to specify which DMA Controller to use by specifying
the compatible in the driver's data. I hope that I have answered your question.
Please let me know otherwise.

> 
>> +{
>> +	struct k3_udma_glue_tx_channel *tx_chn;
>> +	int ret;
>> +
>> +	tx_chn = devm_kzalloc(dev, sizeof(*tx_chn), GFP_KERNEL);
>> +	if (!tx_chn)
>> +		return ERR_PTR(-ENOMEM);
>> +
>> +	tx_chn->common.dev = dev;
>> +	tx_chn->common.swdata_size = cfg->swdata_size;
>> +	tx_chn->tx_pause_on_err = cfg->tx_pause_on_err;
>> +	tx_chn->tx_filt_einfo = cfg->tx_filt_einfo;
>> +	tx_chn->tx_filt_pswords = cfg->tx_filt_pswords;
>> +	tx_chn->tx_supr_tdpkt = cfg->tx_supr_tdpkt;
>> +
>> +	ret = of_k3_udma_glue_parse_chn_by_id(udmax_np, &tx_chn->common, true, thread_id);
>> +	if (ret)
>> +		goto err;
>> +
>> +	ret = k3_udma_glue_request_tx_chn_common(dev, tx_chn, cfg);
>> +	if (ret)
>> +		goto err;
>> +
>> +	return tx_chn;
>> +
>> +err:
>> +	k3_udma_glue_release_tx_chn(tx_chn);
>> +	return ERR_PTR(ret);
>> +}
>> +EXPORT_SYMBOL_GPL(k3_udma_glue_request_tx_chn_by_id);
>> +
>>  void k3_udma_glue_release_tx_chn(struct k3_udma_glue_tx_channel *tx_chn)
>>  {
>>  	if (tx_chn->psil_paired) {
>> diff --git a/include/linux/dma/k3-udma-glue.h b/include/linux/dma/k3-udma-glue.h
>> index e443be4d3b4b..6205d84430ca 100644
>> --- a/include/linux/dma/k3-udma-glue.h
>> +++ b/include/linux/dma/k3-udma-glue.h
>> @@ -26,6 +26,10 @@ struct k3_udma_glue_tx_channel;
>>  struct k3_udma_glue_tx_channel *k3_udma_glue_request_tx_chn(struct device *dev,
>>  		const char *name, struct k3_udma_glue_tx_channel_cfg *cfg);
>>  
>> +struct k3_udma_glue_tx_channel *
>> +k3_udma_glue_request_tx_chn_by_id(struct device *dev, struct k3_udma_glue_tx_channel_cfg *cfg,
> 
> I know it is going to be longer, but can the function be named more
> precisely?
> k3_udma_glue_request_tx_chn_by_thread_id
> 
> For TX/RX: a channel is always for one thread, right?
> Probably:
> k3_udma_glue_request_tx_chn_for_thread_id()
> 
> ?

Sure, I will rename it to:
k3_udma_glue_request_tx_chn_for_thread_id()
as suggested by you.

> 
> Other then this the series looks OK.

Thank you for reviewing the series. I will rename the API as mentioned above and
if the question you had above regarding the of_node has been addressed, I will
post the v3 series. Kindly let me know.

-- 
Regards,
Siddharth.