The mask name AD5398_CURRENT_EN_MASK is misleading, as it implies that
setting bit 16 of the AD5398 enables current flow. In fact, setting this
bit prevents current flow, due to this bit being a software power down
control. This bit is referred to as "soft power down" in the datasheet.
As such, change the name of the bit and modify its use in the driver to
make the regulator more intuitively usable.

(When calling ad5398_enable, current will start flowing, and vice
versa).

Signed-off-by: Isaac Scott <isaac.scott@ideasonboard.com>
---
 drivers/regulator/ad5398.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/drivers/regulator/ad5398.c b/drivers/regulator/ad5398.c
index 40f7dba42b5a..e6f45c6e750c 100644
--- a/drivers/regulator/ad5398.c
+++ b/drivers/regulator/ad5398.c
@@ -15,7 +15,7 @@
 #include <linux/regulator/driver.h>
 #include <linux/regulator/machine.h>
 
-#define AD5398_CURRENT_EN_MASK	0x8000
+#define AD5398_SW_POWER_DOWN	BIT(16)
 
 struct ad5398_chip_info {
 	struct i2c_client *client;
@@ -113,7 +113,7 @@ static int ad5398_set_current_limit(struct regulator_dev *rdev, int min_uA, int
 
 	/* prepare register data */
 	selector = (selector << chip->current_offset) & chip->current_mask;
-	data = (unsigned short)selector | (data & AD5398_CURRENT_EN_MASK);
+	data = (unsigned short)selector | (data & AD5398_SW_POWER_DOWN);
 
 	/* write the new current value back as well as enable bit */
 	ret = ad5398_write_reg(client, data);
@@ -132,10 +132,10 @@ static int ad5398_is_enabled(struct regulator_dev *rdev)
 	if (ret < 0)
 		return ret;
 
-	if (data & AD5398_CURRENT_EN_MASK)
-		return 1;
-	else
+	if (data & AD5398_SW_POWER_DOWN)
 		return 0;
+	else
+		return 1;
 }
 
 static int ad5398_enable(struct regulator_dev *rdev)
@@ -149,10 +149,10 @@ static int ad5398_enable(struct regulator_dev *rdev)
 	if (ret < 0)
 		return ret;
 
-	if (data & AD5398_CURRENT_EN_MASK)
+	if (!(data & AD5398_SW_POWER_DOWN))
 		return 0;
 
-	data |= AD5398_CURRENT_EN_MASK;
+	data &= ~AD5398_SW_POWER_DOWN;
 
 	ret = ad5398_write_reg(client, data);
 
@@ -170,10 +170,10 @@ static int ad5398_disable(struct regulator_dev *rdev)
 	if (ret < 0)
 		return ret;
 
-	if (!(data & AD5398_CURRENT_EN_MASK))
+	if (data & AD5398_SW_POWER_DOWN)
 		return 0;
 
-	data &= ~AD5398_CURRENT_EN_MASK;
+	data |= AD5398_SW_POWER_DOWN;
 
 	ret = ad5398_write_reg(client, data);
 
-- 
2.43.0

On Tue, Jan 28, 2025 at 05:31:41PM +0000, Isaac Scott wrote:
> The mask name AD5398_CURRENT_EN_MASK is misleading, as it implies that
> setting bit 16 of the AD5398 enables current flow. In fact, setting this
> bit prevents current flow, due to this bit being a software power down
> control. This bit is referred to as "soft power down" in the datasheet.
> As such, change the name of the bit and modify its use in the driver to
> make the regulator more intuitively usable.
> 
> (When calling ad5398_enable, current will start flowing, and vice
> versa).
> 
> Signed-off-by: Isaac Scott <isaac.scott@ideasonboard.com>
> ---
[ ... ]
>  
> -#define AD5398_CURRENT_EN_MASK	0x8000
> +#define AD5398_SW_POWER_DOWN	BIT(16)

0x8000 is BIT(15).

Guenter

Hi Guenter,

On Wed, 2025-02-05 at 10:45 -0800, Guenter Roeck wrote:
> On Tue, Jan 28, 2025 at 05:31:41PM +0000, Isaac Scott wrote:
> > The mask name AD5398_CURRENT_EN_MASK is misleading, as it implies
> > that
> > setting bit 16 of the AD5398 enables current flow. In fact, setting
> > this
> > bit prevents current flow, due to this bit being a software power
> > down
> > control. This bit is referred to as "soft power down" in the
> > datasheet.
> > As such, change the name of the bit and modify its use in the
> > driver to
> > make the regulator more intuitively usable.
> > 
> > (When calling ad5398_enable, current will start flowing, and vice
> > versa).
> > 
> > Signed-off-by: Isaac Scott <isaac.scott@ideasonboard.com>
> > ---
> [ ... ]
> >  
> > -#define AD5398_CURRENT_EN_MASK	0x8000
> > +#define AD5398_SW_POWER_DOWN	BIT(16)
> 
> 0x8000 is BIT(15).

You're totally right! The reason this got through is because in my use
case, I am powering the regulator on and off in multiple ways. This
should definitely be BIT(15), so I will send a V3 shortly.

Thank you very much!

Best wishes,

Isaac

> 
> Guenter

On Thu, Feb 06, 2025 at 09:59:13AM +0000, Isaac Scott wrote:
> On Wed, 2025-02-05 at 10:45 -0800, Guenter Roeck wrote:

> > 0x8000 is BIT(15).

> You're totally right! The reason this got through is because in my use
> case, I am powering the regulator on and off in multiple ways. This
> should definitely be BIT(15), so I will send a V3 shortly.

Please do not submit new versions of already applied patches, please
submit incremental updates to the existing code.  Modifying existing
commits creates problems for other users building on top of those
commits so it's best practice to only change pubished git commits if
absolutely essential.