lkml.org 
[lkml]   [2016]   [Nov]   [8]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
From
SubjectRe: [PATCH v3 0/3] dmaengine: DW DMAC: split pdata to hardware properties
Date
On Mon, 2016-11-07 at 15:55 +0200, Andy Shevchenko wrote:

> Thanks for an update, but, please, answer to all my comments to your
> patch v2. Either you are okay with them, then you didn't address few,
> or
> you are not okay, I didn't get why. Deffer newer version until we get
> an
> agreement on the implementation.


Thanks for response.
My comments are given inline below.


---
> > Changes for v2:
> >    - use separate bool values for quirks in "dw_dma_platform_data"
> > instead of
> >      common bit field.
> > 
> >    - convert device tree properties reading to unified device
> > property
> > API.
> This should be a separate patch.

Agree. Implemented as separate patch in PATCH v3 series.

> > 
> > 
> > I was wrong about DW_DMA_IS_SOFT_LLP flag - it is used to check
> > about
> > ongoing soft llp transfer. So DW_DMA_IS_SOFT_LLP flag and "dwc-
> > > 
> > > nollp" 
> > variable have different functions and I couldn't just get rid of
> > "dwc-
> > > 
> > > nollp"
> > and use DW_DMA_IS_SOFT_LLP flag instead. So I left "dwc->nollp"
> > untouched.
> So, then perhaps we may convert it to another flag let's say
> DW_DMA_IS_LLP_SUPPORTED.

> But this is other story independent of the subject.

Implemented in PATCH v3 series. 
"dwc->nollp" was converted to "DW_DMA_IS_LLP_SUPPORTED" flag.

> > 
> > --- a/drivers/dma/dw/core.c
> > +++ b/drivers/dma/dw/core.c
> > @@ -1452,9 +1452,24 @@ int dw_dma_probe(struct dw_dma_chip *chip)
> >   dw->regs = chip->regs;
> >   chip->dw = dw;
> >  
> > + /* Reassign the platform data pointer */
> > + pdata = dw->pdata;
> > +
> >   pm_runtime_get_sync(chip->dev);
> >  
> > - if (!chip->pdata) {
> > + if ((!chip->pdata) || (chip->pdata && chip->pdata-
> > > 
> > > only_quirks_used)) {
> It's simple as
> if (!chip->pdata || chip->pdata->only_quirks_used)

> >  [--sources--]
> > 
> Would we leave the first part in the place it is now and add new
> piece
> after?

> > [--sources--]
> > 
> ...like

> /* Apply platform defined quirks */
> if (chip->data && chip->data->only_quirks_used) {
>  ...
> }
Agree. That looks better.


> > 
> > - if (of_property_read_u32(np, "dma-channels",
> > &nr_channels))
> > - return NULL;
> > + if (device_property_read_bool(dev, "is-private"))
> As I mentioned above, please do a separate patch for this.
Implemented as separate patch in PATCH v3 series. 


> > 
> > @@ -183,7 +186,7 @@ static int dw_probe(struct platform_device
> > *pdev)
> >  
> >   pdata = dev_get_platdata(dev);
> >   if (!pdata)
> > - pdata = dw_dma_parse_dt(pdev);
> > + pdata = dw_dma_parse_dt(dev);
> Perhaps you might rename the function to something like

> dw_dma_parse_properties(dev);
Implemented in PATCH v3 series.


> > 
> > + * @only_quirks_used: Only read quirks (like "is_private" or
> > "is_memcpy") from
> > + * platform data structure. Read other parameters from
> > device
> > tree
> > + * node (if exists) or from hardware autoconfig registers.
> Can you somehow be more clear that all listed quirks will be copied
> from
> platform data.
See comment below.


> > 
> >   * @is_nollp: The device channels does not support multi block
> > transfers.
> >   * @chan_allocation_order: Allocate channels starting from 0 or 7
> >   * @chan_priority: Set channel priority increasing from 0 to 7 or
> > 7
> > to 0.
> > @@ -52,6 +55,7 @@ struct dw_dma_platform_data {
> >   unsigned int nr_channels;
> >   bool is_private;
> >   bool is_memcpy;
> > 
> > + bool only_quirks_used;
> Perhaps add if at the end of quirk list and name just 

> > 
> >   bool is_nollp;
> ...here

> bool use_quirks;


I don't treat "is_nollp" as quirks like "is_private" or "is_memcpy".
It is like general pdata field: we can easily read it from autoconfig
registers (and we don't have any problem with that) in case of
pdata/device-tree absence (as opposed to quirks like "is_private" or
"is_memcpy")

So, in PATCH v3 series "is_nollp" used as regular pdata field.

-- 
 Paltsev Eugeniy
\
 
 \ /
  Last update: 2016-11-08 13:23    [W:0.087 / U:0.048 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site