| Message ID | 20220327121404.1702631-2-eugene.shalygin@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | asus-ec-sensors: add support for board families | expand |
On 3/27/22 05:14, Eugene Shalygin wrote: > We need to keep some more information about the current board than just > the sensors set, and with more boards to add the dmi id array grows > quickly. Our probe code is always the same so let's switch to a custom > test code and a custom board info array. That allows us to omit board > vendor string (ASUS uses two strings that differ in case) in the board > info and use case-insensitive comparison, and also do not duplicate > sensor definitions for such board variants as " (WI-FI)" when sensors > are identical to the base variant. > > Also saves a quarter of the module size by replacing big dmi_system_id > structs with smaller ones. > > Signed-off-by: Eugene Shalygin <eugene.shalygin@gmail.com> > --- > drivers/hwmon/asus-ec-sensors.c | 209 ++++++++++++++++++-------------- > 1 file changed, 119 insertions(+), 90 deletions(-) > > diff --git a/drivers/hwmon/asus-ec-sensors.c b/drivers/hwmon/asus-ec-sensors.c > index b5cf0136360c..7e28fc62f717 100644 > --- a/drivers/hwmon/asus-ec-sensors.c > +++ b/drivers/hwmon/asus-ec-sensors.c > @@ -54,8 +54,7 @@ static char *mutex_path_override; > /* ACPI mutex for locking access to the EC for the firmware */ > #define ASUS_HW_ACCESS_MUTEX_ASMX "\\AMW0.ASMX" > > -/* There are two variants of the vendor spelling */ > -#define VENDOR_ASUS_UPPER_CASE "ASUSTeK COMPUTER INC." > +#define MAX_IDENTICAL_BOARD_VARIATIONS 2 > > typedef union { > u32 value; > @@ -164,68 +163,88 @@ static const struct ec_sensor_info known_ec_sensors[] = { > (SENSOR_TEMP_CHIPSET | SENSOR_TEMP_CPU | SENSOR_TEMP_MB) > #define SENSOR_SET_TEMP_WATER (SENSOR_TEMP_WATER_IN | SENSOR_TEMP_WATER_OUT) > > -#define DMI_EXACT_MATCH_BOARD(vendor, name, sensors) { \ > - .matches = { \ > - DMI_EXACT_MATCH(DMI_BOARD_VENDOR, vendor), \ > - DMI_EXACT_MATCH(DMI_BOARD_NAME, name), \ > - }, \ > - .driver_data = (void *)(sensors), \ > -} > +struct ec_board_info { > + const char *board_names[MAX_IDENTICAL_BOARD_VARIATIONS]; > + unsigned long sensors; > +}; > > -static const struct dmi_system_id asus_ec_dmi_table[] __initconst = { > - DMI_EXACT_MATCH_BOARD(VENDOR_ASUS_UPPER_CASE, "PRIME X570-PRO", > - SENSOR_SET_TEMP_CHIPSET_CPU_MB | SENSOR_TEMP_VRM | > - SENSOR_TEMP_T_SENSOR | SENSOR_FAN_CHIPSET), > - DMI_EXACT_MATCH_BOARD(VENDOR_ASUS_UPPER_CASE, "Pro WS X570-ACE", > - SENSOR_SET_TEMP_CHIPSET_CPU_MB | SENSOR_TEMP_VRM | > - SENSOR_FAN_CHIPSET | SENSOR_CURR_CPU | SENSOR_IN_CPU_CORE), > - DMI_EXACT_MATCH_BOARD(VENDOR_ASUS_UPPER_CASE, > - "ROG CROSSHAIR VIII DARK HERO", > - SENSOR_SET_TEMP_CHIPSET_CPU_MB | SENSOR_TEMP_T_SENSOR | > - SENSOR_TEMP_VRM | SENSOR_SET_TEMP_WATER | > - SENSOR_FAN_CPU_OPT | SENSOR_FAN_WATER_FLOW | > - SENSOR_CURR_CPU | SENSOR_IN_CPU_CORE), > - DMI_EXACT_MATCH_BOARD(VENDOR_ASUS_UPPER_CASE, > - "ROG CROSSHAIR VIII FORMULA", > - SENSOR_SET_TEMP_CHIPSET_CPU_MB | SENSOR_TEMP_T_SENSOR | > - SENSOR_TEMP_VRM | SENSOR_FAN_CPU_OPT | SENSOR_FAN_CHIPSET | > - SENSOR_CURR_CPU | SENSOR_IN_CPU_CORE), > - DMI_EXACT_MATCH_BOARD(VENDOR_ASUS_UPPER_CASE, "ROG CROSSHAIR VIII HERO", > - SENSOR_SET_TEMP_CHIPSET_CPU_MB | SENSOR_TEMP_T_SENSOR | > - SENSOR_TEMP_VRM | SENSOR_SET_TEMP_WATER | > - SENSOR_FAN_CPU_OPT | SENSOR_FAN_CHIPSET | > - SENSOR_FAN_WATER_FLOW | SENSOR_CURR_CPU | SENSOR_IN_CPU_CORE), > - DMI_EXACT_MATCH_BOARD(VENDOR_ASUS_UPPER_CASE, > - "ROG CROSSHAIR VIII HERO (WI-FI)", > - SENSOR_SET_TEMP_CHIPSET_CPU_MB | SENSOR_TEMP_T_SENSOR | > - SENSOR_TEMP_VRM | SENSOR_SET_TEMP_WATER | > - SENSOR_FAN_CPU_OPT | SENSOR_FAN_CHIPSET | > - SENSOR_FAN_WATER_FLOW | SENSOR_CURR_CPU | SENSOR_IN_CPU_CORE), > - DMI_EXACT_MATCH_BOARD(VENDOR_ASUS_UPPER_CASE, > - "ROG CROSSHAIR VIII IMPACT", > - SENSOR_SET_TEMP_CHIPSET_CPU_MB | SENSOR_TEMP_T_SENSOR | > - SENSOR_TEMP_VRM | SENSOR_FAN_CHIPSET | > - SENSOR_CURR_CPU | SENSOR_IN_CPU_CORE), > - DMI_EXACT_MATCH_BOARD(VENDOR_ASUS_UPPER_CASE, "ROG STRIX B550-E GAMING", > - SENSOR_SET_TEMP_CHIPSET_CPU_MB | > - SENSOR_TEMP_T_SENSOR | > - SENSOR_TEMP_VRM | SENSOR_FAN_CPU_OPT), > - DMI_EXACT_MATCH_BOARD(VENDOR_ASUS_UPPER_CASE, "ROG STRIX B550-I GAMING", > - SENSOR_SET_TEMP_CHIPSET_CPU_MB | > - SENSOR_TEMP_T_SENSOR | > - SENSOR_TEMP_VRM | SENSOR_FAN_VRM_HS | > - SENSOR_CURR_CPU | SENSOR_IN_CPU_CORE), > - DMI_EXACT_MATCH_BOARD(VENDOR_ASUS_UPPER_CASE, "ROG STRIX X570-E GAMING", > - SENSOR_SET_TEMP_CHIPSET_CPU_MB | > - SENSOR_TEMP_T_SENSOR | > - SENSOR_TEMP_VRM | SENSOR_FAN_CHIPSET | > - SENSOR_CURR_CPU | SENSOR_IN_CPU_CORE), > - DMI_EXACT_MATCH_BOARD(VENDOR_ASUS_UPPER_CASE, "ROG STRIX X570-F GAMING", > - SENSOR_SET_TEMP_CHIPSET_CPU_MB | > - SENSOR_TEMP_T_SENSOR | SENSOR_FAN_CHIPSET), > - DMI_EXACT_MATCH_BOARD(VENDOR_ASUS_UPPER_CASE, "ROG STRIX X570-I GAMING", > - SENSOR_TEMP_T_SENSOR | SENSOR_FAN_VRM_HS | > - SENSOR_FAN_CHIPSET | SENSOR_CURR_CPU | SENSOR_IN_CPU_CORE), > +static const struct ec_board_info board_info[] __initconst = { > + { > + .board_names = {"PRIME X570-PRO"}, > + .sensors = SENSOR_SET_TEMP_CHIPSET_CPU_MB | SENSOR_TEMP_VRM | > + SENSOR_TEMP_T_SENSOR | SENSOR_FAN_CHIPSET, > + }, > + { > + .board_names = {"Pro WS X570-ACE"}, > + .sensors = SENSOR_SET_TEMP_CHIPSET_CPU_MB | SENSOR_TEMP_VRM | > + SENSOR_FAN_CHIPSET | SENSOR_CURR_CPU | > + SENSOR_IN_CPU_CORE, > + }, > + { > + .board_names = {"ROG CROSSHAIR VIII DARK HERO"}, > + .sensors = SENSOR_SET_TEMP_CHIPSET_CPU_MB | > + SENSOR_TEMP_T_SENSOR | > + SENSOR_TEMP_VRM | SENSOR_SET_TEMP_WATER | > + SENSOR_FAN_CPU_OPT | SENSOR_FAN_WATER_FLOW | > + SENSOR_CURR_CPU | SENSOR_IN_CPU_CORE, > + }, > + { > + .board_names = {"ROG CROSSHAIR VIII FORMULA"}, > + .sensors = SENSOR_SET_TEMP_CHIPSET_CPU_MB | > + SENSOR_TEMP_T_SENSOR | SENSOR_TEMP_VRM | > + SENSOR_FAN_CPU_OPT | SENSOR_FAN_CHIPSET | > + SENSOR_CURR_CPU | SENSOR_IN_CPU_CORE, > + }, > + { > + .board_names = { > + "ROG CROSSHAIR VIII HERO", > + "ROG CROSSHAIR VIII HERO (WI-FI)", > + }, > + .sensors = SENSOR_SET_TEMP_CHIPSET_CPU_MB | > + SENSOR_TEMP_T_SENSOR | > + SENSOR_TEMP_VRM | SENSOR_SET_TEMP_WATER | > + SENSOR_FAN_CPU_OPT | SENSOR_FAN_CHIPSET | > + SENSOR_FAN_WATER_FLOW | SENSOR_CURR_CPU | > + SENSOR_IN_CPU_CORE, > + }, > + { > + .board_names = {"ROG CROSSHAIR VIII IMPACT"}, > + .sensors = SENSOR_SET_TEMP_CHIPSET_CPU_MB | > + SENSOR_TEMP_T_SENSOR | SENSOR_TEMP_VRM | > + SENSOR_FAN_CHIPSET | SENSOR_CURR_CPU | > + SENSOR_IN_CPU_CORE, > + }, > + { > + .board_names = {"ROG STRIX B550-E GAMING"}, > + .sensors = SENSOR_SET_TEMP_CHIPSET_CPU_MB | > + SENSOR_TEMP_T_SENSOR | SENSOR_TEMP_VRM | > + SENSOR_FAN_CPU_OPT, > + }, > + { > + .board_names = {"ROG STRIX B550-I GAMING"}, > + .sensors = SENSOR_SET_TEMP_CHIPSET_CPU_MB | > + SENSOR_TEMP_T_SENSOR | SENSOR_TEMP_VRM | > + SENSOR_FAN_VRM_HS | SENSOR_CURR_CPU | > + SENSOR_IN_CPU_CORE, > + }, > + { > + .board_names = {"ROG STRIX X570-E GAMING"}, > + .sensors = SENSOR_SET_TEMP_CHIPSET_CPU_MB | > + SENSOR_TEMP_T_SENSOR | SENSOR_TEMP_VRM | > + SENSOR_FAN_CHIPSET | SENSOR_CURR_CPU | > + SENSOR_IN_CPU_CORE, > + }, > + { > + .board_names = {"ROG STRIX X570-F GAMING"}, > + .sensors = SENSOR_SET_TEMP_CHIPSET_CPU_MB | > + SENSOR_TEMP_T_SENSOR | SENSOR_FAN_CHIPSET, > + }, > + { > + .board_names = {"ROG STRIX X570-I GAMING"}, > + .sensors = SENSOR_TEMP_T_SENSOR | SENSOR_FAN_VRM_HS | > + SENSOR_FAN_CHIPSET | SENSOR_CURR_CPU | > + SENSOR_IN_CPU_CORE, > + }, > {} > }; > > @@ -235,7 +254,7 @@ struct ec_sensor { > }; > > 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. > struct ec_sensor *sensors; > /* EC registers to read from */ > u16 *registers; > @@ -245,8 +264,6 @@ struct ec_sensors_data { > /* in jiffies */ > unsigned long last_updated; > acpi_handle aml_mutex; > - /* number of board EC sensors */ > - u8 nr_sensors; > /* > * number of EC registers to read > * (sensor might span more than 1 register) > @@ -281,12 +298,17 @@ get_sensor_info(const struct ec_sensors_data *state, int index) > return &known_ec_sensors[state->sensors[index].info_index]; > } > > +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. > + > static int find_ec_sensor_index(const struct ec_sensors_data *ec, > enum hwmon_sensor_types type, int channel) > { > unsigned int i; > > - for (i = 0; i < ec->nr_sensors; i++) { > + for (i = 0; i < sensor_count(&ec->board_info); i++) { > if (get_sensor_info(ec, i)->type == type) { > if (channel == 0) > return i; > @@ -301,11 +323,6 @@ static int __init bank_compare(const void *a, const void *b) > return *((const s8 *)a) - *((const s8 *)b); > } > > -static int __init board_sensors_count(unsigned long sensors) > -{ > - return hweight_long(sensors); > -} > - > static void __init setup_sensor_data(struct ec_sensors_data *ec) > { > struct ec_sensor *s = ec->sensors; > @@ -316,8 +333,8 @@ static void __init setup_sensor_data(struct ec_sensors_data *ec) > ec->nr_banks = 0; > ec->nr_registers = 0; > > - for_each_set_bit(i, &ec->board_sensors, > - BITS_PER_TYPE(ec->board_sensors)) { > + for_each_set_bit(i, &ec->board_info.sensors, > + BITS_PER_TYPE(ec->board_info.sensors)) { > s->info_index = i; > s->cached_value = 0; > ec->nr_registers += > @@ -343,7 +360,7 @@ static void __init fill_ec_registers(struct ec_sensors_data *ec) > const struct ec_sensor_info *si; > unsigned int i, j, register_idx = 0; > > - for (i = 0; i < ec->nr_sensors; ++i) { > + for (i = 0; i < sensor_count(&ec->board_info); ++i) { > si = get_sensor_info(ec, i); > for (j = 0; j < si->addr.components.size; ++j, ++register_idx) { > ec->registers[register_idx] = > @@ -457,9 +474,10 @@ static inline s32 get_sensor_value(const struct ec_sensor_info *si, u8 *data) > static void update_sensor_values(struct ec_sensors_data *ec, u8 *data) > { > const struct ec_sensor_info *si; > - struct ec_sensor *s; > + struct ec_sensor *s, *sensor_end; > > - for (s = ec->sensors; s != ec->sensors + ec->nr_sensors; s++) { > + sensor_end = ec->sensors + sensor_count(&ec->board_info); > + for (s = ec->sensors; s != sensor_end; s++) { > si = &known_ec_sensors[s->info_index]; > s->cached_value = get_sensor_value(si, data); > data += si->addr.components.size; > @@ -597,12 +615,24 @@ static struct hwmon_chip_info asus_ec_chip_info = { > .ops = &asus_ec_hwmon_ops, > }; > > -static unsigned long __init get_board_sensors(void) > +static const struct ec_board_info * __init get_board_info(void) > { > - const struct dmi_system_id *dmi_entry = > - dmi_first_match(asus_ec_dmi_table); > + const char *dmi_board_vendor = dmi_get_system_info(DMI_BOARD_VENDOR); > + const char *dmi_board_name = dmi_get_system_info(DMI_BOARD_NAME); > + const struct ec_board_info *board; > + > + if (!dmi_board_vendor || !dmi_board_name || > + strcasecmp(dmi_board_vendor, "ASUSTeK COMPUTER INC.")) > + return NULL; > + > + for (board = board_info; board->sensors; board++) { > + if (match_string(board->board_names, > + MAX_IDENTICAL_BOARD_VARIATIONS, > + dmi_board_name) >= 0) > + return board; > + } > > - return dmi_entry ? (unsigned long)dmi_entry->driver_data : 0; > + return NULL; > } > > static int __init asus_ec_probe(struct platform_device *pdev) > @@ -610,17 +640,17 @@ static int __init asus_ec_probe(struct platform_device *pdev) > const struct hwmon_channel_info **ptr_asus_ec_ci; > int nr_count[hwmon_max] = { 0 }, nr_types = 0; > struct hwmon_channel_info *asus_ec_hwmon_chan; > + const struct ec_board_info *pboard_info; > const struct hwmon_chip_info *chip_info; > struct device *dev = &pdev->dev; > struct ec_sensors_data *ec_data; > const struct ec_sensor_info *si; > enum hwmon_sensor_types type; > - unsigned long board_sensors; > struct device *hwdev; > unsigned int i; > > - board_sensors = get_board_sensors(); > - if (!board_sensors) > + pboard_info = get_board_info(); > + if (!pboard_info) > return -ENODEV; > > ec_data = devm_kzalloc(dev, sizeof(struct ec_sensors_data), > @@ -629,9 +659,8 @@ static int __init asus_ec_probe(struct platform_device *pdev) > return -ENOMEM; > > dev_set_drvdata(dev, ec_data); > - ec_data->board_sensors = board_sensors; > - ec_data->nr_sensors = board_sensors_count(ec_data->board_sensors); > - ec_data->sensors = devm_kcalloc(dev, ec_data->nr_sensors, > + ec_data->board_info = *pboard_info; > + ec_data->sensors = devm_kcalloc(dev, sensor_count(&ec_data->board_info), > sizeof(struct ec_sensor), GFP_KERNEL); > > setup_sensor_data(ec_data); > @@ -647,7 +676,7 @@ static int __init asus_ec_probe(struct platform_device *pdev) > > ec_data->aml_mutex = asus_hw_access_mutex(dev); > > - for (i = 0; i < ec_data->nr_sensors; ++i) { > + for (i = 0; i < sensor_count(&ec_data->board_info); ++i) { > si = get_sensor_info(ec_data, i); > if (!nr_count[si->type]) > ++nr_types; > @@ -681,7 +710,7 @@ static int __init asus_ec_probe(struct platform_device *pdev) > } > > dev_info(dev, "board has %d EC sensors that span %d registers", > - ec_data->nr_sensors, ec_data->nr_registers); > + sensor_count(&ec_data->board_info), ec_data->nr_registers); > > hwdev = devm_hwmon_device_register_with_info(dev, "asusec", > ec_data, chip_info, NULL); > @@ -703,8 +732,8 @@ static struct platform_driver asus_ec_sensors_platform_driver = { > }, > }; > > -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 ? > > module_param_named(mutex_path, mutex_path_override, charp, 0); > MODULE_PARM_DESC(mutex_path,
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
On 3/29/22 12:22, Eugene Shalygin wrote: > 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? > 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
On Tue, 29 Mar 2022 at 22:28, Guenter Roeck <linux@roeck-us.net> wrote: > > On 3/29/22 12:22, Eugene Shalygin wrote: > > 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? > > > > What happens if you build the driver into the kernel and then instantiate > and de-instantiate it multiple times ? Sorry, I have no idea because I don't know how to load a built-in driver multiple times. But since this driver is attached to a motherboard device, which is persistent and always single, do I need to consider such a scenario? > > >>> +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". OK, I'll roll it back until I remove the other size variables and arrays. Best regards, Eugene
On 3/30/22 00:51, Eugene Shalygin wrote: > On Tue, 29 Mar 2022 at 22:28, Guenter Roeck <linux@roeck-us.net> wrote: >> >> On 3/29/22 12:22, Eugene Shalygin wrote: >>> 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? >>> >> >> What happens if you build the driver into the kernel and then instantiate >> and de-instantiate it multiple times ? > > Sorry, I have no idea because I don't know how to load a built-in > driver multiple times. But since this driver is attached to a > motherboard device, which is persistent and always single, do I need > to consider such a scenario? > Drivers have "unbind" and "bind" attributes, which can be used to unbind/bind the driver mutliple times. That is quite useful for testing, including for built-in drivers. As long as the attributes exists, they have to be supported. This is not about having to consider a scenario, it is about preventing crashes if existing functionality is used. Having said that, I notice that the probe function is marked __init. I guess I didn't pay attention. It may be interesting to build the driver into the kernel, unbind/bind it, and see what happens. Guenter
diff --git a/drivers/hwmon/asus-ec-sensors.c b/drivers/hwmon/asus-ec-sensors.c index b5cf0136360c..7e28fc62f717 100644 --- a/drivers/hwmon/asus-ec-sensors.c +++ b/drivers/hwmon/asus-ec-sensors.c @@ -54,8 +54,7 @@ static char *mutex_path_override; /* ACPI mutex for locking access to the EC for the firmware */ #define ASUS_HW_ACCESS_MUTEX_ASMX "\\AMW0.ASMX" -/* There are two variants of the vendor spelling */ -#define VENDOR_ASUS_UPPER_CASE "ASUSTeK COMPUTER INC." +#define MAX_IDENTICAL_BOARD_VARIATIONS 2 typedef union { u32 value; @@ -164,68 +163,88 @@ static const struct ec_sensor_info known_ec_sensors[] = { (SENSOR_TEMP_CHIPSET | SENSOR_TEMP_CPU | SENSOR_TEMP_MB) #define SENSOR_SET_TEMP_WATER (SENSOR_TEMP_WATER_IN | SENSOR_TEMP_WATER_OUT) -#define DMI_EXACT_MATCH_BOARD(vendor, name, sensors) { \ - .matches = { \ - DMI_EXACT_MATCH(DMI_BOARD_VENDOR, vendor), \ - DMI_EXACT_MATCH(DMI_BOARD_NAME, name), \ - }, \ - .driver_data = (void *)(sensors), \ -} +struct ec_board_info { + const char *board_names[MAX_IDENTICAL_BOARD_VARIATIONS]; + unsigned long sensors; +}; -static const struct dmi_system_id asus_ec_dmi_table[] __initconst = { - DMI_EXACT_MATCH_BOARD(VENDOR_ASUS_UPPER_CASE, "PRIME X570-PRO", - SENSOR_SET_TEMP_CHIPSET_CPU_MB | SENSOR_TEMP_VRM | - SENSOR_TEMP_T_SENSOR | SENSOR_FAN_CHIPSET), - DMI_EXACT_MATCH_BOARD(VENDOR_ASUS_UPPER_CASE, "Pro WS X570-ACE", - SENSOR_SET_TEMP_CHIPSET_CPU_MB | SENSOR_TEMP_VRM | - SENSOR_FAN_CHIPSET | SENSOR_CURR_CPU | SENSOR_IN_CPU_CORE), - DMI_EXACT_MATCH_BOARD(VENDOR_ASUS_UPPER_CASE, - "ROG CROSSHAIR VIII DARK HERO", - SENSOR_SET_TEMP_CHIPSET_CPU_MB | SENSOR_TEMP_T_SENSOR | - SENSOR_TEMP_VRM | SENSOR_SET_TEMP_WATER | - SENSOR_FAN_CPU_OPT | SENSOR_FAN_WATER_FLOW | - SENSOR_CURR_CPU | SENSOR_IN_CPU_CORE), - DMI_EXACT_MATCH_BOARD(VENDOR_ASUS_UPPER_CASE, - "ROG CROSSHAIR VIII FORMULA", - SENSOR_SET_TEMP_CHIPSET_CPU_MB | SENSOR_TEMP_T_SENSOR | - SENSOR_TEMP_VRM | SENSOR_FAN_CPU_OPT | SENSOR_FAN_CHIPSET | - SENSOR_CURR_CPU | SENSOR_IN_CPU_CORE), - DMI_EXACT_MATCH_BOARD(VENDOR_ASUS_UPPER_CASE, "ROG CROSSHAIR VIII HERO", - SENSOR_SET_TEMP_CHIPSET_CPU_MB | SENSOR_TEMP_T_SENSOR | - SENSOR_TEMP_VRM | SENSOR_SET_TEMP_WATER | - SENSOR_FAN_CPU_OPT | SENSOR_FAN_CHIPSET | - SENSOR_FAN_WATER_FLOW | SENSOR_CURR_CPU | SENSOR_IN_CPU_CORE), - DMI_EXACT_MATCH_BOARD(VENDOR_ASUS_UPPER_CASE, - "ROG CROSSHAIR VIII HERO (WI-FI)", - SENSOR_SET_TEMP_CHIPSET_CPU_MB | SENSOR_TEMP_T_SENSOR | - SENSOR_TEMP_VRM | SENSOR_SET_TEMP_WATER | - SENSOR_FAN_CPU_OPT | SENSOR_FAN_CHIPSET | - SENSOR_FAN_WATER_FLOW | SENSOR_CURR_CPU | SENSOR_IN_CPU_CORE), - DMI_EXACT_MATCH_BOARD(VENDOR_ASUS_UPPER_CASE, - "ROG CROSSHAIR VIII IMPACT", - SENSOR_SET_TEMP_CHIPSET_CPU_MB | SENSOR_TEMP_T_SENSOR | - SENSOR_TEMP_VRM | SENSOR_FAN_CHIPSET | - SENSOR_CURR_CPU | SENSOR_IN_CPU_CORE), - DMI_EXACT_MATCH_BOARD(VENDOR_ASUS_UPPER_CASE, "ROG STRIX B550-E GAMING", - SENSOR_SET_TEMP_CHIPSET_CPU_MB | - SENSOR_TEMP_T_SENSOR | - SENSOR_TEMP_VRM | SENSOR_FAN_CPU_OPT), - DMI_EXACT_MATCH_BOARD(VENDOR_ASUS_UPPER_CASE, "ROG STRIX B550-I GAMING", - SENSOR_SET_TEMP_CHIPSET_CPU_MB | - SENSOR_TEMP_T_SENSOR | - SENSOR_TEMP_VRM | SENSOR_FAN_VRM_HS | - SENSOR_CURR_CPU | SENSOR_IN_CPU_CORE), - DMI_EXACT_MATCH_BOARD(VENDOR_ASUS_UPPER_CASE, "ROG STRIX X570-E GAMING", - SENSOR_SET_TEMP_CHIPSET_CPU_MB | - SENSOR_TEMP_T_SENSOR | - SENSOR_TEMP_VRM | SENSOR_FAN_CHIPSET | - SENSOR_CURR_CPU | SENSOR_IN_CPU_CORE), - DMI_EXACT_MATCH_BOARD(VENDOR_ASUS_UPPER_CASE, "ROG STRIX X570-F GAMING", - SENSOR_SET_TEMP_CHIPSET_CPU_MB | - SENSOR_TEMP_T_SENSOR | SENSOR_FAN_CHIPSET), - DMI_EXACT_MATCH_BOARD(VENDOR_ASUS_UPPER_CASE, "ROG STRIX X570-I GAMING", - SENSOR_TEMP_T_SENSOR | SENSOR_FAN_VRM_HS | - SENSOR_FAN_CHIPSET | SENSOR_CURR_CPU | SENSOR_IN_CPU_CORE), +static const struct ec_board_info board_info[] __initconst = { + { + .board_names = {"PRIME X570-PRO"}, + .sensors = SENSOR_SET_TEMP_CHIPSET_CPU_MB | SENSOR_TEMP_VRM | + SENSOR_TEMP_T_SENSOR | SENSOR_FAN_CHIPSET, + }, + { + .board_names = {"Pro WS X570-ACE"}, + .sensors = SENSOR_SET_TEMP_CHIPSET_CPU_MB | SENSOR_TEMP_VRM | + SENSOR_FAN_CHIPSET | SENSOR_CURR_CPU | + SENSOR_IN_CPU_CORE, + }, + { + .board_names = {"ROG CROSSHAIR VIII DARK HERO"}, + .sensors = SENSOR_SET_TEMP_CHIPSET_CPU_MB | + SENSOR_TEMP_T_SENSOR | + SENSOR_TEMP_VRM | SENSOR_SET_TEMP_WATER | + SENSOR_FAN_CPU_OPT | SENSOR_FAN_WATER_FLOW | + SENSOR_CURR_CPU | SENSOR_IN_CPU_CORE, + }, + { + .board_names = {"ROG CROSSHAIR VIII FORMULA"}, + .sensors = SENSOR_SET_TEMP_CHIPSET_CPU_MB | + SENSOR_TEMP_T_SENSOR | SENSOR_TEMP_VRM | + SENSOR_FAN_CPU_OPT | SENSOR_FAN_CHIPSET | + SENSOR_CURR_CPU | SENSOR_IN_CPU_CORE, + }, + { + .board_names = { + "ROG CROSSHAIR VIII HERO", + "ROG CROSSHAIR VIII HERO (WI-FI)", + }, + .sensors = SENSOR_SET_TEMP_CHIPSET_CPU_MB | + SENSOR_TEMP_T_SENSOR | + SENSOR_TEMP_VRM | SENSOR_SET_TEMP_WATER | + SENSOR_FAN_CPU_OPT | SENSOR_FAN_CHIPSET | + SENSOR_FAN_WATER_FLOW | SENSOR_CURR_CPU | + SENSOR_IN_CPU_CORE, + }, + { + .board_names = {"ROG CROSSHAIR VIII IMPACT"}, + .sensors = SENSOR_SET_TEMP_CHIPSET_CPU_MB | + SENSOR_TEMP_T_SENSOR | SENSOR_TEMP_VRM | + SENSOR_FAN_CHIPSET | SENSOR_CURR_CPU | + SENSOR_IN_CPU_CORE, + }, + { + .board_names = {"ROG STRIX B550-E GAMING"}, + .sensors = SENSOR_SET_TEMP_CHIPSET_CPU_MB | + SENSOR_TEMP_T_SENSOR | SENSOR_TEMP_VRM | + SENSOR_FAN_CPU_OPT, + }, + { + .board_names = {"ROG STRIX B550-I GAMING"}, + .sensors = SENSOR_SET_TEMP_CHIPSET_CPU_MB | + SENSOR_TEMP_T_SENSOR | SENSOR_TEMP_VRM | + SENSOR_FAN_VRM_HS | SENSOR_CURR_CPU | + SENSOR_IN_CPU_CORE, + }, + { + .board_names = {"ROG STRIX X570-E GAMING"}, + .sensors = SENSOR_SET_TEMP_CHIPSET_CPU_MB | + SENSOR_TEMP_T_SENSOR | SENSOR_TEMP_VRM | + SENSOR_FAN_CHIPSET | SENSOR_CURR_CPU | + SENSOR_IN_CPU_CORE, + }, + { + .board_names = {"ROG STRIX X570-F GAMING"}, + .sensors = SENSOR_SET_TEMP_CHIPSET_CPU_MB | + SENSOR_TEMP_T_SENSOR | SENSOR_FAN_CHIPSET, + }, + { + .board_names = {"ROG STRIX X570-I GAMING"}, + .sensors = SENSOR_TEMP_T_SENSOR | SENSOR_FAN_VRM_HS | + SENSOR_FAN_CHIPSET | SENSOR_CURR_CPU | + SENSOR_IN_CPU_CORE, + }, {} }; @@ -235,7 +254,7 @@ struct ec_sensor { }; struct ec_sensors_data { - unsigned long board_sensors; + struct ec_board_info board_info; struct ec_sensor *sensors; /* EC registers to read from */ u16 *registers; @@ -245,8 +264,6 @@ struct ec_sensors_data { /* in jiffies */ unsigned long last_updated; acpi_handle aml_mutex; - /* number of board EC sensors */ - u8 nr_sensors; /* * number of EC registers to read * (sensor might span more than 1 register) @@ -281,12 +298,17 @@ get_sensor_info(const struct ec_sensors_data *state, int index) return &known_ec_sensors[state->sensors[index].info_index]; } +static int sensor_count(const struct ec_board_info *board) +{ + return hweight_long(board->sensors); +} + static int find_ec_sensor_index(const struct ec_sensors_data *ec, enum hwmon_sensor_types type, int channel) { unsigned int i; - for (i = 0; i < ec->nr_sensors; i++) { + for (i = 0; i < sensor_count(&ec->board_info); i++) { if (get_sensor_info(ec, i)->type == type) { if (channel == 0) return i; @@ -301,11 +323,6 @@ static int __init bank_compare(const void *a, const void *b) return *((const s8 *)a) - *((const s8 *)b); } -static int __init board_sensors_count(unsigned long sensors) -{ - return hweight_long(sensors); -} - static void __init setup_sensor_data(struct ec_sensors_data *ec) { struct ec_sensor *s = ec->sensors; @@ -316,8 +333,8 @@ static void __init setup_sensor_data(struct ec_sensors_data *ec) ec->nr_banks = 0; ec->nr_registers = 0; - for_each_set_bit(i, &ec->board_sensors, - BITS_PER_TYPE(ec->board_sensors)) { + for_each_set_bit(i, &ec->board_info.sensors, + BITS_PER_TYPE(ec->board_info.sensors)) { s->info_index = i; s->cached_value = 0; ec->nr_registers += @@ -343,7 +360,7 @@ static void __init fill_ec_registers(struct ec_sensors_data *ec) const struct ec_sensor_info *si; unsigned int i, j, register_idx = 0; - for (i = 0; i < ec->nr_sensors; ++i) { + for (i = 0; i < sensor_count(&ec->board_info); ++i) { si = get_sensor_info(ec, i); for (j = 0; j < si->addr.components.size; ++j, ++register_idx) { ec->registers[register_idx] = @@ -457,9 +474,10 @@ static inline s32 get_sensor_value(const struct ec_sensor_info *si, u8 *data) static void update_sensor_values(struct ec_sensors_data *ec, u8 *data) { const struct ec_sensor_info *si; - struct ec_sensor *s; + struct ec_sensor *s, *sensor_end; - for (s = ec->sensors; s != ec->sensors + ec->nr_sensors; s++) { + sensor_end = ec->sensors + sensor_count(&ec->board_info); + for (s = ec->sensors; s != sensor_end; s++) { si = &known_ec_sensors[s->info_index]; s->cached_value = get_sensor_value(si, data); data += si->addr.components.size; @@ -597,12 +615,24 @@ static struct hwmon_chip_info asus_ec_chip_info = { .ops = &asus_ec_hwmon_ops, }; -static unsigned long __init get_board_sensors(void) +static const struct ec_board_info * __init get_board_info(void) { - const struct dmi_system_id *dmi_entry = - dmi_first_match(asus_ec_dmi_table); + const char *dmi_board_vendor = dmi_get_system_info(DMI_BOARD_VENDOR); + const char *dmi_board_name = dmi_get_system_info(DMI_BOARD_NAME); + const struct ec_board_info *board; + + if (!dmi_board_vendor || !dmi_board_name || + strcasecmp(dmi_board_vendor, "ASUSTeK COMPUTER INC.")) + return NULL; + + for (board = board_info; board->sensors; board++) { + if (match_string(board->board_names, + MAX_IDENTICAL_BOARD_VARIATIONS, + dmi_board_name) >= 0) + return board; + } - return dmi_entry ? (unsigned long)dmi_entry->driver_data : 0; + return NULL; } static int __init asus_ec_probe(struct platform_device *pdev) @@ -610,17 +640,17 @@ static int __init asus_ec_probe(struct platform_device *pdev) const struct hwmon_channel_info **ptr_asus_ec_ci; int nr_count[hwmon_max] = { 0 }, nr_types = 0; struct hwmon_channel_info *asus_ec_hwmon_chan; + const struct ec_board_info *pboard_info; const struct hwmon_chip_info *chip_info; struct device *dev = &pdev->dev; struct ec_sensors_data *ec_data; const struct ec_sensor_info *si; enum hwmon_sensor_types type; - unsigned long board_sensors; struct device *hwdev; unsigned int i; - board_sensors = get_board_sensors(); - if (!board_sensors) + pboard_info = get_board_info(); + if (!pboard_info) return -ENODEV; ec_data = devm_kzalloc(dev, sizeof(struct ec_sensors_data), @@ -629,9 +659,8 @@ static int __init asus_ec_probe(struct platform_device *pdev) return -ENOMEM; dev_set_drvdata(dev, ec_data); - ec_data->board_sensors = board_sensors; - ec_data->nr_sensors = board_sensors_count(ec_data->board_sensors); - ec_data->sensors = devm_kcalloc(dev, ec_data->nr_sensors, + ec_data->board_info = *pboard_info; + ec_data->sensors = devm_kcalloc(dev, sensor_count(&ec_data->board_info), sizeof(struct ec_sensor), GFP_KERNEL); setup_sensor_data(ec_data); @@ -647,7 +676,7 @@ static int __init asus_ec_probe(struct platform_device *pdev) ec_data->aml_mutex = asus_hw_access_mutex(dev); - for (i = 0; i < ec_data->nr_sensors; ++i) { + for (i = 0; i < sensor_count(&ec_data->board_info); ++i) { si = get_sensor_info(ec_data, i); if (!nr_count[si->type]) ++nr_types; @@ -681,7 +710,7 @@ static int __init asus_ec_probe(struct platform_device *pdev) } dev_info(dev, "board has %d EC sensors that span %d registers", - ec_data->nr_sensors, ec_data->nr_registers); + sensor_count(&ec_data->board_info), ec_data->nr_registers); hwdev = devm_hwmon_device_register_with_info(dev, "asusec", ec_data, chip_info, NULL); @@ -703,8 +732,8 @@ static struct platform_driver asus_ec_sensors_platform_driver = { }, }; -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); module_param_named(mutex_path, mutex_path_override, charp, 0); MODULE_PARM_DESC(mutex_path,
We need to keep some more information about the current board than just the sensors set, and with more boards to add the dmi id array grows quickly. Our probe code is always the same so let's switch to a custom test code and a custom board info array. That allows us to omit board vendor string (ASUS uses two strings that differ in case) in the board info and use case-insensitive comparison, and also do not duplicate sensor definitions for such board variants as " (WI-FI)" when sensors are identical to the base variant. Also saves a quarter of the module size by replacing big dmi_system_id structs with smaller ones. Signed-off-by: Eugene Shalygin <eugene.shalygin@gmail.com> --- drivers/hwmon/asus-ec-sensors.c | 209 ++++++++++++++++++-------------- 1 file changed, 119 insertions(+), 90 deletions(-)