| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <554A3B18.7050401@intel.com>
Date: Wed, 06 May 2015 19:02:32 +0300
From: Anda-Maria Nicolae <anda-maria.nicolae@...el.com>
To: Krzysztof Kozłowski <k.kozlowski.k@...il.com>
CC: sre@...nel.org, dbaryshkov@...il.com, robh+dt@...nel.org,
pawel.moll@....com, mark.rutland@....com,
ijc+devicetree@...lion.org.uk, galak@...eaurora.org,
dwmw2@...radead.org, linux-pm@...r.kernel.org,
linux-kernel@...r.kernel.org, devicetree@...r.kernel.org
Subject: Re: [PATCH v3] power_supply: Add support for Richtek rt9455 battery
charger
On 05/06/2015 10:58 AM, Krzysztof Kozłowski wrote:
> 2015-05-06 1:32 GMT+09:00 Anda-Maria Nicolae <anda-maria.nicolae@...el.com>:
>> Based on the datasheet found here:
>> http://www.richtek.com/download_ds.jsp?p=RT9455
>>
>> Signed-off-by: Anda-Maria Nicolae <anda-maria.nicolae@...el.com>
>> ---
>>
>> Updates from v2 version:
>> - removed unused masks and keep the used ones. I have tried to access mask field
>> from struct regmap_field, but I have received the following compilation error:
>> "dereferencing pointer to incomplete type". I think this happens because I include
>> the file include/linux/regmap.h and in this file struct regmap_field is only declared
>> and not defined. Struct regmap_field is defined in drivers/base/regmap/internal.h.
>> If I include this file, compilation works. But I do not think it is a good idea
>> to include it; I did not find any other driver which includes this file. I also
>> could not find any driver that accesses mask field from struct regmap_field.
>> For instance, drivers/pwm/pwm-sti.c at lines 157, 158, also uses masks.
>>
>> - I have also kept REG_FIELD definitions for interrupt registers, since, for instance,
>> I use F_BATAB to check battery presence property.
>>
>> - cached regs 0x05 and 0x06. Also, I have added rt9455_is_writeable_reg function
>> for writeable_reg field of regmap_config.
>>
>> - used POWER_STATUS_DISCHARGING, replaced break with and replaced it with return and
>> indented it correctly. I spent some time on this and I finally understood what is happening.
>> So, if PWR_RDY bit is set, but STAT bits value is 0, the charger may be in one
>> of the following cases:
>> 1. CHG_EN bit is 0.
>> 2. CHG_EN bit is 1 but the battery is not connected.
>> In any of these cases, POWER_SUPPLY_STATUS_NOT_CHARGING is returned.
>> If the PWR_RDY bit is cleared, POWER_SUPPLY_STATUS_DISCHARGING is returned.
>>
>> - used VOREG bits value instead of VMREG in functions rt9455_charger_get_voltage_max()/
>> rt9455_charger_set_voltage_max(). Although RT9455 charger has VMREG bits which,
>> according to the datasheet, represent "Maximum battery regulation voltage", the
>> charger never uses VMREG value as maximum threshold for battery voltage. This
>> happens because TE and TE_SHDN_EN bits are set during rt9455_probe(), and charging
>> operation is terminated when charging current is less than ICHRG x IEOC. When charging
>> operation is terminated, battery voltage is almost equal to VOREG. Therefore,
>> VMREG value is not taken into account. This is the reason why VOREG value is
>> set/returned in these functions.
>>
>> - corrected comment from rt9455_usb_event_id() function.
>>
>> - replaced IS_ERR_OR_NULL() with IS_ERR() to check the result of usb_get_phy()
>> function. Also, if usb_register_notifier() fails, since usb_put_phy() is immediately
>> called, I have set info->usb_phy to ERR_PTR(-ENODEV) so that in rt9455_remove()
>> usb_put_phy is not mistakenly called again.
>>
>> .../devicetree/bindings/power/rt9455_charger.txt | 43 +
>> .../devicetree/bindings/vendor-prefixes.txt | 1 +
>> drivers/power/Kconfig | 6 +
>> drivers/power/Makefile | 1 +
>> drivers/power/rt9455_charger.c | 1801 ++++++++++++++++++++
>> 5 files changed, 1852 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..7e8aed6
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/power/rt9455_charger.txt
>> @@ -0,0 +1,43 @@
>> +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.
>> +
>> +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>;
>> +
>> + richtek,output-charge-current = <500000>;
>> + richtek,end-of-charge-percentage = <10>;
>> + richtek,battery-regulation-voltage = <4200000>;
>> +
>> + richtek,min-input-voltage-regulation = <4500000>;
>> + richtek,avg-input-current-regulation = <500000>;
>> +};
>> diff --git a/Documentation/devicetree/bindings/vendor-prefixes.txt b/Documentation/devicetree/bindings/vendor-prefixes.txt
>> index 8033919..7b8c129 100644
>> --- a/Documentation/devicetree/bindings/vendor-prefixes.txt
>> +++ b/Documentation/devicetree/bindings/vendor-prefixes.txt
>> @@ -161,6 +161,7 @@ ralink Mediatek/Ralink Technology Corp.
>> ramtron Ramtron International
>> realtek Realtek Semiconductor Corp.
>> renesas Renesas Electronics Corporation
>> +richtek Richtek Technology Corporation
>> ricoh Ricoh Co. Ltd.
>> rockchip Fuzhou Rockchip Electronics Co., Ltd
>> samsung Samsung Semiconductor
>> diff --git a/drivers/power/Kconfig b/drivers/power/Kconfig
>> index 4091fb0..39f208d 100644
>> --- a/drivers/power/Kconfig
>> +++ b/drivers/power/Kconfig
>> @@ -439,6 +439,12 @@ config BATTERY_RT5033
>> The fuelgauge calculates and determines the battery state of charge
>> according to battery open circuit voltage.
>>
>> +config CHARGER_RT9455
>> + tristate "Richtek RT9455 battery charger driver"
>> + depends on I2C && GPIOLIB
> Laurentiu Palcu pointed already the need of REGMAP_I2C dependency.
> Actually you need to select it.
Will do it.
>
> (...)
>
>> +
>> +static int rt9455_charger_property_is_writeable(struct power_supply *psy,
>> + enum power_supply_property psp)
>> +{
>> + switch (psp) {
>> + case POWER_SUPPLY_PROP_CONSTANT_CHARGE_CURRENT:
>> + case POWER_SUPPLY_PROP_CONSTANT_CHARGE_VOLTAGE:
>> + case POWER_SUPPLY_PROP_CONSTANT_CHARGE_VOLTAGE_MAX:
>> + return 1;
>> + default:
>> + return 0;
>> + }
>> +}
> And comments from previous email? Which "numerous charger drivers" expose these
> properties as writable?
>
Yesterday I run: POWER_SUPPLY_PROP_CONSTANT_CHARGE_CURRENT and drivers
were shown. What I did not do was to look in those drivers to see if
indeed the property is writeable, or if it is readable.
I agree with removing them from the driver. I've spent some time to
understand them and this is why I did not agree with this in the beginning.
> (...)
>
>> +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 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,
>> + &voreg, &mivr, &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(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);
>> + /*
>> + * Since usb_put_phy() has been called, info->usb_phy is
>> + * set to ERR_PTR so that in rt9455_remove()
>> + * usb_put_phy() is not mistakenly called again.
>> + */
>> + info->usb_phy = ERR_PTR(-ENODEV);
>> + }
>> + }
>> +#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");
> I pointed this 2 times, so maybe third time lucky? I think we
> misunderstood each other. Why are you not cleaning the usb_phy? If
> this fails then you should goto put_usb_phy because now it leaks.
Ok, I understand.
>
>> + return ret;
>> + }
>> +
>> + ret = rt9455_hw_init(info, ichrg, ieoc_percentage, voreg, mivr, iaicr);
>> + if (ret) {
>> + dev_err(dev, "Failed to set charger to its default values\n");
>> + return ret;
> Ditto.
Ok, I understand.
> Best regards,
> Krzysztof
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists