Date:	Thu, 18 Jun 2015 23:24:35 +0530
From:	Shobhit Kumar <kumar@...bhit.info>
To:	Paul Bolle <pebolle@...cali.nl>
Cc:	Shobhit Kumar <shobhit.kumar@...el.com>,
	linux-pwm <linux-pwm@...r.kernel.org>,
	Jani Nikula <jani.nikula@...el.com>,
	Samuel Ortiz <sameo@...ux.intel.com>,
	Alexandre Courbot <gnurou@...il.com>,
	David Airlie <airlied@...ux.ie>,
	Povilas Staniulis <wdmonster@...il.com>,
	intel-gfx <intel-gfx@...ts.freedesktop.org>,
	linux-kernel <linux-kernel@...r.kernel.org>,
	dri-devel <dri-devel@...ts.freedesktop.org>,
	linux-gpio <linux-gpio@...r.kernel.org>,
	Chih-Wei Huang <cwhuang@...roid-x86.org>,
	Thierry Reding <thierry.reding@...il.com>,
	Daniel Vetter <daniel.vetter@...el.com>,
	Lee Jones <lee.jones@...aro.org>,
	Linus Walleij <linus.walleij@...aro.org>
Subject: Re: [Intel-gfx] [PATCH 6/8] drivers/pwm: Add Crystalcove (CRC) PWM driver

On Fri, May 1, 2015 at 2:42 AM, Paul Bolle <pebolle@...cali.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. Still it is good to keep the module based initialization.
Firstly because it causes no harm and even though some of the macros
are pre-processed out, gives info about the driver. Secondly there
were discussion on why INTEL_SOC_PMIC is bool (note this driver also
has module based initialization even when bool). 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.

Regards
Shobhit

>
> (If I'm wrong, and that object file can actually be part of a module,
> you can stop reading here.)
>
>> --- /dev/null
>> +++ b/drivers/pwm/pwm-crc.c
>
>> +#include <linux/module.h>
>
> Perhaps this include is not needed.
>
>> +static const struct pwm_ops crc_pwm_ops = {
>> +     .config = crc_pwm_config,
>> +     .enable = crc_pwm_enable,
>> +     .disable = crc_pwm_disable,
>> +     .owner = THIS_MODULE,
>
> For built-in only code THIS_MODULE is basically equivalent to NULL (see
> include/linux/export.h). So I guess this line can be dropped.
>
>> +};
>
>> +static struct platform_driver crystalcove_pwm_driver = {
>> +     .probe = crystalcove_pwm_probe,
>> +     .remove = crystalcove_pwm_remove,
>> +     .driver = {
>> +             .name = "crystal_cove_pwm",
>> +     },
>> +};
>> +
>> +module_platform_driver(crystalcove_pwm_driver);
>
> Speaking from memory: for built-in only code this is equivalent to
> calling
>     platform_driver_register(&crystalcove_pwm_driver);
>
> from a wrapper, and marking that wrapper with device_initcall().
>
>> +MODULE_AUTHOR("Shobhit Kumar <shobhit.kumar@...el.com>");
>> +MODULE_DESCRIPTION("Intel Crystal Cove PWM Driver");
>> +MODULE_LICENSE("GPL v2");
>
> These macros will be effectively preprocessed away for built-in only
> code.
>
>
> Paul Bolle
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@...ts.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists