Re: [PATCH v3 1/3] iio: adc: Cosmic Circuits 10001 ADC driver

Ezequiel Garcia schrieb am 25.11.2014 23:03:

On 11/25/2014 06:41 PM, Hartmut Knaack wrote:

quoted

quoted

Unless I'm missing something, that's exactly what XOR does.

Example:

reserved_channels = 0x2;
channel_map = 0x7 ^ reserved_channels -> 0x5

You miss to check ret for a valid range, which you don't need to do when masking out channels.
Example:
reserved_channels = 0x8;
channel_map = 0x7 ^ reserved_channels -> 0xf
And this is actually happening in the current revision (with the wrong channel_map assignment), that a reserved channel in DT would become usable.

Right, definitely a check is needed.

No, that's not needed when using AND, as it can not add bits in the channel map. The following will do the job:
		adc_dev->channel_map &= ~ret;

quoted

quoted

quoted

quoted

+
+	adc_dev->reg = devm_regulator_get(&pdev->dev, "vref");
+	if (IS_ERR_OR_NULL(adc_dev->reg))
+		return -EINVAL;

	if (IS_ERR(adc_dev->reg))
		return PTR_ERR(adc_dev->reg);

Are you sure? What if devm_regulator_get() returns NULL?

I had a look at it, but couldn't figure out a condition in which it would return NULL. But I'd be happy to be informed of anything different.

If CONFIG_REGULATOR=n you get NULL there (although I added a select REGULATOR
on the v4 I just posted).

Indeed, thanks. Now I'm thinking if it would make sense to handle this special case in a special way (as it would be caused by an invalid kernel config). Well, I guess, I leave it up to you.

quoted

quoted

quoted

quoted

+
+	ret = regulator_enable(adc_dev->reg);
+	if (ret)
+		return ret;
+
+	indio_dev->dev.parent = &pdev->dev;
+	indio_dev->name = dev_name(&pdev->dev);
+	indio_dev->info = &cc10001_adc_info;
+	indio_dev->modes = INDIO_DIRECT_MODE;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	adc_dev->reg_base = devm_ioremap_resource(&pdev->dev, res);
+	if (IS_ERR(adc_dev->reg_base))

Need to put error code into ret.

Right.

quoted

quoted

+		goto err_disable_reg;
+
+	adc_dev->adc_clk = devm_clk_get(&pdev->dev, "adc");
+	if (IS_ERR(adc_dev->adc_clk)) {
+		dev_err(&pdev->dev, "Failed to get the clock\n");

Need to put error code into ret.

quoted

+		goto err_disable_reg;
+	}
+
+	ret = clk_prepare_enable(adc_dev->adc_clk);
+	if (ret) {
+		dev_err(&pdev->dev, "Failed to enable the clock\n");
+		goto err_disable_reg;
+	}
+
+	adc_clk_rate = clk_get_rate(adc_dev->adc_clk);
+	if (!adc_clk_rate) {
+		ret = -EINVAL;
+		dev_err(&pdev->dev, "null clock rate!\n");

Start message with upper case?

Actually, I'd rather make the others start with lower case.

Fair enough, unless they are full sentences like here. My english grammar tells me they should start with capital letters.

Yeah, but it's not the start of the message, as it's prefixed
with the device stuff.

quoted

quoted

quoted

quoted

+		goto err_disable_clk;
+	}
+
+	adc_dev->eoc_delay_ns = NSEC_PER_SEC / adc_clk_rate;
+	adc_dev->start_delay_ns = adc_dev->eoc_delay_ns * CC10001_WAIT_CYCLES;
+
+	/* Setup the ADC channels available on the device */
+	ret = cc10001_adc_channel_init(indio_dev);
+	if (ret < 0) {
+		dev_err(&pdev->dev, "Could not initialize the channels.\n");
+		goto err_disable_clk;
+	}
+
+	mutex_init(&adc_dev->lock);
+
+	ret = iio_triggered_buffer_setup(indio_dev, NULL,
+					 &cc10001_adc_trigger_h, NULL);
+	if (ret < 0)
+		goto err_disable_clk;
+
+	ret = iio_device_register(indio_dev);
+	if (ret < 0)
+		goto err_cleanup_buffer;
+
+	platform_set_drvdata(pdev, indio_dev);

Move this above iio_device_register.

What for?

To make iio_device_register the last operation of the probe.

I really don't think it matters, since platform_get_drvdata
is only called in the remove.

I'll move it if you think it's worth it, though.

Thanks for the review!