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

[Ryan Mallon <ryan@bluewatersys.com>]

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@bluewatersys.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@bluewatersys.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