On 31/03/16 07:26, Shardar Shariff Md wrote: > Initialize default channel slave_id(req_sel) to -1 to avoid > overwriting of slave_id with client data as zero is the > valid slave_id(request_select). > > Signed-off-by: Shardar Shariff Md <[email protected]> > --- > drivers/dma/tegra20-apb-dma.c | 7 ++++--- > 1 file changed, 4 insertions(+), 3 deletions(-) > > diff --git a/drivers/dma/tegra20-apb-dma.c b/drivers/dma/tegra20-apb-dma.c > index 3871f29..35a0df0 100644 > --- a/drivers/dma/tegra20-apb-dma.c > +++ b/drivers/dma/tegra20-apb-dma.c > @@ -204,7 +204,7 @@ struct tegra_dma_channel { > struct tasklet_struct tasklet; > > /* Channel-slave specific configuration */ > - unsigned int slave_id; > + int slave_id;
Thanks for the fix. Looking at this a bit more I would prefer that we keep this a unsigned int and instead of using a negative value. My reasoning for that is if we make this a signed type, then technically we should check it is neither less than 0 or greater than the max slave_id supported. So that said, I think that we should ... 1. In tegra_dma_of_xlate() check to see if the slave_id is greater than the maximum slave_id allowed. We should define a TEGRA_APBDMA_SLAVE_ID_MAX which we should use in tegra_dma_of_xlate() to ensure that the slave_id is valid. 2. Define a TEGRA_APBDMA_SLAVE_ID_INVALID (TEGRA_APBDMA_SLAVEID_MAX + 1) and set the slave_id to this value in tegra_dma_free_chan_resources() and probe(). Then we can simply check if the slave_id is equal to this. This way we can ensure that slave_id is between 0 and the max value supported and not need to worry about negative values. > struct dma_slave_config dma_sconfig; > struct tegra_dma_channel_regs channel_reg; > }; > @@ -353,7 +353,7 @@ static int tegra_dma_slave_config(struct dma_chan *dc, > } > > memcpy(&tdc->dma_sconfig, sconfig, sizeof(*sconfig)); > - if (!tdc->slave_id) > + if (tdc->slave_id == -1) > tdc->slave_id = sconfig->slave_id; Hmmm ... I know that this is how it is today, but is this not an error condition? In other words, if the slave_id is NOT equal to -1, then should we return an error? It seems that we could silently ignore the new slave_id and if it has been already set which seems bad. > tdc->config_init = true; > return 0; > @@ -1236,7 +1236,7 @@ static void tegra_dma_free_chan_resources(struct > dma_chan *dc) > } > pm_runtime_put(tdma->dev); > > - tdc->slave_id = 0; > + tdc->slave_id = -1; > } > > static struct dma_chan *tegra_dma_of_xlate(struct of_phandle_args *dma_spec, > @@ -1389,6 +1389,7 @@ static int tegra_dma_probe(struct platform_device *pdev) > &tdma->dma_dev.channels); > tdc->tdma = tdma; > tdc->id = i; > + tdc->slave_id = -1; > > tasklet_init(&tdc->tasklet, tegra_dma_tasklet, > (unsigned long)tdc); > Cheers Jon
