RE: [PATCH 2/3] OMAP3:WDT:Enable support for IVA and SECURE

2009-06-15 Thread Hald, Ulrik Bech 
 -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 u...@ti.com 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 u...@ti.com 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 u...@ti.com
   ---
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

Re: [PATCH 2/3] OMAP3:WDT:Enable support for IVA and SECURE

2009-06-15 Thread Kevin Hilman 
Hald, Ulrik Bech u...@ti.com 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

2009-06-09 Thread Kevin Hilman 
Ulrik Bech Hald u...@ti.com 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 u...@ti.com
 ---
  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

RE: [PATCH 2/3] OMAP3:WDT:Enable support for IVA and SECURE

2009-06-09 Thread Hald, Ulrik Bech 
 -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 u...@ti.com 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 u...@ti.com
  ---
   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

2009-06-09 Thread Kevin Hilman 
Hald, Ulrik Bech u...@ti.com 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 u...@ti.com 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 u...@ti.com
  ---
   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)

 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