Re: [PATCH v2 8/9] DMA: shdma: initial of common code

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 14/04/14 23:06, Arnd Bergmann wrote:
On Monday 14 April 2014 22:35:11 Ben Dooks wrote:
Add support for building shdma internal data from the device tree to allow
converting the driver to be device tree enabled.

It includes a helper for the of case to build the internal data used to
select and filter out the DMA channels from the ID information in the
device tree. Also updates the documentation for the DT case.

Signed-off-by: Ben Dooks <ben.dooks@xxxxxxxxxxxxxxx>

I think you should try to explain better here why you need all the logic
that other drivers don't for the DT case.

I've tried to understand what you are doing with shdma_of_client and
couldn't quite figure it out. The driver is already different from
all the others, because it still uses the 'slave_id' field in
dma_slave_config (only for the non-DT case) that all other drivers
don't use at all.

My guess is that if you manage to clean that up first, all of this
wouldn't be necessary.

Does anyone else have comments on this from a Renesas perspective? We
can uniquely identify a channel on a single DMAC using the MID/RID mux
control value.

If we can unify the slave-id and the MID/RID values then the driver is
simpler and we can drop part of the update and make the entire SHDMA
system simpler.

  static struct dma_chan *shdma_of_xlate(struct of_phandle_args *dma_spec,
  				       struct of_dma *ofdma)
  {
+	struct platform_device *pdev = ofdma->of_dma_data;
  	u32 id = dma_spec->args[0];
  	dma_cap_mask_t mask;
  	struct dma_chan *chan;

-	if (dma_spec->args_count != 1)
+	if (dma_spec->args_count == 2) {
+		struct shdma_of_state *state = platform_get_drvdata(pdev);
+		struct shdma_of_client *client;
+
+		client = shdma_of_find_client(state, id);
+		if (!client) {
+			client = devm_kzalloc(&pdev->dev, sizeof(*client),
+					      GFP_KERNEL);
+			if (!client)
+				return NULL;
+
+			client->index = atomic_inc_return(&of_slave_index);
+			client->cfg.slave_id = id;
+			client->cfg.mid_rid = id;
+			client->cfg.chcr = dma_spec->args[1];

Can you explain the purpose of setting the chcr in DT? For all I can
tell, this should come from the device driver when that calls the
dma_slave_config function, unlike the mid_rid value.

See previous note about transfer-widths and register-sizes not always
matching. I also do not have any idea about other SoCs in the series
if there are any differences between channel settings.

--
Ben Dooks				http://www.codethink.co.uk/
Senior Engineer				Codethink - Providing Genius
--
To unsubscribe from this list: send the line "unsubscribe dmaengine" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html




[Index of Archives]     [Linux Kernel]     [Linux ARM (vger)]     [Linux ARM MSM]     [Linux Omap]     [Linux Arm]     [Linux Tegra]     [Fedora ARM]     [Linux for Samsung SOC]     [eCos]     [Linux PCI]     [Linux Fastboot]     [Gcc Help]     [Git]     [DCCP]     [IETF Announce]     [Security]     [Linux MIPS]     [Yosemite Campsites]

  Powered by Linux