On 03/09/11 13:42, Michael Hennerich wrote: > On 03/09/2011 11:52 AM, Jonathan Cameron wrote: >> On 03/08/11 14:15, michael.hennerich@xxxxxxxxxx wrote: >> >>> From: Michael Hennerich <michael.hennerich@xxxxxxxxxx> >>> >>> This driver did not conform with the IIO ABI for such devices. >>> Also the sysfs files that this driver adds were not complete and >>> partially un-documented. >>> >>> Update and document ABI >>> Change License notice, stick to GPL-v2. >>> Fix indention style >>> Add option to specify external reference voltage via the regulator framework. >>> Add mandatory name attribute >>> Add mandatory out_scale attribute >>> >>> >> Hi Michael, >> >> Another nice bit of cleaning up. >> >> Couple of trivial bits and bobs inline. Only one I really care about is >> that documentation of powerdown_mode should include descriptions of >> all options that are used. Do that or convince me otherwise before >> sending on. >> >> > I'll add the existing options, but there might be some more > - depending on the parts were going to add in future. Yup. We'll add those when they occur. ... >>> +What: /sys/bus/iio/devices/deviceX/out_powerdown >>> +KernelVersion: 2.6.38 >>> +Contact: linux-iio@xxxxxxxxxxxxxxx >>> +Description: >>> + Writing 1 causes output Y to enter the power down mode specified >>> + by the corresponding outY_powerdown_mode. Clearing returns to >>> + normal operation. Y may be suppressed if all outputs are >>> + controlled together. >>> >> I'm happy with the interface you've defined here, but do vaguely wonder if there >> is anything similar in kernel already... Can't immediately think where, but worth >> keeping in mind. >> > There is global power management support CONFIG_PM, allowing you to > shutdown devices during sleep or hibernate. > It might make sense, to support it here as well. But power down a set of > outputs may be required during normal operation as well. True. Guess we put it here until something better comes along. .. >>> +/** >>> + * struct ad5624r_chip_info - chip specific information >>> + * @bits: accuracy of the DAC in bits >>> + * @int_vref_mv: AD5620/40/60: the internal reference voltage >>> + */ >>> + >>> +struct ad5624r_chip_info { >>> + u8 bits; >>> + u16 int_vref_mv; >>> +}; >>> + >>> >> Whilst I have no particular problem with having these structures defined >> in the header, is there a particular reason for doing so? >> > No - If you prefer - I'll move them back int the main driver file. I don't mind. Just in general go for minimal changes where possible. >>> +/** >>> + * struct ad5446_state - driver instance specific data >>> + * @indio_dev: the industrial I/O device >>> + * @us: spi_device >>> + * @chip_info: chip model specific constants, available modes etc >>> + * @reg: supply regulator >>> + * @vref_mv: actual reference voltage used >>> + * @pwr_down_mask power down mask >>> + * @pwr_down_mode current power down mode >>> + */ >>> + >>> +struct ad5624r_state { >>> + struct iio_dev *indio_dev; >>> + struct spi_device *us; >>> + const struct ad5624r_chip_info *chip_info; >>> + struct regulator *reg; >>> + unsigned short vref_mv; >>> + unsigned pwr_down_mask; >>> + unsigned pwr_down_mode; >>> +}; ... -- To unsubscribe from this list: send the line "unsubscribe linux-iio" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html