On 3/29/22 12:22, Eugene Shalygin wrote:
On Tue, 29 Mar 2022 at 15:44, Guenter Roeck <linux@xxxxxxxxxxxx> 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?
What happens if you build the driver into the kernel and then instantiate
and de-instantiate it multiple times ?
+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.
The rule of "one logical change per patch" still applies. If you start
intermixing parts of future clean-up efforts into current patches, you'll
see a very unhappy maintainer - especially since this change makes up
a significant part of this patch, complicates review significantly,
and makes me wonder if other unrelated changes are included that I don't
see right now due to all the noise.
Besides, at least in this patch, I don't buy the "deduplication" argument.
Keeping a single additional variable in a data structure is much simpler
and straightforward than calling hweight_long() several times. I'd call
that "complification".
Guenter
