[Meego-kernel] [PATCH] Intel Medfield Battery driver Patch

[Arjan van de Ven]

  On 9/26/2010 10:55 PM, Pallala, Ramakrishna wrote:
> +#define MSIC_SRAM_IOMAP_ADDRESS 0xFFFF7FC3
>> I don't think so. This address needs to come from either firmware or
>> PCI, but not be hardcoded in the driver like this..... Absolutely not.
> The driver is platform driver in Medfield and there is no Medfield specific code
> As we have in Moorestown(mrst.c)

first of all, mrst.c is for both moorestown and medfield

second.. this value STILL needs to come from firmware. Really.
either in the form of a bar or some SFI field.

consider this a critical bug in the firmware if it doesn't provide this.
why is your driver populating something that is supposed to come from
>> firmware????????
>> something must be very wrong here.
> Yes I should get this info from Firmware. But as of now there is no support from the firmware.
> Will change as soon as the firmware adds this support.

what's the bug number against teh firmware, and what is the ETA for the 
fix ?



>> +static int msic_battery_suspend(struct platform_device *pdev,
>>
>> your suspend/resumes are awfully empty...
>> are you sure that's right? Wouldn't you need to at least turn the
>> charging into a safe (slow) mode?
> When the charging is in progress suspend will fail.
> If charger is not present there are no msic power rails I can shut it down.

you are making an assumption that a device driver can block the suspend.
while today that is kinda possible, this is not for long.... the 
direction will be that the OS *will* suspend regardless, at least over time.
(this is also what other OSes do)
Please don't depend on this behavior being possible.


> I will fix the above accepted comments with proper description of the
> driver and will re-submit the patch.
> This is basic working driver tested on hardware, I will add locking and synchronization in
> In my future or next version of the patch.


I'll be waiting for the next version of the patch...