Re: [Intel-gfx] [PATCH 6/8] drivers/pwm: Add Crystalcove (CRC) PWM driver
From: Shobhit Kumar
Date: Mon Jun 22 2015 - 03:55:27 EST
On Sat, Jun 20, 2015 at 11:34 PM, Paul Gortmaker
> [Re: [Intel-gfx] [PATCH 6/8] drivers/pwm: Add Crystalcove (CRC) PWM driver] On 20/06/2015 (Sat 13:23) Paul Bolle wrote:
>> [Added Paul Gortmaker.]
>> Hi Shobhit,
>> On Fri, 2015-06-19 at 12:16 +0530, Shobhit Kumar wrote:
>> > So what is the exact big problem with this ?
>> The main problem I have is that it's hard to read a submitter's mind.
>> And, I think, in cases like this we need to know if the submitter just
>> made some silly mistake or that the mismatch (between Kconfig type and
>> code) was intentional. So each time such a mismatch is spotted the
>> submitter ought to be asked about it.
>> (I'd guess that one or two new drivers are submitted _each_ day. And
>> these mismatches are quite common. I'd say I receive answers like:
>> - "Oops, yes bool should have been tristate"; or
>> - "Oops, forgot to clean up after noticing tristate didn't work"; or
>> - "I just copy-and-pasted a similar driver, the module stuff isn't
>> actually needed"
>> at least once a week. Not sure, I don't keep track of this stuff.)
>> Furthermore, it appears that Paul Gortmaker is on a mission to, badly
>> summarized, untangle the module and init code. See for instance
>> https://lkml.org/lkml/2015/5/28/809 and
>> https://lkml.org/lkml/2015/5/31/205 .
>> Now, I don't know whether (other) Paul is bothered by these MODULE_*
>> macros. But Paul did submit a series that adds
> Yes, I agree that it would be nice to not see these mismatches,
> regardless of whether we can get away with it or not.
>> builtin_platform_driver(), see https://lkml.org/lkml/2015/5/10/131 .
>> That new macro ensures built-in only code doesn't have to use
>> module_platform_driver(), which your patch also uses. So perhaps Paul
>> can explain some of the non-obvious issues caused by built-in only code
>> using module specific constructs.
> In https://lkml.org/lkml/2015/5/10/125 I'd written:
> There are several downsides to this:
> 1) The code can appear modular to a reader of the code, and they
> won't know if the code really is modular without checking the
> Makefile and Kconfig to see if compilation is governed by a
> bool or tristate.
> 2) Coders of drivers may be tempted to code up an __exit function
> that is never used, just in order to satisfy the required three
> args of the modular registration function.
> 3) Non-modular code ends up including the <module.h> which increases
> CPP overhead that they don't need.
> 4) It hinders us from performing better separation of the module
> init code and the generic init code.
Okay. Get the idea and the need in terms of clear separation. Its just
that there are quite a few built-in drivers using module
initialization that I assumed its okay.
> The nature of linux means that thousands of developers are reading the
> code every day -- so I think that there is a genuine value in having the
> code convey a clear message on how it was designed to be used. Only
> using module related headers/macros for genuinely modular code helps us
> (albeit in a small way) towards achieving that.
> Looking at this thread, I see that one of the reasons given for this
> code's ambiguous module vs. built-in identity was the observation of a
> similar identity crisis of the related INTEL_SOC_PMIC code. Does that
> not back up the point above about the value in having the code speak for
> itself? So IMHO we probably should clarify the PMIC code vs. adding
> another example that looks just like it.
Okay agree. I think there are quite of them lurking in the sources
which would need correction. For this PWM driver I will take care as
>> > I can anyway shove out these macros to end the discussion.
>> I'd rather convince you than annoy you into doing as I suggested.
>> > BTW whether you buy the argument or not, please do treat yourself
>> > with ice cream for being able to make such a comment.
>> Will do.
>> Paul Bolle
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
Please read the FAQ at http://www.tux.org/lkml/