Re: [PATCH] irda: driver for Freescale FIRI controller

2008-07-08 Thread Anton Vorontsov
On Thu, Jun 12, 2008 at 12:25:25AM +0200, Samuel Ortiz wrote:
 Hi Anton,
 
 On Wed, Jun 04, 2008 at 07:45:10PM +0400, Anton Vorontsov wrote:
  From: Zhang Wei [EMAIL PROTECTED]
  
  The driver supports SIR, MIR, FIR modes and maximum 400bps rate.
  
  Signed-off-by: Zhang Wei [EMAIL PROTECTED]
  [AV: few small fixes, plus had made platform ops passing via node-data
   to avoid #ifdef stuff in the fsl_soc (think DIU). ]
  Signed-off-by: Anton Vorontsov [EMAIL PROTECTED]
 Some comments below:

Sorry, it took me so long to answer. :-/

Much thanks for the comments, will fix them all. That is, your review
work isn't in vain, I'm much appreciated it.

Thanks again,

-- 
Anton Vorontsov
email: [EMAIL PROTECTED]
irc://irc.freenode.net/bd2
___
Linuxppc-dev mailing list
Linuxppc-dev@ozlabs.org
https://ozlabs.org/mailman/listinfo/linuxppc-dev


Re: [PATCH] irda: driver for Freescale FIRI controller

2008-06-11 Thread Samuel Ortiz
Hi Anton,

On Wed, Jun 04, 2008 at 07:45:10PM +0400, Anton Vorontsov wrote:
 From: Zhang Wei [EMAIL PROTECTED]
 
 The driver supports SIR, MIR, FIR modes and maximum 400bps rate.
 
 Signed-off-by: Zhang Wei [EMAIL PROTECTED]
 [AV: few small fixes, plus had made platform ops passing via node-data
  to avoid #ifdef stuff in the fsl_soc (think DIU). ]
 Signed-off-by: Anton Vorontsov [EMAIL PROTECTED]
Some comments below:


 diff --git a/drivers/net/irda/Kconfig b/drivers/net/irda/Kconfig
 index ce816ba..da24f57 100644
 --- a/drivers/net/irda/Kconfig
 +++ b/drivers/net/irda/Kconfig
 @@ -341,5 +341,10 @@ config MCS_FIR
 To compile it as a module, choose M here: the module will be called
 mcs7780.
  
 +config FSL_FIR
 + tristate Freescale Irda driver
I guess you could be a little more descriptive here. At least specify which
chipsets this driver support, and that it is a FIR one.


 --- /dev/null
 +++ b/drivers/net/irda/fsl_ir.c
 @@ -0,0 +1,792 @@
 +/*
 + * Copyright (C) 2007 Freescale Semiconductor, Inc. All rights reserved.
 + *
 + * Author: Zhang Wei, [EMAIL PROTECTED], Oct. 2007
 + *
 + * Description:
 + * The IrDA driver for Freescale PowerPC MPC8610 processor. The driver
 + * support SIR and FIR mode. The maximum speed is 4Mbps.
 + *
 + * Changelog:
 + * Oct 2007 Zhang Wei [EMAIL PROTECTED]
 + * - Initial version.
 + *
 + * This file is part of the Linux kernel
 + *
 + * This is free software; you can redistribute it and/or modify
 + * it under the terms of the GNU General Public License as published by
 + * the Free Software Foundation; either version 2 of the License, or
 + * (at your option) any later version.
 + *
 + */
 +
 +#include linux/kernel.h
 +#include linux/interrupt.h
 +#include linux/delay.h
 +#include linux/of_platform.h
 +#include linux/fsl_ir.h
 +
 +#include net/irda/irda.h
 +#include net/irda/wrapper.h
 +#include net/irda/irda_device.h
 +#include net/irda/irlap_frame.h
 +#include net/irda/irlap.h
 +
 +#include sysdev/fsl_soc.h
 +
 +#define is_sir_speed(speed) ((speed = 115200) ? 1 : 0)
 +#define is_sir(ir) (is_sir_speed(ir-speed))
 +
 +static void init_iobuf(iobuff_t *io, void *buff, int size)
 +{
 + io-head = buff;
 + io-truesize = size;
 + io-in_frame = FALSE;
 + io-state = OUTSIDE_FRAME;
 + io-data = io-head;
 + io-skb = NULL;
 +}
 +
 +static void ir_switch_mode(struct fsl_ir *ir, u32 speed)
 +{
 +
 + if (ir-speed  (ir-speed  115200))  /* Switch from SIR to FIR */
Dont you want = 115200 here ?

 + /* Disable SIRI */
 + clrbits32(ir-reg_base-scr1, FSL_IR_SCR1_IREN |
 + FSL_IR_SCR1_SIRIEN);
 + else {  /* Switch from FIR to SIR */
 + /* Disable FIRI */
 + out_be32(ir-reg_base-firitcr, 0);
 + out_be32(ir-reg_base-firircr, 0);
 + }
 +
 + /* Switch the IrDA mode on board */
 + if (ir-plat_op  ir-plat_op-set_ir_mode)
 + ir-plat_op-set_ir_mode(ir, speed);
 +}
 +
 +static int ir_crc_len(struct fsl_ir *ir)
 +{
 + int crc_len;
 +
 + switch (ir-speed) {
 + case 576000:
 + case 1152000:
 + crc_len = 2;/* CRC16, 16 bits */
 + break;
 + case 400:
 + crc_len = 4;/* CRC32, 32 bits */
 + break;
 + default:
 + crc_len = 0;
 + break;
 + }
 + return crc_len;
 +}
 +
 +static void fir_rx(struct fsl_ir *ir, int len)
 +{
 + struct net_device *ndev = dev_get_drvdata(ir-dev);
 + struct sk_buff *skb;
 + int i;
 +
 + if (len = ir_crc_len(ir))
 + return;
 +
 + do_gettimeofday(ir-stamp);
 + /* Now, for new packet arriving */
 + skb = alloc_skb(len + 1, GFP_ATOMIC);
Please use dev_alloc_skb here.

 + if (!skb) {
 + ir-stats.rx_dropped++;
 + return;
 + }
 + skb_reserve(skb, 1);
A comment here describing why you need a 1 byte header would be nice.


 +
 + for (i = 0; i  len; i++)
 + skb-data[i] = in_8((u8 *)ir-reg_base-rfifo);
 +
 + len -= ir_crc_len(ir);
 + skb_put(skb, len);
 +
 + ir-stats.rx_packets++;
 + ir-stats.rx_bytes += len;
 +
 + skb-dev = ndev;
 + skb_reset_mac_header(skb);
 + skb-protocol = htons(ETH_P_IRDA);
 + netif_rx(skb);
 +}
On a general note, this RX routine could be part of a bottom half.
You're calling this from the IRQ handler, and this may not be a good idea.


 +
 +static void fir_tx(struct fsl_ir *ir)
 +{
 + int free_bytes;
 + struct sk_buff *skb = ir-tx_buff.skb;
 + size_t len;
 +
 + if (!skb)
 + return;
 +
 + spin_lock(ir-tx_lock);
 + do {
 + free_bytes = 128 -
 + ((in_be32(ir-reg_base-firitsr)  8)  0xff);
 + for (len = min(free_bytes, ir-tx_buff.len); len  0;
 + len--, ir-tx_buff.len--)
 + out_8((u8 *)ir-reg_base-tfifo,