Re: [PATCH v5] power_supply: Add support for Richtek rt9455 battery charger

From: Sebastian Reichel
Date: Sun May 24 2015 - 07:02:39 EST

Next message: Pali RohÃr: "Re: [PATCH v3 2/3] dell-rbtn: Export notifier for other kernel modules"
Previous message: Petko Manolov: "Re: [PATCH 0/8] MODSIGN: Use PKCS#7 for module signatures [ver #4]"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

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@xxxxxxxxx>
> ---
>
> 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

Attachment: signature.asc
Description: Digital signature

Next message: Pali RohÃr: "Re: [PATCH v3 2/3] dell-rbtn: Export notifier for other kernel modules"
Previous message: Petko Manolov: "Re: [PATCH 0/8] MODSIGN: Use PKCS#7 for module signatures [ver #4]"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]