Re: [PATCH 05/12] staging:tidspbridge - set5 remove hungarian from structs
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
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
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
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
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
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
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
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
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
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
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
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()
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
"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()
'!' 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
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
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()
"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
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
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()
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()
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
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
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))
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
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
"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
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
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
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()
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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()
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