Re: [PATCH 3/9] dm355 ccdc module for vpfe capture driver

[Kevin Hilman <khilman@deeprootsystems.com>]

"Karicheri, Muralidharan" <m-karicheri2@ti.com> writes:

> Laurent,
>
> Thanks for reviewing this. I have not gone through all of your comments, but would like to respond to the following one first. I will respond to the rest as I do the rework.
>
>>I've had a quick look at the DM355 and DM6446 datasheets. The CCDC and VPSS
>>registers share the same memory block. Can't you use a single resource ?
>>
>>One nice (and better in my opinion) solution would be to declare a
>>structure
>>with all the VPSS/CCDC registers as they are implemented in hardware and
>>access the structure fields with __raw_read/write*. You would then store a
>>single pointer only. Check arch/powerpc/include/asm/immap_cpm2.h for an
>>example.
>
> I think, a better solution will be to move the vpss system module
> part to the board specific file dm355.c or dm6446.c 

Just to clarify, the files you mention are SoC specific files, not
board-specific files...

> and export functions to configure them as needed by the different
> drivers.

My first reaction to this is... no.  I'm reluctant to have a bunch of
driver specific hooks in the core davinci SoC specific code.  I'd much
rather see this stuff kept along with the driver in drivers/media/*
and abstracted as necessary there.

> The vpss has evolved quite a lot from DM6446 to DM355 to
> DM365. Registers such as INTSEL and INTSTAT and others are
> applicable to other modules as well, not just the ccdc module. The
> VPBE and resizer drivers will need to configure them too. Also the
> vpss system module features available in DM365 is much more than
> that in DM355. 

Based on this, it sounds to me that this driver needs to be broken up
a little bit more and some of the shared pieces need to be abstracted
out so they can be shared among the modules.

> Interrupts line to ARM are programmable in DM365 vpss and multiple
> vpss irq lines can be muxed to the ARM side. So I would imaging
> functions enable/disable irq line to arm, clearing irq bits,
> enabling various clocks etc can be done in a specific soc specific
> file (dm355.c, dm365.c or dm6446.c) and can be exported for use in
> specific drivers. I just want to get your feedback so that I can
> make this change. With this change, there is no need to use
> structures for holding register offsets as you have suggested.

Kevin