staging: tidspbridge: remove file handling functions for loader

PATCH In review Omar Ramirez Luna 2 participants 7 messages Dec 7, 2010 – Dec 10, 2010 Staging

lore

Share & tools

lore

Omar Ramirez Luna

author

2010-12-07 06:09 UTC #0 lore raw

PATCH 1/1 staging: tidspbridge: remove file handling functions for loader

Instead use request_firmware and friends to get a valid firmware image.

Right now the image is supplied dynamically through udev and the following rule:

KERNEL=="omap-dsp", SUBSYSTEM=="firmware", ACTION=="add",	\
	RUN+="/bin/sh -c 'echo 1 > /sys/$DEVPATH/loading;	\
		cat $FIRMWARE > /sys/$DEVPATH/data;		\
		echo 0 > /sys/$DEVPATH/loading'"

Signed-off-by Omar Ramirez Luna <[email protected]>

---
 .../tidspbridge/include/dspbridge/dbldefs.h        |   10 --
 .../staging/tidspbridge/include/dspbridge/dbll.h   |    7 ++
 .../tidspbridge/include/dspbridge/dblldefs.h       |   35 ------
 drivers/staging/tidspbridge/pmgr/cod.c             |  100 -----------------
 drivers/staging/tidspbridge/pmgr/dbll.c            |  114 +++++++++++--------
 5 files changed, 73 insertions(+), 193 deletions(-)

drivers/staging/tidspbridge/include/dspbridge/dbldefs.h +0 ~~-10~~

index bf4fb99..c74321b 100644
@@ -126,16 +126,6 @@ struct dbl_attrs {
 	dbl_sym_lookup sym_lookup;
 	void *sym_handle;
 	void *sym_arg;
-
-	/*
-	 *  These file manipulation functions should be compatible with the
-	 *  "C" run time library functions of the same name.
-	 */
-	 s32(*fread) (void *, size_t, size_t, void *);
-	 s32(*fseek) (void *, long, int);
-	 s32(*ftell) (void *);
-	 s32(*fclose) (void *);
-	void *(*fopen) (const char *, const char *);
 };
 
 #endif /* DBLDEFS_ */

drivers/staging/tidspbridge/include/dspbridge/dbll.h +7 -0

index b018676..ad081e0 100644
@@ -20,9 +20,16 @@
 #ifndef DBLL_
 #define DBLL_
 
+#include <linux/firmware.h>
 #include <dspbridge/dbdefs.h>
 #include <dspbridge/dblldefs.h>
 
+struct dsp_fw {
+	const struct firmware *img;
+	const char *name;
+	const u8 *pos;
+};
+
 extern bool symbols_reloaded;
 
 extern void dbll_close(struct dbll_library_obj *zl_lib);

drivers/staging/tidspbridge/include/dspbridge/dblldefs.h +0 ~~-35~~

index d2b4fda..f353a14 100644
@@ -79,11 +79,6 @@ typedef s32(*dbll_alloc_fxn) (void *hdl, s32 space, u32 size, u32 align,
 			      bool reserved);
 
 /*
- *  ======== dbll_close_fxn ========
- */
-typedef s32(*dbll_f_close_fxn) (void *);
-
-/*
  *  ======== dbll_free_fxn ========
  *  Free memory function.  Free, or unreserve (if reserved == TRUE) "size"
  *  bytes of memory from segment "space"

@@ -92,11 +87,6 @@ typedef bool(*dbll_free_fxn) (void *hdl, u32 addr, s32 space, u32 size,
 			      bool reserved);
 
 /*
- *  ======== dbll_f_open_fxn ========
- */
-typedef void *(*dbll_f_open_fxn) (const char *, const char *);
-
-/*
  *  ======== dbll_log_write_fxn ========
  *  Function to call when writing data from a section, to log the info.
  *  Can be NULL if no logging is required.

@@ -106,16 +96,6 @@ typedef int(*dbll_log_write_fxn) (void *handle,
 					 u32 bytes);
 
 /*
- *  ======== dbll_read_fxn ========
- */
-typedef s32(*dbll_read_fxn) (void *, size_t, size_t, void *);
-
-/*
- *  ======== dbll_seek_fxn ========
- */
-typedef s32(*dbll_seek_fxn) (void *, long, int);
-
-/*
  *  ======== dbll_sym_lookup ========
  *  Symbol lookup function - Find the symbol name and return its value.
  *

@@ -133,11 +113,6 @@ typedef bool(*dbll_sym_lookup) (void *handle, void *parg, void *rmm_handle,
 				const char *name, struct dbll_sym_val ** sym);
 
 /*
- *  ======== dbll_tell_fxn ========
- */
-typedef s32(*dbll_tell_fxn) (void *);
-
-/*
  *  ======== dbll_write_fxn ========
  *  Write memory function.  Write "n" HOST bytes of memory to segment "mtype"
  *  starting at address "dsp_address" from the buffer "buf".  The buffer is

@@ -163,16 +138,6 @@ struct dbll_attrs {
 	dbll_sym_lookup sym_lookup;
 	void *sym_handle;
 	void *sym_arg;
-
-	/*
-	 *  These file manipulation functions should be compatible with the
-	 *  "C" run time library functions of the same name.
-	 */
-	 s32(*fread) (void *, size_t, size_t, void *);
-	 s32(*fseek) (void *, long, int);
-	 s32(*ftell) (void *);
-	 s32(*fclose) (void *);
-	void *(*fopen) (const char *, const char *);
 };
 
 /*

drivers/staging/tidspbridge/pmgr/cod.c +0 ~~-100~~

112 lines not on this page · raw message

drivers/staging/tidspbridge/pmgr/dbll.c +66 ~~-48~~

191 lines not on this page · raw message

Signature · 1 line

1.7.1

Greg KH

2010-12-08 22:26 UTC #1 lore raw

Quoted 10 lines · Omar Ramirez Luna

Instead use request_firmware and friends to get a valid firmware image.
Right now the image is supplied dynamically through udev and the following rule:
KERNEL=="omap-dsp", SUBSYSTEM=="firmware", ACTION=="add",	\
	RUN+="/bin/sh -c 'echo 1 > /sys/$DEVPATH/loading;	\
		cat $FIRMWARE > /sys/$DEVPATH/data;		\
		echo 0 > /sys/$DEVPATH/loading'"

Why do you need a custom firmware rule? Why doesn't the default firmware loading rule that comes with udev work properly for you? What are you needing different here that works properly for all other drivers?

confused,

greg k-h

Ramirez Luna, Omar

author

2010-12-08 23:02 UTC #2 lore raw

Hi,

Quoted 7 lines · Greg KH

Quoted 6 lines · Omar Ramirez Luna
Instead use request_firmware and friends to get a valid firmware image.
Right now the image is supplied dynamically through udev and the following rule:

Quoted 6 lines

Quoted 4 lines
KERNEL=="omap-dsp", SUBSYSTEM=="firmware", ACTION=="add", \
RUN+="/bin/sh -c 'echo 1 > /sys/$DEVPATH/loading; \
cat $FIRMWARE > /sys/$DEVPATH/data; \
echo 0 > /sys/$DEVPATH/loading'"
Why do you need a custom firmware rule?

It was meant as an example, when I compiled my minimal file system it didn't supply the firmware.sh script nor created /lib/firmware... I thought that not everybody would have the firmware.sh, so I just provided a sample rule.

Quoted 2 lines

Why doesn't the default firmware loading rule that comes with udev work properly for you? What are you needing different here that works properly for all other drivers?

firmware.sh under /lib/udev/ and dsp binaries installed under /lib/firmware/, my rule is the brute version of firmware.sh so nothing different in the script.

Probably the only change would be to supply the firmware name only, as of now the insmod parameter requires the entire path, e.g.:

insmod bridgedriver.ko base_img=/lib/dsp/baseimage.dof

if using firmware.sh and placing firmware files under /lib/firmware/, then

insmod bridgedriver.ko base_img=baseimage.dof

Should be enough.

Regards,

Omar

Greg KH

2010-12-08 23:08 UTC #3 lore raw

Quoted 15 lines · Omar

Hi,
Quoted 12 lines · Greg KH
Quoted 10 lines · Omar Ramirez Luna
Instead use request_firmware and friends to get a valid firmware image.
Right now the image is supplied dynamically through udev and the following rule:
KERNEL=="omap-dsp", SUBSYSTEM=="firmware", ACTION=="add", \
RUN+="/bin/sh -c 'echo 1 > /sys/$DEVPATH/loading; \
cat $FIRMWARE > /sys/$DEVPATH/data; \
echo 0 > /sys/$DEVPATH/loading'"

Quoted 6 lines

Quoted 1 line
Why do you need a custom firmware rule?
It was meant as an example, when I compiled my minimal file system it didn't supply the firmware.sh script nor created /lib/firmware... I thought that not everybody would have the firmware.sh, so I just provided a sample rule.

So, can I remove this from the changelog comment, as it's not really needed at all?

Quoted 7 lines

Quoted 2 lines
Why doesn't the default firmware loading rule that comes with udev work properly for you? What are you needing different here that works properly for all other drivers?
firmware.sh under /lib/udev/ and dsp binaries installed under /lib/firmware/, my rule is the brute version of firmware.sh so nothing different in the script.

Quoted 8 lines

Probably the only change would be to supply the firmware name only, as of now the insmod parameter requires the entire path, e.g.:
insmod bridgedriver.ko base_img=/lib/dsp/baseimage.dof
if using firmware.sh and placing firmware files under /lib/firmware/, then
insmod bridgedriver.ko base_img=baseimage.dof

Ick, why use a module parameter name at all? Why is this "special" and different from all other firmware users? They don't have to manually specify a file name, the driver does that.

Please fix up the patch to not require a module parameter, distros hate them, and users hate them even more.

thanks,

greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Ramirez Luna, Omar

author

2010-12-08 23:32 UTC #4 lore raw

Quoted 18 lines · Greg KH

Quoted 17 lines · Omar
Hi,
Quoted 13 lines · Greg KH
Quoted 10 lines · Omar Ramirez Luna
Instead use request_firmware and friends to get a valid firmware image.
Right now the image is supplied dynamically through udev and the following rule:
KERNEL=="omap-dsp", SUBSYSTEM=="firmware", ACTION=="add", \
RUN+="/bin/sh -c 'echo 1 > /sys/$DEVPATH/loading; \
cat $FIRMWARE > /sys/$DEVPATH/data; \
echo 0 > /sys/$DEVPATH/loading'"
Why do you need a custom firmware rule?

Quoted 7 lines

Quoted 4 lines
It was meant as an example, when I compiled my minimal file system it didn't supply the firmware.sh script nor created /lib/firmware... I thought that not everybody would have the firmware.sh, so I just provided a sample rule.
So, can I remove this from the changelog comment, as it's not really needed at all?

Yes it can be removed.

BTW, I don't expect this pushed through staging yet, I need to include it to my branch first and then I'll send a pull request with the pile of patches. Sorry for the misunderstanding and thanks for the review.

Quoted 5 lines

Quoted 1 line
insmod bridgedriver.ko base_img=baseimage.dof
Ick, why use a module parameter name at all? Why is this "special" and different from all other firmware users? They don't have to manually specify a file name, the driver does that.

The thing is that there are N number of firmwares, e.g.:

There is the official and usable firmware to play with multimedia "baseimage.dof"

But there are also minimal firmwares to just ping or swap buffers with the dsp, when you just want to play around with basic features.

Quoted 2 lines

Please fix up the patch to not require a module parameter, distros hate them, and users hate them even more.

The driver is the one requiring the parameter (it was already this way), this patch doesn't introduce any parameter. I'll check what can be done and if nobody rejects I'll use the baseimage.dof as firmware by default.

Regards,

Omar

Greg KH

2010-12-08 23:50 UTC #5 lore raw

Quoted 24 lines · Omar

Quoted 23 lines · Greg KH
Quoted 21 lines · Omar
Hi,
Quoted 7 lines · Greg KH
Quoted 6 lines · Omar Ramirez Luna
Instead use request_firmware and friends to get a valid firmware image.
Right now the image is supplied dynamically through udev and the following rule:
Quoted 6 lines
Quoted 4 lines
KERNEL=="omap-dsp", SUBSYSTEM=="firmware", ACTION=="add", \
RUN+="/bin/sh -c 'echo 1 > /sys/$DEVPATH/loading; \
cat $FIRMWARE > /sys/$DEVPATH/data; \
echo 0 > /sys/$DEVPATH/loading'"
Why do you need a custom firmware rule?
It was meant as an example, when I compiled my minimal file system it didn't supply the firmware.sh script nor created /lib/firmware... I thought that not everybody would have the firmware.sh, so I just provided a sample rule.

Quoted 8 lines

Quoted 2 lines
So, can I remove this from the changelog comment, as it's not really needed at all?
Yes it can be removed.
BTW, I don't expect this pushed through staging yet, I need to include it to my branch first and then I'll send a pull request with the pile of patches. Sorry for the misunderstanding and thanks for the review.

Well, don't send me patches you don't want me to apply without a big "DO NOT APPLY" in them, otherwise I might try to :)

Quoted 6 lines

Quoted 5 lines
Quoted 1 line
insmod bridgedriver.ko base_img=baseimage.dof
Ick, why use a module parameter name at all? Why is this "special" and different from all other firmware users? They don't have to manually specify a file name, the driver does that.

Quoted 7 lines

The thing is that there are N number of firmwares, e.g.:
There is the official and usable firmware to play with multimedia "baseimage.dof"
But there are also minimal firmwares to just ping or swap buffers with the dsp, when you just want to play around with basic features.

Then mess with the firmware symlink in userspace, don't have the driver have to worry about it.

Quoted 7 lines

Quoted 2 lines
Please fix up the patch to not require a module parameter, distros hate them, and users hate them even more.
The driver is the one requiring the parameter (it was already this way), this patch doesn't introduce any parameter. I'll check what can be done and if nobody rejects I'll use the baseimage.dof as firmware by default.

That would be good. Again, you can put whatever firmware image you want in that location if you wish to use different ones. That is for userspace to do, not have the kernel worry about.

thanks,

greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Ramirez Luna, Omar

author

2010-12-10 18:43 UTC #6 lore raw

Quoted 6 lines · Greg KH

Quoted 3 lines
BTW, I don't expect this pushed through staging yet, I need to include it to my branch first and then I'll send a pull request with the pile of patches. Sorry for the misunderstanding and thanks for the review.
Well, don't send me patches you don't want me to apply without a big "DO NOT APPLY" in them, otherwise I might try to :)

Will do.

Quoted 2 lines

Then mess with the firmware symlink in userspace, don't have the driver have to worry about it.

Ok, agree.

Quoted 3 lines

That would be good. Again, you can put whatever firmware image you want in that location if you wish to use different ones. That is for userspace to do, not have the kernel worry about.

Agree.

Regards,

Omar