login
Search
This patch actually contains quite a few fixes, but it addresses the
Kconfig issue as well. The linux-next tree does not seem to have a
config for the zoom2, and trying to build it for that board seems to
make the compilation break. As such, the only thing that I tested was
compilation using the two different HALs (one of which is added in this
patch). Please let me know if there are problems or questions with this.
Thanks,
David
Signed-off-by: David Cross <david.cross@cypress.com>
diff -uprN -X linux-next-vanilla/Documentation/dontdiff linux-next-vanilla/arch/arm/mach-omap2/gpmc.c linux-next-incl-sdk/arch/arm/mach-omap2/gpmc.c
--- linux-next-vanilla/arch/arm/mach-omap2/gpmc.c 2010-08-31 19:32:51.000000000 -0700
+++ linux-next-incl-sdk/arch/arm/mach-omap2/gpmc.c 2010-09-01 16:10:21.000000000 -0700
@@ -133,6 +133,7 @@ void gpmc_cs_write_reg(int cs, int idx,
reg_addr = gpmc_base + GPMC_CS0_OFFSET + (cs * GPMC_CS_SIZE) + idx;
__raw_writel(val, reg_addr);
}
+EXPORT_SYMBOL(gpmc_cs_write_reg);
u32 gpmc_cs_read_reg(int cs, int idx)
{
@@ -141,6 +142,7 @@ u32 gpmc_cs_read_reg(int cs, int idx)
reg_addr = gpmc_base + GPMC_CS0_OFFSET + (cs * GPMC_CS_SIZE) + idx;
return __raw_readl(reg_addr);
}
+EXPORT_SYMBOL(gpmc_cs_read_reg);
/* TODO: Add support for gpmc_fck to clock framework and use it */
unsigned long gpmc_get_fclk_period(void)
@@ -294,6 +296,7 @@ int gpmc_cs_set_timings(int cs, const st
return 0;
}
+EXPORT_SYMBOL(gpmc_cs_set_timings);
static void gpmc_cs_enable_mem(int cs, u32 base, u32 size)
{
diff -uprN -X linux-next-vanilla/Documentation/dontdiff linux-next-vanilla/drivers/staging/westbridge/astoria/arch/arm/mach-omap2/cyashalomap_cram.c linux-next-incl-sdk/drivers/staging/westbridge/astoria/arch/arm/mach-omap2/cyashalomap_cram.c
--- linux-next-vanilla/drivers/staging/westbridge/astoria/arch/arm/mach-omap2/cyashalomap_cram.c 1969-12-31 16:00:00.000000000 -0800
+++ ...Hm, I need a single-patch-per-change, as described in the Documentation/SubmittingPatches file. So, could you break this up into the logical changes, so that I can apply them all? That makes it easier for everyone involved (reviewing, bisecting, etc.) and it's what is needed for the rest of the kernel as well. thanks, greg k-h --
On Wed, 2010-09-01 at 17:08 -0700, David Cross wrote:
This patch actually contains the KConfig changes necessary an additional
HAL layer. The Kconfig changes are pretty closely related to this HAL
layer, as such this patch is difficult to logically separate. The HAL
layers also rely on the export of some gpmc functions, which are added
to this patch. Although I don't own this driver, it is difficult for me
to understand why there would be issues with exporting these symbols.
The linux-next tree does not seem to have a config for the zoom2, and
trying to build it for that board seems to make the compilation break.
As such, the only thing that I tested was compilation using the two
different HALs (one of which is added in this patch). Please let me know
if there are problems or questions with this.
Thanks,
David
Signed-off-by: David Cross <david.cross@cypress.com>
diff -uprN -X linux-next-vanilla/Documentation/dontdiff linux-next-vanilla/arch/arm/mach-omap2/gpmc.c linux-next-incl-sdk/arch/arm/mach-omap2/gpmc.c
--- linux-next-vanilla/arch/arm/mach-omap2/gpmc.c 2010-08-31 19:32:51.000000000 -0700
+++ linux-next-incl-sdk/arch/arm/mach-omap2/gpmc.c 2010-09-01 16:10:21.000000000 -0700
@@ -133,6 +133,7 @@ void gpmc_cs_write_reg(int cs, int idx,
reg_addr = gpmc_base + GPMC_CS0_OFFSET + (cs * GPMC_CS_SIZE) + idx;
__raw_writel(val, reg_addr);
}
+EXPORT_SYMBOL(gpmc_cs_write_reg);
u32 gpmc_cs_read_reg(int cs, int idx)
{
@@ -141,6 +142,7 @@ u32 gpmc_cs_read_reg(int cs, int idx)
reg_addr = gpmc_base + GPMC_CS0_OFFSET + (cs * GPMC_CS_SIZE) + idx;
return __raw_readl(reg_addr);
}
+EXPORT_SYMBOL(gpmc_cs_read_reg);
/* TODO: Add support for gpmc_fck to clock framework and use it */
unsigned long gpmc_get_fclk_period(void)
@@ -294,6 +296,7 @@ int gpmc_cs_set_timings(int cs, const st
return 0;
}
+EXPORT_SYMBOL(gpmc_cs_set_timings);
static void gpmc_cs_enable_mem(int cs, u32 base, u32 size)
{
diff -uprN -X linux-next-vanilla/Documentation/dontdiff ...Note, when sending a patch, don't include all of the stuff above, that makes no sense. Also, your Subject: should be a bit cleaner. For example, I would edit this subject from: Subject: Re: [PATCH] West Bridge Astoria Driver 2.6.35, Kconfig and HAL fixes to Subject: [PATCH] Staging: westbridge: Kconfig and HAL fixes You really need all of this at once to fix the Kconfig issues? It's a Sorry, I can't change things outside of the drivers/staging/ directory unless you get the maintainer/owner of this file to agree to export these symbols. Can you do that as a separate patch and get their ack? Then I can apply it. Also, how about "EXPORT_SYMBOL_GPL()" instead? So, care to redo this patch? thanks, greg k-h --
This patch contains the kconfig changes necessary to fix build errors that could come up in the linux-next version. It also includes an additional HAL layer for the west bridge CRAM interface. The inclusion of this interface support did require the reorganization of some of the existing code, which is part of the reason for the size of the patch. Moving files and directories makes this patch seem larger than it really is. The Kconfig changes are closely related to the inclusion of the CRAM HAL layer, and as such this patch is difficult to logically separate. The linux-next tree does not seem to have a config for the zoom2, and trying to build it for that board seems to make the compilation break. As such, the only thing that I tested was compilation using the two different HALs (one of which is added in this patch). Please let me know if there are problems or questions with this. Thanks, David Signed-off-by: David Cross <david.cross@cypress.com> diff -uprN -X linux-next-vanilla/Documentation/dontdiff linux-next-vanilla/drivers/staging/westbridge/astoria/arch/arm/mach-omap2/cyashalomap_cram.c linux-next-incl-sdk/drivers/staging/westbridge/astoria/arch/arm/mach-omap2/cyashalomap_cram.c --- linux-next-vanilla/drivers/staging/westbridge/astoria/arch/arm/mach-omap2/cyashalomap_cram.c 1969-12-31 16:00:00.000000000 -0800 +++ linux-next-incl-sdk/drivers/staging/westbridge/astoria/arch/arm/mach-omap2/cyashalomap_cram.c 2010-09-07 11:57:13.000000000 -0700 @@ -0,0 +1,1845 @@ +/* Cypress WestBridge OMAP3430 Kernel Hal source file (cyashalomap_kernel.c) +## =========================== +## Copyright (C) 2010 Cypress Semiconductor +## +## This program is free software; you can redistribute it and/or +## modify it under the terms of the GNU General Public License +## as published by the Free Software Foundation; either version 2 +## of the License, or (at your option) any later version. +## +## This program is distributed in the hope that it will be useful, +## but WITHOUT ANY WARRANTY; ...
On Tue, Sep 07, 2010 at 12:22:10PM -0700, David Cross wrote: First off, what's with the "Re:" of the Subject? What are you If you use git you can send a patch that properly shows only what Why should the driver care about which arch it is built for? It should Why do you need a "HAL" at all here? confused, greg k-h --
From: Greg KH [mailto:greg@kroah.com] I will removed that going forward when submitting patches, it does seem Can I break it up as 1) moving PNAND HAL layer + Kconfig changes and 2) CRAM HAL layer inclusion or is this still too tightly coupled? The issue that I am having is that I was working on these drivers while waiting on inclusion in the staging area in order to incorporate all of the feedback I have gotten so far. As such, I now either have to artificially break up the patches or start with a clean kernel and start sequentially hand coding them again. I think this latter might be what you want, but this is really time-consuming to do at this point. While I definitely understand the need for this type of review of patches for changes to the kernel itself, it is hard for me to understand why the same rigor needs to apply to this driver in the staging area. As you mentioned, it needs a lot of work to meet the standards for acceptance. I have done a fair amount of work to move it closer. Do I really need to undo and repeat all of that in order to clean up "BROKEN" code in the staging I have not used git so far, other than to pull from repositories. I probably could, but it will take me some time to learn. Is this "nice to have" or do The HAL layer in general or this specific HAL layer? One of the reasons for needing memory interface options is the allocation of pins on a given platform. For example, if CLE was used as a GPIO for some other purpose, it is nice to have the option to move to an SRAM like interface. Also, I needed to develop this for unrelated reasons. As such, making minor structural changes such that new HALs could be easily incorporated in an intuitive manner seemed to be a plus. I also hoped that it would make the structure and rationale behind the initial driver structure more intuitive to the It should build on all architectures that have an appropriate HAL implemented. Otherwise, if you don't have a method for low level For low level ...
Wait, look, you had a "and" in there. Why? Why not just do: - move PNAND HAL layer - Kconfig changes as individual patches. Well, it shouldn't take all that long of time, as you know what you want to accomplish. It's also how kernel development works, you usually go off and do a bunch of work, then step back and figure out how to break it up into logical chunks to submit it. Everyone does it. After time you will get used to doing things in logical steps so you don't have to There is no difference here for what type of patches I can accept in the staging area. Staging is for code that doesn't meet the "normal" acceptance guidelines, but that does not mean that the proper development process does not also apply here at all. It's a matter of both cleaning up the code, and cleaning up the developers who are It's marked "BROKEN" because it breaks the build. Once that is fixed we What are you using to develop your patches? If you don't use git, you can use quilt, or some other type of patch management system. But it might be worth spending a few hours to get up and running on one of them as this is going to be something you use a lot in the future, and it will help you get work done much faster and be able to work with the rest of the community better as well. Note that almost no other driver in the kernel needs such a layer. What you do is have a "backend" that talks to either GPIO or SRAM or something else. Then, at runtime, you can pick the one you need/want depending on the hardware the code is running on, and everything "just works". Remember, the goal is to be able to build your driver once for any platform it might run on. Don't rely on Kconfig options to select the hardware types, that's not the correct way to do things in the end at Testing that the build isn't broken? Building one image to run on lots of different platforms? Distros really need this and there is a large unification effort for ARM to get this to work for that ...
This patch contains update to the gadget driver, some of which are based on feedback from the Linux community concerning the usage of fat_get_block.
It also contains the addition of mpage_clear_dirty, a function used to clear dirty pages from the page cache. These two patches are also interdependent
and difficult to separate. As such, they are included as one patch.
Thanks,
David
Signed-off-by: David Cross <david.cross@cypress.com>
diff -uprN -X linux-next-vanilla/Documentation/dontdiff linux-next-vanilla/drivers/staging/westbridge/astoria/gadget/cyasgadget.c linux-next-incl-sdk/drivers/staging/westbridge/astoria/gadget/cyasgadget.c
--- linux-next-vanilla/drivers/staging/westbridge/astoria/gadget/cyasgadget.c 2010-08-31 19:32:51.000000000 -0700
+++ linux-next-incl-sdk/drivers/staging/westbridge/astoria/gadget/cyasgadget.c 2010-08-26 11:18:18.000000000 -0700
@@ -224,6 +224,8 @@ static void cy_as_gadget_mtp_event_callb
cy_as_mtp_send_object_complete_data *send_obj_data =
(cy_as_mtp_send_object_complete_data *) evdata ;
+ cy_as_release_common_lock();
+
#ifndef WESTBRIDGE_NDEBUG
cy_as_hal_print_message(
"<6>MTP EVENT: send_object_complete\n");
@@ -246,6 +248,8 @@ static void cy_as_gadget_mtp_event_callb
{
cy_as_mtp_get_object_complete_data *get_obj_data =
(cy_as_mtp_get_object_complete_data *) evdata ;
+;
+ cy_as_release_common_lock();
#ifndef WESTBRIDGE_NDEBUG
cy_as_hal_print_message(
@@ -752,6 +756,9 @@ static int cyasgadget_queue(
_req->status = -EINPROGRESS;
_req->actual = 0;
+ if (as_req)
+ list_add_tail(&as_req->queue, &as_ep->queue);
+
spin_unlock_irqrestore(&cy_as_dev->lock, flags);
/* Call Async functions */
@@ -772,16 +779,6 @@ static int cyasgadget_queue(
else
_req->status = -EALREADY ;
} else if (as_ep->num == 0) {
- /*
- ret = cy_as_usb_write_data_async(cy_as_dev->dev_handle,
- as_ep->num, _req->length, _req->buf, cy_false,
- cyasgadget_setupwritecallback) ;
-
- if (ret != ...Please wrap your lines at 72 columns. And is this really hard to separate? I can't take a patch that touches anything outside of drivers/staging/ at the moment. How about a patch here to fix up the locking issues, and the rest as a separate patch? Oh, you also added lines with trailing spaces and that's it, like the Why? I think you might want to make sure your editor doesn't do this in the future. Also, if you ran your patch through the scripts/checkpatch.pl script, it would have caught this error. Care to redo it? thanks, greg k-h --
This patch contains minor updates to the west bridge block and device
drivers, including the addition of a common lock and a minor update to
remove the blk_fs_request call based on a kernel update. These changes
could possibly be separated, but are very minor in scope and as such
have been submitted together.
Thanks,
David
Signed-off-by: David Cross <david.cross@cypress.com>
diff -uprN -X linux-next-vanilla/Documentation/dontdiff linux-next-vanilla/drivers/staging/westbridge/astoria/block/cyasblkdev_block.c linux-next-incl-sdk/drivers/staging/westbridge/astoria/block/cyasblkdev_block.c
--- linux-next-vanilla/drivers/staging/westbridge/astoria/block/cyasblkdev_block.c 2010-08-31 19:32:51.000000000 -0700
+++ linux-next-incl-sdk/drivers/staging/westbridge/astoria/block/cyasblkdev_block.c 2010-08-25 18:29:44.000000000 -0700
@@ -389,7 +389,7 @@ int cyasblkdev_media_changed(struct gend
struct cyasblkdev_blk_data *bd;
#ifndef WESTBRIDGE_NDEBUG
- cy_as_hal_print_message("cyasblkdev_media_changed() is called\n");
+ cy_as_hal_print_message(KERN_INFO"cyasblkdev_media_changed() is called\n");
#endif
if (gd)
@@ -466,7 +466,7 @@ static int cyasblkdev_blk_prep_rq(
/* Check for excessive requests.*/
if (blk_rq_pos(req) + blk_rq_sectors(req) > get_capacity(req->rq_disk)) {
- cy_as_hal_print_message("cyasblkdev: bad request address\n");
+ cy_as_hal_print_message(KERN_INFO"cyasblkdev: bad request address\n");
stat = BLKPREP_KILL;
}
@@ -493,7 +493,7 @@ static void cyasblkdev_issuecallback(
{
int retry_cnt = 0;
DBGPRN_FUNC_NAME;
-
+
if (status != CY_AS_ERROR_SUCCESS) {
#ifndef WESTBRIDGE_NDEBUG
cy_as_hal_print_message(
@@ -509,7 +509,10 @@ static void cyasblkdev_issuecallback(
__func__, (unsigned int) gl_bd->queue.req, status,
(unsigned int) blk_rq_sectors(gl_bd->queue.req)) ;
#endif
-
+
+ if(rq_data_dir(gl_bd->queue.req) != READ) {
+ cy_as_release_common_lock();
+ }
/* note: blk_end_request w/o __ prefix should
* not ...Again, one patch per logical change please. That's the way patches for
Ick. How about a ' ' after KERN_INFO? And do you really always want to
The whole NDEBUG stuff should be removed from the .c files and put in
No need for the extra {}, please take a look at
Documentation/CodingStyle for how to do things for the kernel. Also a
space is needed after the 'if'.
Please run your patches through the scripts/checkpatch.pl tool before
sending them to me. Yes, there's lots of checkpatch issues in the
driver already, but don't add new ones now please.
Care to redo this?
thanks,
greg k-h
--
