Re: [PATCH 1/2] arm: ixp4xx: set cohorent_dma_mask for ethernetplatform devices

[Ben Hutchings]

On Sun, 2013-07-21 at 16:45 +0200, Krzysztof Halasa wrote:
> Jonas Gorski <jogo@xxxxxxxxxxx> writes:
> 
> > ARM requires the cohorent_dma_mask set, so set it for the platform
> > devices so that the ethernet driver has access to it.
> 
> I recognize the need to fix this issue and I appreciate your efforts,
> but... I think this patch tries to make the driver functional again at
> all costs and this a very bad idea. The IXP4xx Ethernet MACs are not
> normal platform devices, they are in fact built-in CPU resources.

Sounds like a normal platform device to me...

> The
> platform device structs are only used to set parameters. What the patch
> does is unneeded and IMHO harmful code duplication. It makes completely
> no sense to set DMA masks in code for individual platforms as it's not
> something platforms can decide, or *even should know of*. It's simply
> a CPU attribute, a value that is shared by all IXP4xx CPUs and thus all
> platforms and systems using it.
> 
> This is against the "line of code" count rules (or "rules").
> 
> Also the dev->dev.parent is IMHO a bad idea. The queue numbers and MAC
> addresses are in no way "parents" of Ethernet controllers,

Well, those are the platform data.

> and even if
> they somehow were, I find it rather hard to believe they can have DMA
> masks.

I think the problem is that the platform device and the platform data
have not been properly separated.  The machine-specific setup functions
should be passing the eth_plat_info and MAC ID into a common ixp4xx
function which would then create the platform device with the correct
DMA mask etc.

Ben.

> I think the previous patch (which sets the masks in one place, in
> Ethernet driver code) was better, though not perfect.
> 
> My fault is I haven't fixed it yet. Will try to invent something.

-- 
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/