From: Greg KH on
On Fri, Aug 06, 2010 at 04:23:36PM -0700, David Cross wrote:
> Re-Re-Re-Re-sending with unnecessary files removed and line wrap turned off in
> new email client. The last one did include a Signed-off-by: at the end, I
> believe, and I am re-including here. Please let me know if there are
> formatting issues with this sign off.

Ok, this one worked better, thanks.

Here's the diffstat, for those playing along at home:

arch/arm/boot/compressed/piggy.gzip |binary
arch/arm/mach-omap2/gpmc.c | 3
drivers/staging/Kconfig | 2
drivers/staging/Makefile | 1
drivers/staging/westbridge/Kconfig | 34
drivers/staging/westbridge/astoria/Kconfig | 9
drivers/staging/westbridge/astoria/Makefile | 11
drivers/staging/westbridge/astoria/api/Makefile | 14
drivers/staging/westbridge/astoria/api/src/cyasdma.c | 1107 ++
drivers/staging/westbridge/astoria/api/src/cyasintr.c | 143
drivers/staging/westbridge/astoria/api/src/cyaslep2pep.c | 358
drivers/staging/westbridge/astoria/api/src/cyaslowlevel.c | 1264 +++
drivers/staging/westbridge/astoria/api/src/cyasmisc.c | 3474 ++++++++
drivers/staging/westbridge/astoria/api/src/cyasmtp.c | 1128 ++
drivers/staging/westbridge/astoria/api/src/cyasstorage.c | 4104 ++++++++++
drivers/staging/westbridge/astoria/api/src/cyasusb.c | 3718 +++++++++
drivers/staging/westbridge/astoria/arch/arm/mach-omap2/cyashalomap_kernel.c | 2450 +++++
drivers/staging/westbridge/astoria/arch/arm/plat-omap/include/mach/westbridge/cyashaldef.h | 55
drivers/staging/westbridge/astoria/arch/arm/plat-omap/include/mach/westbridge/westbridge-omap3-pnand-hal/cyashalomap_kernel.h | 319
drivers/staging/westbridge/astoria/arch/arm/plat-omap/include/mach/westbridge/westbridge-omap3-pnand-hal/cyasmemmap.h | 555 +
drivers/staging/westbridge/astoria/arch/arm/plat-omap/include/mach/westbridge/westbridge-omap3-pnand-hal/cyasomapdev_kernel.h | 72
drivers/staging/westbridge/astoria/block/Kconfig | 9
drivers/staging/westbridge/astoria/block/Makefile | 11
drivers/staging/westbridge/astoria/block/cyasblkdev_block.c | 1628 +++
drivers/staging/westbridge/astoria/block/cyasblkdev_queue.c | 417 +
drivers/staging/westbridge/astoria/block/cyasblkdev_queue.h | 64
drivers/staging/westbridge/astoria/device/Kconfig | 9
drivers/staging/westbridge/astoria/device/Makefile | 23
drivers/staging/westbridge/astoria/device/cyandevice_export.h | 132
drivers/staging/westbridge/astoria/device/cyasdevice.c | 394
drivers/staging/westbridge/astoria/gadget/Kconfig | 9
drivers/staging/westbridge/astoria/gadget/Makefile | 11
drivers/staging/westbridge/astoria/gadget/cyasgadget.c | 2211 +++++
drivers/staging/westbridge/astoria/gadget/cyasgadget.h | 193
drivers/staging/westbridge/astoria/gadget/cyasgadget_ioctl.h | 99
drivers/staging/westbridge/astoria/include/linux/westbridge/cyanerr.h | 418 +
drivers/staging/westbridge/astoria/include/linux/westbridge/cyanmedia.h | 59
drivers/staging/westbridge/astoria/include/linux/westbridge/cyanmisc.h | 614 +
drivers/staging/westbridge/astoria/include/linux/westbridge/cyanregs.h | 180
drivers/staging/westbridge/astoria/include/linux/westbridge/cyansdkversion.h | 30
drivers/staging/westbridge/astoria/include/linux/westbridge/cyanstorage.h | 419 +
drivers/staging/westbridge/astoria/include/linux/westbridge/cyantioch.h | 35
drivers/staging/westbridge/astoria/include/linux/westbridge/cyantypes.h | 31
drivers/staging/westbridge/astoria/include/linux/westbridge/cyanusb.h | 619 +
drivers/staging/westbridge/astoria/include/linux/westbridge/cyas_cplus_end.h | 11
drivers/staging/westbridge/astoria/include/linux/westbridge/cyas_cplus_start.h | 11
drivers/staging/westbridge/astoria/include/linux/westbridge/cyascast.h | 35
drivers/staging/westbridge/astoria/include/linux/westbridge/cyasdevice.h | 1057 ++
drivers/staging/westbridge/astoria/include/linux/westbridge/cyasdma.h | 375
drivers/staging/westbridge/astoria/include/linux/westbridge/cyaserr.h | 1094 ++
drivers/staging/westbridge/astoria/include/linux/westbridge/cyashal.h | 108
drivers/staging/westbridge/astoria/include/linux/westbridge/cyashalcb.h | 44
drivers/staging/westbridge/astoria/include/linux/westbridge/cyashaldoc.h | 800 +
drivers/staging/westbridge/astoria/include/linux/westbridge/cyasintr.h | 104
drivers/staging/westbridge/astoria/include/linux/westbridge/cyaslep2pep.h | 36
drivers/staging/westbridge/astoria/include/linux/westbridge/cyaslowlevel.h | 366
drivers/staging/westbridge/astoria/include/linux/westbridge/cyasmedia.h | 54
drivers/staging/westbridge/astoria/include/linux/westbridge/cyasmisc.h | 1549 +++
drivers/staging/westbridge/astoria/include/linux/westbridge/cyasmisc_dep.h | 53
drivers/staging/westbridge/astoria/include/linux/westbridge/cyasmtp.h | 646 +
drivers/staging/westbridge/astoria/include/linux/westbridge/cyasprotocol.h | 3838 +++++++++
drivers/staging/westbridge/astoria/include/linux/westbridge/cyasregs.h | 201
drivers/staging/westbridge/astoria/include/linux/westbridge/cyasstorage.h | 2759 ++++++
drivers/staging/westbridge/astoria/include/linux/westbridge/cyasstorage_dep.h | 309
drivers/staging/westbridge/astoria/include/linux/westbridge/cyastoria.h | 36
drivers/staging/westbridge/astoria/include/linux/westbridge/cyastsdkversion.h | 30
drivers/staging/westbridge/astoria/include/linux/westbridge/cyastypes.h | 71
drivers/staging/westbridge/astoria/include/linux/westbridge/cyasusb.h | 1862 ++++
drivers/staging/westbridge/astoria/include/linux/westbridge/cyasusb_dep.h | 224
fs/fat/inode.c | 1
fs/mpage.c | 39
71 files changed, 42149 insertions(+)

That's big for just one patch.

How about breaking it up into one-patch-per-thing, like the file,
Documentation/SubmittingPatches describes. At the very least, one patch
per driver/module, which will give people a chance to review it in a
semi-sane manner.

Also, those exports, I'm not so sure about them, or why they are needed.
Why would you be reading fat filesystem blocks directly from within a
kernel module? It looks like this is within an ioctl, right? If so,
why not just read the filesystem stuff from userspace instead?

And for those ioctls, you have audited them for proper user/kernel
interfaces and permissions and overflows, right? And documented them as
to what they do?

And about that "HAL" layer. It needs to be stripped down. Way down.
Function calls like:
+ cy_as_hal_print_message("%s, bad ioctl code = 0x%x\n",
+ __func__, _IOC_NR(code));

Are not acceptable. Just call printk() and be done with it. You aren't
porting this code to any other operating system, so it's not needed.

But that's something that can be done once the code is in the staging
tree, for now, let's get it into a format that I can apply it in :)

thanks,

greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Greg KH on
On Fri, Aug 06, 2010 at 05:29:03PM -0700, David Cross wrote:
> Re-Re-Re-Re-Re-sending with unnecessary files removed, line wrap turned off in
> new email client, Signed-off-by included in the correct place, and TODO file added.

Looks much better.

I can queue this up in the staging-next tree for the .37 merge, after
the .36 merge window closes, if you don't mind me removing the non
drivers/staging/ file changes here.

Is that ok?

> --- linux-2.6-35-vanilla/arch/arm/mach-omap2/gpmc.c 2010-08-03 14:40:10.000000000 -0700
> +++ linux-2.6-35_incl_sdk/arch/arm/mach-omap2/gpmc.c 2010-08-05 16:50:24.000000000 -0700
> @@ -115,6 +115,7 @@ void gpmc_cs_write_reg(int cs, int idx,
> reg_addr = gpmc_base + GPMC_CS0 + (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)
> {
> @@ -123,6 +124,7 @@ u32 gpmc_cs_read_reg(int cs, int idx)
> reg_addr = gpmc_base + GPMC_CS0 + (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)
> @@ -276,6 +278,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)
> {

What are these symbols needed for?

Can they be marked EXPORT_SYMBOL_GPL()?

thanks,

greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: David Cross on
Sorry, there seems to be a bit of a disconnect between when I am sending
mail and getting replies, I just sent another patch with TODO and
Signed-off-by included, although it includes none of this feedback.

On Fri, 2010-08-06 at 17:16 -0700, Greg KH wrote:
> On Fri, Aug 06, 2010 at 04:23:36PM -0700, David Cross wrote:
> > Re-Re-Re-Re-sending with unnecessary files removed and line wrap turned off in
> > new email client. The last one did include a Signed-off-by: at the end, I
> > believe, and I am re-including here. Please let me know if there are
> > formatting issues with this sign off.
>
> Ok, this one worked better, thanks.
>
> Here's the diffstat, for those playing along at home:
>
> arch/arm/boot/compressed/piggy.gzip |binary
> arch/arm/mach-omap2/gpmc.c | 3
> drivers/staging/Kconfig | 2
> drivers/staging/Makefile | 1
> drivers/staging/westbridge/Kconfig | 34
> drivers/staging/westbridge/astoria/Kconfig | 9
> drivers/staging/westbridge/astoria/Makefile | 11
> drivers/staging/westbridge/astoria/api/Makefile | 14
> drivers/staging/westbridge/astoria/api/src/cyasdma.c | 1107 ++
> drivers/staging/westbridge/astoria/api/src/cyasintr.c | 143
> drivers/staging/westbridge/astoria/api/src/cyaslep2pep.c | 358
> drivers/staging/westbridge/astoria/api/src/cyaslowlevel.c | 1264 +++
> drivers/staging/westbridge/astoria/api/src/cyasmisc.c | 3474 ++++++++
> drivers/staging/westbridge/astoria/api/src/cyasmtp.c | 1128 ++
> drivers/staging/westbridge/astoria/api/src/cyasstorage.c | 4104 ++++++++++
> drivers/staging/westbridge/astoria/api/src/cyasusb.c | 3718 +++++++++
> drivers/staging/westbridge/astoria/arch/arm/mach-omap2/cyashalomap_kernel.c | 2450 +++++
> drivers/staging/westbridge/astoria/arch/arm/plat-omap/include/mach/westbridge/cyashaldef.h | 55
> drivers/staging/westbridge/astoria/arch/arm/plat-omap/include/mach/westbridge/westbridge-omap3-pnand-hal/cyashalomap_kernel.h | 319
> drivers/staging/westbridge/astoria/arch/arm/plat-omap/include/mach/westbridge/westbridge-omap3-pnand-hal/cyasmemmap.h | 555 +
> drivers/staging/westbridge/astoria/arch/arm/plat-omap/include/mach/westbridge/westbridge-omap3-pnand-hal/cyasomapdev_kernel.h | 72
> drivers/staging/westbridge/astoria/block/Kconfig | 9
> drivers/staging/westbridge/astoria/block/Makefile | 11
> drivers/staging/westbridge/astoria/block/cyasblkdev_block.c | 1628 +++
> drivers/staging/westbridge/astoria/block/cyasblkdev_queue.c | 417 +
> drivers/staging/westbridge/astoria/block/cyasblkdev_queue.h | 64
> drivers/staging/westbridge/astoria/device/Kconfig | 9
> drivers/staging/westbridge/astoria/device/Makefile | 23
> drivers/staging/westbridge/astoria/device/cyandevice_export.h | 132
> drivers/staging/westbridge/astoria/device/cyasdevice.c | 394
> drivers/staging/westbridge/astoria/gadget/Kconfig | 9
> drivers/staging/westbridge/astoria/gadget/Makefile | 11
> drivers/staging/westbridge/astoria/gadget/cyasgadget.c | 2211 +++++
> drivers/staging/westbridge/astoria/gadget/cyasgadget.h | 193
> drivers/staging/westbridge/astoria/gadget/cyasgadget_ioctl.h | 99
> drivers/staging/westbridge/astoria/include/linux/westbridge/cyanerr.h | 418 +
> drivers/staging/westbridge/astoria/include/linux/westbridge/cyanmedia.h | 59
> drivers/staging/westbridge/astoria/include/linux/westbridge/cyanmisc.h | 614 +
> drivers/staging/westbridge/astoria/include/linux/westbridge/cyanregs.h | 180
> drivers/staging/westbridge/astoria/include/linux/westbridge/cyansdkversion.h | 30
> drivers/staging/westbridge/astoria/include/linux/westbridge/cyanstorage.h | 419 +
> drivers/staging/westbridge/astoria/include/linux/westbridge/cyantioch.h | 35
> drivers/staging/westbridge/astoria/include/linux/westbridge/cyantypes.h | 31
> drivers/staging/westbridge/astoria/include/linux/westbridge/cyanusb.h | 619 +
> drivers/staging/westbridge/astoria/include/linux/westbridge/cyas_cplus_end.h | 11
> drivers/staging/westbridge/astoria/include/linux/westbridge/cyas_cplus_start.h | 11
> drivers/staging/westbridge/astoria/include/linux/westbridge/cyascast.h | 35
> drivers/staging/westbridge/astoria/include/linux/westbridge/cyasdevice.h | 1057 ++
> drivers/staging/westbridge/astoria/include/linux/westbridge/cyasdma.h | 375
> drivers/staging/westbridge/astoria/include/linux/westbridge/cyaserr.h | 1094 ++
> drivers/staging/westbridge/astoria/include/linux/westbridge/cyashal.h | 108
> drivers/staging/westbridge/astoria/include/linux/westbridge/cyashalcb.h | 44
> drivers/staging/westbridge/astoria/include/linux/westbridge/cyashaldoc.h | 800 +
> drivers/staging/westbridge/astoria/include/linux/westbridge/cyasintr.h | 104
> drivers/staging/westbridge/astoria/include/linux/westbridge/cyaslep2pep.h | 36
> drivers/staging/westbridge/astoria/include/linux/westbridge/cyaslowlevel.h | 366
> drivers/staging/westbridge/astoria/include/linux/westbridge/cyasmedia.h | 54
> drivers/staging/westbridge/astoria/include/linux/westbridge/cyasmisc.h | 1549 +++
> drivers/staging/westbridge/astoria/include/linux/westbridge/cyasmisc_dep.h | 53
> drivers/staging/westbridge/astoria/include/linux/westbridge/cyasmtp.h | 646 +
> drivers/staging/westbridge/astoria/include/linux/westbridge/cyasprotocol.h | 3838 +++++++++
> drivers/staging/westbridge/astoria/include/linux/westbridge/cyasregs.h | 201
> drivers/staging/westbridge/astoria/include/linux/westbridge/cyasstorage.h | 2759 ++++++
> drivers/staging/westbridge/astoria/include/linux/westbridge/cyasstorage_dep.h | 309
> drivers/staging/westbridge/astoria/include/linux/westbridge/cyastoria.h | 36
> drivers/staging/westbridge/astoria/include/linux/westbridge/cyastsdkversion.h | 30
> drivers/staging/westbridge/astoria/include/linux/westbridge/cyastypes.h | 71
> drivers/staging/westbridge/astoria/include/linux/westbridge/cyasusb.h | 1862 ++++
> drivers/staging/westbridge/astoria/include/linux/westbridge/cyasusb_dep.h | 224
> fs/fat/inode.c | 1
> fs/mpage.c | 39
> 71 files changed, 42149 insertions(+)
>
> That's big for just one patch.
>
> How about breaking it up into one-patch-per-thing, like the file,
> Documentation/SubmittingPatches describes. At the very least, one patch
> per driver/module, which will give people a chance to review it in a
> semi-sane manner.
>
I can probably break it up into these three patches:
1) sdk, common code and hal as well as minor kernel updates
2) block driver
3) gadget driver

But (1) will still have the majority of the code. Also the block driver
and gadget driver won't compile independently as is. In order to break
it up, I think I would need to stage things, and (1) does not make sense
independent of the other two functions (it does nothing for the kernel
on its own). Would you prefer I staged it such that the block driver and
gadget were not included initially?

> Also, those exports, I'm not so sure about them, or why they are needed.
> Why would you be reading fat filesystem blocks directly from within a
> kernel module? It looks like this is within an ioctl, right? If so,
> why not just read the filesystem stuff from userspace instead?
>
The usage case for these is as follows: we need to pre-allocate a file
in the file system and transfer data to it external to the CPU. West
Bridge has both access to USB and storage, so it transfers data directly
to the file from a USB host once that file is allocated in order to
improve performance and avoid loading the CPU. The way to pre-allocate a
FAT file that I could find was through the fat_get_block API. I don't
know of any userspace method that implements this functionality, but am
open to using any that are available.

> And for those ioctls, you have audited them for proper user/kernel
> interfaces and permissions and overflows, right? And documented them as
> to what they do?
>
I don't know, so probably not. I have audited mostly for functionality.
Can I add this to the TODO list and still submit the driver in staging
or does this need to happen first?

> And about that "HAL" layer. It needs to be stripped down. Way down.
> Function calls like:
> + cy_as_hal_print_message("%s, bad ioctl code = 0x%x\n",
> + __func__, _IOC_NR(code));
>
> Are not acceptable. Just call printk() and be done with it. You aren't
> porting this code to any other operating system, so it's not needed.
As you probably guessed, the original idea of the SDK was portability,
which is the reason for the implementation choice. But I understand your
point, I will add that to the TODO list. I am guessing the cplusplus
defines are out as well?


> But that's something that can be done once the code is in the staging
> tree, for now, let's get it into a format that I can apply it in :)
Just got your next email (checked before send this time), let me know if
other changes are needed.

> On Fri, Aug 06, 2010 at 05:29:03PM -0700, David Cross wrote:
> > Re-Re-Re-Re-Re-sending with unnecessary files removed, line wrap
> turned off in
> > new email client, Signed-off-by included in the correct place, and
> TODO file added.
>
> Looks much better.
>
> I can queue this up in the staging-next tree for the .37 merge, after
> the .36 merge window closes, if you don't mind me removing the non
> drivers/staging/ file changes here.
>
> Is that ok?
Yes, that is fine by me, but it won't compile as is without these
changes in place. Do you need me to comment out the lines that would
need to be added back in later (gpmc_cs_write_reg, etc.)?

> > --- linux-2.6-35-vanilla/arch/arm/mach-omap2/gpmc.c 2010-08-03
> 14:40:10.000000000 -0700
> > +++ linux-2.6-35_incl_sdk/arch/arm/mach-omap2/gpmc.c 2010-08-05
> 16:50:24.000000000 -0700
> > @@ -115,6 +115,7 @@ void gpmc_cs_write_reg(int cs, int idx,
> > reg_addr = gpmc_base + GPMC_CS0 + (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)
> > {
> > @@ -123,6 +124,7 @@ u32 gpmc_cs_read_reg(int cs, int idx)
> > reg_addr = gpmc_base + GPMC_CS0 + (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)
> > @@ -276,6 +278,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)
> > {
>
> What are these symbols needed for?

These symbols are needed to configure timing on the OMAP: hardware
configuration such as setup, hold times, how long the address is on the
bus for, how long CE is asserted, etc.

> Can they be marked EXPORT_SYMBOL_GPL()?
Probably, I am not their maintainer though so I don't know if I am
authorized to make that choice.

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(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/