Message-ID: <04CAA605B7EE4630B1EB550AAAD0B746@stanford.edu>
Date:	Wed, 8 Sep 2010 13:56:09 -0700
From:	"David Cross" <david.cross@...ress.com>
To:	"'Greg KH'" <greg@...ah.com>
Cc:	<linux-kernel@...r.kernel.org>
Subject: RE: [PATCH] west bridge, kconfig and hal fixes


From: Greg KH [mailto:greg@...ah.com] 

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
> responding to here?
I will removed that going forward when submitting patches, it does seem
appropriate on this response, so am leaving it in.

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

> Again, one patch per change, please break this up.

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
area of the linux-next tree? 

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

> If you use git you can send a patch that properly shows only what
> changes even if the file is moved.  Care to do that?

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
I need to do it to get this patch applied?

> > The Kconfig changes are closely related to the inclusion of the CRAM
> > HAL layer, and as such this patch is difficult to logically separate. 

> Why is it necessary?

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
kernel maintainers.

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

> Why should the driver care about which arch it is built for?  It should
> build on _all_ arches, right?

It should build on all architectures that have an appropriate HAL
implemented. Otherwise, if you don't have a method for low level
communication with west bridge, why would you want to build the driver?

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

> Why do you need a "HAL" at all here?

For low level communication. The interface to the processor is a memory
interface across which mailbox messages are passed. These mailbox messages
makeup a sort of data link layer on top of which higher levels of
abstraction can be built. 
Additionally, the HAL defines how you pump data to the device's endpoints,
since this is also done across the memory interface from the processor's
perspective. Other power management items are also included, and the
complete list of functions can be seen in the header files.

Thanks,
david


---------------------------------------------------------------
This message and any attachments may contain Cypress (or its
subsidiaries) confidential information. If it has been received
in error, please advise the sender and immediately delete this
message.
---------------------------------------------------------------

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists