Re: [PATCH 5/5] iio: adc: New driver for Cirrus Logic EP93xx ADC

[Alexander Sverdlin <alexander.sverdlin@gmail.com>]

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.