On Fri, Feb 12, 2021 at 3:14 PM Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx> wrote: > > Refactor acpi_data_prop_read_single() for less LOCs and better maintenance. > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx> > --- > drivers/acpi/property.c | 80 ++++++++++++++++++----------------------- > 1 file changed, 34 insertions(+), 46 deletions(-) > > diff --git a/drivers/acpi/property.c b/drivers/acpi/property.c > index e312ebaed8db..494cf283a573 100644 > --- a/drivers/acpi/property.c > +++ b/drivers/acpi/property.c > @@ -785,60 +785,48 @@ static int acpi_data_prop_read_single(const struct acpi_device_data *data, > enum dev_prop_type proptype, void *val) > { > const union acpi_object *obj; > - int ret; > + int ret = 0; > > - if (proptype >= DEV_PROP_U8 && proptype <= DEV_PROP_U64) { > + if (proptype >= DEV_PROP_U8 && proptype <= DEV_PROP_U64) > ret = acpi_data_get_property(data, propname, ACPI_TYPE_INTEGER, &obj); > - if (ret) > - return ret; > - > - switch (proptype) { > - case DEV_PROP_U8: > - if (obj->integer.value > U8_MAX) > - return -EOVERFLOW; > - > - if (val) > - *(u8 *)val = obj->integer.value; > - > - break; The empty lines of code above are intentional, so please retain them. > - case DEV_PROP_U16: > - if (obj->integer.value > U16_MAX) > - return -EOVERFLOW; > - > - if (val) > - *(u16 *)val = obj->integer.value; > - > - break; > - case DEV_PROP_U32: > - if (obj->integer.value > U32_MAX) > - return -EOVERFLOW; > - > - if (val) > - *(u32 *)val = obj->integer.value; > - > - break; > - default: > - if (val) > - *(u64 *)val = obj->integer.value; > - > - break; > - } > - > - if (!val) > - return 1; > - } else if (proptype == DEV_PROP_STRING) { > + else if (proptype == DEV_PROP_STRING) > ret = acpi_data_get_property(data, propname, ACPI_TYPE_STRING, &obj); > - if (ret) > - return ret; > + if (ret) > + return ret; else if (!val) ret = 1; > > + switch (proptype) { > + case DEV_PROP_U8: > + if (obj->integer.value > U8_MAX) > + return -EOVERFLOW; > + if (val) > + *(u8 *)val = obj->integer.value; > + break; > + case DEV_PROP_U16: > + if (obj->integer.value > U16_MAX) > + return -EOVERFLOW; > + if (val) > + *(u16 *)val = obj->integer.value; > + break; > + case DEV_PROP_U32: > + if (obj->integer.value > U32_MAX) > + return -EOVERFLOW; > + if (val) > + *(u32 *)val = obj->integer.value; > + break; > + case DEV_PROP_U64: > + if (val) > + *(u64 *)val = obj->integer.value; > + break; > + case DEV_PROP_STRING: > if (val) > *(char **)val = obj->string.pointer; > - > return 1; > - } else { > - ret = -EINVAL; > + default: > + return -EINVAL; > } > - return ret; Retain this. > + > + /* When no storage provided return number of available values */ > + return val ? 0 : 1; And this is just not looking good to me, sorry. > } > > static int acpi_copy_property_array_u8(const union acpi_object *items, u8 *val, > --