| Message ID | 1328697985-22504-1-git-send-email-ldewangan@nvidia.com |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
On Wed, Feb 08, 2012 at 04:16:24PM +0530, Laxman Dewangan wrote: > +static bool is_volatile_reg(struct device *dev, unsigned int reg) > +{ > + struct tps65910 *tps65910 = dev_get_drvdata(dev); > + return test_bit(reg, tps65910->cache_reg) ? false : true; > +} This is *really* odd. Why is this not static data (or mostly static data), why does it vary at runtime? > +static bool regmap_volatile_range(struct tps65910 *tps65910, > + unsigned int reg, unsigned int bytes) > +{ > + unsigned int i; > + for (i = 0; i < bytes; i++) > + if (!is_volatile_reg(tps65910->dev, reg + i)) > + return false; > + return true; > +} I don't think this should be here (the naming is a bit of a clue - it's named like a core function), see below where you're using it... > + unsigned char *wbuf = src; > + unsigned int ival; > + > + if (regmap_volatile_range(tps65910, reg, bytes)) > + return regmap_raw_write(tps65910->regmap, reg, src, bytes); > + > + /* If any of register is non-volatile then use byte-wise transfer */ > + for (i = 0; i < bytes; ++i) { > + ival = (unsigned int) (*wbuf++); > + ret = regmap_write(tps65910->regmap, reg, ival); > + if (ret < 0) > + return ret; > + } There's nothing specific to the driver about this, if this is a good idea add support for it to the core. > +void tps65910_enable_reg_cache(struct tps65910 *tps65910, int reg) > +{ > + set_bit(reg, tps65910->cache_reg); > +} > +EXPORT_SYMBOL_GPL(tps65910_enable_reg_cache); Why are you doing this? This looks very icky, and if it is needed there needs to be more code here to make sure the register cache and device are in sync.
Thanks for quick review. On Wednesday 08 February 2012 05:11 PM, Mark Brown wrote: > * PGP Signed by an unknown key > > On Wed, Feb 08, 2012 at 04:16:24PM +0530, Laxman Dewangan wrote: > >> +static bool is_volatile_reg(struct device *dev, unsigned int reg) >> +{ >> + struct tps65910 *tps65910 = dev_get_drvdata(dev); >> + return test_bit(reg, tps65910->cache_reg) ? false : true; >> +} > This is *really* odd. Why is this not static data (or mostly static > data), why does it vary at runtime? > I did not wanted to make the list of register in core driver. Wanted to leave the decision to the sub-devices driver where they need to enable cache based on their requirements. Do you think that the register list (although it is used in the regulator driver) should be in the core file? If this is allow then I can make the static table in core driver. >> +static bool regmap_volatile_range(struct tps65910 *tps65910, >> + unsigned int reg, unsigned int bytes) >> +{ >> + unsigned int i; >> + for (i = 0; i< bytes; i++) >> + if (!is_volatile_reg(tps65910->dev, reg + i)) >> + return false; >> + return true; >> +} > I don't think this should be here (the naming is a bit of a clue - it's > named like a core function), see below where you're using it... > Oops, the function is copied from the regmap and so this error... sorry.. There is no core function where bulk_write is happening on which some of register is cached. >> + unsigned char *wbuf = src; >> + unsigned int ival; >> + >> + if (regmap_volatile_range(tps65910, reg, bytes)) >> + return regmap_raw_write(tps65910->regmap, reg, src, bytes); >> + >> + /* If any of register is non-volatile then use byte-wise transfer */ >> + for (i = 0; i< bytes; ++i) { >> + ival = (unsigned int) (*wbuf++); >> + ret = regmap_write(tps65910->regmap, reg, ival); >> + if (ret< 0) >> + return ret; >> + } > There's nothing specific to the driver about this, if this is a good > idea add support for it to the core. > This function added because there is no bulk_write function in core driver which supports the non-volatile in the list. Even if number of bytes read is 1. Should we move the above logic to core driver? - If any of the register is non-volatile in bulk write then split the transfer into the byte-wise/short-wise/long-wise (format.val_bytes) based on register width? - If all register is volatile the uses the regmap_raw_write() Does it sounds reasonable? If yes then I can move this code to regmap.c as regmap_bulk_write() i.e. new function. >> +void tps65910_enable_reg_cache(struct tps65910 *tps65910, int reg) >> +{ >> + set_bit(reg, tps65910->cache_reg); >> +} >> +EXPORT_SYMBOL_GPL(tps65910_enable_reg_cache); > Why are you doing this? This looks very icky, and if it is needed there > needs to be more code here to make sure the register cache and device > are in sync. I think If register are volatile then read/write happen to device only i.e. without caching. There is no entry for such register in cache table. Once this is cache enable, new entry is created on cache and the first value for the cache-val is read from device. So it will be in sync. All read/writes are io-locked in regmap and so not seeing any issue here. > * Unknown Key > * 0x6E30FDDD -- To unsubscribe from this list: send the line "unsubscribe linux-tegra" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Feb 08, 2012 at 05:45:43PM +0530, Laxman Dewangan wrote: > On Wednesday 08 February 2012 05:11 PM, Mark Brown wrote: > >This is *really* odd. Why is this not static data (or mostly static > >data), why does it vary at runtime? > I did not wanted to make the list of register in core driver. Wanted > to leave the decision to the sub-devices driver where they need to > enable cache based on their requirements. > Do you think that the register list (although it is used in the > regulator driver) should be in the core file? If this is allow then > I can make the static table in core driver. Yes, it should be in the core driver. > >>+ /* If any of register is non-volatile then use byte-wise transfer */ > >>+ for (i = 0; i< bytes; ++i) { > >>+ ival = (unsigned int) (*wbuf++); > >>+ ret = regmap_write(tps65910->regmap, reg, ival); > >>+ if (ret< 0) > >>+ return ret; > >>+ } > >There's nothing specific to the driver about this, if this is a good > >idea add support for it to the core. > This function added because there is no bulk_write function in core > driver which supports the non-volatile in the list. Even if number > of bytes read is 1. > Should we move the above logic to core driver? This is the core driver? If you mean the regmap core then yes. > - If any of the register is non-volatile in bulk write then split > the transfer into the byte-wise/short-wise/long-wise > (format.val_bytes) based on register width? > - If all register is volatile the uses the regmap_raw_write() > Does it sounds reasonable? If yes then I can move this code to > regmap.c as regmap_bulk_write() i.e. new function. Yes, though bulk_write() is tricky as it's *really* unclear what it should take as an argument - should it be raw register size (in which case it's just raw_write()) or should it be ints (in which case it needs to repack the data too)? I suspect ints but I'm really not convinced there's much use case for this.
On Wednesday 08 February 2012 06:37 PM, Mark Brown wrote: > * PGP Signed by an unknown key > > On Wed, Feb 08, 2012 at 05:45:43PM +0530, Laxman Dewangan wrote: > >> I did not wanted to make the list of register in core driver. Wanted >> to leave the decision to the sub-devices driver where they need to >> enable cache based on their requirements. >> Do you think that the register list (although it is used in the >> regulator driver) should be in the core file? If this is allow then >> I can make the static table in core driver. > Yes, it should be in the core driver. > Fine, it will much simple in this case. > >> This function added because there is no bulk_write function in core >> driver which supports the non-volatile in the list. Even if number >> of bytes read is 1. >> Should we move the above logic to core driver? > This is the core driver? If you mean the regmap core then yes. > Yes, change regmap core driver i.e. regmap.c >> - If any of the register is non-volatile in bulk write then split >> the transfer into the byte-wise/short-wise/long-wise >> (format.val_bytes) based on register width? >> - If all register is volatile the uses the regmap_raw_write() >> Does it sounds reasonable? If yes then I can move this code to >> regmap.c as regmap_bulk_write() i.e. new function. > Yes, though bulk_write() is tricky as it's *really* unclear what it > should take as an argument - should it be raw register size (in which > case it's just raw_write()) or should it be ints (in which case it needs > to repack the data too)? I suspect ints but I'm really not convinced > there's much use case for this. > * @map: Register map to write to * @reg: Initial register to write to * @val: Block of data to be written, laid out for direct transmission to the * device * @@val_count: Number of registers to write int regmap_bulk_write(struct regmap *map, unsigned int reg, void *val, size_t val_count) only support if map->format.parse_val not null like bulk_read. It will just do the regamp_raw_write() if all regs are volatile otherwise make the unsigned int from the val by function map->format.parse_val for separate write for each register. > * Unknown Key > * 0x6E30FDDD -- To unsubscribe from this list: send the line "unsubscribe linux-tegra" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Feb 08, 2012 at 07:04:14PM +0530, Laxman Dewangan wrote: > On Wednesday 08 February 2012 06:37 PM, Mark Brown wrote: > >Yes, though bulk_write() is tricky as it's *really* unclear what it > >should take as an argument - should it be raw register size (in which > >case it's just raw_write()) or should it be ints (in which case it needs > >to repack the data too)? I suspect ints but I'm really not convinced > >there's much use case for this. > * @map: Register map to write to > * @reg: Initial register to write to > * @val: Block of data to be written, laid out for direct transmission to the > * device > * @@val_count: Number of registers to write > int regmap_bulk_write(struct regmap *map, unsigned int reg, void *val, > size_t val_count) > only support if map->format.parse_val not null like bulk_read. > It will just do the regamp_raw_write() if all regs are volatile > otherwise make the unsigned int from the val by function > map->format.parse_val for separate write for each register. But that's just raw_write(), there's no benefit to the additional API call.
On Wednesday 08 February 2012 07:28 PM, Mark Brown wrote: > * PGP Signed by an unknown key > > On Wed, Feb 08, 2012 at 07:04:14PM +0530, Laxman Dewangan wrote: >> On Wednesday 08 February 2012 06:37 PM, Mark Brown wrote: >>> Yes, though bulk_write() is tricky as it's *really* unclear what it >>> should take as an argument - should it be raw register size (in which >>> case it's just raw_write()) or should it be ints (in which case it needs >>> to repack the data too)? I suspect ints but I'm really not convinced >>> there's much use case for this. >> * @map: Register map to write to >> * @reg: Initial register to write to >> * @val: Block of data to be written, laid out for direct transmission to the >> * device >> * @@val_count: Number of registers to write >> int regmap_bulk_write(struct regmap *map, unsigned int reg, void *val, >> size_t val_count) >> only support if map->format.parse_val not null like bulk_read. >> It will just do the regamp_raw_write() if all regs are volatile >> otherwise make the unsigned int from the val by function >> map->format.parse_val for separate write for each register. > But that's just raw_write(), there's no benefit to the additional API > call. > raw_write supported for the volatile register only. I require an api which can work for volatile as well as non-volatile registers. Either extend raw_write to support both cases (remove warning and handle properly) or add bulk_write. I think as bulk_read is there and so having bulk_write on same line should be OK. > * Unknown Key > * 0x6E30FDDD -- To unsubscribe from this list: send the line "unsubscribe linux-tegra" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Feb 09, 2012 at 10:33:01AM +0530, Laxman Dewangan wrote: > On Wednesday 08 February 2012 07:28 PM, Mark Brown wrote: > >But that's just raw_write(), there's no benefit to the additional API > >call. > raw_write supported for the volatile register only. That's an implementation detail since there's little if any use case for such use, the main use of raw_write() is dumping blocks of data like firmwares and coefficients which are rarely cached. > I require an api which can work for volatile as well as non-volatile > registers. > Either extend raw_write to support both cases (remove warning and > handle properly) or add bulk_write. > I think as bulk_read is there and so having bulk_write on same line > should be OK. Note that there's a very important difference between the bulk and raw versions of the API which you're not taking into account: the bulk version takes care of endianness differences between the device and the host. This would mean that for send we'd need to rewrite the input array which means we have to take a copy of the data as we probably shouldn't be changing the input data. Indeed, looking at the drivers in mainline I can't actually see any of them which do block writes.
On Thursday 09 February 2012 05:25 PM, Mark Brown wrote: > * PGP Signed by an unknown key > > On Thu, Feb 09, 2012 at 10:33:01AM +0530, Laxman Dewangan wrote: >> On Wednesday 08 February 2012 07:28 PM, Mark Brown wrote: >>> But that's just raw_write(), there's no benefit to the additional API >>> call. >> raw_write supported for the volatile register only. > > Note that there's a very important difference between the bulk and raw > versions of the API which you're not taking into account: the bulk > version takes care of endianness differences between the device and the > host. This would mean that for send we'd need to rewrite the input > array which means we have to take a copy of the data as we probably > shouldn't be changing the input data. > Yes, I am taking care of this. Probably my code will explain this :) I have my patch ready. Let me send this.. > Indeed, looking at the drivers in mainline I can't actually see any of > them which do block writes. > I am working on mfd/tps65910.c and function is: static int tps65910_i2c_write(struct tps65910 *tps65910, u8 reg, int bytes, void *src) All your feedback will help me to improve my next two mfd/pmic drivers which I am planning to upstream... > * Unknown Key > * 0x6E30FDDD -- To unsubscribe from this list: send the line "unsubscribe linux-tegra" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Feb 09, 2012 at 05:29:38PM +0530, Laxman Dewangan wrote: > I am working on mfd/tps65910.c and function is: > static int tps65910_i2c_write(struct tps65910 *tps65910, u8 reg, > int bytes, void *src) > All your feedback will help me to improve my next two mfd/pmic > drivers which I am planning to upstream... Yeah, I know that's the i2c_write() function but all the callers I noticed were calling it with bytes set to 1.
On Thursday 09 February 2012 05:33 PM, Mark Brown wrote: > * PGP Signed by an unknown key > > On Thu, Feb 09, 2012 at 05:29:38PM +0530, Laxman Dewangan wrote: > >> I am working on mfd/tps65910.c and function is: >> static int tps65910_i2c_write(struct tps65910 *tps65910, u8 reg, >> int bytes, void *src) >> All your feedback will help me to improve my next two mfd/pmic >> drivers which I am planning to upstream... > Yeah, I know that's the i2c_write() function but all the callers I > noticed were calling it with bytes set to 1. > Yes, currently all callers are calling with 1 byte but when we will add rtc (which is not there currently), at that time, it may call multibyte read/write. The interface is such that it can support multi-byte transfer. And when using for single byte transfer (currently) and if it is cached, regmap_raw_write() is creating warnings and that's why I broken transfer in multi-transfer in tps65910.c which was not a good idea.. > * Unknown Key > * 0x6E30FDDD -- To unsubscribe from this list: send the line "unsubscribe linux-tegra" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig index cd13e9f..0e066cd 100644 --- a/drivers/mfd/Kconfig +++ b/drivers/mfd/Kconfig @@ -162,6 +162,7 @@ config MFD_TPS65910 depends on I2C=y && GPIOLIB select MFD_CORE select GPIO_TPS65910 + select REGMAP_I2C help if you say yes here you get support for the TPS65910 series of Power Management chips. diff --git a/drivers/mfd/tps65910.c b/drivers/mfd/tps65910.c index 01cf501..1f2b004 100644 --- a/drivers/mfd/tps65910.c +++ b/drivers/mfd/tps65910.c @@ -16,10 +16,12 @@ #include <linux/module.h> #include <linux/moduleparam.h> #include <linux/init.h> +#include <linux/err.h> #include <linux/slab.h> #include <linux/i2c.h> #include <linux/gpio.h> #include <linux/mfd/core.h> +#include <linux/regmap.h> #include <linux/mfd/tps65910.h> static struct mfd_cell tps65910s[] = { @@ -34,103 +36,76 @@ static struct mfd_cell tps65910s[] = { }, }; +static bool is_volatile_reg(struct device *dev, unsigned int reg) +{ + struct tps65910 *tps65910 = dev_get_drvdata(dev); + return test_bit(reg, tps65910->cache_reg) ? false : true; +} + +static bool regmap_volatile_range(struct tps65910 *tps65910, + unsigned int reg, unsigned int bytes) +{ + unsigned int i; + for (i = 0; i < bytes; i++) + if (!is_volatile_reg(tps65910->dev, reg + i)) + return false; + return true; +} static int tps65910_i2c_read(struct tps65910 *tps65910, u8 reg, int bytes, void *dest) { - struct i2c_client *i2c = tps65910->i2c_client; - struct i2c_msg xfer[2]; - int ret; - - /* Write register */ - xfer[0].addr = i2c->addr; - xfer[0].flags = 0; - xfer[0].len = 1; - xfer[0].buf = ® - - /* Read data */ - xfer[1].addr = i2c->addr; - xfer[1].flags = I2C_M_RD; - xfer[1].len = bytes; - xfer[1].buf = dest; - - ret = i2c_transfer(i2c->adapter, xfer, 2); - if (ret == 2) - ret = 0; - else if (ret >= 0) - ret = -EIO; - - return ret; + return regmap_bulk_read(tps65910->regmap, reg, dest, bytes); } static int tps65910_i2c_write(struct tps65910 *tps65910, u8 reg, - int bytes, void *src) + int bytes, void *src) { - struct i2c_client *i2c = tps65910->i2c_client; - /* we add 1 byte for device register */ - u8 msg[TPS65910_MAX_REGISTER + 1]; + int i; int ret; - - if (bytes > TPS65910_MAX_REGISTER) - return -EINVAL; - - msg[0] = reg; - memcpy(&msg[1], src, bytes); - - ret = i2c_master_send(i2c, msg, bytes + 1); - if (ret < 0) - return ret; - if (ret != bytes + 1) - return -EIO; + unsigned char *wbuf = src; + unsigned int ival; + + if (regmap_volatile_range(tps65910, reg, bytes)) + return regmap_raw_write(tps65910->regmap, reg, src, bytes); + + /* If any of register is non-volatile then use byte-wise transfer */ + for (i = 0; i < bytes; ++i) { + ival = (unsigned int) (*wbuf++); + ret = regmap_write(tps65910->regmap, reg, ival); + if (ret < 0) + return ret; + } return 0; } int tps65910_set_bits(struct tps65910 *tps65910, u8 reg, u8 mask) { - u8 data; - int err; - - mutex_lock(&tps65910->io_mutex); - err = tps65910_i2c_read(tps65910, reg, 1, &data); - if (err) { - dev_err(tps65910->dev, "read from reg %x failed\n", reg); - goto out; - } - - data |= mask; - err = tps65910_i2c_write(tps65910, reg, 1, &data); - if (err) - dev_err(tps65910->dev, "write to reg %x failed\n", reg); - -out: - mutex_unlock(&tps65910->io_mutex); - return err; + return regmap_update_bits(tps65910->regmap, reg, mask, mask); } EXPORT_SYMBOL_GPL(tps65910_set_bits); int tps65910_clear_bits(struct tps65910 *tps65910, u8 reg, u8 mask) { - u8 data; - int err; - - mutex_lock(&tps65910->io_mutex); - err = tps65910_i2c_read(tps65910, reg, 1, &data); - if (err) { - dev_err(tps65910->dev, "read from reg %x failed\n", reg); - goto out; - } - - data &= ~mask; - err = tps65910_i2c_write(tps65910, reg, 1, &data); - if (err) - dev_err(tps65910->dev, "write to reg %x failed\n", reg); - -out: - mutex_unlock(&tps65910->io_mutex); - return err; + return regmap_update_bits(tps65910->regmap, reg, mask, 0); } EXPORT_SYMBOL_GPL(tps65910_clear_bits); +void tps65910_enable_reg_cache(struct tps65910 *tps65910, int reg) +{ + set_bit(reg, tps65910->cache_reg); +} +EXPORT_SYMBOL_GPL(tps65910_enable_reg_cache); + +static const struct regmap_config rc5t583_regmap_config = { + .reg_bits = 8, + .val_bits = 8, + .volatile_reg = is_volatile_reg, + .max_register = TPS65910_MAX_REGISTER, + .num_reg_defaults_raw = TPS65910_MAX_REGISTER, + .cache_type = REGCACHE_RBTREE, +}; + static int tps65910_i2c_probe(struct i2c_client *i2c, const struct i2c_device_id *id) { @@ -161,6 +136,13 @@ static int tps65910_i2c_probe(struct i2c_client *i2c, tps65910->write = tps65910_i2c_write; mutex_init(&tps65910->io_mutex); + tps65910->regmap = regmap_init_i2c(i2c, &rc5t583_regmap_config); + if (IS_ERR(tps65910->regmap)) { + ret = PTR_ERR(tps65910->regmap); + dev_err(&i2c->dev, "regmap initialization failed: %d\n", ret); + goto regmap_err; + } + ret = mfd_add_devices(tps65910->dev, -1, tps65910s, ARRAY_SIZE(tps65910s), NULL, 0); @@ -178,6 +160,9 @@ static int tps65910_i2c_probe(struct i2c_client *i2c, return ret; err: + mfd_remove_devices(tps65910->dev); + regmap_exit(tps65910->regmap); +regmap_err: kfree(tps65910); kfree(init_data); return ret; @@ -189,6 +174,7 @@ static int tps65910_i2c_remove(struct i2c_client *i2c) tps65910_irq_exit(tps65910); mfd_remove_devices(tps65910->dev); + regmap_exit(tps65910->regmap); kfree(tps65910); return 0; diff --git a/include/linux/mfd/tps65910.h b/include/linux/mfd/tps65910.h index 5c76b96..6c68e2e 100644 --- a/include/linux/mfd/tps65910.h +++ b/include/linux/mfd/tps65910.h @@ -805,6 +805,7 @@ struct tps65910_board { struct tps65910 { struct device *dev; struct i2c_client *i2c_client; + struct regmap *regmap; struct mutex io_mutex; unsigned int id; int (*read)(struct tps65910 *tps65910, u8 reg, int size, void *dest); @@ -824,6 +825,7 @@ struct tps65910 { int irq_base; int irq_num; u32 irq_mask; + unsigned long cache_reg[BITS_TO_LONGS(TPS65910_MAX_REGISTER)]; }; struct tps65910_platform_data { @@ -837,6 +839,7 @@ void tps65910_gpio_init(struct tps65910 *tps65910, int gpio_base); int tps65910_irq_init(struct tps65910 *tps65910, int irq, struct tps65910_platform_data *pdata); int tps65910_irq_exit(struct tps65910 *tps65910); +void tps65910_enable_reg_cache(struct tps65910 *tps65910, int reg); static inline int tps65910_chip_id(struct tps65910 *tps65910) {
Using regmap apis for accessing the device register. using the RBTREE caching mechanims for caching registers. Also provided apis for enabling register cache from different sub devices of ts65910. Signed-off-by: Laxman Dewangan <ldewangan@nvidia.com> --- This patch is towards the performance improvements in the voltage change. drivers/mfd/Kconfig | 1 + drivers/mfd/tps65910.c | 136 +++++++++++++++++++----------------------- include/linux/mfd/tps65910.h | 3 + 3 files changed, 65 insertions(+), 75 deletions(-)