Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1550919 > unrolled thread

Re: [PATCH v4 2/3] dmaeninge: xilinx_dma: Fix bug in multiple frame stores scenario in vdma

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v4 2/3] dmaeninge: xilinx_dma: Fix bug in multiple frame  stores scenario in vdma Rob Herring <robh@kernel.org> - 2017-01-04 16:30 +0100
    Re: [PATCH v4 2/3] dmaeninge: xilinx_dma: Fix bug in multiple frame  stores scenario in vdma Rob Herring <robh@kernel.org> - 2017-01-05 17:40 +0100

#1550919 — Re: [PATCH v4 2/3] dmaeninge: xilinx_dma: Fix bug in multiple frame stores scenario in vdma

On Wed, Jan 04, 2017 at 07:05:53PM +0530, Kedareswara rao Appana wrote:
> When VDMA is configured for more than one frame in the h/w
> for example h/w is configured for n number of frames and user
> Submits n number of frames and triggered the DMA using issue_pending API.
> In the current driver flow we are submitting one frame at a time
> but we should submit all the n number of frames at one time as the h/w
> Is configured for n number of frames.

Please fix run-on sentences, capitalization, and word wrapping.

> 
> This patch fixes this issue.
> 
> Reviewed-by: Jose Abreu <joabreu@synopsys.com>
> Signed-off-by: Kedareswara rao Appana <appanad@xilinx.com>
> ---
> Changes for v4:
> ---> Add Check for framestore configuration on Transmit case as well
>      as suggested by Jose Abreu.
> ---> Modified the dev_dbg checks to dev_warn checks as suggested
>      by Jose Abreu. 
> Changes for v3:
> ---> Added Checks for frame store configuration. If frame store
>      Configuration is not present at the h/w level and user
>      Submits less frames added debug prints in the driver as relevant.
> Changes for v2:
> ---> Fixed race conditions in the driver as suggested by Jose Abreu
> ---> Fixed unnecessray if else checks in the vdma_start_transfer
>      as suggested by Laurent Pinchart.
> 
>  .../devicetree/bindings/dma/xilinx/xilinx_dma.txt  |  2 +
>  drivers/dma/xilinx/xilinx_dma.c                    | 79 +++++++++++++++-------
>  2 files changed, 58 insertions(+), 23 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/dma/xilinx/xilinx_dma.txt b/Documentation/devicetree/bindings/dma/xilinx/xilinx_dma.txt
> index a2b8bfa..1f65e09 100644
> --- a/Documentation/devicetree/bindings/dma/xilinx/xilinx_dma.txt
> +++ b/Documentation/devicetree/bindings/dma/xilinx/xilinx_dma.txt
> @@ -66,6 +66,8 @@ Optional child node properties:
>  Optional child node properties for VDMA:
>  - xlnx,genlock-mode: Tells Genlock synchronization is
>  	enabled/disabled in hardware.
> +- xlnx,fstore-config: Tells Whether Frame Store Configuration is
> +	enabled/disabled in hardware.

What's the default (when not present)? That should be the most 
common case. Looks like the code treats this as bool, but that's 
not clear here. The name is not clear what it is doing. Enabling or 
disabling the feature?


>  Optional child node properties for AXI DMA:
>  -dma-channels: Number of dma channels in child node.
>  

[toc] | [next] | [standalone]

#1552103

On Wed, Jan 4, 2017 at 11:00 AM, Appana Durga Kedareswara Rao
<appana.durga.rao@xilinx.com> wrote:
> Hi Rob,
>
>         Thanks for the review....
>
>> On Wed, Jan 04, 2017 at 07:05:53PM +0530, Kedareswara rao Appana wrote:
>> > When VDMA is configured for more than one frame in the h/w for example
>> > h/w is configured for n number of frames and user Submits n number of
>> > frames and triggered the DMA using issue_pending API.
>> > In the current driver flow we are submitting one frame at a time but
>> > we should submit all the n number of frames at one time as the h/w Is
>> > configured for n number of frames.
>>
>> Please fix run-on sentences, capitalization, and word wrapping.
>
> Sure will fix in the next version....
>
> [Snip]
> -- a/Documentation/devicetree/bindings/dma/xilinx/xilinx_dma.txt
>> > +++ b/Documentation/devicetree/bindings/dma/xilinx/xilinx_dma.txt
>> > @@ -66,6 +66,8 @@ Optional child node properties:
>> >  Optional child node properties for VDMA:
>> >  - xlnx,genlock-mode: Tells Genlock synchronization is
>> >     enabled/disabled in hardware.
>> > +- xlnx,fstore-config: Tells Whether Frame Store Configuration is
>> > +   enabled/disabled in hardware.
>>
>> What's the default (when not present)? That should be the most common case.
>> Looks like the code treats this as bool, but that's not clear here. The name is not
>> clear what it is doing. Enabling or disabling the feature?
>
> Default value is zero...
> When this property is present it tells hardware is configured for frame store configuration.

So most people will not want "frame store configuration"?

> Will fix the explanation part in the next version like below.
>  xlnx,fstore-config: Tells hardware is configured for frame store configuration.
> Is the above explanation clear???

No, I mean make it obvious from the name of the property:
xlnx,fstore-config-enable or xlnx,fstore-enable

And the description needs to say it is boolean.

Rob

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web