lkml.org 
[lkml]   [2015]   [May]   [24]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH v5] power_supply: Add support for Richtek rt9455 battery charger
    Hi Anda-Maria,

    On Fri, May 08, 2015 at 03:57:49PM +0300, Anda-Maria Nicolae wrote:
    > Based on the datasheet found here:
    > http://www.richtek.com/download_ds.jsp?p=RT9455
    >
    > Signed-off-by: Anda-Maria Nicolae <anda-maria.nicolae@intel.com>
    > ---
    >
    > Updates from v4 version:
    > - replaced depends on REGMAP_I2C with select REGMAP_I2C
    > - got the latest version of vendor_prefixes file; prev patch contained an older one
    > - added explanation in rt9455_charger.txt about what boost-output-voltage represents
    >
    > .../devicetree/bindings/power/rt9455_charger.txt | 46 +

    Please move this into its own patch, see

    Documentation/devicetree/bindings/submitting-patches.txt

    > .../devicetree/bindings/vendor-prefixes.txt | 1 +

    Please also move this into its own patch

    > drivers/power/Kconfig | 7 +
    > drivers/power/Makefile | 1 +
    > drivers/power/rt9455_charger.c | 1821 ++++++++++++++++++++
    > 5 files changed, 1876 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..a87c0f5
    > --- /dev/null
    > +++ b/Documentation/devicetree/bindings/power/rt9455_charger.txt
    > @@ -0,0 +1,46 @@
    > +Binding for Richtek rt9455 battery charger
    > +
    > +Required properties:
    > +- compatible: Should contain one of the following:
    > + * "richtek,rt9455"
    > +
    > +- reg: integer, i2c address of the device.
    > +- richtek,output-charge-current: integer, output current from the charger to the
    > + battery, in uA.
    > +- richtek,end-of-charge-percentage: integer, percent of the output charge current.
    > + When the current in constant-voltage phase drops
    > + below output_charge_current x end-of-charge-percentage,
    > + charge is terminated.
    > +- richtek,battery-regulation-voltage: integer, maximum battery voltage in uV.
    > +- richtek,boost-output-voltage: integer, maximum voltage provided to consumer
    > + devices, when the charger is in boost mode.
    > +
    > +Optional properties:
    > +- richtek,min-input-voltage-regulation: integer, input voltage level in uA, used to
    > + decrease voltage level when the over current
    > + of the input power source occurs.
    > + This prevents input voltage drop due to insufficient
    > + current provided by the power source.
    > +- richtek,avg-input-current-regulation: integer, input current value drained by the
    > + charger from the power source.
    > +
    > +Example:
    > +
    > +rt9455@22 {
    > + compatible = "richtek,rt9455";
    > + reg = <0x22>;
    > +
    > + interrupt-parent = <&gpio1>;
    > + interrupts = <0 1>;
    > +
    > + interrupt-gpio = <&gpio1 0 1>;
    > + reset-gpio = <&gpio1 1 1>;

    Neither the gpios, nor the interrupts are specified as
    required/optional properties. Also the reset gpio is
    not referenced in the sourcecode at all.

    > + richtek,output-charge-current = <500000>;
    > + richtek,end-of-charge-percentage = <10>;
    > + richtek,battery-regulation-voltage = <4200000>;
    > + richtek,boost-output-voltage = <5050000>;
    > +
    > + richtek,min-input-voltage-regulation = <4500000>;
    > + richtek,avg-input-current-regulation = <500000>;
    > +};
    >
    > [...]
    >
    > +/*
    > + * Before setting the charger into boost mode, boost output voltage is
    > + * set. This is needed because boost output voltage may differ from battery
    > + * regulation voltage. F_VOREG bits represent either battery regulation voltage
    > + * or boost output voltage, depending on the mode the charger is. Both battery
    > + * regulation voltage and boost output voltage are read from DT/ACPI during
    > + * probe.
    > + */
    > +static int rt9455_set_boost_voltage_before_boost_mode(struct rt9455_info *info)
    > +{
    > + struct device *dev = &info->client->dev;
    > + int curr_boost_voltage, ret;
    > +
    > + ret = rt9455_get_field_val(info, F_VOREG,
    > + rt9455_boost_voltage_values,
    > + ARRAY_SIZE(rt9455_boost_voltage_values),
    > + &curr_boost_voltage);
    > + if (ret) {
    > + dev_err(dev, "Failed to read boost output voltage value\n");
    > + return ret;
    > + }
    > +
    > + if (curr_boost_voltage != info->boost_voltage) {
    > + ret = rt9455_set_field_val(info, F_VOREG,
    > + rt9455_boost_voltage_values,
    > + ARRAY_SIZE(rt9455_boost_voltage_values),
    > + info->boost_voltage);
    > + if (ret) {
    > + dev_err(dev, "Failed to set boost output voltage value\n");
    > + return ret;
    > + }
    > + }
    > +
    > + return 0;
    > +}

    The register comparision looks over-engineered to me.
    Just write the new value into the register.

    > +/*
    > + * Before setting the charger into charge mode, battery regulation voltage is
    > + * set. This is needed because boost output voltage may differ from battery
    > + * regulation voltage. F_VOREG bits represent either battery regulation voltage
    > + * or boost output voltage, depending on the mode the charger is. Both battery
    > + * regulation voltage and boost output voltage are read from DT/ACPI during
    > + * probe.
    > + */
    > +static int rt9455_set_voreg_before_charge_mode(struct rt9455_info *info)
    > +{
    > + struct device *dev = &info->client->dev;
    > + int curr_voreg, ret;
    > +
    > + ret = rt9455_get_field_val(info, F_VOREG,
    > + rt9455_voreg_values,
    > + ARRAY_SIZE(rt9455_voreg_values),
    > + &curr_voreg);
    > + if (ret) {
    > + dev_err(dev, "Failed to read VOREG value\n");
    > + return ret;
    > + }
    > +
    > + if (curr_voreg != info->voreg) {
    > + ret = rt9455_set_field_val(info, F_VOREG,
    > + rt9455_voreg_values,
    > + ARRAY_SIZE(rt9455_voreg_values),
    > + info->voreg);
    > + if (ret) {
    > + dev_err(dev, "Failed to set VOREG value\n");
    > + return ret;
    > + }
    > + }
    > +
    > + return 0;
    > +}

    see last comment

    > [...]
    >
    > +static int rt9455_setup_irq(struct rt9455_info *info)
    > +{
    > + struct device *dev = &info->client->dev;
    > + int ret;
    > +
    > + /* Obtain IRQ GPIO */
    > + info->gpiod_irq = devm_gpiod_get_index(dev, RT9455_IRQ_NAME, 0);
    > + if (IS_ERR(info->gpiod_irq)) {
    > + dev_err(dev, "Failed to get IRQ GPIO\n");
    > + return PTR_ERR(info->gpiod_irq);
    > + }
    > +
    > + /* Configure IRQ GPIO */
    > + ret = gpiod_direction_input(info->gpiod_irq);
    > + if (ret) {
    > + dev_err(dev, "Failed to set IRQ GPIO direction err = %d\n",
    > + ret);
    > + return ret;
    > + }
    > +
    > + /* Map the pin to an IRQ */
    > + ret = gpiod_to_irq(info->gpiod_irq);
    > + if (ret < 0) {
    > + dev_err(dev, "Failed to associate GPIO with an IRQ err = %d\n",
    > + ret);
    > + return ret;
    > + }
    > +
    > + dev_dbg(dev, "GPIO resource, no:%d irq:%d\n",
    > + desc_to_gpio(info->gpiod_irq), ret);
    > + info->client->irq = ret;
    > +
    > + info->gpio_irq = desc_to_gpio(info->gpiod_irq);

    info->gpio_irq is never referenced. Drop it.

    > + return 0;
    > +}

    There is no usage of info->gpiod_irq except for irq conversion, so you
    can just drop the whole gpio usage and reference the gpio as irq in the
    DT specification:

    interrupts-parent = <&gpio42>;
    interrupts = <23 IRQ_TYPE_LEVEL_HIGH>;

    This way the irq will be assigned to client->irq automatically. For
    ACPI the same will be supported once this patchset is merged:

    https://lkml.org/lkml/2015/4/28/455

    > [...]
    >
    > +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. Also, VOREG and boost output
    > + * voltage are mandatory values, but they are stored in rt9455_info
    > + * structure.
    > + */
    > + u32 ichrg, ieoc_percentage;
    > + /* Optional device-specific data values. */
    > + u32 mivr = -1, iaicr = -1;
    > + int i, 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);
    > +
    > + info->regmap = devm_regmap_init_i2c(client,
    > + &rt9455_regmap_config);
    > + if (IS_ERR(info->regmap)) {
    > + dev_err(dev, "Failed to initialize register map\n");
    > + return -EINVAL;
    > + }
    > +
    > + for (i = 0; i < F_MAX_FIELDS; i++) {
    > + info->regmap_fields[i] =
    > + devm_regmap_field_alloc(dev, info->regmap,
    > + rt9455_reg_fields[i]);
    > + if (IS_ERR(info->regmap_fields[i])) {
    > + dev_err(dev,
    > + "Failed to allocate regmap field = %d\n", i);
    > + return PTR_ERR(info->regmap_fields[i]);
    > + }
    > + }
    > +
    > + ret = rt9455_discover_charger(info, &ichrg, &ieoc_percentage,
    > + &mivr, &iaicr);
    > + if (ret) {
    > + dev_err(dev, "Failed to discover charger\n");
    > + return ret;
    > + }
    > +
    > +#if IS_ENABLED(CONFIG_USB_PHY)
    > + info->usb_phy = devm_usb_get_phy(dev, USB_PHY_TYPE_USB2);
    > + if (IS_ERR(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");
    > + /*
    > + * If usb_register_notifier() fails, set notifier_call
    > + * to NULL, to avoid calling usb_unregister_notifier().
    > + */
    > + info->nb.notifier_call = NULL;
    > + }
    > + }
    > +#endif
    > +
    > + INIT_DEFERRABLE_WORK(&info->pwr_rdy_work, rt9455_pwr_rdy_work_callback);
    > + INIT_DEFERRABLE_WORK(&info->max_charging_time_work,
    > + rt9455_max_charging_time_work_callback);
    > + INIT_DEFERRABLE_WORK(&info->batt_presence_work,
    > + rt9455_batt_presence_work_callback);
    > +
    > + 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) {
    > + dev_err(dev, "Failed to register IRQ handler\n");
    > + goto put_usb_notifier;
    > + }
    > +
    > + ret = rt9455_hw_init(info, ichrg, ieoc_percentage, mivr, iaicr);
    > + if (ret) {
    > + dev_err(dev, "Failed to set charger to its default values\n");
    > + goto put_usb_notifier;
    > + }
    > +
    > + info->charger = power_supply_register(dev, &rt9455_charger_desc,
    > + &rt9455_charger_config);
    > + if (IS_ERR(info->charger)) {
    > + dev_err(dev, "Failed to register charger\n");
    > + ret = PTR_ERR(info->charger);
    > + goto put_usb_notifier;
    > + }

    You can use devm_power_supply_register() and remove the
    power_supply_unregister() call from rt9455_remove().

    > +
    > + return 0;
    > +
    > +put_usb_notifier:
    > +#if IS_ENABLED(CONFIG_USB_PHY)
    > + if (info->nb.notifier_call) {
    > + usb_unregister_notifier(info->usb_phy, &info->nb);
    > + info->nb.notifier_call = NULL;
    > + }
    > +#endif
    > + return ret;
    > +}
    >
    > [...]

    -- Sebastian
    [unhandled content-type:application/pgp-signature]
    \
     
     \ /
      Last update: 2015-05-24 13:21    [W:3.664 / U:0.012 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site