Re: [PATCH 05/12] staging:tidspbridge - set5 remove hungarian from structs

2010-12-15 Thread Dan Carpenter 
On Tue, Dec 14, 2010 at 02:33:11PM -0600, Rene Sapiens wrote:
> - pfn_unload(pnode->nldr_node_obj,
> + load(pnode->nldr_node_obj,
>  NLDR_EXECUTE);

Unload got swapped with load here.

This might have been easier to with Coccinelle instead of by hand...

regards,
dan carpenter

--
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 3/5] staging: tidspbridge: Lindent to drv_interface.c

2012-01-30 Thread Dan Carpenter 
On Mon, Jan 30, 2012 at 11:25:34AM -0600, Ramirez Luna, Omar wrote:
> > +                       pr_info("%s:%d handle(s) still opened\n", __func__,
> > +                               atomic_read(&bridge_cref));
> 
> I remember the rule was to break lines as far to the right as
> possible, no? Chapter 2 CodingStyle, same for the other similar
> changes.
> 

It doesn't mean you have to right justify things, it just means
indented.  The original code is fine here and the new code is fine
here.  It's up to whoever writes the code to decide.

regards,
dan carpenter



signature.asc
Description: Digital signature

Re: [PATCH v2 4/8] staging: tidspbridge: silence the compiler

2012-01-31 Thread Dan Carpenter 
On Tue, Jan 31, 2012 at 12:12:20AM +0100, Víctor Manuel Jáquez Leal wrote:
> Silence the warning when compiling drv_interface.c
> 
> Signed-off-by: Víctor Manuel Jáquez Leal 

What does the compiler warn about here?  Normally you would cut and
paste the warning into the commit message.  But at least give us a
hint.

You could also reformat that printk so the message is on one line.

dev_dbg(bridge,
"%s: vm filp %p offset %x start %lx end %lx page_prot %ulx 
flags %lx\n",
__func__, filp, offset, vma->vm_start, vma->vm_end,
vma->vm_page_prot, vma->vm_flags);

regards,
dan carpenter




signature.asc
Description: Digital signature

Re: [PATCH 1/2] staging: tidspbridge: fix bridge_open memory leaks

2012-01-31 Thread Dan Carpenter 
On Mon, Jan 30, 2012 at 07:20:17PM -0600, Omar Ramirez Luna wrote:
> There are two members of pr_ctxt allocated during bridge_open that
> are never freed resulting in memory leaks, these are stream_id and
> node_id, they are now freed on release of the handle (bridge_release)
> right before freeing pr_ctxt.
> 
> Error path for bridge_open was also fixed since the same variables
> could result in memory leaking due to missing handling of failure
> scenarios. While at it, the indentation changes were introduced to
> avoid interleaved goto statements inside big if blocks.
> 

You mentioned in the cover letter email that you wanted these to go
into stable.  I don't think that is needed here.  These are tiny
leaks that we aren't going to hit in real life.

But if we did want to include them in stable then we would add a tag
to the patch next to the Signed-off-by.

Read Documentation/stable_kernel_rules.txt.  It's short.

regards,
dan carpenter



signature.asc
Description: Digital signature

Re: [PATCH 2/2] staging: tidspbridge: fix incorrect free to drv_datap

2012-01-31 Thread Dan Carpenter 
On Mon, Jan 30, 2012 at 07:20:18PM -0600, Omar Ramirez Luna wrote:
> This structure is still used after it has been freed, since it
> is being allocated in probe, calls to free it have been moved to
> module's remove routine.
> 
> This should fix the follwoing messages when attempting to remove the
> module:
>  drv_get_first_dev_extension: Failed to retrieve the object handle
>  drv_get_first_dev_extension: Failed to retrieve the object handle
>  drv_destroy: Failed to store DRV object
>  mgr_destroy: Failed to store MGR object
> 

So this is only triggered when you do an rmmod to remove the module?

Probably that's not stable material.

regards,
dan carpenter



signature.asc
Description: Digital signature

Re: [PATCH v2 4/8] staging: tidspbridge: silence the compiler

2012-01-31 Thread Dan Carpenter 
On Tue, Jan 31, 2012 at 12:19:52PM +0100, Víctor M. Jáquez L. wrote:
> On Tue, Jan 31, 2012 at 11:05:43AM +0300, Dan Carpenter wrote:
> > On Tue, Jan 31, 2012 at 12:12:20AM +0100, Víctor Manuel Jáquez Leal wrote:
> > > Silence the warning when compiling drv_interface.c
> > > 
> > > Signed-off-by: Víctor Manuel Jáquez Leal 
> > 
> > What does the compiler warn about here?  Normally you would cut and
> > paste the warning into the commit message.  But at least give us a
> > hint.
> > 
> > You could also reformat that printk so the message is on one line.
> > 
> > dev_dbg(bridge,
> > "%s: vm filp %p offset %x start %lx end %lx page_prot %ulx 
> > flags %lx\n",
> > __func__, filp, offset, vma->vm_start, vma->vm_end,
> > vma->vm_page_prot, vma->vm_flags);
> 
> Ok. I'll do it.
> 
> I have a doubt about the process in this case: if this is the only
> modification request, should I resend all the patch set, or just this one?
> 

If you sent just the one, and you redid the printk() then it would
make the later patches not apply.

Maybe just send the one, with a fixed changelog and leave the patch
as is.  The problems in the printk() were not introduced in this
patch so I can't really complain about that.  A couple people have
already volunteered to fix that in a later patch.

regards,
dan carpenter



signature.asc
Description: Digital signature

Re: [PATCH 1/2] staging: tidspbridge: fix bridge_open memory leaks

2012-01-31 Thread Dan Carpenter 
On Tue, Jan 31, 2012 at 12:09:17PM -0600, Omar Ramirez Luna wrote:
> On Tue, Jan 31, 2012 at 2:17 AM, Dan Carpenter  
> wrote:
> > On Mon, Jan 30, 2012 at 07:20:17PM -0600, Omar Ramirez Luna wrote:
> >> There are two members of pr_ctxt allocated during bridge_open that
> >> are never freed resulting in memory leaks, these are stream_id and
> >> node_id, they are now freed on release of the handle (bridge_release)
> >> right before freeing pr_ctxt.
> >>
> >> Error path for bridge_open was also fixed since the same variables
> >> could result in memory leaking due to missing handling of failure
> >> scenarios. While at it, the indentation changes were introduced to
> >> avoid interleaved goto statements inside big if blocks.
> >>
> >
> > You mentioned in the cover letter email that you wanted these to go
> > into stable.  I don't think that is needed here.  These are tiny
> > leaks that we aren't going to hit in real life.
> 
> They are hit each time an app opens a handle e.g.:
> 
> gst-launch 
> 
> So after 'X' number of playbacks there won't be memory left.
> 

Ah.  That sounds worth while then.

regards,
dan carpenter



signature.asc
Description: Digital signature

Re: [PATCH 2/2] staging: tidspbridge: fix incorrect free to drv_datap

2012-01-31 Thread Dan Carpenter 
On Tue, Jan 31, 2012 at 12:19:00PM -0600, Ramirez Luna, Omar wrote:
> On Tue, Jan 31, 2012 at 2:21 AM, Dan Carpenter  
> wrote:
> > On Mon, Jan 30, 2012 at 07:20:18PM -0600, Omar Ramirez Luna wrote:
> >> This structure is still used after it has been freed, since it
> >> is being allocated in probe, calls to free it have been moved to
> >> module's remove routine.
> >>
> >> This should fix the follwoing messages when attempting to remove the
> >> module:
> >>  drv_get_first_dev_extension: Failed to retrieve the object handle
> >>  drv_get_first_dev_extension: Failed to retrieve the object handle
> >>  drv_destroy: Failed to store DRV object
> >>  mgr_destroy: Failed to store MGR object
> >>
> >
> > So this is only triggered when you do an rmmod to remove the module?
> 
> Yes.
> 

How often do people rmmod things on a production system?  Hopefully,
never right?

regards,
dan carpenter



signature.asc
Description: Digital signature

Re: [PATCH 2/2] staging: tidspbridge: fix incorrect free to drv_datap

2012-01-31 Thread Dan Carpenter 
On Tue, Jan 31, 2012 at 09:39:00PM +0200, Felipe Contreras wrote:
> On Tue, Jan 31, 2012 at 8:43 PM, Dan Carpenter  
> wrote:
> > How often do people rmmod things on a production system?  Hopefully,
> > never right?
> 
> That's right... At least in recent versions of the tidspbrdige, that
> have recovery support. Before, we needed a script to detect MMU
> faults, and reload the module =/ IIRC this is still the case on the
> Nokia N900.

If you have a script that's doing rmmods then that's different.  I
didn't know about that.

regards,
dan carpenter



signature.asc
Description: Digital signature

[patch] usb: dwc3: make dwc3_get_device_id() return the id

2012-02-04 Thread Dan Carpenter 
We always return zero instead of the id we found.

Signed-off-by: Dan Carpenter 
---
I don't have this hardware so it's not tested.

diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
index 7c9df63..73df745 100644
--- a/drivers/usb/dwc3/core.c
+++ b/drivers/usb/dwc3/core.c
@@ -86,7 +86,7 @@ again:
id = -ENOMEM;
}
 
-   return 0;
+   return id;
 }
 EXPORT_SYMBOL_GPL(dwc3_get_device_id);
 
--
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] staging: tidspbridge: enable watchdog by default

2012-02-10 Thread Dan Carpenter 
On Fri, Feb 10, 2012 at 01:30:48AM +0200, Felipe Contreras wrote:
> It's not an oops, it's a warning, and again, it depends on the
> firmware being used. We don't have control over that, and we have no
> way to detect if this feature is there. It's up to the user.

Perhaps just remove the warning message and handle the condition
instead of printing a stack dump?  The user should be triggering
stack dumps.  What on earth?

regards,
dan carpenter


signature.asc
Description: Digital signature

Re: [PATCH] staging: tidspbridge: enable watchdog by default

2012-02-10 Thread Dan Carpenter 
On Fri, Feb 10, 2012 at 09:42:32PM +0200, Felipe Contreras wrote:
> On Fri, Feb 10, 2012 at 8:00 PM, Dan Carpenter  
> wrote:
> > On Fri, Feb 10, 2012 at 01:30:48AM +0200, Felipe Contreras wrote:
> >> It's not an oops, it's a warning, and again, it depends on the
> >> firmware being used. We don't have control over that, and we have no
> >> way to detect if this feature is there. It's up to the user.
> >
> > Perhaps just remove the warning message and handle the condition
> > instead of printing a stack dump?  The user should be triggering
> > stack dumps.  What on earth?
> 
> The warning doesn't come from the driver.

I'm not sure I understand.  Are you saying that because it comes
from the arch/ directory, it can't be fixed?  I have good news for
you my friend.  :)  It's all open source!  \o/

Anyway, I saw in another email that Omar is working on a fix so
probably we can just wait for his patch, yes?

regards,
dan carpenter


signature.asc
Description: Digital signature

[patch] dwc3: add a kfree() on error to dwc3_testmode_open()

2011-08-26 Thread Dan Carpenter 
We may as well fix this potential leak so we don't have to listen to
the static checkers complain.

Signed-off-by: Dan Carpenter 
---
Btw.  This function returns -EBUSY on success.  Was that really what
you want?

diff --git a/drivers/usb/dwc3/debugfs.c b/drivers/usb/dwc3/debugfs.c
index 432df53..dd861c4 100644
--- a/drivers/usb/dwc3/debugfs.c
+++ b/drivers/usb/dwc3/debugfs.c
@@ -445,8 +445,10 @@ static int dwc3_testmode_open(struct inode *inode, struct 
file *file)
if (!buf0)
return -ENOMEM;
buf1 = kmalloc(BUF_SIZE, GFP_KERNEL);
-   if (!buf1)
+   if (!buf1) {
+   kfree(buf0);
return -ENOMEM;
+   }
 
memset(buf0, 0xaa, BUF_SIZE);
memset(buf1, 0x33, BUF_SIZE);
--
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] usb: phy: signedness bugs in suspend/resume functions

2013-08-21 Thread Dan Carpenter 
"ret" needs to be signed for the error handling to work.

Signed-off-by: Dan Carpenter 
---
I can't compile this.

diff --git a/drivers/usb/phy/phy-omap-usb2.c b/drivers/usb/phy/phy-omap-usb2.c
index 844ab68..d266861 100644
--- a/drivers/usb/phy/phy-omap-usb2.c
+++ b/drivers/usb/phy/phy-omap-usb2.c
@@ -98,8 +98,8 @@ static int omap_usb_set_peripheral(struct usb_otg *otg,
 
 static int omap_usb2_suspend(struct usb_phy *x, int suspend)
 {
-   u32 ret;
struct omap_usb *phy = phy_to_omapusb(x);
+   int ret;
 
if (suspend && !phy->is_suspended) {
omap_control_usb_phy_power(phy->control_dev, 0);
@@ -108,8 +108,7 @@ static int omap_usb2_suspend(struct usb_phy *x, int suspend)
} else if (!suspend && phy->is_suspended) {
ret = pm_runtime_get_sync(phy->dev);
if (ret < 0) {
-   dev_err(phy->dev, "get_sync failed with err %d\n",
-   ret);
+   dev_err(phy->dev, "get_sync failed with err %d\n", ret);
return ret;
}
omap_control_usb_phy_power(phy->control_dev, 1);
@@ -209,9 +208,9 @@ static int omap_usb2_runtime_suspend(struct device *dev)
 
 static int omap_usb2_runtime_resume(struct device *dev)
 {
-   u32 ret = 0;
struct platform_device  *pdev = to_platform_device(dev);
struct omap_usb *phy = platform_get_drvdata(pdev);
+   int ret;
 
ret = clk_enable(phy->wkupclk);
if (ret < 0) {
--
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] mmc: omap_hsmmc: precedence bug in omap_hsmmc_context_restore()

2013-08-22 Thread Dan Carpenter 
'!' has higher precedence than '&' so this doesn't work as intended
although since RESETDONE is 1 it would work if none of the other bits
are set.

Signed-off-by: Dan Carpenter 
---
Untested.

diff --git a/drivers/mmc/host/omap_hsmmc.c b/drivers/mmc/host/omap_hsmmc.c
index 1865321..7346b15 100644
--- a/drivers/mmc/host/omap_hsmmc.c
+++ b/drivers/mmc/host/omap_hsmmc.c
@@ -612,7 +612,7 @@ static int omap_hsmmc_context_restore(struct 
omap_hsmmc_host *host)
if (host->context_loss == context_loss)
return 1;
 
-   if (!OMAP_HSMMC_READ(host->base, SYSSTATUS) & RESETDONE)
+   if (!(OMAP_HSMMC_READ(host->base, SYSSTATUS) & RESETDONE))
return 1;
 
if (host->pdata->controller_flags & OMAP_HSMMC_SUPPORTS_DUAL_VOLT) {
--
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: OMAPDSS: remove dispc's dependency to VENC/HDMI

2013-08-26 Thread Dan Carpenter 
Hello Tomi Valkeinen,

This is a semi-automatic email about new static checker warnings.

The patch 5391e87d1259: "OMAPDSS: remove dispc's dependency to 
VENC/HDMI" from May 16, 2013, leads to the following Smatch complaint:

drivers/video/omap2/dss/hdmi.c:672 omapdss_hdmi_display_set_timing()
 error: we previously assumed 't' could be null (see line 669)

drivers/video/omap2/dss/hdmi.c
   668  t = hdmi_get_timings();
   669  if (t != NULL)
^
Existing check.

   670  hdmi.ip_data.cfg = *t;
   671  
   672  dispc_set_tv_pclk(t->timings.pixel_clock * 1000);
  ^^
Patch added a dereference.

   673  
   674  mutex_unlock(&hdmi.lock);

regards,
dan carpenter
--
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] OMAPDSS: pll: NULL dereference in error handling

2014-12-16 Thread Dan Carpenter 
The regulator_disable() doesn't accept NULL pointers.

Signed-off-by: Dan Carpenter 

diff --git a/drivers/video/fbdev/omap2/dss/pll.c 
b/drivers/video/fbdev/omap2/dss/pll.c
index 50bc62c5..335ffac 100644
--- a/drivers/video/fbdev/omap2/dss/pll.c
+++ b/drivers/video/fbdev/omap2/dss/pll.c
@@ -97,7 +97,8 @@ int dss_pll_enable(struct dss_pll *pll)
return 0;
 
 err_enable:
-   regulator_disable(pll->regulator);
+   if (pll->regulator)
+   regulator_disable(pll->regulator);
 err_reg:
clk_disable_unprepare(pll->clkin);
return r;
--
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] usb: dwc3: omap: signedness bug in dwc3_omap_set_utmi_mode()

2014-07-31 Thread Dan Carpenter 
"ret" should be signed.  It's only used for zero and negative error
codes.

Signed-off-by: Dan Carpenter 

diff --git a/drivers/usb/dwc3/dwc3-omap.c b/drivers/usb/dwc3/dwc3-omap.c
index ef4936f..9dcfbe7 100644
--- a/drivers/usb/dwc3/dwc3-omap.c
+++ b/drivers/usb/dwc3/dwc3-omap.c
@@ -425,7 +425,7 @@ static void dwc3_omap_set_utmi_mode(struct dwc3_omap *omap)
 
 static int dwc3_omap_extcon_register(struct dwc3_omap *omap)
 {
-   u32 ret;
+   int ret;
struct device_node  *node = omap->dev->of_node;
struct extcon_dev   *edev;
 
--
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: OMAPDSS: HDMI: Add OMAP5 HDMI support

2014-12-03 Thread Dan Carpenter 
Hello Tomi Valkeinen,

The patch f5bab2229190: "OMAPDSS: HDMI: Add OMAP5 HDMI support" from
Mar 13, 2014, leads to the following static checker warning:

drivers/video/fbdev/omap2/dss/hdmi5_core.c:719 hdmi5_core_audio_config()
warn: '(3) - (4)' negative one

Uh...  This is really weird, I have no idea what this warning is or why
it's triggered.  I've even looked at the code which triggers it but that
has no documentation.  :P  Still it seems like something worth looking
into.

drivers/video/fbdev/omap2/dss/hdmi5_core.c
   713  /* PCM audio mode */
   714  val = (cfg->iec60958_cfg->status[0] & IEC958_AES0_CON_MODE) >> 
6;
   715  REG_FLD_MOD(base, HDMI_CORE_FC_AUDSCHNLS(2), val, 6, 4);
   716  
   717  /* Source number */
   718  val = cfg->iec60958_cfg->status[2] & IEC958_AES2_CON_SOURCE;
   719  REG_FLD_MOD(base, HDMI_CORE_FC_AUDSCHNLS(2), val, 3, 4);
  
Aren't these reversed?  This seems like an invalid bitfield range.

   720  
   721  /* Channel number right 0  */
   722  REG_FLD_MOD(base, HDMI_CORE_FC_AUDSCHNLS(3), 2, 3, 0);
   723  /* Channel number right 1*/
   724  REG_FLD_MOD(base, HDMI_CORE_FC_AUDSCHNLS(3), 4, 7, 4);
   725  /* Channel number right 2  */
   726  REG_FLD_MOD(base, HDMI_CORE_FC_AUDSCHNLS(4), 6, 3, 0);
   727  /* Channel number right 3*/
   728  REG_FLD_MOD(base, HDMI_CORE_FC_AUDSCHNLS(4), 8, 7, 4);
   729  /* Channel number left 0  */
   730  REG_FLD_MOD(base, HDMI_CORE_FC_AUDSCHNLS(5), 1, 3, 0);
   731  /* Channel number left 1*/
   732  REG_FLD_MOD(base, HDMI_CORE_FC_AUDSCHNLS(5), 3, 7, 4);
   733  /* Channel number left 2  */
   734  REG_FLD_MOD(base, HDMI_CORE_FC_AUDSCHNLS(6), 5, 3, 0);
   735  /* Channel number left 3*/

regards,
dan carpenter
--
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: usb: musb: omap: Add device tree support for omap musb glue

2014-06-10 Thread Dan Carpenter 
Hello Kishon Vijay Abraham I,

The patch 00a0b1d58af873d8: "usb: musb: omap: Add device tree support
for omap musb glue", from Sep 11 2012, leads to the following static
checker warning:

drivers/usb/musb/omap2430.c:569 omap2430_probe()
warn: does endianness matter for 'config->num_eps'?

drivers/usb/musb/omap2430.c
   565  
   566  of_property_read_u32(np, "mode", (u32 *)&pdata->mode);
   567  of_property_read_u32(np, "interface-type",
   568  (u32 
*)&data->interface_type);
   569  of_property_read_u32(np, "num-eps", (u32 
*)&config->num_eps);

^^^

This is not endian safe, but more importantly ->num_eps is a u8 so when
we write 32 bits to it, we are corrupting ->dma_channels,
->dyn_fifo_size, and ->vendor_ctrl.  On little endian, it's going to be
setting them to zero so it might not cause and immediate problem.

The way to do this is to use a 32 bit temporary variable and then save
the value to ->num_eps afterward.  Create a small function to do this in
a nice way.

All the casts here are a bit scary.

   570  of_property_read_u32(np, "ram-bits", (u32 
*)&config->ram_bits);
   571  of_property_read_u32(np, "power", (u32 *)&pdata->power);

regards,
dan carpenter
--
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] OMAPDSS: reading past end of array in dispc_dump_regs()

2012-12-14 Thread Dan Carpenter 
We added another kind of plane in 66a0f9e4ac "OMAPDSS: Use WB fifo for
GFX overlay" so this array needs a new entry as well.

Signed-off-by: Dan Carpenter 
---
Static checker work.  I don't have a way to test this.

diff --git a/drivers/video/omap2/dss/dispc.c b/drivers/video/omap2/dss/dispc.c
index fedbd2c..bfe62cc 100644
--- a/drivers/video/omap2/dss/dispc.c
+++ b/drivers/video/omap2/dss/dispc.c
@@ -3163,6 +3163,7 @@ static void dispc_dump_regs(struct seq_file *s)
[OMAP_DSS_VIDEO1]   = "VID1",
[OMAP_DSS_VIDEO2]   = "VID2",
[OMAP_DSS_VIDEO3]   = "VID3",
+   [OMAP_DSS_WB]   = "WB",
};
const char **p_names;
 
--
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] OMAPDSS: reading past end of array in dispc_dump_regs()

2012-12-17 Thread Dan Carpenter 
On Mon, Dec 17, 2012 at 02:09:00PM +0200, Tomi Valkeinen wrote:
> Why does the static checker think OMAP_DSS_WB is needed in the array?

drivers/video/omap2/dss/dispc.c +3284

  3274  #define DISPC_REG(plane, name, i) name(plane, i)
  3275  #define DUMPREG(plane, name, i) \
  3276  seq_printf(s, "%s_%d(%s)%*s %08x\n", #name, i, p_names[plane], \
  3277  (int)(46 - strlen(#name) - strlen(p_names[plane])), " ", \
  3278  dispc_read_reg(DISPC_REG(plane, name, i)))
  3279  
  3280  /* Video pipeline coefficient registers */
  3281  
  3282  /* start from OMAP_DSS_VIDEO1 */
  3283  for (i = 1; i < dss_feat_get_num_ovls(); i++) {
  3284  for (j = 0; j < 8; j++)
  3285  DUMPREG(i, DISPC_OVL_FIR_COEF_H, j);

The logic here is that we pass i to DISPC_OVL_FIR_COEF_H() which
passes i to DISPC_FIR_COEF_H_OFFSET().  Anything higher than
OMAP_DSS_WB will trigger a BUG() in DISPC_FIR_COEF_H_OFFSET().

So it's not rock hard logic at all.

regards,
dan carpenter

--
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 1/4] staging: omap-thermal: Correct checkpatch.pl warnings

2012-09-12 Thread Dan Carpenter 
On Tue, Sep 11, 2012 at 07:06:52PM +0300, Eduardo Valentin wrote:
> From: J Keerthy 
> 
> Removes checkpatch warnings on omap-bandgap.c.
> 

Which checkpatch.pl warnings?

> + omap_bandgap_writel(bg_ptr,
> + rval->tshut_threshold,
> +tsr->tshut_threshold);

That's just whacky.

Personally, I've never cared much about long lines, so I'd prefer
to leave these as is until someone can break the functions up and
remove them in a proper way instead of just shifting everything
randomly to the left.

regards,
dan carpenter

--
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 1/4] staging: omap-thermal: Correct checkpatch.pl warnings

2012-09-12 Thread Dan Carpenter 
On Wed, Sep 12, 2012 at 11:11:27AM +0300, Dan Carpenter wrote:
> On Tue, Sep 11, 2012 at 07:06:52PM +0300, Eduardo Valentin wrote:
> > From: J Keerthy 
> > 
> > Removes checkpatch warnings on omap-bandgap.c.
> > 
> 
> Which checkpatch.pl warnings?
> 
> > +   omap_bandgap_writel(bg_ptr,
> > +   rval->tshut_threshold,
> > +  tsr->tshut_threshold);
> 
> That's just whacky.
> 
> Personally, I've never cared much about long lines, so I'd prefer
> to leave these as is until someone can break the functions up and
> remove them in a proper way instead of just shifting everything
> randomly to the left.
> 

Sorry, that was my default response without looking at the code.

This is already broken up into small functions pretty nicely.  You
might want to consider using shorter names.  For example
omap_bandgap_writel() could be changed to "obg_writel()" and "bg_ptr"
could be changed to just "bg" because it's obviously a pointer.

regards,
dan carpenter
--
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: [balbi-usb:merge-result-for-greg 66/99] drivers/usb/core/hub.c:2654 usb_get_hub_port_power_state() error: doing dma on the stack ((null))

2012-09-12 Thread Dan Carpenter 
On Wed, Sep 12, 2012 at 04:26:22PM +0800, Fengguang Wu wrote:
> Hi Felipe,
> 
> FYI, there are new smatch warnings show up in
> 
> tree:   git://git.kernel.org/pub/scm/linux/kernel/git/balbi/usb.git 
> merge-result-for-greg
> head:   23953bde3e4d6aa8780dc054f6ad9882ac63f4f4
> commit: e918fa161f510136fce45a524e934fe20e62c8b1 [66/99] Merge tag 
> 'gadget-for-v3.7' into merge-result-for-greg
> 
> drivers/usb/core/hub.c:2654 usb_get_hub_port_power_state() error: doing dma 
> on the stack ((null))

Smatch prints out a lot of these warnings.  I haven't looked at the
rules in a while, so maybe there is a bounce buffer somewhere where
it detects stack memory and allocates a DMA'able buffer?

It would be better if the function documentation for
usb_control_msg() said that the *data pointer had to be kmalloc()ed.

Also I wonder if Documentation/DMA-API-HOWTO.txt is out of data.
Are we allowed to DMA to vmalloc()ed memory yet?

regards,
dan carpenter

--
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 1/4] staging: omap-thermal: Correct checkpatch.pl warnings

2012-09-12 Thread Dan Carpenter 
On Wed, Sep 12, 2012 at 12:19:00PM +0300, Valentin, Eduardo wrote:
> Hello Dan,
> 
> On Wed, Sep 12, 2012 at 11:26 AM, Dan Carpenter
>  wrote:
> > On Wed, Sep 12, 2012 at 11:11:27AM +0300, Dan Carpenter wrote:
> >> On Tue, Sep 11, 2012 at 07:06:52PM +0300, Eduardo Valentin wrote:
> >> > From: J Keerthy 
> >> >
> >> > Removes checkpatch warnings on omap-bandgap.c.
> >> >
> >>
> >> Which checkpatch.pl warnings?
> >>
> >> > +   omap_bandgap_writel(bg_ptr,
> >> > +   rval->tshut_threshold,
> >> > +  tsr->tshut_threshold);
> >>
> >> That's just whacky.
> >>
> >> Personally, I've never cared much about long lines, so I'd prefer
> >> to leave these as is until someone can break the functions up and
> >> remove them in a proper way instead of just shifting everything
> >> randomly to the left.
> >>
> >
> > Sorry, that was my default response without looking at the code.
> >
> > This is already broken up into small functions pretty nicely.  You
> > might want to consider using shorter names.  For example
> > omap_bandgap_writel() could be changed to "obg_writel()" and "bg_ptr"
> > could be changed to just "bg" because it's obviously a pointer.
> 
> Yeah, that's one option. Of course the deal is to find the proper
> balance between cryptic symbol names and code mangled / shifted to the
> left.
> 

Another option would be to just let checkpatch complain.  It's a
perl script, not the king of us.  Do what looks the nicest to human
beings.

regards,
dan carpenter

--
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] usb: phy: signedness bug in suspend/resume

2012-09-13 Thread Dan Carpenter 
"ret" should be signed here for the error handling to work.

Signed-off-by: Dan Carpenter 

diff --git a/drivers/usb/phy/omap-usb2.c b/drivers/usb/phy/omap-usb2.c
index 15ab3d6..d36c282 100644
--- a/drivers/usb/phy/omap-usb2.c
+++ b/drivers/usb/phy/omap-usb2.c
@@ -120,7 +120,7 @@ static int omap_usb_set_peripheral(struct usb_otg *otg,
 
 static int omap_usb2_suspend(struct usb_phy *x, int suspend)
 {
-   u32 ret;
+   int ret;
struct omap_usb *phy = phy_to_omapusb(x);
 
if (suspend && !phy->is_suspended) {
@@ -223,7 +223,7 @@ static int omap_usb2_runtime_suspend(struct device *dev)
 
 static int omap_usb2_runtime_resume(struct device *dev)
 {
-   u32 ret = 0;
+   int ret;
struct platform_device  *pdev = to_platform_device(dev);
struct omap_usb *phy = platform_get_drvdata(pdev);
 
--
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: ARM: OMAP: mcbsp: Make wakeup control generic

2013-11-06 Thread Dan Carpenter 
Hello Jarkko Nikula,

This is a semi-automatic email about new static checker warnings.

The patch 1a6458847dd2: "ARM: OMAP: mcbsp: Make wakeup control 
generic" from Sep 26, 2011, leads to the following Smatch complaint:

sound/soc/omap/mcbsp.c:590 omap_mcbsp_free()
 error: we previously assumed 'mcbsp->pdata' could be null (see line 
586)

sound/soc/omap/mcbsp.c
   585  
   586  if (mcbsp->pdata && mcbsp->pdata->ops && 
mcbsp->pdata->ops->free)

Existing check.

   587  mcbsp->pdata->ops->free(mcbsp->id - 1);
   588  
   589  /* Disable wakeup behavior */
   590  if (mcbsp->pdata->has_wakeup)

Patch introduces an unchecked dereference.

   591  MCBSP_WRITE(mcbsp, WAKEUPEN, 0);
   592  

regards,
dan carpenter
--
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: ARM: OMAP2+: Powerdomain: Remove the need to always have a voltdm associated to a pwrdm

2013-11-06 Thread Dan Carpenter 
Hello Rajendra Nayak,

This is a semi-automatic email about new static checker warnings.

The patch cd8abed1da91: "ARM: OMAP2+: Powerdomain: Remove the need to 
always have a voltdm associated to a pwrdm" from Jun 17, 2013, leads 
to the following Smatch complaint:

arch/arm/mach-omap2/powerdomain.c:131 _pwrdm_register()
 error: we previously assumed 'arch_pwrdm' could be null (see line 105)

arch/arm/mach-omap2/powerdomain.c
   104  
   105  if (arch_pwrdm && arch_pwrdm->pwrdm_has_voltdm)
^^
Patch introduces new check.

   106  if (!arch_pwrdm->pwrdm_has_voltdm())
   107  goto skip_voltdm;
   108  
   109  voltdm = voltdm_lookup(pwrdm->voltdm.name);
   110  if (!voltdm) {
   111  pr_err("powerdomain: %s: voltagedomain %s does not 
exist\n",
   112 pwrdm->name, pwrdm->voltdm.name);
   113  return -EINVAL;
   114  }
   115  pwrdm->voltdm.ptr = voltdm;
   116  INIT_LIST_HEAD(&pwrdm->voltdm_node);
   117  voltdm_add_pwrdm(voltdm, pwrdm);
   118  skip_voltdm:
   119  spin_lock_init(&pwrdm->_lock);
   120  
   121  list_add(&pwrdm->node, &pwrdm_list);
   122  
   123  /* Initialize the powerdomain's state counter */
   124  for (i = 0; i < PWRDM_MAX_PWRSTS; i++)
   125  pwrdm->state_counter[i] = 0;
   126  
   127  pwrdm->ret_logic_off_counter = 0;
   128  for (i = 0; i < pwrdm->banks; i++)
   129  pwrdm->ret_mem_off_counter[i] = 0;
   130  
   131  arch_pwrdm->pwrdm_wait_transition(pwrdm);

Existing unchecked dereference.

   132  pwrdm->state = pwrdm_read_pwrst(pwrdm);
   133  pwrdm->state_counter[pwrdm->state] = 1;

regards,
dan carpenter
--
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: mmc: omap: Fix DMA configuration to not rely on device id

2013-12-02 Thread Dan Carpenter 
Hello Tony Lindgren,

This is a semi-automatic email about new static checker warnings.

The patch 31ee9181eb92: "mmc: omap: Fix DMA configuration to not rely 
on device id" from Nov 26, 2013, leads to the following Smatch 
complaint:

drivers/mmc/host/omap.c:1468 mmc_omap_probe()
 error: we previously assumed 'res' could be null (see line 1410)

drivers/mmc/host/omap.c
  1409  res = platform_get_resource_byname(pdev, IORESOURCE_DMA, "rx");
  1410  if (res)
^^^
Patch introduces a check.

  1411  sig = res->start;
  1412  host->dma_rx = dma_request_slave_channel_compat(mask,
  1413  omap_dma_filter_fn, &sig, &pdev->dev, 
"rx");
  1414  if (!host->dma_rx)
  1415  dev_warn(host->dev, "unable to obtain RX DMA engine 
channel %u\n",
  1416  sig);
  1417  
  1418  ret = request_irq(host->irq, mmc_omap_irq, 0, DRIVER_NAME, 
host);
  1419  if (ret)
  1420  goto err_free_dma;
  1421  
  1422  if (pdata->init != NULL) {
  1423  ret = pdata->init(&pdev->dev);
  1424  if (ret < 0)
  1425  goto err_free_irq;
  1426  }
  1427  
  1428  host->nr_slots = pdata->nr_slots;
  1429  host->reg_shift = (mmc_omap7xx() ? 1 : 2);
  1430  
  1431  host->mmc_omap_wq = alloc_workqueue("mmc_omap", 0, 0);
  1432  if (!host->mmc_omap_wq)
  1433  goto err_plat_cleanup;
  1434  
  1435  for (i = 0; i < pdata->nr_slots; i++) {
  1436  ret = mmc_omap_new_slot(host, i);
  1437  if (ret < 0) {
  1438  while (--i >= 0)
  1439  mmc_omap_remove_slot(host->slots[i]);
  1440  
  1441  goto err_destroy_wq;
  1442  }
  1443  }
  1444  
  1445  return 0;
  1446  
  1447  err_destroy_wq:
  1448  destroy_workqueue(host->mmc_omap_wq);
  1449  err_plat_cleanup:
  1450  if (pdata->cleanup)
  1451  pdata->cleanup(&pdev->dev);
  1452  err_free_irq:
  1453  free_irq(host->irq, host);
  1454  err_free_dma:
  1455  if (host->dma_tx)
  1456  dma_release_channel(host->dma_tx);
  1457  if (host->dma_rx)
  1458  dma_release_channel(host->dma_rx);
  1459  clk_put(host->fclk);
  1460  err_free_iclk:
  1461  clk_disable(host->iclk);
  1462  clk_put(host->iclk);
  1463  err_free_mmc_host:
  1464  iounmap(host->virt_base);
  1465  err_ioremap:
  1466  kfree(host);
  1467  err_free_mem_region:
  1468  release_mem_region(res->start, resource_size(res));
   ^^
Existing unchecked dereferences.

  1469  return ret;
  1470  }

regards,
dan carpenter
--
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] mmc: omap_hsmmc: precedence bug in omap_hsmmc_context_restore()

2014-01-30 Thread Dan Carpenter 
On Thu, Aug 22, 2013 at 08:16:28PM +0530, Balaji T K wrote:
> On Thursday 22 August 2013 06:26 PM, Dan Carpenter wrote:
> >'!' has higher precedence than '&' so this doesn't work as intended
> >although since RESETDONE is 1 it would work if none of the other bits
> >are set.
> >
> Hi Dan,
> 
> Thanks for the patch, Indeed other bits are reserved.
> however ...
> 
> >Signed-off-by: Dan Carpenter 
> >---
> >Untested.
> >
> >diff --git a/drivers/mmc/host/omap_hsmmc.c b/drivers/mmc/host/omap_hsmmc.c
> >index 1865321..7346b15 100644
> >--- a/drivers/mmc/host/omap_hsmmc.c
> >+++ b/drivers/mmc/host/omap_hsmmc.c
> >@@ -612,7 +612,7 @@ static int omap_hsmmc_context_restore(struct 
> >omap_hsmmc_host *host)
> > if (host->context_loss == context_loss)
> > return 1;
> >
> >-if (!OMAP_HSMMC_READ(host->base, SYSSTATUS) & RESETDONE)
> >+if (!(OMAP_HSMMC_READ(host->base, SYSSTATUS) & RESETDONE))
> > return 1;
> 
> This check is unnecessary, will send a patch to remove this.

What happened with this?

regards,
dan carpenter

--
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 1/2] mmc: omap_hsmmc: remove a duplicative test

2014-01-30 Thread Dan Carpenter 
Static checkers complain that testing for both "next" and "!next" is
duplicative.

Signed-off-by: Dan Carpenter 

diff --git a/drivers/mmc/host/omap_hsmmc.c b/drivers/mmc/host/omap_hsmmc.c
index 2a629fbde613..bfb0dbd052c0 100644
--- a/drivers/mmc/host/omap_hsmmc.c
+++ b/drivers/mmc/host/omap_hsmmc.c
@@ -1235,8 +1235,7 @@ static int omap_hsmmc_pre_dma_transfer(struct 
omap_hsmmc_host *host,
}
 
/* Check if next job is already prepared */
-   if (next ||
-   (!next && data->host_cookie != host->next_data.cookie)) {
+   if (next || data->host_cookie != host->next_data.cookie) {
dma_len = dma_map_sg(chan->device->dev, data->sg, data->sg_len,
 omap_hsmmc_get_dma_dir(host, data));
 
--
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/2] mmc: omap_hsmmc: checking for ERR_PTR instead of NULL

2014-01-30 Thread Dan Carpenter 
The of_get_hsmmc_pdata() function returns NULL on error, it doesn't
return ERR_PTRs.

Signed-off-by: Dan Carpenter 

diff --git a/drivers/mmc/host/omap_hsmmc.c b/drivers/mmc/host/omap_hsmmc.c
index bfb0dbd052c0..76dbc7b759c2 100644
--- a/drivers/mmc/host/omap_hsmmc.c
+++ b/drivers/mmc/host/omap_hsmmc.c
@@ -1761,9 +1761,8 @@ static int omap_hsmmc_probe(struct platform_device *pdev)
match = of_match_device(of_match_ptr(omap_mmc_of_match), &pdev->dev);
if (match) {
pdata = of_get_hsmmc_pdata(&pdev->dev);
-
-   if (IS_ERR(pdata))
-   return PTR_ERR(pdata);
+   if (!pdata)
+   return -ENOMEM;
 
if (match->data) {
const u16 *offsetp = match->data;
--
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 0/5] staging: tidspbridge: for 3.9

2013-01-20 Thread Dan Carpenter 
On Thu, Jan 17, 2013 at 04:47:46PM -0800, Greg Kroah-Hartman wrote:
> On Thu, Jan 10, 2013 at 03:36:57AM -0600, Omar Ramirez Luna wrote:
> > Patches for staging-next, fixing comments and suggestions provided
> > by Chen Gang.
> > 
> > There is an additional scm patch, that removes hardcoded defines
> > related to direct register handling for SCM, it was dependent on
> > changes that already made it to mainline.
> 
> What is the status on getting this out of the staging tree?  What needs
> to be done still?
> 

Why can't tidspbridge be built as a module?

The Kconfig help files aren't very informative.

regards,
dan carpenter

--
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 0/5] staging: tidspbridge: for 3.9

2013-01-21 Thread Dan Carpenter 
On Mon, Jan 21, 2013 at 10:25:24AM +0300, Dan Carpenter wrote:
> On Thu, Jan 17, 2013 at 04:47:46PM -0800, Greg Kroah-Hartman wrote:
> > On Thu, Jan 10, 2013 at 03:36:57AM -0600, Omar Ramirez Luna wrote:
> > > Patches for staging-next, fixing comments and suggestions provided
> > > by Chen Gang.
> > > 
> > > There is an additional scm patch, that removes hardcoded defines
> > > related to direct register handling for SCM, it was dependent on
> > > changes that already made it to mainline.
> > 
> > What is the status on getting this out of the staging tree?  What needs
> > to be done still?
> > 
> 
> Why can't tidspbridge be built as a module?
> 
> The Kconfig help files aren't very informative.

Huh.  Ignore me.  I don't know what I was looking at before.  Both
the things I said were wrong.

Sorry for the noise.

regards,
dan carpenter

--
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 07/50] staging: omap-thermal: introduce RMW_BITS macro

2013-03-15 Thread Dan Carpenter 
On Fri, Mar 15, 2013 at 08:59:55AM -0400, Eduardo Valentin wrote:
> This patch introduce a macro to read, update, write bitfields.
> It will be specific to bandgap data structures.
> 
> Signed-off-by: Eduardo Valentin 
> ---
>  drivers/staging/omap-thermal/omap-bandgap.c |  178 
> +++
>  1 files changed, 46 insertions(+), 132 deletions(-)
> 
> diff --git a/drivers/staging/omap-thermal/omap-bandgap.c 
> b/drivers/staging/omap-thermal/omap-bandgap.c
> index 9f2d7cc..1c1b905 100644
> --- a/drivers/staging/omap-thermal/omap-bandgap.c
> +++ b/drivers/staging/omap-thermal/omap-bandgap.c
> @@ -52,25 +52,29 @@ static void omap_bandgap_writel(struct omap_bandgap 
> *bg_ptr, u32 val, u32 reg)
>   writel(val, bg_ptr->base + reg);
>  }
>  
> +/* update bits, value will be shifted */
> +#define RMW_BITS(bg_ptr, id, reg, mask, val) \
> +do { \
> + struct temp_sensor_registers *t;\
> + u32 r;  \
> + \
> + t = bg_ptr->conf->sensors[(id)].registers;  \
> + r = omap_bandgap_readl(bg_ptr, t->reg); \
> + r &= ~t->mask;  \
> + r |= (val) << __ffs(t->mask);   \
> + omap_bandgap_writel(bg_ptr, r, t->reg); \
> +} while (0)
> +
>  static int omap_bandgap_power(struct omap_bandgap *bg_ptr, bool on)
>  {
> - struct temp_sensor_registers *tsr;
>   int i;
> - u32 ctrl;
>  
>   if (!OMAP_BANDGAP_HAS(bg_ptr, POWER_SWITCH))
>   return 0;
>  
> - for (i = 0; i < bg_ptr->conf->sensor_count; i++) {
> - tsr = bg_ptr->conf->sensors[i].registers;
> - ctrl = omap_bandgap_readl(bg_ptr, tsr->temp_sensor_ctrl);
> - ctrl &= ~tsr->bgap_tempsoff_mask;
> + for (i = 0; i < bg_ptr->conf->sensor_count; i++)
>   /* active on 0 */
> - ctrl |= !on << __ffs(tsr->bgap_tempsoff_mask);
> -
> - /* write BGAP_TEMPSOFF should be reset to 0 */
> - omap_bandgap_writel(bg_ptr, ctrl, tsr->temp_sensor_ctrl);
> - }
> + RMW_BITS(bg_ptr, i, temp_sensor_ctrl, bgap_tempsoff_mask, !on);
>  
>   return 0;
>  }
> @@ -78,15 +82,13 @@ static int omap_bandgap_power(struct omap_bandgap 
> *bg_ptr, bool on)
>  static u32 omap_bandgap_read_temp(struct omap_bandgap *bg_ptr, int id)
>  {

This patch is fine and it makes it cleaner than before.

But that said, I don't care for the RMW_BITS() very much as a long
term thing.  If we just used pointers instead of passing the offset
into the bg_ptr->conf->sensors[] array then everything would be a
lot cleaner.

In other words, instead of this:

static u32 omap_bandgap_read_temp(struct omap_bandgap *bg_ptr, int id)

We would have:

static u32 omap_bandgap_read_temp(struct omap_bandgap *bg_ptr,
  struct temp_sensor_registers *tsr)

If you have the pointer then it's easy write RMW_BITS() as a
function.

static void rmw_bits(struct omap_bandgap *bg_ptr, u32 reg, u32 mask, u32 val)
{
u32 r;

r = omap_bandgap_readl(bg_ptr, reg);
r &= ~mask;
r |= val << __ffs(mask);
omap_bandgap_writel(bg_ptr, r, reg);
}

It's called like:

rmw_bits(bg_ptr, tsr->bgap_mask_ctrl, tsr->mask_freeze_mask, 1);

regards,
dan carpenter


--
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 09/50] staging: omap-thermal: make a omap_bandgap_power with only one exit point

2013-03-15 Thread Dan Carpenter 
On Fri, Mar 15, 2013 at 08:59:57AM -0400, Eduardo Valentin wrote:
> Change the way the omap_bandgap_power is written so that it has only
> one exit entry (Documentation/CodingStyle).
> 

It's only if there is an unlock or something that you should do
this.  Otherwise the pointless bunny hop is misleading and annoying.

regards,
dan carpenter

--
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 22/50] staging: omap-thermal: rewrite omap_bandgap_adc_to_mcelsius on kernel coding style

2013-03-16 Thread Dan Carpenter 
On Fri, Mar 15, 2013 at 09:00:10AM -0400, Eduardo Valentin wrote:
> Follow Documentation/CodingStyle while doing  omap_bandgap_adc_to_mcelsius.

Someone should probably fix CodingStyle to be more clear.  That's
not what was intended at all...  :/

regards,
dan carpenter

--
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 25/50] staging: omap-thermal: rewrite omap_bandgap_mcelsius_to_adc on kernel coding style

2013-03-16 Thread Dan Carpenter 
On Fri, Mar 15, 2013 at 09:00:13AM -0400, Eduardo Valentin wrote:
> Follow Documentation/CodingStyle while doing omap_bandgap_mcelsius_to_adc
> 

I have the same response for all these bunny hop patches.

When you're reading the code and you see a goto then you assume that
it's there for a reason so it's misleading when it doesn't do
anything.  It's simpler if the code is simple.

regards,
dan carpenter

--
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 33/50] staging: omap-thermal: refactor APIs handling threshold values

2013-03-16 Thread Dan Carpenter 
On Fri, Mar 15, 2013 at 09:00:21AM -0400, Eduardo Valentin wrote:
>   if (ret) {
>   dev_err(bg_ptr->dev, "failed to read thot\n");
> - return -EIO;
> + ret = -EIO;
> + goto exit;
>   }
>  
> - *thot = temp;
> + *val = temp;
>  
> +exit:
>   return 0;
>  }


The bunny hop has introduced a bug and this always returns success.

regards,
dan carpenter

--
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 47/50] staging: omap-thermal: switch mutex to spinlock inside omap-bandgap

2013-03-16 Thread Dan Carpenter 
On Fri, Mar 15, 2013 at 09:00:35AM -0400, Eduardo Valentin wrote:
> Because there is a need to lock inside IRQ handler, this patch
> changes the locking mechanism inside the omap-bandgap.[c,h] to
> spinlocks. Now this lock is used to protect omap_bandgap struct
> during APIs exposed (possibly used in sysfs handling functions)
> and inside the ALERT IRQ handler.
> 
> Because there are registers shared among the sensors, this lock
> is global, not per sensor.
> 
> Signed-off-by: Eduardo Valentin 
> ---
>  drivers/staging/omap-thermal/TODO   |1 -
>  drivers/staging/omap-thermal/omap-bandgap.c |   18 ++
>  drivers/staging/omap-thermal/omap-bandgap.h |4 ++--
>  3 files changed, 12 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/staging/omap-thermal/TODO 
> b/drivers/staging/omap-thermal/TODO
> index 77b761b..0f24e9b 100644
> --- a/drivers/staging/omap-thermal/TODO
> +++ b/drivers/staging/omap-thermal/TODO
> @@ -1,7 +1,6 @@
>  List of TODOs (by Eduardo Valentin)
>  
>  on omap-bandgap.c:
> -- Rework locking
>  - Improve driver code by adding usage of regmap-mmio
>  - Test every exposed API to userland
>  - Add support to hwmon
> diff --git a/drivers/staging/omap-thermal/omap-bandgap.c 
> b/drivers/staging/omap-thermal/omap-bandgap.c
> index 4b631fd..846ced6 100644
> --- a/drivers/staging/omap-thermal/omap-bandgap.c
> +++ b/drivers/staging/omap-thermal/omap-bandgap.c
> @@ -33,7 +33,7 @@
>  #include 
>  #include 
>  #include 
> -#include 
> +#include 
>  #include 
>  #include 
>  #include 
> @@ -170,6 +170,7 @@ static irqreturn_t omap_bandgap_talert_irq_handler(int 
> irq, void *data)
>   u32 t_hot = 0, t_cold = 0, ctrl;
>   int i;
>  
> + spin_lock(&bg_ptr->lock);
>   for (i = 0; i < bg_ptr->conf->sensor_count; i++) {
>   tsr = bg_ptr->conf->sensors[i].registers;
>   ctrl = omap_bandgap_readl(bg_ptr, tsr->bgap_status);
> @@ -208,6 +209,7 @@ static irqreturn_t omap_bandgap_talert_irq_handler(int 
> irq, void *data)
>   if (bg_ptr->conf->report_temperature)
>   bg_ptr->conf->report_temperature(bg_ptr, i);
>   }
> + spin_unlock(&bg_ptr->lock);
>  
>   return IRQ_HANDLED;
>  }
> @@ -502,9 +504,9 @@ int _omap_bandgap_write_threshold(struct omap_bandgap 
> *bg_ptr, int id, int val,
>   if (ret < 0)
>   goto exit;
>  
> - mutex_lock(&bg_ptr->bg_mutex);
> + spin_lock(&bg_ptr->lock);

These need to disable interrupts because we take the spin lock in
the IRQ handler.

regards,
dan carpenter


--
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 00/50] staging: omap-thermal: several code refactoring

2013-03-16 Thread Dan Carpenter 
I've reviewed this set.

I hate to make people redo whole patchset sets, and I hate
re-reviewing code.  Obviously, I don't really like the bunny hop
patches and I'm trying to discourage that going forward.  ;P  But
I wouldn't say it's a "Redo the whole thing" kind of problem.

Could just resend patch 33 and 47?  You should probably be able to
redo those without changing the rest.

regards,
dan carpenter

--
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 09/50] staging: omap-thermal: make a omap_bandgap_power with only one exit point

2013-03-16 Thread Dan Carpenter 
On Sat, Mar 16, 2013 at 08:39:11AM -0400, Eduardo Valentin wrote:
> Hey Dan,
> 
> On 15-03-2013 17:22, Dan Carpenter wrote:
> >On Fri, Mar 15, 2013 at 08:59:57AM -0400, Eduardo Valentin wrote:
> >>Change the way the omap_bandgap_power is written so that it has only
> >>one exit entry (Documentation/CodingStyle).
> >>
> >
> >It's only if there is an unlock or something that you should do
> >this.  Otherwise the pointless bunny hop is misleading and annoying.
> 
> Well, if that is the case the Chapter 7 needs to be rewritten, don't
> you think? The way it is stated, it is clear that it is a design
> decision to use it for keeping only one exit point (quoting):
> 
> "Albeit deprecated by some people, the equivalent of the goto statement is
> used frequently by compilers in form of the unconditional jump instruction.
> 
> The goto statement comes in handy when a function exits from multiple
> locations and some common work such as cleanup has to be done.
> 
> The rationale is:
> 
> - unconditional statements are easier to understand and follow
> - nesting is reduced
> - errors by not updating individual exit points when making
> modifications are prevented
> - saves the compiler work to optimize redundant code away ;)"
> 
> I believe this patch falls into at least three of the above rationale.

There isn't any cleanup work done.  That's what I was explaining
about.  When I see a goto I expect cleanup work to be done, but in
this case it was misleading.

Where you would do the one exit point is when there is cleanup:

ret = function_one();
if (ret)
goto unlock;

ret = function_two();
if (ret)
goto unlock;


That's better than:
ret = function_one();
if (ret) {
mutex_unlock();
return ret;
}

ret = function_two();
if (ret) {
mutex_unlock();
return ret;
}

On the one hand, I think the documentation should be updated because
obviously people get confused by it.  But on the other hand, to me
the documentation is already pretty clear.  There is no style
guideline that says every function should only have a single return.
That would be silly.

regards,
dan carpenter

--
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 33/50] staging: omap-thermal: refactor APIs handling threshold values

2013-03-16 Thread Dan Carpenter 
On Sat, Mar 16, 2013 at 08:49:20AM -0400, Eduardo Valentin wrote:
> On 16-03-2013 04:39, Dan Carpenter wrote:
> >On Fri, Mar 15, 2013 at 09:00:21AM -0400, Eduardo Valentin wrote:
> >>if (ret) {
> >>dev_err(bg_ptr->dev, "failed to read thot\n");
> >>-   return -EIO;
> >>+   ret = -EIO;
> >>+   goto exit;
> >>}
> >>
> >>-   *thot = temp;
> >>+   *val = temp;
> >>
> >>+exit:
> >>return 0;
> >>  }
> >
> >
> >The bunny hop has introduced a bug and this always returns success.
> 
> What was the bug introduced?
> 
> I will send a patch to fix the return value.

Returning the wrong value *is* the bug.

regards,
dan carpenter

--
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 07/50] staging: omap-thermal: introduce RMW_BITS macro

2013-03-16 Thread Dan Carpenter 
On Sat, Mar 16, 2013 at 08:36:51AM -0400, Eduardo Valentin wrote:
> >But that said, I don't care for the RMW_BITS() very much as a long
> >term thing.  If we just used pointers instead of passing the offset
> >into the bg_ptr->conf->sensors[] array then everything would be a
> >lot cleaner.
> >
> >In other words, instead of this:
> >
> >static u32 omap_bandgap_read_temp(struct omap_bandgap *bg_ptr, int id)
> >
> >We would have:
> >
> >static u32 omap_bandgap_read_temp(struct omap_bandgap *bg_ptr,
> >   struct temp_sensor_registers *tsr)
> 
> I see. That will require a change in the whole driver though. If you
> see, the driver as of today uses the former approach, not only for
> read_temp or rmw_bits.

Yep.

> 
> >
> >If you have the pointer then it's easy write RMW_BITS() as a
> >function.
> >
> >static void rmw_bits(struct omap_bandgap *bg_ptr, u32 reg, u32 mask, u32 val)
> >{
> > u32 r;
> >
> > r = omap_bandgap_readl(bg_ptr, reg);
> > r &= ~mask;
> > r |= val << __ffs(mask);
> > omap_bandgap_writel(bg_ptr, r, reg);
> >}
> >
> >It's called like:
> >
> > rmw_bits(bg_ptr, tsr->bgap_mask_ctrl, tsr->mask_freeze_mask, 1);
> 
> This is nice, but it will require fetching tsr from .conf before
> every call o rmw_bits. And for that you need the sensor index.
> 

No.  I'm suggesting that you re-write the driver to pass the tsr
pointer directly instead of the index.

regards,
dan carpenter

--
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 47/50] staging: omap-thermal: switch mutex to spinlock inside omap-bandgap

2013-03-16 Thread Dan Carpenter 
On Sat, Mar 16, 2013 at 08:41:30AM -0400, Eduardo Valentin wrote:
> On 16-03-2013 04:59, Dan Carpenter wrote:
> >On Fri, Mar 15, 2013 at 09:00:35AM -0400, Eduardo Valentin wrote:
> >>@@ -502,9 +504,9 @@ int _omap_bandgap_write_threshold(struct omap_bandgap 
> >>*bg_ptr, int id, int val,
> >>if (ret < 0)
> >>goto exit;
> >>
> >>-   mutex_lock(&bg_ptr->bg_mutex);
> >>+   spin_lock(&bg_ptr->lock);
> >
> >These need to disable interrupts because we take the spin lock in
> >the IRQ handler.
> 
> This IRQ gets masked at the IRQ controller level when served
> (ONE_SHOT). Not sure if your comment is applicable in this case..

Yes.  It still applies.  We need to disable IRQs from the current
CPU while we are holding a spin_lock which they will need.

regards,
dan carpenter

--
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 1/8] staging: omap-thermal: fix return value

2013-03-18 Thread Dan Carpenter 
Thanks.

Acked-by: Dan Carpenter 

regards,
dan carpenter

--
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/8] staging: omap-thermal: use spin_lock_irqsave inside IRQ handler

2013-03-18 Thread Dan Carpenter 
On Mon, Mar 18, 2013 at 10:59:10AM -0400, Eduardo Valentin wrote:
> Even if the IRQ is not firing because it is ONE_SHOT and disable
> at INTC level, the IRQ handler must use spin_lock_irqsave.
> It is necessary to disable IRQs from the current
> CPU while it is holding a spin_lock which is need.
> 

Gar...  I think I was just totally wrong on this.  I think your
original code was fine.  Sorry Eduardo and Greg.

This is a threaded IRQ so the regular spin_lock is fine or even the
mutex would have been.

IRQ_ONESHOT is about triggering a second IRQ before the first one
has been finished, btw.

I am an idiot.

regards,
dan carpenter

--
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 4/8] staging: rename omap-thermal driver to ti-soc-thermal

2013-03-18 Thread Dan Carpenter 
On Mon, Mar 18, 2013 at 10:59:12AM -0400, Eduardo Valentin wrote:
> Because this driver will support also OMAP derivatives,
> this patch does a big rename inside this driver, so it
> better fits its usage.
> 

It would be better to do a minimal move patch which just renames the
files and updated the Makefile.  The following patches would rename
the functions.  That makes it easier to review.


> Cc: Santosh Shilimkar 
> Cc: Benoit 
> Cc: Nishanth Menon 
> 
> Signed-off-by: Eduardo Valentin 

Put a --- line here after the Signed-off-by line so that "git am"
doesn't include the diff stat in the changelog.

regards,
dan carpenter

--
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/8] staging: omap-thermal: use spin_lock_irqsave inside IRQ handler

2013-03-18 Thread Dan Carpenter 
On Mon, Mar 18, 2013 at 03:38:38PM -0400, Eduardo Valentin wrote:
> On 18-03-2013 15:16, Dan Carpenter wrote:
> >On Mon, Mar 18, 2013 at 10:59:10AM -0400, Eduardo Valentin wrote:
> >>Even if the IRQ is not firing because it is ONE_SHOT and disable
> >>at INTC level, the IRQ handler must use spin_lock_irqsave.
> >>It is necessary to disable IRQs from the current
> >>CPU while it is holding a spin_lock which is need.
> >>
> >
> >Gar...  I think I was just totally wrong on this.  I think your
> >original code was fine.  Sorry Eduardo and Greg.
> >
> >This is a threaded IRQ so the regular spin_lock is fine or even the
> >mutex would have been.
> 
> In fact it is. But I rather prefer to use spinlocks there, just to
> keep the irq handler sane, even if it is moved to non-threaded IRQ.

Yep.  I'd agree there.

> 
> >
> >IRQ_ONESHOT is about triggering a second IRQ before the first one
> >has been finished, btw.
> 
> It is, and that gets done by masking the IRQ at INTC level.
> 
> >
> >I am an idiot.
> 
> 
> Not really. Thanks for your time reviewing the driver.
> 
>  I will resend this series. Drop this one and split patch 4/8 into
> two I think (one for moving files, one for renaming functions)

Great.  Much appreciated.

regards,
dan carpenter

--
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: [PATCHv2 00/12] staging: [omap,ti-soc]-thermal: fixes and renaming

2013-03-20 Thread Dan Carpenter 
These look nice.  Thanks for breaking up the move and api rename
into separate patches.

regards,
dan carpenter

--
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] OMAPDSS: DSI: cleanup DSI_IRQ_ERROR_MASK define

2015-11-23 Thread Dan Carpenter 
DSI_IRQ_SYNC_LOST was ORed twice so we can remove one.

Signed-off-by: Dan Carpenter 

diff --git a/drivers/video/fbdev/omap2/dss/dsi.c 
b/drivers/video/fbdev/omap2/dss/dsi.c
index b3606de..e86df6d 100644
--- a/drivers/video/fbdev/omap2/dss/dsi.c
+++ b/drivers/video/fbdev/omap2/dss/dsi.c
@@ -144,7 +144,7 @@ struct dsi_reg { u16 module; u16 idx; };
 #define DSI_IRQ_TA_TIMEOUT (1 << 20)
 #define DSI_IRQ_ERROR_MASK \
(DSI_IRQ_HS_TX_TIMEOUT | DSI_IRQ_LP_RX_TIMEOUT | DSI_IRQ_SYNC_LOST | \
-   DSI_IRQ_TA_TIMEOUT | DSI_IRQ_SYNC_LOST)
+   DSI_IRQ_TA_TIMEOUT)
 #define DSI_IRQ_CHANNEL_MASK   0xf
 
 /* Virtual channel interrupts */
--
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] OMAPDSS: DSS: fix a warning message

2015-12-04 Thread Dan Carpenter 
The WARN() macro has to take a condition.  The current code will just
print the stack trace and the function name instead of the intended
warning message.

Signed-off-by: Dan Carpenter 

diff --git a/drivers/video/fbdev/omap2/dss/dss.h 
b/drivers/video/fbdev/omap2/dss/dss.h
index 2406bcd..da3a85a 100644
--- a/drivers/video/fbdev/omap2/dss/dss.h
+++ b/drivers/video/fbdev/omap2/dss/dss.h
@@ -343,7 +343,8 @@ u8 dsi_get_pixel_size(enum omap_dss_dsi_pixel_format fmt);
 #else
 static inline u8 dsi_get_pixel_size(enum omap_dss_dsi_pixel_format fmt)
 {
-   WARN("%s: DSI not compiled in, returning pixel_size as 0\n", __func__);
+   WARN(1, "%s: DSI not compiled in, returning pixel_size as 0\n",
+__func__);
return 0;
 }
 #endif
--
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 v4 01/58] mtd: nand: denali: add missing nand_release() call in denali_remove()

2015-12-11 Thread Dan Carpenter 
On Fri, Dec 11, 2015 at 02:53:20PM +0100, Boris Brezillon wrote:
> Hi Brian,
> 
> On Thu, 10 Dec 2015 16:40:08 -0800
> Brian Norris  wrote:
> 
> > On Thu, Dec 10, 2015 at 08:59:45AM +0100, Boris Brezillon wrote:
> > > Unregister the NAND device from the NAND subsystem when removing a denali
> > > NAND controller, otherwise the MTD attached to the NAND device is still
> > > exposed by the MTD layer, and accesses to this device will likely crash
> > > the system.
> > > 
> > > Signed-off-by: Boris Brezillon 
> > > Cc:  #3.8+
> > 
> > Does this follow these rules, from
> > Documentation/stable_kernel_rules.txt?
> > 
> >  - It must be obviously correct and tested.
> > 
> >  - It must fix a real bug that bothers people (not a, "This could be a
> >problem..." type thing).
> 
> As you wish, I'll remove those Cc and Fixes tags, or just drop the
> patch if you think it's useless...

The fixes tag is a separate thing from CCing stable.  It's useful on by
itself.  I always put the person who wrote the original patch in the To:
header so they can review and comment if I have made a mistake.

regards,
dan carpenter

--
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