On Tuesday 05 February 2013, Padma Venkat wrote:
> In none of my patches I have changed the pl330_filter args. This
> function always takes the same argument void*. In non-DT case 'enum
> dma_ch' was typecasted to void* and in DT case I am passing a pointer
> to dma_pl330_filter_args and in pl330_filter function they are
> converted back. In both cases it finally comes down to
> dma_request_channel which takes them as void* which in turn calls the
> pl330_filter.
>
> I think this is what you are pointing to. Please let me know if I am
> still wrong :( .
I think I see the misunderstanding now. The pl330_filter function
you have actually interprets the void* argument differently, based
on whether the pl330 device was instantiated from device tree or not.
I failed to see this, and you are probably right that this works
correctly.
It is however a rather unusual interface, and it would be safer and
easier to understand if you used separate filter functions for
the two cases, like this:
bool pl330_filter(struct dma_chan *chan, void *param)
{
u8 *peri_id;
if (chan->device->dev->driver != &pl330_driver.drv)
return false;
peri_id = chan->private;
return *peri_id == (unsigned)param;
}
EXPORT_SYMBOL(pl330_filter);
static bool pl330_dt_filter(struct dma_chan *chan, void *param)
{
struct dma_pl330_filter_args *fargs = param;
if (chan->device != &fargs->pdmac->ddma)
return false;
return (chan->chan_id == fargs->chan_id);
}
So this is not a correctness issue, but more one of readability. I would
assume however that if I was misunderstanding the code, the next person
also wouldn't know what is going on here if you have one filter function
that performs two completely different tasks.
Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
