Hi Rafael,
Thanks for the review.
On 04/19, Rafael J. Wysocki wrote:
> Hi,
>
> On Tue, Apr 18, 2017 at 2:35 PM, Dmitry Frank <mail@xxxxxxxxxxxxxxx> wrote:
> > The first two items in the _BCL method response are special:
> >
> > - Level when machine has full power
> > - Level when machine is on batteries
> > - .... actual supported levels go there ....
> >
> > So this commits adds an enum and uses its descriptive elements
> > throughout the code, instead of magic numbers.
> >
> > Some subtle cases are commented additionally (the comment for
> > acpi_video_bqc_quirk is by Felipe Contreras, CCed)
>
> So that should be a separate patch. Please split.
>
Sure, done; please see the "v2" patchset. (It contains one more change:
renaming of the global flag device_id_scheme, since the old name is
misleading)
> >
> > Signed-off-by: Dmitry Frank <mail@xxxxxxxxxxxxxxx>
> > ---
> > drivers/acpi/acpi_video.c | 134 ++++++++++++++++++++++++++++++----------------
> > 1 file changed, 89 insertions(+), 45 deletions(-)
> >
> > diff --git a/drivers/acpi/acpi_video.c b/drivers/acpi/acpi_video.c
> > index d00bc0ef87a6..856694e5f325 100644
> > --- a/drivers/acpi/acpi_video.c
> > +++ b/drivers/acpi/acpi_video.c
> > @@ -88,6 +88,18 @@ static int acpi_video_bus_remove(struct acpi_device *device);
> > static void acpi_video_bus_notify(struct acpi_device *device, u32 event);
> > void acpi_video_detect_exit(void);
> >
> > +/*
> > + * Indices in the _BCL method response: the first two items are special,
> > + * the rest are all supported levels.
> > + *
> > + * See page 575 of the ACPI spec 3.0
> > + */
> > +enum lvl_idx {
> > + LVL_IDX_AC, /* level when machine has full power */
> > + LVL_IDX_BATTERY, /* level when machine is on batteries */
> > + LVL_IDX_FIRST, /* actual supported levels begin here */
>
> What about using different symbol names, like ACPI_VIDEO_AC_LEVEL,
> ACPI_VIDEO_BATTERY_LEVEL and ACPI_VIDEO_FIRST_LEVEL, respectively?
>
Honestly, I find ACPI_VIDEO_..._LEVEL a bit misleading, because these
values are not levels, they are indices of levels. So I'd probably
prefer ..._LEVEL_INDEX, or ..._LEVEL_IDX, but it probably gets too long,
so...
Anyway, the patchset v2 contains the names you proposed; if you agree
that LEVEL_INDEX or LEVEL_IDX would be better, please let me know, and
I'll be happy to craft v3. Otherwise, I'm fine with these names too.
> > +};
> > +
> > static const struct acpi_device_id video_device_ids[] = {
> > {ACPI_VIDEO_HID, 0},
> > {"", 0},
> > @@ -132,7 +144,7 @@ struct acpi_video_device_attrib {
> > the VGA device. */
> > u32 pipe_id:3; /* For VGA multiple-head devices. */
> > u32 reserved:10; /* Must be 0 */
> > - u32 device_id_scheme:1; /* Device ID Scheme */
> > + u32 device_id_scheme:1; /* Whether the device ID follows this scheme above */
>
> That still is not particularly clear (although admittedly it is better
> than what we have).
>
Ok, I added a descriptive comment above the field.
Thanks,
Dmitry
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
