* [PATCH 5/6] staging: sm750fb: fix mixed declarations
From: Sudip Mukherjee @ 2015-03-10 17:28 UTC (permalink / raw)
To: Greg Kroah-Hartman; +Cc: linux-fbdev, devel, linux-kernel, Sudip Mukherjee
In-Reply-To: <1426007817-1884-1-git-send-email-sudipm.mukherjee@gmail.com>
we were getting build warning about mixed declaration. the variable
is now declared at the beginning of the block.
Signed-off-by: Sudip Mukherjee <sudip@vectorindia.org>
---
drivers/staging/sm750fb/sm750.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/staging/sm750fb/sm750.c b/drivers/staging/sm750fb/sm750.c
index 87029b6..021b863 100644
--- a/drivers/staging/sm750fb/sm750.c
+++ b/drivers/staging/sm750fb/sm750.c
@@ -1175,6 +1175,7 @@ ALLOC_FB:
else
{
struct lynxfb_par * par;
+ int errno;
pr_info("framebuffer #%d alloc okay\n",fbidx);
share->fbinfo[fbidx] = info[fbidx];
par = info[fbidx]->par;
@@ -1191,7 +1192,7 @@ ALLOC_FB:
/* register frame buffer*/
pr_info("Ready to register framebuffer #%d.\n",fbidx);
- int errno = register_framebuffer(info[fbidx]);
+ errno = register_framebuffer(info[fbidx]);
if (errno < 0) {
pr_err("Failed to register fb_info #%d. err %d\n",fbidx, errno);
if(fbidx = 0)
--
1.8.1.2
^ permalink raw reply related
* [PATCH 4/6] staging: sm750fb: correct incompatible pointer type
From: Sudip Mukherjee @ 2015-03-10 17:28 UTC (permalink / raw)
To: Greg Kroah-Hartman; +Cc: linux-fbdev, devel, linux-kernel, Sudip Mukherjee
In-Reply-To: <1426007817-1884-1-git-send-email-sudipm.mukherjee@gmail.com>
we were getting build warnings about assignment of incompatible
pointer types. some of the function definitions were having wrong
return type or arguments.
Signed-off-by: Sudip Mukherjee <sudip@vectorindia.org>
---
drivers/staging/sm750fb/sm750.h | 11 ++++++-----
drivers/staging/sm750fb/sm750_accel.c | 4 ++--
drivers/staging/sm750fb/sm750_accel.h | 4 ++--
3 files changed, 10 insertions(+), 9 deletions(-)
diff --git a/drivers/staging/sm750fb/sm750.h b/drivers/staging/sm750fb/sm750.h
index d39968c..edb955a 100644
--- a/drivers/staging/sm750fb/sm750.h
+++ b/drivers/staging/sm750fb/sm750.h
@@ -23,7 +23,7 @@ struct lynx_accel{
volatile unsigned char __iomem * dpPortBase;
/* function fointers */
- int (*de_init)(struct lynx_accel *);
+ void (*de_init)(struct lynx_accel *);
int (*de_wait)(void);/* see if hardware ready to work */
@@ -34,8 +34,8 @@ struct lynx_accel{
u32,u32,u32,u32,
u32,u32,u32,u32);
- int (*de_imageblit)(struct lynx_accel *,const char *,u32,u32,u32,
- u32,u32,u32,u32,u32,u32,u32,u32,u32);
+ int (*de_imageblit)(struct lynx_accel *, const char *, u32, u32, u32,
+ u32, u32, u32, u32, u32, u32, u32, u32, u32);
};
@@ -120,8 +120,9 @@ struct lynxfb_crtc{
int(*proc_setColReg)(struct lynxfb_crtc*,ushort,ushort,ushort,ushort);
void (*clear)(struct lynxfb_crtc*);
/* pan display */
- int(*proc_panDisplay)(struct lynxfb_crtc*, struct fb_var_screeninfo*,
- struct fb_info*);
+ int (*proc_panDisplay)(struct lynxfb_crtc*,
+ const struct fb_var_screeninfo*,
+ const struct fb_info*);
/* cursor information */
struct lynx_cursor cursor;
};
diff --git a/drivers/staging/sm750fb/sm750_accel.c b/drivers/staging/sm750fb/sm750_accel.c
index ee211de..421adef 100644
--- a/drivers/staging/sm750fb/sm750_accel.c
+++ b/drivers/staging/sm750fb/sm750_accel.c
@@ -397,8 +397,8 @@ static unsigned int deGetTransparency(struct lynx_accel * accel)
int hw_imageblit(
struct lynx_accel * accel,
-unsigned char *pSrcbuf, /* pointer to start of source buffer in system memory */
-int srcDelta, /* Pitch value (in bytes) of the source buffer, +ive means top down and -ive mean button up */
+const char *pSrcbuf, /* pointer to start of source buffer in system memory */
+unsigned int srcDelta, /* Pitch value (in bytes) of the source buffer, +ive means top down and -ive mean button up */
unsigned int startBit, /* Mono data can start at any bit in a byte, this value should be 0 to 7 */
unsigned int dBase, /* Address of destination: offset in frame buffer */
unsigned int dPitch, /* Pitch value of destination surface in BYTE */
diff --git a/drivers/staging/sm750fb/sm750_accel.h b/drivers/staging/sm750fb/sm750_accel.h
index 575f4a7..9c16618 100644
--- a/drivers/staging/sm750fb/sm750_accel.h
+++ b/drivers/staging/sm750fb/sm750_accel.h
@@ -260,8 +260,8 @@ unsigned int rop2);
int hw_imageblit(
struct lynx_accel * accel,
-unsigned char *pSrcbuf, /* pointer to start of source buffer in system memory */
-int srcDelta, /* Pitch value (in bytes) of the source buffer, +ive means top down and -ive mean button up */
+const char *pSrcbuf, /* pointer to start of source buffer in system memory */
+unsigned int srcDelta, /* Pitch value (in bytes) of the source buffer, +ive means top down and -ive mean button up */
unsigned int startBit, /* Mono data can start at any bit in a byte, this value should be 0 to 7 */
unsigned int dBase, /* Address of destination: offset in frame buffer */
unsigned int dPitch, /* Pitch value of destination surface in BYTE */
--
1.8.1.2
^ permalink raw reply related
* [PATCH 3/6] staging: sm750fb: correct function return
From: Sudip Mukherjee @ 2015-03-10 17:28 UTC (permalink / raw)
To: Greg Kroah-Hartman; +Cc: linux-fbdev, devel, linux-kernel, Sudip Mukherjee
In-Reply-To: <1426007817-1884-1-git-send-email-sudipm.mukherjee@gmail.com>
hw_cursor_setData2() is a function with void return type but it was
returning an integer.
Signed-off-by: Sudip Mukherjee <sudip@vectorindia.org>
---
drivers/staging/sm750fb/sm750_cursor.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/drivers/staging/sm750fb/sm750_cursor.c b/drivers/staging/sm750fb/sm750_cursor.c
index 76b6029..7bd17d1 100644
--- a/drivers/staging/sm750fb/sm750_cursor.c
+++ b/drivers/staging/sm750fb/sm750_cursor.c
@@ -248,6 +248,4 @@ void hw_cursor_setData2(struct lynx_cursor * cursor,
}
}
- return 0;
-
}
--
1.8.1.2
^ permalink raw reply related
* [PATCH 2/6] staging: sm750fb: remove unused variables
From: Sudip Mukherjee @ 2015-03-10 17:28 UTC (permalink / raw)
To: Greg Kroah-Hartman; +Cc: linux-fbdev, devel, linux-kernel, Sudip Mukherjee
In-Reply-To: <1426007817-1884-1-git-send-email-sudipm.mukherjee@gmail.com>
removed some variables which were only declared but were never used.
Signed-off-by: Sudip Mukherjee <sudip@vectorindia.org>
---
drivers/staging/sm750fb/sm750_cursor.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/staging/sm750fb/sm750_cursor.c b/drivers/staging/sm750fb/sm750_cursor.c
index 480615c..76b6029 100644
--- a/drivers/staging/sm750fb/sm750_cursor.c
+++ b/drivers/staging/sm750fb/sm750_cursor.c
@@ -99,7 +99,6 @@ void hw_cursor_setData(struct lynx_cursor * cursor,
u8 color,mask,opr;
u16 data;
u16 * pbuffer,*pstart;
- static ulong odd = 0;
/* in byte*/
pitch = cursor->w >> 3;
@@ -188,7 +187,7 @@ void hw_cursor_setData2(struct lynx_cursor * cursor,
u16 rop,const u8* pcol,const u8* pmsk)
{
int i,j,count,pitch,offset;
- u8 color,mask,opr;
+ u8 color, mask;
u16 data;
u16 * pbuffer,*pstart;
--
1.8.1.2
^ permalink raw reply related
* [PATCH 1/6] staging: sm750fb: remove unused functions
From: Sudip Mukherjee @ 2015-03-10 17:28 UTC (permalink / raw)
To: Greg Kroah-Hartman; +Cc: linux-fbdev, devel, linux-kernel, Sudip Mukherjee
In-Reply-To: <1426007817-1884-1-git-send-email-sudipm.mukherjee@gmail.com>
removed the functions which were not used anywhere.
it has been build tested also confirmed with git grep that there is
no other reference of these functions.
Signed-off-by: Sudip Mukherjee <sudip@vectorindia.org>
---
drivers/staging/sm750fb/ddk750_display.c | 11 ------
drivers/staging/sm750fb/ddk750_swi2c.c | 8 ----
drivers/staging/sm750fb/sm750.c | 65 --------------------------------
3 files changed, 84 deletions(-)
diff --git a/drivers/staging/sm750fb/ddk750_display.c b/drivers/staging/sm750fb/ddk750_display.c
index 57192e5..a282a94 100644
--- a/drivers/staging/sm750fb/ddk750_display.c
+++ b/drivers/staging/sm750fb/ddk750_display.c
@@ -201,17 +201,6 @@ static void waitNextVerticalSync(int ctrl,int delay)
}
}
-static void swPanelPowerSequence_sm750le(int disp,int delay)
-{
- unsigned int reg;
- reg = PEEK32(DISPLAY_CONTROL_750LE);
- if(disp)
- reg |= 0xf;
- else
- reg &= ~0xf;
- POKE32(DISPLAY_CONTROL_750LE,reg);
-}
-
static void swPanelPowerSequence(int disp,int delay)
{
unsigned int reg;
diff --git a/drivers/staging/sm750fb/ddk750_swi2c.c b/drivers/staging/sm750fb/ddk750_swi2c.c
index cae6b9b..1249759 100644
--- a/drivers/staging/sm750fb/ddk750_swi2c.c
+++ b/drivers/staging/sm750fb/ddk750_swi2c.c
@@ -79,14 +79,6 @@ static unsigned long g_i2cDataGPIOMuxReg = GPIO_MUX;
static unsigned long g_i2cDataGPIODataReg = GPIO_DATA;
static unsigned long g_i2cDataGPIODataDirReg = GPIO_DATA_DIRECTION;
-static unsigned char peekIO(unsigned short port,unsigned short index)
-{
-#if defined(__i386__) || defined( __x86_64__)
- outb_p(index,port);
- return inb_p(port+1);
-#endif
-}
-
/*
* This function puts a delay between command
*/
diff --git a/drivers/staging/sm750fb/sm750.c b/drivers/staging/sm750fb/sm750.c
index 8c260ee..87029b6 100644
--- a/drivers/staging/sm750fb/sm750.c
+++ b/drivers/staging/sm750fb/sm750.c
@@ -510,69 +510,6 @@ static int lynxfb_resume(struct pci_dev* pdev)
}
#endif
-static int lynxfb_ops_mmap(struct fb_info * info, struct vm_area_struct * vma)
-{
- unsigned long off;
- unsigned long start;
- u32 len;
- struct file *file;
-
- file = vma->vm_file;
-
- if (!info)
- return -ENODEV;
- if (vma->vm_pgoff > (~0UL >> PAGE_SHIFT))
- return -EINVAL;
- off = vma->vm_pgoff << PAGE_SHIFT;
- printk("lynxfb mmap pgoff: %lx\n", vma->vm_pgoff);
- printk("lynxfb mmap off 1: %lx\n", off);
-
- /* frame buffer memory */
- start = info->fix.smem_start;
- len = PAGE_ALIGN((start & ~PAGE_MASK) + info->fix.smem_len);
-
- printk("lynxfb mmap start 1: %lx\n", start);
- printk("lynxfb mmap len 1: %x\n", len);
-
- if (off >= len) {
- /* memory mapped io */
- off -= len;
- printk("lynxfb mmap off 2: %lx\n", off);
- if (info->var.accel_flags) {
- printk("lynxfb mmap accel flags true");
- return -EINVAL;
- }
- start = info->fix.mmio_start;
- len = PAGE_ALIGN((start & ~PAGE_MASK) + info->fix.mmio_len);
-
- printk("lynxfb mmap start 2: %lx\n", start);
- printk("lynxfb mmap len 2: %x\n", len);
- }
- start &= PAGE_MASK;
- printk("lynxfb mmap start 3: %lx\n", start);
- printk("lynxfb mmap vm start: %lx\n", vma->vm_start);
- printk("lynxfb mmap vm end: %lx\n", vma->vm_end);
- printk("lynxfb mmap len: %x\n", len);
- printk("lynxfb mmap off: %lx\n", off);
- if ((vma->vm_end - vma->vm_start + off) > len)
- {
- return -EINVAL;
- }
- off += start;
- printk("lynxfb mmap off 3: %lx\n", off);
- vma->vm_pgoff = off >> PAGE_SHIFT;
- /* This is an IO map - tell maydump to skip this VMA */
- vma->vm_flags |= VM_IO | VM_DONTEXPAND | VM_DONTDUMP;
- vma->vm_page_prot = vm_get_page_prot(vma->vm_flags);
- fb_pgprotect(file, vma, off);
- printk("lynxfb mmap off 4: %lx\n", off);
- printk("lynxfb mmap pgprot: %lx\n", (unsigned long) pgprot_val(vma->vm_page_prot));
- if (io_remap_pfn_range(vma, vma->vm_start, off >> PAGE_SHIFT,
- vma->vm_end - vma->vm_start, vma->vm_page_prot))
- return -EAGAIN;
- return 0;
-}
-
static int lynxfb_ops_check_var(struct fb_var_screeninfo* var,struct fb_info* info)
{
struct lynxfb_par * par;
@@ -824,8 +761,6 @@ static struct fb_ops lynxfb_ops={
.fb_set_par = lynxfb_ops_set_par,
.fb_setcolreg = lynxfb_ops_setcolreg,
.fb_blank = lynxfb_ops_blank,
- /*.fb_mmap = lynxfb_ops_mmap,*/
- /* will be hooked by hardware */
.fb_fillrect = cfb_fillrect,
.fb_imageblit = cfb_imageblit,
.fb_copyarea = cfb_copyarea,
--
1.8.1.2
^ permalink raw reply related
* [PATCH 0/6] staging: sm750fb: fixed all build warnings
From: Sudip Mukherjee @ 2015-03-10 17:28 UTC (permalink / raw)
To: Greg Kroah-Hartman; +Cc: linux-fbdev, devel, linux-kernel, Sudip Mukherjee
Hi Greg,
all the build warnings have been taken care of in this series.
some of the patches will generate a few checkpatch warning and for some I tried
to address few of the chcekpatch warnings while modifying them.
regards
sudip
Sudip Mukherjee (6):
staging: sm750fb: remove unused functions
staging: sm750fb: remove unused variables
staging: sm750fb: correct function return
staging: sm750fb: correct incompatible pointer type
staging: sm750fb: fix mixed declarations
staging: sm750fb: correct integer comparison
drivers/staging/sm750fb/ddk750_display.c | 11 -----
drivers/staging/sm750fb/ddk750_swi2c.c | 8 ----
drivers/staging/sm750fb/sm750.c | 70 ++------------------------------
drivers/staging/sm750fb/sm750.h | 11 ++---
drivers/staging/sm750fb/sm750_accel.c | 4 +-
drivers/staging/sm750fb/sm750_accel.h | 4 +-
drivers/staging/sm750fb/sm750_cursor.c | 5 +--
7 files changed, 14 insertions(+), 99 deletions(-)
--
1.8.1.2
^ permalink raw reply
* Re: [PATCH v2] staging: sm750fb: Fix sparse warning
From: Lorenzo Stoakes @ 2015-03-10 15:33 UTC (permalink / raw)
To: Greg KH; +Cc: Sudip Mukherjee, teddy.wang, devel, linux-fbdev, linux-kernel
In-Reply-To: <20150310150612.GA22560@kroah.com>
On 10 March 2015 at 15:06, Greg KH <gregkh@linuxfoundation.org> wrote:
> This doesn't apply anymore due to other patches I just took, so can you
> rebase this patch on my latest staging-testing branch of staging.git?
Fixed in v3 of staging: sm750fb: Cleanup the type of mmio75.
> Also, how about someone fixing the real compiler warnings this driver is
> spitting out first, before we worry about sparse warnings? That's much
> more relevant here as it's keeping the driver from being built in
> linux-next at the moment.
>
> thanks,
>
> greg k-h
I am more than happy to take a look at this later today when I have
the opportunity to do so :)
--
Lorenzo Stoakes
https:/ljs.io
^ permalink raw reply
* Re: [PATCH] video: treat signal like timeout as failure
From: Russell King - ARM Linux @ 2015-03-10 15:26 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <54FF05FC.5030704@ti.com>
On Tue, Mar 10, 2015 at 04:55:56PM +0200, Tomi Valkeinen wrote:
> On 10/03/15 16:46, Russell King - ARM Linux wrote:
>
> > In which case, let me propose that the exynos fbdev driver needs to be
> > moved to drivers/staging, and stay there until this stuff gets fixed.
> > drivers/staging is supposed to be for stuff which isn't up to the mark,
> > and which is potentially unstable. And that's what this driver exactly
> > is.
>
> There is drivers/gpu/drm/exynos/ which is getting a lot of updates. So...
>
> I'd propose removing the exynos fbdev driver if the exynos drm driver
> offers the same functionality. I don't know if that's the case. Does the
> drm driver support all the devices the fbdev supports?
>
> Also, I'm not sure if and how we can remove drivers. If exynos fbdev
> driver is dropped, that would perhaps break boards that have exynos
> fbdev in their .dts file. And if the drm driver doesn't offer the exact
> same /dev/fbX interface, it would break the userspace.
>
> So I don't know if that's possible. But that's what I'd like to do,
> eventually, for all the fbdev drivers. Implement drm driver, remove the
> fbdev one.
That's why I suggested moving it to drivers/staging - it's a hint that
the driver needs a serious amount of work, and when built as a module,
it also provides users with the hint that the module they're loading is
of questionable quality (which is definitely the case here.)
Others have done that kind of thing before - we've had drivers which
have fallen by the way side, and at some point the decision has been
made to move them to drivers/staging, and if nothing happens to fix
them up (showing that no one cares about them), they've eventually
been dropped.
Of course, us talking about this might be enough to spur some effort
to get the thing properly fixed. :)
--
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.
^ permalink raw reply
* [PATCH v3] staging: sm750fb: Cleanup the type of mmio750
From: Lorenzo Stoakes @ 2015-03-10 15:25 UTC (permalink / raw)
To: sudipm.mukherjee, teddy.wang, gregkh
Cc: linux-fbdev, devel, linux-kernel, Lorenzo Stoakes
This patch assigns the more appropriate void* type to the mmio750 variable
eliminating an unnecessary volatile qualifier in the process. Additionally it
updates parameter types as necessary where those parameters interact with
mmio750, removes unnecessary casts and updates the type of the
lynx_share->pvReg field which is passed to the ddk750_set_mmio method.
As a consequence, this patch fixes the following sparse warning:-
drivers/staging/sm750fb/ddk750_help.c:12:17: warning: incorrect type in assignment (different address spaces)
Signed-off-by: Lorenzo Stoakes <lstoakes@gmail.com>
---
drivers/staging/sm750fb/ddk750_chip.h | 4 +++-
drivers/staging/sm750fb/ddk750_help.c | 4 ++--
drivers/staging/sm750fb/ddk750_help.h | 10 +++++-----
drivers/staging/sm750fb/sm750.h | 2 +-
4 files changed, 11 insertions(+), 9 deletions(-)
diff --git a/drivers/staging/sm750fb/ddk750_chip.h b/drivers/staging/sm750fb/ddk750_chip.h
index d761b72..04cb0d5 100644
--- a/drivers/staging/sm750fb/ddk750_chip.h
+++ b/drivers/staging/sm750fb/ddk750_chip.h
@@ -5,6 +5,8 @@
#define SM750LE_REVISION_ID ((unsigned char)0xfe)
#endif
+#include <linux/io.h>
+
/* This is all the chips recognized by this library */
typedef enum _logical_chip_type_t
{
@@ -72,7 +74,7 @@ logical_chip_type_t getChipType(void);
unsigned int calcPllValue(unsigned int request,pll_value_t *pll);
unsigned int calcPllValue2(unsigned int,pll_value_t *);
unsigned int formatPllReg(pll_value_t *pPLL);
-void ddk750_set_mmio(volatile unsigned char *,unsigned short,char);
+void ddk750_set_mmio(void __iomem *,unsigned short,char);
unsigned int ddk750_getVMSize(void);
int ddk750_initHw(initchip_param_t *);
unsigned int getPllValue(clock_type_t clockType, pll_value_t *pPLL);
diff --git a/drivers/staging/sm750fb/ddk750_help.c b/drivers/staging/sm750fb/ddk750_help.c
index cc00d2b..c68ff3b 100644
--- a/drivers/staging/sm750fb/ddk750_help.c
+++ b/drivers/staging/sm750fb/ddk750_help.c
@@ -2,12 +2,12 @@
//#include "ddk750_chip.h"
#include "ddk750_help.h"
-volatile unsigned char __iomem * mmio750 = NULL;
+void __iomem * mmio750 = NULL;
char revId750 = 0;
unsigned short devId750 = 0;
/* after driver mapped io registers, use this function first */
-void ddk750_set_mmio(volatile unsigned char * addr,unsigned short devId,char revId)
+void ddk750_set_mmio(void __iomem * addr,unsigned short devId,char revId)
{
mmio750 = addr;
devId750 = devId;
diff --git a/drivers/staging/sm750fb/ddk750_help.h b/drivers/staging/sm750fb/ddk750_help.h
index 4fc93b5..07c8264 100644
--- a/drivers/staging/sm750fb/ddk750_help.h
+++ b/drivers/staging/sm750fb/ddk750_help.h
@@ -12,14 +12,14 @@
#if 0
/* if 718 big endian turned on,be aware that don't use this driver for general use,only for ppc big-endian */
#warning "big endian on target cpu and enable nature big endian support of 718 capability !"
-#define PEEK32(addr) __raw_readl((void __iomem *)(mmio750)+(addr))
-#define POKE32(addr,data) __raw_writel((data),(void __iomem*)(mmio750)+(addr))
+#define PEEK32(addr) __raw_readl(mmio750 + addr)
+#define POKE32(addr,data) __raw_writel(data, mmio750 + addr)
#else /* software control endianess */
-#define PEEK32(addr) readl((addr)+mmio750)
-#define POKE32(addr,data) writel((data),(addr)+mmio750)
+#define PEEK32(addr) readl(addr + mmio750)
+#define POKE32(addr,data) writel(data, addr + mmio750)
#endif
-extern volatile unsigned char __iomem * mmio750;
+extern void __iomem * mmio750;
extern char revId750;
extern unsigned short devId750;
#else
diff --git a/drivers/staging/sm750fb/sm750.h b/drivers/staging/sm750fb/sm750.h
index d39968c..5361116 100644
--- a/drivers/staging/sm750fb/sm750.h
+++ b/drivers/staging/sm750fb/sm750.h
@@ -63,7 +63,7 @@ struct lynx_share{
unsigned long vidreg_start;
__u32 vidmem_size;
__u32 vidreg_size;
- volatile unsigned char __iomem * pvReg;
+ void __iomem * pvReg;
unsigned char __iomem * pvMem;
/* locks*/
spinlock_t slock;
--
2.3.2
^ permalink raw reply related
* Re: [PATCH v2] staging: sm750fb: Cleanup the type of mmio750
From: Greg KH @ 2015-03-10 15:09 UTC (permalink / raw)
To: Lorenzo Stoakes
Cc: sudipm.mukherjee, teddy.wang, devel, linux-fbdev, linux-kernel
In-Reply-To: <1425994068-8535-1-git-send-email-lstoakes@gmail.com>
On Tue, Mar 10, 2015 at 01:27:48PM +0000, Lorenzo Stoakes wrote:
> This patch assigns the more appropriate void* type to the mmio750 variable
> eliminating an unnecessary volatile qualifier in the process. Additionally it
> updates parameter types as necessary where those parameters interact with
> mmio750, removes unnecessary casts and updates the type of the
> lynx_share->pvReg field which is passed to the ddk750_set_mmio method.
>
> As a consequence, this patch fixes the following sparse warning:-
>
> drivers/staging/sm750fb/ddk750_help.c:12:17: warning: incorrect type in assignment (different address spaces)
>
> Signed-off-by: Lorenzo Stoakes <lstoakes@gmail.com>
>
> ---
> drivers/staging/sm750fb/ddk750_chip.h | 4 +++-
> drivers/staging/sm750fb/ddk750_help.c | 4 ++--
> drivers/staging/sm750fb/ddk750_help.h | 10 +++++-----
> drivers/staging/sm750fb/sm750.h | 2 +-
> 4 files changed, 11 insertions(+), 9 deletions(-)
This doesn't apply either anymore, for the same reason :(
^ permalink raw reply
* Re: [PATCH] staging: sm750fb: Cleanup the type of mmio750
From: Lorenzo Stoakes @ 2015-03-10 15:08 UTC (permalink / raw)
To: Greg KH
Cc: Sudip Mukherjee, devel, linux-fbdev, teddy.wang, linux-kernel,
Dan Carpenter
In-Reply-To: <20150310150454.GA22451@kroah.com>
On 10 March 2015 at 15:04, Greg KH <gregkh@linuxfoundation.org> wrote:
> I can't apply patches that add new build warnings, sorry. Please fix
> this up in the patch itself.
>
> greg k-h
Hi Greg,
Apologies for this, I've resolved this issue in v2 of the patch, no
warning messages are added in the updated version of this patch.
Best,
--
Lorenzo Stoakes
https:/ljs.io
^ permalink raw reply
* Re: [PATCH v2] staging: sm750fb: Fix sparse warning
From: Greg KH @ 2015-03-10 15:06 UTC (permalink / raw)
To: Lorenzo Stoakes
Cc: sudipm.mukherjee, teddy.wang, devel, linux-fbdev, linux-kernel
In-Reply-To: <1425977519-4810-1-git-send-email-lstoakes@gmail.com>
On Tue, Mar 10, 2015 at 08:51:59AM +0000, Lorenzo Stoakes wrote:
> This patch fixes the following sparse warning:-
>
> drivers/staging/sm750fb/ddk750_help.c: warning: incorrect type in assignment (different address spaces)
>
> In addition it eliminates an unnecessary volatile.
This doesn't apply anymore due to other patches I just took, so can you
rebase this patch on my latest staging-testing branch of staging.git?
Also, how about someone fixing the real compiler warnings this driver is
spitting out first, before we worry about sparse warnings? That's much
more relevant here as it's keeping the driver from being built in
linux-next at the moment.
thanks,
greg k-h
^ permalink raw reply
* Re: [PATCH] staging: sm750fb: Cleanup the type of mmio750
From: Greg KH @ 2015-03-10 15:04 UTC (permalink / raw)
To: Lorenzo Stoakes
Cc: Sudip Mukherjee, devel, linux-fbdev, teddy.wang, linux-kernel,
Dan Carpenter
In-Reply-To: <CAA5enKZMZDVzEjZG1mccn089MEN3AvSQc+TECc6vMZBKB06TRA@mail.gmail.com>
On Tue, Mar 10, 2015 at 12:47:44PM +0000, Lorenzo Stoakes wrote:
> On 10 March 2015 at 12:36, Sudip Mukherjee <sudipm.mukherjee@gmail.com> wrote:
> > but it is introducing two new build warnings:
> >
> > drivers/staging/sm750fb/sm750_hw.c: In function ‘hw_sm750_map’:
> > drivers/staging/sm750fb/sm750_hw.c:67:2: warning: passing argument 1 of ‘ddk750_set_mmio’ discards ‘volatile’ qualifier from pointer target type [enabled by default]
> > In file included from drivers/staging/sm750fb/ddk750_mode.h:4:0,
> > from drivers/staging/sm750fb/ddk750.h:15,
> > from drivers/staging/sm750fb/sm750_hw.c:24:
> >
> > and
> >
> > drivers/staging/sm750fb/ddk750_chip.h:77:6: note: expected ‘void *’ but argument is of type ‘volatile unsigned char *’
> >
> > care to make another patch to solve these two new warnings, and send this patch and the new one in a series and while sending mark the version number in the subject.
>
> I think the second warning is simply additional information attached
> to the 1st to give context?
>
> I noticed this issue but felt changing the type of this field would
> sit outside the purview of this patch as then I'm not only changing
> the type of mmio750 and code that *directly* interacts with this
> variable, but also code that indirectly interacts with it, so I felt
> that should perhaps be a separate patch.
>
> I'd love to additionally provide some further patches to help out with
> issues here too, incidentally! I will try to prepare some further
> patches tonight in this vein.
I can't apply patches that add new build warnings, sorry. Please fix
this up in the patch itself.
greg k-h
^ permalink raw reply
* Re: [PATCH] video: treat signal like timeout as failure
From: Tomi Valkeinen @ 2015-03-10 14:55 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20150310144640.GM8656@n2100.arm.linux.org.uk>
[-- Attachment #1: Type: text/plain, Size: 1077 bytes --]
On 10/03/15 16:46, Russell King - ARM Linux wrote:
> In which case, let me propose that the exynos fbdev driver needs to be
> moved to drivers/staging, and stay there until this stuff gets fixed.
> drivers/staging is supposed to be for stuff which isn't up to the mark,
> and which is potentially unstable. And that's what this driver exactly
> is.
There is drivers/gpu/drm/exynos/ which is getting a lot of updates. So...
I'd propose removing the exynos fbdev driver if the exynos drm driver
offers the same functionality. I don't know if that's the case. Does the
drm driver support all the devices the fbdev supports?
Also, I'm not sure if and how we can remove drivers. If exynos fbdev
driver is dropped, that would perhaps break boards that have exynos
fbdev in their .dts file. And if the drm driver doesn't offer the exact
same /dev/fbX interface, it would break the userspace.
So I don't know if that's possible. But that's what I'd like to do,
eventually, for all the fbdev drivers. Implement drm driver, remove the
fbdev one.
Tomi
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* Re: [PATCH] video: treat signal like timeout as failure
From: Russell King - ARM Linux @ 2015-03-10 14:46 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20150310143928.GA19501@opentech.at>
On Tue, Mar 10, 2015 at 03:39:28PM +0100, Nicholas Mc Guire wrote:
> On Tue, 10 Mar 2015, Russell King - ARM Linux wrote:
> > On Tue, Mar 10, 2015 at 01:51:16PM +0100, Nicholas Mc Guire wrote:
> > > On Tue, 10 Mar 2015, Tomi Valkeinen wrote:
> > >
> > > > On 20/01/15 07:23, Nicholas Mc Guire wrote:
> > > > > if(!wait_for_completion_interruptible_timeout(...))
> > > > > only handles the timeout case - this patch adds handling the
> > > > > signal case the same as timeout and cleans up.
> > > > >
> > > > > Signed-off-by: Nicholas Mc Guire <der.herr@hofr.at>
> > > > > ---
> > > > >
> > > > > Only the timeout case was being handled, return of 0 in
> > > > > wait_for_completion_interruptible_timeout, the signal case (-ERESTARTSYS)
> > > > > was treated just like the case of successful completion, which is most
> > > > > likely not reasonable.
> > > > >
> > > > > Note that exynos_mipi_dsi_wr_data/exynos_mipi_dsi_rd_data return values
> > > > > are not checked at the call sites in s6e8ax0.c (cmd_read/cmd_write)!
> > > > >
> > > > > This patch simply treats the signal case the same way as the timeout case,
> > > > > by releasing locks and returning 0 - which might not be the right thing to
> > > > > do - this needs a review by someone knowing the details of this driver.
> > > >
> > > > While I agree that this patch is a bit better than the current state,
> > > > the code still looks wrong as Russell said.
> > > >
> > > > I can merge this, but I'd rather have someone from Samsung look at the
> > > > code and change it to use wait_for_completion_killable_timeout() if
> > > > that's what this code is really supposed to use.
> > > >
> > > If someone that knows the details takes care of it
> > > that is of course the best solution. If someone Samsung is
> > > going to look into it then it is probably best to completly
> > > drop this speculative patch so that this does not lead
> > > to more confusion than it does good.
> >
> > IMHO, just change it to wait_for_completion_killable_timeout() - that's
> > a much better change than the change you're proposing.
> >
> > If we think about it... The current code uses this:
> >
> > if (!wait_for_completion_interruptible_timeout(&dsim_wr_comp,
> > MIPI_FIFO_TIMEOUT)) {
> > dev_warn(dsim->dev, "command write timeout.\n");
> > mutex_unlock(&dsim->lock);
> > return -EAGAIN;
> > }
> >
> > which has the effect of treating a signal as "success", and doesn't return
> > an error. So, if the calling application receives (eg) a SIGPIPE or a
> > SIGALRM, we proceed as if we received the FIFO empty interrupt and doesn't
> > cause an error.
> >
> > Your change results in:
> >
> > timeout = wait_for_completion_interruptible_timeout(
> > &dsim_wr_comp, MIPI_FIFO_TIMEOUT);
> > if (timeout <= 0) {
> > dev_warn(dsim->dev,
> > "command write timed-out/interrupted.\n");
> > mutex_unlock(&dsim->lock);
> > return -EAGAIN;
> > }
> >
> > which now means that this call returns -EAGAIN when a signal is raised.
>
> but in case of wait_for_completion_killable_timeout it also would return
> -ERESTARTSYS (unless I'm missreading do_wait_for_common -> signal_pending_state(state, current)) so I still think it would be better to have the
> dev_warn() in the path and then when the task is killed it atleast leaves
> some trace of the of what was going on ?
>
> >
> > Now, further auditing of this exynos crap (and I really do mean crap)
> > shows that this function is assigned to a method called "cmd_write".
> > Grepping for that shows that *no caller ever checks the return value*!
> >
>
> yup - as was noted in the patch - and this is also why it was
> not really possible to figure out what should really be done
> as it runs into a dead end in all cases - the only point of the patch was
> to atleast generate a debug message and return some signal
> indicating error ... which is then unhandled...
>
> > So, really, there's a bug here in that we should _never_ complete on a
> > signal, and we most *definitely can not* error out on a signal either.
> > The *only* sane change to this code without author/maintainer input is
> > to change this to wait_for_completion_killable_timeout() - so that
> > signals do not cause either premature completion nor premature failure
> > of the wait.
> >
> > The proper fix is absolutely huge: all call paths need to be augmented
> > with code to detect this function failing, and back out whatever changes
> > they've made, and restoring the previous state (if they can) and
> > propagate the error all the way back to userland, so that syscall
> > restarting can work correctly. _Only then_ is it safe to use a call
> > which causes an interruptible sleep.
> >
> > Personally, I'd be happier seeing this moved into drivers/staging and
> > eventually deleted from the kernel unless someone is willing to review
> > the driver and fix some of these glaring problems. I wouldn't be
> > surprised if there was _loads_ of this kind of crap there.
> >
> there is plenty of this - actually all of the wait_for_completion* related
> findings I've been posting in the past 2 month are based on the attempt to
> write up a more or less complete API spec in form of coccinelle scripts that
> then can be used to scan and sometimes fix-up this kind of problems - but of
> course just "local-fixes" - this can't fix fundamentally broken code.
In which case, let me propose that the exynos fbdev driver needs to be
moved to drivers/staging, and stay there until this stuff gets fixed.
drivers/staging is supposed to be for stuff which isn't up to the mark,
and which is potentially unstable. And that's what this driver exactly
is.
--
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.
^ permalink raw reply
* Re: [PATCH] video: treat signal like timeout as failure
From: Nicholas Mc Guire @ 2015-03-10 14:39 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20150310141511.GL8656@n2100.arm.linux.org.uk>
On Tue, 10 Mar 2015, Russell King - ARM Linux wrote:
> On Tue, Mar 10, 2015 at 01:51:16PM +0100, Nicholas Mc Guire wrote:
> > On Tue, 10 Mar 2015, Tomi Valkeinen wrote:
> >
> > > On 20/01/15 07:23, Nicholas Mc Guire wrote:
> > > > if(!wait_for_completion_interruptible_timeout(...))
> > > > only handles the timeout case - this patch adds handling the
> > > > signal case the same as timeout and cleans up.
> > > >
> > > > Signed-off-by: Nicholas Mc Guire <der.herr@hofr.at>
> > > > ---
> > > >
> > > > Only the timeout case was being handled, return of 0 in
> > > > wait_for_completion_interruptible_timeout, the signal case (-ERESTARTSYS)
> > > > was treated just like the case of successful completion, which is most
> > > > likely not reasonable.
> > > >
> > > > Note that exynos_mipi_dsi_wr_data/exynos_mipi_dsi_rd_data return values
> > > > are not checked at the call sites in s6e8ax0.c (cmd_read/cmd_write)!
> > > >
> > > > This patch simply treats the signal case the same way as the timeout case,
> > > > by releasing locks and returning 0 - which might not be the right thing to
> > > > do - this needs a review by someone knowing the details of this driver.
> > >
> > > While I agree that this patch is a bit better than the current state,
> > > the code still looks wrong as Russell said.
> > >
> > > I can merge this, but I'd rather have someone from Samsung look at the
> > > code and change it to use wait_for_completion_killable_timeout() if
> > > that's what this code is really supposed to use.
> > >
> > If someone that knows the details takes care of it
> > that is of course the best solution. If someone Samsung is
> > going to look into it then it is probably best to completly
> > drop this speculative patch so that this does not lead
> > to more confusion than it does good.
>
> IMHO, just change it to wait_for_completion_killable_timeout() - that's
> a much better change than the change you're proposing.
>
> If we think about it... The current code uses this:
>
> if (!wait_for_completion_interruptible_timeout(&dsim_wr_comp,
> MIPI_FIFO_TIMEOUT)) {
> dev_warn(dsim->dev, "command write timeout.\n");
> mutex_unlock(&dsim->lock);
> return -EAGAIN;
> }
>
> which has the effect of treating a signal as "success", and doesn't return
> an error. So, if the calling application receives (eg) a SIGPIPE or a
> SIGALRM, we proceed as if we received the FIFO empty interrupt and doesn't
> cause an error.
>
> Your change results in:
>
> timeout = wait_for_completion_interruptible_timeout(
> &dsim_wr_comp, MIPI_FIFO_TIMEOUT);
> if (timeout <= 0) {
> dev_warn(dsim->dev,
> "command write timed-out/interrupted.\n");
> mutex_unlock(&dsim->lock);
> return -EAGAIN;
> }
>
> which now means that this call returns -EAGAIN when a signal is raised.
but in case of wait_for_completion_killable_timeout it also would return
-ERESTARTSYS (unless I'm missreading do_wait_for_common -> signal_pending_state(state, current)) so I still think it would be better to have the
dev_warn() in the path and then when the task is killed it atleast leaves
some trace of the of what was going on ?
>
> Now, further auditing of this exynos crap (and I really do mean crap)
> shows that this function is assigned to a method called "cmd_write".
> Grepping for that shows that *no caller ever checks the return value*!
>
yup - as was noted in the patch - and this is also why it was
not really possible to figure out what should really be done
as it runs into a dead end in all cases - the only point of the patch was
to atleast generate a debug message and return some signal
indicating error ... which is then unhandled...
> So, really, there's a bug here in that we should _never_ complete on a
> signal, and we most *definitely can not* error out on a signal either.
> The *only* sane change to this code without author/maintainer input is
> to change this to wait_for_completion_killable_timeout() - so that
> signals do not cause either premature completion nor premature failure
> of the wait.
>
> The proper fix is absolutely huge: all call paths need to be augmented
> with code to detect this function failing, and back out whatever changes
> they've made, and restoring the previous state (if they can) and
> propagate the error all the way back to userland, so that syscall
> restarting can work correctly. _Only then_ is it safe to use a call
> which causes an interruptible sleep.
>
> Personally, I'd be happier seeing this moved into drivers/staging and
> eventually deleted from the kernel unless someone is willing to review
> the driver and fix some of these glaring problems. I wouldn't be
> surprised if there was _loads_ of this kind of crap there.
>
there is plenty of this - actually all of the wait_for_completion* related
findings I've been posting in the past 2 month are based on the attempt to
write up a more or less complete API spec in form of coccinelle scripts that
then can be used to scan and sometimes fix-up this kind of problems - but of
course just "local-fixes" - this can't fix fundamentally broken code.
thx!
hofrat
^ permalink raw reply
* Re: [PATCH] video: treat signal like timeout as failure
From: Russell King - ARM Linux @ 2015-03-10 14:15 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20150310125116.GA26020@opentech.at>
On Tue, Mar 10, 2015 at 01:51:16PM +0100, Nicholas Mc Guire wrote:
> On Tue, 10 Mar 2015, Tomi Valkeinen wrote:
>
> > On 20/01/15 07:23, Nicholas Mc Guire wrote:
> > > if(!wait_for_completion_interruptible_timeout(...))
> > > only handles the timeout case - this patch adds handling the
> > > signal case the same as timeout and cleans up.
> > >
> > > Signed-off-by: Nicholas Mc Guire <der.herr@hofr.at>
> > > ---
> > >
> > > Only the timeout case was being handled, return of 0 in
> > > wait_for_completion_interruptible_timeout, the signal case (-ERESTARTSYS)
> > > was treated just like the case of successful completion, which is most
> > > likely not reasonable.
> > >
> > > Note that exynos_mipi_dsi_wr_data/exynos_mipi_dsi_rd_data return values
> > > are not checked at the call sites in s6e8ax0.c (cmd_read/cmd_write)!
> > >
> > > This patch simply treats the signal case the same way as the timeout case,
> > > by releasing locks and returning 0 - which might not be the right thing to
> > > do - this needs a review by someone knowing the details of this driver.
> >
> > While I agree that this patch is a bit better than the current state,
> > the code still looks wrong as Russell said.
> >
> > I can merge this, but I'd rather have someone from Samsung look at the
> > code and change it to use wait_for_completion_killable_timeout() if
> > that's what this code is really supposed to use.
> >
> If someone that knows the details takes care of it
> that is of course the best solution. If someone Samsung is
> going to look into it then it is probably best to completly
> drop this speculative patch so that this does not lead
> to more confusion than it does good.
IMHO, just change it to wait_for_completion_killable_timeout() - that's
a much better change than the change you're proposing.
If we think about it... The current code uses this:
if (!wait_for_completion_interruptible_timeout(&dsim_wr_comp,
MIPI_FIFO_TIMEOUT)) {
dev_warn(dsim->dev, "command write timeout.\n");
mutex_unlock(&dsim->lock);
return -EAGAIN;
}
which has the effect of treating a signal as "success", and doesn't return
an error. So, if the calling application receives (eg) a SIGPIPE or a
SIGALRM, we proceed as if we received the FIFO empty interrupt and doesn't
cause an error.
Your change results in:
timeout = wait_for_completion_interruptible_timeout(
&dsim_wr_comp, MIPI_FIFO_TIMEOUT);
if (timeout <= 0) {
dev_warn(dsim->dev,
"command write timed-out/interrupted.\n");
mutex_unlock(&dsim->lock);
return -EAGAIN;
}
which now means that this call returns -EAGAIN when a signal is raised.
Now, further auditing of this exynos crap (and I really do mean crap)
shows that this function is assigned to a method called "cmd_write".
Grepping for that shows that *no caller ever checks the return value*!
So, really, there's a bug here in that we should _never_ complete on a
signal, and we most *definitely can not* error out on a signal either.
The *only* sane change to this code without author/maintainer input is
to change this to wait_for_completion_killable_timeout() - so that
signals do not cause either premature completion nor premature failure
of the wait.
The proper fix is absolutely huge: all call paths need to be augmented
with code to detect this function failing, and back out whatever changes
they've made, and restoring the previous state (if they can) and
propagate the error all the way back to userland, so that syscall
restarting can work correctly. _Only then_ is it safe to use a call
which causes an interruptible sleep.
Personally, I'd be happier seeing this moved into drivers/staging and
eventually deleted from the kernel unless someone is willing to review
the driver and fix some of these glaring problems. I wouldn't be
surprised if there was _loads_ of this kind of crap there.
--
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.
^ permalink raw reply
* [PATCH v2] staging: sm750fb: Cleanup the type of mmio750
From: Lorenzo Stoakes @ 2015-03-10 13:27 UTC (permalink / raw)
To: sudipm.mukherjee, teddy.wang, gregkh
Cc: linux-fbdev, devel, linux-kernel, Lorenzo Stoakes
This patch assigns the more appropriate void* type to the mmio750 variable
eliminating an unnecessary volatile qualifier in the process. Additionally it
updates parameter types as necessary where those parameters interact with
mmio750, removes unnecessary casts and updates the type of the
lynx_share->pvReg field which is passed to the ddk750_set_mmio method.
As a consequence, this patch fixes the following sparse warning:-
drivers/staging/sm750fb/ddk750_help.c:12:17: warning: incorrect type in assignment (different address spaces)
Signed-off-by: Lorenzo Stoakes <lstoakes@gmail.com>
---
drivers/staging/sm750fb/ddk750_chip.h | 4 +++-
drivers/staging/sm750fb/ddk750_help.c | 4 ++--
drivers/staging/sm750fb/ddk750_help.h | 10 +++++-----
drivers/staging/sm750fb/sm750.h | 2 +-
4 files changed, 11 insertions(+), 9 deletions(-)
diff --git a/drivers/staging/sm750fb/ddk750_chip.h b/drivers/staging/sm750fb/ddk750_chip.h
index 1c78875..d067b06 100644
--- a/drivers/staging/sm750fb/ddk750_chip.h
+++ b/drivers/staging/sm750fb/ddk750_chip.h
@@ -3,6 +3,8 @@
#define DEFAULT_INPUT_CLOCK 14318181 /* Default reference clock */
#define SM750LE_REVISION_ID (char)0xfe
+#include <linux/io.h>
+
/* This is all the chips recognized by this library */
typedef enum _logical_chip_type_t
{
@@ -70,7 +72,7 @@ logical_chip_type_t getChipType(void);
unsigned int calcPllValue(unsigned int request,pll_value_t *pll);
unsigned int calcPllValue2(unsigned int,pll_value_t *);
unsigned int formatPllReg(pll_value_t *pPLL);
-void ddk750_set_mmio(volatile unsigned char *,unsigned short,char);
+void ddk750_set_mmio(void __iomem *,unsigned short,char);
unsigned int ddk750_getVMSize(void);
int ddk750_initHw(initchip_param_t *);
unsigned int getPllValue(clock_type_t clockType, pll_value_t *pPLL);
diff --git a/drivers/staging/sm750fb/ddk750_help.c b/drivers/staging/sm750fb/ddk750_help.c
index cc00d2b..c68ff3b 100644
--- a/drivers/staging/sm750fb/ddk750_help.c
+++ b/drivers/staging/sm750fb/ddk750_help.c
@@ -2,12 +2,12 @@
//#include "ddk750_chip.h"
#include "ddk750_help.h"
-volatile unsigned char __iomem * mmio750 = NULL;
+void __iomem * mmio750 = NULL;
char revId750 = 0;
unsigned short devId750 = 0;
/* after driver mapped io registers, use this function first */
-void ddk750_set_mmio(volatile unsigned char * addr,unsigned short devId,char revId)
+void ddk750_set_mmio(void __iomem * addr,unsigned short devId,char revId)
{
mmio750 = addr;
devId750 = devId;
diff --git a/drivers/staging/sm750fb/ddk750_help.h b/drivers/staging/sm750fb/ddk750_help.h
index 4fc93b5..07c8264 100644
--- a/drivers/staging/sm750fb/ddk750_help.h
+++ b/drivers/staging/sm750fb/ddk750_help.h
@@ -12,14 +12,14 @@
#if 0
/* if 718 big endian turned on,be aware that don't use this driver for general use,only for ppc big-endian */
#warning "big endian on target cpu and enable nature big endian support of 718 capability !"
-#define PEEK32(addr) __raw_readl((void __iomem *)(mmio750)+(addr))
-#define POKE32(addr,data) __raw_writel((data),(void __iomem*)(mmio750)+(addr))
+#define PEEK32(addr) __raw_readl(mmio750 + addr)
+#define POKE32(addr,data) __raw_writel(data, mmio750 + addr)
#else /* software control endianess */
-#define PEEK32(addr) readl((addr)+mmio750)
-#define POKE32(addr,data) writel((data),(addr)+mmio750)
+#define PEEK32(addr) readl(addr + mmio750)
+#define POKE32(addr,data) writel(data, addr + mmio750)
#endif
-extern volatile unsigned char __iomem * mmio750;
+extern void __iomem * mmio750;
extern char revId750;
extern unsigned short devId750;
#else
diff --git a/drivers/staging/sm750fb/sm750.h b/drivers/staging/sm750fb/sm750.h
index 711676c..376df5f 100644
--- a/drivers/staging/sm750fb/sm750.h
+++ b/drivers/staging/sm750fb/sm750.h
@@ -63,7 +63,7 @@ struct lynx_share{
resource_size_t vidreg_start;
resource_size_t vidmem_size;
resource_size_t vidreg_size;
- volatile unsigned char __iomem * pvReg;
+ void __iomem * pvReg;
unsigned char __iomem * pvMem;
/* locks*/
spinlock_t slock;
--
2.3.2
^ permalink raw reply related
* Re: [PATCH] staging: sm750fb: Cleanup the type of mmio750
From: Lorenzo Stoakes @ 2015-03-10 13:22 UTC (permalink / raw)
To: Dan Carpenter
Cc: Sudip Mukherjee, teddy.wang, Greg KH, devel, linux-fbdev,
linux-kernel
In-Reply-To: <20150310130624.GC16501@mwanda>
On 10 March 2015 at 13:06, Dan Carpenter <dan.carpenter@oracle.com> wrote:
> You should have said that in the patch description or under the ---
> cut off. But anyway, it's not ok. And we'll need to redo this patch.
> Breaking up patches into logical changes is sort of tricky because
> everything touches everything else so the patch gets larger and larger.
>
Major apologies, I am still getting used to kernel development! I'll
be careful to not make such assumptions in future when it comes to
warnings/errors.
[snip]
> But maybe it's only one line larger than the patch you just send? In
> that case just fold it in and don't do the temporary casting.
>
> The next patch after that could get rid of all the ramaining "volatile"
> keywords.
It seems that we can in fact fix this problem with a single additional
change, I will submit a v2 shortly.
Best,
--
Lorenzo Stoakes
https:/ljs.io
^ permalink raw reply
* Re: [PATCH] staging: sm750fb: Cleanup the type of mmio750
From: Dan Carpenter @ 2015-03-10 13:06 UTC (permalink / raw)
To: Lorenzo Stoakes
Cc: Sudip Mukherjee, teddy.wang, Greg KH, devel, linux-fbdev,
linux-kernel
In-Reply-To: <CAA5enKZMZDVzEjZG1mccn089MEN3AvSQc+TECc6vMZBKB06TRA@mail.gmail.com>
On Tue, Mar 10, 2015 at 12:47:44PM +0000, Lorenzo Stoakes wrote:
> On 10 March 2015 at 12:36, Sudip Mukherjee <sudipm.mukherjee@gmail.com> wrote:
> > but it is introducing two new build warnings:
> >
> > drivers/staging/sm750fb/sm750_hw.c: In function ‘hw_sm750_map’:
> > drivers/staging/sm750fb/sm750_hw.c:67:2: warning: passing argument 1 of ‘ddk750_set_mmio’ discards ‘volatile’ qualifier from pointer target type [enabled by default]
> > In file included from drivers/staging/sm750fb/ddk750_mode.h:4:0,
> > from drivers/staging/sm750fb/ddk750.h:15,
> > from drivers/staging/sm750fb/sm750_hw.c:24:
> >
> > and
> >
> > drivers/staging/sm750fb/ddk750_chip.h:77:6: note: expected ‘void *’ but argument is of type ‘volatile unsigned char *’
> >
> > care to make another patch to solve these two new warnings, and send this patch and the new one in a series and while sending mark the version number in the subject.
>
> I think the second warning is simply additional information attached
> to the 1st to give context?
>
> I noticed this issue but felt changing the type of this field would
> sit outside the purview of this patch as then I'm not only changing
> the type of mmio750 and code that *directly* interacts with this
> variable, but also code that indirectly interacts with it, so I felt
> that should perhaps be a separate patch.
You should have said that in the patch description or under the ---
cut off. But anyway, it's not ok. And we'll need to redo this patch.
Breaking up patches into logical changes is sort of tricky because
everything touches everything else so the patch gets larger and larger.
You could maybe break it up:
[patch 1/2] staging: sm750fb: Cleanup the type of mmio750
This would add some temporary casting until the rest of the
code was cleaned up. It wouldn't touch change the function
parameters of ddk750_set_mmio().
[patch 2/2] staging: sm750fb: Cleanup the types for ddk750_set_mmio()
This would change the function paramters and the type for
->pvReg and remove the temporary casts.
But maybe it's only one line larger than the patch you just send? In
that case just fold it in and don't do the temporary casting.
The next patch after that could get rid of all the ramaining "volatile"
keywords.
regards,
dan carpenter
^ permalink raw reply
* Re: [PATCH] video: treat signal like timeout as failure
From: Nicholas Mc Guire @ 2015-03-10 12:51 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <54FEE70D.3010307@ti.com>
On Tue, 10 Mar 2015, Tomi Valkeinen wrote:
> On 20/01/15 07:23, Nicholas Mc Guire wrote:
> > if(!wait_for_completion_interruptible_timeout(...))
> > only handles the timeout case - this patch adds handling the
> > signal case the same as timeout and cleans up.
> >
> > Signed-off-by: Nicholas Mc Guire <der.herr@hofr.at>
> > ---
> >
> > Only the timeout case was being handled, return of 0 in
> > wait_for_completion_interruptible_timeout, the signal case (-ERESTARTSYS)
> > was treated just like the case of successful completion, which is most
> > likely not reasonable.
> >
> > Note that exynos_mipi_dsi_wr_data/exynos_mipi_dsi_rd_data return values
> > are not checked at the call sites in s6e8ax0.c (cmd_read/cmd_write)!
> >
> > This patch simply treats the signal case the same way as the timeout case,
> > by releasing locks and returning 0 - which might not be the right thing to
> > do - this needs a review by someone knowing the details of this driver.
>
> While I agree that this patch is a bit better than the current state,
> the code still looks wrong as Russell said.
>
> I can merge this, but I'd rather have someone from Samsung look at the
> code and change it to use wait_for_completion_killable_timeout() if
> that's what this code is really supposed to use.
>
If someone that knows the details takes care of it
that is of course the best solution. If someone Samsung is
going to look into it then it is probably best to completly
drop this speculative patch so that this does not lead
to more confusion than it does good.
thx!
hofrat
^ permalink raw reply
* Re: [PATCH] staging: sm750fb: Cleanup the type of mmio750
From: Sudip Mukherjee @ 2015-03-10 12:48 UTC (permalink / raw)
To: Dan Carpenter
Cc: Lorenzo Stoakes, teddy.wang, gregkh, devel, linux-fbdev,
linux-kernel
In-Reply-To: <20150310114030.GP10964@mwanda>
On Tue, Mar 10, 2015 at 02:40:30PM +0300, Dan Carpenter wrote:
> On Tue, Mar 10, 2015 at 09:57:06AM +0000, Lorenzo Stoakes wrote:
> > This patch assigns the more appropriate void* type to the mmio750 variable
> > eliminating an unnecessary volatile qualifier in the process. Additionally it
> > updates parameter types as necessary where those parameters interact with
> > mmio750 and removes unnecessary casts.
> >
> > As a consequence, this patch fixes the following sparse warning:-
> >
> > drivers/staging/sm750fb/ddk750_help.c:12:17: warning: incorrect type in assignment (different address spaces)
> >
> > Signed-off-by: Lorenzo Stoakes <lstoakes@gmail.com>
>
> Looks good. Thanks for doing this.
but it is introducing two new build warnings:
drivers/staging/sm750fb/sm750_hw.c: In function ‘hw_sm750_map’:
drivers/staging/sm750fb/sm750_hw.c:67:2: warning: passing argument 1 of ‘ddk750_set_mmio’ discards ‘volatile’ qualifier from pointer target type [enabled by default]
In file included from drivers/staging/sm750fb/ddk750_mode.h:4:0,
from drivers/staging/sm750fb/ddk750.h:15,
from drivers/staging/sm750fb/sm750_hw.c:24:
and
drivers/staging/sm750fb/ddk750_chip.h:77:6: note: expected ‘void *’ but argument is of type ‘volatile unsigned char *’
care to make another patch to solve these two new warnings, and send this patch and the new one in a series and while sending mark the version number in the subject.
regards
sudip
>
> regards,
> dan carpenter
>
^ permalink raw reply
* Re: [PATCH] staging: sm750fb: Cleanup the type of mmio750
From: Lorenzo Stoakes @ 2015-03-10 12:47 UTC (permalink / raw)
To: Sudip Mukherjee
Cc: Dan Carpenter, teddy.wang, Greg KH, devel, linux-fbdev,
linux-kernel
In-Reply-To: <20150310123633.GA2782@sudip-PC>
On 10 March 2015 at 12:36, Sudip Mukherjee <sudipm.mukherjee@gmail.com> wrote:
> but it is introducing two new build warnings:
>
> drivers/staging/sm750fb/sm750_hw.c: In function ‘hw_sm750_map’:
> drivers/staging/sm750fb/sm750_hw.c:67:2: warning: passing argument 1 of ‘ddk750_set_mmio’ discards ‘volatile’ qualifier from pointer target type [enabled by default]
> In file included from drivers/staging/sm750fb/ddk750_mode.h:4:0,
> from drivers/staging/sm750fb/ddk750.h:15,
> from drivers/staging/sm750fb/sm750_hw.c:24:
>
> and
>
> drivers/staging/sm750fb/ddk750_chip.h:77:6: note: expected ‘void *’ but argument is of type ‘volatile unsigned char *’
>
> care to make another patch to solve these two new warnings, and send this patch and the new one in a series and while sending mark the version number in the subject.
I think the second warning is simply additional information attached
to the 1st to give context?
I noticed this issue but felt changing the type of this field would
sit outside the purview of this patch as then I'm not only changing
the type of mmio750 and code that *directly* interacts with this
variable, but also code that indirectly interacts with it, so I felt
that should perhaps be a separate patch.
I'd love to additionally provide some further patches to help out with
issues here too, incidentally! I will try to prepare some further
patches tonight in this vein.
--
Lorenzo Stoakes
https:/ljs.io
^ permalink raw reply
* Re: [patch] fbdev: pm3fb: cleanup some confusing indenting
From: Tomi Valkeinen @ 2015-03-10 12:47 UTC (permalink / raw)
To: linux-fbdev
[-- Attachment #1: Type: text/plain, Size: 896 bytes --]
On 25/02/15 15:25, Dan Carpenter wrote:
> This if statement should be pushed out one tab to line up with the rest.
>
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
>
> diff --git a/drivers/video/fbdev/pm3fb.c b/drivers/video/fbdev/pm3fb.c
> index 4bf3273..77b99ed 100644
> --- a/drivers/video/fbdev/pm3fb.c
> +++ b/drivers/video/fbdev/pm3fb.c
> @@ -1479,9 +1479,9 @@ static void pm3fb_remove(struct pci_dev *dev)
> fb_dealloc_cmap(&info->cmap);
>
> #ifdef CONFIG_MTRR
> - if (par->mtrr_handle >= 0)
> - mtrr_del(par->mtrr_handle, info->fix.smem_start,
> - info->fix.smem_len);
> + if (par->mtrr_handle >= 0)
> + mtrr_del(par->mtrr_handle, info->fix.smem_start,
> + info->fix.smem_len);
> #endif /* CONFIG_MTRR */
> iounmap(info->screen_base);
> release_mem_region(fix->smem_start, fix->smem_len);
Thanks, queued for 4.1.
Tomi
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* Re: [PATCH] video: treat signal like timeout as failure
From: Tomi Valkeinen @ 2015-03-10 12:43 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1421731430-13207-1-git-send-email-der.herr@hofr.at>
[-- Attachment #1: Type: text/plain, Size: 1232 bytes --]
On 20/01/15 07:23, Nicholas Mc Guire wrote:
> if(!wait_for_completion_interruptible_timeout(...))
> only handles the timeout case - this patch adds handling the
> signal case the same as timeout and cleans up.
>
> Signed-off-by: Nicholas Mc Guire <der.herr@hofr.at>
> ---
>
> Only the timeout case was being handled, return of 0 in
> wait_for_completion_interruptible_timeout, the signal case (-ERESTARTSYS)
> was treated just like the case of successful completion, which is most
> likely not reasonable.
>
> Note that exynos_mipi_dsi_wr_data/exynos_mipi_dsi_rd_data return values
> are not checked at the call sites in s6e8ax0.c (cmd_read/cmd_write)!
>
> This patch simply treats the signal case the same way as the timeout case,
> by releasing locks and returning 0 - which might not be the right thing to
> do - this needs a review by someone knowing the details of this driver.
While I agree that this patch is a bit better than the current state,
the code still looks wrong as Russell said.
I can merge this, but I'd rather have someone from Samsung look at the
code and change it to use wait_for_completion_killable_timeout() if
that's what this code is really supposed to use.
Tomi
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox