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
