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
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
