Hi Hans, first of all, thanks for the review. More below. On Tue, Oct 19, 2021 at 11:41:30AM +0200, Hans de Goede wrote: > Hi Len, > > On 10/3/21 11:19, Len Baker wrote: > > Platform drivers have the option of having the platform core create and > > remove any needed sysfs attribute files. So take advantage of that and > > refactor the attributes management to avoid to register them "by hand". > > > > Also, due to some attributes are optionals, refactor the code and move > > the logic inside the "is_visible" callbacks of the attribute_group > > structures. > > > > Suggested-by: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx> > > Signed-off-by: Len Baker <len.baker@xxxxxxx> > > Thank you for the patch, this indeed results in a nice improvement. > > Unfortunately I cannot take this as is (because it will trigger > a BUG_ON). See my inline remarks, if you can do a v2 with those > fixed that would be great. Ok, no problem. > > [...] > > > > +static umode_t fan_attr_is_visible(struct kobject *kobj, struct attribute *attr, > > + int n) > > +{ > > + if (fan_status_access_mode != TPACPI_FAN_NONE || > > + fan_control_access_mode != TPACPI_FAN_WR_NONE) { > > + if (attr == &dev_attr_fan2_input.attr) { > > + if (!tp_features.second_fan) > > + return 0; > > + } > > + > > + return attr->mode; > > + } > > > Can you refactor this one to not have nested if-s and put the > "return attr->mode;" at the end like the other is_visible > functions please ? Ok, I will work on it for the next version. > > [...] > > > > -static struct ibm_struct proxsensor_driver_data = { > > - .name = "proximity-sensor", > > - .exit = proxsensor_exit, > > -}; > > - > > So when I came here during the review I decided a v2 was necessary. > > The way the sub-drivers inside thinkpad_acpi work is they must have a > struct ibm_struct associated with them, even if it is just for the name > field. > > This is enforced rather harshly (something to fix in another patch) > by this bit of code: > > ``` > static int __init ibm_init(struct ibm_init_struct *iibm) > { > int ret; > struct ibm_struct *ibm = iibm->data; > struct proc_dir_entry *entry; > > BUG_ON(ibm == NULL); > ``` > > The name is used in various places and the struct is also used to > store various house-keeping flags. > > So for v2 please keep the proxsensor_driver_data struct here, as well > as for dprc_driver_data. Ok, I will fix it for the v2 version. > > [...] > > > > -static int tpacpi_kbdlang_init(struct ibm_init_struct *iibm) > > +static umode_t kbdlang_attr_is_visible(struct kobject *kobj, > > + struct attribute *attr, int n) > > { > > int err, output; > > > > err = get_keyboard_lang(&output); > > - /* > > - * If support isn't available (ENODEV) then don't return an error > > - * just don't create the sysfs group. > > - */ > > - if (err == -ENODEV) > > - return 0; > > - > > - if (err) > > - return err; > > - > > - /* Platform supports this feature - create the sysfs file */ > > - return sysfs_create_group(&tpacpi_pdev->dev.kobj, &kbdlang_attr_group); > > + return err ? 0 : attr->mode; > > } > > get_keyboard_lang() consists of 2 not cheap ACPI calls, one of > which involves talking to the embedded-controller over some slow bus. > > Please keep kbdlang_init() and make it set a flag to use inside > kbdlang_attr_is_visible(). Understood, thanks. > > [...] > > > > -static struct ibm_struct dprc_driver_data = { > > - .name = "dprc", > > - .exit = dprc_exit, > > As mentioned above this struct needs to be kept around, > with just the name set. No problem. Again, thanks for the review, Len