Re: [PATCH] IIO:DAC: AD5624R: Update to IIO ABI

[Jonathan Cameron <jic23@cam.ac.uk>]

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@analog.com wrote:
>>   
>>> From: Michael Hennerich <michael.hennerich@analog.com>
>>>
>>> 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@vger.kernel.org
>>> +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;
>>> +};

...