On Thu, 2016-09-15 at 16:14 +0300, Eugeniy Paltsev wrote: > This patch is to address a proposal by Andy in this thread: > http://www.spinics.net/lists/dmaengine/msg10754.html > Split platform data to actual hardware properties, and platform > quirks. > Now we able to use quirks and hardware properties separately from > different sources (pdata, device tree or autoconfig registers) > My comments below. Sorry for delayed answer. > Signed-off-by: Eugeniy Paltsev <Eugeniy.Paltsev at synopsys.com> > --- > ?drivers/dma/dw/core.c????????????????| 31 +++++++++++++++----------- > ?drivers/dma/dw/platform.c????????????| 42 +++++++++++++++++++++---- > ----------- > ?include/linux/platform_data/dma-dw.h | 20 +++++++++++------ > ?3 files changed, 57 insertions(+), 36 deletions(-) > > diff --git a/drivers/dma/dw/core.c b/drivers/dma/dw/core.c > index c2c0a61..9352735 100644 > --- a/drivers/dma/dw/core.c > +++ b/drivers/dma/dw/core.c > @@ -1451,10 +1451,25 @@ int dw_dma_probe(struct dw_dma_chip *chip) > ? > ? dw->regs = chip->regs; > ? chip->dw = dw; + empty line. > + /* Reassign the platform data pointer */ > + pdata = dw->pdata; > ? > ? pm_runtime_get_sync(chip->dev); > ? > - if (!chip->pdata) { > + if ((!chip->pdata) || > + ???(chip->pdata && test_bit(QUIRKS_ONLY_USED, &chip->pdata- I don't think you need atomic test / set of those bits. > >quirks))) { > + > + /* > + ?* Fill quirks with the default values in case of > pdata absence /* * Multi-line comments should include full sentences * (punctuation matters). */ > + ?*/ > + if (!chip->pdata) { > + set_bit(QUIRKS_IS_PRIVATE, &pdata->quirks); > + set_bit(QUIRKS_IS_MEMCPY, &pdata->quirks); > + set_bit(QUIRKS_IS_NOLLP, &pdata->quirks); > + } else { > + pdata->quirks = chip->pdata->quirks; > + } > + > ? dw_params = dma_readl(dw, DW_PARAMS); > ? dev_dbg(chip->dev, "DW_PARAMS: 0x%08x\n", dw_params); > ? > @@ -1464,9 +1479,6 @@ int dw_dma_probe(struct dw_dma_chip *chip) > ? goto err_pdata; > ? } > ? > - /* Reassign the platform data pointer */ > - pdata = dw->pdata; > - > ? /* Get hardware configuration parameters */ > ? pdata->nr_channels = (dw_params >> DW_PARAMS_NR_CHAN > & 7) + 1; > ? pdata->nr_masters = (dw_params >> DW_PARAMS_NR_MASTER > & 3) + 1; > @@ -1477,8 +1489,6 @@ int dw_dma_probe(struct dw_dma_chip *chip) > ? pdata->block_size = dma_readl(dw, MAX_BLK_SIZE); > ? > ? /* Fill platform data with the default values */ > - pdata->is_private = true; > - pdata->is_memcpy = true; > ? pdata->chan_allocation_order = > CHAN_ALLOCATION_ASCENDING; > ? pdata->chan_priority = CHAN_PRIORITY_ASCENDING; > ? } else if (chip->pdata->nr_channels > DW_DMA_MAX_NR_CHANNELS) > { > @@ -1486,9 +1496,6 @@ int dw_dma_probe(struct dw_dma_chip *chip) > ? goto err_pdata; > ? } else { > ? memcpy(dw->pdata, chip->pdata, sizeof(*dw->pdata)); > - > - /* Reassign the platform data pointer */ > - pdata = dw->pdata; > ? } > ? > ? dw->chan = devm_kcalloc(chip->dev, pdata->nr_channels, > sizeof(*dw->chan), > @@ -1569,7 +1576,7 @@ int dw_dma_probe(struct dw_dma_chip *chip) > ? (dwc_params >> DWC_PARAMS_MBLK_EN & > 0x1) == 0; > ? } else { > ? dwc->block_size = pdata->block_size; > - dwc->nollp = pdata->is_nollp; > + dwc->nollp = test_bit(QUIRKS_IS_NOLLP, > &pdata->quirks); Perhaps you need another patch which actually moves nollp to dwc->flags. > ? } > ? } > ? > @@ -1582,9 +1589,9 @@ int dw_dma_probe(struct dw_dma_chip *chip) > ? > ? /* Set capabilities */ > ? dma_cap_set(DMA_SLAVE, dw->dma.cap_mask); > - if (pdata->is_private) > + if (test_bit(QUIRKS_IS_PRIVATE, &pdata->quirks)) > ? dma_cap_set(DMA_PRIVATE, dw->dma.cap_mask); > - if (pdata->is_memcpy) > + if (test_bit(QUIRKS_IS_MEMCPY, &pdata->quirks)) > ? dma_cap_set(DMA_MEMCPY, dw->dma.cap_mask); > ? > ? dw->dma.dev = chip->dev; > diff --git a/drivers/dma/dw/platform.c b/drivers/dma/dw/platform.c > index 5bda0eb..308b977 100644 > --- a/drivers/dma/dw/platform.c > +++ b/drivers/dma/dw/platform.c > @@ -12,6 +12,7 @@ > ? * published by the Free Software Foundation. > ? */ > ? > +#include <linux/bitops.h> > ?#include <linux/module.h> > ?#include <linux/device.h> > ?#include <linux/clk.h> > @@ -111,41 +112,48 @@ dw_dma_parse_dt(struct platform_device *pdev) > ? return NULL; > ? } > ? > - if (of_property_read_u32(np, "dma-masters", &nr_masters)) > - return NULL; > - if (nr_masters < 1 || nr_masters > DW_DMA_MAX_NR_MASTERS) > - return NULL; > - > - if (of_property_read_u32(np, "dma-channels", &nr_channels)) > - return NULL; > - > ? pdata = devm_kzalloc(&pdev->dev, sizeof(*pdata), GFP_KERNEL); > ? if (!pdata) > ? return NULL; > ? > + set_bit(QUIRKS_ONLY_USED, &pdata->quirks); > + > + if (of_property_read_bool(np, "is-private")) > + set_bit(QUIRKS_IS_PRIVATE, &pdata->quirks); > + > + if (of_property_read_bool(np, "is-memcpy")) > + set_bit(QUIRKS_IS_MEMCPY, &pdata->quirks); > + > + if (of_property_read_bool(np, "is-nollp")) > + set_bit(QUIRKS_IS_NOLLP, &pdata->quirks); I would suggest to convert to unified device property API first. > + > + if (of_property_read_u32(np, "dma-masters", &nr_masters)) > + return pdata; > + if (nr_masters < 1 || nr_masters > DW_DMA_MAX_NR_MASTERS) > + return pdata; > + > ? pdata->nr_masters = nr_masters; > - pdata->nr_channels = nr_channels; > ? > - if (of_property_read_bool(np, "is_private")) > - pdata->is_private = true; > + if (of_property_read_u32(np, "dma-channels", &nr_channels)) > + return pdata; > ? > - if (!of_property_read_u32(np, "chan_allocation_order", &tmp)) > + pdata->nr_channels = nr_channels; > + > + if (!of_property_read_u32(np, "chan-allocation-order", &tmp)) > ? pdata->chan_allocation_order = (unsigned char)tmp; > ? > - if (!of_property_read_u32(np, "chan_priority", &tmp)) > + if (!of_property_read_u32(np, "chan-priority", &tmp)) > ? pdata->chan_priority = tmp; > ? > - if (!of_property_read_u32(np, "block_size", &tmp)) > + if (!of_property_read_u32(np, "block-size", &tmp)) > ? pdata->block_size = tmp; > ? > ? if (!of_property_read_u32_array(np, "data-width", arr, > nr_masters)) { > ? for (tmp = 0; tmp < nr_masters; tmp++) > ? pdata->data_width[tmp] = arr[tmp]; > - } else if (!of_property_read_u32_array(np, "data_width", arr, > nr_masters)) { > - for (tmp = 0; tmp < nr_masters; tmp++) > - pdata->data_width[tmp] = BIT(arr[tmp] & > 0x07); > ? } > ? > + clear_bit(QUIRKS_ONLY_USED, &pdata->quirks); > ? return pdata; > ?} > ?#else > diff --git a/include/linux/platform_data/dma-dw.h > b/include/linux/platform_data/dma-dw.h > index 5f0e11e..9cd8199 100644 > --- a/include/linux/platform_data/dma-dw.h > +++ b/include/linux/platform_data/dma-dw.h > @@ -37,10 +37,7 @@ struct dw_dma_slave { > ?/** > ? * struct dw_dma_platform_data - Controller configuration parameters > ? * @nr_channels: Number of channels supported by hardware (max 8) > - * @is_private: The device channels should be marked as private and > not for > - * by the general purpose DMA channel allocator. > - * @is_memcpy: The device channels do support memory-to-memory > transfers. > - * @is_nollp: The device channels does not support multi block > transfers. > + * @quirks: Bit field with platform quirks > ? * @chan_allocation_order: Allocate channels starting from 0 or 7 > ? * @chan_priority: Set channel priority increasing from 0 to 7 or 7 > to 0. > ? * @block_size: Maximum block size supported by the controller > @@ -50,9 +47,18 @@ struct dw_dma_slave { > ? */ > ?struct dw_dma_platform_data { > ? unsigned int nr_channels; > - bool is_private; > - bool is_memcpy; > - bool is_nollp; > +/* Only use quirks from platform data structure */ > +#define QUIRKS_ONLY_USED 0 > +/* > + * The device channels should be marked as private and not for > + * by the general purpose DMA channel allocator. > + */ > +#define QUIRKS_IS_PRIVATE 1 > +/* The device channels do support memory-to-memory transfers. */ > +#define QUIRKS_IS_MEMCPY 2 > +/* The device channels do not support multi block transfers. */ > +#define QUIRKS_IS_NOLLP 3 You may move descriptions to above struct description field. It will be consolidated and moreover visible in kernel-doc processed documents. > + unsigned long quirks; > ?#define CHAN_ALLOCATION_ASCENDING 0 /* zero to seven */ > ?#define CHAN_ALLOCATION_DESCENDING 1 /* seven to zero > */ > ? unsigned char chan_allocation_order; -- Andy Shevchenko <andriy.shevchenko at linux.intel.com> Intel Finland Oy
