Hello Krzysztof, Thanks for feedback :). More comments inline. Thanks, Anda ________________________________________ From: Krzysztof Kozłowski [k.kozlowski.k@xxxxxxxxx] Sent: Saturday, April 25, 2015 10:25 AM To: Nicolae, Anda-maria Cc: sre@xxxxxxxxxx; dbaryshkov@xxxxxxxxx; robh+dt@xxxxxxxxxx; pawel.moll@xxxxxxx; mark.rutland@xxxxxxx; ijc+devicetree@xxxxxxxxxxxxxx; galak@xxxxxxxxxxxxxx; dwmw2@xxxxxxxxxxxxx; linux-pm@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; devicetree@xxxxxxxxxxxxxxx Subject: Re: [PATCH] power_supply: Add support for Richtek rt9455 battery charger 2015-04-24 16:57 GMT+09:00 Anda-Maria Nicolae <anda-maria.nicolae@xxxxxxxxx>: > Based on the datasheet found here: > http://www.richtek.com/download_ds.jsp?p=RT9455 Hi, Some comments below. > Signed-off-by: Anda-Maria Nicolae <anda-maria.nicolae@xxxxxxxxx> > --- > .../devicetree/bindings/power/rt9455_charger.txt | 38 + > .../devicetree/bindings/vendor-prefixes.txt | 1 + > drivers/power/Kconfig | 6 + > drivers/power/Makefile | 1 + > drivers/power/rt9455_charger.c | 1770 ++++++++++++++++++++ > 5 files changed, 1816 insertions(+) > create mode 100644 Documentation/devicetree/bindings/power/rt9455_charger.txt > create mode 100644 drivers/power/rt9455_charger.c > > diff --git a/Documentation/devicetree/bindings/power/rt9455_charger.txt b/Documentation/devicetree/bindings/power/rt9455_charger.txt > new file mode 100644 > index 0000000..f716cf6 > --- /dev/null > +++ b/Documentation/devicetree/bindings/power/rt9455_charger.txt > @@ -0,0 +1,38 @@ > +Binding for RT rt9455 battery charger > + > +Required properties: > +- compatible: Should contain one of the following: > + * "rt,rt9455" > + > +- reg: integer, i2c address of the device. > +- rt,ICHRG: integer, output charge current in uA. > +- rt,IEOC_PERCENTAGE: integer, the percent of the output charge current (ICHRG). > + When the current in constant-voltage phase drops below > + ICHRG x IEOC_PERCENTAGE, charge is terminated. > +- rt,VOREG: integer, battery regulation voltage in uV. > + > +Optional properties: > +- rt,VMIVR: integer, minimum input voltage regulation in uV. > + Prevents input voltage drop due to insufficient current > + provided by the power source. > +- rt,IAICR: integer, input current regulation in uA. These should be lowercase separated with dash '-'. Will rename the properties to be lowercase separated with dash '-'. > + > +Example: > + > +rt9455@22 { > + compatible = "rt,rt9455"; > + reg = <0x22>; > + > + interrupt-parent = <&gpio1>; > + interrupts = <0 1>; > + > + interrupt-gpio = <&gpio1 0 1>; > + reset-gpio = <&gpio1 1 1>; > + > + rt,ICHRG = <500000>; > + rt,IEOC_PERCENTAGE = <10>; > + rt,VOREG = <4200000>; > + > + rt,VMIVR = <4500000>; > + rt,IAICR = <500000>; > +}; > diff --git a/Documentation/devicetree/bindings/vendor-prefixes.txt b/Documentation/devicetree/bindings/vendor-prefixes.txt > index 389ca13..dc868ed 100644 > --- a/Documentation/devicetree/bindings/vendor-prefixes.txt > +++ b/Documentation/devicetree/bindings/vendor-prefixes.txt > @@ -148,6 +148,7 @@ raidsonic RaidSonic Technology GmbH > ralink Mediatek/Ralink Technology Corp. > ramtron Ramtron International > realtek Realtek Semiconductor Corp. > +rt Richtek Technology Corporation Actually I would prefer sticking to "richtek" prefix, sent some time ago by Beomho Seo: http://lwn.net/Articles/625133/ "rt" could be easily taken as "Realtek". Will use richtek. (...) > +/* > + * Each array initialised below shows the possible real-world values for a > + * group of bits belonging to RT9455 registers. The arrays are sorted in > + * ascending order. The index of each real-world value represents the value > + * that is encoded in the group of bits belonging to RT9455 registers. > + */ > +/* REG06[6:4] (ICHRG) in uAh */ > +static const int rt9455_ichrg_values[] = { > + 500000, 650000, 800000, 950000, 1100000, 1250000, 1400000, 1550000 > +}; > + > +/* REG02[7:2] (VOREG) in uV */ > +static const int rt9455_voreg_values[] = { > + 3500000, 3520000, 3540000, 3560000, 3580000, 3600000, 3620000, 3640000, > + 3660000, 3680000, 3700000, 3720000, 3740000, 3760000, 3780000, 3800000, > + 3820000, 3840000, 3860000, 3880000, 3900000, 3920000, 3940000, 3960000, > + 3980000, 4000000, 4020000, 4040000, 4060000, 4080000, 4100000, 4120000, > + 4140000, 4160000, 4180000, 4200000, 4220000, 4240000, 4260000, 4280000, > + 4300000, 4330000, 4350000, 4370000, 4390000, 4410000, 4430000, 4450000, > + 4450000, 4450000, 4450000, 4450000, 4450000, 4450000, 4450000, 4450000, > + 4450000, 4450000, 4450000, 4450000, 4450000, 4450000, 4450000, 4450000 > +}; > + > +/* REG07[3:0] (VMREG) in uV */ > +static const int rt9455_vmreg_values[] = { > + 4200000, 4220000, 4240000, 4260000, 4280000, 4300000, 4320000, 4340000, > + 4360000, 4380000, 4400000, 4430000, 4450000, 4450000, 4450000, 4450000 > +}; > + > +/* REG05[5:4] (IEOC_PERCENTAGE) */ > +static const int rt9455_ieoc_percentage_values[] = { > + 10, 30, 20, 30 > +}; > + > +/* REG05[1:0] (VMIVR) in uV */ > +static const int rt9455_vmivr_values[] = { > + 4000000, 4250000, 4500000, 5000000 > +}; > + > +/* REG05[1:0] (IAICR) in uA */ > +static const int rt9455_iaicr_values[] = { > + 100000, 500000, 1000000, 2000000 > +}; > + > +struct rt9455_info { > + struct i2c_client *client; > + struct power_supply *charger; > +#if IS_ENABLED(CONFIG_USB_PHY) > + struct usb_phy *usb_phy; > + struct notifier_block nb; > +#endif > + struct gpio_desc *gpiod_irq; > + unsigned int gpio_irq; > + struct delayed_work pwr_rdy_work; > + struct delayed_work max_charging_time_work; > + struct delayed_work batt_presence_work; > +}; > + > +/* I2C read/write API */ > +static int rt9455_read(struct rt9455_info *info, u8 reg, u8 *data) > +{ > + int ret; > + > + ret = i2c_smbus_read_byte_data(info->client, reg); > + if (ret < 0) > + return ret; > + > + *data = ret; > + return 0; > +} > + > +static int rt9455_write(struct rt9455_info *info, u8 reg, u8 data) > +{ > + return i2c_smbus_write_byte_data(info->client, reg, data); > +} > + > +static int rt9455_read_mask(struct rt9455_info *info, u8 reg, > + u8 mask, u8 shift, u8 *data) > +{ > + u8 v; > + int ret; > + > + ret = rt9455_read(info, reg, &v); > + if (ret < 0) > + return ret; > + > + v &= mask; > + v >>= shift; > + *data = v; > + > + return 0; > +} > + > +static int rt9455_write_mask(struct rt9455_info *info, u8 reg, > + u8 mask, u8 shift, u8 data) > +{ > + u8 v; > + int ret; > + > + ret = rt9455_read(info, reg, &v); > + if (ret < 0) > + return ret; > + > + v &= ~mask; > + v |= ((data << shift) & mask); > + > + return rt9455_write(info, reg, v); > +} Maybe just regmap_read(), regmap_write() and regmap_update_bits(). Ok, I will use regmap. > + > +/* > + * Iterate through each element of the 'tbl' array until an element whose value > + * is greater than v is found. Return the index of the respective element, > + * or the index of the last element in the array, if no such element is found. > + */ > +static u8 rt9455_find_idx(const int tbl[], int tbl_size, int v) > +{ > + int i; > + > + /* No need to iterate until the last index in the table because > + * if no element greater than v is found in the table, > + * or if only the last element is greater than v, > + * function returns the index of the last element. > + */ Here and in other places - please use standard kernel multi-line comment: /* * lorem * ipsum */ Will do. (...) > + > +static void rt9455_batt_presence_work_callback(struct work_struct *work) > +{ > + struct rt9455_info *info = container_of(work, struct rt9455_info, > + batt_presence_work.work); > + struct device *dev = &info->client->dev; > + u8 irq1; > + int ret; > + > + ret = rt9455_read(info, RT9455_REG_IRQ1, &irq1); > + if (ret) { > + dev_err(dev, "Failed to read IRQ1 register\n"); > + return; > + } > + > + /* If the battery is still absent, batt_presence_work is rescheduled. > + Otherwise, max_charging_time is scheduled. > + */ > + if (irq1 & RT9455_REG_IRQ1_BATAB_MASK) { > + schedule_delayed_work(&info->batt_presence_work, > + RT9455_BATT_PRESENCE_DELAY * HZ); > + } else { > + schedule_delayed_work(&info->max_charging_time_work, > + RT9455_MAX_CHARGING_TIME * HZ); > + } Do all of these schedules have to be executed on system_wq? Maybe power efficient workqueue could be used? I don't see any locking so probably not... but still it is worth investigating. Ok, I will use queue_delayed_work(system_power_efficient_wq, ...). I'm not sure if I understand exactly what do you mean by "I don't see any locking so probably not". I can see that __queue_work() uses spinlocks. And other drivers that use system_power_efficient_wq also do not use additional locking. I intend to replace schedule_delayed_work() with queue_delayed_work(system_power_efficient_wq, ...), without adding any locking in the driver. Please let me know if I understood correctly what you meant . > +} > + > +struct power_supply_desc rt9455_charger_desc = { > + .name = RT9455_DRIVER_NAME, > + .type = POWER_SUPPLY_TYPE_USB, > + .properties = rt9455_charger_properties, > + .num_properties = ARRAY_SIZE(rt9455_charger_properties), > + .get_property = rt9455_charger_get_property, > + .set_property = rt9455_charger_set_property, > + .property_is_writeable = rt9455_charger_property_is_writeable, > +}; This can be "static const". Will do. > + > +static int rt9455_probe(struct i2c_client *client, > + const struct i2c_device_id *id) > +{ > + struct i2c_adapter *adapter = to_i2c_adapter(client->dev.parent); > + struct device *dev = &client->dev; > + struct rt9455_info *info; > + struct power_supply_config rt9455_charger_config = {}; > + /* mandatory device-specific data values */ > + u32 ichrg, ieoc_percentage, voreg; > + /* optional device-specific data values */ > + u32 vmivr = -1, iaicr = -1; > + int ret; > + > + if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_BYTE_DATA)) { > + dev_err(dev, "No support for SMBUS_BYTE_DATA\n"); > + return -ENODEV; > + } > + info = devm_kzalloc(dev, sizeof(*info), GFP_KERNEL); > + if (!info) > + return -ENOMEM; > + > + info->client = client; > + i2c_set_clientdata(client, info); > + > + ret = rt9455_discover_charger(info, &ichrg, &ieoc_percentage, > + &voreg, &vmivr, &iaicr); > + if (ret) { > + dev_err(dev, "Failed to discover charger\n"); > + return ret; > + } > + > +#if IS_ENABLED(CONFIG_USB_PHY) > + info->usb_phy = usb_get_phy(USB_PHY_TYPE_USB2); > + if (IS_ERR_OR_NULL(info->usb_phy)) { > + dev_err(dev, "Failed to get USB transceiver\n"); > + } else { > + info->nb.notifier_call = rt9455_usb_event; > + ret = usb_register_notifier(info->usb_phy, &info->nb); > + if (ret) { > + dev_err(dev, "Failed to register USB notifier\n"); > + usb_put_phy(info->usb_phy); > + } > + } > +#endif > + > + INIT_DELAYED_WORK(&info->pwr_rdy_work, rt9455_pwr_rdy_work_callback); > + INIT_DELAYED_WORK(&info->max_charging_time_work, > + rt9455_max_charging_time_work_callback); > + INIT_DELAYED_WORK(&info->batt_presence_work, > + rt9455_batt_presence_work_callback); Maybe any of these could be made as DEFERRABLE_WORK? Just thinking out loud... I cannot use DEFERRABLE_WORK instead of DELAYED_WORK. DEFERRABLE_WORK runs in interrupt context, while DELAYED_WORK runs in process context. All handlers for DELAYED_WORK from the driver perform read/write operations via I2C, which may sleep. This is the reason why DELAYED_WORK is used. > + > + rt9455_charger_config.of_node = dev->of_node; > + rt9455_charger_config.drv_data = info; > + rt9455_charger_config.supplied_to = rt9455_charger_supplied_to; > + rt9455_charger_config.num_supplicants = > + ARRAY_SIZE(rt9455_charger_supplied_to); > + ret = devm_request_threaded_irq(dev, client->irq, NULL, > + rt9455_irq_handler_thread, > + IRQF_TRIGGER_LOW | IRQF_ONESHOT, > + RT9455_DRIVER_NAME, info); > + if (ret < 0) { > + dev_err(dev, "Failed to register IRQ handler\n"); > + return ret; goto put_usb_phy; > + } > + > + ret = rt9455_hw_init(info, ichrg, ieoc_percentage, voreg, vmivr, iaicr); > + if (ret) { > + dev_err(dev, "Failed to set charger to its default values\n"); > + return ret; Ditto. > + } > + > + info->charger = power_supply_register(dev, &rt9455_charger_desc, > + &rt9455_charger_config); > + if (IS_ERR_OR_NULL(info->charger)) { I think here it cannot be NULL. IS_ERR() is sufficient. i checked it and you are right. Will do. > + dev_err(dev, "Failed to register charger\n"); > + ret = PTR_ERR(info->charger); > + goto put_usb_phy; > + } > + > + return 0; > + > +put_usb_phy: > +#if IS_ENABLED(CONFIG_USB_PHY) > + if (!IS_ERR_OR_NULL(info->usb_phy)) { > + usb_unregister_notifier(info->usb_phy, &info->nb); > + usb_put_phy(info->usb_phy); > + } > +#endif > + return ret; > +} > + > +static int rt9455_remove(struct i2c_client *client) > +{ > + int ret; > + struct rt9455_info *info = i2c_get_clientdata(client); > + > + ret = rt9455_register_reset(info); > + if (ret) > + dev_err(&info->client->dev, "Failed to set charger to its default values\n"); > + > +#if IS_ENABLED(CONFIG_USB_PHY) > + if (!IS_ERR_OR_NULL(info->usb_phy)) { > + usb_unregister_notifier(info->usb_phy, &info->nb); > + usb_put_phy(info->usb_phy); > + } > +#endif > + > + cancel_delayed_work_sync(&info->pwr_rdy_work); > + cancel_delayed_work_sync(&info->max_charging_time_work); > + cancel_delayed_work_sync(&info->batt_presence_work); > + > + power_supply_unregister(info->charger); > + > + return ret; > +} > + > +static const struct i2c_device_id rt9455_i2c_id_table[] = { > + { RT9455_DRIVER_NAME, 0 }, > + { }, > +}; > +MODULE_DEVICE_TABLE(i2c, rt9455_i2c_id_table); > + > +static const struct of_device_id rt9455_of_match[] = { > + { .compatible = "rt,rt9455", }, > + { }, > +}; > +MODULE_DEVICE_TABLE(of, rt9455_of_match); > + > +static const struct acpi_device_id rt9455_i2c_acpi_match[] = { > + {"RTK9455", 0}, To be consistent: spaces after/before brackets. Will do. > + {} > +}; > +MODULE_DEVICE_TABLE(acpi, rt9455_i2c_acpi_match); > + > +static struct i2c_driver rt9455_driver = { > + .probe = rt9455_probe, > + .remove = rt9455_remove, > + .id_table = rt9455_i2c_id_table, > + .driver = { > + .name = RT9455_DRIVER_NAME, > + .owner = THIS_MODULE, Owner is now set by core and such initialization was removed from drivers. Will remove it. Best regards, Krzysztof -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
