> -----Original Message-----
> From: Hilman, Kevin
> Sent: Friday, March 04, 2011 10:23 PM
> To: DebBarma, Tarun Kanti
> Cc: linux-omap@xxxxxxxxxxxxxxx; Gopinath, Thara
> Subject: Re: [PATCH v11 5/8] OMAP: dmtimer: platform driver
>
> "DebBarma, Tarun Kanti" <tarun.kanti@xxxxxx> writes:
>
> >> -----Original Message-----
> >> From: Hilman, Kevin
> >> Sent: Friday, March 04, 2011 6:59 AM
> >> To: DebBarma, Tarun Kanti
> >> Cc: linux-omap@xxxxxxxxxxxxxxx; Gopinath, Thara
> >> Subject: Re: [PATCH v11 5/8] OMAP: dmtimer: platform driver
> >>
> >> Tarun Kanti DebBarma <tarun.kanti@xxxxxx> writes:
> >>
> >> > From: Thara Gopinath <thara@xxxxxx>
> >> >
> >> > Add dmtimer platform driver functions which include:
> >> > (1) platform driver initialization
> >> > (2) driver probe function
> >> > (3) driver remove function
> >> >
> >> > Signed-off-by: Tarun Kanti DebBarma <tarun.kanti@xxxxxx>
> >> > Signed-off-by: Thara Gopinath <thara@xxxxxx>
> >> > Acked-by: Cousson, Benoit <b-cousson@xxxxxx>
> >>
> >> [...]
> >>
> >> > +/**
> >> > + * omap_dm_timer_probe - probe function called for every registered
> >> device
> >> > + * @pdev: pointer to current timer platform device
> >> > + *
> >> > + * Called by driver framework at the end of device registration for
> all
> >> > + * timer devices.
> >> > + */
> >> > +static int __devinit omap_dm_timer_probe(struct platform_device
> *pdev)
> >> > +{
> >> > + int ret;
> >> > + unsigned long flags;
> >> > + struct omap_dm_timer *timer;
> >> > + struct resource *mem, *irq, *ioarea;
> >> > + struct dmtimer_platform_data *pdata = pdev->dev.platform_data;
> >> > +
> >> > + if (!pdata) {
> >> > + dev_err(&pdev->dev, "%s: no platform data\n", __func__);
> >> > + return -ENODEV;
> >> > + }
> >> > +
> >> > + spin_lock_irqsave(&dm_timer_lock, flags);
> >> > + list_for_each_entry(timer, &omap_timer_list, node)
> >> > + if (!pdata->is_early_init && timer->id == pdev->id) {
> >> > + timer->pdev = pdev;
> >> > + spin_unlock_irqrestore(&dm_timer_lock, flags);
> >> > + dev_dbg(&pdev->dev, "Regular Probed\n");
> >> > + return 0;
> >> > + }
> >> > + spin_unlock_irqrestore(&dm_timer_lock, flags);
> >> > +
> >> > + irq = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
> >> > + if (unlikely(!irq)) {
> >> > + dev_err(&pdev->dev, "%s: no IRQ resource\n", __func__);
> >> > + ret = -ENODEV;
> >> > + goto err_free_pdev;
> >> > + }
> >> > +
> >> > + mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> >> > + if (unlikely(!mem)) {
> >> > + dev_err(&pdev->dev, "%s: no memory resource\n", __func__);
> >> > + ret = -ENODEV;
> >> > + goto err_free_pdev;
> >> > + }
> >> > +
> >> > + ioarea = request_mem_region(mem->start, resource_size(mem),
> >> > + pdev->name);
> >> > + if (!ioarea) {
> >> > + dev_err(&pdev->dev, "%s: region already claimed\n",
> __func__);
> >> > + ret = -EBUSY;
> >> > + goto err_free_pdev;
> >> > + }
> >> > +
> >> > + timer = kzalloc(sizeof(struct omap_dm_timer), GFP_KERNEL);
> >> > + if (!timer) {
> >> > + dev_err(&pdev->dev, "%s: no memory for omap_dm_timer\n",
> >> > + __func__);
> >> > + ret = -ENOMEM;
> >> > + goto err_release_ioregion;
> >> > + }
> >> > +
> >> > + timer->io_base = ioremap(mem->start, resource_size(mem));
> >> > + if (!timer->io_base) {
> >> > + dev_err(&pdev->dev, "%s: ioremap failed\n", __func__);
> >> > + ret = -ENOMEM;
> >> > + goto err_free_mem;
> >> > + }
> >> > +
> >> > + /*
> >> > + * Following func pointers are required by OMAP1's reset code
> >> > + * in mach-omap1/dmtimer.c to access to low level read/write.
> >> > + */
> >> > + if (pdata->is_omap16xx) {
> >> > + pdata->dm_timer_read_reg = omap_dm_timer_read_reg;
> >> > + pdata->dm_timer_write_reg = omap_dm_timer_write_reg;
> >> > + pdata->is_early_init = 0;
> >> > + }
> >>
> >> Can this 'is_omap16xx' check be replaced with an IP revision check?
> > Hmm, this not really!
> > Unless we introduce a new version to indicate OMAP1.
>
> Ah, I see. Looks like the revision change was after OMAP3, I thought it
> was after OMAP1. Sorry.
>
> > If this is what you mean I will make the change.
>
> An OMAP1 check would make this more clear, but really the flag should be
> a bool called something like "needs_manual_reset" since after all the
> other 'is_omap16xx' checks are removed, this is the only one left.
>
> That being said, I still don't really like this redirection, and find it
> rather non intuitive. I'm basically not crazy about the driver passing
> functions into the device-specific code. Typically the device-specific
> code passes functions into the driver. I see why it's done for the
> reset logic, but it's a bit confusing: device code sets 'is_omap16xx'
> flag for driver, driver checks flag and sets function pointers for
> device code.
>
> What's probably cleaner is to just move the reset functions (back) into
> the driver, but only set the reset pointer if pdata->needs_manual_reset
> == true, and that flag will only be set on OMAP1 since OMAP2+ is using
> hwmod.
Alright, I will make this change.
--
Tarun
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
