Message ID | 1428055432-12120-17-git-send-email-zhaoshenglong@huawei.com |
---|---|
State | New |
Headers | show |
Shannon Zhao <zhaoshenglong@huawei.com> writes: > From: Shannon Zhao <shannon.zhao@linaro.org> > > Signed-off-by: Shannon Zhao <zhaoshenglong@huawei.com> > Signed-off-by: Shannon Zhao <shannon.zhao@linaro.org> I was considering if the magic numbers should be defined somewhere but I guess that is the point of the stub functions. Reviewed-by: Alex Bennée <alex.bennee@linaro.org> > --- > hw/acpi/aml-build.c | 7 +++++++ > include/hw/acpi/aml-build.h | 1 + > 2 files changed, 8 insertions(+) > > diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c > index 4f936f7..9b8b422 100644 > --- a/hw/acpi/aml-build.c > +++ b/hw/acpi/aml-build.c > @@ -626,6 +626,13 @@ Aml *aml_if(Aml *predicate) > return var; > } > > +/* ACPI 1.0: 16.2.3 Operators: DefElse */ > +Aml *aml_else(void) > +{ > + Aml *var = aml_bundle(0xA1 /* ElseOp */, AML_PACKAGE); > + return var; > +} > + > /* ACPI 1.0b: 16.2.5.2 Named Objects Encoding: DefMethod */ > Aml *aml_method(const char *name, int arg_count) > { > diff --git a/include/hw/acpi/aml-build.h b/include/hw/acpi/aml-build.h > index 6b407d5..3901515 100644 > --- a/include/hw/acpi/aml-build.h > +++ b/include/hw/acpi/aml-build.h > @@ -206,6 +206,7 @@ Aml *aml_scope(const char *name_format, ...) GCC_FMT_ATTR(1, 2); > Aml *aml_device(const char *name_format, ...) GCC_FMT_ATTR(1, 2); > Aml *aml_method(const char *name, int arg_count); > Aml *aml_if(Aml *predicate); > +Aml *aml_else(void); > Aml *aml_package(uint8_t num_elements); > Aml *aml_buffer(void); > Aml *aml_resource_template(void);
On 2015/4/13 23:55, Alex Bennée wrote: > > Shannon Zhao <zhaoshenglong@huawei.com> writes: > >> From: Shannon Zhao <shannon.zhao@linaro.org> >> >> Signed-off-by: Shannon Zhao <zhaoshenglong@huawei.com> >> Signed-off-by: Shannon Zhao <shannon.zhao@linaro.org> > > I was considering if the magic numbers should be defined somewhere but I > guess that is the point of the stub functions. > Yes, I think so. We can add another patch to do this. > Reviewed-by: Alex Bennée <alex.bennee@linaro.org> > > >> --- >> hw/acpi/aml-build.c | 7 +++++++ >> include/hw/acpi/aml-build.h | 1 + >> 2 files changed, 8 insertions(+) >> >> diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c >> index 4f936f7..9b8b422 100644 >> --- a/hw/acpi/aml-build.c >> +++ b/hw/acpi/aml-build.c >> @@ -626,6 +626,13 @@ Aml *aml_if(Aml *predicate) >> return var; >> } >> >> +/* ACPI 1.0: 16.2.3 Operators: DefElse */ >> +Aml *aml_else(void) >> +{ >> + Aml *var = aml_bundle(0xA1 /* ElseOp */, AML_PACKAGE); >> + return var; >> +} >> + >> /* ACPI 1.0b: 16.2.5.2 Named Objects Encoding: DefMethod */ >> Aml *aml_method(const char *name, int arg_count) >> { >> diff --git a/include/hw/acpi/aml-build.h b/include/hw/acpi/aml-build.h >> index 6b407d5..3901515 100644 >> --- a/include/hw/acpi/aml-build.h >> +++ b/include/hw/acpi/aml-build.h >> @@ -206,6 +206,7 @@ Aml *aml_scope(const char *name_format, ...) GCC_FMT_ATTR(1, 2); >> Aml *aml_device(const char *name_format, ...) GCC_FMT_ATTR(1, 2); >> Aml *aml_method(const char *name, int arg_count); >> Aml *aml_if(Aml *predicate); >> +Aml *aml_else(void); >> Aml *aml_package(uint8_t num_elements); >> Aml *aml_buffer(void); >> Aml *aml_resource_template(void); >
On Mon, Apr 13, 2015 at 04:55:13PM +0100, Alex Bennée wrote: > > Shannon Zhao <zhaoshenglong@huawei.com> writes: > > > From: Shannon Zhao <shannon.zhao@linaro.org> > > > > Signed-off-by: Shannon Zhao <zhaoshenglong@huawei.com> > > Signed-off-by: Shannon Zhao <shannon.zhao@linaro.org> > > I was considering if the magic numbers should be defined somewhere but I > guess that is the point of the stub functions. I don't think so: wrappers make sure each is used only once. enums would just obfuscate code. comments are better in that they match the language used in the acpi spec, exactly. > Reviewed-by: Alex Bennée <alex.bennee@linaro.org> > > > --- > > hw/acpi/aml-build.c | 7 +++++++ > > include/hw/acpi/aml-build.h | 1 + > > 2 files changed, 8 insertions(+) > > > > diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c > > index 4f936f7..9b8b422 100644 > > --- a/hw/acpi/aml-build.c > > +++ b/hw/acpi/aml-build.c > > @@ -626,6 +626,13 @@ Aml *aml_if(Aml *predicate) > > return var; > > } > > > > +/* ACPI 1.0: 16.2.3 Operators: DefElse */ > > +Aml *aml_else(void) > > +{ > > + Aml *var = aml_bundle(0xA1 /* ElseOp */, AML_PACKAGE); > > + return var; > > +} > > + > > /* ACPI 1.0b: 16.2.5.2 Named Objects Encoding: DefMethod */ > > Aml *aml_method(const char *name, int arg_count) > > { > > diff --git a/include/hw/acpi/aml-build.h b/include/hw/acpi/aml-build.h > > index 6b407d5..3901515 100644 > > --- a/include/hw/acpi/aml-build.h > > +++ b/include/hw/acpi/aml-build.h > > @@ -206,6 +206,7 @@ Aml *aml_scope(const char *name_format, ...) GCC_FMT_ATTR(1, 2); > > Aml *aml_device(const char *name_format, ...) GCC_FMT_ATTR(1, 2); > > Aml *aml_method(const char *name, int arg_count); > > Aml *aml_if(Aml *predicate); > > +Aml *aml_else(void); > > Aml *aml_package(uint8_t num_elements); > > Aml *aml_buffer(void); > > Aml *aml_resource_template(void); > > -- > Alex Bennée
On 2015/4/14 13:56, Michael S. Tsirkin wrote: > On Mon, Apr 13, 2015 at 04:55:13PM +0100, Alex Bennée wrote: >> >> Shannon Zhao <zhaoshenglong@huawei.com> writes: >> >>> From: Shannon Zhao <shannon.zhao@linaro.org> >>> >>> Signed-off-by: Shannon Zhao <zhaoshenglong@huawei.com> >>> Signed-off-by: Shannon Zhao <shannon.zhao@linaro.org> >> >> I was considering if the magic numbers should be defined somewhere but I >> guess that is the point of the stub functions. > > I don't think so: wrappers make sure each is used > only once. enums would just obfuscate code. > comments are better in that they match the language > used in the acpi spec, exactly. > I think this has nothing to do with the Opcode used once or other. Maybe the Opcodes should be defined at one place, then have a whole scope of the Opcodes and drop the magic numbers. >> Reviewed-by: Alex Bennée <alex.bennee@linaro.org> > >> >>> --- >>> hw/acpi/aml-build.c | 7 +++++++ >>> include/hw/acpi/aml-build.h | 1 + >>> 2 files changed, 8 insertions(+) >>> >>> diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c >>> index 4f936f7..9b8b422 100644 >>> --- a/hw/acpi/aml-build.c >>> +++ b/hw/acpi/aml-build.c >>> @@ -626,6 +626,13 @@ Aml *aml_if(Aml *predicate) >>> return var; >>> } >>> >>> +/* ACPI 1.0: 16.2.3 Operators: DefElse */ >>> +Aml *aml_else(void) >>> +{ >>> + Aml *var = aml_bundle(0xA1 /* ElseOp */, AML_PACKAGE); >>> + return var; >>> +} >>> + >>> /* ACPI 1.0b: 16.2.5.2 Named Objects Encoding: DefMethod */ >>> Aml *aml_method(const char *name, int arg_count) >>> { >>> diff --git a/include/hw/acpi/aml-build.h b/include/hw/acpi/aml-build.h >>> index 6b407d5..3901515 100644 >>> --- a/include/hw/acpi/aml-build.h >>> +++ b/include/hw/acpi/aml-build.h >>> @@ -206,6 +206,7 @@ Aml *aml_scope(const char *name_format, ...) GCC_FMT_ATTR(1, 2); >>> Aml *aml_device(const char *name_format, ...) GCC_FMT_ATTR(1, 2); >>> Aml *aml_method(const char *name, int arg_count); >>> Aml *aml_if(Aml *predicate); >>> +Aml *aml_else(void); >>> Aml *aml_package(uint8_t num_elements); >>> Aml *aml_buffer(void); >>> Aml *aml_resource_template(void); >> >> -- >> Alex Bennée > > > . >
On Tue, 14 Apr 2015 14:38:18 +0800 Shannon Zhao <zhaoshenglong@huawei.com> wrote: > On 2015/4/14 13:56, Michael S. Tsirkin wrote: > > On Mon, Apr 13, 2015 at 04:55:13PM +0100, Alex Bennée wrote: > >> > >> Shannon Zhao <zhaoshenglong@huawei.com> writes: > >> > >>> From: Shannon Zhao <shannon.zhao@linaro.org> > >>> > >>> Signed-off-by: Shannon Zhao <zhaoshenglong@huawei.com> > >>> Signed-off-by: Shannon Zhao <shannon.zhao@linaro.org> > >> > >> I was considering if the magic numbers should be defined somewhere > >> but I guess that is the point of the stub functions. > > > > I don't think so: wrappers make sure each is used > > only once. enums would just obfuscate code. > > comments are better in that they match the language > > used in the acpi spec, exactly. > > > > I think this has nothing to do with the Opcode used once or other. > Maybe the Opcodes should be defined at one place, then have a whole > scope of the Opcodes and drop the magic numbers. I agree with Michael, With one off usage, It's easier to read code with magic numbers and comments, without having to jump around to find out what define or enum equals. > > >> Reviewed-by: Alex Bennée <alex.bennee@linaro.org> > > > >> > >>> --- > >>> hw/acpi/aml-build.c | 7 +++++++ > >>> include/hw/acpi/aml-build.h | 1 + > >>> 2 files changed, 8 insertions(+) > >>> > >>> diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c > >>> index 4f936f7..9b8b422 100644 > >>> --- a/hw/acpi/aml-build.c > >>> +++ b/hw/acpi/aml-build.c > >>> @@ -626,6 +626,13 @@ Aml *aml_if(Aml *predicate) > >>> return var; > >>> } > >>> > >>> +/* ACPI 1.0: 16.2.3 Operators: DefElse */ > >>> +Aml *aml_else(void) > >>> +{ > >>> + Aml *var = aml_bundle(0xA1 /* ElseOp */, AML_PACKAGE); > >>> + return var; > >>> +} > >>> + > >>> /* ACPI 1.0b: 16.2.5.2 Named Objects Encoding: DefMethod */ > >>> Aml *aml_method(const char *name, int arg_count) > >>> { > >>> diff --git a/include/hw/acpi/aml-build.h > >>> b/include/hw/acpi/aml-build.h index 6b407d5..3901515 100644 > >>> --- a/include/hw/acpi/aml-build.h > >>> +++ b/include/hw/acpi/aml-build.h > >>> @@ -206,6 +206,7 @@ Aml *aml_scope(const char *name_format, ...) > >>> GCC_FMT_ATTR(1, 2); Aml *aml_device(const char *name_format, ...) > >>> GCC_FMT_ATTR(1, 2); Aml *aml_method(const char *name, int > >>> arg_count); Aml *aml_if(Aml *predicate); > >>> +Aml *aml_else(void); > >>> Aml *aml_package(uint8_t num_elements); > >>> Aml *aml_buffer(void); > >>> Aml *aml_resource_template(void); > >> > >> -- > >> Alex Bennée > > > > > > . > > > > >
diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c index 4f936f7..9b8b422 100644 --- a/hw/acpi/aml-build.c +++ b/hw/acpi/aml-build.c @@ -626,6 +626,13 @@ Aml *aml_if(Aml *predicate) return var; } +/* ACPI 1.0: 16.2.3 Operators: DefElse */ +Aml *aml_else(void) +{ + Aml *var = aml_bundle(0xA1 /* ElseOp */, AML_PACKAGE); + return var; +} + /* ACPI 1.0b: 16.2.5.2 Named Objects Encoding: DefMethod */ Aml *aml_method(const char *name, int arg_count) { diff --git a/include/hw/acpi/aml-build.h b/include/hw/acpi/aml-build.h index 6b407d5..3901515 100644 --- a/include/hw/acpi/aml-build.h +++ b/include/hw/acpi/aml-build.h @@ -206,6 +206,7 @@ Aml *aml_scope(const char *name_format, ...) GCC_FMT_ATTR(1, 2); Aml *aml_device(const char *name_format, ...) GCC_FMT_ATTR(1, 2); Aml *aml_method(const char *name, int arg_count); Aml *aml_if(Aml *predicate); +Aml *aml_else(void); Aml *aml_package(uint8_t num_elements); Aml *aml_buffer(void); Aml *aml_resource_template(void);