Subject: Re: RFC: cleaning up j720ssp.c
To: None <port-hpcarm@NetBSD.org>
From: Quentin Garnier <cube@cubidou.net>
List: port-hpcarm
Date: 02/21/2006 13:07:02
--GAoked8QSizNecZ5
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

On Tue, Feb 21, 2006 at 12:28:26PM +0100, Peter Postma wrote:
> On Tue, Jan 17, 2006 at 03:30:21PM +0100, Peter Postma wrote:
> > j720ssp.c contains drivers for the touch-panel, keyboard, lcd control
> > and battery status. I'd like to split this into multiple files, because
> > it's a bit messy now and having each driver in a separate file is much
> > cleaner. It will then also be easier then to improve the drivers (e.g.
> > changing keyboard driver to use hpckbd(4)). This is what I propose:
> >=20
>=20
> Changes to hpcarm files are available here:
> ftp://ftp.netbsd.org/pub/NetBSD/misc/peter/j720-cleanup.diff
>=20
> New files are available here:
> ftp://ftp.netbsd.org/pub/NetBSD/misc/peter/j720-cleanup.new
>=20
> The following files have been removed: apm.c, j720kbdmap.c.

Those look mostly fine.

[...]
> > j720kbd.c     - keyboard driver (using hpckbd(4))
>=20
> Three issues remain here:
>=20
> - When hpckbd goes into polling mode, I need to disable the keyboard
> interrupt, but this does not seem to be possible with hpckbd (maybe we
> should add a callback in hpckbd_cnpollc?). This is not a big problem
> because polling works, but it can generate some debug messages (which is
> actually not a big problem either).
>=20
> - Button event is #if 0'ed and in sed_saip.c (framebuffer driver) for
> now. Eventually, sed_saip.c should be modified to support lcd light
> toggling, but then it should keep a global instance of sed1356_softc.

Why don't you use simply register the config_hook(9) from sed1356attach?
Possibly delaying it with config_defer(9).

> An alternative might be to call the lcd light power hook in
> j720kbd_button_event, but this is not optimal because the onoff value
> passed by hpckbd can be wrong (and actually is wrong at the first keypres=
s,
> it should at least be initialized to 1 -- assuming lcd light is on at
> boottime).

This is clearly wrong.

[...]
> > j720lcd.c     - lcd contrast/brightness/power control
>=20
> I'm not sure if j720lcd_power is at the right place, because it seems more
> to be specific to epson 1356 <-> sa1110 (sed_saip.c). However, the XXX in
> sed_saip.c makes me think it's not...?

The XXX is probably there because of this:

if (platid_match(&platid, &platid_mask_MACH_HP_JORNADA_7XX)) {

So yes, j720lcd.c does seem to be the correct place.  Otherwise, all
platforms using that combination of chips would use it.

> > j720pwr.c     - power management (using hpcapm(4))
>=20
> Not using hpcapm(4) yet, because I can't attach devices to mainbus (and
> I don't think putting #ifdef hpcarm in arm/mainbus/mainbus.c is right, so
> suggestions are welcome).

Who says you must attach hpcapm to mainbus?  Looking at hpcsh, the port
is responsible for defining the hpcapm device, so you can attach it to
anything you want.

device hpcapm: apmdevif
attach hpcapm at j720ssp
file dev/hpc/hpcapm.c hpcapm

> > j720tp.c      - touch-panel driver
>=20
> Seems to work fine.

Looks fine, too.

--=20
Quentin Garnier - cube@cubidou.net - cube@NetBSD.org
"When I find the controls, I'll go where I like, I'll know where I want
to be, but maybe for now I'll stay right here on a silent sea."
KT Tunstall, Silent Sea, Eye to the Telescope, 2004.

--GAoked8QSizNecZ5
Content-Type: application/pgp-signature
Content-Disposition: inline

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.2.6 (NetBSD)

iQEVAwUBQ/sCZtgoQloHrPnoAQIZBQf/TaFJw78XhvDPr/Bj4iEcnxcbnG/2Ur4l
llIr79/FqwU40q/hRt7g9V18B5ub6151rwwUJLcf2f4zNbTTae9vfEiE9hfO/x4S
V27o+FUj/tbYmvuD245bblc6jFIFYS8PM3Icdie6QiQ15/rGavgqzy5B6EzC5Qa5
qqf8mzgdMzClMF2r/B17K8Fp+AHlh+bf5JIsPXWonB5YrdtTaOyhlrWEvHMdQwPF
YIBUtWC02ygFyqVld+V7vodwwwz+0gzswUIkExcDf3i23c6Ii83xnkVB7bMD1KoD
MtTsvxGpwkrzzufBZ/uBmdgMxtNcXDmbKnYLBI8g6zhW6zT1FlsDiQ==
=lZGn
-----END PGP SIGNATURE-----

--GAoked8QSizNecZ5--