Re: [PATCH v3 0/3] dmaengine: DW DMAC: split pdata to hardware properties

[Eugeniy Paltsev]

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