Messages in this thread | | | Date | Wed, 28 Sep 2022 17:16:19 +0300 | From | Andy Shevchenko <> | Subject | Re: [PATCH v2 2/2] platform/x86: dell: Add new dell-wmi-ddv driver |
| |
On Wed, Sep 28, 2022 at 01:33:53PM +0200, Hans de Goede wrote: > On 9/28/22 12:47, Andy Shevchenko wrote: > > On Tue, Sep 27, 2022 at 10:45:21PM +0200, Armin Wolf wrote:
...
> >> + default m > > > > Why? (Imagine I have Dell, but old machine) > > Then you can select N if you really want to. > > > (And yes, I see that other Kconfig options are using it, but we shall avoid > > cargo cult and each default choice like this has to be explained at least.) > > This has been discussed during the review of v1 already. > > There are quite a few dell modules and the choice has > been made to put these all behind a dell platform drivers > options and then default all the individual modules to 'm'.
Okay, thanks for pointing out that this was discussed. I was not aware.
...
> >> + kfree(obj); > > > > I'm wondering what is the best to use in the drivers: > > 1) kfree() > > 2) acpi_os_free() > > 3) ACPI_FREE()
> > ? > > Most ACPI driver code I know of just uses kfree() the other 2 > are more ACPI-core / ACPICA internal helpers.
To me 2) would look more consistent, esp. in case if it is extended in the future.
...
> >> + ret = device_create_file(&battery->dev, &data->temp_attr); > >> + if (ret < 0) > >> + return ret; > >> + > >> + ret = device_create_file(&battery->dev, &data->eppid_attr); > >> + if (ret < 0) { > >> + device_remove_file(&battery->dev, &data->temp_attr); > >> + > >> + return ret; > >> + } > > > > Why dev_groups member can't be utilized? > > Because this is an extension to the ACPI battery driver, IOW > this adds extra attributes to the power-supply-class device > registered by the ACPI battery driver. Note that the device > in this case is managed by the power-supply-class code, so > there is no access to dev_groups even in the ACPI battery code.
Ah, I see now, so we extend the attributes of the 3rd party driver here.
-- With Best Regards, Andy Shevchenko
| |