diff for duplicates of <1478607771.2603.31.camel@synopsys.com> diff --git a/a/1.txt b/N1/1.txt index f109806..bc64cd1 100644 --- a/a/1.txt +++ b/N1/1.txt @@ -1,111 +1,111 @@ -On Mon, 2016-11-07@15:55 +0200, Andy Shevchenko wrote: ->? +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. +My comments are given inline below. --- > > Changes for v2: -> >????- use separate bool values for quirks in "dw_dma_platform_data" +> > - use separate bool values for quirks in "dw_dma_platform_data" > > instead of -> >??????common bit field. -> >? -> >????- convert device tree properties reading to unified device +> > 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. +> +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"? +> > > +> > > 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.? +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; -> >?? +> > dw->regs = chip->regs; +> > chip->dw = dw; +> > > > + /* Reassign the platform data pointer */ > > + pdata = dw->pdata; > > + -> >?? pm_runtime_get_sync(chip->dev); -> >?? +> > 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--] -> >? +> +> > [--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.? +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 = 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. +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 @@ -115,38 +115,38 @@ Implemented in?PATCH v3 series. > Can you somehow be more clear that all listed quirks will be copied > from > platform data. -See?comment below. +See comment below. ->? -> >? -> >???* @is_nollp: The device channels does not support multi block +> +> > +> > * @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 +> > * @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; -> >? +> > 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; +> 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 +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 +-- + Paltsev Eugeniy diff --git a/a/content_digest b/N1/content_digest index 2f4c20e..4102252 100644 --- a/a/content_digest +++ b/N1/content_digest @@ -1,120 +1,127 @@ "ref\01477670402-23943-1-git-send-email-Eugeniy.Paltsev@synopsys.com\0" "ref\01478087707.2603.7.camel@synopsys.com\0" "ref\01478526908.5295.67.camel@linux.intel.com\0" - "From\0Eugeniy.Paltsev@synopsys.com (Eugeniy Paltsev)\0" - "Subject\0[PATCH v3 0/3] dmaengine: DW DMAC: split pdata to hardware properties\0" + "From\0Eugeniy Paltsev <Eugeniy.Paltsev@synopsys.com>\0" + "Subject\0Re: [PATCH v3 0/3] dmaengine: DW DMAC: split pdata to hardware properties\0" "Date\0Tue, 8 Nov 2016 12:22:51 +0000\0" - "To\0linux-snps-arc@lists.infradead.org\0" + "To\0andriy.shevchenko@linux.intel.com <andriy.shevchenko@linux.intel.com>\0" + "Cc\0dan.j.williams@intel.com <dan.j.williams@intel.com>" + linux-kernel@vger.kernel.org <linux-kernel@vger.kernel.org> + Eugeniy.Paltsev@synopsys.com <Eugeniy.Paltsev@synopsys.com> + dmaengine@vger.kernel.org <dmaengine@vger.kernel.org> + vinod.koul@intel.com <vinod.koul@intel.com> + vireshk@kernel.org <vireshk@kernel.org> + " linux-snps-arc@lists.infradead.org <linux-snps-arc@lists.infradead.org>\0" "\00:1\0" "b\0" - "On Mon, 2016-11-07@15:55 +0200, Andy Shevchenko wrote:\n" - ">?\n" + "On Mon, 2016-11-07 at 15:55 +0200, Andy Shevchenko wrote:\n" + ">\302\240\n" "> Thanks for an update, but, please, answer to all my comments to your\n" "> patch v2. Either you are okay with them, then you didn't address few,\n" "> or\n" "> you are not okay, I didn't get why. Deffer newer version until we get\n" "> an\n" "> agreement on the implementation.\n" - ">?\n" + ">\302\240\n" "\n" "Thanks for response.\n" - "My comments are given inline?below.\n" + "My comments are given inline\302\240below.\n" "\n" "\n" "---\n" "> > Changes for v2:\n" - "> >????- use separate bool values for quirks in \"dw_dma_platform_data\"\n" + "> >\302\240\302\240\302\240\302\240- use separate bool values for quirks in \"dw_dma_platform_data\"\n" "> > instead of\n" - "> >??????common bit field.\n" - "> >?\n" - "> >????- convert device tree properties reading to unified device\n" + "> >\302\240\302\240\302\240\302\240\302\240\302\240common bit field.\n" + "> >\302\240\n" + "> >\302\240\302\240\302\240\302\240- convert device tree properties reading to unified device\n" "> > property\n" "> > API.\n" "> This should be a separate patch.\n" - ">?\n" - "Agree. Implemented as?separate patch in?PATCH v3 series.\n" + ">\302\240\n" + "Agree. Implemented as\302\240separate patch in\302\240PATCH v3 series.\n" "\n" - "> >?\n" - "> >?\n" + "> >\302\240\n" + "> >\302\240\n" "> > I was wrong about DW_DMA_IS_SOFT_LLP flag - it is used to check\n" "> > about\n" "> > ongoing soft llp transfer. So DW_DMA_IS_SOFT_LLP flag and \"dwc-\n" - "> > >?\n" - "> > > nollp\"?\n" + "> > >\302\240\n" + "> > > nollp\"\302\240\n" "> > variable have different functions and I couldn't just get rid of\n" "> > \"dwc-\n" - "> > >?\n" + "> > >\302\240\n" "> > > nollp\"\n" "> > and use DW_DMA_IS_SOFT_LLP flag instead. So I left \"dwc->nollp\"\n" "> > untouched.\n" "> So, then perhaps we may convert it to another flag let's say\n" "> DW_DMA_IS_LLP_SUPPORTED.\n" - ">?\n" + ">\302\240\n" "> But this is other story independent of the subject.\n" "\n" - "Implemented in?PATCH v3 series.?\n" + "Implemented in\302\240PATCH v3 series.\302\240\n" "\"dwc->nollp\" was converted to \"DW_DMA_IS_LLP_SUPPORTED\" flag.\n" "\n" - "> >?\n" + "> >\302\240\n" "> > --- a/drivers/dma/dw/core.c\n" "> > +++ b/drivers/dma/dw/core.c\n" "> > @@ -1452,9 +1452,24 @@ int dw_dma_probe(struct dw_dma_chip *chip)\n" - "> >??\tdw->regs = chip->regs;\n" - "> >??\tchip->dw = dw;\n" - "> >??\n" + "> >\302\240\302\240\tdw->regs = chip->regs;\n" + "> >\302\240\302\240\tchip->dw = dw;\n" + "> >\302\240\302\240\n" "> > +\t/* Reassign the platform data pointer */\n" "> > +\tpdata = dw->pdata;\n" "> > +\n" - "> >??\tpm_runtime_get_sync(chip->dev);\n" - "> >??\n" + "> >\302\240\302\240\tpm_runtime_get_sync(chip->dev);\n" + "> >\302\240\302\240\n" "> > -\tif (!chip->pdata) {\n" "> > +\tif ((!chip->pdata) || (chip->pdata && chip->pdata-\n" - "> > >?\n" + "> > >\302\240\n" "> > > only_quirks_used)) {\n" "> It's simple as\n" "> if (!chip->pdata || chip->pdata->only_quirks_used)\n" - ">?\n" - "> > ?[--sources--]\n" - "> >?\n" + ">\302\240\n" + "> > \302\240[--sources--]\n" + "> >\302\240\n" "> Would we leave the first part in the place it is now and add new\n" "> piece\n" "> after?\n" - ">?\n" + ">\302\240\n" "> > [--sources--]\n" - "> >?\n" + "> >\302\240\n" "> ...like\n" - ">?\n" + ">\302\240\n" "> /* Apply platform defined quirks */\n" "> if (chip->data && chip->data->only_quirks_used) {\n" - ">??...\n" + ">\302\240\302\240...\n" "> }\n" "Agree. That looks better.\n" "\n" - ">?\n" - "> >?\n" + ">\302\240\n" + "> >\302\240\n" "> > -\tif (of_property_read_u32(np, \"dma-channels\",\n" "> > &nr_channels))\n" "> > -\t\treturn NULL;\n" "> > +\tif (device_property_read_bool(dev, \"is-private\"))\n" "> As I mentioned above, please do a separate patch for this.\n" - "Implemented as?separate patch in?PATCH v3 series.?\n" + "Implemented as\302\240separate patch in\302\240PATCH v3 series.\302\240\n" "\n" - ">?\n" - "> >?\n" + ">\302\240\n" + "> >\302\240\n" "> > @@ -183,7 +186,7 @@ static int dw_probe(struct platform_device\n" "> > *pdev)\n" - "> >??\n" - "> >??\tpdata = dev_get_platdata(dev);\n" - "> >??\tif (!pdata)\n" + "> >\302\240\302\240\n" + "> >\302\240\302\240\tpdata = dev_get_platdata(dev);\n" + "> >\302\240\302\240\tif (!pdata)\n" "> > -\t\tpdata = dw_dma_parse_dt(pdev);\n" "> > +\t\tpdata = dw_dma_parse_dt(dev);\n" "> Perhaps you might rename the function to something like\n" - ">?\n" + ">\302\240\n" "> dw_dma_parse_properties(dev);\n" - "Implemented in?PATCH v3 series.\n" + "Implemented in\302\240PATCH v3 series.\n" "\n" - ">?\n" - "> >?\n" + ">\302\240\n" + "> >\302\240\n" "> > + * @only_quirks_used: Only read quirks (like \"is_private\" or\n" "> > \"is_memcpy\") from\n" "> > + *\tplatform data structure. Read other parameters from\n" @@ -124,40 +131,40 @@ "> Can you somehow be more clear that all listed quirks will be copied\n" "> from\n" "> platform data.\n" - "See?comment below.\n" + "See\302\240comment below.\n" "\n" - ">?\n" - "> >?\n" - "> >???* @is_nollp: The device channels does not support multi block\n" + ">\302\240\n" + "> >\302\240\n" + "> >\302\240\302\240\302\240* @is_nollp: The device channels does not support multi block\n" "> > transfers.\n" - "> >???* @chan_allocation_order: Allocate channels starting from 0 or 7\n" - "> >???* @chan_priority: Set channel priority increasing from 0 to 7 or\n" + "> >\302\240\302\240\302\240* @chan_allocation_order: Allocate channels starting from 0 or 7\n" + "> >\302\240\302\240\302\240* @chan_priority: Set channel priority increasing from 0 to 7 or\n" "> > 7\n" "> > to 0.\n" "> > @@ -52,6 +55,7 @@ struct dw_dma_platform_data {\n" - "> >??\tunsigned int\tnr_channels;\n" - "> >??\tbool\t\tis_private;\n" - "> >??\tbool\t\tis_memcpy;\n" - "> >?\n" + "> >\302\240\302\240\tunsigned int\tnr_channels;\n" + "> >\302\240\302\240\tbool\t\tis_private;\n" + "> >\302\240\302\240\tbool\t\tis_memcpy;\n" + "> >\302\240\n" "> > +\tbool\t\tonly_quirks_used;\n" - "> Perhaps add if at the end of quirk list and name just?\n" - ">?\n" - "> >?\n" - "> >??\tbool\t\tis_nollp;\n" + "> Perhaps add if at the end of quirk list and name just\302\240\n" + ">\302\240\n" + "> >\302\240\n" + "> >\302\240\302\240\tbool\t\tis_nollp;\n" "> ...here\n" - ">?\n" + ">\302\240\n" "> bool use_quirks;\n" - ">?\n" + ">\302\240\n" "\n" "I don't treat \"is_nollp\" as quirks like \"is_private\" or \"is_memcpy\".\n" - "It is like general pdata field: we can easily?read it from autoconfig\n" + "It is like general pdata field: we can easily\302\240read it from autoconfig\n" "registers (and we don't have any problem with that) in case of\n" "pdata/device-tree absence (as opposed to quirks like \"is_private\" or\n" "\"is_memcpy\")\n" "\n" "So, in PATCH v3 series \"is_nollp\" used as regular pdata field.\n" "\n" - "--?\n" - ?Paltsev Eugeniy + "--\302\240\n" + "\302\240Paltsev Eugeniy" -1661a462600c57a5088e1b1679e80bb1eb97773ab2f44feb43c61ce9a5b3d3cd +f06864f869ec84a624facc159dd69d10526edceb92ed448f326f0cb32108a9d0
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.