...
>>> +
>>> +#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
--
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
