Re: [PATCH v3 08/15] dmaengine: dw: revisit data_width property

[Andy Shevchenko <andriy.shevchenko@linux.intel.com>]

On Wed, 2016-04-13 at 21:47 +0530, Vinod Koul wrote:
> On Wed, Apr 13, 2016 at 07:05:48PM +0300, Andy Shevchenko wrote:
> > 
> > On Wed, 2016-04-13 at 21:27 +0530, Vinod Koul wrote:
> > > 
> > > On Fri, Mar 18, 2016 at 04:24:47PM +0200, Andy Shevchenko wrote:
> > > The driver should still work with older DT implementation, so you
> > > need
> > > to
> > > keep support for that in driver and hence I don't see any benfit
> > > we
> > > would
> > > get from doing both in driver!
> > The device tree is screwed by a process that allows to do almost
> > whatever you want. Here I'm trying to rectify the usage of the
> > field.
> Well at least we should try to do the right thing which means pushing
> back
> on ABI breakage..

There is no such.

> 
> > 
> > The old is still supported and benefit is apparently in unifying
> > standard properties across the drivers.
> Hrmmm how is that?

The common usage for data-width property is "in bytes". And I like the
idea. I don't know why at all I chose to keep encoded value there in the
first place and no one commented at that time. I suppose because of
screwed device tree process. I think now it's better to follow some
standard / registered properties in new drivers.

> 
> > 
> > > 
> > > > 
> > > > @@ -102,8 +102,8 @@ dw_dma_parse_dt(struct platform_device
> > > > *pdev)
> > > >  {
> > > >  	struct device_node *np = pdev->dev.of_node;
> > > >  	struct dw_dma_platform_data *pdata;
> > > > -	u32 tmp, arr[DW_DMA_MAX_NR_MASTERS];
> > > >  	u32 nr_channels;
> > > > +	u32 tmp;
> > > >  
> > > >  	if (!np) {
> > > >  		dev_err(&pdev->dev, "Missing DT data\n");
> > > > @@ -138,10 +138,10 @@ dw_dma_parse_dt(struct platform_device
> > > > *pdev)
> > > >  		pdata->nr_masters = tmp;
> > > >  	}
> > > >  
> > > > -	if (!of_property_read_u32_array(np, "data_width", arr,
> > > > -				pdata->nr_masters))
> > > > -		for (tmp = 0; tmp < pdata->nr_masters; tmp++)
> > > > -			pdata->data_width[tmp] = arr[tmp];
> You stop reading the array

Yes, due to "- Use one value for all AHB masters for now". No reason to
read all of them.

> > > > +	if (!of_property_read_u32(np, "data-width", &tmp))
> > > > +		pdata->data_width = tmp;
> > > > +	else if (!of_property_read_u32(np, "data_width", &tmp))
> > > > +		pdata->data_width = BIT(tmp & 0x07);
> And read a value? how will this work with older array?

It will read first element.

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