On Mon, Feb 28, 2011 at 07:33:05PM +0100, Thomas Gleixner wrote:
> On Mon, 28 Feb 2011, Sascha Hauer wrote:
> > +
> > +static int ipu_use_count;
> > +
> > +static struct ipu_channel channels[64];
> > +
> > +struct ipu_channel *ipu_idmac_get(unsigned num)
> > +{
> > + struct ipu_channel *channel;
> > +
> > + dev_dbg(ipu_dev, "%s %d\n", __func__, num);
> > +
> > + if (num > 63)
>
> >= ARRAY_SIZE(channels) or a sensible define please
>
> > + return ERR_PTR(-ENODEV);
> > +
> > + mutex_lock(&ipu_channel_lock);
> > +
> > + channel = &channels[num];
> > +
> > + if (channel->busy) {
> > + channel = ERR_PTR(-EBUSY);
> > + goto out;
> > + }
> > +
> > + channel->busy = 1;
> > + channel->num = num;
> > +
> > +out:
> > + mutex_unlock(&ipu_channel_lock);
> > +
> > + return channel;
> > +}
> > +EXPORT_SYMBOL(ipu_idmac_get);
>
> EXPORT_SYMBOL_GPL all over the place
>
> > +void ipu_idmac_put(struct ipu_channel *channel)
> > +{
> > + dev_dbg(ipu_dev, "%s %d\n", __func__, channel->num);
>
> Do we really need this debug stuff in all these functions ?
Reading this comment I expected tons of dev_dbg in the driver. The one
you mentioned above (plus the corresponding one in ipu_idmac_get) are
indeed not particularly useful, but do you think there is still too much
debug code left?
>
> > + mutex_lock(&ipu_channel_lock);
> > +
> > + channel->busy = 0;
> > +
> > + mutex_unlock(&ipu_channel_lock);
> > +}
> > +EXPORT_SYMBOL(ipu_idmac_put);
> > +
>
> Also exported functions want a proper kerneldoc comment.
>
> > +void ipu_idmac_set_double_buffer(struct ipu_channel *channel, bool doublebuffer)
>
> > +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);
>
> Why the hell do we need this? It's a bog standard bitmap, right ?
It's defined as:
#define DECLARE_IPU_IRQ_BITMAP(name) DECLARE_BITMAP(name, IPU_IRQ_COUNT)
So yes, it's a standard bitmask. It can be used in client drivers
aswell. Where's the problem of adding a define for this so that client
drivers do not have to care about the size of the bitmap?
>
> > + 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));
> > +}
>
> > +static void ipu_completion_handler(unsigned long *bitmask, void *context)
> > +{
> > + struct completion *completion = context;
> > +
> > + complete(completion);
> > +}
> > +
> > +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;
>
> return ret > 0 ? 0 : -ETIMEDOUT;
>
> perhaps ?
ok.
>
>
> > +}
> > +EXPORT_SYMBOL(ipu_wait_for_interrupt);
> > +
> > +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);
> > + }
>
> And what protects the list walk? Just the fact that this is a UP
> machine?
Will fix.
>
> > +void ipu_put(void)
> > +{
> > + mutex_lock(&ipu_channel_lock);
> > +
> > + ipu_use_count--;
> > +
> > + if (ipu_use_count == 0)
> > + clk_disable(ipu_clk);
> > +
> > + if (ipu_use_count < 0) {
> > + dev_err(ipu_dev, "ipu use count < 0\n");
>
> This wants to be a WARN_ON(ipu_use_count < 0) so you get some
> information which code is calling this.
yes
>
> > + ipu_use_count = 0;
> > + }
> > +
> > + mutex_unlock(&ipu_channel_lock);
> > +}
>
> > +static int __devinit ipu_probe(struct platform_device *pdev)
> > +{
> > + struct resource *res;
> > + unsigned long ipu_base;
> > + int ret, irq1, irq2;
> > +
> > + /* There can be only one */
> > + if (ipu_dev)
> > + return -EBUSY;
> > +
> > + spin_lock_init(&ipu_lock);
> > +
> > + ipu_dev = &pdev->dev;
> > +
> > + irq1 = platform_get_irq(pdev, 0);
> > + irq2 = platform_get_irq(pdev, 1);
> > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > +
> > + if (!res || irq1 < 0 || irq2 < 0)
> > + return -ENODEV;
> > +
> > + ipu_base = res->start;
> > +
> > + ipu_cm_reg = ioremap(ipu_base + IPU_CM_REG_BASE, PAGE_SIZE);
> > + if (!ipu_cm_reg) {
> > + ret = -ENOMEM;
> > + goto failed_ioremap1;
> > + }
> > +
> > + ipu_idmac_reg = ioremap(ipu_base + IPU_IDMAC_REG_BASE, PAGE_SIZE);
> > + if (!ipu_idmac_reg) {
> > + ret = -ENOMEM;
> > + goto failed_ioremap2;
> > + }
> > +
> > + ipu_clk = clk_get(&pdev->dev, "ipu");
> > + if (IS_ERR(ipu_clk)) {
> > + ret = PTR_ERR(ipu_clk);
> > + dev_err(&pdev->dev, "clk_get failed with %d", ret);
> > + goto failed_clk_get;
> > + }
> > +
> > + ipu_get();
> > +
> > + ret = request_irq(irq1, ipu_irq_handler, IRQF_DISABLED, pdev->name,
> > + &pdev->dev);
>
> s/IRQF_DISABLED/0/ We run all handlers with interrupts disabled
> nowadays.
ok.
>
> > + ret = ipu_submodules_init(pdev, ipu_base, ipu_clk);
> > + if (ret)
> > + goto failed_submodules_init;
> > +
> > + /* Set sync refresh channels as high priority */
> > + ipu_idmac_write(0x18800000, IDMAC_CHA_PRI(0));
>
> Hmm, this random prio setting here is odd.
This is 1:1 from the Freescale Kernel and I never thought about it. We
can remove it and see what happens. Maybe then some day we'll learn
*why* this is done.
>
> > + ret = ipu_add_client_devices(pdev);
> > + if (ret) {
> > + dev_err(&pdev->dev, "adding client devices failed with %d\n", ret);
> > + goto failed_add_clients;
> > + }
>
> White space damage.
>
> > + ret = ipu_wait_for_interrupt(irq, 50);
> > + if (ret)
> > + return;
> > +
> > + /* Wait for DC triple buffer to empty */
> > + if (dc_channels[dc_chan].di == 0)
> > + while ((__raw_readl(DC_STAT) & 0x00000002)
> > + != 0x00000002) {
> > + msleep(2);
> > + timeout -= 2;
> > + if (timeout <= 0)
> > + break;
>
> So we poll stuff which is updated from some other function ?
We poll the DC_STAT register here which is updated from the hardware.
Sascha
--
Pengutronix e.K. | |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
--
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
