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.
--
Yauhen Kharuzhy
