Re: [PATCH net-next v2 2/4] can: cc770: add legacy ISA bus driver for the CC770 and AN82527

[Marc Kleine-Budde <mkl@pengutronix.de>]

[-- Attachment #1: Type: text/plain, Size: 2839 bytes --]

On 01/11/2012 10:05 AM, Wolfgang Zarre wrote:
[...]

The patch looks good, it has some style issues though, see comments
inline. Please fix them, add a description to the patch, don't forget
your S-o-b, same applies to the other patch which removes the extra IRQ
ack. Please create a series of two patches, first this (the spinlock
patch), the second the rermoval of the extra IRQ ack. Send this patches
to this list. See [1] for more information about patch submitting.

Marc

[1] http://lxr.linux.no/linux+v3.2/Documentation/SubmittingPatches#L86

> Test was successful, 2.000.000 telegrams again without problems.
> Here so far the patch as You suggested:
> 
> -------------------------------------------------------------------
>  diff --git a/drivers/net/can/cc770/cc770_isa.c
> b/drivers/net/can/cc770/cc770_isa.c
> index 4be5fe2..adf3e45 100644
> --- a/drivers/net/can/cc770/cc770_isa.c
> +++ b/drivers/net/can/cc770/cc770_isa.c
> @@ -110,6 +110,10 @@ MODULE_PARM_DESC(bcr, "Bus configuration register
> (default=0x40 [CBY])");
>  #define CC770_IOSIZE          0x20
>  #define CC770_IOSIZE_INDIRECT 0x02
> 
> +/* Spinlock for cc770_isa_port_write_reg_indirect */

The spinlock is used in read_reg_indirect, too

> +static DEFINE_SPINLOCK(cc770_isa_port_lock);
> +
> +

Just one new blank line please.

>  static struct platform_device *cc770_isa_devs[MAXDEV];
> 
>  static u8 cc770_isa_mem_read_reg(const struct cc770_priv *priv, int reg)
> @@ -138,18 +142,26 @@ static u8 cc770_isa_port_read_reg_indirect(const
> struct cc770_priv *priv,
>                           int reg)
>  {
>      unsigned long base = (unsigned long)priv->reg_base;
> +    unsigned long flags;
> +    u8 val;
> 
> +    spin_lock_irqsave(&cc770_isa_port_lock, flags);
>      outb(reg, base);
> -    return inb(base + 1);
> +    val = inb(base + 1);
> +    spin_unlock_irqrestore(&cc770_isa_port_lock, flags);

Please insert a newline before return.

> +    return val;
>  }
> 
>  static void cc770_isa_port_write_reg_indirect(const struct cc770_priv
> *priv,
>                          int reg, u8 val)
>  {
>      unsigned long base = (unsigned long)priv->reg_base;
> +    unsigned long flags;
> 
> +    spin_lock_irqsave(&cc770_isa_port_lock, flags);
>      outb(reg, base);
>      outb(val, base + 1);
> +    spin_unlock_irqrestore(&cc770_isa_port_lock, flags);    
>  }
> 
>  static int __devinit cc770_isa_probe(struct platform_device *pdev)
> -------------------------------------------------------


-- 
Pengutronix e.K.                  | Marc Kleine-Budde           |
Industrial Linux Solutions        | Phone: +49-231-2826-924     |
Vertretung West/Dortmund          | Fax:   +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686  | http://www.pengutronix.de   |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 262 bytes --]