[...] Please, be consistent with naming convention. All the above should have same prefix as others routines.
> > > +static ssize_t post_reset_wdog_store(struct device_driver *drv, > > + const char *buf, size_t count) { > > + int err; > > + unsigned long watchdog; > > + > > + err = kstrtoul(buf, 10, &watchdog); > > + if (err) > > + return err; > > + > > > + if (mlxbf_bootctl_smc_call1(MLXBF_BOOTCTL_SET_POST_RESET_WDOG, > > + watchdog) < 0) > > + return -EINVAL; > > If that call returns an error it shouldn't be shadowed here. > > > + > > + return count; > > +} > > + > > +static ssize_t reset_action_show(struct device_driver *drv, char > > +*buf) { > > > + return sprintf(buf, "%s\n", reset_action_to_string( > > + > > + mlxbf_bootctl_smc_call0(MLXBF_BOOTCTL_GET_RESET_ACTION))); > > Wouldn't be easy to parse this as > > int action = ...call0(); > return sprintf(...); > > ? > > (int is an arbitrary type here, choose one that suits) > > > +} > > + > > +static ssize_t reset_action_store(struct device_driver *drv, > > + const char *buf, size_t count) { > > + int action = reset_action_to_val(buf, count); > > + > > > + if (action < 0 || action == MLXBF_BOOTCTL_NONE) > > + return -EINVAL; > > Don't shadow an error. > > > + > > + if (mlxbf_bootctl_smc_call1(MLXBF_BOOTCTL_SET_RESET_ACTION, > action) < 0) > > + return -EINVAL; > > Same. > > > + > > + return count; > > +} > > + > > +static ssize_t second_reset_action_show(struct device_driver *drv, > > +char *buf) { > > > + return sprintf(buf, "%s\n", reset_action_to_string( > > + mlxbf_bootctl_smc_call0( > > + MLXBF_BOOTCTL_GET_SECOND_RESET_ACTION))); > > Use temp variable. > > > +} > > + > > +static ssize_t second_reset_action_store(struct device_driver *drv, > > + const char *buf, size_t > > +count) { > > + int action = reset_action_to_val(buf, count); > > + > > + if (action < 0) > > + return -EINVAL; > > Don't shadow an error. > > > + > > + if > (mlxbf_bootctl_smc_call1(MLXBF_BOOTCTL_SET_SECOND_RESET_ACTION, > > + action) < 0) > > + return -EINVAL; > > Same. > > > + > > + return count; > > +} > > + > > +static ssize_t lifecycle_state_show(struct device_driver *drv, char > > +*buf) { > > > + int lc_state = mlxbf_bootctl_smc_call1( > > + MLXBF_BOOTCTL_GET_TBB_FUSE_STATUS, > > + MLXBF_BOOTCTL_FUSE_STATUS_LIFECYCLE); > > Split it as > > int ...; > > ... = call1(); > if (...) > > > + > > + if (lc_state < 0) > > + return -EINVAL; > > Don't shadow an error. > > > + > > + lc_state &= (MLXBF_BOOTCTL_SB_MODE_TEST_MASK | > > + MLXBF_BOOTCTL_SB_MODE_SECURE_MASK); > > Better to split like > > xxx = > (A | B); > > > + /* > > + * If the test bits are set, we specify that the current state may > > be > > + * due to using the test bits. > > + */ > > > + if ((lc_state & MLXBF_BOOTCTL_SB_MODE_TEST_MASK) != 0) { > > ' != 0' is redundant. > > > + > > + lc_state &= MLXBF_BOOTCTL_SB_MODE_SECURE_MASK; > > + > > > + return sprintf(buf, "%s(test)\n", > > + > > + mlxbf_bootctl_lifecycle_states[lc_state]); > > One line? > > > + } > > + > > + return sprintf(buf, "%s\n", > > +mlxbf_bootctl_lifecycle_states[lc_state]); > > +} > > + > > +static ssize_t secure_boot_fuse_state_show(struct device_driver *drv, > > +char *buf) { > > + int key; > > + int buf_len = 0; > > + int upper_key_used = 0; > > + int sb_key_state = mlxbf_bootctl_smc_call1( > > + MLXBF_BOOTCTL_GET_TBB_FUSE_STATUS, > > + MLXBF_BOOTCTL_FUSE_STATUS_KEYS); > > + > > + if (sb_key_state < 0) > > + return -EINVAL; > > + > > > + for (key = MLXBF_SB_KEY_NUM - 1; key >= 0; key--) { > > I'm not sure it's a good idea to put several lines in one sysfs attribute. > > > + int burnt = ((sb_key_state & (1 << key)) != 0); > > Redundant ' != 0', redundant parens. > > > + int valid = ((sb_key_state & > > + (1 << (key + MLXBF_SB_KEY_NUM))) != 0); > > Same. > > > + > > + buf_len += sprintf(buf + buf_len, "Ver%d:", key); > > + if (upper_key_used) { > > + if (burnt) { > > + if (valid) > > + buf_len += sprintf(buf + buf_len, > > + "Used"); > > Oh, why not just > > const char *status; > > if (...) { > ... > status = "1"; > ... > status = "2"; > ... > } > len = snprintf(buf + len, ..., status); > > ? > > > + else > > + buf_len += sprintf(buf + buf_len, > > + "Wasted"); > > + } else { > > + if (valid) > > + buf_len += sprintf(buf + buf_len, > > + "Invalid"); > > + else > > + buf_len += sprintf(buf + buf_len, > > + "Skipped"); > > + } > > + } else { > > + if (burnt) { > > + if (valid) { > > + upper_key_used = 1; > > + buf_len += sprintf(buf + buf_len, > > + "In use"); > > + } else > > + buf_len += sprintf(buf + buf_len, > > + "Burn > > incomplete"); > > + } else { > > + if (valid) > > + buf_len += sprintf(buf + buf_len, > > + "Invalid"); > > + else > > + buf_len += sprintf(buf + buf_len, > > + "Free"); > > + } > > + } > > + buf_len += sprintf(buf + buf_len, "\n"); > > + } > > + > > + return buf_len; > > +} > > + > > > +#define MLXBF_BOOTCTL_DRV_ATTR(_name) DRIVER_ATTR_RW(_name) > > What the point? > > > +static struct attribute_group mlxbf_bootctl_attr_group = { > > + .attrs = mlxbf_bootctl_dev_attrs > > + comma. > [...]