On 03/31/2011 06:18 PM, Jonathan Cameron wrote:
> On 03/31/11 15:56, michael.hennerich@xxxxxxxxxx wrote:
>
>> From: Michael Hennerich <michael.hennerich@xxxxxxxxxx>
>>
>>
>>
> Hi Michael,
>
> I wonder if it makes sense to handle an over temp warning
> as you have done here with a new rather complex threshold type.
> What do we loose by 'defining' a separate temperature channel?
> That would have no direct access and only exist for the purposes
> of the threshold.
>
Hi Jonathan,
I think you know that this threshold event is just an ALARM interrupt,
that signals
the junction temperature exceeded 110°C, and that the part therefore
automatically
entered thermal shutdown mode?
So there is no temperature that can be read...
> Thus we would have
> temp0_thresh_rising_value
Always 110000 mDeg C.
> and temp0_thresh_rising_en (always 1)
> The event code is then just a standard temperature one. What
> you currently have to my mind implies that the threshold is
> on the output voltage rather than the temperature.
>
I see you point here.
But can you tell me how the standard temperature would look like?
> Also need documentation for the powerdown attributes.
> (powerdown_mode seems to be specified, but se haven't had explicit
> per channel controls on this before).
>
>
There are no channel controls for powerdown_mode.
There are channel controls for outY_powerdown, but not for the mode.
All of them are already documented.
> I'm a little confused about what the powerdown attributes do consistent.
> Looks like read gives 1 if the relevant bit of pwr_down_mask is set.
> If you write a 1 it seems to unset that bit. Thus write 1 and
> you'll read back a 0.
>
Good catch, that's a bug.
> As ever, pretty clean. Thanks!
>
> Jonathan
>
>> Signed-off-by: Michael Hennerich <michael.hennerich@xxxxxxxxxx>
>> ---
>> drivers/staging/iio/dac/Kconfig | 10 +
>> drivers/staging/iio/dac/Makefile | 1 +
>> drivers/staging/iio/dac/ad5504.c | 429 ++++++++++++++++++++++++++++++++++++++
>> drivers/staging/iio/dac/ad5504.h | 74 +++++++
>> drivers/staging/iio/sysfs.h | 1 +
>> 5 files changed, 515 insertions(+), 0 deletions(-)
>> create mode 100644 drivers/staging/iio/dac/ad5504.c
>> create mode 100644 drivers/staging/iio/dac/ad5504.h
>>
>> diff --git a/drivers/staging/iio/dac/Kconfig b/drivers/staging/iio/dac/Kconfig
>> index 67defcb..1b0188a 100644
>> --- a/drivers/staging/iio/dac/Kconfig
>> +++ b/drivers/staging/iio/dac/Kconfig
>> @@ -21,6 +21,16 @@ config AD5446
>> To compile this driver as a module, choose M here: the
>> module will be called ad5446.
>>
>> +config AD5504
>> + tristate "Analog Devices AD5504/AD5501 DAC SPI driver"
>> + depends on SPI
>> + help
>> + Say yes here to build support for Analog Devices AD5504, AD5501,
>>
> Why reverse numerical order? (not that I really care, just curious ;)
>
The driver is named after the AD5504, so I thought I list it first.
Actually I don't really care, too.
>> + High Voltage Digital to Analog Converter.
>> +
>> + To compile this driver as a module, choose M here: the
>> + module will be called ad5504.
>> +
>>
> ...
>
>> +static ssize_t ad5504_read_powerdown_mode(struct device *dev,
>> + struct device_attribute *attr, char *buf)
>> +{
>> + struct iio_dev *indio_dev = dev_get_drvdata(dev);
>> + struct ad5504_state *st = iio_dev_get_devdata(indio_dev);
>> +
>>
> I count 14 characters needed. Also, why not const char* for these
> and share them across multiple driver instances.
>
ok
>> + char mode[][15] = {"20kohm_to_gnd", "three_state"};
>> +
>> + return sprintf(buf, "%s\n", mode[st->pwr_down_mode]);
>> +}
>> +
>>
> ...
>
>
>
>> +}
>> +
>> +static ssize_t ad5504_read_dac_powerdown(struct device *dev,
>> + struct device_attribute *attr,
>> + char *buf)
>> +{
>> + struct iio_dev *indio_dev = dev_get_drvdata(dev);
>> + struct ad5504_state *st = iio_dev_get_devdata(indio_dev);
>> + struct iio_dev_attr *this_attr = to_iio_dev_attr(attr);
>> +
>> + return sprintf(buf, "%d\n",
>>
> So, if mask is set, the channel is powered down?
> I wonder if flipping the logic here to have dac0_en etc might be
> more consistent with everything else? Perhaps not given it
> would disassociate this from the powerdown_mode attributes...
>
As I said the reversed logic is bug here.
I don't really like to call it enable. Since it is actually a power
down, associated with the
various modes.
>> + !!(st->pwr_down_mask & (1 << this_attr->address)));
>> +}
>> +
>> +static ssize_t ad5504_write_dac_powerdown(struct device *dev,
>> + struct device_attribute *attr,
>> + const char *buf, size_t len)
>> +{
>> + long readin;
>> + int ret;
>> + struct iio_dev *indio_dev = dev_get_drvdata(dev);
>> + struct ad5504_state *st = iio_dev_get_devdata(indio_dev);
>> + struct iio_dev_attr *this_attr = to_iio_dev_attr(attr);
>> +
>> + ret = strict_strtol(buf, 10, &readin);
>> + if (ret)
>> + return ret;
>> +
>> + if (readin == 0)
>> + st->pwr_down_mask |= (1 << this_attr->address);
>> + else if (readin == 1)
>> + st->pwr_down_mask &= ~(1 << this_attr->address);
>>
> Doesn't this technically mean this is a power up mask? If we right
> 1 to power down we unset the bit in this mask.
>
Unlike to the other DACs having this feature, it's here used as an power
up mask.
>> + else
>> + ret = -EINVAL;
>> +
>> + ret = ad5504_spi_write(st->spi, AD5504_ADDR_CTRL,
>> + AD5504_DAC_PWRDWN_MODE(st->pwr_down_mode) |
>> + AD5504_DAC_PWR(st->pwr_down_mask));
>> +
>> + /* writes to the CTRL register must be followed by a NOOP */
>> + ad5504_spi_write(st->spi, AD5504_ADDR_NOOP, 0);
>> +
>> + return ret ? ret : len;
>> +}
>> +
>>
> ...
>
>
>> +static struct attribute *ad5504_attributes[] = {
>> + &iio_dev_attr_out0_raw.dev_attr.attr,
>> + &iio_dev_attr_out1_raw.dev_attr.attr,
>> + &iio_dev_attr_out2_raw.dev_attr.attr,
>> + &iio_dev_attr_out3_raw.dev_attr.attr,
>> + &iio_dev_attr_out0_powerdown.dev_attr.attr,
>>
> Not actually documented as yet (I think). Please add that as well.
>
It is -
+What: /sys/bus/iio/devices/deviceX/outY_powerdown
+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.
>> + &iio_dev_attr_out1_powerdown.dev_attr.attr,
>> + &iio_dev_attr_out2_powerdown.dev_attr.attr,
>> + &iio_dev_attr_out3_powerdown.dev_attr.attr,
>> + &iio_dev_attr_out_powerdown_mode.dev_attr.attr,
>> + &iio_const_attr_out_powerdown_mode_available.dev_attr.attr,
>> + &iio_dev_attr_out_scale.dev_attr.attr,
>> + &iio_dev_attr_name.dev_attr.attr,
>> + NULL,
>> +};
>> +
>>
> Not convinced you are gaining much here vs just having two attribute
> tables and picking that based on the id...
>
ok
>> +static mode_t ad5504_attr_is_visible(struct kobject *kobj,
>> + struct attribute *attr, int n)
>> +{
>> + struct device *dev = container_of(kobj, struct device, kobj);
>> + struct iio_dev *indio_dev = dev_get_drvdata(dev);
>> + struct ad5504_state *st = iio_dev_get_devdata(indio_dev);
>> +
>> + mode_t mode = attr->mode;
>> +
>> + if (spi_get_device_id(st->spi)->driver_data == ID_AD5501 &&
>> + (attr == &iio_dev_attr_out1_raw.dev_attr.attr ||
>> + attr == &iio_dev_attr_out2_raw.dev_attr.attr ||
>> + attr == &iio_dev_attr_out3_raw.dev_attr.attr ||
>> + attr == &iio_dev_attr_out1_powerdown.dev_attr.attr ||
>> + attr == &iio_dev_attr_out2_powerdown.dev_attr.attr ||
>> + attr == &iio_dev_attr_out3_powerdown.dev_attr.attr))
>> + mode = 0;
>> +
>> + return mode;
>> +}
>> +
>> +static const struct attribute_group ad5504_attribute_group = {
>> + .attrs = ad5504_attributes,
>> + .is_visible = ad5504_attr_is_visible,
>> +};
>> +
>> +static IIO_CONST_ATTR(out_alarm_overtemp_thresh_high_value, "110000");
>> +
>> +static struct attribute *ad5504_ev_attributes[] = {
>> + &iio_const_attr_out_alarm_overtemp_thresh_high_value.dev_attr.attr,
>>
> For consistency we also want the _en element, (always 1 of course).
>
ok
>
>> + NULL,
>> +};
>> +
>> +static struct attribute_group ad5504_ev_attribute_group = {
>> + .attrs = ad5504_ev_attributes,
>> +};
>> +
>> +static void ad5504_interrupt_bh(struct work_struct *work_s)
>> +{
>> + struct ad5504_state *st = container_of(work_s,
>> + struct ad5504_state, work_alarm);
>> +
>> + iio_push_event(st->indio_dev, 0,
>> + IIO_UNMOD_EVENT_CODE(IIO_EV_CLASS_OUT,
>> + 0,
>> + IIO_EV_TYPE_THRESH,
>> + IIO_EV_DIR_RISING),
>> + st->last_timestamp);
>>
> Not happy with this event type. See intro.
>
Please propose a better one.
>> +
>> + enable_irq(st->spi->irq);
>> +}
>> +
>>
> ...
>
>
>> +#define AD5504_ADDR_ALL_DAC 5
>> +#define AD5504_ADDR_CTRL 7
>> +
>> +/* Control Register */
>> +#define AD5504_DAC_PWR(ch) ((ch) << 2)
>> +#define AD5504_DAC_PWRDWN_MODE(mode) ((mode) << 6)
>> +#define AD5504_DAC_PWRDN_20K 0
>> +#define AD5504_DAC_PWRDN_3STATE 1
>> +
>> +/*
>> + * TODO: struct ad5504_platform_data needs to go into include/linux/iio
>> + */
>> +
>> +struct ad5504_platform_data {
>> + u16 vref_mv;
>> +};
>>
> Would it matter to you if we stopped doing this vref passing in platform
> data and started insisting on regulator always being queriable? (came up in
> the max1363 discussion of yesterday?) When we first started doing this
> almost no boards had regulators specified. Is this still true?
>
The regulator stuff is not really common.
I always try to support it as an alternative way, since I know passing
platform dependent information
through platform_data is a bit inconvenient on systems using devicetree.
A bandgap reference directly connected to an ADC or DAC, can be
considered as a fixed voltage regulator.
However there is no way to enable or disable it. it's a pretty
complicated way telling the driver it's reference.
I would propose to always support the regulator framework, however I
don't mind having the platform_data approach as well.
>> +
>> +/**
>> + * struct ad5446_state - driver instance specific data
>> + * @indio_dev: the industrial I/O device
>> + * @us: spi_device
>> + * @reg: supply regulator
>> + * @vref_mv: actual reference voltage used
>> + * @work_alarm: bh work structure for event handling
>> + * @last_timestamp: timestamp of last event interrupt
>> + * @pwr_down_mask power down mask
>> + * @pwr_down_mode current power down mode
>> + */
>> +
>> +struct ad5504_state {
>> + struct iio_dev *indio_dev;
>> + struct spi_device *spi;
>> + struct regulator *reg;
>> + unsigned short vref_mv;
>> + struct work_struct work_alarm;
>> + s64 last_timestamp;
>> + unsigned pwr_down_mask;
>> + unsigned pwr_down_mode;
>> +};
>> +
>> +/**
>> + * ad5504_supported_device_ids:
>> + */
>> +
>> +enum ad5504_supported_device_ids {
>> + ID_AD5504,
>> + ID_AD5501,
>> +};
>> +
>> +#endif /* SPI_AD5504_H_ */
>> diff --git a/drivers/staging/iio/sysfs.h b/drivers/staging/iio/sysfs.h
>> index 24b74dd..9e3b784 100644
>> --- a/drivers/staging/iio/sysfs.h
>> +++ b/drivers/staging/iio/sysfs.h
>> @@ -266,6 +266,7 @@ struct iio_const_attr {
>> #define IIO_EV_CLASS_MAGN 4
>> #define IIO_EV_CLASS_LIGHT 5
>> #define IIO_EV_CLASS_PROXIMITY 6
>> +#define IIO_EV_CLASS_OUT 7
>>
> This is fine but I think shouldn't be used here so shouldn't be in the patch!
>
ok
>> #define IIO_EV_MOD_X 0
>> #define IIO_EV_MOD_Y 1
>>
>
--
Greetings,
Michael
--
Analog Devices GmbH Wilhelm-Wagenfeld-Str. 6 80807 Muenchen
Sitz der Gesellschaft: Muenchen; Registergericht: Muenchen HRB 40368;
Geschaeftsfuehrer:Dr.Carsten Suckrow, Thomas Wessel, William A. Martin,
Margaret Seif
--
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
