Re: [PATCH 2/2] iio: adc: New driver for AD7280A Lithium Ion Battery Monitoring System

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

On 07/18/11 13:48, Michael Hennerich wrote:
> On 07/18/2011 01:43 PM, Jonathan Cameron wrote:
>> On 07/15/11 13:59, michael.hennerich@analog.com wrote:
>>> From: Michael Hennerich<michael.hennerich@analog.com>
>> Hi Michael, although anyone reading the datasheets will quickly
>> discover that this isn't a simple laptop style battery monitor,
>> it's probably worth making that clear in the patch title or
>> at very least here.
>>
>> Driver is pretty clear. Couple of nitpicks / queries inline.
>>
>> Thanks
>>
>> Jonathan
>>>
>>> Signed-off-by: Michael Hennerich<michael.hennerich@analog.com>
>>> ---
...
>>> +static irqreturn_t ad7280_event_handler(int irq, void *private)
>>> +{
>>> +     struct iio_dev *dev_info = private;
>>> +
>>> +     iio_push_event(dev_info, 0,
>>> +                    IIO_UNMOD_EVENT_CODE(IIO_IN,
>>> +                                         0,
>>> +                                         IIO_EV_TYPE_THRESH,
>>> +                                         IIO_EV_DIR_EITHER),
>>> +                    iio_get_time_ns());
>> You have thresholds for temp and voltage below, but only voltage
>> event. I wonder if the right thing here is to issue two events
>> (subject to what is enabled).  If everything is turned on, there
>> doesn't seem to be anyway to tell what happened.  If the event
>> is consistent, I guess you could write a strobe function that would
>> enable events up the chain and see when it kicked in. That would
>> tell you where it came from.  No idea if one ever wants to know though.
> Alternatively I could read all channels in the stack and compare
> against the set thresholds. I think that would make the most sense here.
Good point. That's much simpler.
> 
>>
>>> +
>>> +     return IRQ_HANDLED;
>>> +}
>>> +
>>> +static IIO_DEVICE_ATTR(in_thresh_low_value,
>>> +                    S_IRUGO | S_IWUSR,
>>> +                    ad7280_read_channel_config,
>>> +                    ad7280_write_channel_config,
>>> +                    AD7280A_CELL_UNDERVOLTAGE);
>>> +
>>> +static IIO_DEVICE_ATTR(in_thresh_high_value,
>>> +                    S_IRUGO | S_IWUSR,
>>> +                    ad7280_read_channel_config,
>>> +                    ad7280_write_channel_config,
>>> +                    AD7280A_CELL_OVERVOLTAGE);
>>> +
>>> +static IIO_DEVICE_ATTR(temp_thresh_low_value,
>>> +                    S_IRUGO | S_IWUSR,
>>> +                    ad7280_read_channel_config,
>>> +                    ad7280_write_channel_config,
>>> +                    AD7280A_AUX_ADC_UNDERVOLTAGE);
>>> +
>>> +static IIO_DEVICE_ATTR(temp_thresh_high_value,
>>> +                    S_IRUGO | S_IWUSR,
>>> +                    ad7280_read_channel_config,
>>> +                    ad7280_write_channel_config,
>>> +                    AD7280A_AUX_ADC_OVERVOLTAGE);
>> These are shared over all channels?  Hmm. That's not a case
>> I'd considered when writing the chan spec stuff for events,
>> perhaps we need to allow for 'shared' events.  Technically
>> the only abi meeting approach right now is to have these
>> for every channel.  So write one channels threshold value
>> then a read would tell you they have all changed.
> Technically I could have different thresholds for any single
> device in the chain, each featuring 6 cell and 6 temp/aux channels.
> 
> But in practice, you would set them to all being the same.
> What prevents us from having the shared event thresholds?
The difficulty in concisely specifying them in the chan spec
structures.  Good point though, it's fine abi wise.  Maybe
just leave these as you have them here for now and we can
think about how to concisely map them into the chan_spec
stuff at a later date..
> 
>>> +
>>> +
>>> +static struct attribute *ad7280_event_attributes[] = {
>>> +&iio_dev_attr_in_thresh_low_value.dev_attr.attr,
>>> +&iio_dev_attr_in_thresh_high_value.dev_attr.attr,
>>> +&iio_dev_attr_temp_thresh_low_value.dev_attr.attr,
>>> +&iio_dev_attr_temp_thresh_high_value.dev_attr.attr,
>>> +     NULL,
>>> +};
>>> +
>>> +static struct attribute_group ad7280_event_attrs_group = {
>>> +     .attrs = ad7280_event_attributes,
>>> +};
>>> +
>>> +static int ad7280_read_raw(struct iio_dev *dev_info,
>>> +                        struct iio_chan_spec const *chan,
>>> +                        int *val,
>>> +                        int *val2,
>>> +                        long m)
>>> +{
>>> +     struct ad7280_state *st = iio_priv(dev_info);
>>> +     unsigned int scale_uv;
>>> +     int ret;
>>> +
>>> +     switch (m) {
>>> +     case 0:
>>> +             mutex_lock(&dev_info->mlock);
>>> +             switch (chan->type) {
>>> +             case IIO_IN:
>>> +                     ret = ad7280_read_channel(st, chan->address>>  8,
>>> +                                               chan->address&  0xFF);
>>> +                     break;
>>> +             case IIO_IN_DIFF:
>>> +                     ret = ad7280_read_all_channels(st, st->scan_cnt, NULL);
>> Err... Really need some explanation for what this channel is!
> Voltage across all cells.
Fair enough. So are the other voltages not differential as well? (across a given
cell rather than referred to a base voltage)
...

>>> +
>>> +#endif /* IIO_ADC_AD7280_H_ */
>> -- 
>> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>
> 
> Thanks for the review.
> 
You are welcome.