On Wednesday 16 July 2014 22:33:47 Greg KH wrote:
> + mmc: Char SDIO Device Driver
A few comments on the ioctl interface, since I suppose we'll have
to live with that once it gets merged.
> +Interface
> +=========
> +The Char SDIO Device Driver has two char device interfaces:
> + - Control Interface;
> + - Function Interface.
> +
> +Char SDIO Device Driver Control Interface consists of:
> + - open() - device node is /dev/csdio0;
> + - close()
> + - ioctl() - the following options are available:
> + - CSDIO_IOC_ENABLE_HIGHSPEED_MODE;
> + - CSDIO_IOC_SET_DATA_TRANSFER_CLOCKS;
> + - CSDIO_IOC_ENABLE_ISR;
> + - CSDIO_IOC_DISABLE_ISR.
The documentation doesn't match the code.:
CSDIO_IOC_GET/SET_VDD are implemented in the control device,
not the function device, and that is probably a good idea.
CSDIO_IOC_ENABLE_HIGHSPEED_MODE is not implemented
at all. CSDIO_IOC_ENABLE_ISR seems redundant with what
the CSDIO_IOC_{DIS,}CONNECT_ISR functions do, but I could be
wrong here.
The sysfs interface may actually be more appropriate than the
control node, if I was to pick one of the two.
> +Char SDIO Device Driver Function Interface consists of:
> + - open() - device node is /dev/csdiofX, where X is Function Id;
> + - close()
> + - read() - send CMD53 read;
> + - write() - send CMD53 write;
> + - ioctl() - the following options are available:
> + - CSDIO_IOC_SET_OP_CODE - 0 fixed adrress, 1 autoincrement.
> + - CSDIO_IOC_FUNCTION_SET_BLOCK_SIZE;
> + - CSDIO_IOC_SET_BLOCK_MODE - 0 byte mode, 1 block mode;
> + - CSDIO_IOC_CMD52 - execute CMD52, receives the
> + following structure as a parameter:
> + struct csdio_cmd52_ctrl_t {
> + uint32_t m_write; // 0 - read, 1 -write
> + uint32_t m_address;
> + uint32_t m_data; // data to write or read data
> + uint32_t m_ret; // command execution status
> + }__attribute__ ((packed));
> + - CSDIO_IOC_CMD53 - setup CMD53 data transfer, receives the
> + following structure as a parameter:
> + struct csdio_cmd53_ctrl_t {
> + uint32_t m_block_mode;
> + uint32_t m_op_code;
> + uint32_t m_address;
> + }__attribute__ ((packed));
> + - CSDIO_IOC_CONNECT_ISR;
> + - CSDIO_IOC_DISCONNECT_ISR;
> + - CSDIO_IOC_GET_VDD;
> + - CSDIO_IOC_SET_VDD.
> +
> +Additionally, user space application can use fcntl system call with
> +parameters F_SETOWN and F_SETFL in order to set an asynchronous
> +callback for SDIO interrupt.
The read/write API seems very strange. SDIO has multiple ways of
passing data between OS and device using CMD52 and CMD53, and they
are all done differently here:
- A one-byte read or write uses a single CSDIO_IOC_CMD52 ioctl.
This could be done using pread/pwrite with length '1' rather than
an ioctl, if we want to.
- a byte-addressed multi-byte read/write needs to first 'seek' using
CSDIO_IOC_CMD53 or and then transfer using read() or write().
- a block-addressed read/write is similar, but the number of bytes
copied to/from user space is now multiplied with the block size (!).
The block size needs to be set using both CSDIO_IOC_SET_BLOCK_MODE
and CSDIO_IOC_CMD53, using the same value.
- to do a multi-byte read/write at a constant address rather than
a block copy, CSDIO_IOC_CMD53 needs to set at least one bit (not
necessarily the one used in the actual command) in m_op_code.
I guess all of this could be done using pread/pwrite, using the
loff_t value to encode the m_address in the lower bits and the
mode (single-byte, multi-byte, multi-byte auto-increment,
block, block auto-increment) in some of the upper bits.
Alternatively, the read/write functions could be removed completely
and everything be done in the ioctls, but in a more consistent
way.
> +
> +#define CSDIO_IOC_MAGIC 'm'
> +
> +#define CSDIO_IOC_ENABLE_HIGHSPEED_MODE _IO(CSDIO_IOC_MAGIC, 0)
> +#define CSDIO_IOC_SET_DATA_TRANSFER_CLOCKS _IO(CSDIO_IOC_MAGIC, 1)
> +#define CSDIO_IOC_SET_OP_CODE _IO(CSDIO_IOC_MAGIC, 2)
> +#define CSDIO_IOC_FUNCTION_SET_BLOCK_SIZE _IO(CSDIO_IOC_MAGIC, 3)
> +#define CSDIO_IOC_SET_BLOCK_MODE _IO(CSDIO_IOC_MAGIC, 4)
> +#define CSDIO_IOC_CONNECT_ISR _IO(CSDIO_IOC_MAGIC, 5)
> +#define CSDIO_IOC_DISCONNECT_ISR _IO(CSDIO_IOC_MAGIC, 6)
> +#define CSDIO_IOC_CMD52 _IO(CSDIO_IOC_MAGIC, 7)
> +#define CSDIO_IOC_CMD53 _IO(CSDIO_IOC_MAGIC, 8)
> +#define CSDIO_IOC_ENABLE_ISR _IO(CSDIO_IOC_MAGIC, 9)
> +#define CSDIO_IOC_DISABLE_ISR _IO(CSDIO_IOC_MAGIC, 10)
> +#define CSDIO_IOC_SET_VDD _IO(CSDIO_IOC_MAGIC, 11)
> +#define CSDIO_IOC_GET_VDD _IO(CSDIO_IOC_MAGIC, 12)
> +
> +#define CSDIO_IOC_MAXNR 12
The command codes above should use _IOR or _IOW where appropriate to
encode the size of the transferred data.
> +
> +struct csdio_cmd53_ctrl_t {
> + uint32_t m_block_mode; /* data tran. byte(0)/block(1) mode */
> + uint32_t m_op_code; /* address auto increment flag */
> + uint32_t m_address;
> +} __attribute__ ((packed));
> +
> +struct csdio_cmd52_ctrl_t {
> + uint32_t m_write;
> + uint32_t m_address;
> + uint32_t m_data;
> + uint32_t m_ret;
> +} __attribute__ ((packed));
The packing is not necessary here, but at least all of these are
compatible between 32 and 64 bit, so enabling compat mode is a one-line
change and requires no conversion.
Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
