Re: HD6446x driver

["Valeriy E. Ushakov" <uwe%stderr.spb.ru@localhost>]

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
-- 
uwe%stderr.spb.ru@localhost                       |       Zu Grunde kommen
http://snark.ptc.spbu.ru/~uwe/          |       Ist zu Grunde gehen