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��.n��������+%������w��{.n��������)�)�� jg��� �����ݢj����G���� ���j:+v���w�m������w�������h�����٥
