Message-ID: <20150524110203.GA4188@earth>
Date:	Sun, 24 May 2015 13:02:03 +0200
From:	Sebastian Reichel <sre@...nel.org>
To:	Anda-Maria Nicolae <anda-maria.nicolae@...el.com>
Cc:	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 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@...el.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

Download attachment "signature.asc" of type "application/pgp-signature" (820 bytes)

Powered by blists - more mailing lists