> -----Original Message----- > From: Kevin Hilman [mailto:khilman@xxxxxxxxxxxxxxxxxxx] > Sent: Tuesday, June 09, 2009 4:10 PM > To: Hald, Ulrik Bech > Cc: linux-omap@xxxxxxxxxxxxxxx > Subject: Re: [PATCH 2/3] OMAP3:WDT:Enable support for IVA and SECURE > > "Hald, Ulrik Bech" <ubh@xxxxxx> writes: > > >> -----Original Message----- > >> From: Kevin Hilman [mailto:khilman@xxxxxxxxxxxxxxxxxxx] > >> Sent: Tuesday, June 09, 2009 9:47 AM > >> To: Hald, Ulrik Bech > >> Cc: linux-omap@xxxxxxxxxxxxxxx > >> Subject: Re: [PATCH 2/3] OMAP3:WDT:Enable support for IVA and SECURE > >> > >> Ulrik Bech Hald <ubh@xxxxxx> writes: > >> > >> > This patch enables the IVA and SECURE WDTs in the omap_wdt > >> > driver for omap34xx family. SECURE will be available as > /dev/watchdog1 > >> > HS/EMU devices and IVA will be available as /dev/watchdog3. > >> > MPU will be available as /dev/watchdog2 > >> > For omap versions earlier than 34xx only MPU watchdog is present and > >> > will be available as /dev/watchdog for backwards compatibility. > >> > > >> > Signed-off-by: Ulrik Bech Hald <ubh@xxxxxx> > >> > --- > >> > drivers/watchdog/omap_wdt.c | 42 > +++++++++++++++++++++++++++++++++++- > >> ------ > >> > 1 files changed, 35 insertions(+), 7 deletions(-) > >> > mode change 100644 => 100755 drivers/watchdog/omap_wdt.c > >> > > >> > diff --git a/drivers/watchdog/omap_wdt.c > b/drivers/watchdog/omap_wdt.c > >> > >> [...] > >> > >> > static int omap_wdt_open(struct inode *inode, struct file *file) > >> > { > >> > - struct omap_wdt_dev *wdev = > platform_get_drvdata(omap_wdt_dev); > >> > - void __iomem *base = wdev->base; > >> > + struct omap_wdt_dev *wdev; > >> > + void __iomem *base; > >> > + > >> > + /* by default MPU wdt is present across omap family */ > >> > + wdev = platform_get_drvdata(omap_wdt_dev[1]); > >> > + > >> > + /* if multiple wdts, find match between device node and wdt > device > >> */ > >> > + if (cpu_is_omap34xx()) { > >> > + int i; > >> > + for (i = 0; i < NUM_WDTS; i++) { > >> > + if (omap_wdt_dev[i]) { > >> > + wdev = > platform_get_drvdata(omap_wdt_dev[i]); > >> > + if (iminor(inode) > >> > + == wdev->omap_wdt_miscdev.minor) > >> > + break; > >> > + } > >> > + } > >> > + } > >> > + > >> > + base = wdev->base; > >> > >> Hmm, this does not look right to me. Any use of cpu_is_* in driver > >> code usually indicates that something is not quite right. This same > >> watchdog IP is used in some DaVinci family processors and needs to be > >> reused there. > >> > >> I didn't do the miscdev research, but can't you get the pdev from the > >> miscdev.parent, and from there get the right wdev pointer from the > >> pdev.drvdata. > >> > >> Otherwise, drop the cpu_is_* and just do inode checking all the time. > >> The way you've done it looks error prone to me. > > > > I can't really see a to way pair the driver and device other than > > doing inode/minor number checking , so I'll just go for the inode > > checking without any dependency on cpu_is_xxx(). > > OK. > > >> > >> > if (test_and_set_bit(1, (unsigned long *)&(wdev- > >omap_wdt_users))) > >> > return -EBUSY; > >> > @@ -263,6 +283,7 @@ static int __devinit omap_wdt_probe(struct > >> platform_device *pdev) > >> > struct resource *res, *mem; > >> > struct omap_wdt_dev *wdev; > >> > int ret; > >> > + static char wdt_name[32]; > >> > > >> > /* reserve static register mappings */ > >> > res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > >> > @@ -271,7 +292,7 @@ static int __devinit omap_wdt_probe(struct > >> platform_device *pdev) > >> > goto err_get_resource; > >> > } > >> > > >> > - if (omap_wdt_dev) { > >> > + if (omap_wdt_dev[pdev->id-1]) { > >> > ret = -EBUSY; > >> > goto err_busy; > >> > } > >> > @@ -318,8 +339,14 @@ static int __devinit omap_wdt_probe(struct > >> platform_device *pdev) > >> > > >> > wdev->omap_wdt_miscdev.parent = &pdev->dev; > >> > wdev->omap_wdt_miscdev.minor = WATCHDOG_MINOR; > >> > - wdev->omap_wdt_miscdev.name = "watchdog"; > >> > wdev->omap_wdt_miscdev.fops = &omap_wdt_fops; > >> > + wdev->omap_wdt_miscdev.name = "watchdog"; > >> > + if (cpu_is_omap34xx()) { > >> > + snprintf(wdt_name, sizeof(wdt_name), "watchdog%d", pdev- > >id); > >> > + wdev->omap_wdt_miscdev.name = wdt_name; > >> > + if (pdev->id != 2) > >> > + wdev->omap_wdt_miscdev.minor = MISC_DYNAMIC_MINOR; > >> > + } > >> > >> Again, red flag: cpu_is_* > >> > >> It might be more consistent to use pdev->dev->name which is already of > >> the form "watchdog.%d". > > > > Not sure I follow. I don't see a "name" field in struct device, only > init_name. > > Sorry, I meant dev->bus_id. Look at dev_set_name() which creates > a bus_id of the form ("%s.%d", pdev->name, pdev->id) As far as I can see, dev->bus_id has been removed from the device struct as per 1fa5ae857bb14f6046205171d98506d8112dd74e. Furthermore, the pdev->name is "omap_wdt", which should be presented as "watchdog" in wdev->omap_wdt_miscdev.name anyway, so I think I'll just go with the sprintf() approach below, keeping your wdt_name comments in mind. /Ulrik > > > Do you mean just using: > > snprintf(wdt_name, sizeof(wdt_name), "watchdog%d", pdev->id); > > wdev->omap_wdt_miscdev.name = wdt_name; > > Using pdev->id is ok, but that's what dev_set_name() does. > > Note that if you decide to go this route, note that wdt_name doesn't > need to be static. The driver core will copy the name field the > misc_device. If it wasn't copying, you'd have all of the miscdev.name > fields ending up being the same thing since they are all assigned the > same pointer. > > Kevin > > > ...for all the WDT devices, despite how many are present? > > > >> > >> Any reason not to use MISC_DYNAMIC_MINOR for all of them? > > > > No particular reason other than I thought that retaining the use of > WATCHDOG_MINOR for the MPU WDT case would be appropriate. But it does make > the code cleaner to use only MISC_DYNAMIC_MINOR. I'll change it. > > > > Thanks for reviewing! > > > >> > >> Kevin -- 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
