Re: [PATCHv4] dmaengine: Add support for BCM2835

[Tomasz Figa <tomasz.figa-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>]

On Thursday 14 of November 2013 16:41:56 Florian Meier wrote:
> 2013/11/14 Tomasz Figa <tomasz.figa-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>:
> > On Thursday 14 of November 2013 15:44:05 Florian Meier wrote:
> >> 2013/11/14 Tomasz Figa <tomasz.figa-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>:
> >> > On Thursday 14 of November 2013 08:12:46 Florian Meier wrote:
> >> >> On 13.11.2013 21:39, Tomasz Figa wrote:
> >> >> > On Wednesday 13 of November 2013 20:35:22 Florian Meier wrote:
> >> >> >>>> diff --git a/drivers/dma/bcm2835-dma.c b/drivers/dma/bcm2835-dma.c
> >> >> >>>> new file mode 100644
> >> >> >>>> index 0000000..baf072e
> >> >> >>>> --- /dev/null
> >> >> >>>> +++ b/drivers/dma/bcm2835-dma.c
> >> >> >>> [snip]
> >> >> >>>> +static int bcm2835_dma_probe(struct platform_device *pdev)
> >> >> >>>> +{
> >> >> >>>> +        struct bcm2835_dmadev *od;
> >> >> >>>> +        struct resource *dma_res = NULL;
> >> >> >>>> +        void __iomem *dma_base = NULL;
> >> >> >>>> +        int rc = 0;
> >> >> >>>> +        int i = 0;
> >> >> >>>> +        int irq;
> >> >> >>>> +        uint32_t chans_available;
> >> >> >>> [snip]
> >> >> >>>> +        if (pdev->dev.of_node) {
> >> >> >>>
> >> >> >>> Is this driver supposed to support non-DT based instantation (aka board
> >> >> >>> files)? If not, maybe it would be cleaner to simply check for
> >> >> >>> !pdev->dev.of_node at the beginning of probe and return an error?
> >> >> >>
> >> >> >> I would like to maintain the possibility for board file based
> >> >> >> instatiation, because the Raspberry Pi downstream kernel still doesn't
> >> >> >> support device tree. If this is a no-go, I will accept that.
> >> >> >
> >> >> > Sure, you are free to do so.
> >> >> >
> >> >> > What I meant is that your probe won't call bcm2835_dma_chan_init() at all
> >> >> > if there is no pdev->dev.of_node, because the loop iterating over channels
> >> >> > is under the if clause.
> >> >>
> >> >> Yes you are right, but I think it will make the patching easier, later.
> >> >> Currently, nothing bad happens without device tree - it just allocates
> >> >> no channels.
> >> >
> >> > But isn't it really an error condition, if no channels are allocated?
> >>
> >> A fridge is still a working fridge, even if no beer is inside ;-)
> >> Ok, bad example, but you will get an error message anyway when you try
> >> to get a channel.
> >>
> >> > Anyway, back to my point about leaving non-DT support in a driver, the
> >> > point is still valid only for drivers, not for platforms/boards. So if
> >> > there are no boards supported using board files in mainline that could
> >> > benefit from this driver, then this driver can be safely made DT-only,
> >> > because no new non-DT platforms/boards can be added.
> >>
> >> I don't have a telling argument against this, but just thought writing
> >> it this way will
> >> make the migration of the downstream kernel to upstream easier, but if you say I
> >> should change it, I will of course do that.
> >
> > I'm just presenting you the possible options. You are still free to have
> > non-DT support in the driver, but if you don't need it (because you can't
> > have any new non-DT platforms in mainline) then you can simplify some
> > code.
> >
> > However the driver shouldn't be left with illusionary support for non-DT
> > platforms until you decide to implement that. Instead, if you don't want
> > to add non-DT support now, just make the driver DT-only, while keeping
> > its design in a way allowing you to add non-DT support in future.
> 
> This statement hits the nail on the head. Thank you!
> 
> > In other words, a driver should not be able to probe using board files
> > if support for such probing method is not available in it yet.
> 
> That is meaningful. So would the following be ok?
> 
> + .....
> +       if (pdev->dev.of_node) {
> + .....
> +       } else {
> +               dev_err(&pdev->dev,
> +                       "Failed to initialize channels, because device
> tree not available: %d\n", rc);
> +               bcm2835_dma_free(od);
> +               return rc;
> +      }
> +
> +       dev_dbg(&pdev->dev, "Initialized %i DMA channels\n", i);
> +
> + .....
> 

I mean, something closer to:

static int bcm2835_dma_probe(struct platform_device *pdev)
{
	uint32_t chan_mask;
	// ...

	if (pdev->dev.of_node) {
		rc = of_property_read_u32(..., &chan_mask);
		if (rc) {
			// Error out
		}
	} else {
		// Error message
		return -EINVAL;
	}

	// ...
	// Channel registration loop (independent of any OF functions)
	// ...

	if (pdev->dev.of_node) {
		rc = of_dma_controller_register(...);
		// ...
	}

	// ...
}

> >> I am becoming desperate anyway that this migration will ever fully
> >> take place....
> >
> > Why not? It's just a matter of people like you working on this (and
> > addressing some review comments ;)).
> 
> The most common comment about this is that people will not put effort in
> the upstream kernel as long as there is no comfortable way for debugging
> in the upstream kernel (i.e. at least USB support).......

Right, you need USB to have ethernet working. Still, IMHO UART (for
console) + ethernet (for file transfer or NFS root) is the reasonable
setup allowing you to debug further drivers in a comfortable way.

Best regards,
Tomasz

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html