Re: [PATCH 1/2] drivers:staging:ti-st: move TI_ST from staging

[Jiri Slaby <jirislaby@gmail.com>]

Hi,

I have few comments below.

On 10/06/2010 06:18 PM, pavan_savoy@ti.com wrote:
> --- /dev/null
> +++ b/drivers/misc/ti-st/st_core.c
> @@ -0,0 +1,1031 @@
...
> +#define PROTO_ENTRY(type, name)	name
> +const unsigned char *protocol_strngs[] = {
> +	PROTO_ENTRY(ST_BT, "Bluetooth"),
> +	PROTO_ENTRY(ST_FM, "FM"),
> +	PROTO_ENTRY(ST_GPS, "GPS"),
> +};

Is this planned to be used somewhere?

> +void st_send_frame(enum proto_type protoid, struct st_data_s *st_gdata)
> +{
> +	pr_info(" %s(prot:%d) ", __func__, protoid);

This is rather a kind of debug info... (with missing \n)

> +	if (unlikely
> +	    (st_gdata == NULL || st_gdata->rx_skb == NULL
> +	     || st_gdata->list[protoid] == NULL)) {

This is not much readable.

> +		pr_err("protocol %d not registered, no data to send?",
> +			   protoid);

Missing \n. And all over the code.

> +static inline int st_check_data_len(struct st_data_s *st_gdata,

It doesn't look like a candidate for inlining.

> +	int protoid, int len)
> +{
> +	register int room = skb_tailroom(st_gdata->rx_skb);

register... hmm, leave this on compiler.

> +	pr_debug("len %d room %d", len, room);
> +
> +	if (!len) {
> +		/* Received packet has only packet header and
> +		 * has zero length payload. So, ask ST CORE to
> +		 * forward the packet to protocol driver (BT/FM/GPS)
> +		 */
> +		st_send_frame(protoid, st_gdata);
> +
> +	} else if (len > room) {
> +		/* Received packet's payload length is larger.
> +		 * We can't accommodate it in created skb.
> +		 */
> +		pr_err("Data length is too large len %d room %d", len,
> +			   room);
> +		kfree_skb(st_gdata->rx_skb);
> +	} else {
> +		/* Packet header has non-zero payload length and
> +		 * we have enough space in created skb. Lets read
> +		 * payload data */
> +		st_gdata->rx_state = ST_BT_W4_DATA;
> +		st_gdata->rx_count = len;
> +		return len;
> +	}
> +
> +	/* Change ST state to continue to process next
> +	 * packet */
> +	st_gdata->rx_state = ST_W4_PACKET_TYPE;
> +	st_gdata->rx_skb = NULL;
> +	st_gdata->rx_count = 0;
> +
> +	return 0;
> +}
> +
> +/**
> + * st_wakeup_ack - internal function for action when wake-up ack
> + *	received
> + */
> +static inline void st_wakeup_ack(struct st_data_s *st_gdata,
> +	unsigned char cmd)
> +{
> +	register struct sk_buff *waiting_skb;
> +	unsigned long flags = 0;

No need to initialize.

> +	spin_lock_irqsave(&st_gdata->lock, flags);
> +	/* de-Q from waitQ and Q in txQ now that the
> +	 * chip is awake
> +	 */
> +	while ((waiting_skb = skb_dequeue(&st_gdata->tx_waitq)))
> +		skb_queue_tail(&st_gdata->txq, waiting_skb);
> +
> +	/* state forwarded to ST LL */
> +	st_ll_sleep_state(st_gdata, (unsigned long)cmd);
> +	spin_unlock_irqrestore(&st_gdata->lock, flags);
> +
> +	/* wake up to send the recently copied skbs from waitQ */
> +	st_tx_wakeup(st_gdata);
> +}
> +
> +/**
> + * st_int_recv - ST's internal receive function.
> + *	Decodes received RAW data and forwards to corresponding
> + *	client drivers (Bluetooth,FM,GPS..etc).
> + *	This can receive various types of packets,
> + *	HCI-Events, ACL, SCO, 4 types of HCI-LL PM packets
> + *	CH-8 packets from FM, CH-9 packets from GPS cores.
> + */
> +void st_int_recv(void *disc_data,
> +	const unsigned char *data, long count)
> +{
> +	register char *ptr;
> +	struct hci_event_hdr *eh;
> +	struct hci_acl_hdr *ah;
> +	struct hci_sco_hdr *sh;
> +	struct fm_event_hdr *fm;
> +	struct gps_event_hdr *gps;
> +	register int len = 0, type = 0, dlen = 0;
> +	static enum proto_type protoid = ST_MAX;
> +	struct st_data_s *st_gdata = (struct st_data_s *)disc_data;
> +
> +	ptr = (char *)data;

Too much of casts and registers.

> +	/* tty_receive sent null ? */
> +	if (unlikely(ptr == NULL) || (st_gdata == NULL)) {
> +		pr_err(" received null from TTY ");
> +		return;
> +	}
> +
> +	pr_info("count %ld rx_state %ld"
> +		   "rx_count %ld", count, st_gdata->rx_state,
> +		   st_gdata->rx_count);

It's a debug info. + \n

> +int st_core_init(struct st_data_s **core_data)
> +{
> +	struct st_data_s *st_gdata;
> +	long err;
> +	static struct tty_ldisc_ops *st_ldisc_ops;
> +
> +	/* populate and register to TTY line discipline */
> +	st_ldisc_ops = kzalloc(sizeof(*st_ldisc_ops), GFP_KERNEL);
> +	if (!st_ldisc_ops) {
> +		pr_err("no mem to allocate");
> +		return -ENOMEM;
> +	}
> +
> +	st_ldisc_ops->magic = TTY_LDISC_MAGIC;
> +	st_ldisc_ops->name = "n_st";	/*"n_hci"; */
> +	st_ldisc_ops->open = st_tty_open;
> +	st_ldisc_ops->close = st_tty_close;
> +	st_ldisc_ops->receive_buf = st_tty_receive;
> +	st_ldisc_ops->write_wakeup = st_tty_wakeup;
> +	st_ldisc_ops->flush_buffer = st_tty_flush_buffer;
> +	st_ldisc_ops->owner = THIS_MODULE;

This can be static structure, you don't need to allocate this on heap.
It should be a singleton.

> +
> +	err = tty_register_ldisc(N_TI_WL, st_ldisc_ops);
> +	if (err) {
> +		pr_err("error registering %d line discipline %ld",
> +			   N_TI_WL, err);
> +		kfree(st_ldisc_ops);
> +		return err;
> +	}
> +	pr_debug("registered n_shared line discipline");
> +
> +	st_gdata = kzalloc(sizeof(struct st_data_s), GFP_KERNEL);
> +	if (!st_gdata) {
> +		pr_err("memory allocation failed");
> +		err = tty_unregister_ldisc(N_TI_WL);
> +		if (err)
> +			pr_err("unable to un-register ldisc %ld", err);
> +		kfree(st_ldisc_ops);
> +		err = -ENOMEM;
> +		return err;
> +	}
> +
> +	/* Initialize ST TxQ and Tx waitQ queue head. All BT/FM/GPS module skb's
> +	 * will be pushed in this queue for actual transmission.
> +	 */
> +	skb_queue_head_init(&st_gdata->txq);
> +	skb_queue_head_init(&st_gdata->tx_waitq);
> +
> +	/* Locking used in st_int_enqueue() to avoid multiple execution */
> +	spin_lock_init(&st_gdata->lock);
> +
> +	/* ldisc_ops ref to be only used in __exit of module */
> +	st_gdata->ldisc_ops = st_ldisc_ops;
> +
> +#if 0
> +	err = st_kim_init();
> +	if (err) {
> +		pr_err("error during kim initialization(%ld)", err);
> +		kfree(st_gdata);
> +		err = tty_unregister_ldisc(N_TI_WL);
> +		if (err)
> +			pr_err("unable to un-register ldisc");
> +		kfree(st_ldisc_ops);
> +		return -1;
> +	}
> +#endif
> +
> +	err = st_ll_init(st_gdata);
> +	if (err) {
> +		pr_err("error during st_ll initialization(%ld)", err);
> +		kfree(st_gdata);
> +		err = tty_unregister_ldisc(N_TI_WL);
> +		if (err)
> +			pr_err("unable to un-register ldisc");
> +		kfree(st_ldisc_ops);

Please use goto fail-paths.

> +		return -1;
> +	}
> +	*core_data = st_gdata;
> +	return 0;
> +}
...
> --- /dev/null
> +++ b/drivers/misc/ti-st/st_kim.c
> @@ -0,0 +1,798 @@
...
> +#define PROTO_ENTRY(type, name)	name
> +const unsigned char *protocol_names[] = {
> +	PROTO_ENTRY(ST_BT, "Bluetooth"),
> +	PROTO_ENTRY(ST_FM, "FM"),
> +	PROTO_ENTRY(ST_GPS, "GPS"),
> +};

Here they appear again. It should be static and have a better name to
not collide with the rest of the world.

> +#define MAX_ST_DEVICES	3	/* Imagine 1 on each UART for now */
> +struct platform_device *st_kim_devices[MAX_ST_DEVICES];

static? Doesn't sparse warn about this?

> +void kim_int_recv(struct kim_data_s *kim_gdata,
> +	const unsigned char *data, long count)
> +{
> +	register char *ptr;
> +	struct hci_event_hdr *eh;
> +	register int len = 0, type = 0;

registers

> +	pr_debug("%s", __func__);

\n

> +	/* Decode received bytes here */
> +	ptr = (char *)data;

Casting from const to non-const. It doesn't look correct.

> +static long read_local_version(struct kim_data_s *kim_gdata, char *bts_scr_name)
> +{
> +	unsigned short version = 0, chip = 0, min_ver = 0, maj_ver = 0;

No need to initialize all of them.

> +	char read_ver_cmd[] = { 0x01, 0x01, 0x10, 0x00 };

static const perhaps.

> +long st_kim_start(void *kim_data)
> +{
> +	long err = 0;
> +	long retry = POR_RETRY_COUNT;
> +	struct kim_data_s	*kim_gdata = (struct kim_data_s *)kim_data;
> +
> +	pr_info(" %s", __func__);
> +
> +	do {
> +		/* TODO: this is only because rfkill sub-system
> +		 * doesn't send events to user-space if the state
> +		 * isn't changed
> +		 */
> +		rfkill_set_hw_state(kim_gdata->rfkill[ST_BT], 1);
> +		/* Configure BT nShutdown to HIGH state */
> +		gpio_set_value(kim_gdata->gpios[ST_BT], GPIO_LOW);
> +		mdelay(5);	/* FIXME: a proper toggle */
> +		gpio_set_value(kim_gdata->gpios[ST_BT], GPIO_HIGH);
> +		mdelay(100);

You can sleep here instead (below you wait for completion). 100 ms of
busy waiting is way too much.

> +		/* re-initialize the completion */
> +		INIT_COMPLETION(kim_gdata->ldisc_installed);
> +#if 0 /* older way of signalling user-space UIM */
> +		/* send signal to UIM */
> +		err = kill_pid(find_get_pid(kim_gdata->uim_pid), SIGUSR2, 0);
> +		if (err != 0) {
> +			pr_info(" sending SIGUSR2 to uim failed %ld", err);
> +			err = -1;
> +			continue;
> +		}
> +#endif
> +		/* unblock and send event to UIM via /dev/rfkill */
> +		rfkill_set_hw_state(kim_gdata->rfkill[ST_BT], 0);
> +		/* wait for ldisc to be installed */
> +		err = wait_for_completion_timeout(&kim_gdata->ldisc_installed,
> +				msecs_to_jiffies(LDISC_TIME));

regards,
-- 
js