[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <175E0F9A9EFCEA46A65F5552BB0572980445DB31FB@bgsmsx502.gar.corp.intel.com>
Date: Fri, 15 Jan 2010 17:31:55 +0530
From: "Mahalingam, Nithish" <nithish.mahalingam@...el.com>
To: "avorontsov@...mvista.com" <avorontsov@...mvista.com>
CC: "dwmw2@...radead.org" <dwmw2@...radead.org>,
"cbou@...l.ru" <cbou@...l.ru>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: RE: [RFC] [PATCH] Adding Intel Moorestown PMIC Battery Driver
> You can add lkml as well.
Will do it going forward.
>> +static int pmicbatteryDebug;
>> +module_param(pmicbatteryDebug, int, 0444);
>
> Please don't use camelCase in kernel.
My bad, sorry... I am planning to remove this code anyway.
>> +MODULE_PARM_DESC(pmicbatteryDebug,
>> + "Flag to enable PMIC Battery debug messages.");
>> +
>> +#define PMIC_BATT_DEBUG (pmicbatteryDebug)
>
> I think you don't need this. There is a dynamic debug
> infrastructure exist (see include/linux/device.h, dev_dbg()
> stuff).
Exactly, realized it very late and missed to update the patch with the
change. Will take care...
>> +/* battery cca value */
>> +struct batt_cca_data {
>> + signed int cca_val;
>
> Why explicitly state that this is signed?
My bad... was not intended. Will correct.
>> + unsigned int batt_charge_now; /* in mAS */
>> + unsigned int batt_prev_charge_full; /* in mAS */
>> + unsigned int batt_charge_rate; /* in units per second */
>
> Per include/linux/power_supply.h and
> Documentation/power/power_supply_class.txt
>
> * All voltages, currents, charges, energies, time and temperatures in uV,
> * uA, uAh, uWh, seconds and tenths of degree Celsius unless otherwise
> * stated. It's driver's job to convert its raw values to units in which
> * this class operates.
I just now checked the hardware spec and it is indeed mAh. I will correct
the comment appropriately.
>> + pmic_batt_cmd.ioc = TRUE;
>> + pmic_batt_cmd.cmd = IPC_BATT_CCA_READ;
>> + if (ipc_config_cmd(pmic_batt_cmd, sizeof(struct batt_cca_data),
>
> I'd write it as
>
> ret = ipc_config_cmd(...);
> if (ret) {
> dev_warn(..., "%s(): ipc config cmd failed %d\n", __func__, ret);
> return;
> }
>
> But that's a matter of taste...
:-)... I will accept the comment as it improves readability.
>> + /* set batt_status */
>> + if ((batt_present) && (!batt_exception)) {
>
> No need for these parenthesis.
OK, I tried to be extra cautious... unnecessary I guess.
>> + /* set batt_charge_rate */
>> + if ((pbi->is_dev_info_updated) && (batt_present) && (!batt_exception)) {
>
> Ditto.
I will remove it..
>> + /* batt_charge_now */
>> + if ((batt_present) && (!batt_exception))
>
> The parenthesis aren't needed.
Will take care...
>> + struct pmic_power_module_info *pbi =
>> + (struct pmic_power_module_info *)dev;
>
>This type casting isn't needed.
Yup, redundant type case... will clean.
>> + schedule_work(&pbi->handler);
>
> I think you can use threaded irq for this.
>
> See documentation for request_threaded_irq() in kernel/irq/manage.c.
> And as an example of usage see drivers/mfd/wm8350-irq.c.
Haa that is useful information... completely missed to read about this
feature. Will modify the code to make use of threaded IRQ.
>> + /* check if pmic_power_module_info is initialized */
>> + if (!pbi)
>
> This check is useless. container_of will always return non-NULL
> result.
Hmm good point... I guess I need to have some other clean mechanism to
check it the module is initialized. Will take care in my next patch.
>> + if (PMIC_BATT_DEBUG)
>> + printk(KERN_INFO "pmic-battery: %s() - setting up battery "
>> + "charger successful\n", __func__);
>
> dev_dbg().
Yes will take care of this in my next patch.
>> +static int pmic_battery_probe(struct spi_device *spi)
>> +{
>> + int retval = 0;
>> + struct pmic_power_module_info *pbi = 0;
>
> Do not initialize pointers with 0. Plus, you don't actually need
> to initialize pbi here.
Accepted. Will take care...
>> + /* initialize all required framework before enabling interrupts */
>> + INIT_WORK(&pbi->handler, (void *)pmic_battery_handle_intrpt);
>
> No need for (void *) cast.
Yup, redundant type case... will clean.
>> + /* register interrupt */
>> + retval = request_irq(pbi->irq, pmic_battery_interrupt_handler,
>> + 0, DRIVER_NAME, pbi);
>
> I think you'd better use dev_name() instead of DRIVER_NAME here,
> to distinguish interrupts from several devices.
Good point. Will make the change...
>> +
>> + /* register pmic-batt with power supply subsystem */
>> + pbi->batt.name = "pmic-batt";
>
> This won't work if we have several pmic batteries. I think you need
> kasprintf(GFP_KERNEL, "%s-batt", dev_name(..));
Got it. Will change as suggested.
>> + /* register pmic-usb with power supply subsystem */
>> + pbi->usb.name = "pmic-usb";
>
> kasprintf(GFP_KERNEL, "%s-usb", dev_name(..));
OK, will incorporate the change as suggested.
>> +/**
>> + * pmic_battery_remove - pmic battery finalize
>> + * @spi: pmic battery spi device structure
>> + * Context: can sleep
>> + *
>> + * PMIC battery finalizes its internal data structue and other
>> + * infrastructure components that it initialized in
>> + * pmic_battery_probe.
>> + */
>> +static int pmic_battery_remove(struct spi_device *spi)
>
> __devexit?
Haa right... bad miss.
>> +{
>> + struct pmic_power_module_info *pbi = dev_get_drvdata(&spi->dev);
>> +
>> + if (pbi) {
>
> The check isn't needed. _remove() won't run if device didn't probe
> successfully.
Good point. Will remove it.
>> +};
>> +
>> +
>
> No need for two empty lines.
OK
Thank a lot for the comments Anton. I will incorporate all of these and will re-test
on the hardware before submitting it again.
Regards,
Nithish
Powered by blists - more mailing lists