Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20230519201300.12964-1-jorge.lopez2@hp.com> <20230519201300.12964-4-jorge.lopez2@hp.com>
 <f147240d-7da8-4fd7-9201-a11476b0b9e6@t-8ch.de>
In-Reply-To: <f147240d-7da8-4fd7-9201-a11476b0b9e6@t-8ch.de>
From:   Jorge Lopez <jorgealtxwork@gmail.com>
Date:   Wed, 31 May 2023 15:09:15 -0500
Message-ID: <CAOOmCE-GWjcncveifa5=T4O3tt1wkZKM0og-7ZUMUQSdr6d5cQ@mail.gmail.com>
Subject: Re: [PATCH v15 03/13] hp-bioscfg: bioscfg
To:     =?UTF-8?Q?Thomas_Wei=C3=9Fschuh?= <thomas@t-8ch.de>
Cc:     hdegoede@redhat.com, platform-driver-x86@vger.kernel.org,
        linux-kernel@vger.kernel.org, ilpo.jarvinen@linux.intel.com
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Fri, May 26, 2023 at 10:28=E2=80=AFAM Thomas Wei=C3=9Fschuh <thomas@t-8c=
h.de> 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/pla=
tform/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@hp.com>");
> > +MODULE_DESCRIPTION("HP BIOS Configuration Driver");
> > +MODULE_LICENSE("GPL");
> > +
> > +struct bioscfg_priv bioscfg_drv =3D {
> > +     .mutex =3D __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 *int=
eger)
> > +{
> > +     int *ptr =3D 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 =3D *(ptr++);
> > +     *buffer =3D (u8 *)ptr;
> > +     *buffer_size -=3D 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 ty=
pes
> > + * respective properties under sysfs files
> > + */
> > +static int hp_init_bios_attributes(enum hp_wmi_data_type attr_type, co=
nst char *guid)
> > +{
> > +     union acpi_object *obj =3D 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 =3D 0;
> > +     int retval =3D 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
> >