Re: [PATCH] platform/x86: thinkpad_acpi: Convert platform driver to use dev_groups

[Len Baker]

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