Re: [PATCH] ACPI/Intel: Rework Opregion support

[Matthew Garrett]

On Tue, Mar 15, 2011 at 02:18:02AM +0100, Indan Zupancic wrote:
> > +
> > +	if (dev->set_backlight)
> > +		dev->set_backlight(dev->drm_dev, bclp * max / 255);
> 
> I would hide the max backlight from the opregion code and move this
> calculation into set_brightness. Then change the interface to one
> based on a scale from 0-255 or 0-100, which fits better with what
> opregion actually does. On the other hand, it's the same code in
> a different place, so it doesn't make much difference.

That would require an extra layer of indirection in every driver, so 
keeping it here seems reasonable.

> > +void igd_opregion_enable_asle(struct opregion_dev *dev)
> > +{
> > +	struct opregion_asle *asle = dev->opregion.asle;
> > +
> > +	if (asle && dev->enable_asle) {
> > +		dev->enable_asle(dev->drm_dev);
> > +
> > +		asle->tche = ASLE_ALS_EN | ASLE_BLC_EN | ASLE_PFIT_EN |
> > +			ASLE_PFMB_EN;
> 
> Shouldn't which flags are set depend on which callback functions are set?
> Then you can remove all the function != NULL tests too. e.g:

Our experience is that we should give BIOSes the full set of support 
flags even if we don't actually do anything with them. That way we avoid 
cases where BIOS authors refuse to do anything with partial 
implementations (even if we support everything they use) and we get 
feedback when we actually find cases of advanced functionality in the 
real world.

> > +	if (IS_MOBILE(dev)) {
> > +		dev_priv->opregion_dev.max_backlight = intel_panel_get_max_backlight(dev);
> > +		dev_priv->opregion_dev.set_backlight = intel_panel_set_backlight;
> > +	}
> > +	dev_priv->opregion_dev.enable_asle = intel_enable_asle;
> > +	dev_priv->opregion_dev.drm_dev = dev;
> 
> So we have a drm_device->drm_i915_private->opregion_dev.drm_device loop,
> which is a bit messy, but consistent with other existing code.

Anyone who wants to clean that up is welcome to do so, but really I 
optimised for ease of transition here.

> >
> > -	/* XXX Should this validation be moved to intel_opregion.c? */
> > -	if (dev_priv->opregion.vbt) {
> > -		struct vbt_header *vbt = dev_priv->opregion.vbt;
> > +	/* XXX Should this validation be moved to acpi_igd_opregion.c? */
> 
> Should it? Seems like a good moment to do so.

Now that we've got multiple consumers it's probably not helpful to move 
the (potentially chip-specific) VBT handling to general code. We've got 
zero documentation on how GMA500 handles VBT, and not a great deal more 
for i915.

> About the naming, as this is Intel-only intel_opregion seems clearer than
> igd-opregion, which sounds like it could be anything.

It's Intel-only in implementation, not in specification. As much as I 
dislike vendor-neutral specs that have been pushed and implemented by a 
single vendor, I'd rather not make the naming Intel-specific if there's 
a chance someone else can end up depending on it.

-- 
Matthew Garrett | mjg59@xxxxxxxxxxxxx
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/