Hi Sascha,
I've had a brief look around the driver. It looks reasonable in general,
but the division into subdrivers feels a bit ad-hoc. If all the components
are built into a single module, I don't see the need for separating the
data by functional units by file. It seems simple enough to turn this
into a layered driver with multiple sub-devices each derived from a
platform_device on its own.
On Monday 28 February 2011, Sascha Hauer wrote:
> arch/arm/plat-mxc/include/mach/ipu-v3.h | 49 +++
> drivers/video/Kconfig | 2 +
> drivers/video/Makefile | 1 +
> drivers/video/imx-ipu-v3/Kconfig | 10 +
> drivers/video/imx-ipu-v3/Makefile | 3 +
> drivers/video/imx-ipu-v3/ipu-common.c | 666 +++++++++++++++++++++++++++++++
> drivers/video/imx-ipu-v3/ipu-cpmem.c | 612 ++++++++++++++++++++++++++++
> drivers/video/imx-ipu-v3/ipu-dc.c | 364 +++++++++++++++++
> drivers/video/imx-ipu-v3/ipu-di.c | 550 +++++++++++++++++++++++++
> drivers/video/imx-ipu-v3/ipu-dmfc.c | 355 ++++++++++++++++
> drivers/video/imx-ipu-v3/ipu-dp.c | 476 ++++++++++++++++++++++
> drivers/video/imx-ipu-v3/ipu-prv.h | 216 ++++++++++
> include/video/imx-ipu-v3.h | 219 ++++++++++
I wonder if this is something that would fit better in drivers/gpu instead
of drivers/video. We recently discussed the benefits of KMS vs fb drivers,
and I think it would be good to be prepared for making this a KMS driver
in the long run, even if the immediate target has to be a simple frame buffer
driver.
> +#include "ipu-prv.h"
> +
> +static struct clk *ipu_clk;
> +static struct device *ipu_dev;
> +
> +static DEFINE_SPINLOCK(ipu_lock);
> +static DEFINE_MUTEX(ipu_channel_lock);
> +void __iomem *ipu_cm_reg;
> +void __iomem *ipu_idmac_reg;
> +
> +static int ipu_use_count;
> +
> +static struct ipu_channel channels[64];
Keeping these global limits you to just one ipu, and makes
understanding the code a little harder. How about moving
these variables into a struct ipu_data (or similar) that
is referenced from the platform_device?
> +EXPORT_SYMBOL(ipu_idmac_put);
Why not EXPORT_SYMBOL_GPL?
> +static LIST_HEAD(ipu_irq_handlers);
> +
> +static void ipu_irq_update_irq_mask(void)
> +{
> + struct ipu_irq_handler *handler;
> + int i;
> +
> + DECLARE_IPU_IRQ_BITMAP(irqs);
> +
> + bitmap_zero(irqs, IPU_IRQ_COUNT);
> +
> + list_for_each_entry(handler, &ipu_irq_handlers, list)
> + bitmap_or(irqs, irqs, handler->ipu_irqs, IPU_IRQ_COUNT);
> +
> + for (i = 0; i < BITS_TO_LONGS(IPU_IRQ_COUNT); i++)
> + ipu_cm_write(irqs[i], IPU_INT_CTRL(i + 1));
> +}
> +
> +int ipu_irq_add_handler(struct ipu_irq_handler *ipuirq)
> +{
> + unsigned long flags;
> +
> + spin_lock_irqsave(&ipu_lock, flags);
> +
> + list_add_tail(&ipuirq->list, &ipu_irq_handlers);
> + ipu_irq_update_irq_mask();
> +
> + spin_unlock_irqrestore(&ipu_lock, flags);
> + return 0;
> +}
> +EXPORT_SYMBOL(ipu_irq_add_handler);
The interrupt logic needs some comments. What are you trying to achieve here?
> +int ipu_wait_for_interrupt(int interrupt, int timeout_ms)
> +{
> + struct ipu_irq_handler handler;
> + DECLARE_COMPLETION_ONSTACK(completion);
> + int ret;
> +
> + bitmap_zero(handler.ipu_irqs, IPU_IRQ_COUNT);
> + bitmap_set(handler.ipu_irqs, interrupt, 1);
> +
> + ipu_cm_write(1 << (interrupt % 32), IPU_INT_STAT(interrupt / 32 + 1));
> +
> + handler.handler = ipu_completion_handler;
> + handler.context = &completion;
> + ipu_irq_add_handler(&handler);
> +
> + ret = wait_for_completion_timeout(&completion,
> + msecs_to_jiffies(timeout_ms));
> +
> + ipu_irq_remove_handler(&handler);
> +
> + if (ret > 0)
> + ret = 0;
> + else
> + ret = -ETIMEDOUT;
> +
> + return ret;
> +}
> +EXPORT_SYMBOL(ipu_wait_for_interrupt);
If I understand this correctly, this is a very complicated way to implement
something that could be done with a single wait_event_timeout() and
wake_up_interruptible_all() from the irq handler.
> +static irqreturn_t ipu_irq_handler(int irq, void *desc)
> +{
> + DECLARE_IPU_IRQ_BITMAP(status);
> + struct ipu_irq_handler *handler;
> + int i;
> +
> + for (i = 0; i < BITS_TO_LONGS(IPU_IRQ_COUNT); i++) {
> + status[i] = ipu_cm_read(IPU_INT_STAT(i + 1));
> + ipu_cm_write(status[i], IPU_INT_STAT(i + 1));
> + }
> +
> + list_for_each_entry(handler, &ipu_irq_handlers, list) {
> + DECLARE_IPU_IRQ_BITMAP(tmp);
> + if (bitmap_and(tmp, status, handler->ipu_irqs, IPU_IRQ_COUNT))
> + handler->handler(tmp, handler->context);
> + }
> +
> + return IRQ_HANDLED;
> +}
This needs to take spin_lock_irqsave before walking the ipu_irq_handlers
list, in order to prevent another CPU from modifying the list
while you are in the handler.
> +static int ipu_reset(void)
> +{
> + int timeout = 10000;
> + u32 val;
> +
> + /* hard reset the IPU */
> + val = readl(MX51_IO_ADDRESS(MX51_SRC_BASE_ADDR));
> + val |= 1 << 3;
> + writel(val, MX51_IO_ADDRESS(MX51_SRC_BASE_ADDR));
> +
> + ipu_cm_write(0x807FFFFF, IPU_MEM_RST);
> +
> + while (ipu_cm_read(IPU_MEM_RST) & 0x80000000) {
> + if (!timeout--)
> + return -ETIME;
> + udelay(100);
> + }
You have a timeout of over one second with udelay, which
is not acceptable on many systems. AFAICT, the function
can sleep, so you can replace udelay with msleep(1), and
you should use a better logic to determine the end of the
loop:
unsigned long timeout = jiffies + msecs_to_jiffies(1000);
while (ipu_cm_read(IPU_MEM_RST) & 0x80000000) {
if (time_after(jiffies, timeout))
return -ETIME;
msleep(1);
}
> +static u32 *ipu_cpmem_base;
> +static struct device *ipu_dev;
> +
> +struct ipu_ch_param_word {
> + u32 data[5];
> + u32 res[3];
> +};
> +
> +struct ipu_ch_param {
> + struct ipu_ch_param_word word[2];
> +};
Same comment as for the previous file
> +
> +static void __iomem *ipu_dc_reg;
> +static void __iomem *ipu_dc_tmpl_reg;
> +static struct device *ipu_dev;
> +
> +struct ipu_dc {
> + unsigned int di; /* The display interface number assigned to this dc channel */
> + unsigned int channel_offset;
> +};
> +
> +static struct ipu_dc dc_channels[10];
And here again.
> +static void ipu_dc_link_event(int chan, int event, int addr, int priority)
> +{
> + u32 reg;
> +
> + reg = __raw_readl(DC_RL_CH(chan, event));
> + reg &= ~(0xFFFF << (16 * (event & 0x1)));
> + reg |= ((addr << 8) | priority) << (16 * (event & 0x1));
> + __raw_writel(reg, DC_RL_CH(chan, event));
> +}
Better use readl/writel instead of __raw_readl/__raw_writel, throughout the
code.
> +int ipu_di_init(struct platform_device *pdev, int id, unsigned long base,
> + u32 module, struct clk *ipu_clk);
> +void ipu_di_exit(struct platform_device *pdev, int id);
> +
> +int ipu_dmfc_init(struct platform_device *pdev, unsigned long base,
> + struct clk *ipu_clk);
> +void ipu_dmfc_exit(struct platform_device *pdev);
> +
> +int ipu_dp_init(struct platform_device *pdev, unsigned long base);
> +void ipu_dp_exit(struct platform_device *pdev);
> +
> +int ipu_dc_init(struct platform_device *pdev, unsigned long base,
> + unsigned long template_base);
> +void ipu_dc_exit(struct platform_device *pdev);
> +
> +int ipu_cpmem_init(struct platform_device *pdev, unsigned long base);
> +void ipu_cpmem_exit(struct platform_device *pdev);
If you make the main driver an mfd device, the sub-drivers could become
real platform drivers, which can structure the layering in a more modular
way.
E.g. instead of a single module init function, each subdriver can be
a module by itself and look at the resources associated with the
platform device it matches.
Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
