| Message ID | CAKohpok=nv2XnFdPkD_yJgUsDqx2gEh=mG_Xhbe-BvT7swVsyg@mail.gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, 2012-10-12 at 19:36 +0530, Viresh Kumar wrote: > On 12 October 2012 18:58, Shevchenko, Andriy > <andriy.shevchenko@intel.com> wrote: > > On Fri, 2012-10-12 at 11:14 +0530, Viresh Kumar wrote: > > >> + 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. > > Not everyone have four masters in there SoC configurations. So its better > to export correct values. I meant is to use that constant instead of hard coding 4 everywhere. It's a maximum value, not the SoC specific. > >> + 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... > > :) > Maybe yes. I don't want to do it with this patchset, as that will take more > time to go through maintainers. Agreed, the comment just for future. I don't know if there any other users of *_u8/u16_array(). > > it will probably require to split this part in a separate function and > > calculate count of slave_info children before pdata memory allocation. > > Liked the idea partially. Check my new implementation in fixup for all > these comments: Let me see. > diff --git a/drivers/dma/dw_dmac.c b/drivers/dma/dw_dmac.c > @@ -1516,7 +1516,7 @@ __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]; > + u32 val, arr[4]; what about to use tmp name instead of val? It's really minor, but I think val name is little bit unrelated to a loop counter. > @@ -1533,63 +1533,57 @@ __devinit dw_dma_parse_dt(struct platform_device *pdev) > - count = 0; > + pdata->sd = sd; > + pdata->sd_count = val; > + > 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); > + sd->dma_dev = &pdev->dev; > + of_property_read_string(cn, "bus_id", &sd->bus_id); > + of_property_read_u32(cn, "cfg_hi", &sd->cfg_hi); > + of_property_read_u32(cn, "cfg_lo", &sd->cfg_lo); > if (!of_property_read_u32(cn, "src_master", &val)) > - sd[count].src_master = (u8)val; > + sd->src_master = val; > > if (!of_property_read_u32(cn, "dst_master", &val)) > - sd[count].dst_master = (u8)val; > - > - sd[count].dma_dev = &pdev->dev; > - count++; > + sd->dst_master = val; > + sd++; > } This looks good.
On 12 October 2012 20:11, Shevchenko, Andriy <andriy.shevchenko@intel.com> wrote: > On Fri, 2012-10-12 at 19:36 +0530, Viresh Kumar wrote: > I meant is to use that constant instead of hard coding 4 everywhere. > It's a maximum value, not the SoC specific. Can be done. >> + u32 val, arr[4]; > what about to use tmp name instead of val? It's really minor, but I > think val name is little bit unrelated to a loop counter. Its not minor at all, its major. Even when i was coding it, i thought about renaming it several times during this last patch. But couldn't find a better name. I don't like tmp. Give me something better. viresh
diff --git a/drivers/dma/dw_dmac.c b/drivers/dma/dw_dmac.c index c24859e..d72c26f 100644 --- a/drivers/dma/dw_dmac.c +++ b/drivers/dma/dw_dmac.c @@ -1179,7 +1179,7 @@ static void dwc_free_chan_resources(struct dma_chan *chan) dev_vdbg(chan2dev(chan), "%s: done\n", __func__); } -bool dw_generic_filter(struct dma_chan *chan, void *param) +bool dw_dma_generic_filter(struct dma_chan *chan, void *param) { struct dw_dma *dw = to_dw_dma(chan->device); static struct dw_dma *last_dw; @@ -1224,7 +1224,7 @@ bool dw_generic_filter(struct dma_chan *chan, void *param) last_bus_id = param; return false; } -EXPORT_SYMBOL(dw_generic_filter); +EXPORT_SYMBOL(dw_dma_generic_filter); /* --------------------- Cyclic DMA API extensions -------------------- */ @@ -1516,7 +1516,7 @@ __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]; + u32 val, arr[4]; if (!np) { dev_err(&pdev->dev, "Missing DT data\n"); @@ -1533,63 +1533,57 @@ __devinit dw_dma_parse_dt(struct platform_device *pdev) if (of_property_read_bool(np, "is_private")) pdata->is_private = true; - if (!of_property_read_u32(np, "chan_allocation_order", - &val)) + if (!of_property_read_u32(np, "chan_allocation_order", &val)) pdata->chan_allocation_order = (unsigned char)val; if (!of_property_read_u32(np, "chan_priority", &val)) - pdata->chan_priority = (unsigned char)val; + pdata->chan_priority = val; if (!of_property_read_u32(np, "block_size", &val)) - pdata->block_size = (unsigned short)val; + pdata->block_size = val; if (!of_property_read_u32(np, "nr_masters", &val)) { if (val > 4) return NULL; - pdata->nr_masters = (unsigned char)val; + pdata->nr_masters = 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]; + for (val = 0; val < pdata->nr_masters; val++) + pdata->data_width[val] = arr[val]; /* 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++; - - if (!count) + val = of_get_child_count(sn); + if (!val) return NULL; - sd = devm_kzalloc(&pdev->dev, sizeof(*sd) * count, GFP_KERNEL); + sd = devm_kzalloc(&pdev->dev, sizeof(*sd) * val, GFP_KERNEL); if (!sd) return NULL; - count = 0; + pdata->sd = sd; + pdata->sd_count = val; + 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); + sd->dma_dev = &pdev->dev; + of_property_read_string(cn, "bus_id", &sd->bus_id); + of_property_read_u32(cn, "cfg_hi", &sd->cfg_hi); + of_property_read_u32(cn, "cfg_lo", &sd->cfg_lo); if (!of_property_read_u32(cn, "src_master", &val)) - sd[count].src_master = (u8)val; + sd->src_master = val; if (!of_property_read_u32(cn, "dst_master", &val)) - sd[count].dst_master = (u8)val; - - sd[count].dma_dev = &pdev->dev; - count++; + sd->dst_master = val; + sd++; } - pdata->sd = sd; - pdata->sd_count = count; - return pdata; } #else diff --git a/include/linux/dw_dmac.h b/include/linux/dw_dmac.h index 4d1c8c3..41766de 100644 --- a/include/linux/dw_dmac.h +++ b/include/linux/dw_dmac.h @@ -114,6 +114,6 @@ void dw_dma_cyclic_stop(struct dma_chan *chan); dma_addr_t dw_dma_get_src_addr(struct dma_chan *chan); dma_addr_t dw_dma_get_dst_addr(struct dma_chan *chan); -bool dw_generic_filter(struct dma_chan *chan, void *param); +bool dw_dma_generic_filter(struct dma_chan *chan, void *param); #endif /* DW_DMAC_H */