Re: [MeeGo-Dev][PATCH] Topcliff: Update PCH_I2C driver to 2.6.35
2010/9/1 Masayuki Ohtak :
> I2C driver of Topcliff PCH
> (...)
> +++ b/drivers/i2c/busses/i2c-pch.c
> (...)
> +/**
> + * pch_wait_for_bus_idle() - check the status of bus.
> + * @adap: Pointer to struct i2c_algo_pch_data.
> + * @timeout: waiting time counter (us).
> + */
> +static s32 pch_wait_for_bus_idle(struct i2c_algo_pch_data *adap,
> + s32 timeout)
> +{
> + void __iomem *p = adap->pch_base_address;
> +
> + /* MAX timeout value is timeout*1000*1000nsec */
> + ktime_t ns_val = ktime_add_ns(ktime_get(), timeout*1000*1000);
> + do {
> + if ((ioread32(p + PCH_I2CSR) & I2CMBB_BIT) == 0)
> + break;
> + msleep(1);
> + } while (ktime_lt(ktime_get(), ns_val));
> +
> + dev_dbg(adap->pch_adapter.dev.parent,
> + "%s : I2CSR = %x\n", __func__, ioread32(p +
> PCH_I2CSR));
> +
> + if (timeout == 0) {
> + dev_err(adap->pch_adapter.dev.parent,
> + "%s :return%d\n", __func__, -ETIME);
Why not just return -ETIME; here?
> + } else {
> + dev_dbg(adap->pch_adapter.dev.parent,
> + "%s : return %d\n", __func__, 0);
> + }
Delete this else clause, who is interested in return 0???
> + return ((timeout <= 0) ? (-ETIME) : (0));
return 0;
> (...)
> +/**
> + * pch_wait_for_xfer_complete() - initiates a wait for the tx complete event
> + * @adap: Pointer to struct i2c_algo_pch_data.
> + */
> +static s32 pch_wait_for_xfer_complete(struct i2c_algo_pch_data *adap)
> +{
> + s32 ret;
> + ret = wait_event_interruptible_timeout(pch_event,
> + (adap->pch_event_flag != 0), msecs_to_jiffies(50));
> + if (ret < 0)
> + goto out;
The goto construction is unnecessary, just
return ret;
> +
> + if (ret == 0) {
> + ret = -ETIMEDOUT;
> + goto out;
return -ETIMEDOUT;
> + }
> +
> + if (adap->pch_event_flag & I2C_ERROR_MASK) {
> + ret = -EIO;
> + dev_err(adap->pch_adapter.dev.parent,
> + "error bits set: %x\n", adap->pch_event_flag);
> + goto out;
Skip ret assignment
return -EIO;
> + }
> +
> + adap->pch_event_flag = 0;
> + ret = 0;
Skip this
> +out:
> + return ret;
return 0;
> +}
> (...)
> +/**
> + * pch_getack() - to confirm ACK/NACK
> + * @adap: Pointer to struct i2c_algo_pch_data.
> + */
> +static s32 pch_getack(struct i2c_algo_pch_data *adap)
> +{
> + u32 reg_val;
> + void __iomem *p = adap->pch_base_address;
> + reg_val = ioread32(p + PCH_I2CSR) & PCH_GETACK;
> +
> + if (reg_val == 0)
> + dev_dbg(adap->pch_adapter.dev.parent, "%s : return 0\n",
> + __func__);
> + else
> + dev_dbg(adap->pch_adapter.dev.parent, "%s : return%d\n",
> + __func__, -EPROTO);
> +
> + return (((reg_val) == 0) ? (0) : (-EPROTO));
Refactor this like the other function, no weirdo debug prints
return 0;
> +}
> (...)
> +/**
> + * pch_writebytes() - write data to I2C bus in normal mode
> + * @i2c_adap: Pointer to the struct i2c_adapter.
> + * @last: specifies whether last message or not.
> + * In the case of compound mode it will be 1 for last message,
> + * otherwise 0.
> + * @first: specifies whether first message or not.
> + * 1 for first message otherwise 0.
> + */
> +static s32 pch_writebytes(struct i2c_adapter *i2c_adap, struct i2c_msg *msgs,
> + u32 last, u32 first)
> +{
> + struct i2c_algo_pch_data *adap = i2c_adap->algo_data;
> + u8 *buf;
> + u32 length;
> + u32 addr;
> + u32 addr_2_msb;
> + u32 addr_8_lsb;
> + s32 wrcount;
You don't assign anything to wrcount...
> + void __iomem *p = adap->pch_base_address;
> + length = msgs->len;
> + buf = msgs->buf;
> + addr = msgs->addr;
> + /* enable master tx */
> + pch_setbit((adap->pch_base_address), PCH_I2CCTL, I2C_TX_MODE);
> +
> + dev_dbg(adap->pch_adapter.dev.parent,
> + "%s : I2CCTL = %x msgs->len = %d\n", __func__,
> + ioread32(p + PCH_I2CCTL), length);
> +
> + if (first) {
> + if (pch_wait_for_bus_idle(adap, BUS_IDLE_TIMEOUT) == -ETIME)
> + return -ETIME;
> + }
> +
> + if (msgs->flags & I2C_M_TEN) {
> + addr_2_msb = ((addr & I2C_MSB_2B_MSK) >> 7);
> + iowrite32(addr_2_msb | TEN_BIT_ADDR_MASK, p + PCH_I2CDR);
> + if (first)
> + pch_start(adap);
> + if ((pch_wait_for_xfer_complete(adap) == 0) &&
> + (pch_getack(adap) == 0)) {
> +
RE: [PATCH 1/1] power supply: add driver for TI BQ20Z75 gas gauge IC
Thanks for the review, fixed up what you suggested, I did leave one place as
-EINVAL as it seemed the most appropriate for the spot. Will repost soon.
-Original Message-
From: Jean Delvare [mailto:kh...@linux-fr.org]
Sent: Tuesday, August 31, 2010 1:24 AM
To: Rhyland Klein
Cc: cbouatmai...@gmail.com; linux-i2c@vger.kernel.org;
linux-ker...@vger.kernel.org; o...@lixom.net; Andrew Chew
Subject: Re: [PATCH 1/1] power supply: add driver for TI BQ20Z75 gas gauge IC
Hi Rhyland,
On Fri, 27 Aug 2010 15:50:43 -0700, rkl...@nvidia.com wrote:
> From: Rhyland Klein
>
> this driver depends on I2C and uses SMBUS for communication with the host.
Quick review (I am not familiar with the power supply API):
> Signed-off-by: Rhyland Klein
> ---
> drivers/power/Kconfig |7 +
> drivers/power/Makefile |1 +
> drivers/power/bq20z75.c | 403
> +++
> 3 files changed, 411 insertions(+), 0 deletions(-)
> create mode 100644 drivers/power/bq20z75.c
>
> diff --git a/drivers/power/Kconfig b/drivers/power/Kconfig
> index 8e9ba17..c72e628 100644
> --- a/drivers/power/Kconfig
> +++ b/drivers/power/Kconfig
> @@ -142,4 +142,11 @@ config CHARGER_PCF50633
> help
>Say Y to include support for NXP PCF50633 Main Battery Charger.
>
> +config BQ20Z75_GASGAUGE
Following the naming used for other options in this menu, this would be
BATTERY_BQ20Z75.
-Done
> +tristate "TI BQ20Z75 GAS GAUGE"
Not all capitals, please.
-Fixed, changed to "TI BQ20z75 gas gauge"
> +depends on I2C
> +help
> + Say Y to include support for TI BQ20Z75 SBS-compliant
> + gas gauge and protection IC.
> +
> endif # POWER_SUPPLY
> diff --git a/drivers/power/Makefile b/drivers/power/Makefile
> index 0005080..98abc31 100644
> --- a/drivers/power/Makefile
> +++ b/drivers/power/Makefile
> @@ -34,3 +34,4 @@ obj-$(CONFIG_BATTERY_DA9030)+= da9030_battery.o
> obj-$(CONFIG_BATTERY_MAX17040) += max17040_battery.o
> obj-$(CONFIG_BATTERY_Z2) += z2_battery.o
> obj-$(CONFIG_CHARGER_PCF50633) += pcf50633-charger.o
> +obj-$(CONFIG_BQ20Z75_GASGAUGE) += bq20z75.o
> diff --git a/drivers/power/bq20z75.c b/drivers/power/bq20z75.c
> new file mode 100644
> index 000..4eb6638
> --- /dev/null
> +++ b/drivers/power/bq20z75.c
> @@ -0,0 +1,403 @@
> +/*
> + * drivers/power/bq20z75.c
Please remove, this doesn't add much value and is likely to get
outdated at some point.
-Done
> + *
> + * Gas Gauge driver for TI's BQ20Z75
> + *
> + * Copyright (c) 2010, NVIDIA 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.
> + *
> + * You should have received a copy of the GNU General Public License along
> + * with this program; if not, write to the Free Software Foundation, Inc.,
> + * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA.
> + */
> +
> +#include
> +#include
> +#include
> +#include
> +#include
I don't think you need this one?
-Removed.
> +#include
> +#include
> +#include
> +
> +enum {
> + REG_MANUFACTURER_DATA,
> + REG_TEMPERATURE,
> + REG_VOLTAGE,
> + REG_CURRENT,
> + REG_CAPACITY,
> + REG_TIME_TO_EMPTY,
> + REG_TIME_TO_FULL,
> + REG_STATUS,
> + REG_CYCLE_COUNT,
> + REG_SERIAL_NUMBER,
> + REG_MAX
> +};
> +
> +#define BATTERY_MANUFACTURER_SIZE12
> +#define BATTERY_NAME_SIZE8
You don't use these anywhere?
-Removed
> +
> +/* manufacturer access defines */
> +#define MANUFACTURER_ACCESS_STATUS 0x0006
> +#define MANUFACTURER_ACCESS_SLEEP0x0011
> +
> +/* battery status value bits */
> +#define BATTERY_CHARGING 0x40
> +#define BATTERY_FULL_CHARGED 0x20
> +#define BATTERY_FULL_DISCHARGED 0x10
> +
> +#define BQ20Z75_DATA(_psp, _addr, _min_value, _max_value) { \
> + .psp = _psp, \
> + .addr = _addr, \
> + .min_value = _min_value, \
> + .max_value = _max_value, \
> +}
> +
> +static struct bq20z75_device_data {
This could be const.
-Done
> + enum power_supply_property psp;
> + u8 addr;
> + int min_value;
> + int max_value;
> +} bq20z75_data[] = {
> + [REG_MANUFACTURER_DATA] =
> + BQ20Z75_DATA(POWER_SUPPLY_PROP_PRESENT, 0x00, 0, 65535),
> + [REG_TEMPERATURE] =
> + BQ20Z75_DATA(POWER_SUPPLY_PROP_TEMP, 0x08, 0, 65535),
> + [REG_VOLTAGE] =
> + BQ20Z75_DATA(POWER_SUPPLY_PROP_VOLTAGE_NOW, 0x09, 0, 2),
> + [REG_CURRENT] =
> + BQ20Z75_DATA(POWER_SUPP
RE: [PATCH 1/1] power supply: add driver for TI BQ20Z75 gas gauge IC
Thanks for the review, fixed up what you suggested. Will repost soon.
-Original Message-
From: Mark Brown [mailto:broo...@opensource.wolfsonmicro.com]
Sent: Tuesday, August 31, 2010 6:13 AM
To: Rhyland Klein
Cc: cbouatmai...@gmail.com; linux-i2c@vger.kernel.org;
linux-ker...@vger.kernel.org; o...@lixom.net; Andrew Chew
Subject: Re: [PATCH 1/1] power supply: add driver for TI BQ20Z75 gas gauge IC
On Fri, Aug 27, 2010 at 03:50:43PM -0700, rkl...@nvidia.com wrote:
> + case POWER_SUPPLY_PROP_STATUS:
> + case POWER_SUPPLY_PROP_CYCLE_COUNT:
> + case POWER_SUPPLY_PROP_VOLTAGE_NOW:
> + case POWER_SUPPLY_PROP_CURRENT_NOW:
> + case POWER_SUPPLY_PROP_TEMP:
> + case POWER_SUPPLY_PROP_TIME_TO_EMPTY_AVG:
> + case POWER_SUPPLY_PROP_TIME_TO_FULL_AVG:
> + case POWER_SUPPLY_PROP_SERIAL_NUMBER:
> + for (count = 0; count < REG_MAX; count++) {
> + if (psp == bq20z75_data[count].psp)
> + break;
> + }
Rather than using REG_MAX it'd seem safer to use ARRAY_SIZE() to make
sure you're within the array.
-Done
> + if (bq20z75_get_battery_property(count, psp, val))
> + return -EINVAL;
> + break;
Indentation is messed up here.
-Done
> + printk(KERN_INFO "%s: property = %d, value = %d\n", __func__,
> + psp, val->intval);
Remove unconditional logging like this, make it dev_dbg() if you want to
keep it in so it's only visible when explicitly requested.
-Done
> + bq20z75_device = kzalloc(sizeof(*bq20z75_device), GFP_KERNEL);
> + if (!bq20z75_device)
> + return -ENOMEM;
> +
> +memset(bq20z75_device, 0, sizeof(*bq20z75_device));
Indentation again.
-Done
> +/* any smbus transaction will wake up bq20z75 */
> +static int bq20z75_resume(struct i2c_client *client)
> +{
> + return 0;
> +}
No need for null functions like this.
-Done
> +#if defined CONFIG_PM
> + .suspend= bq20z75_suspend,
> + .resume = bq20z75_resume,
> +#endif
The standard way to do this is with the suspend/resume functions -
#define them to NULL which turns their assignments into noops.
-Done
> +MODULE_AUTHOR("NVIDIA Graphics Pvt Ltd");
This only makes sense if it's an actual person (or other contact
address); there's already copyright statements on the file, the author
information is more there to help find someone who might know something
about the driver.
-Done
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] I2C: Fix for suspend/resume issue in i2c-core
On Wed, 1 Sep 2010 01:12:11 +0200, Rafael J. Wysocki wrote:
> On Tuesday, August 31, 2010, Mark Brown wrote:
> > On Mon, Aug 30, 2010 at 11:43:23AM -0700, Kevin Hilman wrote:
> > > Vishwanath BS writes:
> > >
> > > > In current i2c core driver, pm_runtime_set_active call from
> > > > i2c_device_pm_resume
> > > > is not balanced by pm_runtime_set_suspended call from
> > > > i2c_device_pm_suspend.
> > > > pm_runtime_set_active called from resume path will increase the
> > > > child_count of
> > > > the device's parent. However, matching pm_runtime_set_suspended is not
> > > > called
> > > > in suspend routine because of which child_count of the device's parent
> > > > is not balanced, preventing the parent device to idle.
> > > > Issue has been fixed by adding pm_runtime_set_suspended call inside
> > > > suspend
> > > > reoutine which will make sure that child_counts are balanced.
> > > > This fix has been tested on OMAP4430.
> > > >
> > > > Signed-off-by: Partha Basak
> > > > Signed-off-by: Vishwanath BS
> > > >
> > > > Cc: Rafael J. Wysocki
> > > > Cc: Kevin Hilman
> > > > Cc: Ben Dooks
> > >
> > > Also Cc'ing Mark Brown as original author of runtime PM for i2-core.
> >
> > Also Jean Delvare who maintains the I2C core. To be honest Rafael did
> > all the actual work here (and has since rewritten the code anyway).
>
> Sorry for the delay.
>
> The fix looks reasonable to me.
I take this as an Acked-by. Patch applied, thanks.
>
> > > > ---
> > > > drivers/i2c/i2c-core.c | 12 ++--
> > > > 1 files changed, 10 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
> > > > index 6649176..3146bff
> > > > --- a/drivers/i2c/i2c-core.c
> > > > +++ b/drivers/i2c/i2c-core.c
> > > > @@ -196,14 +196,22 @@ static int i2c_legacy_resume(struct device *dev)
> > > > static int i2c_device_pm_suspend(struct device *dev)
> > > > {
> > > > const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm :
> > > > NULL;
> > > > + int ret;
> > > >
> > > > if (pm_runtime_suspended(dev))
> > > > return 0;
> > > >
> > > > if (pm)
> > > > - return pm->suspend ? pm->suspend(dev) : 0;
> > > > + ret = pm->suspend ? pm->suspend(dev) : 0;
> > > > + else
> > > > + ret = i2c_legacy_suspend(dev, PMSG_SUSPEND);
> > > >
> > > > - return i2c_legacy_suspend(dev, PMSG_SUSPEND);
> > > > + if (!ret) {
> > > > + pm_runtime_disable(dev);
> > > > + pm_runtime_set_suspended(dev);
> > > > + pm_runtime_enable(dev);
> > > > + }
> > > > + return ret;
> > > > }
> > > >
> > > > static int i2c_device_pm_resume(struct device *dev)
> >
> >
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
Jean Delvare
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
