2010/10/26 Alan Cox <[email protected]>:
Sorry for just reviewing the structs at this point but I hope it
helps out...
> diff --git a/drivers/serial/ifx6x60.h b/drivers/serial/ifx6x60.h
> new file mode 100644
> index 0000000..deb7b8d
> --- /dev/null
> +++ b/drivers/serial/ifx6x60.h
> (...)
> +struct ifx_spi_device {
> + /* Our SPI device */
> + struct spi_device *spi_dev;
> +
> + /* Port specific data */
> + struct kfifo tx_fifo;
> + spinlock_t fifo_lock;
> + unsigned long signal_state;
> +
> + /* TTY Layer logic */
> + struct tty_port tty_port;
> + struct device *tty_dev;
> + int minor;
> +
> + /* Low level I/O work */
> + struct tasklet_struct io_work_tasklet;
> + unsigned long flags;
> + dma_addr_t rx_dma;
> + dma_addr_t tx_dma;
> +
> + int is_6160; /* Modem type */
Should this be a bool? It is suspiciously named much
like something boolean.
> +
> + spinlock_t write_lock;
> + int write_pending;
This should be a bool.
> + spinlock_t power_lock;
> + unsigned char power_status;
This piece of comments from the ifx_spi_power_state_clear
kerneldoc:
"+ * ifx_spi_power_state_clear - clear power bit"
Actually indicates that this is also a bool, albeit using some
opaque "val" to set/clear it. The code handling this flag really
needs an overhaul or I'm not reading it right.
> + unsigned char *rx_buffer;
> + unsigned char *tx_buffer;
> + dma_addr_t rx_bus;
> + dma_addr_t tx_bus;
> + unsigned char spi_more;
> + unsigned char spi_slave_cts;
> +
> + struct timer_list spi_timer;
> +
> + struct spi_message spi_msg;
> + struct spi_transfer spi_xfer;
> +
> + struct {
> + /* gpio lines */
> + unsigned short srdy; /* slave-ready gpio */
> + unsigned short mrdy; /* master-ready gpio */
> + unsigned short reset; /* modem-reset gpio */
> + unsigned short po; /* modem-on gpio */
> + unsigned short reset_out; /* modem-in-reset gpio */
Is there something wrong about using u16 for these?
If it's to fit the gpio function it ought to be unsigned int
I believe.
> + /* state/stats */
> + int unack_srdy_int_nb;
I don't get that comment above. This seems to be some
interrupt counter, this whole struct may need some kerneldoc.
> + } gpio;
> +
> + /* modem reset */
> + unsigned long mdm_reset_state;
> +#define MR_START 0
> +#define MR_INPROGRESS 1
> +#define MR_COMPLETE 2
> + wait_queue_head_t mdm_reset_wait;
> +};
> +
> +#endif /* _IFX6X60_H */
> diff --git a/include/linux/spi/ifx_modem.h b/include/linux/spi/ifx_modem.h
> new file mode 100644
> index 0000000..a68f3b1
> --- /dev/null
> +++ b/include/linux/spi/ifx_modem.h
> @@ -0,0 +1,14 @@
> +#ifndef LINUX_IFX_MODEM_H
> +#define LINUX_IFX_MODEM_H
> +
/**
* Insert some kerneldoc of struct fields here, I think
* it'd be nice.
*/
> +struct ifx_modem_platform_data {
> + unsigned short rst_out; /* modem reset out */
> + unsigned short pwr_on; /* power on */
> + unsigned short rst_pmu; /* reset modem */
> + unsigned short tx_pwr; /* modem power threshold */
> + unsigned short srdy; /* SRDY */
> + unsigned short mrdy; /* MRDY */
If these are GPIO pins they should be unsigned int I believe.
> + unsigned short is_6160; /* Modem type */
.. is this bool?
> +};
> +
> +#endif
Yours,
Linus Walleij
------------------------------------------------------------------------------
Nokia and AT&T present the 2010 Calling All Innovators-North America contest
Create new apps & games for the Nokia N8 for consumers in U.S. and Canada
$10 million total in prizes - $4M cash, 500 devices, nearly $6M in marketing
Develop with Nokia Qt SDK, Web Runtime, or Java and Publish to Ovi Store
http://p.sf.net/sfu/nokia-dev2dev
_______________________________________________
spi-devel-general mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/spi-devel-general
