On July 25, 2015 18:27, Sebastian Reichel wrote: > Hi Adam, > > The driver looks mostly fine. I have a few comments, though: > > On Tue, Jul 07, 2015 at 05:34:19PM +0100, Adam Thomson wrote: > > Signed-off-by: Adam Thomson <Adam.Thomson.Opensource@xxxxxxxxxxx> > > Please add a short description to the commit message. > Ok, I can add something. Didn't feel like it needed to be too descriptive, but can make it a little more verbose. > > [...] > > > > +config FG_DA9150 > > Please name the config entry BATTERY_DA9150 to be consistent with > the other fuel gauges. > Ok no problem. > > [...] > > > > +#include <linux/kernel.h> > > +#include <linux/module.h> > > +#include <linux/platform_device.h> > > +#include <linux/of.h> > > +#include <linux/of_platform.h> > > +#include <linux/slab.h> > > +#include <linux/interrupt.h> > > +#include <linux/delay.h> > > +#include <linux/power_supply.h> > > +#include <linux/list.h> > > +#include <asm/div64.h> > > +#include <linux/mfd/da9150/core.h> > > +#include <linux/mfd/da9150/registers.h> > > I wonder if the "depends CHARGER_DA9150" in Kconfig should be > "depends MFD_DA9150" instead. > I did think about this, but it felt like it was a bit pointless having the fuel-gauge driver without the charger driver, as they're intrinsically tied in HW. I guess if you don't want the charging reporting but still wanted fuel-gauge information then maybe. I don't mind making the change you mention as there should be no functional impact. > > [...] > > + > > +/* Power Supply attributes */ > > +static int da9150_fg_capacity(struct da9150_fg *fg, > > + union power_supply_propval *val) > > +{ > > + da9150_fg_read_sync_start(fg); > > + val->intval = da9150_fg_read_attr(fg, DA9150_QIF_SOC_PCT, > > + DA9150_QIF_SOC_PCT_SIZE); > > + da9150_fg_read_sync_end(fg); > > Create a wrapper function for this. Reading a single value > with sync guards is done multiple times. > > da9150_fg_read_attr_sync(fg, ...) { > da9150_fg_read_sync_start(fg); > da9150_fg_read_attr(fg, ...); > da9150_fg_read_sync_end(fg); > } > Yep, fair point. Can add something for the single attribute accesses. > > + if (val->intval > 100) > > + val->intval = 100; > > + > > + return 0; > > +} > > + > > > > [...] > > + DA9150_QIF_E_FG_STATUS_SIZE); > > + > > + /* Handle warning/critical threhold events */ > > + if ((DA9150_FG_IRQ_LOW_SOC_MASK | DA9150_FG_IRQ_HIGH_SOC_MASK) > & > > + e_fg_status) > > Please make this (e_fg_status & CONSTANT_MASK). > Fine, not a problem. > > + da9150_fg_soc_event_config(fg); > > + > > + /* Clear any FG IRQs */ > > + da9150_fg_write_attr(fg, DA9150_QIF_E_FG_STATUS, > > + DA9150_QIF_E_FG_STATUS_SIZE, e_fg_status); > > + > > + return IRQ_HANDLED; > > +} > > + > > > > [...] > > > > +static int da9150_fg_probe(struct platform_device *pdev) > > +{ > > + struct device *dev = &pdev->dev; > > + struct da9150 *da9150 = dev_get_drvdata(dev->parent); > > + struct da9150_fg_pdata *fg_pdata = dev_get_platdata(dev); > > + struct da9150_fg *fg; > > + int ver, irq, ret; > > + > > + fg = devm_kzalloc(dev, sizeof(*fg), GFP_KERNEL); > > + if (fg == NULL) > > + return -ENOMEM; > > + > > + platform_set_drvdata(pdev, fg); > > + fg->da9150 = da9150; > > + fg->dev = dev; > > + > > + mutex_init(&fg->io_lock); > > + > > + /* Enable QIF */ > > + da9150_set_bits(da9150, DA9150_CORE2WIRE_CTRL_A, > DA9150_FG_QIF_EN_MASK, > > + DA9150_FG_QIF_EN_MASK); > > + > > + fg->battery = power_supply_register(dev, &fg_desc, NULL); > > + if (IS_ERR(fg->battery)) { > > + ret = PTR_ERR(fg->battery); > > + return ret; > > + } > > Please use devm_power_supply_register(...) to simplify the driver. > > > + ver = da9150_fg_read_attr(fg, DA9150_QIF_FW_MAIN_VER, > > + DA9150_QIF_FW_MAIN_VER_SIZE); > > + dev_info(dev, "Version: 0x%x\n", ver); > > + > > + /* Handle DT data if provided */ > > + if (dev->of_node) { > > + fg_pdata = da9150_fg_dt_pdata(dev); > > + dev->platform_data = fg_pdata; > > + } > > + > > + /* Handle any pdata provided */ > > + if (fg_pdata) { > > + fg->interval = fg_pdata->update_interval; > > + > > + if (fg_pdata->warn_soc_lvl > 100) > > + dev_warn(dev, "Invalid SOC warning level provided, > Ignoring"); > > + else > > + fg->warn_soc = fg_pdata->warn_soc_lvl; > > + > > + if ((fg_pdata->crit_soc_lvl > 100) || > > + (fg_pdata->crit_soc_lvl >= fg_pdata->warn_soc_lvl)) > > + dev_warn(dev, "Invalid SOC critical level provided, > Ignoring"); > > + else > > + fg->crit_soc = fg_pdata->crit_soc_lvl; > > + > > + > > + } > > + > > + /* Configure initial SOC level events */ > > + da9150_fg_soc_event_config(fg); > > + > > + /* > > + * If an interval period has been provided then setup repeating > > + * work for reporting data updates. > > + */ > > + if (fg->interval) { > > + INIT_DELAYED_WORK(&fg->work, da9150_fg_work); > > + schedule_delayed_work(&fg->work, > > + msecs_to_jiffies(fg->interval)); > > + } > > + > > + /* Register IRQ */ > > + irq = platform_get_irq_byname(pdev, "FG"); > > + ret = request_threaded_irq(irq, NULL, da9150_fg_irq, IRQF_ONESHOT, "FG", > > + fg); > > + if (ret) > > + goto irq_fail; > > + > > + return ret; > > + > > +irq_fail: > > + cancel_delayed_work(&fg->work); > > + power_supply_unregister(fg->battery); > > + > > + return ret; > > +} > > + > > +static int da9150_fg_remove(struct platform_device *pdev) > > +{ > > + struct da9150_fg *fg = platform_get_drvdata(pdev); > > + int irq; > > + > > + irq = platform_get_irq_byname(pdev, "FG"); > > + free_irq(irq, fg); > > + > > + if (fg->interval) > > + cancel_delayed_work(&fg->work); > > It looks like the order of irq freeing and canceling of the > delayed work is not important. In that case I suggest switching > to devm_request_threaded_irq(...). > Yes, you're right. There's no ordering dependency. Will use devm. > > + power_supply_unregister(fg->battery); > > + > > + return 0; > > +} > > > > [...] > > > > + > > +MODULE_DESCRIPTION("Fuel-Gauge Driver for DA9150"); > > +MODULE_AUTHOR("Adam Thomson > <Adam.Thomson.Opensource@xxxxxxxxxxx>"); > > +MODULE_LICENSE("GPL"); > > MODULE_LICENSE("GPL v2"); If I do this then I need to update the file license disclaimer at the top, as right now they match and are correct as far as I can tell. Is this change needed and if so is this the intention for other drivers as well, just so I'm clear? ��.n��������+%������w��{.n����z�{��ܨ}���Ơz�j:+v�����w����ޙ��&�)ߡ�a����z�ޗ���ݢj��w�f