[Meego-kernel] [PATCH] Intel Medfield Battery driver Patch
Pallala, Ramakrishna
ramakrishna.pallala
Sun Sep 26 22:55:43 PDT 2010
Hi Aryan,
> +module_param(enable_debug, int, 0444);
> +MODULE_PARM_DESC(enable_debug,
> + "Flag to enable MSIC Battery debug messages.");
> +
> +#define MSIC_BATT_DEBUG (enable_debug)
>
> why do you have your own mechanism for this? Linux has a generic
> mechanism for such runtime debug enabling
Accepted and will fix it.
> +#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)
> +#define IS_BTP_VALID_ADC(adc_val) (((adc_val)>= (MSIC_BTP_ADC_MIN))&& \
> + ((adc_val)<= (MSIC_BTP_ADC_MAX)))
>
> this needs to be a function; you're multiple-evaluating your macro's
> arguments (incorrectly I might add)...
> ... that's a sure sign something needs to be a function in practice,
> especially if you otherwise don't do it correctly.
>
> +/* Temparature conversion Macros */
> +#define CONV_ADC_TEMP(val, max, diff_adc, diff_temp) (((max - val) * \
> + (diff_temp)) / (diff_adc))
> +
> +#define IS_VALID_TEMP_ADC_RANGE(val, min, max) (((val)> (min))&& \
> + ((val)<= (max)))
>
> likewise for these.
Accepted and will fix it.
> +/**
> + * msic_battery_interrupt_handler - msic battery interrupt handler
> + * Context: interrupt context
> + *
> + * MSIC battery interrupt handler which will be called on insertion
> + * of valid power source to charge the battery or an exception
> + * condition occurs.
> + */
> +static irqreturn_t msic_battery_interrupt_handler(int id, void *dev)
> +{
> + struct msic_power_module_info *mbi =
> + (struct msic_power_module_info *)dev;
> +
> + /* Copy Interrupt registers locally */
> + mbi->regs[0] = readb(mbi->msic_regs_iomap);
> + mbi->regs[1] = readb(mbi->msic_regs_iomap + 1);
> + mbi->regs[2] = readb(mbi->msic_regs_iomap + 2);
> + mbi->regs[3] = readb(mbi->msic_regs_iomap + 3);
> +
> + schedule_work(&mbi->handler);
> +
> + return IRQ_HANDLED;
> +}
>
> and this is extremely racey.. even if you had mythical locking that your
> driver unfortunately does not have yet.
> how are you protecting against a 2nd interrupt coming in before the
> scheduled work happened? (or even.. during)
Accepted and will fix it using queue with lock in the handler.
> +/**
> + * sfi_table_populate - Simple Firmware Interface table Populate
> + * @sfi_table: Simple Firmware Interface table structure
> + *
> + * SFI table has entries for the temperature limits
> + * which is populated in a local structure
> + */
> +void sfi_table_populate(struct msic_batt_sfi_prop *sfi_table)
> +{
> + memcpy(sfi_table->sign, "BATT", sizeof("BATT"));
> + sfi_table->length = 183;
> + sfi_table->revision = 1;
> + sfi_table->checksum = 15;
> + memcpy(sfi_table->oem_id, "INTEL", sizeof("INTEL"));
> + memcpy(sfi_table->oem_tid, "OEMTID", sizeof("OEMTID"));
> + memcpy(sfi_table->batt_id.manufac, "NK", sizeof("NK"));
> + memcpy(sfi_table->batt_id.model, "BP4L", sizeof("BP4L"));
> + memcpy(sfi_table->batt_id.sub_ver, "00", sizeof("00"));
> + sfi_table->voltage_max = 4200;
> + sfi_table->capacity = 1500;
> + sfi_table->battery_type = 3; /* POWER_SUPPLY_TECHNOLOGY_LIPO */
> + sfi_table->safe_temp_low_lim = 0;
> + sfi_table->safe_temp_up_lim = 60;
> + sfi_table->safe_vol_low_lim = 3700;
> + sfi_table->safe_vol_up_lim = 4200;
> + sfi_table->chrg_cur_lim = 1000;
> + sfi_table->chrg_term_lim = 1;
> + sfi_table->term_cur = 50;
> + sfi_table->temp_mon_ranges = 4;
>
> 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.
> + retval = power_supply_register(&pdev->dev, &mbi->usb);
>
> + if (retval) {
> + dev_err(&pdev->dev, "%s(): failed to register msic usb "
> + "device with power supply subsystem\n",
> + __func__);
> + goto power_reg_failed_usb;
> + }
> +
> + /* Re Map Phy address space for MSIC regs */
> + mbi->msic_regs_iomap = ioremap_nocache(MSIC_SRAM_IOMAP_ADDRESS, 8);
>
> this looks inside out; once you register your power supply with the
> kernel, all its functions can be called....
> and yet the driver keeps doing a bunch of initialization work after this
> point...
> I'll call this highly suspect and likely broken.
Accepted and will fix it.
> + if (mbi) {
>
> + free_irq(mbi->irq, mbi);
> + power_supply_unregister(&mbi->usb);
> + power_supply_unregister(&mbi->batt);
> + if (mbi->msic_regs_iomap != NULL)
> + iounmap(mbi->msic_regs_iomap);
> + penwell_otg_unregister_bc_callback(otg_handle);
> + flush_scheduled_work();
>
> this looks buggy; you only flush scheduled work after you tear down the
> ioremap region
> ... can you prove there's no work trying to access that at this point?
Accepted and will fix it.
> +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.
> Your submission is rather lacking in some key areas unfortunately
>
> 1) There is no real description of what the patch does
> 2) You don't say what kernel / version this is for
> 3) You're introducing new KConfig variables but don't say what values
> they should have.
> (and 4) I assume you have completed the Intel internal open source
> approval process for this)
>
> Looking over the driver, there is not a single lock anywhere in the driver!
> Even though you do various read-modify-write cycles on various things..
> there is absolutely no synchronization. Anywhere.
> How mature is this driver actually? Is this a very early prototype and
> will you add the locking before the actual submission?
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.
Thanks,
Ram
-----Original Message-----
From: Arjan van de Ven [mailto:arjan at linux.intel.com]
Sent: Monday, September 27, 2010 10:18 AM
To: Pallala, Ramakrishna
Cc: meego-kernel at lists.meego.com; Mai, Leonard; Cox, Alan
Subject: Re: [Meego-kernel] [PATCH] Intel Medfield Battery driver Patch
On 9/26/2010 9:11 PM, Pallala, Ramakrishna wrote:
> Hi All,
>
> This is Intel Medfield Battery driver patch.
Your submission is rather lacking in some key areas unfortunately
1) There is no real description of what the patch does
2) You don't say what kernel / version this is for
3) You're introducing new KConfig variables but don't say what values
they should have.
(and 4) I assume you have completed the Intel internal open source
approval process for this)
Looking over the driver, there is not a single lock anywhere in the driver!
Even though you do various read-modify-write cycles on various things..
there is absolutely no synchronization. Anywhere.
How mature is this driver actually? Is this a very early prototype and
will you add the locking before the actual submission?
some more detailed comments below+static int enable_debug = 1;
> +module_param(enable_debug, int, 0444);
> +MODULE_PARM_DESC(enable_debug,
> + "Flag to enable MSIC Battery debug messages.");
> +
> +#define MSIC_BATT_DEBUG (enable_debug)
why do you have your own mechanism for this? Linux has a generic
mechanism for such runtime debug enabling
+/* Status registers */
> +#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.
> +#define IS_BTP_VALID_ADC(adc_val) (((adc_val)>= (MSIC_BTP_ADC_MIN))&& \
> + ((adc_val)<= (MSIC_BTP_ADC_MAX)))
this needs to be a function; you're multiple-evaluating your macro's
arguments (incorrectly I might add)...
... that's a sure sign something needs to be a function in practice,
especially if you otherwise don't do it correctly.
> +/* Temparature conversion Macros */
> +#define CONV_ADC_TEMP(val, max, diff_adc, diff_temp) (((max - val) * \
> + (diff_temp)) / (diff_adc))
> +
> +#define IS_VALID_TEMP_ADC_RANGE(val, min, max) (((val)> (min))&& \
> + ((val)<= (max)))
likewise for these.
+
> +/**
> + * msic_battery_interrupt_handler - msic battery interrupt handler
> + * Context: interrupt context
> + *
> + * MSIC battery interrupt handler which will be called on insertion
> + * of valid power source to charge the battery or an exception
> + * condition occurs.
> + */
> +static irqreturn_t msic_battery_interrupt_handler(int id, void *dev)
> +{
> + struct msic_power_module_info *mbi =
> + (struct msic_power_module_info *)dev;
> +
> + /* Copy Interrupt registers locally */
> + mbi->regs[0] = readb(mbi->msic_regs_iomap);
> + mbi->regs[1] = readb(mbi->msic_regs_iomap + 1);
> + mbi->regs[2] = readb(mbi->msic_regs_iomap + 2);
> + mbi->regs[3] = readb(mbi->msic_regs_iomap + 3);
> +
> + schedule_work(&mbi->handler);
> +
> + return IRQ_HANDLED;
> +}
and this is extremely racey.. even if you had mythical locking that your
driver unfortunately does not have yet.
how are you protecting against a 2nd interrupt coming in before the
scheduled work happened? (or even.. during)
+
> +/**
> + * sfi_table_populate - Simple Firmware Interface table Populate
> + * @sfi_table: Simple Firmware Interface table structure
> + *
> + * SFI table has entries for the temperature limits
> + * which is populated in a local structure
> + */
> +void sfi_table_populate(struct msic_batt_sfi_prop *sfi_table)
> +{
> + memcpy(sfi_table->sign, "BATT", sizeof("BATT"));
> + sfi_table->length = 183;
> + sfi_table->revision = 1;
> + sfi_table->checksum = 15;
> + memcpy(sfi_table->oem_id, "INTEL", sizeof("INTEL"));
> + memcpy(sfi_table->oem_tid, "OEMTID", sizeof("OEMTID"));
> + memcpy(sfi_table->batt_id.manufac, "NK", sizeof("NK"));
> + memcpy(sfi_table->batt_id.model, "BP4L", sizeof("BP4L"));
> + memcpy(sfi_table->batt_id.sub_ver, "00", sizeof("00"));
> + sfi_table->voltage_max = 4200;
> + sfi_table->capacity = 1500;
> + sfi_table->battery_type = 3; /* POWER_SUPPLY_TECHNOLOGY_LIPO */
> + sfi_table->safe_temp_low_lim = 0;
> + sfi_table->safe_temp_up_lim = 60;
> + sfi_table->safe_vol_low_lim = 3700;
> + sfi_table->safe_vol_up_lim = 4200;
> + sfi_table->chrg_cur_lim = 1000;
> + sfi_table->chrg_term_lim = 1;
> + sfi_table->term_cur = 50;
> + sfi_table->temp_mon_ranges = 4;
why is your driver populating something that is supposed to come from
firmware????????
something must be very wrong here.
+ retval = power_supply_register(&pdev->dev, &mbi->usb);
> + if (retval) {
> + dev_err(&pdev->dev, "%s(): failed to register msic usb "
> + "device with power supply subsystem\n",
> + __func__);
> + goto power_reg_failed_usb;
> + }
> +
> + /* Re Map Phy address space for MSIC regs */
> + mbi->msic_regs_iomap = ioremap_nocache(MSIC_SRAM_IOMAP_ADDRESS, 8);
this looks inside out; once you register your power supply with the
kernel, all its functions can be called....
and yet the driver keeps doing a bunch of initialization work after this
point...
I'll call this highly suspect and likely broken.
+ if (mbi) {
> + free_irq(mbi->irq, mbi);
> + power_supply_unregister(&mbi->usb);
> + power_supply_unregister(&mbi->batt);
> + if (mbi->msic_regs_iomap != NULL)
> + iounmap(mbi->msic_regs_iomap);
> + penwell_otg_unregister_bc_callback(otg_handle);
> + flush_scheduled_work();
this looks buggy; you only flush scheduled work after you tear down the
ioremap region
... can you prove there's no work trying to access that at this point?
+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?
More information about the Meego-kernel
mailing list