Hi Jonathan, thank you for the review. I added a few comments below based on your feedback. - Chris On Mon, Oct 08, 2018 at 09:08:29PM +0100, Jonathan Cameron wrote: > 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. > 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. > 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. > 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.
