[irda-users] Re: [PATCH2.6] nsc-ircc to platform device + suspend/resume redo

[Samuel O. <sa...@so...>]

Hi Rudolf,

Rudolf Marek <r.marek@...> writes:

> I would like to know if I need to call .._change_speed
> even when the device was down to provide some defined state
> of those registers, I'm setting speed to 9600.
I don't think it is necessary, since it will be overwritten when the interface
will be brought up.
If the interface is up, it's probably a safe bet to call change_speed() with the
current speed, which is what you do.


> diff -Naur a/drivers/net/irda/nsc-ircc.c b/drivers/net/irda/nsc-ircc.c
> --- a/drivers/net/irda/nsc-ircc.c	2005-12-19 01:36:54.000000000 +0100
> +++ b/drivers/net/irda/nsc-ircc.c	2005-12-23 22:31:58.000000000 +0100
>  @@  -158,12 +172,20  @@ 
>  {
>  	chipio_t info;
>  	nsc_chip_t *chip;
> -	int ret = -ENODEV;
> +	int ret;
This is useless, since you're setting it to -ENODEV later on.

>  	int cfg_base;
>  	int cfg, id;
>  	int reg;
>  	int i = 0;
> -
> +        
> +	ret = platform_driver_register(&nsc_ircc_driver);                          
               
> +        if (ret) {                                                          
                       
> +                IRDA_ERROR("%s, Can't register driver!\n", driver_name);    
                       
> +                return ret;                                                 
                       
> +        }
> +	
> +	ret = -ENODEV;
Useless seting, see above.


>  @@  -860,7 +890,8  @@ 
> 
>  	/* Enable receive interrupts */
>  	switch_bank(iobase, BANK0);
> -	outb(IER_RXHDL_IE, iobase+IER);
> +	if (inte)
> +	    outb(IER_RXHDL_IE, iobase+IER);
Is that really necessary ? When you resume, you avoid this call, but I guess it
would be harmless anyways. It just saves one register write while adding an
obscure argument to the ircc_setup() prototype. Not worth it to my opinion.



> -static void nsc_ircc_suspend(struct nsc_ircc_cb *self)
> +static int nsc_ircc_suspend(struct platform_device *dev, pm_message_t state)
>  {
> -	IRDA_MESSAGE("%s, Suspending\n", driver_name);
> +    	struct nsc_ircc_cb *self = platform_get_drvdata(dev);
> +	int bank;
> +	unsigned long flags;
> +	int iobase = self->io.fir_base; 
> +	if (!self->io.suspended) {
> +		IRDA_DEBUG(1, "%s, Suspending\n", driver_name);
> +
> +		rtnl_lock();
> +		if (netif_running(self->netdev)) {
> +			netif_device_detach(self->netdev);
> +			spin_lock_irqsave(&self->lock, flags);
> +			/* Save current bank */
> +			bank = inb(iobase+BSR);
> +
> +			/* Disable interrupts */
> +			switch_bank(iobase, BANK0);
> +			outb(0, iobase+IER);
> 
> -	if (self->io.suspended)
> -		return;
Cosmetic remark here: This is just my preference, but I think keeping the:
     if (self->io.suspended)
   	     return;

at the beginning of the routine, and removing the:
     if (!self->io.suspended) {

looks better identation-wise. But again, that's just my personal preference.

> +		} else {
> +				printk("tady2 %d\n",self->io.speed);
> +				spin_lock_irqsave(&self->lock, flags);
> +				nsc_ircc_change_speed(self, 9600);
> +				spin_unlock_irqrestore(&self->lock, flags);
> +		}
I think this is not necessary.

Cheers,
Samuel.