Re: [PATCH] power_supply: Add TI BQ24257 charger driver

[Laurentiu Palcu]

On Mon, Apr 13, 2015 at 10:11:57PM +0900, Krzysztof Kozlowski wrote:
> W dniu 10.04.2015 o 22:18, Laurentiu Palcu pisze:
> >Based on the datasheet found here:
> >http://www.ti.com/lit/ds/symlink/bq24257.pdf
> >
> >Signed-off-by: Laurentiu Palcu <laurentiu.palcu@xxxxxxxxx>
> 
> (...)
> 
> >diff --git a/drivers/power/bq24257_charger.c b/drivers/power/bq24257_charger.c
> >new file mode 100644
> >index 0000000..58b9af1
> >--- /dev/null
> >+++ b/drivers/power/bq24257_charger.c
> >@@ -0,0 +1,868 @@
> >+/*
> >+ * TI BQ24257 charger driver
> >+ *
> >+ * Copyright (C) 2015 Intel Corporation
> >+ *
> >+ * This program is free software; you can redistribute it and/or modify
> >+ * it under the terms of the GNU General Public License as published by
> >+ * the Free Software Foundation; either version 2 of the License, or
> >+ * (at your option) any later version.
> >+ *
> >+ * This program is distributed in the hope that it will be useful,
> >+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
> >+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> >+ * GNU General Public License for more details.
> >+ *
> >+ */
> >+
> >+#include <linux/module.h>
> >+#include <linux/i2c.h>
> >+#include <linux/power_supply.h>
> >+#include <linux/regmap.h>
> >+#include <linux/types.h>
> >+#include <linux/gpio/consumer.h>
> >+#include <linux/interrupt.h>
> >+#include <linux/delay.h>
> >+
> >+#include <linux/acpi.h>
> >+#include <linux/of.h>
> >+
> >+#define BQ24257_REG_1			0x00
> >+#define BQ24257_REG_2			0x01
> >+#define BQ24257_REG_3			0x02
> >+#define BQ24257_REG_4			0x03
> >+#define BQ24257_REG_5			0x04
> >+#define BQ24257_REG_6			0x05
> >+#define BQ24257_REG_7			0x06
> >+
> >+#define BQ24257_MANUFACTURER		"Texas Instruments"
> >+#define BQ24257_STAT_IRQ		"stat"
> >+#define BQ24257_PG_GPIO			"pg"
> >+
> >+#define BQ24257_ILIM_SET_DELAY		1000	/* msec */
> >+
> >+enum bq24257_fields {
> >+	F_WD_FAULT, F_WD_EN, F_STAT, F_FAULT,			    /* REG 1 */
> >+	F_RESET, F_IILIMIT, F_EN_STAT, F_EN_TERM, F_CE, F_HZ_MODE,  /* REG 2 */
> >+	F_VBAT, F_USB_DET,					    /* REG 3 */
> >+	F_ICHG, F_ITERM,					    /* REG 4 */
> >+	F_LOOP_STATUS, F_LOW_CHG, F_DPDM_EN, F_CE_STATUS, F_VINDPM, /* REG 5 */
> >+	F_X2_TMR_EN, F_TMR, F_SYSOFF, F_TS_STAT,		    /* REG 6 */
> >+	F_VOVP, F_CLR_VDP, F_FORCE_BATDET, F_FORCE_PTM,		    /* REG 7 */
> >+
> >+	F_MAX_FIELDS
> >+};
> >+
> >+/* initial field values, converted from uV/uA */
> >+struct bq24257_init_data {
> >+	u8 ichg;	/* charge current      */
> >+	u8 vbat;	/* regulation voltage  */
> >+	u8 iterm;	/* termination current */
> >+};
> >+
> >+struct bq24257_state {
> >+	u8 status;
> >+	u8 fault;
> >+	bool power_good;
> >+};
> >+
> >+struct bq24257_device {
> >+	struct i2c_client *client;
> >+	struct device *dev;
> >+	struct power_supply *charger;
> >+	struct power_supply_desc charger_desc;
> >+
> >+	struct regmap *rmap;
> >+	struct regmap_field *rmap_fields[F_MAX_FIELDS];
> >+
> >+	struct gpio_desc *pg;
> >+
> >+	struct delayed_work iilimit_setup_work;
> >+
> >+	struct bq24257_init_data init_data;
> >+	struct bq24257_state state;
> >+
> >+	struct mutex lock; /* protect state data */
> >+};
> >+
> >+static bool bq24257_is_writeable_reg(struct device *dev, unsigned int reg)
> >+{
> >+	return true;
> 
> What is the benefit of such function always returning true?
Apparently, none. :) regmap_writeable() returns true, by default, anyway.
Thanks for spotting it.

> 
> (...)
> 
> >+
> >+static enum power_supply_property bq24257_power_supply_props[] = {
> >+	POWER_SUPPLY_PROP_MANUFACTURER,
> >+	POWER_SUPPLY_PROP_STATUS,
> >+	POWER_SUPPLY_PROP_ONLINE,
> >+	POWER_SUPPLY_PROP_HEALTH,
> >+	POWER_SUPPLY_PROP_CONSTANT_CHARGE_CURRENT,
> >+	POWER_SUPPLY_PROP_CONSTANT_CHARGE_CURRENT_MAX,
> >+	POWER_SUPPLY_PROP_CONSTANT_CHARGE_VOLTAGE,
> >+	POWER_SUPPLY_PROP_CONSTANT_CHARGE_VOLTAGE_MAX,
> >+	POWER_SUPPLY_PROP_CHARGE_TERM_CURRENT,
> >+};
> >+
> >+static char *bq24257_charger_supplied_to[] = {
> >+	"main-battery",
> >+};
> >+
> >+static int bq24257_power_supply_init(struct bq24257_device *bq)
> >+{
> >+	struct power_supply_config psy_cfg = { .drv_data = bq, };
> >+	struct power_supply_desc *psy_desc = &bq->charger_desc;
> >+
> >+	psy_desc->name = "bq24257-charger";
> >+	psy_desc->type = POWER_SUPPLY_TYPE_USB;
> >+	psy_desc->properties = bq24257_power_supply_props;
> >+	psy_desc->num_properties = ARRAY_SIZE(bq24257_power_supply_props);
> >+	psy_desc->get_property = bq24257_power_supply_get_property;
> 
> You are not modifying the power_supply_desc so it can be made a
> static const variable (file scope).
Fair enough. Will change in v2. I'll just give it a couple more days for
others to review.

laurentiu
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/