Re: [RFC 7/8] TILER-DMM: Main TILER driver implementation.

[Russell King - ARM Linux <linux@arm.linux.org.uk>]

On Fri, Jul 23, 2010 at 06:22:27PM -0500, David Sin wrote:
> +struct platform_driver tiler_driver_ldm = {
> +	.driver = {
> +		.owner = THIS_MODULE,
> +		.name = "tiler",
> +	},
> +	.probe = NULL,
> +	.shutdown = NULL,
> +	.remove = NULL,
> +};

What's the purpose of this apparantly stub driver?

> +static s32 refill_pat(struct tmm *tmm, struct tcm_area *area, u32 *ptr)
> +{
> +	s32 res = 0;
> +	struct pat_area p_area = {0};
> +
> +	p_area.x0 = area->p0.x;
> +	p_area.y0 = area->p0.y;
> +	p_area.x1 = area->p1.x;
> +	p_area.y1 = area->p1.y;
> +
> +	memcpy(dmac_va, ptr, sizeof(*ptr) * tcm_sizeof(*area));
> +
> +	if (tmm_map(tmm, p_area, dmac_pa))
> +		res = -EFAULT;

What's wrong with making tmm_map return an error code and propagating it?
This can be a simple:

	return tmm_map(tmm, p_area, dmac_pa);

In any case, I'm not sure I like all this layered code - it makes it much
harder to follow what's going on when you have to look at multiple files
to find out what X does.

If tmm_map() is only used here, move the contents of tmm_map here.

> +/* verify input params and calculate tiler container params for a block */
> +static s32 __analize_area(enum tiler_fmt fmt, u32 width, u32 height,
> +			  u16 *x_area, u16 *y_area, u16 *align, u16 *offs)

Analyze is spelt like that, not with an i.

> +/* driver initialization */
> +static s32 __init tiler_init(void)
> +{
> +	s32 r = -1;
> +	struct tcm *sita = NULL;
> +	struct tmm *tmm_pat = NULL;
> +
> +	tiler_geom_init(&tiler);
> +
> +	/* check module parameters for correctness */
> +	if (default_align > PAGE_SIZE ||
> +	    default_align & (default_align - 1) ||
> +	    granularity < 1 || granularity > PAGE_SIZE ||
> +	    granularity & (granularity - 1))
> +		return -EINVAL;
> +
> +	/*
> +	 * Array of physical pages for PAT programming, which must be a 16-byte
> +	 * aligned physical address.
> +	 */
> +	dmac_va = dma_alloc_coherent(NULL, tiler.width * tiler.height *
> +					sizeof(*dmac_va), &dmac_pa, GFP_ATOMIC);

Why GFP_ATOMIC?  Surely GFP_KERNEL will work here?

What if there ends up being more than one tiler at some point in the future
and you need two of these?  Shouldn't this be in some driver probe function
so you have a struct device to use with it?