Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1168568 > unrolled thread

Re: [Intel-gfx] [PATCH 6/8] drivers/pwm: Add Crystalcove (CRC) PWM driver

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [Intel-gfx] [PATCH 6/8] drivers/pwm: Add Crystalcove (CRC) PWM driver Shobhit Kumar <kumar@shobhit.info> - 2015-06-19 08:50 +0200
    Re: [Intel-gfx] [PATCH 6/8] drivers/pwm: Add Crystalcove (CRC) PWM  driver Paul Gortmaker <paul.gortmaker@windriver.com> - 2015-06-20 20:10 +0200

#1168568 — Re: [Intel-gfx] [PATCH 6/8] drivers/pwm: Add Crystalcove (CRC) PWM driver

Hi Paul,

On Fri, Jun 19, 2015 at 12:11 AM, Paul Bolle <pebolle@tiscali.nl> wrote:
> Hi Shobhit,
>
> On Thu, 2015-06-18 at 23:24 +0530, Shobhit Kumar wrote:
>> On Fri, May 1, 2015 at 2:42 AM, Paul Bolle <pebolle@tiscali.nl> wrote:
>> > On Wed, 2015-04-29 at 19:30 +0530, Shobhit Kumar wrote:
>> >> --- a/drivers/pwm/Kconfig
>> >> +++ b/drivers/pwm/Kconfig
>> >
>> >> +config PWM_CRC
>> >> +     bool "Intel Crystalcove (CRC) PWM support"
>> >> +     depends on X86 && INTEL_SOC_PMIC
>> >> +     help
>> >> +       Generic PWM framework driver for Crystalcove (CRC) PMIC based PWM
>> >> +       control.
>> >
>> >> --- a/drivers/pwm/Makefile
>> >> +++ b/drivers/pwm/Makefile
>> >
>> >> +obj-$(CONFIG_PWM_CRC)                += pwm-crc.o
>> >
>> > PWM_CRC is a bool symbol. So pwm-crc.o can never be part of a module.
>>
>> I actually started this as a module but later decided to make it as
>> bool because INTEL_SOC_PMIC on which this depends is itself a bool as
>> well.
>
> As does GPIO_CRYSTAL_COVE and that's a tristate. So?
>
>> Still it is good to keep the module based initialization.
>> Firstly because it causes no harm
>
> If I got a dime for every time people used an argument like that I ... I
> could treat myself to an ice cream. A really big ice cream. Hmm, that
> doesn't sound too impressive. But still, "causes no harm" is below the
> bar for kernel code. Kernel code needs to add value.
>
>> and even though some of the macros
>> are pre-processed out, gives info about the driver.
>
> None of which can't be gotten elsewhere (ie, the commit message, or the
> file these macro reside in).
>

Causes no harm comment had to be read together with more info about
the driver. It causes no harm while providing more info. And as you
only said those macros are pre-processed out to really the defaults
for built-in drivers. So what is the exact big problem with this ? I
can anyway shove out these macros to end the discussion.

BTW whether you  buy the argument or not, please do treat yourself
with ice cream for being able to make such a comment.

>> Secondly there
>> were discussion on why INTEL_SOC_PMIC is bool (note this driver also
>> has module based initialization even when bool).
>
> Yes, there's copy and paste going on even in kernel development.
>

There are other examples in the kernel. I just gave the one which is
related as well.

Regards
Shobhit

>> I am guessing because
>> of some tricky module load order dependencies. If ever that becomes a
>> module, this can mostly be unchanged to be loaded as a module.
>
> You put in a macro, or any other bit of code, when it's needed, not
> beforehand, "just in case". That's silly.
>
> Thanks,
>
>
> Paul Bolle
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1169420 — Re: [Intel-gfx] [PATCH 6/8] drivers/pwm: Add Crystalcove (CRC) PWM driver

[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.
  
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.

Paul.
--

> 
> > 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.
> 
> Thanks,
> 
> 
> Paul Bolle
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web