Hello Arnd,
I need some clarification on the last bit of your initial feedback below:
On Thu, 17 Mar 2011, Arnd Bergmann wrote:
> On Thursday 17 March 2011 19:28:55 John Calixto wrote:
<snip>
> > diff --git a/drivers/mmc/core/sd_ops.c b/drivers/mmc/core/sd_ops.c
> > index 797cdb5..0453dcd 100644
> > --- a/drivers/mmc/core/sd_ops.c
> > +++ b/drivers/mmc/core/sd_ops.c
> > @@ -20,7 +20,7 @@
> > #include "core.h"
> > #include "sd_ops.h"
> >
> > -static int mmc_app_cmd(struct mmc_host *host, struct mmc_card *card)
> > +int mmc_app_cmd(struct mmc_host *host, struct mmc_card *card)
> > {
> > int err;
> > struct mmc_command cmd;
> > @@ -48,6 +48,7 @@ static int mmc_app_cmd(struct mmc_host *host, struct
> > mmc_card *card)
> >
> > return 0;
> > }
> > +EXPORT_SYMBOL(mmc_app_cmd);
>
> Why not EXPORT_SYMBOL_GPL?
>
I was just using the convention already used in sd_ops.c. I can send a
pre-patch that sets all of the symbol exports in that file to be
EXPORT_SYMBOL_GPL, but without confirmation from you and others on this
list, that seems like overstepping my "jurisdiction". Is that preferable?
> > @@ -84,5 +86,21 @@
> > #define SD_SWITCH_ACCESS_DEF 0
> > #define SD_SWITCH_ACCESS_HS 1
> >
> > +struct sd_ioc_cmd {
> > + unsigned int struct_version;
> > +#define SD_IOC_CMD_STRUCT_VERSION 0
> > + int write_flag; /* implies direction of data. true = write, false =
> > read */
> > + unsigned int opcode;
> > + unsigned int arg;
> > + unsigned int flags;
> > + unsigned int postsleep_us; /* apply usecond delay *after* issuing
> > command */
> > + unsigned int force_timeout_ns; /* force timeout to be
> > force_timeout_ns ns */
> > + unsigned int response[4]; /* CMD response */
> > + unsigned int blksz;
> > + unsigned int blocks;
> > + unsigned char *data; /* DAT buffer */
> > +};
> > +#define SD_IOC_ACMD _IOWR(MMC_BLOCK_MAJOR, 0, struct sd_ioc_cmd *)
> > +
> > #endif
> >
>
> As I mentioned, any ioctl command that gets introduced needs to be
> designed very carefully to make sure we don't need to change it in the
> future. The only two things I can see here are:
>
> * The struct_version should be removed
> * The data pointer is not compatible between 32 and 64 bits.
> One solution for this would be to make it a __u64 argument
> and require the user to cast the pointer to a 64 bit type.
>
> Arnd
I don't understand your comment about the data pointer not being
compatible between 32 and 64 bits. Wouldn't the compiler take care of
pointer size?
Thanks,
John
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
