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
