Re: HD6446x driver

On Fri, May 07, 2010 at 22:53:46 +0900, KIYOHARA Takashi wrote:

> I knew UART of HD64461 did not work on PERSONA recently.  This trouble
> will be corrected by me.

I guess the COM_REGMAP part of the patch does that.  Do AFE clocks
need to be enabled too?


> Moreover, because I look alike HD64461 and HD64465 very much, it proposes
> to merge these.
> # hd64461if + hd64465if => shc (SH companion)
> Do you have a big problem in merging these?  Also more better name?

It's been quite a long time since I looked at hd6446x manuals in any
details, so I don't remember how similar they are.  If we can really
merge them, I guess that's good.  OTOH, I wonder if the return of
investment is really that promising.


> Index: conf/GENERIC
> ===================================================================
> RCS file: /cvsroot/src/sys/arch/hpcsh/conf/GENERIC,v
> retrieving revision 1.87
> diff -u -r1.87 GENERIC
> --- conf/GENERIC	5 Dec 2009 20:11:14 -0000	1.87
> +++ conf/GENERIC	27 Apr 2010 03:07:39 -0000
> @@ -252,6 +252,7 @@
>  # HITACHI PERSONA (HPW-50PAD, HPQ-650PA)
>  #
>  com0		at hd64461if?
> +options 	COM_REGMAP

Shouldn't this be in std.hpcsh instead of the specific kernel config?


> @@ -107,11 +104,18 @@
>  void
>  hd64461uartcninit(struct consdev *cp)
>  {
> +	struct com_regs regs;
> +	int i;
>  
>  	hd64461uart_init();
>  
> -	comcnattach(hd64461uart_chip.io_tag, 0x0, COMCN_SPEED, COM_FREQ,
> -	    COM_TYPE_NORMAL, CONMODE);
> +	memset(&regs, 0, sizeof(regs));
> +	regs.cr_iot = hd64461uart_chip.io_tag;
> +	regs.cr_iobase = 0x00000000;
> +	regs.cr_nports = COM_NPORTS;
> +	for (i = 0; i < __arraycount(regs.cr_map); i++)
> +		regs.cr_map[i] = com_std_map[i] << 1;
> +	comcnattach1(&regs, COMCN_SPEED, COM_FREQ, COM_TYPE_NORMAL, CONMODE);
>  
>  	hd64461uart_chip.console = 1;
>  }

May be define a macro for this, like COM_INIT_REGS, but for the map we
use here.  You are doing the same initialization several times in this
file.


>  	if (strcmp(kgdb_devname, "hd64461uart") != 0)
> -		return (1);
> +		return 1;

It's better not to mix KNF changes with real changes.


> @@ -188,7 +203,7 @@
>  	com_attach_subr(csc);
>  
>  	hd6446x_intr_establish(HD64461_INTC_UART, IST_LEVEL, IPL_TTY,
> -	    comintr, self);
> +	    comintr, csc);
>  }


This looks like the real bug (fallout from splitting softc) you are
fixing.  In that case it's better to make it a separate commit.


Thanks.

SY, Uwe
-- 
[email protected]                       |       Zu Grunde kommen
http://snark.ptc.spbu.ru/~uwe/          |       Ist zu Grunde gehen