| Message ID | 1359967675-624-1-git-send-email-padma.v@samsung.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Monday 04 February 2013, Padmavathi Venna wrote: > diff --git a/arch/arm/plat-samsung/dma-ops.c b/arch/arm/plat-samsung/dma-ops.c > index 71d58dd..ec0d731 100644 > --- a/arch/arm/plat-samsung/dma-ops.c > +++ b/arch/arm/plat-samsung/dma-ops.c > @@ -23,23 +23,15 @@ static unsigned samsung_dmadev_request(enum dma_ch dma_ch, > struct device *dev, char *ch_name) > { > dma_cap_mask_t mask; > - void *filter_param; > > dma_cap_zero(mask); > dma_cap_set(param->cap, mask); > > - /* > - * If a dma channel property of a device node from device tree is > - * specified, use that as the fliter parameter. > - */ > - filter_param = (dma_ch == DMACH_DT_PROP) ? > - (void *)param->dt_dmach_prop : (void *)dma_ch; > - > if (dev->of_node) > return (unsigned)dma_request_slave_channel(dev, ch_name); > else > return (unsigned)dma_request_channel(mask, pl330_filter, > - filter_param); > + (void *)dma_ch); > } This still looks wrong to me, because the pl330_filter function now tkes a struct dma_pl330_filter_args pointer argument, not dma_ch name. Arnd
Hi Arnd, On Mon, Feb 4, 2013 at 11:13 PM, Arnd Bergmann <arnd@arndb.de> wrote: > On Monday 04 February 2013, Padmavathi Venna wrote: >> diff --git a/arch/arm/plat-samsung/dma-ops.c b/arch/arm/plat-samsung/dma-ops.c >> index 71d58dd..ec0d731 100644 >> --- a/arch/arm/plat-samsung/dma-ops.c >> +++ b/arch/arm/plat-samsung/dma-ops.c >> @@ -23,23 +23,15 @@ static unsigned samsung_dmadev_request(enum dma_ch dma_ch, >> struct device *dev, char *ch_name) >> { >> dma_cap_mask_t mask; >> - void *filter_param; >> >> dma_cap_zero(mask); >> dma_cap_set(param->cap, mask); >> >> - /* >> - * If a dma channel property of a device node from device tree is >> - * specified, use that as the fliter parameter. >> - */ >> - filter_param = (dma_ch == DMACH_DT_PROP) ? >> - (void *)param->dt_dmach_prop : (void *)dma_ch; >> - >> if (dev->of_node) >> return (unsigned)dma_request_slave_channel(dev, ch_name); >> else >> return (unsigned)dma_request_channel(mask, pl330_filter, >> - filter_param); >> + (void *)dma_ch); >> } > > This still looks wrong to me, because the pl330_filter function now tkes > a struct dma_pl330_filter_args pointer argument, not dma_ch name. Below is my understanding about generic dma and our discussion on previous versions of my patches. I can’t pass single dma channel number(may be not dma_ch name in your comment above) as void* argument to pl330_filter. Because I also need to compare against the dma controller device node, as my requested channel can belong to any of the available dma controller on SoC. So I either need to pass pointer to dma_spec as void* argument which holds the dma controller node and required channel number or I can pass pointer to dma_pl330_filter_args as per your dw_dmac patches. If I pass pointer to dma_spec I can have a check like below in my filter function return ((chan->private == dma_spec->np) && (chan->chan_id == dma_spec->args[0])) Or if I pass dma_pl330_filter_args I can have a check like below. return ((chan->device == &fargs->pdmac->ddma) && (chan->chan_id == fargs->chan_id)); I modified the pl330_filter function based on your dw_dmac patches. Indeed I don’t need to pass pointer to pdmac object as 3rd arg in of_dma_controller_register . Even I pass NULL here works for me. Can I pass NULL here as the third argument in of_dma_controller_register? Please clarify me which is best way of doing this and correct me if my understanding is wrong. > > Arnd Thanks Padma
On Tuesday 05 February 2013, Padma Venkat wrote: > On Mon, Feb 4, 2013 at 11:13 PM, Arnd Bergmann <arnd@arndb.de> wrote: > > On Monday 04 February 2013, Padmavathi Venna wrote: > >> diff --git a/arch/arm/plat-samsung/dma-ops.c b/arch/arm/plat-samsung/dma-ops.c > >> index 71d58dd..ec0d731 100644 > >> --- a/arch/arm/plat-samsung/dma-ops.c > >> +++ b/arch/arm/plat-samsung/dma-ops.c > >> @@ -23,23 +23,15 @@ static unsigned samsung_dmadev_request(enum dma_ch dma_ch, > >> struct device *dev, char *ch_name) > >> { > >> dma_cap_mask_t mask; > >> - void *filter_param; > >> > >> dma_cap_zero(mask); > >> dma_cap_set(param->cap, mask); > >> > >> - /* > >> - * If a dma channel property of a device node from device tree is > >> - * specified, use that as the fliter parameter. > >> - */ > >> - filter_param = (dma_ch == DMACH_DT_PROP) ? > >> - (void *)param->dt_dmach_prop : (void *)dma_ch; > >> - > >> if (dev->of_node) > >> return (unsigned)dma_request_slave_channel(dev, ch_name); > >> else > >> return (unsigned)dma_request_channel(mask, pl330_filter, > >> - filter_param); > >> + (void *)dma_ch); > >> } > > > > This still looks wrong to me, because the pl330_filter function now tkes > > a struct dma_pl330_filter_args pointer argument, not dma_ch name. > > Below is my understanding about generic dma and our discussion on > previous versions of my patches. > > I can’t pass single dma channel number(may be not dma_ch name in your > comment above) as void* argument to pl330_filter. Because I also need > to compare against the dma controller device node, as my requested > channel can belong to any of the available dma controller on SoC. So > I either need to pass pointer to dma_spec as void* argument which > holds the dma controller node and required channel number or I can > pass pointer to dma_pl330_filter_args as per your dw_dmac patches. Right. > If I pass pointer to dma_spec I can have a check like below in my > filter function > return ((chan->private == dma_spec->np) && (chan->chan_id == dma_spec->args[0])) > > Or if I pass dma_pl330_filter_args I can have a check like below. > return ((chan->device == &fargs->pdmac->ddma) && (chan->chan_id == > fargs->chan_id)); > > I modified the pl330_filter function based on your dw_dmac patches. > Indeed I don’t need to pass pointer to pdmac object as 3rd arg in > of_dma_controller_register . Even I pass NULL here works for me. > Can I pass NULL here as the third argument in of_dma_controller_register? These are all not the issues I am referring to in my comment above. I think it works either way, even if you pass NULL to of_dma_controller_register, although using it for the pdmac object seems cleaner to me. > Please clarify me which is best way of doing this and correct me if my > understanding is wrong. My point was that in the samsung_dmadev_request quoted above, you refer to the same pl330_filter filter function, but the argument there is a pointer to 'enum dma_ch', which is not compatible with any of the methods you list, neither the dma_pl330_filter_args nor the raw property. Also, if you change the calling conventions for the pl330_filter function, you should change both the caller and the function in the same patch. Arnd
Hi Arnd, On Tue, Feb 5, 2013 at 4:43 PM, Arnd Bergmann <arnd@arndb.de> wrote: > On Tuesday 05 February 2013, Padma Venkat wrote: >> On Mon, Feb 4, 2013 at 11:13 PM, Arnd Bergmann <arnd@arndb.de> wrote: >> > On Monday 04 February 2013, Padmavathi Venna wrote: >> >> diff --git a/arch/arm/plat-samsung/dma-ops.c b/arch/arm/plat-samsung/dma-ops.c >> >> index 71d58dd..ec0d731 100644 >> >> --- a/arch/arm/plat-samsung/dma-ops.c >> >> +++ b/arch/arm/plat-samsung/dma-ops.c >> >> @@ -23,23 +23,15 @@ static unsigned samsung_dmadev_request(enum dma_ch dma_ch, >> >> struct device *dev, char *ch_name) >> >> { >> >> dma_cap_mask_t mask; >> >> - void *filter_param; >> >> >> >> dma_cap_zero(mask); >> >> dma_cap_set(param->cap, mask); >> >> >> >> - /* >> >> - * If a dma channel property of a device node from device tree is >> >> - * specified, use that as the fliter parameter. >> >> - */ >> >> - filter_param = (dma_ch == DMACH_DT_PROP) ? >> >> - (void *)param->dt_dmach_prop : (void *)dma_ch; >> >> - >> >> if (dev->of_node) >> >> return (unsigned)dma_request_slave_channel(dev, ch_name); >> >> else >> >> return (unsigned)dma_request_channel(mask, pl330_filter, >> >> - filter_param); >> >> + (void *)dma_ch); >> >> } >> > >> > This still looks wrong to me, because the pl330_filter function now tkes >> > a struct dma_pl330_filter_args pointer argument, not dma_ch name. >> >> Below is my understanding about generic dma and our discussion on >> previous versions of my patches. >> >> I can’t pass single dma channel number(may be not dma_ch name in your >> comment above) as void* argument to pl330_filter. Because I also need >> to compare against the dma controller device node, as my requested >> channel can belong to any of the available dma controller on SoC. So >> I either need to pass pointer to dma_spec as void* argument which >> holds the dma controller node and required channel number or I can >> pass pointer to dma_pl330_filter_args as per your dw_dmac patches. > > Right. > >> If I pass pointer to dma_spec I can have a check like below in my >> filter function >> return ((chan->private == dma_spec->np) && (chan->chan_id == dma_spec->args[0])) >> >> Or if I pass dma_pl330_filter_args I can have a check like below. >> return ((chan->device == &fargs->pdmac->ddma) && (chan->chan_id == >> fargs->chan_id)); >> >> I modified the pl330_filter function based on your dw_dmac patches. >> Indeed I don’t need to pass pointer to pdmac object as 3rd arg in >> of_dma_controller_register . Even I pass NULL here works for me. >> Can I pass NULL here as the third argument in of_dma_controller_register? > > These are all not the issues I am referring to in my comment above. > I think it works either way, even if you pass NULL to > of_dma_controller_register, although using it for the pdmac object > seems cleaner to me. > >> Please clarify me which is best way of doing this and correct me if my >> understanding is wrong. > > My point was that in the samsung_dmadev_request quoted above, you > refer to the same pl330_filter filter function, but the argument there > is a pointer to 'enum dma_ch', which is not compatible with any of > the methods you list, neither the dma_pl330_filter_args nor the > raw property. > > Also, if you change the calling conventions for the pl330_filter > function, you should change both the caller and the function in the > same patch. 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 :( . > > Arnd > -- > To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html Thanks Padma
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
diff --git a/arch/arm/mach-s3c24xx/include/mach/dma.h b/arch/arm/mach-s3c24xx/include/mach/dma.h index 6b72d5a..b55da1d 100644 --- a/arch/arm/mach-s3c24xx/include/mach/dma.h +++ b/arch/arm/mach-s3c24xx/include/mach/dma.h @@ -24,7 +24,6 @@ */ enum dma_ch { - DMACH_DT_PROP = -1, /* not yet supported, do not use */ DMACH_XD0 = 0, DMACH_XD1, DMACH_SDI, diff --git a/arch/arm/mach-s3c64xx/include/mach/dma.h b/arch/arm/mach-s3c64xx/include/mach/dma.h index 57b1ff4..fe1a98c 100644 --- a/arch/arm/mach-s3c64xx/include/mach/dma.h +++ b/arch/arm/mach-s3c64xx/include/mach/dma.h @@ -21,7 +21,6 @@ */ enum dma_ch { /* DMA0/SDMA0 */ - DMACH_DT_PROP = -1, /* not yet supported, do not use */ DMACH_UART0 = 0, DMACH_UART0_SRC2, DMACH_UART1, diff --git a/arch/arm/plat-samsung/dma-ops.c b/arch/arm/plat-samsung/dma-ops.c index 71d58dd..ec0d731 100644 --- a/arch/arm/plat-samsung/dma-ops.c +++ b/arch/arm/plat-samsung/dma-ops.c @@ -23,23 +23,15 @@ static unsigned samsung_dmadev_request(enum dma_ch dma_ch, struct device *dev, char *ch_name) { dma_cap_mask_t mask; - void *filter_param; dma_cap_zero(mask); dma_cap_set(param->cap, mask); - /* - * If a dma channel property of a device node from device tree is - * specified, use that as the fliter parameter. - */ - filter_param = (dma_ch == DMACH_DT_PROP) ? - (void *)param->dt_dmach_prop : (void *)dma_ch; - if (dev->of_node) return (unsigned)dma_request_slave_channel(dev, ch_name); else return (unsigned)dma_request_channel(mask, pl330_filter, - filter_param); + (void *)dma_ch); } static int samsung_dmadev_release(unsigned ch, void *param) diff --git a/arch/arm/plat-samsung/include/plat/dma-ops.h b/arch/arm/plat-samsung/include/plat/dma-ops.h index 1141782..ce6d763 100644 --- a/arch/arm/plat-samsung/include/plat/dma-ops.h +++ b/arch/arm/plat-samsung/include/plat/dma-ops.h @@ -18,7 +18,6 @@ struct samsung_dma_req { enum dma_transaction_type cap; - struct property *dt_dmach_prop; struct s3c2410_dma_client *client; }; diff --git a/arch/arm/plat-samsung/include/plat/dma-pl330.h b/arch/arm/plat-samsung/include/plat/dma-pl330.h index d384a80..abe07fa 100644 --- a/arch/arm/plat-samsung/include/plat/dma-pl330.h +++ b/arch/arm/plat-samsung/include/plat/dma-pl330.h @@ -21,7 +21,6 @@ * use these just as IDs. */ enum dma_ch { - DMACH_DT_PROP = -1, DMACH_UART0_RX = 0, DMACH_UART0_TX, DMACH_UART1_RX,
This patch removes the usage of DMACH_DT_PROP and dt_dmach_prop from dma code as the new generic dma dt binding support has been added. Signed-off-by: Padmavathi Venna <padma.v@samsung.com> --- The functionality of this patch is dependent on following patches in the link. http://www.mail-archive.com/linux-samsung-soc@vger.kernel.org/msg15402.html This patch is made based on Mark Brown next branch. arch/arm/mach-s3c24xx/include/mach/dma.h | 1 - arch/arm/mach-s3c64xx/include/mach/dma.h | 1 - arch/arm/plat-samsung/dma-ops.c | 10 +--------- arch/arm/plat-samsung/include/plat/dma-ops.h | 1 - arch/arm/plat-samsung/include/plat/dma-pl330.h | 1 - 5 files changed, 1 insertions(+), 13 deletions(-)