Re: [PATCH] staging: iio: adc: Enable driver support for ad7887 AD converter

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

...
>>> +
>>> +#define RES_MASK(bits)      ((1 << (bits)) - 1)
>> It feels like this ought to be a standard macro, but I can't find it in
>> any of the obvious headers...
> 
> I thought so too.
> But I also can't find it - maybe add to one of the iio common headers?
Or propose it for inclusion in bitops.h itself?


...
>>> +
>> Use one of the endian macros for this perhaps?
> 
> Can do but need to make data 16-bit aligned.
Ah I'd missed that it wasn't. My mistake. Leave this as it is.
> 
>>> +    return (st->data[(ch * 2)] << 8) | st->data[(ch * 2) + 1]; }
>>> +
...
> ok
> 
>>> +                    mode = 0;
>>> +
>>> +    return mode;
>>> +}
>>> +
>>> +static const struct attribute_group ad7887_attribute_group = {
>>> +    .attrs = ad7887_attributes,
>>> +    .is_visible = ad7887_attr_is_visible, };
>>> +
>>
>> So is there a plan to add more parts to this driver?  Usually one
>> doesn't put this code in until at's needed, but if more are following
>> shortly then it's fine with me. Perhaps add something to the commit
>> message to indicate that this is going to be needed by future device
>> support?
> 
> Plan is to add more drivers - indeed I already have added one.
> But I need to wait for test hardware to arrive.
Cool.
> 
>>
>>> +static const struct ad7887_chip_info ad7887_chip_info_tbl[] = {
>>> +    [ID_AD7887] = {
>>> +            .bits = 12,
>>> +            .storagebits = 16,
>>> +            .res_shift = 0,
>>> +            .sign = IIO_SCAN_EL_TYPE_UNSIGNED,
>>> +            .int_vref_mv = 2500,
>>> +    },
>>> +};
>>> +
>>> +static int __devinit ad7887_probe(struct spi_device *spi) {
>>> +    struct ad7887_platform_data *pdata = spi->dev.platform_data;
>>> +    struct ad7887_state *st;
>>> +    int ret, voltage_uv = 0;
>>> +
>>> +    st = kzalloc(sizeof(*st), GFP_KERNEL);
>>> +    if (st == NULL) {
>>> +            ret = -ENOMEM;
>>> +            goto error_ret;
>>> +    }
>>> +
>>> +    st->reg = regulator_get(&spi->dev, "vcc");
>>> +    if (!IS_ERR(st->reg)) {
>>> +            ret = regulator_enable(st->reg);
>>> +            if (ret)
>>> +                    goto error_put_reg;
>>> +
>>> +            voltage_uv = regulator_get_voltage(st->reg);
>> Technically you might want to register for the voltage change
>> notification from the regulator.  Some other driver might request a
>> different voltage and I don't think you have prevented it from changing.
> 
> I'm not quite sure what you mean. I don't think someone wants to change this voltage on the fly.
It would be a little odd, but you never know... Agreed, it probably isn't worth handling
in here.  If someone actually does do this, then we can add support.
...
>>> +    /* Ensure the timestamp is 8 byte aligned */
>>> +    d_size = bytes + sizeof(s64);
>>> +    if (d_size % sizeof(s64))
>> Spacing looks dubious round that minus sign. Would have thought
>> checkpatch would have moaned about that.
> 
> Actually checkpatch complained about having the spacing.
> I removed it to make checkpatch happy.
> Must be a bug in checkpatch, I'll revert.
Might be worth sending that example on as a bug report for checkpatch
Very strange response...
...

> Thanks for reviewing!
You are welcome.

Jonathan