Re: [PATCH 2/3] OMAP3:WDT:Enable support for IVA and SECURE
"Hald, Ulrik Bech" writes: [...] >> >> >> >> 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. OK Kevin -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH 2/3] OMAP3:WDT:Enable support for IVA and SECURE
> -Original Message- > From: Kevin Hilman [mailto:khil...@deeprootsystems.com] > Sent: Tuesday, June 09, 2009 4:10 PM > To: Hald, Ulrik Bech > Cc: linux-omap@vger.kernel.org > Subject: Re: [PATCH 2/3] OMAP3:WDT:Enable support for IVA and SECURE > > "Hald, Ulrik Bech" writes: > > >> -Original Message- > >> From: Kevin Hilman [mailto:khil...@deeprootsystems.com] > >> Sent: Tuesday, June 09, 2009 9:47 AM > >> To: Hald, Ulrik Bech > >> Cc: linux-omap@vger.kernel.org > >> Subject: Re: [PATCH 2/3] OMAP3:WDT:Enable support for IVA and SECURE > >> > >> Ulrik Bech Hald 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 > >> > --- > >> > 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; > >> > } > >> > > >>
Re: [PATCH 2/3] OMAP3:WDT:Enable support for IVA and SECURE
"Hald, Ulrik Bech" writes: >> -Original Message- >> From: Kevin Hilman [mailto:khil...@deeprootsystems.com] >> Sent: Tuesday, June 09, 2009 9:47 AM >> To: Hald, Ulrik Bech >> Cc: linux-omap@vger.kernel.org >> Subject: Re: [PATCH 2/3] OMAP3:WDT:Enable support for IVA and SECURE >> >> Ulrik Bech Hald 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 >> > --- >> > 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) >> > +
RE: [PATCH 2/3] OMAP3:WDT:Enable support for IVA and SECURE
> -Original Message- > From: Kevin Hilman [mailto:khil...@deeprootsystems.com] > Sent: Tuesday, June 09, 2009 9:47 AM > To: Hald, Ulrik Bech > Cc: linux-omap@vger.kernel.org > Subject: Re: [PATCH 2/3] OMAP3:WDT:Enable support for IVA and SECURE > > Ulrik Bech Hald 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 > > --- > > 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(). > > > 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. Do you mean just using: snprintf(wdt_name, sizeof(wdt_name), "watchdog%d", pdev->id); wdev->omap_wdt_miscdev.name = wdt_name; ...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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/3] OMAP3:WDT:Enable support for IVA and SECURE
Ulrik Bech Hald 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 > --- > 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. > 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". Any reason not to use MISC_DYNAMIC_MINOR for all of them? Kevin -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 2/3] OMAP3:WDT:Enable support for IVA and SECURE
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 --- 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 old mode 100644 new mode 100755 index f271385..26935c7 --- a/drivers/watchdog/omap_wdt.c +++ b/drivers/watchdog/omap_wdt.c @@ -47,7 +47,9 @@ #include "omap_wdt.h" -static struct platform_device *omap_wdt_dev; +#define NUM_WDTS 3 + +static struct platform_device *omap_wdt_dev[NUM_WDTS]; static unsigned timer_margin; module_param(timer_margin, uint, 0); @@ -139,8 +141,26 @@ static void omap_wdt_set_timeout(struct omap_wdt_dev *wdev) */ 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; 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; + } ret = misc_register(&(wdev->omap_wdt_miscdev)); if (ret) @@ -332,7 +359,8 @@ static int __devinit omap_wdt_probe(struct platform_device *pdev) /* autogate OCP interface clock */ __raw_writel(0x01, wdev->base + OMAP_WATCHDOG_SYS_CONFIG); - omap_wdt_dev = pdev; + /* keep track of the wdt platform devices in local device array */ + omap_wdt_dev[pdev->id - 1] = pdev; return 0; @@ -384,7 +412,7 @@ static int __devexit omap_wdt_remove(struct platform_device *pdev) iounmap(wdev->base); kfree(wdev); - omap_wdt_dev = NULL; + omap_wdt_dev[pdev->id-1] = NULL; return 0; } -- 1.5.4.3 -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html