| Message ID | 20250513100514.2492545-1-andriy.shevchenko@linux.intel.com |
|---|---|
| Headers | show |
| Series | gpiolib: acpi: Split quirks to its own file | expand |
On Wed, May 14, 2025 at 6:00 PM Mika Westerberg <mika.westerberg@linux.intel.com> wrote: > > drivers/gpio/Makefile | 1 + > > .../{gpiolib-acpi.c => gpiolib-acpi-core.c} | 344 +---------------- > > drivers/gpio/gpiolib-acpi-quirks.c | 363 ++++++++++++++++++ > > drivers/gpio/gpiolib-acpi.h | 15 + > > All this -foo-core things look redundant to me. Why not just split it out > and call it gpiolib-quirks.c and put there all the quirks not just ACPI? I > Don't think we want to have gpiolib-of-quirks.c and gpiolog-swnode-quirks.c > and so on. For OF/device tree the quirks are in gpiolib-of.c and we probably do not want to put these into a shared file with ACPI (and swnode?) quirks as systems with OF compile objects (Makefile entries) and ACPI compile objects are not always included in the same build, so having them per-hw-config-principle cuts down compiletime overhead. Also it's pretty clear separation of concerns I think. Yours, Linus Walleij
On Wed, May 14, 2025 at 06:59:55PM +0300, Mika Westerberg wrote: > On Tue, May 13, 2025 at 01:00:30PM +0300, Andy Shevchenko wrote: > > The GPIO ACPI helpers use a few quirks which consumes approximately 20% > > of the file. Besides that the necessary bits are sparse and being directly > > referred. Split them to a separate file. There is no functional change. > > > > For the new file I used the Hans' authorship of Hans as he the author of > > all those bits (expect very tiny changes made by this series). > > > > Hans, please check if it's okay and confirm, or suggest better alternative. > > > > Andy Shevchenko (4): > > gpiolib: acpi: Switch to use enum in acpi_gpio_in_ignore_list() > > gpiolib: acpi: Handle deferred list via new API > > gpiolib: acpi: Add acpi_gpio_need_run_edge_events_on_boot() getter > > gpiolib: acpi: Move quirks to a separate file > > > > drivers/gpio/Makefile | 1 + > > .../{gpiolib-acpi.c => gpiolib-acpi-core.c} | 344 +---------------- > > drivers/gpio/gpiolib-acpi-quirks.c | 363 ++++++++++++++++++ > > drivers/gpio/gpiolib-acpi.h | 15 + > > All this -foo-core things look redundant to me. Why not just split it out > and call it gpiolib-quirks.c and put there all the quirks not just ACPI? I > Don't think we want to have gpiolib-of-quirks.c and gpiolog-swnode-quirks.c > and so on. That's might be the next step to have for all of them, but these are ACPI specific. In any case they can't be put to gpiolib-quirks.c due to module parameters. If we do that we will need a dirty hack to support old module parameters (see 8250 how it's done there, and even author of that didn't like the approach).
On Thu, May 15, 2025 at 11:04:22AM +0300, Andy Shevchenko wrote: > On Wed, May 14, 2025 at 06:59:55PM +0300, Mika Westerberg wrote: > > On Tue, May 13, 2025 at 01:00:30PM +0300, Andy Shevchenko wrote: > > > The GPIO ACPI helpers use a few quirks which consumes approximately 20% > > > of the file. Besides that the necessary bits are sparse and being directly > > > referred. Split them to a separate file. There is no functional change. > > > > > > For the new file I used the Hans' authorship of Hans as he the author of > > > all those bits (expect very tiny changes made by this series). > > > > > > Hans, please check if it's okay and confirm, or suggest better alternative. > > > > > > Andy Shevchenko (4): > > > gpiolib: acpi: Switch to use enum in acpi_gpio_in_ignore_list() > > > gpiolib: acpi: Handle deferred list via new API > > > gpiolib: acpi: Add acpi_gpio_need_run_edge_events_on_boot() getter > > > gpiolib: acpi: Move quirks to a separate file > > > > > > drivers/gpio/Makefile | 1 + > > > .../{gpiolib-acpi.c => gpiolib-acpi-core.c} | 344 +---------------- > > > drivers/gpio/gpiolib-acpi-quirks.c | 363 ++++++++++++++++++ > > > drivers/gpio/gpiolib-acpi.h | 15 + > > > > All this -foo-core things look redundant to me. Why not just split it out > > and call it gpiolib-quirks.c and put there all the quirks not just ACPI? I > > Don't think we want to have gpiolib-of-quirks.c and gpiolog-swnode-quirks.c > > and so on. > > That's might be the next step to have for all of them, but these are ACPI > specific. In any case they can't be put to gpiolib-quirks.c due to module > parameters. If we do that we will need a dirty hack to support old module > parameters (see 8250 how it's done there, and even author of that didn't like > the approach). Hmm, how does it affect module paremeters? I thought they are gpiolib.something as all these object files are linked to it? At least can we drop the gpiolib-acpi-core.c rename?
On Thu, May 15, 2025 at 11:41:59AM +0300, Andy Shevchenko wrote: > On Thu, May 15, 2025 at 11:34:51AM +0300, Mika Westerberg wrote: > > On Thu, May 15, 2025 at 11:04:22AM +0300, Andy Shevchenko wrote: > > > On Wed, May 14, 2025 at 06:59:55PM +0300, Mika Westerberg wrote: > > > > On Tue, May 13, 2025 at 01:00:30PM +0300, Andy Shevchenko wrote: > > > > > The GPIO ACPI helpers use a few quirks which consumes approximately 20% > > > > > of the file. Besides that the necessary bits are sparse and being directly > > > > > referred. Split them to a separate file. There is no functional change. > > > > > > > > > > For the new file I used the Hans' authorship of Hans as he the author of > > > > > all those bits (expect very tiny changes made by this series). > > > > > > > > > > Hans, please check if it's okay and confirm, or suggest better alternative. > > > > > > > > > > Andy Shevchenko (4): > > > > > gpiolib: acpi: Switch to use enum in acpi_gpio_in_ignore_list() > > > > > gpiolib: acpi: Handle deferred list via new API > > > > > gpiolib: acpi: Add acpi_gpio_need_run_edge_events_on_boot() getter > > > > > gpiolib: acpi: Move quirks to a separate file > > > > > > > > > > drivers/gpio/Makefile | 1 + > > > > > .../{gpiolib-acpi.c => gpiolib-acpi-core.c} | 344 +---------------- > > > > > drivers/gpio/gpiolib-acpi-quirks.c | 363 ++++++++++++++++++ > > > > > drivers/gpio/gpiolib-acpi.h | 15 + > > > > > > > > All this -foo-core things look redundant to me. Why not just split it out > > > > and call it gpiolib-quirks.c and put there all the quirks not just ACPI? I > > > > Don't think we want to have gpiolib-of-quirks.c and gpiolog-swnode-quirks.c > > > > and so on. > > > > > > That's might be the next step to have for all of them, but these are ACPI > > > specific. In any case they can't be put to gpiolib-quirks.c due to module > > > parameters. If we do that we will need a dirty hack to support old module > > > parameters (see 8250 how it's done there, and even author of that didn't like > > > the approach). > > > > Hmm, how does it affect module paremeters? I thought they are > > gpiolib.something as all these object files are linked to it? > > gpiolib_acpi.FOO because the object file is gpiolib-acpi.o. Ah okay. > > At least can we drop the gpiolib-acpi-core.c rename? > > Unfortunately no due to the above. This does not work? gpiolib-acpi-y := gpiolib-acpi.o gpiolib-acpi-quirks.o
On Thu, May 15, 2025 at 11:58:33AM +0300, Andy Shevchenko wrote: > On Thu, May 15, 2025 at 11:47:27AM +0300, Mika Westerberg wrote: > > On Thu, May 15, 2025 at 11:41:59AM +0300, Andy Shevchenko wrote: > > > On Thu, May 15, 2025 at 11:34:51AM +0300, Mika Westerberg wrote: > > > > On Thu, May 15, 2025 at 11:04:22AM +0300, Andy Shevchenko wrote: > > > > > On Wed, May 14, 2025 at 06:59:55PM +0300, Mika Westerberg wrote: > > ... > > > > > > That's might be the next step to have for all of them, but these are ACPI > > > > > specific. In any case they can't be put to gpiolib-quirks.c due to module > > > > > parameters. If we do that we will need a dirty hack to support old module > > > > > parameters (see 8250 how it's done there, and even author of that didn't like > > > > > the approach). > > > > > > > > Hmm, how does it affect module paremeters? I thought they are > > > > gpiolib.something as all these object files are linked to it? > > > > > > gpiolib_acpi.FOO because the object file is gpiolib-acpi.o. > > > > Ah okay. > > > > > > At least can we drop the gpiolib-acpi-core.c rename? > > > > > > Unfortunately no due to the above. > > > > This does not work? > > > > gpiolib-acpi-y := gpiolib-acpi.o gpiolib-acpi-quirks.o > > No. You can't use the same name on left and right parts. I see :( Okay then I guess there are no other options than name it like this. [ Ideally we would drop the while gpiolib- prefix from all these so you have acpi.c, sysfs.c and so on without the redundancy but that's outside of scope of this work anyways ;-) ] Acked-by: Mika Westerberg <mika.westerberg@linux.intel.com>
On Thu, May 15, 2025 at 12:31:24PM +0300, Mika Westerberg wrote: > On Thu, May 15, 2025 at 11:58:33AM +0300, Andy Shevchenko wrote: > > On Thu, May 15, 2025 at 11:47:27AM +0300, Mika Westerberg wrote: > > > On Thu, May 15, 2025 at 11:41:59AM +0300, Andy Shevchenko wrote: > > > > On Thu, May 15, 2025 at 11:34:51AM +0300, Mika Westerberg wrote: > > > > > On Thu, May 15, 2025 at 11:04:22AM +0300, Andy Shevchenko wrote: > > > > > > On Wed, May 14, 2025 at 06:59:55PM +0300, Mika Westerberg wrote: > > > > ... > > > > > > > > That's might be the next step to have for all of them, but these are ACPI > > > > > > specific. In any case they can't be put to gpiolib-quirks.c due to module > > > > > > parameters. If we do that we will need a dirty hack to support old module > > > > > > parameters (see 8250 how it's done there, and even author of that didn't like > > > > > > the approach). > > > > > > > > > > Hmm, how does it affect module paremeters? I thought they are > > > > > gpiolib.something as all these object files are linked to it? > > > > > > > > gpiolib_acpi.FOO because the object file is gpiolib-acpi.o. > > > > > > Ah okay. > > > > > > > > At least can we drop the gpiolib-acpi-core.c rename? > > > > > > > > Unfortunately no due to the above. > > > > > > This does not work? > > > > > > gpiolib-acpi-y := gpiolib-acpi.o gpiolib-acpi-quirks.o > > > > No. You can't use the same name on left and right parts. > > I see :( Okay then I guess there are no other options than name it like > this. > > [ Ideally we would drop the while gpiolib- prefix from all these so you have > acpi.c, sysfs.c and so on without the redundancy but that's outside of > scope of this work anyways ;-) ] I also would like to have prefix to be dropped for the files, but the modules (and it's can be achieved, but it's indeed out of scope here). > Acked-by: Mika Westerberg <mika.westerberg@linux.intel.com> Thank you! I am going to push to my review and testing queue soon.
The GPIO ACPI helpers use a few quirks which consumes approximately 20% of the file. Besides that the necessary bits are sparse and being directly referred. Split them to a separate file. There is no functional change. For the new file I used the Hans' authorship of Hans as he the author of all those bits (expect very tiny changes made by this series). Hans, please check if it's okay and confirm, or suggest better alternative. Andy Shevchenko (4): gpiolib: acpi: Switch to use enum in acpi_gpio_in_ignore_list() gpiolib: acpi: Handle deferred list via new API gpiolib: acpi: Add acpi_gpio_need_run_edge_events_on_boot() getter gpiolib: acpi: Move quirks to a separate file drivers/gpio/Makefile | 1 + .../{gpiolib-acpi.c => gpiolib-acpi-core.c} | 344 +---------------- drivers/gpio/gpiolib-acpi-quirks.c | 363 ++++++++++++++++++ drivers/gpio/gpiolib-acpi.h | 15 + 4 files changed, 392 insertions(+), 331 deletions(-) rename drivers/gpio/{gpiolib-acpi.c => gpiolib-acpi-core.c} (79%) create mode 100644 drivers/gpio/gpiolib-acpi-quirks.c