[v2] mfd: da9052-spi: change read-mask to write-mask

[PATCH v2] mfd: da9052-spi: change read-mask to write-mask

Posted by Marcus Folkesson 1 year, 4 months ago

Driver has mixed up the R/W bit.
The LSB bit is set on write rather than read.
Change it to avoid nasty things to happen.

Fixes: e9e9d3973594 ("mfd: da9052: Avoid setting read_flag_mask for da9052-i2c driver")
Signed-off-by: Marcus Folkesson <marcus.folkesson@gmail.com>
---
Changes in v2:
Reword the commit message and add Fixes: tag.
- Link to v1: https://lore.kernel.org/r/20240918-da9052-v1-1-ce7e7024e48c@gmail.com
---
 drivers/mfd/da9052-spi.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/mfd/da9052-spi.c b/drivers/mfd/da9052-spi.c
index be5f2b34e18aebc7a9506123b366af5a5447df43..80fc5c0cac2fb0e5e99e6db8972f80ae4155b226 100644
--- a/drivers/mfd/da9052-spi.c
+++ b/drivers/mfd/da9052-spi.c
@@ -37,7 +37,7 @@ static int da9052_spi_probe(struct spi_device *spi)
 	spi_set_drvdata(spi, da9052);
 
 	config = da9052_regmap_config;
-	config.read_flag_mask = 1;
+	config.write_flag_mask = 1;
 	config.reg_bits = 7;
 	config.pad_bits = 1;
 	config.val_bits = 8;

---
base-commit: 200289db261f0c8131a5756133e9d30966289c3b
change-id: 20240918-da9052-df843a1b9d0e

Best regards,
-- 
Marcus Folkesson <marcus.folkesson@gmail.com>

Re: [PATCH v2] mfd: da9052-spi: change read-mask to write-mask

Posted by Ian Ray 4 months, 1 week ago

Hello Marcus,

On Wed, Sep 25, 2024 at 12:19:53PM +0200, Marcus Folkesson wrote:
> Driver has mixed up the R/W bit.
> The LSB bit is set on write rather than read.
> Change it to avoid nasty things to happen.
> 
> Fixes: e9e9d3973594 ("mfd: da9052: Avoid setting read_flag_mask for da9052-i2c driver")
> Signed-off-by: Marcus Folkesson <marcus.folkesson@gmail.com>

Your patch breaks DA9053 SPI communication (for me, at least) on the
6.1.y branch (I have not tested on master).

The datasheets [1] and [2] both refer to R/Wn in the SPI signalling.

What led to the assertion that "The LSB bit is set on write rather
than read."?

In the original code "config.read_flag_mask = 1;", is OR'd into the
buffer in regmap_set_work_buf_flag_mask.  This sets the "R" bit as
expected.

[1] DA9052 CFR0011-120-00 Rev 5, Revision 2.5, 13-Feb-2017, page 67.
[2] DA9053 DA9053-00-IDS2n_131017, page 54.

Blue skies,
Ian

Re: [PATCH v2] mfd: da9052-spi: change read-mask to write-mask

Posted by Marcus Folkesson 4 months, 1 week ago

Hello Ian,

On Wed, Oct 01, 2025 at 04:03:37PM +0300, Ian Ray wrote:
> Hello Marcus,
> 
> On Wed, Sep 25, 2024 at 12:19:53PM +0200, Marcus Folkesson wrote:
> > Driver has mixed up the R/W bit.
> > The LSB bit is set on write rather than read.
> > Change it to avoid nasty things to happen.
> > 
> > Fixes: e9e9d3973594 ("mfd: da9052: Avoid setting read_flag_mask for da9052-i2c driver")
> > Signed-off-by: Marcus Folkesson <marcus.folkesson@gmail.com>
> 
> Your patch breaks DA9053 SPI communication (for me, at least) on the
> 6.1.y branch (I have not tested on master).
> 
> The datasheets [1] and [2] both refer to R/Wn in the SPI signalling.
> 
> What led to the assertion that "The LSB bit is set on write rather
> than read."?
> 
> In the original code "config.read_flag_mask = 1;", is OR'd into the
> buffer in regmap_set_work_buf_flag_mask.  This sets the "R" bit as
> expected.

Hrm.

I follow you and agree with what you say.
Could you please read out R19 INTERFACE register?
Bit 3, R/W POL, deviates from the default value (1) in my setup, which
is probably the reason why it doesn't work for me without the patch.

Your datasheet revision is later than mine, could you plese verify that
the default value is still 1 for DA9052 in your revision?

If that is the case, either the datasheet is wrong or my chips must
somehow been preloaded with some values.

As nobody else has reported any issue I guess it is safer to revert
this patch.

> 
> [1] DA9052 CFR0011-120-00 Rev 5, Revision 2.5, 13-Feb-2017, page 67.
> [2] DA9053 DA9053-00-IDS2n_131017, page 54.
> 
> Blue skies,
> Ian

Best regards,
Marcus Folkesson

Re: [PATCH v2] mfd: da9052-spi: change read-mask to write-mask

Posted by Ian Ray 4 months, 1 week ago

On Thu, Oct 02, 2025 at 09:23:21AM +0200, Marcus Folkesson wrote:
> Hello Ian,
> 
> On Wed, Oct 01, 2025 at 04:03:37PM +0300, Ian Ray wrote:
> > Hello Marcus,
> > 
> > On Wed, Sep 25, 2024 at 12:19:53PM +0200, Marcus Folkesson wrote:
> > > Driver has mixed up the R/W bit.
> > > The LSB bit is set on write rather than read.
> > > Change it to avoid nasty things to happen.
> > > 
> > > Fixes: e9e9d3973594 ("mfd: da9052: Avoid setting read_flag_mask for da9052-i2c driver")
> > > Signed-off-by: Marcus Folkesson <marcus.folkesson@gmail.com>
> > 
> > Your patch breaks DA9053 SPI communication (for me, at least) on the
> > 6.1.y branch (I have not tested on master).
> > 
> > The datasheets [1] and [2] both refer to R/Wn in the SPI signalling.
> > 
> > What led to the assertion that "The LSB bit is set on write rather
> > than read."?
> > 
> > In the original code "config.read_flag_mask = 1;", is OR'd into the
> > buffer in regmap_set_work_buf_flag_mask.  This sets the "R" bit as
> > expected.
> 
> Hrm.
> 
> I follow you and agree with what you say.
> Could you please read out R19 INTERFACE register?
> Bit 3, R/W POL, deviates from the default value (1) in my setup, which
> is probably the reason why it doesn't work for me without the patch.

-- >8 -- 
# cat /sys/kernel/debug/regmap/spi2.0/registers |grep "^13:"
13: 88
-- >8 -- 

> 
> Your datasheet revision is later than mine, could you plese verify that
> the default value is still 1 for DA9052 in your revision?

Yes it is still 1 (the complete set of defaults for R19 is 10011000).

> 
> If that is the case, either the datasheet is wrong or my chips must
> somehow been preloaded with some values.

Yes, in my understanding these PMICs are highly configurable via OTP.
(Almost every register can be customized.)

> 
> As nobody else has reported any issue I guess it is safer to revert
> this patch.

Agree -- following the datasheet default is probably the right thing to
do here.

For the future, we could consider an optional device tree property to
describe this read/write bit polarity.

Thanks,
Ian

> 
> > 
> > [1] DA9052 CFR0011-120-00 Rev 5, Revision 2.5, 13-Feb-2017, page 67.
> > [2] DA9053 DA9053-00-IDS2n_131017, page 54.
> > 
> > Blue skies,
> > Ian
> 
> 
> Best regards,
> Marcus Folkesson

Re: [PATCH v2] mfd: da9052-spi: change read-mask to write-mask

Posted by Marcus Folkesson 4 months, 1 week ago

Hello Ian,

On Thu, Oct 02, 2025 at 10:49:32AM +0300, Ian Ray wrote:
> On Thu, Oct 02, 2025 at 09:23:21AM +0200, Marcus Folkesson wrote:
> > Hello Ian,
> > 
> > On Wed, Oct 01, 2025 at 04:03:37PM +0300, Ian Ray wrote:
> > > Hello Marcus,
> > > 
> > > On Wed, Sep 25, 2024 at 12:19:53PM +0200, Marcus Folkesson wrote:
> > > > Driver has mixed up the R/W bit.
> > > > The LSB bit is set on write rather than read.
> > > > Change it to avoid nasty things to happen.
> > > > 
> > > > Fixes: e9e9d3973594 ("mfd: da9052: Avoid setting read_flag_mask for da9052-i2c driver")
> > > > Signed-off-by: Marcus Folkesson <marcus.folkesson@gmail.com>
> > > 
> > > Your patch breaks DA9053 SPI communication (for me, at least) on the
> > > 6.1.y branch (I have not tested on master).
> > > 
> > > The datasheets [1] and [2] both refer to R/Wn in the SPI signalling.
> > > 
> > > What led to the assertion that "The LSB bit is set on write rather
> > > than read."?
> > > 
> > > In the original code "config.read_flag_mask = 1;", is OR'd into the
> > > buffer in regmap_set_work_buf_flag_mask.  This sets the "R" bit as
> > > expected.
> > 
> > Hrm.
> > 
> > I follow you and agree with what you say.
> > Could you please read out R19 INTERFACE register?
> > Bit 3, R/W POL, deviates from the default value (1) in my setup, which
> > is probably the reason why it doesn't work for me without the patch.
> 
> -- >8 -- 
> # cat /sys/kernel/debug/regmap/spi2.0/registers |grep "^13:"
> 13: 88
> -- >8 -- 
> 
> > 
> > Your datasheet revision is later than mine, could you plese verify that
> > the default value is still 1 for DA9052 in your revision?
> 
> Yes it is still 1 (the complete set of defaults for R19 is 10011000).
> 
> > 
> > If that is the case, either the datasheet is wrong or my chips must
> > somehow been preloaded with some values.
> 
> Yes, in my understanding these PMICs are highly configurable via OTP.
> (Almost every register can be customized.)
> 
> > 
> > As nobody else has reported any issue I guess it is safer to revert
> > this patch.
> 
> Agree -- following the datasheet default is probably the right thing to
> do here.
> 
> For the future, we could consider an optional device tree property to
> describe this read/write bit polarity.

Yes, I'm actually working on a patch for this, I will send it out
shortly.

> 
> Thanks,
> Ian
> 
> > 
> > > 
> > > [1] DA9052 CFR0011-120-00 Rev 5, Revision 2.5, 13-Feb-2017, page 67.
> > > [2] DA9053 DA9053-00-IDS2n_131017, page 54.
> > > 
> > > Blue skies,
> > > Ian
> > 
> > 
> > Best regards,
> > Marcus Folkesson

Best regards,
Marcus Folkesson

Re: (subset) [PATCH v2] mfd: da9052-spi: change read-mask to write-mask

Posted by Lee Jones 1 year, 4 months ago

On Wed, 25 Sep 2024 12:19:53 +0200, Marcus Folkesson wrote:
> Driver has mixed up the R/W bit.
> The LSB bit is set on write rather than read.
> Change it to avoid nasty things to happen.
> 
> 

Applied, thanks!

[1/1] mfd: da9052-spi: change read-mask to write-mask
      commit: f77c2e93771be2d66c639c8eb0b463de2c321579

--
Lee Jones [李琼斯]