From: Dmitry Osipenko <digetx@gmail.com>
To: "Jon Hunter" <jonathanh@nvidia.com>,
"Laxman Dewangan" <ldewangan@nvidia.com>,
"Vinod Koul" <vkoul@kernel.org>,
"Dan Williams" <dan.j.williams@intel.com>,
"Thierry Reding" <thierry.reding@gmail.com>,
"Michał Mirosław" <mirq-linux@rere.qmqm.pl>
Cc: dmaengine@vger.kernel.org, linux-tegra@vger.kernel.org,
linux-kernel@vger.kernel.org
Subject: Re: [PATCH v4 08/14] dmaengine: tegra-apb: Fix coding style problems
Date: Tue, 28 Jan 2020 18:01:38 +0300 [thread overview]
Message-ID: <a99ff711-22af-a377-9500-ab4b8644b0ff@gmail.com> (raw)
In-Reply-To: <bab1eec0-8b1d-6005-e9eb-05e93da844aa@nvidia.com>
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@gmail.com>
>>>> ---
>>>> 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? :)
next prev parent reply other threads:[~2020-01-28 15:01 UTC|newest]
Thread overview: 46+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-01-12 17:29 [PATCH v4 00/14] NVIDIA Tegra APB DMA driver fixes and improvements Dmitry Osipenko
2020-01-12 17:29 ` [PATCH v4 01/14] dmaengine: tegra-apb: Fix use-after-free Dmitry Osipenko
2020-01-14 15:09 ` Jon Hunter
2020-01-14 20:33 ` Dmitry Osipenko
2020-01-15 9:00 ` Jon Hunter
2020-01-16 20:10 ` Dmitry Osipenko
2020-01-28 14:02 ` Jon Hunter
2020-01-28 14:51 ` Dmitry Osipenko
2020-01-29 0:12 ` Dmitry Osipenko
2020-01-29 10:42 ` Jon Hunter
2020-01-12 17:29 ` [PATCH v4 02/14] dmaengine: tegra-apb: Implement synchronization callback Dmitry Osipenko
2020-01-14 15:15 ` Jon Hunter
2020-01-14 21:02 ` Dmitry Osipenko
2020-01-15 9:18 ` Jon Hunter
2020-01-15 10:25 ` Jon Hunter
2020-01-12 17:29 ` [PATCH v4 03/14] dmaengine: tegra-apb: Prevent race conditions on channel's freeing Dmitry Osipenko
2020-01-14 15:16 ` Jon Hunter
2020-01-12 17:29 ` [PATCH v4 04/14] dmaengine: tegra-apb: Clean up tasklet releasing Dmitry Osipenko
2020-01-14 15:36 ` Jon Hunter
2020-01-12 17:29 ` [PATCH v4 05/14] dmaengine: tegra-apb: Prevent race conditions of tasklet vs free list Dmitry Osipenko
2020-01-14 15:43 ` Jon Hunter
2020-01-12 17:29 ` [PATCH v4 06/14] dmaengine: tegra-apb: Use devm_platform_ioremap_resource Dmitry Osipenko
2020-01-14 15:44 ` Jon Hunter
2020-01-12 17:29 ` [PATCH v4 07/14] dmaengine: tegra-apb: Use devm_request_irq Dmitry Osipenko
2020-01-14 15:44 ` Jon Hunter
2020-01-12 17:30 ` [PATCH v4 08/14] dmaengine: tegra-apb: Fix coding style problems Dmitry Osipenko
2020-01-15 9:49 ` Jon Hunter
2020-01-16 17:37 ` Dmitry Osipenko
2020-01-28 14:05 ` Jon Hunter
2020-01-28 15:01 ` Dmitry Osipenko [this message]
2020-01-12 17:30 ` [PATCH v4 09/14] dmaengine: tegra-apb: Clean up runtime PM teardown Dmitry Osipenko
2020-01-15 9:57 ` Jon Hunter
2020-01-16 17:18 ` Dmitry Osipenko
2020-01-12 17:30 ` [PATCH v4 10/14] dmaengine: tegra-apb: Keep clock enabled only during of DMA transfer Dmitry Osipenko
2020-01-15 10:08 ` Jon Hunter
2020-01-16 17:01 ` Dmitry Osipenko
2020-01-12 17:30 ` [PATCH v4 11/14] dmaengine: tegra-apb: Clean up suspend-resume Dmitry Osipenko
2020-01-21 21:23 ` Dmitry Osipenko
2020-01-28 14:10 ` Jon Hunter
2020-01-28 14:53 ` Dmitry Osipenko
2020-01-12 17:30 ` [PATCH v4 12/14] dmaengine: tegra-apb: Add missing of_dma_controller_free Dmitry Osipenko
2020-01-15 10:10 ` Jon Hunter
2020-01-12 17:30 ` [PATCH v4 13/14] dmaengine: tegra-apb: Allow to compile as a loadable kernel module Dmitry Osipenko
2020-01-15 10:10 ` Jon Hunter
2020-01-12 17:30 ` [PATCH v4 14/14] dmaengine: tegra-apb: Remove MODULE_ALIAS Dmitry Osipenko
2020-01-15 10:11 ` Jon Hunter
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=a99ff711-22af-a377-9500-ab4b8644b0ff@gmail.com \
--to=digetx@gmail.com \
--cc=dan.j.williams@intel.com \
--cc=dmaengine@vger.kernel.org \
--cc=jonathanh@nvidia.com \
--cc=ldewangan@nvidia.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-tegra@vger.kernel.org \
--cc=mirq-linux@rere.qmqm.pl \
--cc=thierry.reding@gmail.com \
--cc=vkoul@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).