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

[Shobhit Kumar]

On Fri, May 1, 2015 at 2:42 AM, Paul Bolle <pebolle@xxxxxxxxxx> 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@xxxxxxxxx>");
>> +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@xxxxxxxxxxxxxxxxxxxxx
> 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@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/