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 <redacted>
---

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

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