Re: [PATCH] Add OMAP Support for Generic PWM Devices using Dual-mode Timers

[Grant Erickson <marathon96@gmail.com>]

On 11/16/10 10:54 AM, Premi, Sanjeev wrote:
> [sp] pr_info(), dev_info() are already macros - conditional to DEBUG.
>      We should use them instead of defining another layer.

The macros pr_devel, pr_dbg, and dev_dbg are so conditionalized; however,
*_info are straight pass-throughs to printk and dev_printk, respectively.

>> +struct pwm_device {
>> + struct list_head       head;
>> + struct platform_device     *pdev;
>> + struct omap_dm_timer     *dm_timer;
>> + struct omap2_pwm_platform_config   config;
>> + const char      
>>   *label;
>> + unsigned int        use_count;
>> + unsigned int        pwm_id;
>> +};
> 
> [sp] This block has mix of tabs and white spaces. We should be consistent.
>      There are similar instances below as well...
> 
>      You may also want to line up the field names - they start at different
> columns.

Will do. Oddly, these sailed through checkpatch.pl.

>> +int pwm_config(struct pwm_device *pwm, int duty_ns, int period_ns)
>> +{
>> + int status = 0;
>> + const bool enable = true;
>> + const bool autoreload = true;
>> + const bool toggle = true;
>> + const int trigger = OMAP_TIMER_TRIGGER_OVERFLOW_AND_COMPARE;
> 
> [sp] Based on code abobe, I believe that OMAP...COMPARE is macro;
>      shouldn't we use it as is - where trigger is used in code below?

We certainly could; however, I found the above constants to make the code
far more self-documenting.

>> + if (pdata == NULL) {
>> +  dev_err(&pdev->dev, "Could not find required
>> platform data.\n");
>> +  status = -ENOENT;
>> +  goto done;
> 
> [sp] We can simply return status from here. goto and label can be avoided.
> 
>> + }
>> +
>> + /* Allocate memory for the driver-private PWM data and state */
>> +
>> + pwm = kzalloc(sizeof(struct pwm_device), GFP_KERNEL);
>> +
>> + if (pwm == NULL) {
>> +  dev_err(&pdev->dev, "Could not allocate memory.\n");
>> +  status = -ENOMEM;
>> +  goto done;
> 
> [sp] We can simply return status from here. goto and label can be avoided.

We certainly can; however, I prefer to code my functions so there's a single
exit point or block, even if it's a trivial return. See chapter 7 in
Documentation/CodingStyle.txt.

Thanks for your feedback and comments.

Regards,

Grant