iio: dac: mcp4922: Add powerdown support

This patch adds support for per-channel powerdown on the Microchip MCP 4902/4912/4922 family of DACs.

Signed-off-by Chris Coffey <[email protected]>

---
 drivers/iio/dac/mcp4922.c | 126 ++++++++++++++++++++++++++++++++++++++++------
 1 file changed, 110 insertions(+), 16 deletions(-)

drivers/iio/dac/mcp4922.c +5 -15

index b5190d1dae..15cd17aa9d 100644
@@ -28,6 +28,9 @@
 
 #define MCP4922_NUM_CHANNELS	2
 
+#define MCP4922_OUTA_POWER_DOWN	0x20
+#define MCP4922_OUTB_POWER_DOWN	0xa0
+
 enum mcp4922_supported_device_ids {
 	ID_MCP4902,
 	ID_MCP4912,

@@ -37,26 +40,13 @@ enum mcp4922_supported_device_ids {
 struct mcp4922_state {
 	struct spi_device *spi;
 	unsigned int value[MCP4922_NUM_CHANNELS];
+	bool powerdown[MCP4922_NUM_CHANNELS];
+	unsigned int powerdown_mode;
 	unsigned int vref_mv;
 	struct regulator *vref_reg;
 	u8 mosi[2] ____cacheline_aligned;
 };
 
-#define MCP4922_CHAN(chan, bits) {			\
-	.type = IIO_VOLTAGE,				\
-	.output = 1,					\
-	.indexed = 1,					\
-	.channel = chan,				\
-	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),	\
-	.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),	\
-	.scan_type = {					\
-		.sign = 'u',				\
-		.realbits = (bits),			\
-		.storagebits = 16,			\
-		.shift = 12 - (bits),			\
-	},						\
-}
-
 static int mcp4922_spi_write(struct mcp4922_state *state, u8 addr, u32 val)
 {
 	state->mosi[1] = val & 0xff;

drivers/iio/dac/mcp4922.c +10 -0

+		}
 		return ret;
 
 	default:

@@ -115,6 +107,108 @@ static int mcp4922_write_raw(struct iio_dev *indio_dev,
 	}
 }
 
+static const char * const mcp4922_powerdown_modes[] = {
+	"500kohm_to_gnd"
+};
+
+static int mcp4922_get_powerdown_mode(struct iio_dev *indio_dev,
+					const struct iio_chan_spec *chan)
+{
+	struct mcp4922_state *state = iio_priv(indio_dev);
+

drivers/iio/dac/mcp4922.c +83 -0

+
+	return 0;
+}
+
+static ssize_t mcp4922_read_powerdown(struct iio_dev *indio_dev,
+					uintptr_t private,
+					const struct iio_chan_spec *chan,
+					char *buf)
+{
+	struct mcp4922_state *state = iio_priv(indio_dev);
+
+	return sprintf(buf, "%d\n", state->powerdown[chan->channel]);
+}
+
+static ssize_t mcp4922_write_powerdown(struct iio_dev *indio_dev,
+					 uintptr_t private,
+					 const struct iio_chan_spec *chan,
+					 const char *buf, size_t len)
+{
+	struct mcp4922_state *state = iio_priv(indio_dev);
+	bool powerdown;
+	int ret;
+
+	ret = strtobool(buf, &powerdown);
+	if (ret)
+		return ret;
+
+	if (powerdown) {
+		state->mosi[0] = (chan->channel == 0) ?
+				MCP4922_OUTA_POWER_DOWN :
+				MCP4922_OUTB_POWER_DOWN;
+		state->mosi[1] = 0x00;
+
+		ret = spi_write(state->spi, state->mosi, 2);
+	} else {
+		/* Restore previous voltage level */
+		ret = mcp4922_write_raw(indio_dev, chan,
+					state->value[chan->channel], 0,
+					IIO_CHAN_INFO_RAW);
+	}
+	if (!ret)
+		state->powerdown[chan->channel] = powerdown;
+
+	return ret ? ret : len;
+}
+
+static const struct iio_enum mcp4922_powerdown_mode_enum = {
+	.items = mcp4922_powerdown_modes,
+	.num_items = ARRAY_SIZE(mcp4922_powerdown_modes),
+	.get = mcp4922_get_powerdown_mode,
+	.set = mcp4922_set_powerdown_mode,
+};
+
+static const struct iio_chan_spec_ext_info mcp4922_ext_info[] = {
+	{
+		.name = "powerdown",
+		.read = mcp4922_read_powerdown,
+		.write = mcp4922_write_powerdown,
+		.shared = IIO_SEPARATE,
+	},
+	IIO_ENUM("powerdown_mode", IIO_SHARED_BY_TYPE,
+			&mcp4922_powerdown_mode_enum),
+	IIO_ENUM_AVAILABLE("powerdown_mode",
+			&mcp4922_powerdown_mode_enum),
+	{ },
+};
+
+#define MCP4922_CHAN(chan, bits) {			\
+	.type = IIO_VOLTAGE,				\
+	.output = 1,					\
+	.indexed = 1,					\
+	.channel = chan,				\
+	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),	\
+	.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),	\
+	.scan_type = {					\
+		.sign = 'u',				\
+		.realbits = (bits),			\
+		.storagebits = 16,			\
+		.shift = 12 - (bits),			\
+	},						\
+	.ext_info = mcp4922_ext_info,			\
+}
+
 static const struct iio_chan_spec mcp4922_channels[3][MCP4922_NUM_CHANNELS] = {
 	[ID_MCP4902] = { MCP4922_CHAN(0, 8),	MCP4922_CHAN(1, 8) },
 	[ID_MCP4912] = { MCP4922_CHAN(0, 10),	MCP4922_CHAN(1, 10) },

That's interesting. I'm not sure we have any consistency of interface around whether a write to the value when powered down results in the device powering up or not. It certainly feels like we should be consistent on this and document it.

I checked the first random driver I found the ad5360 and it appears to have the powerdown on the front end in some sense in that there is a specific bit to clear in order to power up again and it is not done by changing the current value.

To my mind that is the more logical option, but I'd like others opinions on this.

It's a little unusual to have an enum specified with just one value.. I can see the advantage in terms of this looking like every other powerdown mode control.

I don't suppose it hurts though. Seems a little pointless
to have this set function actually do anything though...

Also no really point in having the get actually read the value.

So I would just provide a stub for this that returns the same value ever time and nothing at all for the set.

Hi Jonathan, thank you for the review. I added a few comments below based on your feedback.

- Chris

Quoted 5 lines · Jonathan Cameron

That's interesting. I'm not sure we have any consistency of interface around whether a write to the value when powered down results in the device powering up or not. It certainly feels like we should be consistent on this and document it.

Quoted 8 lines

I checked the first random driver I found the ad5360 and it appears to have the powerdown on the front end in some sense in that there is a specific bit to clear in order to power up again and it is not done by changing the current value.

To my mind that is the more logical option, but I'd like others opinions on this.

I see what you mean. My intent was to mirror the behavior of userspace programs and APIs that I've written and seen for these chips. They take advantage of the fact a single command to the chip sets the voltage level and power state. That way there is no need to issue a separate power-up command, though it's available in sysfs if a user wants it (e.g., to toggle the channel's power state without specifying a new voltage level).

Like you, I'd be curious to hear what others have to say. My DAC experience is almost exclusively with Microchip devices, so I may be suffering from a bit of tunnel vision here.

Quoted 5 lines

It's a little unusual to have an enum specified with just one value.. I can see the advantage in terms of this looking like every other powerdown mode control.

Quoted 8 lines

I don't suppose it hurts though. Seems a little pointless
to have this set function actually do anything though...

Also no really point in having the get actually read the value.

So I would just provide a stub for this that returns the same value ever time and nothing at all for the set.

Makes sense. I was indeed trying to be consistent with the powerdown mode interface in other drivers, but as you note, there's no point in actually doing anything in these get/set functions, as the value will never change.