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: <20220327121404.1702631-1-eugene.shalygin@gmail.com>
 <20220327121404.1702631-2-eugene.shalygin@gmail.com> <16a36e23-bab2-4314-54ea-ff6b1011e6d7@roeck-us.net>
In-Reply-To: <16a36e23-bab2-4314-54ea-ff6b1011e6d7@roeck-us.net>
From:   Eugene Shalygin <eugene.shalygin@gmail.com>
Date:   Tue, 29 Mar 2022 21:22:20 +0200
Message-ID: <CAB95QATxYy5aj97P=S=L-kGE9dppuEbTYq=K=HGMBuZYVUBR4g@mail.gmail.com>
Subject: Re: [PATCH 1/4] hwmon: (asus-ec-sensors) introduce ec_board_info
 struct for board data
To:     Guenter Roeck <linux@roeck-us.net>
Cc:     darcagn@protonmail.com, Jean Delvare <jdelvare@suse.com>,
        linux-hwmon@vger.kernel.org,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Tue, 29 Mar 2022 at 15:44, Guenter Roeck <linux@roeck-us.net> wrote:
> >
> >   struct ec_sensors_data {
> > -     unsigned long board_sensors;
> > +     struct ec_board_info board_info;
>
> Please explain why this needs to be the entire structure and not
> just a pointer to it.

I marked the board_info array as __initconst assuming that this large
array will be unloaded from memory after the init phase, while we keep
only a single element. Is that assumption incorrect?

> > +static int sensor_count(const struct ec_board_info *board)
> > +{
> > +     return hweight_long(board->sensors);
> > +}
>
> This function is called several times. Does it really make sense, or is it
> necessary, to re-calculate the number of sensors over and over again
> instead of keeping it in ec->nr_sensors as before ? What are the benefits ?
> Unless there is a good explanation I see that as unrelated and unnecessary
> change.

This had something to do with data deduplication. However, I need the
count value only for looping over the sensor array, thus I can as well
add an invalid element to the end of the array. I rushed to submit
this driver to replace the wmi one, and it still has an artifact for
the WMI code I'd like to get rid of eventually, which is the read
buffer and the registers array. This will remove all the nr_ variables
and two dynamically allocated arrays. I will understand, of course, if
you ask to submit that refactoring separately.

> > -MODULE_DEVICE_TABLE(dmi, asus_ec_dmi_table);
> >   module_platform_driver_probe(asus_ec_sensors_platform_driver, asus_ec_probe);
> > +MODULE_DEVICE_TABLE(acpi, acpi_ec_ids);
>
> Why is MODULE_DEVICE_TABLE moved ?
Accidentally, probably. Thank you, will be corrected.


Thanks,
Eugene