Hi
2023. március 11., szombat 15:40 keltezéssel, Nikita Kravets <teackot@xxxxxxxxx> írta:
> [...]
> +static ssize_t
> +charge_control_start_threshold_show(struct device *device,
> + struct device_attribute *attr, char *buf)
> +{
> + return charge_control_threshold_show(conf.charge_control.offset_start,
> + device, attr, buf);
> +}
> +
> +static ssize_t
> +charge_control_start_threshold_store(struct device *dev,
> + struct device_attribute *attr,
> + const char *buf, size_t count)
> +{
> + return charge_control_threshold_store(conf.charge_control.offset_start,
> + dev, attr, buf, count);
> +}
The above two functions are inconsistent with the rest of the file because
they have the return type in a separate line.
> +
> +static ssize_t charge_control_end_threshold_show(struct device *device,
> + struct device_attribute *attr,
> + char *buf)
> +{
> + return charge_control_threshold_show(conf.charge_control.offset_end,
> + device, attr, buf);
> +}
> +
> +static ssize_t charge_control_end_threshold_store(struct device *dev,
> + struct device_attribute *attr,
> + const char *buf, size_t count)
> +{
> + return charge_control_threshold_store(conf.charge_control.offset_end,
> + dev, attr, buf, count);
> +}
> +
> +static DEVICE_ATTR_RW(charge_control_start_threshold);
> +static DEVICE_ATTR_RW(charge_control_end_threshold);
> +
> +static struct attribute *msi_battery_attrs[] = {
> + &dev_attr_charge_control_start_threshold.attr,
> + &dev_attr_charge_control_end_threshold.attr,
> + NULL
> +};
> +
> +ATTRIBUTE_GROUPS(msi_battery);
> +
> +static int msi_battery_add(struct power_supply *battery,
> + struct acpi_battery_hook *hook)
> +{
> + return device_add_groups(&battery->dev, msi_battery_groups);
> +}
> +
> +static int msi_battery_remove(struct power_supply *battery,
> + struct acpi_battery_hook *hook)
> +{
> + device_remove_groups(&battery->dev, msi_battery_groups);
> + return 0;
> +}
> +
> +static struct acpi_battery_hook battery_hook = {
> + .add_battery = msi_battery_add,
> + .remove_battery = msi_battery_remove,
> + .name = MSI_EC_DRIVER_NAME,
> +};
> +
> +// ============================================================ //
> +// Module load/unload
> +// ============================================================ //
It's a small thing, but usually /* ... */ comments are preferred.
Hans will tell you if you need to change it.
> +
> +static int __init load_configuration(void)
> +{
> + int result;
> +
> + u8 fw_version[MSI_EC_FW_VERSION_LENGTH + 1];
> +
> + // get firmware version
> + result = ec_get_firmware_version(fw_version);
> + if (result < 0)
> + return result;
> +
> + // load the suitable configuration, if exists
> + for (int i = 0; CONFIGURATIONS[i]; i++) {
> + for (int j = 0; CONFIGURATIONS[i]->allowed_fw[j]; j++) {
> + if (strcmp(CONFIGURATIONS[i]->allowed_fw[j], fw_version) == 0) {
Previously you said `match_string()` works here. Has something changed?
> + memcpy(&conf, CONFIGURATIONS[i], sizeof(struct msi_ec_conf));
Why not simply
conf = *CONFIGURATIONS[i];
?
> + conf.allowed_fw = NULL;
> + return 0;
> + }
> + }
> + }
> +
> + pr_err("Your firmware version is not supported!\n");
> + return -EOPNOTSUPP;
> +}
> [...]
Regards,
Barnabás Pőcze
