Hi,
On 09-08-19 16:51, Yauhen Kharuzhy wrote:
On Fri, Aug 09, 2019 at 12:49:27PM +0200, Hans de Goede wrote:
Hi,
Overall this looks good, thank you for your work on this.
I have some small remarks inline / below:
+EXPORT_SYMBOL_GPL(cht_int33fe_check_hw_compatible);
+
+MODULE_DESCRIPTION("Intel Cherry Trail ACPI INT33FE pseudo device driver (common part)");
+MODULE_AUTHOR("Yauhen Kharuzhy <jekhor@xxxxxxxxx>");
+MODULE_LICENSE("GPL v2");
I see from the Makefile changes that you are linking the common code
into both intel_cht_int33fe_typec.ko and into intel_cht_int33fe_musb.ko, that is fine
since it is tiny and not worth the trouble of creating its own .ko file for.
No, this Makefile fragment adds two targets for every config variables,
and intel_cht_int33fe_common.c compiles into one .ko file even if it was
added twice
Ah right, I misread it. But adding a new ko file just for the one helper function
seems like massive overkill, the overhead will be quite big and on most systems
all 3 .ko files will end up getting loaded anyways, so we should probably try to
reduce the number of ko files here.
I do wonder what happens if you set the Kconfig value for both modules to Y,
since that will like put the common code twice in the builtin.a file, I guess / hope
ar is smart enough to only add it once, but I'm not sure... can you please give
this a try?
For both Y it should be OK, but for one M and one Y... OK, it need to be
corrected.
How about moving the entire helper function into intel_cht_int33fe_common.h as
static inline ... then both remaining .ko files get a private copy but since it is
small that is fine. This nicely solves the need for a third .ko file and also
problems with one of the Kconfig options being builtin and the other being modular.
Regards,
Hans
