2010/10/26 Alan Cox <alan@xxxxxxxxxxxxxxxxxxx>:
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
--
To unsubscribe from this list: send the line "unsubscribe linux-serial" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
