| Message ID | 1439374365-20623-10-git-send-email-mpa@pengutronix.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Aug 12, 2015 at 12:12:34PM +0200, Markus Pargmann wrote: > @@ -1229,6 +1229,11 @@ int _regmap_raw_write(struct regmap *map, unsigned int reg, > } > } > > + if (!map->bus->write && val_len == map->format.val_bytes) { > + ret = _regmap_bus_reg_write(map, reg, *(unsigned int *)val); > + return ret; > + } This is broken - you can't use a raw value as a register value. The endianness of the device may not be the same as the endianness of the system and you can't cast a value to unsigned int, the value may be of any size. > @@ -1340,7 +1345,7 @@ int _regmap_raw_write(struct regmap *map, unsigned int reg, > * send the work_buf directly, otherwise try to do a gather > * write. > */ > - if (val == work_val) { > + if (val == work_val && map->bus->write) { > ret = map->bus->write(map->bus_context, map->work_buf, > map->format.reg_bytes + > map->format.pad_bytes + This appears to be another case of merging an unrelated change :(
On Wed, Aug 12, 2015 at 12:20:35PM +0100, Mark Brown wrote: > On Wed, Aug 12, 2015 at 12:12:34PM +0200, Markus Pargmann wrote: > > > @@ -1229,6 +1229,11 @@ int _regmap_raw_write(struct regmap *map, unsigned int reg, > > } > > } > > > > + if (!map->bus->write && val_len == map->format.val_bytes) { > > + ret = _regmap_bus_reg_write(map, reg, *(unsigned int *)val); > > + return ret; > > + } > > This is broken - you can't use a raw value as a register value. The I am not sure what you mean here? The register value given to _regmap_raw_write is the real register value, not formatted differenty. This is given directly towards bus->reg_write() which should handle the rest. At least that's how I understood the code. For example regmap_read() directly calls _regmap_read() which in turn calls directly bus->reg_read() without any formating. > endianness of the device may not be the same as the endianness of the > system and you can't cast a value to unsigned int, the value may be of > any size. Yes right. On the other hand if bus->read() and bus->write() was not set in the init method (before this patch series) no formatting functions at all were assigned. So it was always ignored for bus->reg_read() and bus->reg_write()?! > > > @@ -1340,7 +1345,7 @@ int _regmap_raw_write(struct regmap *map, unsigned int reg, > > * send the work_buf directly, otherwise try to do a gather > > * write. > > */ > > - if (val == work_val) { > > + if (val == work_val && map->bus->write) { > > ret = map->bus->write(map->bus_context, map->work_buf, > > map->format.reg_bytes + > > map->format.pad_bytes + > > This appears to be another case of merging an unrelated change :( Yes, will fix. Thanks, Markus
On Wed, Aug 12, 2015 at 02:20:11PM +0200, Markus Pargmann wrote: > On Wed, Aug 12, 2015 at 12:20:35PM +0100, Mark Brown wrote: > > On Wed, Aug 12, 2015 at 12:12:34PM +0200, Markus Pargmann wrote: > > > > > @@ -1229,6 +1229,11 @@ int _regmap_raw_write(struct regmap *map, unsigned int reg, > > > } > > > } > > > > > > + if (!map->bus->write && val_len == map->format.val_bytes) { > > > + ret = _regmap_bus_reg_write(map, reg, *(unsigned int *)val); > > > + return ret; > > > + } > > This is broken - you can't use a raw value as a register value. The > I am not sure what you mean here? > The register value given to _regmap_raw_write is the real register > value, not formatted differenty. This is given directly towards > bus->reg_write() which should handle the rest. I mean the value for the register, not the register address. > At least that's how I understood the code. For example regmap_read() > directly calls _regmap_read() which in turn calls directly > bus->reg_read() without any formating. You're adding this code to regmap_raw_write() which takes raw register values for the device, not unsigned integers. > > endianness of the device may not be the same as the endianness of the > > system and you can't cast a value to unsigned int, the value may be of > > any size. > Yes right. On the other hand if bus->read() and bus->write() was not set > in the init method (before this patch series) no formatting functions at > all were assigned. So it was always ignored for bus->reg_read() and > bus->reg_write()?! I'm not sure what the "it" you're talking about here is, sorry. There are unsupported features in the API especially for cases that don't make a huge amount of sense, the error handling isn't always complete. It sounds like you might be trying to support one of these nonsensical cases - it's not obvious what raw I/O on a device where we don't know the raw format of the device should mean or how anything could sensibly use that.
On Wed, Aug 12, 2015 at 01:34:06PM +0100, Mark Brown wrote: > On Wed, Aug 12, 2015 at 02:20:11PM +0200, Markus Pargmann wrote: > > On Wed, Aug 12, 2015 at 12:20:35PM +0100, Mark Brown wrote: > > > On Wed, Aug 12, 2015 at 12:12:34PM +0200, Markus Pargmann wrote: > > > > > > > @@ -1229,6 +1229,11 @@ int _regmap_raw_write(struct regmap *map, unsigned int reg, > > > > } > > > > } > > > > > > > > + if (!map->bus->write && val_len == map->format.val_bytes) { > > > > + ret = _regmap_bus_reg_write(map, reg, *(unsigned int *)val); > > > > + return ret; > > > > + } > > > > This is broken - you can't use a raw value as a register value. The > > > I am not sure what you mean here? > > > The register value given to _regmap_raw_write is the real register > > value, not formatted differenty. This is given directly towards > > bus->reg_write() which should handle the rest. > > I mean the value for the register, not the register address. > > > At least that's how I understood the code. For example regmap_read() > > directly calls _regmap_read() which in turn calls directly > > bus->reg_read() without any formating. > > You're adding this code to regmap_raw_write() which takes raw register > values for the device, not unsigned integers. Ah yes, I see. > > > > endianness of the device may not be the same as the endianness of the > > > system and you can't cast a value to unsigned int, the value may be of > > > any size. > > > Yes right. On the other hand if bus->read() and bus->write() was not set > > in the init method (before this patch series) no formatting functions at > > all were assigned. So it was always ignored for bus->reg_read() and > > bus->reg_write()?! > > I'm not sure what the "it" you're talking about here is, sorry. There > are unsupported features in the API especially for cases that don't make > a huge amount of sense, the error handling isn't always complete. It > sounds like you might be trying to support one of these nonsensical > cases - it's not obvious what raw I/O on a device where we don't know > the raw format of the device should mean or how anything could sensibly > use that. The bus and the regmap user are separate. So as a regmap user, I am not able to know if the bus the device is connected to actually supports raw reads/writes. At least it should not fail with a null pointer when using these functions anyway so yes error handling is missing a bit here. Also the real use of this function is regmap_bulk_write() which always uses _regmap_raw_write() regardless of a missing bus->write() function. So regmap_bulk_write will fail for those as well and this should be supported. But it is probably better to handle this in regmap_bulk_write() and add a simple check for bus->write() here. Best Regards, Markus
On Wed, Aug 12, 2015 at 03:05:18PM +0200, Markus Pargmann wrote: > On Wed, Aug 12, 2015 at 01:34:06PM +0100, Mark Brown wrote: > > > Yes right. On the other hand if bus->read() and bus->write() was not set > > > in the init method (before this patch series) no formatting functions at > > > all were assigned. So it was always ignored for bus->reg_read() and > > > bus->reg_write()?! > > I'm not sure what the "it" you're talking about here is, sorry. There > > are unsupported features in the API especially for cases that don't make > > a huge amount of sense, the error handling isn't always complete. It > > sounds like you might be trying to support one of these nonsensical > > cases - it's not obvious what raw I/O on a device where we don't know > > the raw format of the device should mean or how anything could sensibly > > use that. > The bus and the regmap user are separate. So as a regmap user, I am not > able to know if the bus the device is connected to actually supports raw > reads/writes. At least it should not fail with a null pointer when using You should generally have a pretty good idea simply by knowing which device you're working with - unless you're writing generic code you know which device you're working with and what it's capabilities are. A driver that doesn't know these things should never be trying to do raw I/O, and a driver that is doing raw I/O clearly depends on having the ability to get a bytestream to and from the device since that's what raw I/O does.
diff --git a/drivers/base/regmap/regmap.c b/drivers/base/regmap/regmap.c index 510dab052a95..78eb96288a68 100644 --- a/drivers/base/regmap/regmap.c +++ b/drivers/base/regmap/regmap.c @@ -1229,6 +1229,11 @@ int _regmap_raw_write(struct regmap *map, unsigned int reg, } } + if (!map->bus->write && val_len == map->format.val_bytes) { + ret = _regmap_bus_reg_write(map, reg, *(unsigned int *)val); + return ret; + } + range = _regmap_range_lookup(map, reg); if (range) { int val_num = val_len / map->format.val_bytes; @@ -1340,7 +1345,7 @@ int _regmap_raw_write(struct regmap *map, unsigned int reg, * send the work_buf directly, otherwise try to do a gather * write. */ - if (val == work_val) { + if (val == work_val && map->bus->write) { ret = map->bus->write(map->bus_context, map->work_buf, map->format.reg_bytes + map->format.pad_bytes + @@ -1354,7 +1359,7 @@ int _regmap_raw_write(struct regmap *map, unsigned int reg, val, val_len); /* If that didn't work fall back on linearising by hand. */ - if (ret == -ENOTSUPP) { + if (ret == -ENOTSUPP && map->bus->write) { len = map->format.reg_bytes + map->format.pad_bytes + val_len; buf = kzalloc(len, GFP_KERNEL);
Some busses don't have a write() function defined. However we can use reg_write() in special cases. This patch adds support for reg_write() and throws errors if it was unsuccessful. Signed-off-by: Markus Pargmann <mpa@pengutronix.de> --- drivers/base/regmap/regmap.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-)