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
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/dri-devel
