Hi!
On 25/11/15 13:43, Peter Meerwald-Stadler wrote:
>> New driver adding support for ADC found on Cirrus Logic EP93xx series of SoCs.
>> > Board specific code must take care to create platform device with all necessary
>> > resources.
> comments below
Thanks for spelling corrections :)
[...]
>> > + /* At this point conversion must be completed, but anyway... */
>> > + while (1) {
>> > + u32 t;
>> > +
>> > + t = ep93xx_adc_reg_read(priv->base + EP93XX_ADC_RESULT);
>> > + if (t & EP93XX_ADC_SDR) {
>> > + /* Sign extend lower 16 bits */
>> > + *value = (s16)t;
> there would be a nice sign_extend function
Yes, but we don't need its flexibility for the 16 bit case?
> maybe have a timeout?
Maybe... I'll add it...
>> > + break;
>> > + }
>> > + }
>> > + mutex_unlock(&priv->lock);
>> > + return IIO_VAL_INT;
>> > +
>> > + case IIO_CHAN_INFO_OFFSET:
>> > + /* According to datasheet, range is -25000..25000 */
> huh? so 0 V returns what value?
Manual says it returns ~-25000. This seems to be true in reality, but I cannot tell
you now if it could be even below -25000. But as I've understood, all the relevant
calculations in IIO core are signed, so this should not be a problem.
[...]
>> > + dev_err(&pdev->dev, "Cannot obtain clock");
> put \n at the end of the message
Yes, thanks for that notice...
[...]
>> > + ret = clk_enable(priv->clk);
>> > + if (ret) {
>> > + dev_err(&pdev->dev, "Cannot enable clock");
>> > + return ret;
>> > + }
>> > +
> clock should be disabled on error in the following error path
True... I'll try to postpone clock enabling to the very end...
[...]
>> > + ret = devm_iio_device_register(&pdev->dev, iiodev);
>> > + if (ret)
>> > + return ret;
>> > +
>> > + platform_set_drvdata(pdev, iiodev);
> this should be done before _register()
I don't agree here, because this is for platform device only and is
only used in _remove() below, not for IIO device.
> just do
> return devm_iio_device_register(..)
Thanks for review!
Regards,
Alexander.
--
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
