On 04/19/2015 09:36 AM, Andrey Utkin wrote:
> I am starting a work on driver for techwell tw5864 media grabber&encoder.
> I am basing on tw68 driver (mentorship, advising and testing by board
> owners are appreciated). And here (and in some other
> drivers/media/pci/ drivers) I see what confuses me:
>
> tw68-core.c:
> dev->lmmio = ioremap(pci_resource_start(pci_dev, 0),
> pci_resource_len(pci_dev, 0));
> dev->bmmio = (__u8 __iomem *)dev->lmmio;
>
> tw68.h:
> #define tw_readl(reg) readl(dev->lmmio + ((reg) >> 2))
> #define tw_readb(reg) readb(dev->bmmio + (reg))
> #define tw_writel(reg, value) writel((value), dev->lmmio + ((reg) >> 2))
> #define tw_writeb(reg, value) writeb((value), dev->bmmio + (reg))
>
> I am mostly userland developer and I wouldn't expect bmmio pointer to
> contain value numerically different from lmmio after such simple
> casting.
Check the types of llmio and bbmio:
u32 __iomem *lmmio;
u8 __iomem *bmmio;
So the values of the pointers are the same, but the types are not.
So 'lmmio + 1' == 'bmmio + sizeof(u32)' == 'bbmio + 4'.
Since all the registers are defined as byte offsets relative to the start
of the memory map you cannot just do 'lmmio + reg' since that would be a
factor 4 off. Instead you have to divide by 4 to get it back in line.
Frankly, I don't think lmmio is necessary at all since readl/writel don't
need a u32 pointer at all since they use void pointers. I never noticed
that when I cleaned up the tw68 driver. Using 'void __iomem *mmio' instead
of lmmio/bmmio and dropping the shifts in the tw_ macros would work just
as well.
> But still, if this is correct, then how should I implement
> tw_{read,write}w to operate on 2 bytes (a word)? Similarly, it would
> look like this:
> #define tw_readl(reg) readl(dev->lmmio + ((reg) >> 1))
As suggested above, just use a single void __iomem *mmio pointer.
> That looks odd.
>
> In contrary, in drivers/media/pci/dm1105, we see no explicit shifting
> of register address. It uses {in,out}{b,w,l} macros, which seem to
> turn out the same {read,write}{b,w,l} (with some reservations):
> http://lxr.free-electrons.com/source/include/asm-generic/io.h#L354
>
> dm1105.c:#define dm_io_mem(reg) ((unsigned long)(&dev->io_mem[reg]))
> dm1105.c:#define dm_readb(reg) inb(dm_io_mem(reg))
> dm1105.c:#define dm_writeb(reg, value) outb((value), (dm_io_mem(reg)))
> dm1105.c:#define dm_readw(reg) inw(dm_io_mem(reg))
> dm1105.c:#define dm_writew(reg, value) outw((value), (dm_io_mem(reg)))
> dm1105.c:#define dm_readl(reg) inl(dm_io_mem(reg))
> dm1105.c:#define dm_writel(reg, value) outl((value), (dm_io_mem(reg)))
>
> This looks like contradiction to me (shifting register address vs. no
> shifting), so that one practice may be even just wrong and broken
> (which is hard to believe due to active maintenance of all drivers).
> I highly appreciate your help me in determining the best practice to
> follow in this new driver.
> Thanks.
>
Hope this helps,
Hans
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
