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

Groups > linux.kernel > #1313447 > unrolled thread

Re: [PATCH 1/3] ata: sata_dwc_460ex: use "dmas" DT property to find dma channel

Started byMåns Rullgård <mans@mansr.com>
First post2016-01-20 20:00 +0100
Last post2016-01-22 13:20 +0100
Articles 12 — 3 participants

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 1/3] ata: sata_dwc_460ex: use "dmas" DT property to find dma channel Måns Rullgård <mans@mansr.com> - 2016-01-20 20:00 +0100
    Re: [PATCH 1/3] ata: sata_dwc_460ex: use "dmas" DT property to find  dma channel Andy Shevchenko <andriy.shevchenko@linux.intel.com> - 2016-01-20 20:30 +0100
      Re: [PATCH 1/3] ata: sata_dwc_460ex: use "dmas" DT property to find dma channel Måns Rullgård <mans@mansr.com> - 2016-01-20 20:30 +0100
        Re: [PATCH 1/3] ata: sata_dwc_460ex: use "dmas" DT property to find  dma channel Andy Shevchenko <andy.shevchenko@gmail.com> - 2016-01-20 20:40 +0100
          Re: [PATCH 1/3] ata: sata_dwc_460ex: use "dmas" DT property to find dma channel Måns Rullgård <mans@mansr.com> - 2016-01-20 20:50 +0100
            Re: [PATCH 1/3] ata: sata_dwc_460ex: use "dmas" DT property to find  dma channel Andy Shevchenko <andy.shevchenko@gmail.com> - 2016-01-20 21:00 +0100
              Re: [PATCH 1/3] ata: sata_dwc_460ex: use "dmas" DT property to find dma channel Måns Rullgård <mans@mansr.com> - 2016-01-20 21:10 +0100
                Re: [PATCH 1/3] ata: sata_dwc_460ex: use "dmas" DT property to find  dma channel Andy Shevchenko <andriy.shevchenko@linux.intel.com> - 2016-01-22 11:10 +0100
                  Re: [PATCH 1/3] ata: sata_dwc_460ex: use "dmas" DT property to find dma channel Måns Rullgård <mans@mansr.com> - 2016-01-22 12:20 +0100
                    Re: [PATCH 1/3] ata: sata_dwc_460ex: use "dmas" DT property to find  dma channel Andy Shevchenko <andriy.shevchenko@linux.intel.com> - 2016-01-22 13:00 +0100
                      Re: [PATCH 1/3] ata: sata_dwc_460ex: use "dmas" DT property to find dma channel Måns Rullgård <mans@mansr.com> - 2016-01-22 13:10 +0100
                        Re: [PATCH 1/3] ata: sata_dwc_460ex: use "dmas" DT property to find  dma channel Andy Shevchenko <andy.shevchenko@gmail.com> - 2016-01-22 13:20 +0100

#1313447 — Re: [PATCH 1/3] ata: sata_dwc_460ex: use "dmas" DT property to find dma channel

FromMåns Rullgård <mans@mansr.com>
Date2016-01-20 20:00 +0100
SubjectRe: [PATCH 1/3] ata: sata_dwc_460ex: use "dmas" DT property to find dma channel
Message-ID<qT694-27H-15@gated-at.bofh.it>
Andy Shevchenko <andriy.shevchenko@linux.intel.com> writes:

>> Tested on AVR32 (DMA driver) and Sigma SMP8642 (SATA driver).
>> 
>> Code here:
>> https://bitbucket.org/mansr/linux-dwc/branch/dwc-sata
>
> Thanks for testing!

Have you had time to look any more at these patches?

-- 
Måns Rullgård

[toc] | [next] | [standalone]

#1313457 — Re: [PATCH 1/3] ata: sata_dwc_460ex: use "dmas" DT property to find dma channel

FromAndy Shevchenko <andriy.shevchenko@linux.intel.com>
Date2016-01-20 20:30 +0100
SubjectRe: [PATCH 1/3] ata: sata_dwc_460ex: use "dmas" DT property to find dma channel
Message-ID<qT6C6-2zD-13@gated-at.bofh.it>
In reply to#1313447
On Wed, 2016-01-20 at 18:50 +0000, Måns Rullgård wrote:
> Andy Shevchenko <andriy.shevchenko@linux.intel.com> writes:
> 
> > > Tested on AVR32 (DMA driver) and Sigma SMP8642 (SATA driver).
> > > 
> > > Code here:
> > > https://bitbucket.org/mansr/linux-dwc/branch/dwc-sata
> > 
> > Thanks for testing!
> 
> Have you had time to look any more at these patches?

Not yet.
I will look soon for sure.

One comment still regarding to lli types. We can avoid warnings by
using (__force u32) in macros.

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

[toc] | [prev] | [next] | [standalone]

#1313459

FromMåns Rullgård <mans@mansr.com>
Date2016-01-20 20:30 +0100
Message-ID<qT6C6-2zD-25@gated-at.bofh.it>
In reply to#1313457
Andy Shevchenko <andriy.shevchenko@linux.intel.com> writes:

> On Wed, 2016-01-20 at 18:50 +0000, Måns Rullgård wrote:
>> Andy Shevchenko <andriy.shevchenko@linux.intel.com> writes:
>> 
>> > > Tested on AVR32 (DMA driver) and Sigma SMP8642 (SATA driver).
>> > > 
>> > > Code here:
>> > > https://bitbucket.org/mansr/linux-dwc/branch/dwc-sata
>> > 
>> > Thanks for testing!
>> 
>> Have you had time to look any more at these patches?
>
> Not yet.
> I will look soon for sure.
>
> One comment still regarding to lli types. We can avoid warnings by
> using (__force u32) in macros.

But that won't give the benefits of having the types checked.

-- 
Måns Rullgård

[toc] | [prev] | [next] | [standalone]

#1313467 — Re: [PATCH 1/3] ata: sata_dwc_460ex: use "dmas" DT property to find dma channel

FromAndy Shevchenko <andy.shevchenko@gmail.com>
Date2016-01-20 20:40 +0100
SubjectRe: [PATCH 1/3] ata: sata_dwc_460ex: use "dmas" DT property to find dma channel
Message-ID<qT6LN-2E2-39@gated-at.bofh.it>
In reply to#1313459
On Wed, Jan 20, 2016 at 9:24 PM, Måns Rullgård <mans@mansr.com> wrote:
> Andy Shevchenko <andriy.shevchenko@linux.intel.com> writes:
>
>> On Wed, 2016-01-20 at 18:50 +0000, Måns Rullgård wrote:
>>> Andy Shevchenko <andriy.shevchenko@linux.intel.com> writes:
>>>
>>> > > Tested on AVR32 (DMA driver) and Sigma SMP8642 (SATA driver).
>>> > >
>>> > > Code here:
>>> > > https://bitbucket.org/mansr/linux-dwc/branch/dwc-sata
>>> >
>>> > Thanks for testing!
>>>
>>> Have you had time to look any more at these patches?
>>
>> Not yet.
>> I will look soon for sure.
>>
>> One comment still regarding to lli types. We can avoid warnings by
>> using (__force u32) in macros.
>
> But that won't give the benefits of having the types checked.

You mean if we access the lli->field directly? I didn't quite get what
use case you are keeping in mind.

-- 
With Best Regards,
Andy Shevchenko

[toc] | [prev] | [next] | [standalone]

#1313474

FromMåns Rullgård <mans@mansr.com>
Date2016-01-20 20:50 +0100
Message-ID<qT6Vt-2Hl-21@gated-at.bofh.it>
In reply to#1313467
Andy Shevchenko <andy.shevchenko@gmail.com> writes:

> On Wed, Jan 20, 2016 at 9:24 PM, Måns Rullgård <mans@mansr.com> wrote:
>> Andy Shevchenko <andriy.shevchenko@linux.intel.com> writes:
>>
>>> On Wed, 2016-01-20 at 18:50 +0000, Måns Rullgård wrote:
>>>> Andy Shevchenko <andriy.shevchenko@linux.intel.com> writes:
>>>>
>>>> > > Tested on AVR32 (DMA driver) and Sigma SMP8642 (SATA driver).
>>>> > >
>>>> > > Code here:
>>>> > > https://bitbucket.org/mansr/linux-dwc/branch/dwc-sata
>>>> >
>>>> > Thanks for testing!
>>>>
>>>> Have you had time to look any more at these patches?
>>>
>>> Not yet.
>>> I will look soon for sure.
>>>
>>> One comment still regarding to lli types. We can avoid warnings by
>>> using (__force u32) in macros.
>>
>> But that won't give the benefits of having the types checked.
>
> You mean if we access the lli->field directly? I didn't quite get what
> use case you are keeping in mind.

Yes, accessing any of those fields directly with my patch gives a sparse
warning.  It's situations like these those checks are intended for.
Defeating them seems foolish to me.

-- 
Måns Rullgård

[toc] | [prev] | [next] | [standalone]

#1313478 — Re: [PATCH 1/3] ata: sata_dwc_460ex: use "dmas" DT property to find dma channel

FromAndy Shevchenko <andy.shevchenko@gmail.com>
Date2016-01-20 21:00 +0100
SubjectRe: [PATCH 1/3] ata: sata_dwc_460ex: use "dmas" DT property to find dma channel
Message-ID<qT759-2KF-13@gated-at.bofh.it>
In reply to#1313474
On Wed, Jan 20, 2016 at 9:46 PM, Måns Rullgård <mans@mansr.com> wrote:

>>>> One comment still regarding to lli types. We can avoid warnings by
>>>> using (__force u32) in macros.
>>>
>>> But that won't give the benefits of having the types checked.
>>
>> You mean if we access the lli->field directly? I didn't quite get what
>> use case you are keeping in mind.
>
> Yes, accessing any of those fields directly with my patch gives a sparse
> warning.  It's situations like these those checks are intended for.
> Defeating them seems foolish to me.

Otherwise it makes that struct looks ugly.
Why not union, though it still ugly, but less.

-- 
With Best Regards,
Andy Shevchenko

[toc] | [prev] | [next] | [standalone]

#1313492

FromMåns Rullgård <mans@mansr.com>
Date2016-01-20 21:10 +0100
Message-ID<qT7eO-33h-19@gated-at.bofh.it>
In reply to#1313478
Andy Shevchenko <andy.shevchenko@gmail.com> writes:

> On Wed, Jan 20, 2016 at 9:46 PM, Måns Rullgård <mans@mansr.com> wrote:
>
>>>>> One comment still regarding to lli types. We can avoid warnings by
>>>>> using (__force u32) in macros.
>>>>
>>>> But that won't give the benefits of having the types checked.
>>>
>>> You mean if we access the lli->field directly? I didn't quite get what
>>> use case you are keeping in mind.
>>
>> Yes, accessing any of those fields directly with my patch gives a sparse
>> warning.  It's situations like these those checks are intended for.
>> Defeating them seems foolish to me.
>
> Otherwise it makes that struct looks ugly.
> Why not union, though it still ugly, but less.

What's so ugly about it?  IMO data should be declared as the type it
actually is, and here we have fields that might have a different byte
order from the host CPU.  The __be32 and __le32 types were invented to
make such situations clear and allow automatic (sparse) checking.  I'd
say the price of one small typedef is well worth it.  The actual code is
not impacted since it must use the accessor macros anyhow.

-- 
Måns Rullgård

[toc] | [prev] | [next] | [standalone]

#1314871 — Re: [PATCH 1/3] ata: sata_dwc_460ex: use "dmas" DT property to find dma channel

FromAndy Shevchenko <andriy.shevchenko@linux.intel.com>
Date2016-01-22 11:10 +0100
SubjectRe: [PATCH 1/3] ata: sata_dwc_460ex: use "dmas" DT property to find dma channel
Message-ID<qTGPi-2B3-51@gated-at.bofh.it>
In reply to#1313492
On Wed, 2016-01-20 at 20:07 +0000, Måns Rullgård wrote:
> Andy Shevchenko <andy.shevchenko@gmail.com> writes:
> 
> > > > > > One comment still regarding to lli types. We can avoid
> > > > > > warnings by
> > > > > > using (__force u32) in macros.
> > > > > 
> > > > > But that won't give the benefits of having the types checked.
> > > > 
> > > > You mean if we access the lli->field directly? I didn't quite
> > > > get what
> > > > use case you are keeping in mind.
> > > 
> > > Yes, accessing any of those fields directly with my patch gives a
> > > sparse
> > > warning.  It's situations like these those checks are intended
> > > for.
> > > Defeating them seems foolish to me.
> > 
> > Otherwise it makes that struct looks ugly.
> > Why not union, though it still ugly, but less.
> 
> What's so ugly about it?  IMO data should be declared as the type it
> actually is, and here we have fields that might have a different byte
> order from the host CPU.  The __be32 and __le32 types were invented
> to
> make such situations clear and allow automatic (sparse)
> checking.  I'd
> say the price of one small typedef is well worth it.  The actual code
> is
> not impacted since it must use the accessor macros anyhow.

Okay, let's move with current state.

I have few style minors and a question.

So, in type definitions can we use __dw32 instead of dw_u32?
In DWC_DEFAULT_CTLLO() can we do tab indentation for \ ?

Now the question: who do you prefer to submit the series (dw_dmac)? Me
or you?

In case you would like to do it (what I see in your dwc-sata branch
today):
Acked-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

[toc] | [prev] | [next] | [standalone]

#1314911

FromMåns Rullgård <mans@mansr.com>
Date2016-01-22 12:20 +0100
Message-ID<qTHV0-3h7-13@gated-at.bofh.it>
In reply to#1314871
Andy Shevchenko <andriy.shevchenko@linux.intel.com> writes:

> On Wed, 2016-01-20 at 20:07 +0000, Måns Rullgård wrote:
>> Andy Shevchenko <andy.shevchenko@gmail.com> writes:
>> 
>> > > > > > One comment still regarding to lli types. We can avoid
>> > > > > > warnings by
>> > > > > > using (__force u32) in macros.
>> > > > > 
>> > > > > But that won't give the benefits of having the types checked.
>> > > > 
>> > > > You mean if we access the lli->field directly? I didn't quite
>> > > > get what
>> > > > use case you are keeping in mind.
>> > > 
>> > > Yes, accessing any of those fields directly with my patch gives a
>> > > sparse
>> > > warning.  It's situations like these those checks are intended
>> > > for.
>> > > Defeating them seems foolish to me.
>> > 
>> > Otherwise it makes that struct looks ugly.
>> > Why not union, though it still ugly, but less.
>> 
>> What's so ugly about it?  IMO data should be declared as the type it
>> actually is, and here we have fields that might have a different byte
>> order from the host CPU.  The __be32 and __le32 types were invented
>> to
>> make such situations clear and allow automatic (sparse)
>> checking.  I'd
>> say the price of one small typedef is well worth it.  The actual code
>> is
>> not impacted since it must use the accessor macros anyhow.
>
> Okay, let's move with current state.
>
> I have few style minors and a question.
>
> So, in type definitions can we use __dw32 instead of dw_u32?

Sure, no problem.

> In DWC_DEFAULT_CTLLO() can we do tab indentation for \ ?

Is there a wrong indentation somewhere?  I don't see it.

> Now the question: who do you prefer to submit the series (dw_dmac)? Me
> or you?
>
> In case you would like to do it (what I see in your dwc-sata branch
> today):
> Acked-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

I'll fix the above, give your changes a review, and add my sign-off
before sending the series, today or during the weekend.

-- 
Måns Rullgård

[toc] | [prev] | [next] | [standalone]

#1314929 — Re: [PATCH 1/3] ata: sata_dwc_460ex: use "dmas" DT property to find dma channel

FromAndy Shevchenko <andriy.shevchenko@linux.intel.com>
Date2016-01-22 13:00 +0100
SubjectRe: [PATCH 1/3] ata: sata_dwc_460ex: use "dmas" DT property to find dma channel
Message-ID<qTIxI-3xw-13@gated-at.bofh.it>
In reply to#1314911
On Fri, 2016-01-22 at 11:13 +0000, Måns Rullgård wrote:
> Andy Shevchenko <andriy.shevchenko@linux.intel.com> writes:
> > On Wed, 2016-01-20 at 20:07 +0000, Måns Rullgård wrote:
> > > Andy Shevchenko <andy.shevchenko@gmail.com> writes:

> > In DWC_DEFAULT_CTLLO() can we do tab indentation for \ ?
> 
> Is there a wrong indentation somewhere?  I don't see it.

My git diff shows this in particular:

--- a/drivers/dma/dw/core.c
+++ b/drivers/dma/dw/core.c
@@ -45,10 +45,10 @@
                        DW_DMA_MSIZE_16;                        \
                u8 _dmsize = _is_slave ? _sconfig->dst_maxburst
:       \
                        DW_DMA_MSIZE_16;                        \
-               u8 _dms = (_is_slave && _dwc->direction ==
DMA_MEM_TO_DEV) ? \
-                       _dwc->p_master : _dwc-
>m_master;                \
-               u8 _sms = (_is_slave && _dwc->direction ==
DMA_DEV_TO_MEM) ? \
-                       _dwc->p_master : _dwc-
>m_master;                \
+               u8 _dms = (_is_slave && _dwc->direction ==
DMA_MEM_TO_DEV) ?    \
+                          _dwc->p_master : _dwc-
>m_master;                     \
+               u8 _sms = (_is_slave && _dwc->direction ==
DMA_DEV_TO_MEM) ?    \
+                          _dwc->p_master : _dwc-
>m_master;                     \

Means in your case the ' \' is used, in mine — '\t\' at the end of
lines.

> Now the question: who do you prefer to submit the series (dw_dmac)?
> > Me
> > or you?
> > 
> > In case you would like to do it (what I see in your dwc-sata branch
> > today):
> > Acked-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> 
> I'll fix the above, give your changes a review, and add my sign-off
> before sending the series, today or during the weekend.

OK.

Just to be sure we are on the same page. I assume your dwc-sata branch
as for submitting.

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

[toc] | [prev] | [next] | [standalone]

#1314935

FromMåns Rullgård <mans@mansr.com>
Date2016-01-22 13:10 +0100
Message-ID<qTIHo-3Qm-23@gated-at.bofh.it>
In reply to#1314929
Andy Shevchenko <andriy.shevchenko@linux.intel.com> writes:

> On Fri, 2016-01-22 at 11:13 +0000, Måns Rullgård wrote:
>> Andy Shevchenko <andriy.shevchenko@linux.intel.com> writes:
>> > On Wed, 2016-01-20 at 20:07 +0000, Måns Rullgård wrote:
>> > > Andy Shevchenko <andy.shevchenko@gmail.com> writes:
>
>> > In DWC_DEFAULT_CTLLO() can we do tab indentation for \ ?
>> 
>> Is there a wrong indentation somewhere?  I don't see it.
>
> My git diff shows this in particular:
>
> --- a/drivers/dma/dw/core.c
> +++ b/drivers/dma/dw/core.c
> @@ -45,10 +45,10 @@
>                         DW_DMA_MSIZE_16;                        \
>                 u8 _dmsize = _is_slave ? _sconfig->dst_maxburst :       \
>                         DW_DMA_MSIZE_16;                        \
> -               u8 _dms = (_is_slave && _dwc->direction == DMA_MEM_TO_DEV) ? \
> -                       _dwc->p_master : _dwc->m_master;                \
> -               u8 _sms = (_is_slave && _dwc->direction == DMA_DEV_TO_MEM) ? \
> -                       _dwc->p_master : _dwc->m_master;                \
> +               u8 _dms = (_is_slave && _dwc->direction == DMA_MEM_TO_DEV) ?    \
> +                          _dwc->p_master : _dwc->m_master;                     \
> +               u8 _sms = (_is_slave && _dwc->direction == DMA_DEV_TO_MEM) ?    \
> +                          _dwc->p_master : _dwc->m_master;                     \
>
> Means in your case the ' \' is used, in mine — '\t\' at the end of
> lines.

Oh, I see it now.  Two lines have a space rather than a tab since a tab
would push them over 80 columns.  Now those lines could be shortened by
simply dropping the "_is_slave" since this is necessarily true if the
other half of the && is.

>> Now the question: who do you prefer to submit the series (dw_dmac)?
>> > Me
>> > or you?
>> > 
>> > In case you would like to do it (what I see in your dwc-sata branch
>> > today):
>> > Acked-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
>> 
>> I'll fix the above, give your changes a review, and add my sign-off
>> before sending the series, today or during the weekend.
>
> OK.
>
> Just to be sure we are on the same page. I assume your dwc-sata branch
> as for submitting.

Yes, I'm not aware of anything that needs to change in that branch.

-- 
Måns Rullgård

[toc] | [prev] | [next] | [standalone]

#1314946 — Re: [PATCH 1/3] ata: sata_dwc_460ex: use "dmas" DT property to find dma channel

FromAndy Shevchenko <andy.shevchenko@gmail.com>
Date2016-01-22 13:20 +0100
SubjectRe: [PATCH 1/3] ata: sata_dwc_460ex: use "dmas" DT property to find dma channel
Message-ID<qTIR4-3TR-27@gated-at.bofh.it>
In reply to#1314935
On Fri, Jan 22, 2016 at 2:05 PM, Måns Rullgård <mans@mansr.com> wrote:
> Andy Shevchenko <andriy.shevchenko@linux.intel.com> writes:
>> On Fri, 2016-01-22 at 11:13 +0000, Måns Rullgård wrote:
>>> Andy Shevchenko <andriy.shevchenko@linux.intel.com> writes:
>>> > On Wed, 2016-01-20 at 20:07 +0000, Måns Rullgård wrote:
>>> > > Andy Shevchenko <andy.shevchenko@gmail.com> writes:

>> +               u8 _dms = (_is_slave && _dwc->direction == DMA_MEM_TO_DEV) ?    \
>> +                          _dwc->p_master : _dwc->m_master;                     \
>> +               u8 _sms = (_is_slave && _dwc->direction == DMA_DEV_TO_MEM) ?    \
>> +                          _dwc->p_master : _dwc->m_master;                     \

> Oh, I see it now.  Two lines have a space rather than a tab since a tab
> would push them over 80 columns.  Now those lines could be shortened by
> simply dropping the "_is_slave" since this is necessarily true if the
> other half of the && is.

Yes, direction is precisely from the _is_slave space.

-- 
With Best Regards,
Andy Shevchenko

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web