Hi,
On 15-09-2019 21:55, Yauhen Kharuzhy wrote:
Hi,
Sorry for last answer, I didn't notice your answers. I have returned to
this project now for some time.
On Mon, Aug 12, 2019 at 07:11:07PM +0200, Hans de Goede wrote:
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.
Sounds reasonable.
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.
Yes, but this header file will contain relative big piece of code which
will be included to modules and compiled twice.
I think that I will use this approach and will move to separate module
only when other common functions will be added.
thinking more about this, since both modules will have the same
acpi_device_id table they will both get auto-loaded anyways.
So how about the following:
1) We put a single struct platform_driver in the common bits
2) We create a new shared struct cht_int33fe_data in intel_cht_int33fe_common.h
which has all the data members needed by both variants, e.g. just put the
intel_cht_int33fe_typec.c struct cht_int33fe_data definition in the common
header and rename max17047 to the more generic battery_fg used by the musb code.
And add an enum int33fe_hw_type hw_type field to this struct
3) Have the typec and musb .c files export their probe + remove function
(make them non-static add prototypes to common.h)
4) Add a new probe to the common.c file which does the usual checks and
figures out hw_type. It then devm_kzalloc-s the struct, sets hw_type in the
struct and calls the right actual probe function depending on the board.
5) Add a new remove function to the common.c file which calls the right remove
function based on the hw_type in the data struct.
And we build all 3 .c files into a single module (since if we have multiple
modules they will all get loaded anyways). I think this is the cleanest / best
approach and it also answers the question of what to name the Kconfig options,
since if we do as I suggest above we just stick with the single Kconfig option
we already have.
Regards,
Hans
