Re: [PATCH 2/3] dmaengine: dw_dmac: Enhance device tree support

[Shevchenko, Andriy]

On Fri, 2012-10-12 at 11:14 +0530, Viresh Kumar wrote: 
> dw_dmac driver already supports device tree but it used to have its platform
> data passed the non-DT way.
Another portion of comments.

> diff --git a/drivers/dma/dw_dmac.c b/drivers/dma/dw_dmac.c

> +static struct dw_dma_platform_data *
> +__devinit dw_dma_parse_dt(struct platform_device *pdev)
> +{
> +	struct device_node *sn, *cn, *np = pdev->dev.of_node;
> +	struct dw_dma_platform_data *pdata;
> +	struct dw_dma_slave *sd;
> +	u32 count, val, arr[4];
> +
> +	if (!np) {
> +		dev_err(&pdev->dev, "Missing DT data\n");
> +		return NULL;
> +	}
> +
> +	pdata = devm_kzalloc(&pdev->dev, sizeof(*pdata), GFP_KERNEL);
> +	if (!pdata)
> +		return NULL;
> +
> +	if (of_property_read_u32(np, "nr_channels", &pdata->nr_channels))
> +		return NULL;
> +
> +	if (of_property_read_bool(np, "is_private"))
> +		pdata->is_private = true;
> +
> +	if (!of_property_read_u32(np, "chan_allocation_order",
> +				&val))
Fits one line?

> +		pdata->chan_allocation_order = (unsigned char)val;
do we really need explicit casting here?

> +
> +	if (!of_property_read_u32(np, "chan_priority", &val))
> +		pdata->chan_priority = (unsigned char)val;
ditto

> +
> +	if (!of_property_read_u32(np, "block_size", &val))
> +		pdata->block_size = (unsigned short)val;
ditto 
> +
> +	if (!of_property_read_u32(np, "nr_masters", &val)) {
> +		if (val > 4)
I thought once that we might introduce constant like #define
DW_MAX_AHB_MASTERS 4. It seems a bit useless because hw is designed for
that number anyway, but maybe you have another opinion. 

> +			return NULL;
> +
> +		pdata->nr_masters = (unsigned char)val;
> +	}
> +
> +	if (!of_property_read_u32_array(np, "data_width", arr,
> +				pdata->nr_masters))
> +		for (count = 0; count < pdata->nr_masters; count++)
> +			pdata->data_width[count] = arr[count];
Ah, it would be nice to have of_property_read_u8_array and so on...

> +
> +	/* parse slave data */
> +	sn = of_find_node_by_name(np, "slave_info");
> +	if (!sn)
> +		return pdata;
> +
> +	count = 0;
> +	/* calculate number of slaves */
> +	for_each_child_of_node(sn, cn)
> +		count++;

of.h: static inline int of_get_child_count(const struct device_node *np)


> +
> +	if (!count)
> +		return NULL;
> +
> +	sd = devm_kzalloc(&pdev->dev, sizeof(*sd) * count, GFP_KERNEL);
> +	if (!sd)
> +		return NULL;
> +
> +	count = 0;
> +	for_each_child_of_node(sn, cn) {
> +		of_property_read_string(cn, "bus_id", &sd[count].bus_id);
> +		of_property_read_u32(cn, "cfg_hi", &sd[count].cfg_hi);
> +		of_property_read_u32(cn, "cfg_lo", &sd[count].cfg_lo);
> +		if (!of_property_read_u32(cn, "src_master", &val))
> +			sd[count].src_master = (u8)val;
Explicit casting?

> +
> +		if (!of_property_read_u32(cn, "dst_master", &val))
> +			sd[count].dst_master = (u8)val;
ditto 
> +
> +		sd[count].dma_dev = &pdev->dev;
> +		count++;
> +	}

what about to define sd as sd[0]; in the platform_data structure and
then use smth like following

struct ... *sd = pdata->sd;
for_each... {
sd->... = ;
sd++;
}

it will probably require to split this part in a separate function and
calculate count of slave_info children before pdata memory allocation.


> +
> +	pdata->sd = sd;
> +	pdata->sd_count = count;
> +
> +	return pdata;
> +}

-- 
Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>
Intel Finland Oy

-- 
Andy Shevchenko <andriy.shevchenko@xxxxxxxxx>
Intel Finland Oy
---------------------------------------------------------------------
Intel Finland Oy
Registered Address: PL 281, 00181 Helsinki 
Business Identity Code: 0357606 - 4 
Domiciled in Helsinki 

This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.
¢éì®&Þ~º&¶¬–+-±éÝ¥Šw®žË±Êâmébžìdz¹Þ)í…æèw*jg¬±¨¶‰šŽŠÝj/êäz¹ÞŠà2ŠÞ¨è­Ú&¢)ß«a¶Úþø®G«éh®æj:+v‰¨Šwè†Ù>Wš±êÞiÛaxPjØm¶ŸÿÃ-»+ƒùdš_