[PATCH] regmap: Allow read_reg_mask to be 0

[PATCH] regmap: Allow read_reg_mask to be 0

[Dan Murphy]

There may be spi devices that do not require a
register read mask to read the registers.

Currently the code sets the read mask based on
a non-zero value passed in from the driver or if that
value is 0 sets the read mask to 0x80.

A mask of 0 is a valid mask as well.  Create a define
to indicate that the read mask can be zero and separate
out the read flag mask and the write flag mask.

Signed-off-by: Dan Murphy <dmurphy@ti.com>
---
 drivers/base/regmap/regmap.c |   11 +++++++----
 include/linux/regmap.h       |    2 ++
 2 files changed, 9 insertions(+), 4 deletions(-)

diff --git a/drivers/base/regmap/regmap.c b/drivers/base/regmap/regmap.c
index 78f43fb..8c0a9b8 100644
--- a/drivers/base/regmap/regmap.c
+++ b/drivers/base/regmap/regmap.c
@@ -528,12 +528,15 @@ struct regmap *regmap_init(struct device *dev,
 	INIT_LIST_HEAD(&map->async_free);
 	init_waitqueue_head(&map->async_waitq);
 
-	if (config->read_flag_mask || config->write_flag_mask) {
+	if (config->read_flag_mask == REGMAP_NO_READ_MASK)
+		map->read_flag_mask = 0x00;
+	else if (config->read_flag_mask)
 		map->read_flag_mask = config->read_flag_mask;
-		map->write_flag_mask = config->write_flag_mask;
-	} else if (bus) {
+	else if (bus)
 		map->read_flag_mask = bus->read_flag_mask;
-	}
+
+	if (config->write_flag_mask)
+		map->write_flag_mask = config->write_flag_mask;
 
 	if (!bus) {
 		map->reg_read  = config->reg_read;
diff --git a/include/linux/regmap.h b/include/linux/regmap.h
index c5ed83f..f1cdfe5 100644
--- a/include/linux/regmap.h
+++ b/include/linux/regmap.h
@@ -18,6 +18,8 @@
 #include <linux/err.h>
 #include <linux/bug.h>
 
+#define REGMAP_NO_READ_MASK		0xff
+
 struct module;
 struct device;
 struct i2c_client;
-- 
1.7.9.5

Re: [PATCH] regmap: Allow read_reg_mask to be 0

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 560 bytes --]

On Tue, Sep 30, 2014 at 11:07:00AM -0500, Dan Murphy wrote:

> -	if (config->read_flag_mask || config->write_flag_mask) {
> +	if (config->read_flag_mask == REGMAP_NO_READ_MASK)
> +		map->read_flag_mask = 0x00;
> +	else if (config->read_flag_mask)

This breaks the symmetry in handling of read and write masks which isn't
great, please make the equivalent update for the write mask too.

> +#define REGMAP_NO_READ_MASK		0xff

An actual out of band value might be preferable here though that'd
involve changing the type and more checking so perhaps inessential.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH] regmap: Allow read_reg_mask to be 0

[Dan Murphy]

Mark

Thanks for the review

On 09/30/2014 12:25 PM, Mark Brown wrote:
> On Tue, Sep 30, 2014 at 11:07:00AM -0500, Dan Murphy wrote:
>
>> -	if (config->read_flag_mask || config->write_flag_mask) {
>> +	if (config->read_flag_mask == REGMAP_NO_READ_MASK)
>> +		map->read_flag_mask = 0x00;
>> +	else if (config->read_flag_mask)
> This breaks the symmetry in handling of read and write masks which isn't
> great, please make the equivalent update for the write mask too.

Hmmm.  If I make the similar change for the write mask I will be adding dead
code as write_flag_mask is not defaulted by the bus like the read_flag_mask which is
defaulted to 0x80 in the regmap-spi code.  The i2c code does not make this assumption.

The original code did not seem to have symmetry as the only instance
that write_flag_mask is modified is if it is non-zero.

Let me know what you think.  I can add the code to make it more symmetrical but
we may be adding code that is dead.  If I remove the defaulted read_flag_mask value out
of the spi driver then I will need to find every spi device that needs that flag and set it.
IMHO that would be really messy and probably mess us some drivers.

>
>> +#define REGMAP_NO_READ_MASK		0xff
> An actual out of band value might be preferable here though that'd
> involve changing the type and more checking so perhaps inessential.

Thought about making this -1 with a variable change but that seemed
really drastic where a value of 0xff seems to be a value that no one should use.

Dan

-- 
------------------
Dan Murphy

Re: [PATCH] regmap: Allow read_reg_mask to be 0

[Dan Murphy]

On 09/30/2014 12:25 PM, Mark Brown wrote:
> On Tue, Sep 30, 2014 at 11:07:00AM -0500, Dan Murphy wrote:
>
>> -	if (config->read_flag_mask || config->write_flag_mask) {
>> +	if (config->read_flag_mask == REGMAP_NO_READ_MASK)
>> +		map->read_flag_mask = 0x00;
>> +	else if (config->read_flag_mask)
> This breaks the symmetry in handling of read and write masks which isn't
> great, please make the equivalent update for the write mask too.

Hmmm.  If I make the similar change for the write mask I will be adding dead
code as write_flag_mask is not defaulted by the bus like the read_flag_mask which is
defaulted to 0x80 in the regmap-spi code.  The i2c code does not make this assumption.

The original code did not seem to have symmetry as the only instance
that write_flag_mask is modified is if it is non-zero.

Let me know what you think.  I can add the code to make it more symmetrical but
we may be adding code that is dead.  If I remove the defaulted read_flag_mask value out
of the spi driver then I will need to find every spi device that needs that flag and set it.
IMHO that would be really messy and probably mess us some drivers.

>> +#define REGMAP_NO_READ_MASK		0xff
> An actual out of band value might be preferable here though that'd
> involve changing the type and more checking so perhaps inessential.

Thought about making this -1 with a variable change but that seemed
really drastic where a value of 0xff seems to be a value that no one should use.

Dan


-- 
------------------
Dan Murphy

Re: [PATCH] regmap: Allow read_reg_mask to be 0

[Dan Murphy]

Mark

Thanks for the quick review

On 09/30/2014 12:25 PM, Mark Brown wrote:
> On Tue, Sep 30, 2014 at 11:07:00AM -0500, Dan Murphy wrote:
>
>> -	if (config->read_flag_mask || config->write_flag_mask) {
>> +	if (config->read_flag_mask == REGMAP_NO_READ_MASK)
>> +		map->read_flag_mask = 0x00;
>> +	else if (config->read_flag_mask)
> This breaks the symmetry in handling of read and write masks which isn't
> great, please make the equivalent update for the write mask too.
>
>
Hmmm.  If I make the similar change for the write mask I will be adding dead
code as write_flag_mask is not defaulted by the bus like the read_flag_mask which is
defaulted to 0x80 in the regmap-spi code.  The i2c code does not make this assumption.

The original code did not seem to have symmetry as the only instance
that write_flag_mask is modified is if it is non-zero.

Let me know what you think.  I can add the code to make it more symmetrical but
we may be adding code that is dead.  If I remove the defaulted read_flag_mask value out
of the spi driver then I will need to find every spi device that needs that flag and set it.
IMHO that would be really messy and probably mess us some drivers.

>> +#define REGMAP_NO_READ_MASK		0xff
> An actual out of band value might be preferable here though that'd
> involve changing the type and more checking so perhaps inessential.

Thought about making this -1 with a variable change but that seemed
really drastic where a value of 0xff seems to be a value that no one should use.

Dan

-- 
------------------
Dan Murphy

Re: [PATCH] regmap: Allow read_reg_mask to be 0

[Lars-Peter Clausen]

On 09/30/2014 06:07 PM, Dan Murphy wrote:
> There may be spi devices that do not require a
> register read mask to read the registers.
>
> Currently the code sets the read mask based on
> a non-zero value passed in from the driver or if that
> value is 0 sets the read mask to 0x80.

It only sets it to the bus default if both read_flag_mask and 
write_flag_mask are 0. The assumption is that both of them being zero is a 
invalid configuration and either of them (or both) have to be non-zero for 
proper operation, since otherwise the device can't tell the difference 
between a read and a write.

Do you have a device where both the read and the write mask is 0?

- Lars

Re: [PATCH] regmap: Allow read_reg_mask to be 0

[Dan Murphy]

Lars

On 09/30/2014 04:03 PM, Lars-Peter Clausen wrote:
> On 09/30/2014 06:07 PM, Dan Murphy wrote:
>> There may be spi devices that do not require a
>> register read mask to read the registers.
>>
>> Currently the code sets the read mask based on
>> a non-zero value passed in from the driver or if that
>> value is 0 sets the read mask to 0x80.
>
> It only sets it to the bus default if both read_flag_mask and write_flag_mask are 0. The assumption is that both of them being zero is a invalid configuration and either of them (or both) have to be non-zero for proper operation, since otherwise the device can't tell the difference between a read and a write.
>
> Do you have a device where both the read and the write mask is 0?
>
> - Lars

Yes I do have a device that the read/write mask are both 0.

The device, which is already in production, has a specific control register that sets either the reading or writing of the rest of the registers.

Here is the data sheet

http://www.ti.com/lit/ds/symlink/afe4403.pdf

See page 61 control0.

Driver is written for this part just want to get this lead patch in or maybe an alternate solution.

Dan

-- 
------------------
Dan Murphy

Re: [PATCH] regmap: Allow read_reg_mask to be 0

[Lars-Peter Clausen]

On 09/30/2014 11:18 PM, Dan Murphy wrote:
> Lars
>
> On 09/30/2014 04:03 PM, Lars-Peter Clausen wrote:
>> On 09/30/2014 06:07 PM, Dan Murphy wrote:
>>> There may be spi devices that do not require a
>>> register read mask to read the registers.
>>>
>>> Currently the code sets the read mask based on
>>> a non-zero value passed in from the driver or if that
>>> value is 0 sets the read mask to 0x80.
>>
>> It only sets it to the bus default if both read_flag_mask and write_flag_mask are 0. The assumption is that both of them being zero is a invalid configuration and either of them (or both) have to be non-zero for proper operation, since otherwise the device can't tell the difference between a read and a write.
>>
>> Do you have a device where both the read and the write mask is 0?
>>
>> - Lars
>
> Yes I do have a device that the read/write mask are both 0.
>
> The device, which is already in production, has a specific control register that sets either the reading or writing of the rest of the registers.
>
> Here is the data sheet
>
> http://www.ti.com/lit/ds/symlink/afe4403.pdf
>
> See page 61 control0.
>
> Driver is written for this part just want to get this lead patch in or maybe an alternate solution.

Looking at this the generic SPI regmap implementation might not necessarily 
be the best thing to use here and you are probably better of implementing 
either your own regmap bus or reg_read/reg_write callbacks that 
automatically set/clear the SPI_READ bit in the control register depending 
on the operation.

- Lars

Re: [PATCH] regmap: Allow read_reg_mask to be 0

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1029 bytes --]

On Tue, Sep 30, 2014 at 11:29:34PM +0200, Lars-Peter Clausen wrote:
> On 09/30/2014 11:18 PM, Dan Murphy wrote:

Dan, please fix your mail client to word wrap within paragraphs.

> >The device, which is already in production, has a specific control register that sets either the reading or writing of the rest of the registers.

> >Here is the data sheet

> >http://www.ti.com/lit/ds/symlink/afe4403.pdf

> >See page 61 control0.

> >Driver is written for this part just want to get this lead patch in or maybe an alternate solution.

> Looking at this the generic SPI regmap implementation might not necessarily
> be the best thing to use here and you are probably better of implementing
> either your own regmap bus or reg_read/reg_write callbacks that
> automatically set/clear the SPI_READ bit in the control register depending
> on the operation.

It definitely needs more than just this change at any rate - any generic
infrastructure that tries to work with the regmap is not going to know
about this read/write register.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH] regmap: Allow read_reg_mask to be 0

[Dan Murphy]

Lars

On 09/30/2014 04:29 PM, Lars-Peter Clausen wrote:
> On 09/30/2014 11:18 PM, Dan Murphy wrote:
>> Lars
>>
>> On 09/30/2014 04:03 PM, Lars-Peter Clausen wrote:
>>> On 09/30/2014 06:07 PM, Dan Murphy wrote:
>>>> There may be spi devices that do not require a
>>>> register read mask to read the registers.
>>>>
>>>> Currently the code sets the read mask based on
>>>> a non-zero value passed in from the driver or if that
>>>> value is 0 sets the read mask to 0x80.
>>>
>>> It only sets it to the bus default if both read_flag_mask and write_flag_mask are 0. The assumption is that both of them being zero is a invalid configuration and either of them (or both) have to be non-zero for proper operation, since otherwise the device can't tell the difference between a read and a write.
>>>
>>> Do you have a device where both the read and the write mask is 0?
>>>
>>> - Lars
>>
>> Yes I do have a device that the read/write mask are both 0.
>>
>> The device, which is already in production, has a specific control register that sets either the reading or writing of the rest of the registers.
>>
>> Here is the data sheet
>>
>> http://www.ti.com/lit/ds/symlink/afe4403.pdf
>>
>> See page 61 control0.
>>
>> Driver is written for this part just want to get this lead patch in or maybe an alternate solution.
>
> Looking at this the generic SPI regmap implementation might not necessarily be the best thing to use here and you are probably better of implementing either your own regmap bus or reg_read/reg_write callbacks that automatically set/clear the SPI_READ bit in the control register depending on the operation.
>
> - Lars

I am not sure implementing a different SPI regmap implementation is really the best thing.
I am handling this control bit toggling in the peripheral driver.
The device only needs to be written at init and then during calibration.  Other then that it is read read read.

Dan

-- 
------------------
Dan Murphy

Re: [PATCH] regmap: Allow read_reg_mask to be 0

[Lars-Peter Clausen]

On 10/01/2014 01:39 PM, Dan Murphy wrote:
> Lars
>
> On 09/30/2014 04:29 PM, Lars-Peter Clausen wrote:
>> On 09/30/2014 11:18 PM, Dan Murphy wrote:
>>> Lars
>>>
>>> On 09/30/2014 04:03 PM, Lars-Peter Clausen wrote:
>>>> On 09/30/2014 06:07 PM, Dan Murphy wrote:
>>>>> There may be spi devices that do not require a
>>>>> register read mask to read the registers.
>>>>>
>>>>> Currently the code sets the read mask based on
>>>>> a non-zero value passed in from the driver or if that
>>>>> value is 0 sets the read mask to 0x80.
>>>>
>>>> It only sets it to the bus default if both read_flag_mask and write_flag_mask are 0. The assumption is that both of them being zero is a invalid configuration and either of them (or both) have to be non-zero for proper operation, since otherwise the device can't tell the difference between a read and a write.
>>>>
>>>> Do you have a device where both the read and the write mask is 0?
>>>>
>>>> - Lars
>>>
>>> Yes I do have a device that the read/write mask are both 0.
>>>
>>> The device, which is already in production, has a specific control register that sets either the reading or writing of the rest of the registers.
>>>
>>> Here is the data sheet
>>>
>>> http://www.ti.com/lit/ds/symlink/afe4403.pdf
>>>
>>> See page 61 control0.
>>>
>>> Driver is written for this part just want to get this lead patch in or maybe an alternate solution.
>>
>> Looking at this the generic SPI regmap implementation might not necessarily be the best thing to use here and you are probably better of implementing either your own regmap bus or reg_read/reg_write callbacks that automatically set/clear the SPI_READ bit in the control register depending on the operation.
>>
>> - Lars
>
> I am not sure implementing a different SPI regmap implementation is really the best thing.
> I am handling this control bit toggling in the peripheral driver.

This is the very thing that regmap is supposed to hide, the specifics of how 
the read or write access happens. regmap_read() is supposed to do a read, 
regmap_write() is supposed to do a write. If regmap_read() only does a write 
if you previously did a regmap_write(regmap, CTRL0, SPI_READ); then the 
semantics of the interface are broken. This means you can't use generic 
infrastructure and it will confuse people who read and review your code.

- Lars