[PATCH] [POWERPC] 8xx: PQ SoC IRDA support

[Samuel Ortiz]

Hi Vitaly,

On 5/8/2007, "Vitaly Bordug" <vitb at kernel.crashing.org> wrote:

>
>Adds support of IRDA transceiver residing on PowerQUICC processors and
>enabling such on mpc885ads reference board. The driver is implemented
>using of_device concept, hereby implies arch/powerpc support of the target.
>
>Signed-off-by: Vitaly Bordug <vitb at kernel.crashing.org>
I'd really prefer this patch to be splitted in 2 parts: The PPC specific
bits, and the IrDA driver that would apply on top of it.
Meanwhile, I can comment on the IrDA driver code:


>diff --git a/drivers/net/irda/Kconfig b/drivers/net/irda/Kconfig
>index 7c8ccc0..b3681e7 100644
>--- a/drivers/net/irda/Kconfig
>+++ b/drivers/net/irda/Kconfig
>@@ -17,6 +17,10 @@ config IRTTY_SIR
>
> 	  If unsure, say Y.
>
>+config 8XX_SIR
>+	  tristate "mpc8xx SIR"
>+	  depends on 8xx && IRDA
>+
Some "help" field here wouldn't hurt.



>+static void mpc8xx_irda_rx(struct net_device *dev)
>+{
>+	struct mpc8xx_irda *si = dev->priv;
>+	cbd_t *bdp;
>+	struct sk_buff *skb;
>+	int len;
>+	ushort status;
>+
>+	bdp = si->cur_rx;
>+
>+	for (;;) {
>+		if (bdp->cbd_sc & BD_AHDLC_RX_EMPTY)
>+			break;
>+		status = bdp->cbd_sc;
>+
>+		if (status & BD_AHDLC_RX_STATS) {
>+			/* process errors
>+			 */
>+			if (bdp->cbd_sc & BD_AHDLC_RX_AB)
>+				si->stats.rx_length_errors++;
>+			if (bdp->cbd_sc & BD_AHDLC_RX_CR)	/* CRC Error */
>+				si->stats.rx_crc_errors++;
>+			if (bdp->cbd_sc & BD_AHDLC_RX_OV)	/* FIFO overrun */
>+				si->stats.rx_over_errors++;
>+		} else {
>+			/* Process the incoming frame.
>+			 */
>+			len = bdp->cbd_datlen;
>+
>+			skb = dev_alloc_skb(len + 1);
>+			if (skb == NULL) {
>+				printk(KERN_INFO
>+				       "%s: Memory squeeze, dropping packet.\n",
>+				       dev->name);
>+				si->stats.rx_dropped++;
>+			} else {
>+				skb->dev = dev;
>+				skb_reserve(skb, 1);
>+				memcpy(skb_put(skb, len),
>+				       si->rx_vaddr[bdp - si->rx_bd_base], len);
>+				skb_trim(skb, skb->len - 2);
>+
>+				si->stats.rx_packets++;
>+				si->stats.rx_bytes += len;
>+
>+				skb->mac.raw = skb->data;
This should be
+				skb_reset_mac_header(skb);


>+static irqreturn_t mpc8xx_irda_irq(int irq, void *dev_id)
>+{
>+	struct net_device *dev = dev_id;
>+	struct mpc8xx_irda *si = dev->priv;
>+	scc_t *sccp = si->sccp;
>+	cbd_t *bdp;
>+	ushort int_events;
>+	int must_restart = 0;
>+
>+	/* Get the interrupt events that caused us to be here.
>+	 */
>+	int_events = in_be16(&sccp->scc_scce);
>+	out_be16(&sccp->scc_scce, int_events);
>+
>+	/* Handle receive event in its own function.
>+	 */
>+	if (int_events & SCC_AHDLC_RXF)
>+		mpc8xx_irda_rx(dev);
>+
>+	spin_lock(&si->lock);
>+
>+	/* Transmit OK, or non-fatal error.  Update the buffer descriptors.
>+	 */
>+	if (int_events & (SCC_AHDLC_TXE | SCC_AHDLC_TXB)) {
>+		bdp = si->dirty_tx;
>+
>+		while ((bdp->cbd_sc & BD_AHDLC_TX_READY) == 0) {
>+			if (si->tx_free == TX_RING_SIZE)
>+				break;
>+
>+			if (bdp->cbd_sc & BD_AHDLC_TX_CTS)
>+				must_restart = 1;
>+
>+			si->stats.tx_packets++;
>+
>+			/* Free the sk buffer associated with this last transmit.
>+			 */
>+			dev_kfree_skb_irq(si->tx_skbuff[si->skb_dirty]);
>+			si->skb_dirty = (si->skb_dirty + 1) & TX_RING_MOD_MASK;
>+
>+			/* Update pointer to next buffer descriptor to be transmitted.
>+			 */
>+			if (bdp->cbd_sc & BD_AHDLC_TX_WRAP)
>+				bdp = si->tx_bd_base;
>+			else
>+				bdp++;
>+
>+			/* Since we have freed up a buffer, the ring is no longer full.
>+			 */
>+			if (!si->tx_free++) {
>+				if (netif_queue_stopped(dev))
>+					netif_wake_queue(dev);
>+			}
>+
>+			si->dirty_tx = (cbd_t *) bdp;
>+
>+			if (si->newspeed) {
>+				mpc8xx_irda_set_speed(dev, si->newspeed);
>+				si->speed = si->newspeed;
>+				si->newspeed = 0;
>+			}
>+		}
>+
>+		if (must_restart) {
>+			cpm8xx_t *cp = immr_map(im_cpm);
>+			printk(KERN_INFO "restart TX\n");
>+
>+			/* Some transmit errors cause the transmitter to shut
>+			 * down.  We now issue a restart transmit.  Since the
>+			 * errors close the BD and update the pointers, the restart
>+			 * _should_ pick up without having to reset any of our
>+			 * pointers either.
>+			 */
>+			out_be16(&cp->cp_cpcr,
>+			    mk_cr_cmd(CPM_CR_CH_SCC2,
>+				      CPM_CR_RESTART_TX) | CPM_CR_FLG);
>+			while (in_be16(&cp->cp_cpcr) & CPM_CR_FLG) ;
You're busy looping in an interrupt handler, that's not really nice.
In general, I think this interrupt handler would deserve a bottom half
split since it looks quite busy.


>+static struct device_driver m8xxir_driver = {
>+	.name = "fsl-cpm-scc:irda",
>+	.bus = &platform_bus_type,
>+	.probe = mpc8xx_irda_probe,
>+	.remove = mpc8xx_irda_remove,
>+};
Why not a platform driver ?

Cheers,
Samuel.