Re: [PATCH v4 08/14] dmaengine: tegra-apb: Fix coding style problems

[Dmitry Osipenko]

28.01.2020 17:05, Jon Hunter ÐÐÑÐÑ:
> 
> On 16/01/2020 17:37, Dmitry Osipenko wrote:
>> 15.01.2020 12:49, Jon Hunter ÐÐÑÐÑ:
>>>
>>>
>>> On 12/01/2020 17:30, Dmitry Osipenko wrote:
>>>> This patch fixes few dozens of coding style problems reported by
>>>> checkpatch and prettifies code where makes sense.
>>>>
>>>> Signed-off-by: Dmitry Osipenko <digetx@xxxxxxxxx>
>>>> ---
>>>>  drivers/dma/tegra20-apb-dma.c | 276 ++++++++++++++++++----------------
>>>>  1 file changed, 144 insertions(+), 132 deletions(-)
>>>>
>>>> diff --git a/drivers/dma/tegra20-apb-dma.c b/drivers/dma/tegra20-apb-dma.c
>>>> index dff21e80ffa4..7158bd3145c4 100644
>>>> --- a/drivers/dma/tegra20-apb-dma.c
>>>> +++ b/drivers/dma/tegra20-apb-dma.c
>>>
>>> ...
>>>
>>>> @@ -1003,20 +1014,23 @@ static void tegra_dma_prep_wcount(struct tegra_dma_channel *tdc,
>>>>  		ch_regs->csr |= len_field;
>>>>  }
>>>>  
>>>> -static struct dma_async_tx_descriptor *tegra_dma_prep_slave_sg(
>>>> -	struct dma_chan *dc, struct scatterlist *sgl, unsigned int sg_len,
>>>> -	enum dma_transfer_direction direction, unsigned long flags,
>>>> -	void *context)
>>>> +static struct dma_async_tx_descriptor *
>>>> +tegra_dma_prep_slave_sg(struct dma_chan *dc,
>>>> +			struct scatterlist *sgl,
>>>> +			unsigned int sg_len,
>>>> +			enum dma_transfer_direction direction,
>>>> +			unsigned long flags,
>>>> +			void *context)
>>>>  {
>>>>  	struct tegra_dma_channel *tdc = to_tegra_dma_chan(dc);
>>>> +	struct tegra_dma_sg_req *sg_req = NULL;
>>>> +	u32 csr, ahb_seq, apb_ptr, apb_seq;
>>>> +	enum dma_slave_buswidth slave_bw;
>>>>  	struct tegra_dma_desc *dma_desc;
>>>> -	unsigned int i;
>>>> -	struct scatterlist *sg;
>>>> -	unsigned long csr, ahb_seq, apb_ptr, apb_seq;
>>>>  	struct list_head req_list;
>>>> -	struct tegra_dma_sg_req  *sg_req = NULL;
>>>> -	u32 burst_size;
>>>> -	enum dma_slave_buswidth slave_bw;
>>>> +	struct scatterlist *sg;
>>>> +	unsigned int burst_size;
>>>> +	unsigned int i;
>>>
>>> This is not really consistent with the rest of the changes by having 'i'
>>> and 'burst_size' on separate lines.
>>
>> The goal wasn't to squash everything into a single line, but to make
>> code more readable. In this particular case the separated lines look
>> better to me.
>>
>>>>  
>>>>  	if (!tdc->config_init) {
>>>>  		dev_err(tdc2dev(tdc), "DMA channel is not configured\n");
>>>> @@ -1028,7 +1042,7 @@ static struct dma_async_tx_descriptor *tegra_dma_prep_slave_sg(
>>>>  	}
>>>>  
>>>>  	if (get_transfer_param(tdc, direction, &apb_ptr, &apb_seq, &csr,
>>>> -				&burst_size, &slave_bw) < 0)
>>>> +			       &burst_size, &slave_bw) < 0)
>>>>  		return NULL;
>>>>  
>>>>  	INIT_LIST_HEAD(&req_list);
>>>> @@ -1074,7 +1088,7 @@ static struct dma_async_tx_descriptor *tegra_dma_prep_slave_sg(
>>>>  		len = sg_dma_len(sg);
>>>>  
>>>>  		if ((len & 3) || (mem & 3) ||
>>>> -				(len > tdc->tdma->chip_data->max_dma_count)) {
>>>> +		    len > tdc->tdma->chip_data->max_dma_count) {
>>>>  			dev_err(tdc2dev(tdc),
>>>>  				"DMA length/memory address is not supported\n");
>>>>  			tegra_dma_desc_put(tdc, dma_desc);
>>>> @@ -1126,20 +1140,21 @@ static struct dma_async_tx_descriptor *tegra_dma_prep_slave_sg(
>>>>  	return &dma_desc->txd;
>>>>  }
>>>>  
>>>> -static struct dma_async_tx_descriptor *tegra_dma_prep_dma_cyclic(
>>>> -	struct dma_chan *dc, dma_addr_t buf_addr, size_t buf_len,
>>>> -	size_t period_len, enum dma_transfer_direction direction,
>>>> -	unsigned long flags)
>>>> +static struct dma_async_tx_descriptor *
>>>> +tegra_dma_prep_dma_cyclic(struct dma_chan *dc, dma_addr_t buf_addr,
>>>> +			  size_t buf_len,
>>>> +			  size_t period_len,
>>>> +			  enum dma_transfer_direction direction,
>>>> +			  unsigned long flags)
>>>>  {
>>>>  	struct tegra_dma_channel *tdc = to_tegra_dma_chan(dc);
>>>> -	struct tegra_dma_desc *dma_desc = NULL;
>>>>  	struct tegra_dma_sg_req *sg_req = NULL;
>>>> -	unsigned long csr, ahb_seq, apb_ptr, apb_seq;
>>>> -	int len;
>>>> -	size_t remain_len;
>>>> -	dma_addr_t mem = buf_addr;
>>>> -	u32 burst_size;
>>>> +	u32 csr, ahb_seq, apb_ptr, apb_seq;
>>>>  	enum dma_slave_buswidth slave_bw;
>>>> +	struct tegra_dma_desc *dma_desc;
>>>> +	dma_addr_t mem = buf_addr;
>>>> +	unsigned int burst_size;
>>>> +	size_t len, remain_len;
>>>>  
>>>>  	if (!buf_len || !period_len) {
>>>>  		dev_err(tdc2dev(tdc), "Invalid buffer/period len\n");
>>>> @@ -1173,13 +1188,13 @@ static struct dma_async_tx_descriptor *tegra_dma_prep_dma_cyclic(
>>>>  
>>>>  	len = period_len;
>>>>  	if ((len & 3) || (buf_addr & 3) ||
>>>> -			(len > tdc->tdma->chip_data->max_dma_count)) {
>>>> +	    len > tdc->tdma->chip_data->max_dma_count) {
>>>>  		dev_err(tdc2dev(tdc), "Req len/mem address is not correct\n");
>>>>  		return NULL;
>>>>  	}
>>>>  
>>>>  	if (get_transfer_param(tdc, direction, &apb_ptr, &apb_seq, &csr,
>>>> -				&burst_size, &slave_bw) < 0)
>>>> +			       &burst_size, &slave_bw) < 0)
>>>>  		return NULL;
>>>>  
>>>>  	ahb_seq = TEGRA_APBDMA_AHBSEQ_INTR_ENB;
>>>> @@ -1269,7 +1284,6 @@ static int tegra_dma_alloc_chan_resources(struct dma_chan *dc)
>>>>  	int ret;
>>>>  
>>>>  	dma_cookie_init(&tdc->dma_chan);
>>>> -	tdc->config_init = false;
>>>
>>> Why is this removed? Does not seem to belong in this patch.
>>
>> Because initially, on driver's probe, the tdc->config_init is false for
>> all channels and then tegra_dma_free_chan_resources() also sets it to
>> false. Thus there is no need to re-initilize the already initialized
>> variable. It's not a very good coding style if variables are
>> unnecessarily initialized, you probably noticed that there are few other
>> cases of removing the unneeded initializations of local variables in
>> this patch.
> 
> OK, but I don't really consider this coding-style and would prefer a
> separate patch for this.

If if it's not a coding-style, then how would you name it? :)