* [PATCH 3/4] power: supply: axp288_fuel_gauge: Read 12 bit values 2 registers at a time
From: Hans de Goede @ 2016-12-14 16:38 UTC (permalink / raw)
To: Sebastian Reichel, Chen-Yu Tsai
Cc: russianneuromancer @ ya . ru, linux-pm, linux-i2c, Hans de Goede
In-Reply-To: <20161214163853.454-1-hdegoede@redhat.com>
In order for the MSB -> LSB latching to work correctly we must read the
2 8 bit registers of a 12 bit value in one consecutive read.
This fixes voltage_ocv reporting inconsistent values on my tablet.
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
drivers/power/supply/axp288_fuel_gauge.c | 40 ++++++++++++++++++--------------
1 file changed, 22 insertions(+), 18 deletions(-)
diff --git a/drivers/power/supply/axp288_fuel_gauge.c b/drivers/power/supply/axp288_fuel_gauge.c
index 99d6d30..089056c 100644
--- a/drivers/power/supply/axp288_fuel_gauge.c
+++ b/drivers/power/supply/axp288_fuel_gauge.c
@@ -210,6 +210,22 @@ static int fuel_gauge_read_15bit_word(struct axp288_fg_info *info, int reg)
return ret & FG_15BIT_VAL_MASK;
}
+static int fuel_gauge_read_12bit_word(struct axp288_fg_info *info, int reg)
+{
+ unsigned char buf[2];
+ int ret;
+
+ ret = regmap_bulk_read(info->regmap, reg, buf, 2);
+ if (ret < 0) {
+ dev_err(&info->pdev->dev, "Error reading reg 0x%02x err: %d\n",
+ reg, ret);
+ return ret;
+ }
+
+ /* 12-bit data values have upper 8 bits in buf[0], lower 4 in buf[1] */
+ return (buf[0] << 4) | ((buf[1] >> 4) & 0x0f);
+}
+
static int pmic_read_adc_val(const char *name, int *raw_val,
struct axp288_fg_info *info)
{
@@ -270,12 +286,9 @@ static int fuel_gauge_debug_show(struct seq_file *s, void *data)
seq_printf(s, " FG_RDC0[%02x] : %02x\n",
AXP288_FG_RDC0_REG,
fuel_gauge_reg_readb(info, AXP288_FG_RDC0_REG));
- seq_printf(s, " FG_OCVH[%02x] : %02x\n",
+ seq_printf(s, " FG_OCV[%02x] : %04x\n",
AXP288_FG_OCVH_REG,
- fuel_gauge_reg_readb(info, AXP288_FG_OCVH_REG));
- seq_printf(s, " FG_OCVL[%02x] : %02x\n",
- AXP288_FG_OCVL_REG,
- fuel_gauge_reg_readb(info, AXP288_FG_OCVL_REG));
+ fuel_gauge_read_12bit_word(info, AXP288_FG_OCVH_REG));
seq_printf(s, " FG_DES_CAP[%02x] : %04x\n",
AXP288_FG_DES_CAP1_REG,
fuel_gauge_read_15bit_word(info, AXP288_FG_DES_CAP1_REG));
@@ -532,21 +545,12 @@ static int fuel_gauge_get_btemp(struct axp288_fg_info *info, int *btemp)
static int fuel_gauge_get_vocv(struct axp288_fg_info *info, int *vocv)
{
- int ret, value;
-
- /* 12-bit data value, upper 8 in OCVH, lower 4 in OCVL */
- ret = fuel_gauge_reg_readb(info, AXP288_FG_OCVH_REG);
- if (ret < 0)
- goto vocv_read_fail;
- value = ret << 4;
+ int ret;
- ret = fuel_gauge_reg_readb(info, AXP288_FG_OCVL_REG);
- if (ret < 0)
- goto vocv_read_fail;
- value |= (ret & 0xf);
+ ret = fuel_gauge_read_12bit_word(info, AXP288_FG_OCVH_REG);
+ if (ret >= 0)
+ *vocv = VOLTAGE_FROM_ADC(ret);
- *vocv = VOLTAGE_FROM_ADC(value);
-vocv_read_fail:
return ret;
}
--
2.9.3
^ permalink raw reply related
* [PATCH 1/4] power: supply: axp288_fuel_gauge: Fix fuel_gauge_reg_readb return on error
From: Hans de Goede @ 2016-12-14 16:38 UTC (permalink / raw)
To: Sebastian Reichel, Chen-Yu Tsai
Cc: russianneuromancer @ ya . ru, linux-pm, linux-i2c, Hans de Goede
If reading the register fails, return the actual error code, instead
of the uninitialized val variable;
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
drivers/power/supply/axp288_fuel_gauge.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/power/supply/axp288_fuel_gauge.c b/drivers/power/supply/axp288_fuel_gauge.c
index 5bdde69..f62f9df 100644
--- a/drivers/power/supply/axp288_fuel_gauge.c
+++ b/drivers/power/supply/axp288_fuel_gauge.c
@@ -169,8 +169,10 @@ static int fuel_gauge_reg_readb(struct axp288_fg_info *info, int reg)
break;
}
- if (ret < 0)
+ if (ret < 0) {
dev_err(&info->pdev->dev, "axp288 reg read err:%d\n", ret);
+ return ret;
+ }
return val;
}
--
2.9.3
^ permalink raw reply related
* [PATCH 4/4] power: supply: axp288_fuel_gauge: Drop platform_data dependency
From: Hans de Goede @ 2016-12-14 16:38 UTC (permalink / raw)
To: Sebastian Reichel, Chen-Yu Tsai
Cc: russianneuromancer @ ya . ru, linux-pm, linux-i2c, Hans de Goede
In-Reply-To: <20161214163853.454-1-hdegoede@redhat.com>
When the axp288_faul_gauge driver was originally merged, it was
merged with a dependency on some other driver providing platform
data for it.
However the battery-data-framework which should provide that data
never got merged, resulting in x86 tablets / laptops with an axp288
having no working battery monitor, as before this commit the driver
would simply return -ENODEV if there is no platform data.
This commit removes the dependency on the platform_data instead
checking that the firmware has initialized the fuel-gauge and
reading the info back from the pmic.
What is missing from the read-back info is the table to map raw adc
values to temperature, so this commit drops the temperature and
temperature limits properties. The min voltage, charge design and
model name info is also missing. Note that none of these are really
important for userspace to have.
All other functionality is preserved and actually made available
by this commit.
BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=88471
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
drivers/power/supply/axp288_fuel_gauge.c | 405 +++----------------------------
include/linux/mfd/axp20x.h | 22 --
2 files changed, 33 insertions(+), 394 deletions(-)
diff --git a/drivers/power/supply/axp288_fuel_gauge.c b/drivers/power/supply/axp288_fuel_gauge.c
index 089056c..1c25443 100644
--- a/drivers/power/supply/axp288_fuel_gauge.c
+++ b/drivers/power/supply/axp288_fuel_gauge.c
@@ -50,11 +50,6 @@
#define CHRG_CCCV_CV_4350MV 0x3 /* 4.35V */
#define CHRG_CCCV_CHG_EN (1 << 7)
-#define CV_4100 4100 /* 4100mV */
-#define CV_4150 4150 /* 4150mV */
-#define CV_4200 4200 /* 4200mV */
-#define CV_4350 4350 /* 4350mV */
-
#define TEMP_IRQ_CFG_QWBTU (1 << 0)
#define TEMP_IRQ_CFG_WBTU (1 << 1)
#define TEMP_IRQ_CFG_QWBTO (1 << 2)
@@ -103,9 +98,7 @@
/* 1.1mV per LSB expressed in uV */
#define VOLTAGE_FROM_ADC(a) ((a * 11) / 10)
-/* properties converted to tenths of degrees, uV, uA, uW */
-#define PROP_TEMP(a) ((a) * 10)
-#define UNPROP_TEMP(a) ((a) / 10)
+/* properties converted to uV, uA */
#define PROP_VOLT(a) ((a) * 1000)
#define PROP_CURR(a) ((a) * 1000)
@@ -121,13 +114,13 @@ enum {
struct axp288_fg_info {
struct platform_device *pdev;
- struct axp20x_fg_pdata *pdata;
struct regmap *regmap;
struct regmap_irq_chip_data *regmap_irqc;
int irq[AXP288_FG_INTR_NUM];
struct power_supply *bat;
struct mutex lock;
int status;
+ int max_volt;
struct delayed_work status_monitor;
struct dentry *debug_file;
};
@@ -137,22 +130,14 @@ static enum power_supply_property fuel_gauge_props[] = {
POWER_SUPPLY_PROP_PRESENT,
POWER_SUPPLY_PROP_HEALTH,
POWER_SUPPLY_PROP_VOLTAGE_MAX_DESIGN,
- POWER_SUPPLY_PROP_VOLTAGE_MIN_DESIGN,
POWER_SUPPLY_PROP_VOLTAGE_NOW,
POWER_SUPPLY_PROP_VOLTAGE_OCV,
POWER_SUPPLY_PROP_CURRENT_NOW,
POWER_SUPPLY_PROP_CAPACITY,
POWER_SUPPLY_PROP_CAPACITY_ALERT_MIN,
- POWER_SUPPLY_PROP_TEMP,
- POWER_SUPPLY_PROP_TEMP_MAX,
- POWER_SUPPLY_PROP_TEMP_MIN,
- POWER_SUPPLY_PROP_TEMP_ALERT_MIN,
- POWER_SUPPLY_PROP_TEMP_ALERT_MAX,
POWER_SUPPLY_PROP_TECHNOLOGY,
POWER_SUPPLY_PROP_CHARGE_FULL,
POWER_SUPPLY_PROP_CHARGE_NOW,
- POWER_SUPPLY_PROP_CHARGE_FULL_DESIGN,
- POWER_SUPPLY_PROP_MODEL_NAME,
};
static int fuel_gauge_reg_readb(struct axp288_fg_info *info, int reg)
@@ -447,102 +432,6 @@ static int fuel_gauge_get_current(struct axp288_fg_info *info, int *cur)
return ret;
}
-static int temp_to_adc(struct axp288_fg_info *info, int tval)
-{
- int rntc = 0, i, ret, adc_val;
- int rmin, rmax, tmin, tmax;
- int tcsz = info->pdata->tcsz;
-
- /* get the Rntc resitance value for this temp */
- if (tval > info->pdata->thermistor_curve[0][1]) {
- rntc = info->pdata->thermistor_curve[0][0];
- } else if (tval <= info->pdata->thermistor_curve[tcsz-1][1]) {
- rntc = info->pdata->thermistor_curve[tcsz-1][0];
- } else {
- for (i = 1; i < tcsz; i++) {
- if (tval > info->pdata->thermistor_curve[i][1]) {
- rmin = info->pdata->thermistor_curve[i-1][0];
- rmax = info->pdata->thermistor_curve[i][0];
- tmin = info->pdata->thermistor_curve[i-1][1];
- tmax = info->pdata->thermistor_curve[i][1];
- rntc = rmin + ((rmax - rmin) *
- (tval - tmin) / (tmax - tmin));
- break;
- }
- }
- }
-
- /* we need the current to calculate the proper adc voltage */
- ret = fuel_gauge_reg_readb(info, AXP20X_ADC_RATE);
- if (ret < 0) {
- dev_err(&info->pdev->dev, "%s:read err:%d\n", __func__, ret);
- ret = 0x30;
- }
-
- /*
- * temperature is proportional to NTS thermistor resistance
- * ADC_RATE[5-4] determines current, 00=20uA,01=40uA,10=60uA,11=80uA
- * [12-bit ADC VAL] = R_NTC(Ω) * current / 800
- */
- adc_val = rntc * (20 + (20 * ((ret >> 4) & 0x3))) / 800;
-
- return adc_val;
-}
-
-static int adc_to_temp(struct axp288_fg_info *info, int adc_val)
-{
- int ret, r, i, tval = 0;
- int rmin, rmax, tmin, tmax;
- int tcsz = info->pdata->tcsz;
-
- ret = fuel_gauge_reg_readb(info, AXP20X_ADC_RATE);
- if (ret < 0) {
- dev_err(&info->pdev->dev, "%s:read err:%d\n", __func__, ret);
- ret = 0x30;
- }
-
- /*
- * temperature is proportional to NTS thermistor resistance
- * ADC_RATE[5-4] determines current, 00=20uA,01=40uA,10=60uA,11=80uA
- * R_NTC(Ω) = [12-bit ADC VAL] * 800 / current
- */
- r = adc_val * 800 / (20 + (20 * ((ret >> 4) & 0x3)));
-
- if (r < info->pdata->thermistor_curve[0][0]) {
- tval = info->pdata->thermistor_curve[0][1];
- } else if (r >= info->pdata->thermistor_curve[tcsz-1][0]) {
- tval = info->pdata->thermistor_curve[tcsz-1][1];
- } else {
- for (i = 1; i < tcsz; i++) {
- if (r < info->pdata->thermistor_curve[i][0]) {
- rmin = info->pdata->thermistor_curve[i-1][0];
- rmax = info->pdata->thermistor_curve[i][0];
- tmin = info->pdata->thermistor_curve[i-1][1];
- tmax = info->pdata->thermistor_curve[i][1];
- tval = tmin + ((tmax - tmin) *
- (r - rmin) / (rmax - rmin));
- break;
- }
- }
- }
-
- return tval;
-}
-
-static int fuel_gauge_get_btemp(struct axp288_fg_info *info, int *btemp)
-{
- int ret, raw_val = 0;
-
- ret = pmic_read_adc_val("axp288-batt-temp", &raw_val, info);
- if (ret < 0)
- goto temp_read_fail;
-
- *btemp = adc_to_temp(info, raw_val);
-
-temp_read_fail:
- return ret;
-}
-
static int fuel_gauge_get_vocv(struct axp288_fg_info *info, int *vocv)
{
int ret;
@@ -556,25 +445,14 @@ static int fuel_gauge_get_vocv(struct axp288_fg_info *info, int *vocv)
static int fuel_gauge_battery_health(struct axp288_fg_info *info)
{
- int temp, vocv;
- int ret, health = POWER_SUPPLY_HEALTH_UNKNOWN;
-
- ret = fuel_gauge_get_btemp(info, &temp);
- if (ret < 0)
- goto health_read_fail;
+ int ret, vocv, health = POWER_SUPPLY_HEALTH_UNKNOWN;
ret = fuel_gauge_get_vocv(info, &vocv);
if (ret < 0)
goto health_read_fail;
- if (vocv > info->pdata->max_volt)
+ if (vocv > info->max_volt)
health = POWER_SUPPLY_HEALTH_OVERVOLTAGE;
- else if (temp > info->pdata->max_temp)
- health = POWER_SUPPLY_HEALTH_OVERHEAT;
- else if (temp < info->pdata->min_temp)
- health = POWER_SUPPLY_HEALTH_COLD;
- else if (vocv < info->pdata->min_volt)
- health = POWER_SUPPLY_HEALTH_DEAD;
else
health = POWER_SUPPLY_HEALTH_GOOD;
@@ -582,28 +460,6 @@ static int fuel_gauge_battery_health(struct axp288_fg_info *info)
return health;
}
-static int fuel_gauge_set_high_btemp_alert(struct axp288_fg_info *info)
-{
- int ret, adc_val;
-
- /* program temperature threshold as 1/16 ADC value */
- adc_val = temp_to_adc(info, info->pdata->max_temp);
- ret = fuel_gauge_reg_writeb(info, AXP20X_V_HTF_DISCHRG, adc_val >> 4);
-
- return ret;
-}
-
-static int fuel_gauge_set_low_btemp_alert(struct axp288_fg_info *info)
-{
- int ret, adc_val;
-
- /* program temperature threshold as 1/16 ADC value */
- adc_val = temp_to_adc(info, info->pdata->min_temp);
- ret = fuel_gauge_reg_writeb(info, AXP20X_V_LTF_DISCHRG, adc_val >> 4);
-
- return ret;
-}
-
static int fuel_gauge_get_property(struct power_supply *ps,
enum power_supply_property prop,
union power_supply_propval *val)
@@ -664,20 +520,6 @@ static int fuel_gauge_get_property(struct power_supply *ps,
goto fuel_gauge_read_err;
val->intval = (ret & 0x0f);
break;
- case POWER_SUPPLY_PROP_TEMP:
- ret = fuel_gauge_get_btemp(info, &value);
- if (ret < 0)
- goto fuel_gauge_read_err;
- val->intval = PROP_TEMP(value);
- break;
- case POWER_SUPPLY_PROP_TEMP_MAX:
- case POWER_SUPPLY_PROP_TEMP_ALERT_MAX:
- val->intval = PROP_TEMP(info->pdata->max_temp);
- break;
- case POWER_SUPPLY_PROP_TEMP_MIN:
- case POWER_SUPPLY_PROP_TEMP_ALERT_MIN:
- val->intval = PROP_TEMP(info->pdata->min_temp);
- break;
case POWER_SUPPLY_PROP_TECHNOLOGY:
val->intval = POWER_SUPPLY_TECHNOLOGY_LION;
break;
@@ -695,17 +537,8 @@ static int fuel_gauge_get_property(struct power_supply *ps,
val->intval = ret * FG_DES_CAP_RES_LSB;
break;
- case POWER_SUPPLY_PROP_CHARGE_FULL_DESIGN:
- val->intval = PROP_CURR(info->pdata->design_cap);
- break;
case POWER_SUPPLY_PROP_VOLTAGE_MAX_DESIGN:
- val->intval = PROP_VOLT(info->pdata->max_volt);
- break;
- case POWER_SUPPLY_PROP_VOLTAGE_MIN_DESIGN:
- val->intval = PROP_VOLT(info->pdata->min_volt);
- break;
- case POWER_SUPPLY_PROP_MODEL_NAME:
- val->strval = info->pdata->battid;
+ val->intval = PROP_VOLT(info->max_volt);
break;
default:
mutex_unlock(&info->lock);
@@ -729,35 +562,6 @@ static int fuel_gauge_set_property(struct power_supply *ps,
mutex_lock(&info->lock);
switch (prop) {
- case POWER_SUPPLY_PROP_STATUS:
- info->status = val->intval;
- break;
- case POWER_SUPPLY_PROP_TEMP_MIN:
- case POWER_SUPPLY_PROP_TEMP_ALERT_MIN:
- if ((val->intval < PD_DEF_MIN_TEMP) ||
- (val->intval > PD_DEF_MAX_TEMP)) {
- ret = -EINVAL;
- break;
- }
- info->pdata->min_temp = UNPROP_TEMP(val->intval);
- ret = fuel_gauge_set_low_btemp_alert(info);
- if (ret < 0)
- dev_err(&info->pdev->dev,
- "temp alert min set fail:%d\n", ret);
- break;
- case POWER_SUPPLY_PROP_TEMP_MAX:
- case POWER_SUPPLY_PROP_TEMP_ALERT_MAX:
- if ((val->intval < PD_DEF_MIN_TEMP) ||
- (val->intval > PD_DEF_MAX_TEMP)) {
- ret = -EINVAL;
- break;
- }
- info->pdata->max_temp = UNPROP_TEMP(val->intval);
- ret = fuel_gauge_set_high_btemp_alert(info);
- if (ret < 0)
- dev_err(&info->pdev->dev,
- "temp alert max set fail:%d\n", ret);
- break;
case POWER_SUPPLY_PROP_CAPACITY_ALERT_MIN:
if ((val->intval < 0) || (val->intval > 15)) {
ret = -EINVAL;
@@ -785,11 +589,6 @@ static int fuel_gauge_property_is_writeable(struct power_supply *psy,
int ret;
switch (psp) {
- case POWER_SUPPLY_PROP_STATUS:
- case POWER_SUPPLY_PROP_TEMP_MIN:
- case POWER_SUPPLY_PROP_TEMP_ALERT_MIN:
- case POWER_SUPPLY_PROP_TEMP_MAX:
- case POWER_SUPPLY_PROP_TEMP_ALERT_MAX:
case POWER_SUPPLY_PROP_CAPACITY_ALERT_MIN:
ret = 1;
break;
@@ -874,158 +673,6 @@ static const struct power_supply_desc fuel_gauge_desc = {
.external_power_changed = fuel_gauge_external_power_changed,
};
-static int fuel_gauge_set_lowbatt_thresholds(struct axp288_fg_info *info)
-{
- int ret;
- u8 reg_val;
-
- ret = fuel_gauge_reg_readb(info, AXP20X_FG_RES);
- if (ret < 0) {
- dev_err(&info->pdev->dev, "%s:read err:%d\n", __func__, ret);
- return ret;
- }
- ret = (ret & FG_REP_CAP_VAL_MASK);
-
- if (ret > FG_LOW_CAP_WARN_THR)
- reg_val = FG_LOW_CAP_WARN_THR;
- else if (ret > FG_LOW_CAP_CRIT_THR)
- reg_val = FG_LOW_CAP_CRIT_THR;
- else
- reg_val = FG_LOW_CAP_SHDN_THR;
-
- reg_val |= FG_LOW_CAP_THR1_VAL;
- ret = fuel_gauge_reg_writeb(info, AXP288_FG_LOW_CAP_REG, reg_val);
- if (ret < 0)
- dev_err(&info->pdev->dev, "%s:write err:%d\n", __func__, ret);
-
- return ret;
-}
-
-static int fuel_gauge_program_vbatt_full(struct axp288_fg_info *info)
-{
- int ret;
- u8 val;
-
- ret = fuel_gauge_reg_readb(info, AXP20X_CHRG_CTRL1);
- if (ret < 0)
- goto fg_prog_ocv_fail;
- else
- val = (ret & ~CHRG_CCCV_CV_MASK);
-
- switch (info->pdata->max_volt) {
- case CV_4100:
- val |= (CHRG_CCCV_CV_4100MV << CHRG_CCCV_CV_BIT_POS);
- break;
- case CV_4150:
- val |= (CHRG_CCCV_CV_4150MV << CHRG_CCCV_CV_BIT_POS);
- break;
- case CV_4200:
- val |= (CHRG_CCCV_CV_4200MV << CHRG_CCCV_CV_BIT_POS);
- break;
- case CV_4350:
- val |= (CHRG_CCCV_CV_4350MV << CHRG_CCCV_CV_BIT_POS);
- break;
- default:
- val |= (CHRG_CCCV_CV_4200MV << CHRG_CCCV_CV_BIT_POS);
- break;
- }
-
- ret = fuel_gauge_reg_writeb(info, AXP20X_CHRG_CTRL1, val);
-fg_prog_ocv_fail:
- return ret;
-}
-
-static int fuel_gauge_program_design_cap(struct axp288_fg_info *info)
-{
- int ret;
-
- ret = fuel_gauge_reg_writeb(info,
- AXP288_FG_DES_CAP1_REG, info->pdata->cap1);
- if (ret < 0)
- goto fg_prog_descap_fail;
-
- ret = fuel_gauge_reg_writeb(info,
- AXP288_FG_DES_CAP0_REG, info->pdata->cap0);
-
-fg_prog_descap_fail:
- return ret;
-}
-
-static int fuel_gauge_program_ocv_curve(struct axp288_fg_info *info)
-{
- int ret = 0, i;
-
- for (i = 0; i < OCV_CURVE_SIZE; i++) {
- ret = fuel_gauge_reg_writeb(info,
- AXP288_FG_OCV_CURVE_REG + i, info->pdata->ocv_curve[i]);
- if (ret < 0)
- goto fg_prog_ocv_fail;
- }
-
-fg_prog_ocv_fail:
- return ret;
-}
-
-static int fuel_gauge_program_rdc_vals(struct axp288_fg_info *info)
-{
- int ret;
-
- ret = fuel_gauge_reg_writeb(info,
- AXP288_FG_RDC1_REG, info->pdata->rdc1);
- if (ret < 0)
- goto fg_prog_ocv_fail;
-
- ret = fuel_gauge_reg_writeb(info,
- AXP288_FG_RDC0_REG, info->pdata->rdc0);
-
-fg_prog_ocv_fail:
- return ret;
-}
-
-static void fuel_gauge_init_config_regs(struct axp288_fg_info *info)
-{
- int ret;
-
- /*
- * check if the config data is already
- * programmed and if so just return.
- */
-
- ret = fuel_gauge_reg_readb(info, AXP288_FG_DES_CAP1_REG);
- if (ret < 0) {
- dev_warn(&info->pdev->dev, "CAP1 reg read err!!\n");
- } else if (!(ret & FG_DES_CAP1_VALID)) {
- dev_info(&info->pdev->dev, "FG data needs to be initialized\n");
- } else {
- dev_info(&info->pdev->dev, "FG data is already initialized\n");
- return;
- }
-
- ret = fuel_gauge_program_vbatt_full(info);
- if (ret < 0)
- dev_err(&info->pdev->dev, "set vbatt full fail:%d\n", ret);
-
- ret = fuel_gauge_program_design_cap(info);
- if (ret < 0)
- dev_err(&info->pdev->dev, "set design cap fail:%d\n", ret);
-
- ret = fuel_gauge_program_rdc_vals(info);
- if (ret < 0)
- dev_err(&info->pdev->dev, "set rdc fail:%d\n", ret);
-
- ret = fuel_gauge_program_ocv_curve(info);
- if (ret < 0)
- dev_err(&info->pdev->dev, "set ocv curve fail:%d\n", ret);
-
- ret = fuel_gauge_set_lowbatt_thresholds(info);
- if (ret < 0)
- dev_err(&info->pdev->dev, "lowbatt thr set fail:%d\n", ret);
-
- ret = fuel_gauge_reg_writeb(info, AXP20X_CC_CTRL, 0xef);
- if (ret < 0)
- dev_err(&info->pdev->dev, "gauge cntl set fail:%d\n", ret);
-}
-
static void fuel_gauge_init_irq(struct axp288_fg_info *info)
{
int ret, i, pirq;
@@ -1065,17 +712,8 @@ static void fuel_gauge_init_irq(struct axp288_fg_info *info)
static void fuel_gauge_init_hw_regs(struct axp288_fg_info *info)
{
- int ret;
unsigned int val;
- ret = fuel_gauge_set_high_btemp_alert(info);
- if (ret < 0)
- dev_err(&info->pdev->dev, "high batt temp set fail:%d\n", ret);
-
- ret = fuel_gauge_set_low_btemp_alert(info);
- if (ret < 0)
- dev_err(&info->pdev->dev, "low batt temp set fail:%d\n", ret);
-
/* enable interrupts */
val = fuel_gauge_reg_readb(info, AXP20X_IRQ3_EN);
val |= TEMP_IRQ_CFG_MASK;
@@ -1101,15 +739,39 @@ static int axp288_fuel_gauge_probe(struct platform_device *pdev)
info->regmap = axp20x->regmap;
info->regmap_irqc = axp20x->regmap_irqc;
info->status = POWER_SUPPLY_STATUS_UNKNOWN;
- info->pdata = pdev->dev.platform_data;
- if (!info->pdata)
- return -ENODEV;
platform_set_drvdata(pdev, info);
mutex_init(&info->lock);
INIT_DELAYED_WORK(&info->status_monitor, fuel_gauge_status_monitor);
+ ret = fuel_gauge_reg_readb(info, AXP288_FG_DES_CAP1_REG);
+ if (ret < 0)
+ return ret;
+
+ if (!(ret & FG_DES_CAP1_VALID)) {
+ dev_err(&pdev->dev, "axp288 not configured by firmware\n");
+ return -ENODEV;
+ }
+
+ ret = fuel_gauge_reg_readb(info, AXP20X_CHRG_CTRL1);
+ if (ret < 0)
+ return ret;
+ switch ((ret & CHRG_CCCV_CV_MASK) >> CHRG_CCCV_CV_BIT_POS) {
+ case CHRG_CCCV_CV_4100MV:
+ info->max_volt = 4100;
+ break;
+ case CHRG_CCCV_CV_4150MV:
+ info->max_volt = 4150;
+ break;
+ case CHRG_CCCV_CV_4200MV:
+ info->max_volt = 4200;
+ break;
+ case CHRG_CCCV_CV_4350MV:
+ info->max_volt = 4350;
+ break;
+ }
+
psy_cfg.drv_data = info;
info->bat = power_supply_register(&pdev->dev, &fuel_gauge_desc, &psy_cfg);
if (IS_ERR(info->bat)) {
@@ -1119,12 +781,11 @@ static int axp288_fuel_gauge_probe(struct platform_device *pdev)
}
fuel_gauge_create_debugfs(info);
- fuel_gauge_init_config_regs(info);
fuel_gauge_init_irq(info);
fuel_gauge_init_hw_regs(info);
schedule_delayed_work(&info->status_monitor, STATUS_MON_DELAY_JIFFIES);
- return ret;
+ return 0;
}
static const struct platform_device_id axp288_fg_id_table[] = {
diff --git a/include/linux/mfd/axp20x.h b/include/linux/mfd/axp20x.h
index fe93e00..fedc301 100644
--- a/include/linux/mfd/axp20x.h
+++ b/include/linux/mfd/axp20x.h
@@ -526,28 +526,6 @@ struct axp20x_dev {
const struct regmap_irq_chip *regmap_irq_chip;
};
-#define BATTID_LEN 64
-#define OCV_CURVE_SIZE 32
-#define MAX_THERM_CURVE_SIZE 25
-#define PD_DEF_MIN_TEMP 0
-#define PD_DEF_MAX_TEMP 55
-
-struct axp20x_fg_pdata {
- char battid[BATTID_LEN + 1];
- int design_cap;
- int min_volt;
- int max_volt;
- int max_temp;
- int min_temp;
- int cap1;
- int cap0;
- int rdc1;
- int rdc0;
- int ocv_curve[OCV_CURVE_SIZE];
- int tcsz;
- int thermistor_curve[MAX_THERM_CURVE_SIZE][2];
-};
-
struct axp20x_chrg_pdata {
int max_cc;
int max_cv;
--
2.9.3
^ permalink raw reply related
* [PATCH 2/4] power: supply: axp288_fuel_gauge: Read 15 bit values 2 registers at a time
From: Hans de Goede @ 2016-12-14 16:38 UTC (permalink / raw)
To: Sebastian Reichel, Chen-Yu Tsai
Cc: russianneuromancer @ ya . ru, linux-pm, linux-i2c, Hans de Goede
In-Reply-To: <20161214163853.454-1-hdegoede@redhat.com>
In order for the MSB -> LSB latching to work correctly we must read the
2 8 bit registers of a 15 bit value in one consecutive read.
This fixes charge_full reporting 3498768 on some reads and 3354624 one
other reads on my tablet (for the 3354624 value the raw LSB is 0x00).
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
drivers/power/supply/axp288_fuel_gauge.c | 63 +++++++++++++++++---------------
1 file changed, 34 insertions(+), 29 deletions(-)
diff --git a/drivers/power/supply/axp288_fuel_gauge.c b/drivers/power/supply/axp288_fuel_gauge.c
index f62f9df..99d6d30 100644
--- a/drivers/power/supply/axp288_fuel_gauge.c
+++ b/drivers/power/supply/axp288_fuel_gauge.c
@@ -29,6 +29,7 @@
#include <linux/iio/consumer.h>
#include <linux/debugfs.h>
#include <linux/seq_file.h>
+#include <asm/unaligned.h>
#define CHRG_STAT_BAT_SAFE_MODE (1 << 3)
#define CHRG_STAT_BAT_VALID (1 << 4)
@@ -73,17 +74,15 @@
#define FG_CNTL_CC_EN (1 << 6)
#define FG_CNTL_GAUGE_EN (1 << 7)
+#define FG_15BIT_WORD_VALID (1 << 15)
+#define FG_15BIT_VAL_MASK 0x7fff
+
#define FG_REP_CAP_VALID (1 << 7)
#define FG_REP_CAP_VAL_MASK 0x7F
#define FG_DES_CAP1_VALID (1 << 7)
-#define FG_DES_CAP1_VAL_MASK 0x7F
-#define FG_DES_CAP0_VAL_MASK 0xFF
#define FG_DES_CAP_RES_LSB 1456 /* 1.456mAhr */
-#define FG_CC_MTR1_VALID (1 << 7)
-#define FG_CC_MTR1_VAL_MASK 0x7F
-#define FG_CC_MTR0_VAL_MASK 0xFF
#define FG_DES_CC_RES_LSB 1456 /* 1.456mAhr */
#define FG_OCV_CAP_VALID (1 << 7)
@@ -189,6 +188,28 @@ static int fuel_gauge_reg_writeb(struct axp288_fg_info *info, int reg, u8 val)
return ret;
}
+static int fuel_gauge_read_15bit_word(struct axp288_fg_info *info, int reg)
+{
+ unsigned char buf[2];
+ int ret;
+
+ ret = regmap_bulk_read(info->regmap, reg, buf, 2);
+ if (ret < 0) {
+ dev_err(&info->pdev->dev, "Error reading reg 0x%02x err: %d\n",
+ reg, ret);
+ return ret;
+ }
+
+ ret = get_unaligned_be16(buf);
+ if (!(ret & FG_15BIT_WORD_VALID)) {
+ dev_err(&info->pdev->dev, "Error reg 0x%02x contents not valid\n",
+ reg);
+ return -ENXIO;
+ }
+
+ return ret & FG_15BIT_VAL_MASK;
+}
+
static int pmic_read_adc_val(const char *name, int *raw_val,
struct axp288_fg_info *info)
{
@@ -255,18 +276,12 @@ static int fuel_gauge_debug_show(struct seq_file *s, void *data)
seq_printf(s, " FG_OCVL[%02x] : %02x\n",
AXP288_FG_OCVL_REG,
fuel_gauge_reg_readb(info, AXP288_FG_OCVL_REG));
- seq_printf(s, "FG_DES_CAP1[%02x] : %02x\n",
+ seq_printf(s, " FG_DES_CAP[%02x] : %04x\n",
AXP288_FG_DES_CAP1_REG,
- fuel_gauge_reg_readb(info, AXP288_FG_DES_CAP1_REG));
- seq_printf(s, "FG_DES_CAP0[%02x] : %02x\n",
- AXP288_FG_DES_CAP0_REG,
- fuel_gauge_reg_readb(info, AXP288_FG_DES_CAP0_REG));
- seq_printf(s, " FG_CC_MTR1[%02x] : %02x\n",
+ fuel_gauge_read_15bit_word(info, AXP288_FG_DES_CAP1_REG));
+ seq_printf(s, " FG_CC_MTR[%02x] : %04x\n",
AXP288_FG_CC_MTR1_REG,
- fuel_gauge_reg_readb(info, AXP288_FG_CC_MTR1_REG));
- seq_printf(s, " FG_CC_MTR0[%02x] : %02x\n",
- AXP288_FG_CC_MTR0_REG,
- fuel_gauge_reg_readb(info, AXP288_FG_CC_MTR0_REG));
+ fuel_gauge_read_15bit_word(info, AXP288_FG_CC_MTR1_REG));
seq_printf(s, " FG_OCV_CAP[%02x] : %02x\n",
AXP288_FG_OCV_CAP_REG,
fuel_gauge_reg_readb(info, AXP288_FG_OCV_CAP_REG));
@@ -663,28 +678,18 @@ static int fuel_gauge_get_property(struct power_supply *ps,
val->intval = POWER_SUPPLY_TECHNOLOGY_LION;
break;
case POWER_SUPPLY_PROP_CHARGE_NOW:
- ret = fuel_gauge_reg_readb(info, AXP288_FG_CC_MTR1_REG);
+ ret = fuel_gauge_read_15bit_word(info, AXP288_FG_CC_MTR1_REG);
if (ret < 0)
goto fuel_gauge_read_err;
- value = (ret & FG_CC_MTR1_VAL_MASK) << 8;
- ret = fuel_gauge_reg_readb(info, AXP288_FG_CC_MTR0_REG);
- if (ret < 0)
- goto fuel_gauge_read_err;
- value |= (ret & FG_CC_MTR0_VAL_MASK);
- val->intval = value * FG_DES_CAP_RES_LSB;
+ val->intval = ret * FG_DES_CAP_RES_LSB;
break;
case POWER_SUPPLY_PROP_CHARGE_FULL:
- ret = fuel_gauge_reg_readb(info, AXP288_FG_DES_CAP1_REG);
+ ret = fuel_gauge_read_15bit_word(info, AXP288_FG_DES_CAP1_REG);
if (ret < 0)
goto fuel_gauge_read_err;
- value = (ret & FG_DES_CAP1_VAL_MASK) << 8;
- ret = fuel_gauge_reg_readb(info, AXP288_FG_DES_CAP0_REG);
- if (ret < 0)
- goto fuel_gauge_read_err;
- value |= (ret & FG_DES_CAP0_VAL_MASK);
- val->intval = value * FG_DES_CAP_RES_LSB;
+ val->intval = ret * FG_DES_CAP_RES_LSB;
break;
case POWER_SUPPLY_PROP_CHARGE_FULL_DESIGN:
val->intval = PROP_CURR(info->pdata->design_cap);
--
2.9.3
^ permalink raw reply related
* Re: [PATCH 5/5] mfd: axp20x: Fix axp288 volatile ranges
From: Hans de Goede @ 2016-12-14 15:07 UTC (permalink / raw)
To: Chen-Yu Tsai; +Cc: Lee Jones, russianneuromancer @ ya . ru, linux-i2c
In-Reply-To: <CAGb2v66Wd06foGn3dxaVrTYZrfhwox0Mc1YB=jM5+Z87UWXMrg@mail.gmail.com>
Hi,
On 14-12-16 15:41, Chen-Yu Tsai wrote:
> On Wed, Dec 14, 2016 at 9:52 PM, Hans de Goede <hdegoede@redhat.com> wrote:
>> The axp288 pmic has a lot more volatile registers then we were
>> listing in axp288_volatile_ranges, fix this.
>>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>> drivers/mfd/axp20x.c | 3 +++
>> 1 file changed, 3 insertions(+)
>>
>> diff --git a/drivers/mfd/axp20x.c b/drivers/mfd/axp20x.c
>> index a294121..b9c1adf 100644
>> --- a/drivers/mfd/axp20x.c
>> +++ b/drivers/mfd/axp20x.c
>> @@ -117,7 +117,10 @@ static const struct regmap_range axp288_writeable_ranges[] = {
>> };
>>
>> static const struct regmap_range axp288_volatile_ranges[] = {
>> + regmap_reg_range(AXP20X_PWR_INPUT_STATUS, AXP20X_PWR_OP_MODE),
>
> Actually register 0x02 is volatile as well. Various fields say "write
> 1 to clear".
> You might need a new define for it though, as the usage is different.
>
>> regmap_reg_range(AXP20X_IRQ1_EN, AXP20X_IPSOUT_V_HIGH_L),
>> + regmap_reg_range(AXP22X_GPIO_STATE, AXP22X_GPIO_STATE),
>> + regmap_reg_range(AXP20X_FG_RES, AXP288_FG_CC_CAP_REG),
>
> Could you also add AXP20X_TIMER_CTRL and 0xa0 ~ 0xa1 (real time
> battery voltage),
> if you're adding defines?
Hmm, ok, I'll do 2 new patches then 1 adding the defines and a v2
of this one. Lee, can you pick up 1 - 4 as they currently are
please? Then I'll just send these 2 new patches.
Regards,
Hans
^ permalink raw reply
* Re: [PATCH] iio: adc: axp288: Drop bogus AXP288_ADC_TS_PIN_CTRL register modifications
From: Chen-Yu Tsai @ 2016-12-14 15:00 UTC (permalink / raw)
To: Hans de Goede
Cc: Jonathan Cameron, Chen-Yu Tsai, Lars-Peter Clausen,
Peter Meerwald-Stadler, russianneuromancer @ ya . ru, linux-iio,
linux-i2c
In-Reply-To: <20161214135525.16477-1-hdegoede@redhat.com>
On Wed, Dec 14, 2016 at 9:55 PM, Hans de Goede <hdegoede@redhat.com> wrote:
> For some reason the axp288_adc driver was modifying the
> AXP288_ADC_TS_PIN_CTRL register, changing bits 0-1 depending on
> whether the GP_ADC channel or another channel was written.
>
> These bits control when a bias current is send to the TS_PIN, the
> GP_ADC has its own pin and a separate bit in another register to
> control the bias current.
>
> Not only does changing when to enable the TS_PIN bias current
> (always or only when sampling) when reading the GP_ADC make no sense
> at all, the code is modifying these bits is writing the entire register,
> assuming that all the other bits have their default value.
>
> So if the firmware has configured a different bias-current for either
> pin, then that change gets clobbered by the write, likewise if the
> firmware has set bit 2 to indicate that the battery has no thermal sensor,
> this will get clobbered by the write.
>
> This commit fixes all this, by simply removing all writes to the
> AXP288_ADC_TS_PIN_CTRL register, they are not needed to read the
> GP_ADC pin, and can actually be harmful.
>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
Acked-by: Chen-Yu Tsai <wens@csie.org>
Note that I do not have hardware to test this.
^ permalink raw reply
* Re: [PATCH 5/5] mfd: axp20x: Fix axp288 volatile ranges
From: Chen-Yu Tsai @ 2016-12-14 14:41 UTC (permalink / raw)
To: Hans de Goede
Cc: Lee Jones, Chen-Yu Tsai, russianneuromancer @ ya . ru, linux-i2c
In-Reply-To: <20161214135209.16369-5-hdegoede@redhat.com>
On Wed, Dec 14, 2016 at 9:52 PM, Hans de Goede <hdegoede@redhat.com> wrote:
> The axp288 pmic has a lot more volatile registers then we were
> listing in axp288_volatile_ranges, fix this.
>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
> drivers/mfd/axp20x.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/drivers/mfd/axp20x.c b/drivers/mfd/axp20x.c
> index a294121..b9c1adf 100644
> --- a/drivers/mfd/axp20x.c
> +++ b/drivers/mfd/axp20x.c
> @@ -117,7 +117,10 @@ static const struct regmap_range axp288_writeable_ranges[] = {
> };
>
> static const struct regmap_range axp288_volatile_ranges[] = {
> + regmap_reg_range(AXP20X_PWR_INPUT_STATUS, AXP20X_PWR_OP_MODE),
Actually register 0x02 is volatile as well. Various fields say "write
1 to clear".
You might need a new define for it though, as the usage is different.
> regmap_reg_range(AXP20X_IRQ1_EN, AXP20X_IPSOUT_V_HIGH_L),
> + regmap_reg_range(AXP22X_GPIO_STATE, AXP22X_GPIO_STATE),
> + regmap_reg_range(AXP20X_FG_RES, AXP288_FG_CC_CAP_REG),
Could you also add AXP20X_TIMER_CTRL and 0xa0 ~ 0xa1 (real time
battery voltage),
if you're adding defines?
Thanks
ChenYu
> };
>
> static const struct regmap_access_table axp288_writeable_table = {
> --
> 2.9.3
>
^ permalink raw reply
* Re: [PATCH 4/5] mfd: axp20x: Drop wrong AXP288_PMIC_ADC_EN define
From: Chen-Yu Tsai @ 2016-12-14 14:29 UTC (permalink / raw)
To: Hans de Goede
Cc: Lee Jones, Chen-Yu Tsai, russianneuromancer @ ya . ru, linux-i2c
In-Reply-To: <20161214135209.16369-4-hdegoede@redhat.com>
On Wed, Dec 14, 2016 at 9:52 PM, Hans de Goede <hdegoede@redhat.com> wrote:
> The adc-enable register for the axp288 is 0x82, not 0x84.
> 0x82 is already defined as AXP20X_ADC_EN1 and that is what the
> axp288_adc driver is actually using, so simply drop the wrong
> AXP288_PMIC_ADC_EN define.
>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
Acked-by: Chen-Yu Tsai <wens@csie.org>
^ permalink raw reply
* Re: [PATCH 3/5] mfd: axp20x: Fix axp288 PEK_DBR and PEK_DBF irqs being swapped
From: Chen-Yu Tsai @ 2016-12-14 14:28 UTC (permalink / raw)
To: Hans de Goede
Cc: Lee Jones, Chen-Yu Tsai, russianneuromancer @ ya . ru, linux-i2c
In-Reply-To: <20161214135209.16369-3-hdegoede@redhat.com>
On Wed, Dec 14, 2016 at 9:52 PM, Hans de Goede <hdegoede@redhat.com> wrote:
> The R in PEK_DBR stands for rising, so it should be mapped to
> AXP288_IRQ_POKP where the last P stands for positive edge.
>
> Likewise PEK_DBF should be mapped to the falling edge, aka the
> _N_egative edge, so it should be mapped to AXP288_IRQ_POKN.
>
> This fixes the inverted powerbutton status reporting by the
> axp20x-pek driver.
>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
Acked-by: Chen-Yu Tsai <wens@csie.org>
^ permalink raw reply
* Re: [PATCH 2/5] mfd: axp20x: Add missing axp288 irqs
From: Chen-Yu Tsai @ 2016-12-14 14:21 UTC (permalink / raw)
To: Hans de Goede
Cc: Lee Jones, Chen-Yu Tsai, russianneuromancer @ ya . ru, linux-i2c
In-Reply-To: <20161214135209.16369-2-hdegoede@redhat.com>
On Wed, Dec 14, 2016 at 9:52 PM, Hans de Goede <hdegoede@redhat.com> wrote:
> The axp288 has the following irqs 2 times: VBUS_FALL, VBUS_RISE,
> VBUS_OV. On boot / reset the enable flags for both the normal and alt
> version of these irqs is set.
>
> Since we were only listing the normal version in the axp288 regmap_irq
> struct, we were never disabling the alt versions of these irqs.
>
> Add the alt versions to the axp288 regmap_irq struct, so that these
> get properly disabled.
>
> Together with the other axp288 fixes in this series, this fixes the axp288
> irq contineously triggering.
>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
Acked-by: Chen-Yu Tsai <wens@csie.org>
^ permalink raw reply
* Re: [PATCH 1/5] mfd: axp20x: Use IRQF_TRIGGER_LOW on the axp288
From: Chen-Yu Tsai @ 2016-12-14 14:19 UTC (permalink / raw)
To: Hans de Goede
Cc: Lee Jones, Chen-Yu Tsai, russianneuromancer @ ya . ru, linux-i2c
In-Reply-To: <20161214135209.16369-1-hdegoede@redhat.com>
On Wed, Dec 14, 2016 at 9:52 PM, Hans de Goede <hdegoede@redhat.com> wrote:
> The interrupt line of the entire family of axp2xx pmics is active-low,
> for devicetree enumerated irqs, this is dealt with in the devicetree.
>
> ACPI irq resources have a flag field for this too, I tried using this
> on my CUBE iwork8 Air tablet, but it does not contain the right data.
>
> The dstd shows the irq listed as either ActiveLow or ActiveHigh,
> depending on the OSID variable, which seems to be set by the
> "OS IMAGE ID" in the BIOS/EFI setup screen.
>
> Since the acpi-resource info is no good, simply pass in IRQF_TRIGGER_LOW
> on the axp288.
>
> Together with the other axp288 fixes in this series, this fixes the axp288
> irq contineously triggering.
>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
This patch looks good to me. However, I do not have any AXP288 hardware
to test it on, nor am I familiar with the ACPI stuff.
Acked-by: Chen-Yu Tsai <wens@csie.org>
P.S. I don't think we're handling IRQ trigger types at all. The hardware
default for the NMI interrupt in the Allwinner chips just happens to be
active low.
^ permalink raw reply
* Re: [[RFC V3] 1/1] i2c: imx: add slave support
From: Vladimir Zapolskiy @ 2016-12-14 13:30 UTC (permalink / raw)
To: Joshua Frkuska, linux-i2c; +Cc: wsa, syrchin, peda, Jiada_Wang
In-Reply-To: <1481695291-11444-2-git-send-email-joshua_frkuska@mentor.com>
Hi Joshua,
please find review comments below, mainly stylistic ones.
On 12/14/2016 08:01 AM, Joshua Frkuska wrote:
> Add I2C slave provider using the generic slave interface.
> It also supports master transactions when the slave in the idle mode.
>
> Signed-off-by: Maxim Syrchin <msyrchin@dev.rtsoft.ru>
> Signed-off-by: Joshua Frkuska <joshua_frkuska@mentor.com>
should you preserve Maxim's authorship?
> ---
> drivers/i2c/busses/i2c-imx.c | 724 +++++++++++++++++++++++++++++++++++++++++--
> 1 file changed, 694 insertions(+), 30 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-imx.c b/drivers/i2c/busses/i2c-imx.c
> index 47fc1f1..7b2aeb8 100644
> --- a/drivers/i2c/busses/i2c-imx.c
> +++ b/drivers/i2c/busses/i2c-imx.c
> @@ -53,6 +53,7 @@
> #include <linux/pm_runtime.h>
> #include <linux/sched.h>
> #include <linux/slab.h>
> +#include <linux/kthread.h>
Please insert #include keeping the list alphabetically numbered.
>
> /* This will be the driver name the kernel reports */
> #define DRIVER_NAME "imx-i2c"
> @@ -60,6 +61,13 @@
> /* Default value */
> #define IMX_I2C_BIT_RATE 100000 /* 100kHz */
>
> +/* Wait delays */
> +#define STATE_MIN_WAIT_TIME 1 /* 1 jiffy */
> +#define STATE_WAIT_TIME (HZ / 10)
> +
> +#define MAX_EVENTS (1<<4)
checkpatch warnings:
CHECK: spaces preferred around that '<<' (ctx:VxV)
#35: FILE: drivers/i2c/busses/i2c-imx.c:68:
+#define MAX_EVENTS (1<<4)
^
CHECK: Prefer using the BIT macro
#35: FILE: drivers/i2c/busses/i2c-imx.c:68:
+#define MAX_EVENTS (1<<4)
The second warning seems to be irrelevant, please fix the first one.
> +#define EUNDEFINED 4000
I would recommend to avoid introduction of new errnos, please use
one of the existing.
> +
> /*
> * Enable DMA if transfer byte size is bigger than this threshold.
> * As the hardware request, it must bigger than 4 bytes.\
> @@ -171,6 +179,30 @@ enum imx_i2c_type {
> VF610_I2C,
> };
>
> +enum i2c_imx_state {
> + STATE_IDLE = 0,
> + STATE_SLAVE,
> + STATE_MASTER,
> + STATE_SP
> +};
> +
> +enum i2c_imx_events {
> + EVT_AL = 0,
> + EVT_SI,
> + EVT_START,
> + EVT_STOP,
> + EVT_POLL,
> + EVT_INVALID,
> + EVT_ENTRY
> +};
> +
> +struct evt_queue {
> + atomic_t count;
> + atomic_t ir;
> + atomic_t iw;
> + unsigned int evt[MAX_EVENTS];
> +};
> +
> struct imx_i2c_hwdata {
> enum imx_i2c_type devtype;
> unsigned regshift;
> @@ -193,6 +225,7 @@ struct imx_i2c_dma {
>
> struct imx_i2c_struct {
> struct i2c_adapter adapter;
> + struct i2c_client *slave;
> struct clk *clk;
> void __iomem *base;
> wait_queue_head_t queue;
> @@ -210,6 +243,18 @@ struct imx_i2c_struct {
> struct pinctrl_state *pinctrl_pins_gpio;
>
> struct imx_i2c_dma *dma;
> +
> + unsigned int state;
> + struct evt_queue events;
> + atomic_t last_error;
Please replace 'last_error' type to 'int', this will require changes
in the code as well.
In general the usage of this variable is questionable, while
it can be set to 0, -EBUSY, -ETIMEDOUT, -EAGAIN and -EUNDEFINED,
from the code there are only two classes impact runtime behaviour:
-EUNDEFINED and anything else, I didn't notice the difference between
e.g. 0 and -EBUSY.
> +
> + int master_interrupt;
> + int start_retry_cnt;
> + int slave_poll_cnt;
> +
> + struct task_struct *slave_task;
> + wait_queue_head_t state_queue;
> +
Please drop the empty line above.
> };
>
> static const struct imx_i2c_hwdata imx1_i2c_hwdata = {
> @@ -414,17 +459,29 @@ static void i2c_imx_dma_free(struct imx_i2c_struct *i2c_imx)
> static int i2c_imx_bus_busy(struct imx_i2c_struct *i2c_imx, int for_busy)
> {
> unsigned long orig_jiffies = jiffies;
> - unsigned int temp;
> + unsigned int temp, ctl;
> +
>
> dev_dbg(&i2c_imx->adapter.dev, "<%s>\n", __func__);
>
> while (1) {
> temp = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2SR);
> + ctl = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2CR);
> +
> + /*
> + * Check for arbitration lost. Datasheet recommends to
> + * clean IAL bit in interrupt handler before any other
> + * action.
> + *
> + * We can detect if controller resets MSTA bit, because
> + * hardware is switched to slave mode if arbitration was
> + * lost.
> + */
>
> - /* check for arbitration lost */
> - if (temp & I2SR_IAL) {
> - temp &= ~I2SR_IAL;
> - imx_i2c_write_reg(temp, i2c_imx, IMX_I2C_I2SR);
> + if (for_busy && !(ctl & I2CR_MSTA)) {
> + dev_dbg(&i2c_imx->adapter.dev,
> + "<%s> Lost arbitration (SR = %02x, CR = %02x)\n",
> + __func__, temp, ctl);
> return -EAGAIN;
> }
>
> @@ -443,16 +500,501 @@ static int i2c_imx_bus_busy(struct imx_i2c_struct *i2c_imx, int for_busy)
> return 0;
> }
>
> +#ifdef CONFIG_I2C_SLAVE
> +
> +static void i2c_imx_enable_i2c_controller(struct imx_i2c_struct *i2c_imx);
> +static void set_state(struct imx_i2c_struct *i2c_imx, unsigned int new);
> +static int i2c_imx_hw_start(struct imx_i2c_struct *i2c_imx);
> +static void i2c_imx_stop(struct imx_i2c_struct *i2c_imx);
> +
> +static inline int evt_find_next_idx(atomic_t *v)
> +{
> + return atomic_inc_return(v) & (MAX_EVENTS - 1);
> +}
> +
> +static unsigned int evt_put(struct evt_queue *stk, unsigned int evt)
> +{
> + int count = atomic_inc_return(&stk->count);
> + int idx;
> +
> + if (count < MAX_EVENTS) {
> + idx = evt_find_next_idx(&stk->iw);
> + stk->evt[idx] = evt;
> + } else {
> + atomic_dec(&stk->count);
> + return EVT_INVALID;
> + }
> +
> + return 0;
> +}
> +
> +static unsigned int evt_get(struct evt_queue *stk)
> +{
> + int count = atomic_dec_return(&stk->count);
> + int idx;
> +
> + if (count >= 0) {
> + idx = evt_find_next_idx(&stk->ir);
> + } else {
> + atomic_inc(&stk->count);
> + return EVT_INVALID;
> + }
> +
> + return stk->evt[idx];
> +}
> +
> +static unsigned int evt_is_empty(struct evt_queue *stk)
static bool evt_is_empty()
> +{
> + int ret = atomic_read(&stk->count);
> +
> + return (ret <= 0);
> +}
> +
> +static void evt_init(struct evt_queue *stk)
> +{
> + atomic_set(&stk->count, 0);
> + atomic_set(&stk->iw, 0);
> + atomic_set(&stk->ir, 0);
> +}
> +
> +
Please don't use multiple blank lines.
> +static void i2c_imx_clear_ial_bit(struct imx_i2c_struct *i2c_imx)
> +{
> + unsigned int status;
> +
> + status = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2SR);
> + status &= ~I2SR_IAL;
> + imx_i2c_write_reg(status, i2c_imx, IMX_I2C_I2SR);
> +}
> +
> +static void i2c_imx_slave_init(struct imx_i2c_struct *i2c_imx)
> +{
> + unsigned int temp;
> +
> + dev_dbg(&i2c_imx->adapter.dev, "<%s>\n", __func__);
> +
> + i2c_imx_enable_i2c_controller(i2c_imx);
> +
> + /* Set Slave mode with interrupt enable */
> + temp = i2c_imx->hwdata->i2cr_ien_opcode | I2CR_IIEN;
> + imx_i2c_write_reg(temp, i2c_imx, IMX_I2C_I2CR);
> +}
> +
> +
Please don't use multiple blank lines.
> +static void i2c_imx_slave_process_irq(struct imx_i2c_struct *i2c_imx)
> +{
> + unsigned int status, ctl;
> + u8 data;
> +
> + status = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2SR);
> + ctl = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2CR);
> +
> + if (status & I2SR_IAAS) {
> + if (status & I2SR_SRW) {
> + /* master wants to read from us */
> + i2c_slave_event(i2c_imx->slave,
> + I2C_SLAVE_READ_REQUESTED, &data);
Alignment should match open parenthesis.
> + ctl |= I2CR_MTX;
> + imx_i2c_write_reg(ctl, i2c_imx, IMX_I2C_I2CR);
> +
> + /*send data */
> + imx_i2c_write_reg(data, i2c_imx, IMX_I2C_I2DR);
> + } else {
> + dev_dbg(&i2c_imx->adapter.dev, "write requested");
> + i2c_slave_event(i2c_imx->slave,
> + I2C_SLAVE_WRITE_REQUESTED, &data);
Alignment should match open parenthesis.
> + /*slave receive */
> + ctl &= ~I2CR_MTX;
> + imx_i2c_write_reg(ctl, i2c_imx, IMX_I2C_I2CR);
> +
> + /*dummy read */
> + data = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2DR);
For dummy read there is no need to assign the result to 'data'.
> + }
> + } else {
> + /* slave send */
> + if (ctl & I2CR_MTX) {
> + if (!(status & I2SR_RXAK)) { /*ACK received */
> + i2c_slave_event(i2c_imx->slave,
> + I2C_SLAVE_READ_PROCESSED, &data);
Alignment should match open parenthesis.
> + ctl |= I2CR_MTX;
> + imx_i2c_write_reg(ctl, i2c_imx, IMX_I2C_I2CR);
> + /*send data */
> + imx_i2c_write_reg(data, i2c_imx, IMX_I2C_I2DR);
> + } else {
> + /*no ACK. */
> + /*dummy read */
> + dev_dbg(&i2c_imx->adapter.dev, "read requested");
> + i2c_slave_event(i2c_imx->slave,
> + I2C_SLAVE_READ_REQUESTED, &data);
Alignment should match open parenthesis.
> +
> + ctl &= ~I2CR_MTX;
> + imx_i2c_write_reg(ctl, i2c_imx, IMX_I2C_I2CR);
> + imx_i2c_read_reg(i2c_imx, IMX_I2C_I2DR);
> + }
> + } else { /*read */
> + ctl &= ~I2CR_MTX;
> + imx_i2c_write_reg(ctl, i2c_imx, IMX_I2C_I2CR);
> +
> + /*read */
> + data = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2DR);
> + dev_dbg(&i2c_imx->adapter.dev, "received %x",
> + (unsigned int) data);
Cast is not needed here IMHO.
> + i2c_slave_event(i2c_imx->slave,
> + I2C_SLAVE_WRITE_RECEIVED, &data);
Alignment should match open parenthesis.
> + }
> + }
> +}
> +
> +
Please don't use multiple blank lines.
> +static int idle_evt_handler(struct imx_i2c_struct *i2c_imx, unsigned int event)
> +{
> + u8 reg;
> +
> + switch (event) {
> + case EVT_ENTRY:
> + imx_i2c_write_reg(i2c_imx->hwdata->i2cr_ien_opcode ^ I2CR_IEN,
> + i2c_imx, IMX_I2C_I2CR);
Alignment should match open parenthesis.
> + i2c_imx_enable_i2c_controller(i2c_imx);
> + imx_i2c_write_reg(i2c_imx->hwdata->i2cr_ien_opcode | I2CR_IIEN,
> + i2c_imx, IMX_I2C_I2CR);
Alignment should match open parenthesis.
> + if (atomic_read(&i2c_imx->last_error) == -EUNDEFINED) {
> + dev_dbg(&i2c_imx->adapter.dev, "Reset lost START event\n");
> + atomic_set(&i2c_imx->last_error, -EBUSY);
> + }
> + i2c_imx->start_retry_cnt = 0;
> + return 0;
> + case EVT_AL:
> + i2c_imx_clear_ial_bit(i2c_imx);
> + break;
> + case EVT_SI:
> + set_state(i2c_imx, STATE_SLAVE);
> + i2c_imx_slave_process_irq(i2c_imx);
> + break;
> + case EVT_START:
> + reg = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2SR);
> + if ((reg & I2SR_IBB) != 0) {
> + atomic_set(&i2c_imx->last_error, -EBUSY);
> + break;
> + }
> +
> + reg = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2CR);
> + reg |= I2CR_MSTA;
> + imx_i2c_write_reg(reg, i2c_imx, IMX_I2C_I2CR);
> + set_state(i2c_imx, STATE_SP);
> + i2c_imx->start_retry_cnt = 0;
> + return 0;
> + case EVT_STOP:
> + case EVT_POLL:
> + default:
> + break;
> + }
> +
> + return STATE_WAIT_TIME;
> +}
> +
> +static int master_evt_handler(struct imx_i2c_struct *i2c_imx,
> + unsigned int event)
> +{
> + switch (event) {
> + case EVT_ENTRY:
> + i2c_imx->start_retry_cnt = 0;
> + return 0;
> + case EVT_AL:
> + set_state(i2c_imx, STATE_IDLE);
> + break;
> + case EVT_SI:
> + break;
> + case EVT_START:
> + atomic_set(&i2c_imx->last_error, -EBUSY);
> + break;
> + case EVT_STOP:
> + imx_i2c_write_reg(i2c_imx->hwdata->i2sr_clr_opcode, i2c_imx,
> + IMX_I2C_I2SR);
Alignment should match open parenthesis.
> + imx_i2c_write_reg(i2c_imx->hwdata->i2cr_ien_opcode | I2CR_IIEN,
> + i2c_imx, IMX_I2C_I2CR);
Alignment should match open parenthesis.
> +
> + i2c_imx->stopped = 1;
> + udelay(50);
CHECK: usleep_range is preferred over udelay; see Documentation/timers/timers-howto.txt
#352: FILE: drivers/i2c/busses/i2c-imx.c:717:
+ udelay(50);
> + set_state(i2c_imx, STATE_IDLE);
> + return 0;
> + case EVT_POLL:
> + default:
> + break;
> + }
> +
> + return STATE_WAIT_TIME;
> +}
> +
> +static int slave_evt_handler (struct imx_i2c_struct *i2c_imx,
Please drop the space ^^^^
> + unsigned int event)
> +{
> + u8 reg, data;
> +
> + switch (event) {
> + case EVT_ENTRY:
> + if (atomic_read(&i2c_imx->last_error) == -EUNDEFINED) {
> + dev_dbg(&i2c_imx->adapter.dev, "Reset lost START event\n");
> + atomic_set(&i2c_imx->last_error, -EBUSY);
> + }
> + i2c_imx->start_retry_cnt = 0;
> + i2c_imx->slave_poll_cnt = 0;
> + return 0;
> + case EVT_AL:
> + set_state(i2c_imx, STATE_IDLE);
> + break;
> + case EVT_START:
> + reg = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2CR);
> + atomic_set(&i2c_imx->last_error, -EBUSY);
> + break;
> + case EVT_STOP:
> + break;
> + case EVT_SI:
> + i2c_imx_slave_process_irq(i2c_imx);
> + reg = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2SR);
> + if ((reg & I2SR_IBB) == 0) {
> + data = 0;
> + set_state(i2c_imx, STATE_IDLE);
> + dev_dbg(&i2c_imx->adapter.dev, "end of package");
> + i2c_slave_event(i2c_imx->slave, I2C_SLAVE_STOP, &data);
> + }
> + if (i2c_imx->slave_poll_cnt > 10) {
> + dev_err(&i2c_imx->adapter.dev,
> + "Too much slave loops (%i)\n",
> + i2c_imx->slave_poll_cnt);
> + }
> +
> + i2c_imx->slave_poll_cnt = 0;
> + break;
> + case EVT_POLL:
> + reg = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2SR);
> + if ((reg & I2SR_IBB) == 0) {
> + data = 0;
> + set_state(i2c_imx, STATE_IDLE);
> + dev_dbg(&i2c_imx->adapter.dev, "end of package");
> + i2c_slave_event(i2c_imx->slave, I2C_SLAVE_STOP, &data);
> + if (i2c_imx->slave_poll_cnt > 10) {
> + dev_err(&i2c_imx->adapter.dev,
> + "Too much slave loops (%i)\n",
> + i2c_imx->slave_poll_cnt);
> + }
> +
> + i2c_imx->slave_poll_cnt = 0;
> + }
> +
> + /*
> + * TODO: do "dummy read" if no interrupts or stop condition
> + * for more then 10 wait loops
> + */
> + i2c_imx->slave_poll_cnt += 1;
> + if (i2c_imx->slave_poll_cnt % 10 == 0)
> + imx_i2c_read_reg(i2c_imx, IMX_I2C_I2DR);
> + break;
> + default:
> + break;
> + }
> +
> + return STATE_MIN_WAIT_TIME;
> +}
> +
> +static int sp_evt_handler (struct imx_i2c_struct *i2c_imx, unsigned int event)
Please drop the space ^^^^
> +{
> + u8 reg;
> +
> + switch (event) {
> + case EVT_AL:
> + dev_dbg(&i2c_imx->adapter.dev, "Lost arbitration on START");
> + atomic_set(&i2c_imx->last_error, -EAGAIN);
> + set_state(i2c_imx, STATE_IDLE);
> + return 0;
> + case EVT_SI:
> + set_state(i2c_imx, STATE_IDLE);
> + evt_put(&i2c_imx->events, EVT_SI);
> + case EVT_START:
> + atomic_set(&i2c_imx->last_error, -EBUSY);
> + break;
> + case EVT_STOP:
> + break;
> + case EVT_ENTRY:
> + return 0;
> + case EVT_POLL:
> + reg = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2SR);
> + if ((reg & I2SR_IBB) && !(reg & I2SR_IAL)) {
> +
Please drop the empty line above.
> + set_state(i2c_imx, STATE_MASTER);
> + reg = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2CR);
> +
> + i2c_imx->stopped = 0;
> + reg |= I2CR_IIEN | I2CR_MTX | I2CR_TXAK;
> + reg &= ~I2CR_DMAEN;
> + imx_i2c_write_reg(reg, i2c_imx, IMX_I2C_I2CR);
> + atomic_set(&i2c_imx->last_error, 0);
> + i2c_imx->start_retry_cnt = 0;
> + return 0;
> + }
> + break;
> + default:
> + break;
> +
Please drop the empty line above.
> + }
> +
> + if (i2c_imx->start_retry_cnt++ < 100) {
> + dev_dbg(&i2c_imx->adapter.dev,
> + "wait for busy cnt = %i evt = %i",
> + i2c_imx->start_retry_cnt, event);
> + } else {
> +
Please drop the empty line above.
> + dev_dbg(&i2c_imx->adapter.dev, "start timeout");
> + i2c_imx->start_retry_cnt = 0;
> + atomic_set(&i2c_imx->last_error, -ETIMEDOUT);
> + set_state(i2c_imx, STATE_IDLE);
> + return STATE_WAIT_TIME;
> + }
> +
> + return STATE_MIN_WAIT_TIME;
> +}
> +
> +static void set_state(struct imx_i2c_struct *i2c_imx, unsigned int new)
> +{
> + i2c_imx->state = new;
> +
> + switch (new) {
> + case STATE_IDLE:
> + idle_evt_handler(i2c_imx, EVT_ENTRY);
> + break;
> + case STATE_SLAVE:
> + slave_evt_handler(i2c_imx, EVT_ENTRY);
> + break;
> + case STATE_SP:
> + sp_evt_handler(i2c_imx, EVT_ENTRY);
> + break;
> + case STATE_MASTER:
> + master_evt_handler(i2c_imx, EVT_ENTRY);
> + break;
> + }
> +}
> +
> +
Please drop one empty line above.
> +static int i2c_imx_slave_threadfn(void *pdata)
> +{
> + unsigned int event, delay;
> + struct imx_i2c_struct *i2c_imx = (struct imx_i2c_struct *) pdata;
Please remove the cast and swap the lines.
> +
> + do {
> +
Please drop one empty line above.
> + event = evt_get(&i2c_imx->events);
> + if (event == EVT_INVALID)
> + event = EVT_POLL;
> +
> + switch (i2c_imx->state) {
> + case STATE_IDLE:
> + delay = idle_evt_handler(i2c_imx, event);
> + break;
> + case STATE_SLAVE:
> + delay = slave_evt_handler(i2c_imx, event);
> + break;
> + case STATE_SP:
> + delay = sp_evt_handler(i2c_imx, event);
> + break;
> + case STATE_MASTER:
> + delay = master_evt_handler(i2c_imx, event);
> + break;
> + default:
> + delay = 0;
> + break;
> + }
> +
> + if (delay)
> + wait_event_interruptible_timeout(i2c_imx->state_queue,
> + (evt_is_empty(&i2c_imx->events) == 0),
> + delay);
I would propose to add here the handling of the return value of
wait_event_interruptible_timeout()
> +
> + } while (kthread_should_stop() == 0);
> +
> + return 0;
> +}
> +
> +static int i2c_imx_reg_slave(struct i2c_client *slave)
> +{
> + struct imx_i2c_struct *i2c_imx = i2c_get_adapdata(slave->adapter);
> + int result;
> + struct sched_param param = { .sched_priority = MAX_RT_PRIO - 1 };
> +
> + dev_dbg(&i2c_imx->adapter.dev, "<%s>\n", __func__);
> +
> + if (i2c_imx->slave)
> + return -EBUSY;
> +
> + if (slave->flags & I2C_CLIENT_TEN)
> + return -EAFNOSUPPORT;
> +
> + i2c_imx->slave = slave;
> +
> + /* Set the Slave address */
> + imx_i2c_write_reg((i2c_imx->slave->addr << 1), i2c_imx, IMX_I2C_IADR);
> +
> + result = i2c_imx_hw_start(i2c_imx);
> + if (result != 0)
if (result)
> + return result;
> +
> + i2c_imx->slave_task = kthread_run(i2c_imx_slave_threadfn,
> + (void *)i2c_imx, "i2c-slave-%s", i2c_imx->adapter.name);
(void *) cast is unneeded.
> +
> + sched_setscheduler(i2c_imx->slave_task, SCHED_FIFO, ¶m);
i2c_imx->slave_task may be set to a ERR_PTR() value, please move this line
after the check for a potential error.
> +
> + if (IS_ERR(i2c_imx->slave_task))
> + return PTR_ERR(i2c_imx->slave_task);
> +
> + i2c_imx_slave_init(i2c_imx);
> +
> + return 0;
> +
Please drop the emtpy line above.
> +}
> +
> +static int i2c_imx_unreg_slave(struct i2c_client *slave)
> +{
> + struct imx_i2c_struct *i2c_imx = i2c_get_adapdata(slave->adapter);
> +
> + dev_dbg(&i2c_imx->adapter.dev, "<%s>\n", __func__);
> + if (i2c_imx->slave_task != NULL)
Who does set "i2c_imx->slave_task" to NULL ?
Here it looks like a redundant check, please confirm.
> + kthread_stop(i2c_imx->slave_task);
> +
> + wake_up(&i2c_imx->state_queue);
> +
> + i2c_imx->slave_task = NULL;
> +
> + i2c_imx->slave = NULL;
> +
> + i2c_imx_stop(i2c_imx);
> +
> + return 0;
> +}
> +
> +
Please drop one of two empty lines above.
> +#else
> +
> +static void evt_init(struct evt_queue *stk)
> +{
> + return;
> +}
> +
> +static unsigned int evt_put(struct evt_queue *stk, unsigned int evt)
> +{
> + return 0;
> +}
> +
> +#endif
> +
> static int i2c_imx_trx_complete(struct imx_i2c_struct *i2c_imx)
> {
> - wait_event_timeout(i2c_imx->queue, i2c_imx->i2csr & I2SR_IIF, HZ / 10);
> + wait_event_timeout(i2c_imx->queue, i2c_imx->master_interrupt,
> + STATE_WAIT_TIME);
>
> - if (unlikely(!(i2c_imx->i2csr & I2SR_IIF))) {
> + if (unlikely(!(i2c_imx->master_interrupt))) {
> dev_dbg(&i2c_imx->adapter.dev, "<%s> Timeout\n", __func__);
> return -ETIMEDOUT;
> }
> dev_dbg(&i2c_imx->adapter.dev, "<%s> TRX complete\n", __func__);
> - i2c_imx->i2csr = 0;
> + i2c_imx->master_interrupt = 0;
> return 0;
> }
>
> @@ -510,27 +1052,92 @@ static void i2c_imx_set_clk(struct imx_i2c_struct *i2c_imx)
> #endif
> }
>
> +static int i2c_imx_configure_clock(struct imx_i2c_struct *i2c_imx)
> +{
> + int result;
> +
> + i2c_imx_set_clk(i2c_imx);
> +
> + result = clk_prepare_enable(i2c_imx->clk);
> + if (result == 0)
if (!result)
> + imx_i2c_write_reg(i2c_imx->ifdr, i2c_imx, IMX_I2C_IFDR);
> +
> + return result;
> +}
> +
> +static void i2c_imx_enable_i2c_controller(struct imx_i2c_struct *i2c_imx)
> +{
> + imx_i2c_write_reg(i2c_imx->hwdata->i2sr_clr_opcode, i2c_imx,
> + IMX_I2C_I2SR);
> + imx_i2c_write_reg(i2c_imx->hwdata->i2cr_ien_opcode, i2c_imx,
> + IMX_I2C_I2CR);
> +
> + /* Wait controller to be stable */
> + udelay(50);
> +}
> +
> +static int i2c_imx_hw_start(struct imx_i2c_struct *i2c_imx)
> +{
> + int result;
> +
> + dev_dbg(&i2c_imx->adapter.dev, "<%s>\n", __func__);
> +
> + result = i2c_imx_configure_clock(i2c_imx);
> + if (result != 0)
> + return result;
> + i2c_imx_enable_i2c_controller(i2c_imx);
> + return 0;
> +}
> +
> +
> static int i2c_imx_start(struct imx_i2c_struct *i2c_imx)
> {
> unsigned int temp = 0;
> - int result;
> + int result, cnt = 0;
>
> dev_dbg(&i2c_imx->adapter.dev, "<%s>\n", __func__);
>
> - i2c_imx_set_clk(i2c_imx);
> + if (i2c_imx->slave_task != NULL) {
if (i2c_imx->slave_task)
>
> - imx_i2c_write_reg(i2c_imx->ifdr, i2c_imx, IMX_I2C_IFDR);
> - /* Enable I2C controller */
> - imx_i2c_write_reg(i2c_imx->hwdata->i2sr_clr_opcode, i2c_imx, IMX_I2C_I2SR);
> - imx_i2c_write_reg(i2c_imx->hwdata->i2cr_ien_opcode, i2c_imx, IMX_I2C_I2CR);
> + atomic_set(&i2c_imx->last_error, -EUNDEFINED);
> + if (evt_put(&i2c_imx->events, EVT_START) != 0) {
> + dev_err(&i2c_imx->adapter.dev,
> + "Event buffer overflow\n");
> + return -EBUSY;
> + }
>
> - /* Wait controller to be stable */
> - usleep_range(50, 150);
> + wake_up(&i2c_imx->state_queue);
> +
> + while ((result = atomic_read(&i2c_imx->last_error)) == -EUNDEFINED) {
> + schedule();
> +
> + /* TODO: debug workaround - start hung monitoring */
> + cnt++;
> + if (cnt == 500000) {
> + dev_err(&i2c_imx->adapter.dev,
> + "Too many start loops\n");
> + imx_i2c_write_reg(i2c_imx->hwdata->i2cr_ien_opcode ^ I2CR_IEN,
> + i2c_imx, IMX_I2C_I2CR);
> + i2c_imx_enable_i2c_controller(i2c_imx);
> + imx_i2c_write_reg(i2c_imx->hwdata->i2cr_ien_opcode | I2CR_IIEN,
> + i2c_imx, IMX_I2C_I2CR);
> +
> + return -ETIMEDOUT;
> + }
> +
> + };
Please drop the trailing semicolon.
> + return result;
> + }
> +
> + result = i2c_imx_hw_start(i2c_imx);
> + if (result != 0)
if (result)
> + return result;
>
> /* Start I2C transaction */
> temp = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2CR);
> temp |= I2CR_MSTA;
> imx_i2c_write_reg(temp, i2c_imx, IMX_I2C_I2CR);
> +
> result = i2c_imx_bus_busy(i2c_imx, 1);
> if (result)
> return result;
> @@ -542,10 +1149,9 @@ static int i2c_imx_start(struct imx_i2c_struct *i2c_imx)
> return result;
> }
>
> -static void i2c_imx_stop(struct imx_i2c_struct *i2c_imx)
> +static void i2c_imx_hw_stop(struct imx_i2c_struct *i2c_imx)
> {
> unsigned int temp = 0;
> -
Please don't drop this empty line.
> if (!i2c_imx->stopped) {
> /* Stop I2C transaction */
> dev_dbg(&i2c_imx->adapter.dev, "<%s>\n", __func__);
> @@ -555,6 +1161,7 @@ static void i2c_imx_stop(struct imx_i2c_struct *i2c_imx)
> temp &= ~I2CR_DMAEN;
> imx_i2c_write_reg(temp, i2c_imx, IMX_I2C_I2CR);
> }
> +
> if (is_imx1_i2c(i2c_imx)) {
> /*
> * This delay caused by an i.MXL hardware bug.
> @@ -568,24 +1175,58 @@ static void i2c_imx_stop(struct imx_i2c_struct *i2c_imx)
> i2c_imx->stopped = 1;
> }
>
> - /* Disable I2C controller */
> - temp = i2c_imx->hwdata->i2cr_ien_opcode ^ I2CR_IEN,
> + temp = i2c_imx->hwdata->i2cr_ien_opcode ^ I2CR_IEN;
> imx_i2c_write_reg(temp, i2c_imx, IMX_I2C_I2CR);
> +
> + clk_disable_unprepare(i2c_imx->clk);
> +
> +}
Please drop the empty line before closing curly bracket.
> +
> +static void i2c_imx_stop(struct imx_i2c_struct *i2c_imx)
> +{
> + if (i2c_imx->slave == NULL) {
if (!i2c_imx->slave)
> + i2c_imx_hw_stop(i2c_imx);
> + } else {
> +
Please drop the empty line above.
> + dev_dbg(&i2c_imx->adapter.dev, "<%s>\n", __func__);
> +
> + evt_put(&i2c_imx->events, EVT_STOP);
> + wake_up(&i2c_imx->state_queue);
> + }
> +}
> +
> +static void i2c_imx_clear_isr_bit(struct imx_i2c_struct *i2c_imx,
> + unsigned int status)
Alignment should match open parenthesis.
> +{
> + status &= ~I2SR_IIF;
> + status |= (i2c_imx->hwdata->i2sr_clr_opcode & I2SR_IIF);
> + imx_i2c_write_reg(status, i2c_imx, IMX_I2C_I2SR);
> }
>
> +
> +
No new empty lines here please.
> static irqreturn_t i2c_imx_isr(int irq, void *dev_id)
> {
> struct imx_i2c_struct *i2c_imx = dev_id;
> - unsigned int temp;
> + unsigned int status, ctl;
> +
> + status = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2SR);
> + ctl = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2CR);
> + if (status & I2SR_IIF) {
> + i2c_imx_clear_isr_bit(i2c_imx, status);
> +
> + if (ctl & I2CR_MSTA) {
> + dev_dbg(&i2c_imx->adapter.dev, "master interrupt");
> + i2c_imx->master_interrupt = 1;
> + wake_up(&i2c_imx->queue);
> + } else if (status & I2SR_IAL) {
> + evt_put(&i2c_imx->events, EVT_AL);
> + } else {
> + dev_dbg(&i2c_imx->adapter.dev, "slave interrupt");
> + evt_put(&i2c_imx->events, EVT_SI);
> + wake_up(&i2c_imx->state_queue);
> + }
>
> - temp = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2SR);
> - if (temp & I2SR_IIF) {
> - /* save status register */
> - i2c_imx->i2csr = temp;
> - temp &= ~I2SR_IIF;
> - temp |= (i2c_imx->hwdata->i2sr_clr_opcode & I2SR_IIF);
> - imx_i2c_write_reg(temp, i2c_imx, IMX_I2C_I2SR);
> - wake_up(&i2c_imx->queue);
> return IRQ_HANDLED;
> }
>
> @@ -895,7 +1536,13 @@ static int i2c_imx_xfer(struct i2c_adapter *adapter,
>
> /* Start I2C transfer */
> result = i2c_imx_start(i2c_imx);
> - if (result) {
> + if (result == -ETIMEDOUT) {
> + /*
> + * Recovery is not started on arbitration lost, since it can
> + * break slave transfer. But in case of "bus timeout" recovery
> + * it could be useful to bring controller out of "strange state".
> + */
> + dev_dbg(&i2c_imx->adapter.dev, "call bus recovery");
> if (i2c_imx->adapter.bus_recovery_info) {
> i2c_recover_bus(&i2c_imx->adapter);
> result = i2c_imx_start(i2c_imx);
> @@ -1040,6 +1687,10 @@ static u32 i2c_imx_func(struct i2c_adapter *adapter)
> static struct i2c_algorithm i2c_imx_algo = {
> .master_xfer = i2c_imx_xfer,
> .functionality = i2c_imx_func,
> +#ifdef CONFIG_I2C_SLAVE
> + .reg_slave = i2c_imx_reg_slave,
> + .unreg_slave = i2c_imx_unreg_slave,
> +#endif
> };
>
> static int i2c_imx_probe(struct platform_device *pdev)
> @@ -1099,6 +1750,12 @@ static int i2c_imx_probe(struct platform_device *pdev)
> return ret;
> }
>
> + i2c_imx->pinctrl = devm_pinctrl_get(&pdev->dev);
> + if (IS_ERR(i2c_imx->pinctrl)) {
> + ret = PTR_ERR(i2c_imx->pinctrl);
> + goto clk_disable;
> + }
> +
This change is irrelevant to slave support and questionalble, please drop it.
> /* Request IRQ */
> ret = devm_request_irq(&pdev->dev, irq, i2c_imx_isr, 0,
> pdev->name, i2c_imx);
> @@ -1109,6 +1766,7 @@ static int i2c_imx_probe(struct platform_device *pdev)
>
> /* Init queue */
> init_waitqueue_head(&i2c_imx->queue);
> + init_waitqueue_head(&i2c_imx->state_queue);
>
> /* Set up adapter data */
> i2c_set_adapdata(&i2c_imx->adapter, i2c_imx);
> @@ -1160,6 +1818,9 @@ static int i2c_imx_probe(struct platform_device *pdev)
> /* Init DMA config if supported */
> i2c_imx_dma_request(i2c_imx, phy_addr);
>
> + /* init slave_state to IDLE */
> + i2c_imx->state = STATE_IDLE;
> + evt_init(&i2c_imx->events);
Please insert an empty line here to impreove readability.
> return 0; /* Return OK */
>
> rpm_disable:
> @@ -1186,6 +1847,9 @@ static int i2c_imx_remove(struct platform_device *pdev)
> dev_dbg(&i2c_imx->adapter.dev, "adapter removed\n");
> i2c_del_adapter(&i2c_imx->adapter);
>
> + if (i2c_imx->slave_task != NULL)
if (i2c_imx->slave_task)
> + kthread_stop(i2c_imx->slave_task);
> +
> if (i2c_imx->dma)
> i2c_imx_dma_free(i2c_imx);
>
>
The patch contains changes to controller start/stop sequences, this part
is better to separate into another patch, if possible.
Review of more important functional changes, which include changes to the
I2C master functionality, are postponed until we are done with bike shedding.
--
With best wishes,
Vladimir
^ permalink raw reply
* [PATCH] iio: adc: axp288: Drop bogus AXP288_ADC_TS_PIN_CTRL register modifications
From: Hans de Goede @ 2016-12-14 13:55 UTC (permalink / raw)
To: Jonathan Cameron, Chen-Yu Tsai
Cc: Lars-Peter Clausen, Peter Meerwald-Stadler,
russianneuromancer @ ya . ru, linux-iio, linux-i2c, Hans de Goede
For some reason the axp288_adc driver was modifying the
AXP288_ADC_TS_PIN_CTRL register, changing bits 0-1 depending on
whether the GP_ADC channel or another channel was written.
These bits control when a bias current is send to the TS_PIN, the
GP_ADC has its own pin and a separate bit in another register to
control the bias current.
Not only does changing when to enable the TS_PIN bias current
(always or only when sampling) when reading the GP_ADC make no sense
at all, the code is modifying these bits is writing the entire register,
assuming that all the other bits have their default value.
So if the firmware has configured a different bias-current for either
pin, then that change gets clobbered by the write, likewise if the
firmware has set bit 2 to indicate that the battery has no thermal sensor,
this will get clobbered by the write.
This commit fixes all this, by simply removing all writes to the
AXP288_ADC_TS_PIN_CTRL register, they are not needed to read the
GP_ADC pin, and can actually be harmful.
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
drivers/iio/adc/axp288_adc.c | 32 +-------------------------------
1 file changed, 1 insertion(+), 31 deletions(-)
diff --git a/drivers/iio/adc/axp288_adc.c b/drivers/iio/adc/axp288_adc.c
index 7fd2494..64799ad 100644
--- a/drivers/iio/adc/axp288_adc.c
+++ b/drivers/iio/adc/axp288_adc.c
@@ -28,8 +28,6 @@
#include <linux/iio/driver.h>
#define AXP288_ADC_EN_MASK 0xF1
-#define AXP288_ADC_TS_PIN_GPADC 0xF2
-#define AXP288_ADC_TS_PIN_ON 0xF3
enum axp288_adc_id {
AXP288_ADC_TS,
@@ -123,16 +121,6 @@ static int axp288_adc_read_channel(int *val, unsigned long address,
return IIO_VAL_INT;
}
-static int axp288_adc_set_ts(struct regmap *regmap, unsigned int mode,
- unsigned long address)
-{
- /* channels other than GPADC do not need to switch TS pin */
- if (address != AXP288_GP_ADC_H)
- return 0;
-
- return regmap_write(regmap, AXP288_ADC_TS_PIN_CTRL, mode);
-}
-
static int axp288_adc_read_raw(struct iio_dev *indio_dev,
struct iio_chan_spec const *chan,
int *val, int *val2, long mask)
@@ -143,16 +131,7 @@ static int axp288_adc_read_raw(struct iio_dev *indio_dev,
mutex_lock(&indio_dev->mlock);
switch (mask) {
case IIO_CHAN_INFO_RAW:
- if (axp288_adc_set_ts(info->regmap, AXP288_ADC_TS_PIN_GPADC,
- chan->address)) {
- dev_err(&indio_dev->dev, "GPADC mode\n");
- ret = -EINVAL;
- break;
- }
ret = axp288_adc_read_channel(val, chan->address, info->regmap);
- if (axp288_adc_set_ts(info->regmap, AXP288_ADC_TS_PIN_ON,
- chan->address))
- dev_err(&indio_dev->dev, "TS pin restore\n");
break;
default:
ret = -EINVAL;
@@ -162,15 +141,6 @@ static int axp288_adc_read_raw(struct iio_dev *indio_dev,
return ret;
}
-static int axp288_adc_set_state(struct regmap *regmap)
-{
- /* ADC should be always enabled for internal FG to function */
- if (regmap_write(regmap, AXP288_ADC_TS_PIN_CTRL, AXP288_ADC_TS_PIN_ON))
- return -EIO;
-
- return regmap_write(regmap, AXP20X_ADC_EN1, AXP288_ADC_EN_MASK);
-}
-
static const struct iio_info axp288_adc_iio_info = {
.read_raw = &axp288_adc_read_raw,
.driver_module = THIS_MODULE,
@@ -199,7 +169,7 @@ static int axp288_adc_probe(struct platform_device *pdev)
* Set ADC to enabled state at all time, including system suspend.
* otherwise internal fuel gauge functionality may be affected.
*/
- ret = axp288_adc_set_state(axp20x->regmap);
+ ret = regmap_write(info->regmap, AXP20X_ADC_EN1, AXP288_ADC_EN_MASK);
if (ret) {
dev_err(&pdev->dev, "unable to enable ADC device\n");
return ret;
--
2.9.3
^ permalink raw reply related
* [PATCH 5/5] mfd: axp20x: Fix axp288 volatile ranges
From: Hans de Goede @ 2016-12-14 13:52 UTC (permalink / raw)
To: Lee Jones, Chen-Yu Tsai
Cc: russianneuromancer @ ya . ru, linux-i2c, Hans de Goede
In-Reply-To: <20161214135209.16369-1-hdegoede@redhat.com>
The axp288 pmic has a lot more volatile registers then we were
listing in axp288_volatile_ranges, fix this.
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
drivers/mfd/axp20x.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/mfd/axp20x.c b/drivers/mfd/axp20x.c
index a294121..b9c1adf 100644
--- a/drivers/mfd/axp20x.c
+++ b/drivers/mfd/axp20x.c
@@ -117,7 +117,10 @@ static const struct regmap_range axp288_writeable_ranges[] = {
};
static const struct regmap_range axp288_volatile_ranges[] = {
+ regmap_reg_range(AXP20X_PWR_INPUT_STATUS, AXP20X_PWR_OP_MODE),
regmap_reg_range(AXP20X_IRQ1_EN, AXP20X_IPSOUT_V_HIGH_L),
+ regmap_reg_range(AXP22X_GPIO_STATE, AXP22X_GPIO_STATE),
+ regmap_reg_range(AXP20X_FG_RES, AXP288_FG_CC_CAP_REG),
};
static const struct regmap_access_table axp288_writeable_table = {
--
2.9.3
^ permalink raw reply related
* [PATCH 4/5] mfd: axp20x: Drop wrong AXP288_PMIC_ADC_EN define
From: Hans de Goede @ 2016-12-14 13:52 UTC (permalink / raw)
To: Lee Jones, Chen-Yu Tsai
Cc: russianneuromancer @ ya . ru, linux-i2c, Hans de Goede
In-Reply-To: <20161214135209.16369-1-hdegoede@redhat.com>
The adc-enable register for the axp288 is 0x82, not 0x84.
0x82 is already defined as AXP20X_ADC_EN1 and that is what the
axp288_adc driver is actually using, so simply drop the wrong
AXP288_PMIC_ADC_EN define.
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
include/linux/mfd/axp20x.h | 1 -
1 file changed, 1 deletion(-)
diff --git a/include/linux/mfd/axp20x.h b/include/linux/mfd/axp20x.h
index 199cce3..fe93e00 100644
--- a/include/linux/mfd/axp20x.h
+++ b/include/linux/mfd/axp20x.h
@@ -232,7 +232,6 @@ enum {
#define AXP288_PMIC_ADC_H 0x56
#define AXP288_PMIC_ADC_L 0x57
#define AXP288_ADC_TS_PIN_CTRL 0x84
-#define AXP288_PMIC_ADC_EN 0x84
/* Fuel Gauge */
#define AXP288_FG_RDC1_REG 0xba
--
2.9.3
^ permalink raw reply related
* [PATCH 3/5] mfd: axp20x: Fix axp288 PEK_DBR and PEK_DBF irqs being swapped
From: Hans de Goede @ 2016-12-14 13:52 UTC (permalink / raw)
To: Lee Jones, Chen-Yu Tsai
Cc: russianneuromancer @ ya . ru, linux-i2c, Hans de Goede
In-Reply-To: <20161214135209.16369-1-hdegoede@redhat.com>
The R in PEK_DBR stands for rising, so it should be mapped to
AXP288_IRQ_POKP where the last P stands for positive edge.
Likewise PEK_DBF should be mapped to the falling edge, aka the
_N_egative edge, so it should be mapped to AXP288_IRQ_POKN.
This fixes the inverted powerbutton status reporting by the
axp20x-pek driver.
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
drivers/mfd/axp20x.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/mfd/axp20x.c b/drivers/mfd/axp20x.c
index 9a81659..a294121 100644
--- a/drivers/mfd/axp20x.c
+++ b/drivers/mfd/axp20x.c
@@ -205,14 +205,14 @@ static struct resource axp22x_pek_resources[] = {
static struct resource axp288_power_button_resources[] = {
{
.name = "PEK_DBR",
- .start = AXP288_IRQ_POKN,
- .end = AXP288_IRQ_POKN,
+ .start = AXP288_IRQ_POKP,
+ .end = AXP288_IRQ_POKP,
.flags = IORESOURCE_IRQ,
},
{
.name = "PEK_DBF",
- .start = AXP288_IRQ_POKP,
- .end = AXP288_IRQ_POKP,
+ .start = AXP288_IRQ_POKN,
+ .end = AXP288_IRQ_POKN,
.flags = IORESOURCE_IRQ,
},
};
--
2.9.3
^ permalink raw reply related
* [PATCH 2/5] mfd: axp20x: Add missing axp288 irqs
From: Hans de Goede @ 2016-12-14 13:52 UTC (permalink / raw)
To: Lee Jones, Chen-Yu Tsai
Cc: russianneuromancer @ ya . ru, linux-i2c, Hans de Goede
In-Reply-To: <20161214135209.16369-1-hdegoede@redhat.com>
The axp288 has the following irqs 2 times: VBUS_FALL, VBUS_RISE,
VBUS_OV. On boot / reset the enable flags for both the normal and alt
version of these irqs is set.
Since we were only listing the normal version in the axp288 regmap_irq
struct, we were never disabling the alt versions of these irqs.
Add the alt versions to the axp288 regmap_irq struct, so that these
get properly disabled.
Together with the other axp288 fixes in this series, this fixes the axp288
irq contineously triggering.
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
drivers/mfd/axp20x.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/mfd/axp20x.c b/drivers/mfd/axp20x.c
index 3d76941..9a81659 100644
--- a/drivers/mfd/axp20x.c
+++ b/drivers/mfd/axp20x.c
@@ -405,6 +405,9 @@ static const struct regmap_irq axp288_regmap_irqs[] = {
INIT_REGMAP_IRQ(AXP288, VBUS_FALL, 0, 2),
INIT_REGMAP_IRQ(AXP288, VBUS_RISE, 0, 3),
INIT_REGMAP_IRQ(AXP288, OV, 0, 4),
+ INIT_REGMAP_IRQ(AXP288, FALLING_ALT, 0, 5),
+ INIT_REGMAP_IRQ(AXP288, RISING_ALT, 0, 6),
+ INIT_REGMAP_IRQ(AXP288, OV_ALT, 0, 7),
INIT_REGMAP_IRQ(AXP288, DONE, 1, 2),
INIT_REGMAP_IRQ(AXP288, CHARGING, 1, 3),
--
2.9.3
^ permalink raw reply related
* [PATCH 1/5] mfd: axp20x: Use IRQF_TRIGGER_LOW on the axp288
From: Hans de Goede @ 2016-12-14 13:52 UTC (permalink / raw)
To: Lee Jones, Chen-Yu Tsai
Cc: russianneuromancer @ ya . ru, linux-i2c, Hans de Goede
The interrupt line of the entire family of axp2xx pmics is active-low,
for devicetree enumerated irqs, this is dealt with in the devicetree.
ACPI irq resources have a flag field for this too, I tried using this
on my CUBE iwork8 Air tablet, but it does not contain the right data.
The dstd shows the irq listed as either ActiveLow or ActiveHigh,
depending on the OSID variable, which seems to be set by the
"OS IMAGE ID" in the BIOS/EFI setup screen.
Since the acpi-resource info is no good, simply pass in IRQF_TRIGGER_LOW
on the axp288.
Together with the other axp288 fixes in this series, this fixes the axp288
irq contineously triggering.
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
drivers/mfd/axp20x.c | 6 +++---
include/linux/mfd/axp20x.h | 1 +
2 files changed, 4 insertions(+), 3 deletions(-)
diff --git a/drivers/mfd/axp20x.c b/drivers/mfd/axp20x.c
index ba130be..3d76941 100644
--- a/drivers/mfd/axp20x.c
+++ b/drivers/mfd/axp20x.c
@@ -800,6 +800,7 @@ int axp20x_match_device(struct axp20x_dev *axp20x)
axp20x->nr_cells = ARRAY_SIZE(axp288_cells);
axp20x->regmap_cfg = &axp288_regmap_config;
axp20x->regmap_irq_chip = &axp288_regmap_irq_chip;
+ axp20x->irq_flags = IRQF_TRIGGER_LOW;
break;
case AXP806_ID:
axp20x->nr_cells = ARRAY_SIZE(axp806_cells);
@@ -829,9 +830,8 @@ int axp20x_device_probe(struct axp20x_dev *axp20x)
int ret;
ret = regmap_add_irq_chip(axp20x->regmap, axp20x->irq,
- IRQF_ONESHOT | IRQF_SHARED, -1,
- axp20x->regmap_irq_chip,
- &axp20x->regmap_irqc);
+ IRQF_ONESHOT | IRQF_SHARED | axp20x->irq_flags,
+ -1, axp20x->regmap_irq_chip, &axp20x->regmap_irqc);
if (ret) {
dev_err(axp20x->dev, "failed to add irq chip: %d\n", ret);
return ret;
diff --git a/include/linux/mfd/axp20x.h b/include/linux/mfd/axp20x.h
index fec597f..199cce3 100644
--- a/include/linux/mfd/axp20x.h
+++ b/include/linux/mfd/axp20x.h
@@ -517,6 +517,7 @@ enum axp809_irqs {
struct axp20x_dev {
struct device *dev;
int irq;
+ unsigned long irq_flags;
struct regmap *regmap;
struct regmap_irq_chip_data *regmap_irqc;
long variant;
--
2.9.3
^ permalink raw reply related
* Re: [[RFC V3] 1/1] i2c: imx: add slave support
From: Wolfram Sang @ 2016-12-14 13:36 UTC (permalink / raw)
To: Joshua Frkuska; +Cc: linux-i2c, syrchin, peda, vladimir_zapolskiy, Jiada_Wang
In-Reply-To: <1481695291-11444-2-git-send-email-joshua_frkuska@mentor.com>
[-- Attachment #1: Type: text/plain, Size: 475 bytes --]
On Wed, Dec 14, 2016 at 03:01:31PM +0900, Joshua Frkuska wrote:
> Add I2C slave provider using the generic slave interface.
> It also supports master transactions when the slave in the idle mode.
I am confused. In the cover letter you write "Furthermore the state
machine introduced to handle the slave states does not handle the master
mode behavior." Yet here you say it supports master transactions? Is the
sentence from the cover letter outdated?
Regards,
Wolfram
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* Re: [PATCH V5] i2c: designware: fix wrong Tx/Rx FIFO for ACPI
From: Mika Westerberg @ 2016-12-14 11:57 UTC (permalink / raw)
To: Tin Huynh
Cc: Jarkko Nikula, Andy Shevchenko, Wolfram Sang, linux-i2c,
linux-kernel, linux-acpi, Loc Ho, Thang Nguyen, Phong Vo, patches
In-Reply-To: <1481707438-32240-1-git-send-email-tnhuynh@apm.com>
On Wed, Dec 14, 2016 at 04:23:58PM +0700, Tin Huynh wrote:
> ACPI always sets Tx/Rx FIFO to 32. This configuration will
> cause problem if the IP core supports a FIFO size of less than 32.
> The driver should read the FIFO size from the IP and select the smaller
> one of the two.
>
> Signed-off-by: Tin Huynh <tnhuynh@apm.com>
Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.com>
^ permalink raw reply
* Re: [PATCH V5] i2c: designware: fix wrong Tx/Rx FIFO for ACPI
From: Jarkko Nikula @ 2016-12-14 11:20 UTC (permalink / raw)
To: Tin Huynh, Andy Shevchenko, Mika Westerberg, Wolfram Sang
Cc: linux-i2c, linux-kernel, linux-acpi, Loc Ho, Thang Nguyen,
Phong Vo, patches
In-Reply-To: <1481707438-32240-1-git-send-email-tnhuynh@apm.com>
On 12/14/2016 11:23 AM, Tin Huynh wrote:
> ACPI always sets Tx/Rx FIFO to 32. This configuration will
> cause problem if the IP core supports a FIFO size of less than 32.
> The driver should read the FIFO size from the IP and select the smaller
> one of the two.
>
> Signed-off-by: Tin Huynh <tnhuynh@apm.com>
>
> ---
> drivers/i2c/busses/i2c-designware-platdrv.c | 31 ++++++++++++++++++++------
> 1 files changed, 24 insertions(+), 7 deletions(-)
Acked-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>
^ permalink raw reply
* [PATCH V5] i2c: designware: fix wrong Tx/Rx FIFO for ACPI
From: Tin Huynh @ 2016-12-14 9:23 UTC (permalink / raw)
To: Jarkko Nikula, Andy Shevchenko, Mika Westerberg, Wolfram Sang
Cc: linux-i2c, linux-kernel, linux-acpi, Loc Ho, Thang Nguyen,
Phong Vo, patches, Tin Huynh
ACPI always sets Tx/Rx FIFO to 32. This configuration will
cause problem if the IP core supports a FIFO size of less than 32.
The driver should read the FIFO size from the IP and select the smaller
one of the two.
Signed-off-by: Tin Huynh <tnhuynh@apm.com>
---
drivers/i2c/busses/i2c-designware-platdrv.c | 31 ++++++++++++++++++++------
1 files changed, 24 insertions(+), 7 deletions(-)
Change from V4:
-Change else condition and add some comments to clarify new approach.
Change from V3:
-Use uppercase of FIFO instead of lowercase.
-Correct else condition when IP core return 0 of FIFO.
Change from V2:
-Add a helper function to set FIFO size.
Change from V1:
-Revert the default 32 for FIFO, read parameter from IP core
and pick the smaller one of the two.
-Correct the title to describe new approach.
diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c b/drivers/i2c/busses/i2c-designware-platdrv.c
index 0b42a12..682adc3 100644
--- a/drivers/i2c/busses/i2c-designware-platdrv.c
+++ b/drivers/i2c/busses/i2c-designware-platdrv.c
@@ -150,6 +150,29 @@ static int i2c_dw_plat_prepare_clk(struct dw_i2c_dev *i_dev, bool prepare)
return 0;
}
+static void dw_i2c_set_fifo_size(struct dw_i2c_dev *dev, int id)
+{
+ u32 param, tx_fifo_depth, rx_fifo_depth;
+
+ /*
+ * Try to detect the FIFO depth if not set by interface driver,
+ * the depth could be from 2 to 256 from HW spec.
+ */
+ param = i2c_dw_read_comp_param(dev);
+ tx_fifo_depth = ((param >> 16) & 0xff) + 1;
+ rx_fifo_depth = ((param >> 8) & 0xff) + 1;
+ if (!dev->tx_fifo_depth) {
+ dev->tx_fifo_depth = tx_fifo_depth;
+ dev->rx_fifo_depth = rx_fifo_depth;
+ dev->adapter.nr = id;
+ } else if (tx_fifo_depth >= 2) {
+ dev->tx_fifo_depth = min_t(u32, dev->tx_fifo_depth,
+ tx_fifo_depth);
+ dev->rx_fifo_depth = min_t(u32, dev->rx_fifo_depth,
+ rx_fifo_depth);
+ }
+}
+
static int dw_i2c_plat_probe(struct platform_device *pdev)
{
struct dw_i2c_platform_data *pdata = dev_get_platdata(&pdev->dev);
@@ -246,13 +269,7 @@ static int dw_i2c_plat_probe(struct platform_device *pdev)
1000000);
}
- if (!dev->tx_fifo_depth) {
- u32 param1 = i2c_dw_read_comp_param(dev);
-
- dev->tx_fifo_depth = ((param1 >> 16) & 0xff) + 1;
- dev->rx_fifo_depth = ((param1 >> 8) & 0xff) + 1;
- dev->adapter.nr = pdev->id;
- }
+ dw_i2c_set_fifo_size(dev, pdev->id);
adap = &dev->adapter;
adap->owner = THIS_MODULE;
--
1.7.1
^ permalink raw reply related
* Re: [PATCH V4] i2c: designware: fix wrong Tx/Rx FIFO for ACPI
From: Jarkko Nikula @ 2016-12-14 8:59 UTC (permalink / raw)
To: Tin Huynh, Andy Shevchenko
Cc: Mika Westerberg, Wolfram Sang, linux-i2c, linux-kernel,
linux-acpi, Loc Ho, Thang Nguyen, Phong Vo, patches
In-Reply-To: <CANQ9TuCuxsMKzfQ=14ppo-5kWnWXc=VK08ca7+dJL2xG_tkd_Q@mail.gmail.com>
On 12/14/2016 05:20 AM, Tin Huynh wrote:
> On Tue, Dec 13, 2016 at 6:25 PM, Andy Shevchenko
>>> + param = i2c_dw_read_comp_param(dev);
>>> + tx_fifo_depth = ((param >> 16) & 0xff) + 1;
>>> + rx_fifo_depth = ((param >> 8) & 0xff) + 1;
>>> + if (!dev->tx_fifo_depth) {
>>> + dev->tx_fifo_depth = tx_fifo_depth;
>>> + dev->rx_fifo_depth = rx_fifo_depth;
>>> + dev->adapter.nr = id;
>>> + } else if (tx_fifo_depth > 1) {
>>
>> This makes sense now, though I would add a comment here and use >= 2 to
>> reflect datasheet.
>>
>> /*
>> * Choose minimum values between HW and interface
>> * driver provided.
>> */
>>
> I will implement as your comment. However , because adding 1 to the
> value , can i use > 2 or >=3 ?
either > 1 or >= 2 since register value 0x01 from HW means FIFO depth 2
and register value 0x00 is reserved.
--
Jarkko
^ permalink raw reply
* [PATCH V1] i2c: xgene: Fix missing code of DTB support
From: Tin Huynh @ 2016-12-14 7:17 UTC (permalink / raw)
To: Wolfram Sang
Cc: linux-i2c, linux-kernel, Loc Ho, Thang Nguyen, Phong Vo, Duc Dang,
Dien Tien Hoang Nguyen, patches, Tin Huynh
In DTB case, i2c-core doesn't create slave device which is installed
on i2c-xgene bus because of missing code in this driver.
This patch fixes this issue.
Signed-off-by: Tin Huynh <tnhuynh@apm.com>
---
drivers/i2c/busses/i2c-xgene-slimpro.c | 1 +
1 files changed, 1 insertions(+), 0 deletions(-)
diff --git a/drivers/i2c/busses/i2c-xgene-slimpro.c b/drivers/i2c/busses/i2c-xgene-slimpro.c
index 263685c..00d1632 100644
--- a/drivers/i2c/busses/i2c-xgene-slimpro.c
+++ b/drivers/i2c/busses/i2c-xgene-slimpro.c
@@ -415,6 +415,7 @@ static int xgene_slimpro_i2c_probe(struct platform_device *pdev)
adapter->algo = &xgene_slimpro_i2c_algorithm;
adapter->class = I2C_CLASS_HWMON;
adapter->dev.parent = &pdev->dev;
+ adapter->dev.of_node = pdev->dev.of_node;
i2c_set_adapdata(adapter, ctx);
rc = i2c_add_adapter(adapter);
if (rc) {
--
1.7.1
^ permalink raw reply related
* [[RFC V3] 0/1] i2c: imx: add slave support
From: Joshua Frkuska @ 2016-12-14 6:01 UTC (permalink / raw)
To: linux-i2c
Cc: joshua_frkuska, wsa, syrchin, peda, vladimir_zapolskiy,
Jiada_Wang
This patch was tested on i.MX6q sabresd and sabreai development boards. It is a continuation of the work started in the following thread https://www.spinics.net/lists/linux-i2c/msg27340.html
Its purpose is to introduce i2c slave and multimaster capabilities to the imx i2c controller driver. Slave mode can be tested using the i2c test EEPROM i2c slave device available in tree. Multimaster can be tested by creating a bus with 1 or more slave devices with 2 masters. For the purpose of this test, a sabreai and sabresd board were hooked together to form a multimaster bus. In this configuration, slave and multimaster configurations can alternatively be stress tested.
Some points to note:
The patch considers when I2C_SLAVE support is not enabled but introduces unused data structs and members even in this case. This can still probably be cleaned up further but it is left this way to reduce the number of conditional code in the driver.
Furthermore the state machine introduced to handle the slave states does not handle the master mode behavior. This is also done in order to allow an evolutionary change to the driver. A future update can integrate the master entirely into the statemachine.
Any constructive comments would be greatly appreciated.
Thank you
Joshua Frkuska (1):
i2c: imx: add slave support
drivers/i2c/busses/i2c-imx.c | 724 +++++++++++++++++++++++++++++++++++++++++--
1 file changed, 694 insertions(+), 30 deletions(-)
--
2.5.5
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox