[LPIA Hardy LUM] Disable RTC wake alarm to reduce power in Hibernate - attempt #3

[Colin Ian King]

On Wed, 2009-03-18 at 14:38 +0000, Andy Whitcroft wrote:
[ stuff deleted ]
> 
> Checkpatch suggests strict_strtoul here, any reason to not use that
> (safer) function?

strict_stroul appeared in commit
06b2a76d25d3cfbd14680021c1d356c91be6904e which only appeared in Intrepid
and is not in Hardy at all, so it's a false error from checkpatch here.

> 
> > +	DPRINT("%s: new value = %lu\n", __func__, val);
> > +	/* See section 10.8.3.2 of Intel(TM) I/O Controller
> > +	 * Hub 7 (ICH7) Family data sheet */
> > +	original_data = new_data = inw(pm_base_address + PM_RTC_EN_OFFSET);
> > +	new_data = original_data;
> 
> That looks to be a double assignment to new_data?

Yep, from original code. Will fix.

> 
> > +
> > +	if (val)
> > +		new_data |= RTC_EN;
> > +	else
> > +		new_data &= ~RTC_EN;
> > +
> > +	if (new_data != original_data)
> > +		outw(new_data, pm_base_address + PM_RTC_EN_OFFSET);
> 
> I might recommend a blank line here.

Will do.

> 
> > +	return count;
> > +}
> > +
> > +
> > +
> >  static SENSOR_DEVICE_ATTR(bluetooth,            /* name */
> > -                          S_IWUSR | S_IRUGO,    /* mode */
> > -                          bluetooth_show,       /* show */
> > -                          bluetooth_set,        /* store */
> > -                          0);                   /* index */
> > +			  S_IWUSR | S_IRUGO,    /* mode */
> > +			  bluetooth_show,       /* show */
> > +			  bluetooth_set,        /* store */
> > +			  0);                   /* index */
> >  
> >  static SENSOR_DEVICE_ATTR(wifi,                 /* name */
> > -                          S_IWUSR | S_IRUGO,    /* mode */
> > -                          wifi_show,            /* show */
> > -                          wifi_set,             /* store */
> > -                          0);                   /* index */
> > +			  S_IWUSR | S_IRUGO,    /* mode */
> > +			  wifi_show,            /* show */
> > +			  wifi_set,             /* store */
> > +			  1);                   /* index */
> >  
> >  static SENSOR_DEVICE_ATTR(wwan,                 /* name */
> > -                          S_IWUSR | S_IRUGO,    /* mode */
> > -                          wwan_show,            /* show */
> > -                          wwan_set,             /* store */
> > -                          0);                   /* index */
> > +			  S_IWUSR | S_IRUGO,    /* mode */
> > +			  wwan_show,            /* show */
> > +			  wwan_set,             /* store */
> > +			  2);                   /* index */
> 
> Ok these are all cleanups right?  and should be in the previous patch?

Yes and no, the 2nd patch changes the last args on all the
SENSOR_DEVICE_ATTR.  Checkpatch grumbled at the formatting, so I fixed
this on the 2nd patch. 

> 
> >  static SENSOR_DEVICE_ATTR(ethernet,             /* name */
> > -                          S_IWUSR | S_IRUGO,    /* mode */
> > -                          ethernet_show,        /* show */
> > -                          ethernet_set,         /* store */
> > -                          0);                   /* index */
> > +			  S_IWUSR | S_IRUGO,    /* mode */
> > +			  ethernet_show,        /* show */
> > +			  ethernet_set,         /* store */
> > +			  3);                   /* index */
> > +
> > +static SENSOR_DEVICE_ATTR(rtc,                  /* name */
> > +			  S_IWUSR | S_IRUGO,    /* mode */
> > +			  rtc_show,             /* show */
> > +			  rtc_set,              /* store */
> > +			  4);                   /* index */
> >  
> >  static struct attribute *hpmini_attributes[] =
> >  {
> > @@ -279,6 +342,7 @@ static struct attribute *hpmini_attributes[] =
> >  	&sensor_dev_attr_wifi.dev_attr.attr,
> >  	&sensor_dev_attr_wwan.dev_attr.attr,
> >  	&sensor_dev_attr_ethernet.dev_attr.attr,
> > +	&sensor_dev_attr_rtc.dev_attr.attr,
> >  	NULL
> >  };
> >  
> > @@ -319,6 +383,14 @@ static int __devinit hpmini_probe(struct platform_device *dev)
> >  
> >  	DPRINT("base address = 0x%x\n", gpio_base_address);
> >  
> > +	pci_read_config_dword(pcidev, PM_BASE_ADDRESS_REGISTER,
> > +			      &pm_base_address);
> > +	/* Only bits 15:7 of the PM base address register are valid,
> > +	 * so mask off all other bits */
> > +	pm_base_address &= PM_BASE_ADDRESS_MASK;
> > +
> > +	DPRINT("base address = 0x%x\n", pm_base_address);
> > +
> >  	return 0;
> >  }
> 
> -apw
> 
-- 
Colin King   <colin.king at canonical.com>
"Me transmitte sursum, caledoni"