Re: [PATCH] Staging: android: alarm: Renamed pr_alarm to alarm_dbg

[Joe Perches]

On Mon, 2012-05-21 at 21:44 +0200, Nasir Abed wrote:
> I renamed a macro to make it explicit that it's for debugging and
> fixed a warning about a quoted string split across multiple lines.

Hi Nasir.

Welcome to linux-kernel developing.

It's generally pretty unnecessary to resubmit a variant of
a patch that someone else has already submitted.

Especially one like this that does less than the original.

It's also considered good form to cc the original patch
submitter.

If you want to get your 2nd patch in, that's great, but
please consider doing work that isn't just checkpatch
cleanups and isn't duplicative of work where you were cc'd.

Style comments below: (duplicates not shown)

> diff --git a/drivers/staging/android/alarm.c b/drivers/staging/android/alarm.c

Is this done against -next or some other -staging tree?

> @@ -97,11 +96,11 @@ static void update_timer_locked(struct alarm_queue *base, bool head_removed)
>  
>  	alarm = container_of(base->first, struct android_alarm, node);
>  
> -	pr_alarm(FLOW, "selected alarm, type %d, func %pF at %lld\n",
> +	alarm_dbg(FLOW, "selected alarm, type %d, func %pF at %lld\n",
>  		alarm->type, alarm->function, ktime_to_ns(alarm->expires));

It's generally nicer to align arguments to the open parenthesis.

>  	if (is_wakeup && suspended) {
> -		pr_alarm(FLOW, "changed alarm while suspened\n");
> +		alarm_dbg(FLOW, "changed alarm while suspened\n");

Please look for and fix spelling typos when submitting changes.

> @@ -292,18 +291,18 @@ int android_alarm_set_rtc(struct timespec new_time)
>  	}
>  	spin_unlock_irqrestore(&alarm_slock, flags);
>  	if (ret < 0) {
> -		pr_alarm(ERROR, "alarm_set_rtc: Failed to set time\n");
> +		alarm_dbg(ERROR, "alarm_set_rtc: Failed to set time\n");

Please use %s: and __func__ when a function name is embedded
into a message string.

> @@ -499,7 +498,7 @@ static int rtc_alarm_add_device(struct device *dev,
>  	if (err)
>  		goto err3;
>  	alarm_rtc_dev = rtc;
> -	pr_alarm(INIT_STATUS, "using rtc device, %s, for alarms", rtc->name);
> +	alarm_dbg(INIT_STATUS, "using rtc device, %s, for alarms", rtc->name);
>  	mutex_unlock(&alarm_setrtc_mutex);
>  
>  	return 0;

Please make sure messages are newline "\n" terminated.
It helps to avoid possible output interleaving.

--
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/