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

Hi Vitaly,

On 5/8/2007, "Vitaly Bordug" [off-list ref] 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 <redacted>

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:

Some "help" field here wouldn't hurt.

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

This should be
+=09=09=09=09skb_reset_mac_header(skb);

+static irqreturn_t mpc8xx_irda_irq(int irq, void *dev_id)
+{
+=09struct net_device *dev =3D dev_id;
+=09struct mpc8xx_irda *si =3D dev->priv;
+=09scc_t *sccp =3D si->sccp;
+=09cbd_t *bdp;
+=09ushort int_events;
+=09int must_restart =3D 0;
+
+=09/* Get the interrupt events that caused us to be here.
+=09 */
+=09int_events =3D in_be16(&sccp->scc_scce);
+=09out_be16(&sccp->scc_scce, int_events);
+
+=09/* Handle receive event in its own function.
+=09 */
+=09if (int_events & SCC_AHDLC_RXF)
+=09=09mpc8xx_irda_rx(dev);
+
+=09spin_lock(&si->lock);
+
+=09/* Transmit OK, or non-fatal error.  Update the buffer descriptors.
+=09 */
+=09if (int_events & (SCC_AHDLC_TXE | SCC_AHDLC_TXB)) {
+=09=09bdp =3D si->dirty_tx;
+
+=09=09while ((bdp->cbd_sc & BD_AHDLC_TX_READY) =3D=3D 0) {
+=09=09=09if (si->tx_free =3D=3D TX_RING_SIZE)
+=09=09=09=09break;
+
+=09=09=09if (bdp->cbd_sc & BD_AHDLC_TX_CTS)
+=09=09=09=09must_restart =3D 1;
+
+=09=09=09si->stats.tx_packets++;
+
+=09=09=09/* Free the sk buffer associated with this last transmit.
+=09=09=09 */
+=09=09=09dev_kfree_skb_irq(si->tx_skbuff[si->skb_dirty]);
+=09=09=09si->skb_dirty =3D (si->skb_dirty + 1) & TX_RING_MOD_MASK;
+
+=09=09=09/* Update pointer to next buffer descriptor to be transmitted.
+=09=09=09 */
+=09=09=09if (bdp->cbd_sc & BD_AHDLC_TX_WRAP)
+=09=09=09=09bdp =3D si->tx_bd_base;
+=09=09=09else
+=09=09=09=09bdp++;
+
+=09=09=09/* Since we have freed up a buffer, the ring is no longer full.
+=09=09=09 */
+=09=09=09if (!si->tx_free++) {
+=09=09=09=09if (netif_queue_stopped(dev))
+=09=09=09=09=09netif_wake_queue(dev);
+=09=09=09}
+
+=09=09=09si->dirty_tx =3D (cbd_t *) bdp;
+
+=09=09=09if (si->newspeed) {
+=09=09=09=09mpc8xx_irda_set_speed(dev, si->newspeed);
+=09=09=09=09si->speed =3D si->newspeed;
+=09=09=09=09si->newspeed =3D 0;
+=09=09=09}
+=09=09}
+
+=09=09if (must_restart) {
+=09=09=09cpm8xx_t *cp =3D immr_map(im_cpm);
+=09=09=09printk(KERN_INFO "restart TX\n");
+
+=09=09=09/* Some transmit errors cause the transmitter to shut
+=09=09=09 * down.  We now issue a restart transmit.  Since the
+=09=09=09 * errors close the BD and update the pointers, the restart
+=09=09=09 * _should_ pick up without having to reset any of our
+=09=09=09 * pointers either.
+=09=09=09 */
+=09=09=09out_be16(&cp->cp_cpcr,
+=09=09=09    mk_cr_cmd(CPM_CR_CH_SCC2,
+=09=09=09=09      CPM_CR_RESTART_TX) | CPM_CR_FLG);
+=09=09=09while (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 =3D {
+=09.name =3D "fsl-cpm-scc:irda",
+=09.bus =3D &platform_bus_type,
+=09.probe =3D mpc8xx_irda_probe,
+=09.remove =3D mpc8xx_irda_remove,
+};

Why not a platform driver ?

Cheers,
Samuel.