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 17:09:45
--VvnGyWaJ9JuxX38U
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

On Tue, Feb 21, 2006 at 04:22:41PM +0100, Peter Postma wrote:
> On Tue, Feb 21, 2006 at 01:07:02PM +0100, Quentin Garnier wrote:
> > On Tue, Feb 21, 2006 at 12:28:26PM +0100, Peter Postma wrote:
> > > - 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.
> >=20
> > Why don't you use simply register the config_hook(9) from sed1356attach?
> > Possibly delaying it with config_defer(9).
> >=20
>=20
> That's what I'm doing now, but I don't like it. It adds knowledge of
> buttons to sed1356, it's ugly.

Ah, right.  I had not understood what the problem was.  Well, you have
several ways of solving that.  Adding the config_hook call in
sed1365attach is not that bad, anyway.  Despite the name and property
of the hook, anything can fire it;  it's way of exposing some kind of
toggle().  One other thing you can do is using the softc of instance #0
of sed(4), using sed_cd, in your sed1356_toggle_lcdlight(), or you can
have sed1356attach set a property on its parent which can later give
the j720kbd instance the softc pointer.  It's a bit like
device_register(), except it happens between siblings.

The right way to do that would be to have a more generic PM framework,
where the LCD device would register, and the keyboard driver would
trigger an event on all the attached objects.  That's something we'll
need at some point, e.g. for ACPI related stuff, but right now a simple
way is fine.

[...]
> > > > 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 XX=
X in
> > > sed_saip.c makes me think it's not...?
> >=20
> > The XXX is probably there because of this:
> >=20
> > if (platid_match(&platid, &platid_mask_MACH_HP_JORNADA_7XX)) {
> >=20
> > So yes, j720lcd.c does seem to be the correct place.  Otherwise, all
> > platforms using that combination of chips would use it.
> >=20
>=20
> Ok, so the XXX indicates that the config hook register should actually
> be in j720lcd.c but cannot because then sc !=3D sed1356_softc?

Well, it could be because it wasn't/couldn't be tested on other similar
platforms.

One thing I don't quite get in your patch though is the extra layer for
each objects.  hpckbd should attach directly to j720ssp;  the split in
different source files is fine, though.

Your j720{kbd,lcd,tp} are only avatars of j720ssp, it doesn't make sense
to simulate devices at this point.

The j720lcd device doesn't exist, that means sed_saip.c is actually
where the real stuff happens, and the platid_match() becomes more
correct under that light.  sed(4) currently is specific to the Jornada
in our code, so I don't think it matters much.

> > > > j720pwr.c     - power management (using hpcapm(4))
> > >=20
> > > Not using hpcapm(4) yet, because I can't attach devices to mainbus (a=
nd
> > > I don't think putting #ifdef hpcarm in arm/mainbus/mainbus.c is right=
, so
> > > suggestions are welcome).
> >=20
> > 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.
> >=20
> > device hpcapm: apmdevif
> > attach hpcapm at j720ssp
> > file dev/hpc/hpcapm.c hpcapm
> >=20
>=20
> But hpcapm_match() expects mainbus attach arguments:
>=20
> static int
> hpcapm_match(struct device *parent, struct cfdata *cf, void *aux)
> {
> 	struct mainbus_attach_args *ma =3D aux;
>=20
> 	if (strcmp(ma->ma_name, hpcapm_cd.cd_name) !=3D 0) {
> [..]
>=20
> Or do you suggest to work around this?

This is completely broken.  There are quite some config(9) abuses in hpc
land...  I'll have a look at hpcapm users and fix that mess.  Th
hpcapm/apmdev separation looks dubious, 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.

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

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

iQEVAwUBQ/s7SdgoQloHrPnoAQK54ggArQsDzxWdvk6m6jNRAJFAAcxrSm/fiaTB
lmidr1CFJSRLIRGk/YP8kq4ejTT/LtU9dUGUfYN59DOASJrfF53n6NO4CQt1ZsaF
cdjO7gnG7lsN6Z3Ny/FAv57GmCKPiFPD39Udt1JnxpP8zWhnxtOvLnL9WHS4dlFu
s8i8+V2Bfo15vvFog91OTp6nySb+KiknNXb3UA5l3tbs6ICoYaXYCX/OrOq9dpCP
OOHqWkP2ZvXCJ3Aa6zral3x9c8q+t+vIXWJNwIgKsxkJydFii27JKm7hSkdRrrXz
SwShqgwsICyQVtAHrHmVfI7jjVPaF6/rmof917KW0Vb7IuUytqBbOA==
=5/8b
-----END PGP SIGNATURE-----

--VvnGyWaJ9JuxX38U--