linux-kernel - Re: [REPOST PATCH] rtc-isl1208: Add alarm support

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <4D61AC33.3070409@bluewatersys.com>
Date:	Mon, 21 Feb 2011 13:05:07 +1300
From:	Ryan Mallon <ryan@...ewatersys.com>
To:	Jesper Juhl <jj@...osbits.net>
CC:	a.zummo@...ertech.it, rtc-linux@...glegroups.com,
	linux kernel <linux-kernel@...r.kernel.org>, hvr@....org
Subject: Re: [REPOST PATCH] rtc-isl1208: Add alarm support

On 02/21/2011 12:52 PM, Jesper Juhl wrote:
> On Mon, 21 Feb 2011, Ryan Mallon wrote:
> 
>> Add alarm/wakeup support to rtc isl1208 driver
>>
> 
> A few (very minor) comments below.

Hi Jepser. Thanks for the review. See answers below.

I'm unsure where this patch is meant to be applied. There is a list and
a maintainer (but no git tree?) for the rtc subsystem, but there doesn't
seem to be much activity from either lately?

> 
> 
>> Signed-off-by: Ryan Mallon <ryan@...ewatersys.com>
>> ---
>>
>> diff --git a/drivers/rtc/rtc-isl1208.c b/drivers/rtc/rtc-isl1208.c
>>  static int
>> +isl1208_rtc_toggle_alarm(struct i2c_client *client, int enable)
>> +{
>> +	int icr;
>> +
>> +	icr = i2c_smbus_read_byte_data(client, ISL1208_REG_INT);
> 
> int icr = i2c_smbus_read_byte_data(client, ISL1208_REG_INT);
> 
> ??
> 
> No functional change, but cuts down on source size.

Sure, I can change this.

>> +static int
>>  isl1208_rtc_proc(struct device *dev, struct seq_file *seq)
>>  {
>>  	struct i2c_client *const client = to_i2c_client(dev);
>> @@ -288,7 +315,7 @@ isl1208_i2c_read_alarm(struct i2c_client *client, struct rtc_wkalrm *alarm)
>>  {
>>  	struct rtc_time *const tm = &alarm->time;
>>  	u8 regs[ISL1208_ALARM_SECTION_LEN] = { 0, };
>> -	int sr;
>> +	int icr, yr, sr;
>>  
>>  	sr = isl1208_i2c_get_sr(client);
> 
> Maybe it's just me, but I'd have written this as
> 
> int icr, yr;
> int sr = isl1208_i2c_get_sr(client);

Same here. I'm not hugely fussed either way.

>> @@ -514,6 +677,9 @@ isl1208_probe(struct i2c_client *client, const struct i2c_device_id *id)
>>  
>>  exit_unregister:
>>  	rtc_device_unregister(rtc);
>> +exit_free_irq:
>> +	if (client->irq)
>> +		free_irq(client->irq, client);
> 
> I didn't dig deep into this, but from a cursory look it seems that 
> free_irq() should be fine being handed NULL. If that's right, then the 
> condifional "if" is not needed here..

I think having the if is a bit clearer, since it will only try and free
the irq in the case where we have actually allocated it. This is on the
error exit path so performance is not exactly critical.

~Ryan

-- 
Bluewater Systems Ltd - ARM Technology Solution Centre

Ryan Mallon         		5 Amuri Park, 404 Barbadoes St
ryan@...ewatersys.com         	PO Box 13 889, Christchurch 8013
http://www.bluewatersys.com	New Zealand
Phone: +64 3 3779127		Freecall: Australia 1800 148 751
Fax:   +64 3 3779135			  USA 1800 261 2934
--
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/