Re: [PATCH v15 03/13] hp-bioscfg: bioscfg

[Jorge Lopez]

On Fri, May 26, 2023 at 10:28 AM Thomas Weißschuh <thomas@xxxxxxxx> wrote:
>
> On 2023-05-19 15:12:50-0500, Jorge Lopez wrote:
>
> <snip>
>
> > ---
> >  drivers/platform/x86/hp/hp-bioscfg/bioscfg.c | 988 +++++++++++++++++++
> >  1 file changed, 988 insertions(+)
> >  create mode 100644 drivers/platform/x86/hp/hp-bioscfg/bioscfg.c
> >
> > diff --git a/drivers/platform/x86/hp/hp-bioscfg/bioscfg.c b/drivers/platform/x86/hp/hp-bioscfg/bioscfg.c
> > new file mode 100644
> > index 000000000000..fe0be40c8dee
> > --- /dev/null
> > +++ b/drivers/platform/x86/hp/hp-bioscfg/bioscfg.c
> > @@ -0,0 +1,988 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Common methods for use with hp-bioscfg driver
> > + *
> > + *  Copyright (c) 2022 HP Development Company, L.P.
> > + */
> > +
> > +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> > +
> > +#include <linux/fs.h>
> > +#include <linux/module.h>
> > +#include <linux/kernel.h>
> > +#include <linux/wmi.h>
> > +#include "bioscfg.h"
> > +#include "../../firmware_attributes_class.h"
> > +#include <linux/nls.h>
> > +#include <linux/errno.h>
> > +
> > +MODULE_AUTHOR("Jorge Lopez <jorge.lopez2@xxxxxx>");
> > +MODULE_DESCRIPTION("HP BIOS Configuration Driver");
> > +MODULE_LICENSE("GPL");
> > +
> > +struct bioscfg_priv bioscfg_drv = {
> > +     .mutex = __MUTEX_INITIALIZER(bioscfg_drv.mutex),
> > +};
> > +
> > +static struct class *fw_attr_class;
> > +
> > +ssize_t display_name_language_code_show(struct kobject *kobj,
> > +                                     struct kobj_attribute *attr,
> > +                                     char *buf)
> > +{
> > +     return sysfs_emit(buf, "%s\n", bioscfg_drv.display_name_language_code);
> > +}
>
> You probably can also define the "struct attribute" for this callback
> right here.
> Then there don't have to be attribute definitions everywhere.
>
> As the language code is always the same anyways this can just print the
> constant. Also remove the variables and explicit setting from all the
> attributes.

Done!

>
> > +
> > +int hp_get_integer_from_buffer(u8 **buffer, u32 *buffer_size, u32 *integer)
> > +{
> > +     int *ptr = PTR_ALIGN((int *)*buffer, 4);
>
> 4 -> sizeof(int)

Done!

>
> > +
> > +     /* Ensure there is enough space remaining to read the integer */
> > +     if (*buffer_size < sizeof(int))
> > +             return -EINVAL;
> > +
> > +     *integer = *(ptr++);
> > +     *buffer = (u8 *)ptr;
> > +     *buffer_size -= sizeof(int);
> > +
> > +     return 0;
> > +}
> > +

<snip>

> > +/**
> > + * hp_init_bios_attributes() - Initialize all attributes for a type
> > + * @attr_type: The attribute type to initialize
> > + * @guid: The WMI GUID associated with this type to initialize
> > + *
> > + * Initialiaze all 5 types of attributes: enumeration, integer,
>
> "Initialize"

Done!

>
> > + * string, password, ordered list  object.  Populates each attrbute types
> > + * respective properties under sysfs files
> > + */
> > +static int hp_init_bios_attributes(enum hp_wmi_data_type attr_type, const char *guid)
> > +{
> > +     union acpi_object *obj = NULL;
> > +     int min_elements;
> > +
> > +     /* instance_id needs to be reset for each type GUID
> > +      * also, instance IDs are unique within GUID but not across
> > +      */
> > +     int instance_id = 0;
> > +     int retval = 0;
>
> More common would be just "ret" instead of "retval".
> Same everywhere else.

Done!

<snip>

> > +     fw_attributes_class_put();
> > +     hp_exit_attr_set_interface();
> > +}
> > +
> > +module_init(hp_init);
> > +module_exit(hp_exit);
> > --
> > 2.34.1
> >