Re: RFC: cleaning up j720ssp.c

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

> > 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 keypress,
> > it should at least be initialized to 1 -- assuming lcd light is on at
> > boottime).
> 
> This is clearly wrong.
> 

Yes.

> > > j720lcd.c     - lcd contrast/brightness/power control
> > 
> > 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.
> 

Ok, so the XXX indicates that the config hook register should actually
be in j720lcd.c but cannot because then sc != sed1356_softc?

> > > j720pwr.c     - power management (using hpcapm(4))
> > 
> > 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
> 

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?

> > > j720tp.c      - touch-panel driver
> > 
> > Seems to work fine.
> 
> Looks fine, too.
> 

Thanks for your comments.

-- 
Peter Postma