Re: RFC: cleaning up j720ssp.c

On Tue, Feb 21, 2006 at 05:09:45PM +0100, Quentin Garnier wrote:
> 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:
> > > Why don't you use simply register the config_hook(9) from sed1356attach?
> > > Possibly delaying it with config_defer(9).
> > 
> > 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.
> 

I see... I'll look into it.

> 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.
> 

> > > 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.
> > > 
> > 
> > Ok, so the XXX indicates that the config hook register should actually
> > be in j720lcd.c but cannot because then sc != 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.
> 

I've added the extra layer because the SSP port is part of the SA-1110
and the keyboard, touch-panel, etc are connected to that port. To me,
it maked sense to seperate it (the hpcsh port also does similar things).

> 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.
> 
> > > 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
> > > 
> > 
> > But hpcapm_match() expects mainbus attach arguments:
> > 
> > static int
> > hpcapm_match(struct device *parent, struct cfdata *cf, void *aux)
> > {
> > 	struct mainbus_attach_args *ma = aux;
> > 
> > 	if (strcmp(ma->ma_name, hpcapm_cd.cd_name) != 0) {
> > [..]
> > 
> > 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.
> 

Ok, thanks.

-- 
Peter Postma