[linux-dvb] [PATCH]: second attempt to fix 64-bit warning in saa7146_hlp.c

[Hans Verkuil]

On Monday 19 December 2005 12:24, Johannes Stezenbach wrote:
> On Mon, Dec 19, 2005, Hans Verkuil wrote:
> > OK, after receiving some comments about my fix in saa7146_hlp.c I'm
> > submitting it again (and this time to the list first, my apologies
> > for not doing that before) and this time with the comment I should
> > have added in the first place.
> >
> > I didn't just hide a warning, I actually thought about it :-)
>
> ;-)
>
> > +	/* The base pointer is a 32-bit pointer into DMA memory.
> > +	   The unsigned long cast is to remove a 64-bit compile warning
> > since +	   it looks like a 64-bit address is cast to a 32-bit
> > value. */ +	u32 base = (u32)(unsigned long)vv->ov_fb.base;
>
> For me at least, this needs more explanation. Why are these pointers
> 32bit even on a 64bit machine?
>
> If they are PCI addresses, then I believe there are machines who have
> the bus not mapped at address zero but at some offset, so you need
> 64 bits to represent a 32bit PCI address. (Also, shouldn't this be
> dma_addr_t in this case? What type does vv->ov_fb.base have?)
>
> If if is just an offset then the name is wrong.
>
> Will this value be set in a 32bit chip register?

From include/media/saa7146_vv.h:

        /* video overlay */
        struct v4l2_framebuffer         ov_fb;

From include/linux/videodev2.h:

struct v4l2_framebuffer
{
        __u32                   capability;
        __u32                   flags;
/* FIXME: in theory we should pass something like PCI device + memory
 * region + offset instead of some physical address */
        void*                   base;
        struct v4l2_pix_format  fmt;
};

So base is a pointer to PCI memory.

In saa7146_set_position() from saa7146_hlp.c the base address is used to 
set DMA addresses which are passed to saa7146_write_out_dma(). And that 
function writes them as 32-bit values to the saa7146.

It is clear that for the saa7146 the base pointer cannot be a 64-bit 
address and should in fact probably be of type dma_addr_t. However, I'm 
not sure if struct v4l2_framebuffer isn't (mis)used in other places and 
for other devices where it can really be a 64-bit pointer. I don't dare 
touch that one.

	Hans