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

[Hartmut Knaack <knaack.h-Mmb7MZpHnFY@public.gmane.org>]

Ezequiel Garcia schrieb am 25.11.2014 23:03:
> 
> 
> On 11/25/2014 06:41 PM, Hartmut Knaack wrote:
>>>
>>> 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;
> 
>>>>> +
>>>>> +	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.
>>>>> +
>>>>> +	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.
>>>
>>>>> +		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.
>>>>> +		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.
> 
>>>
>>>>> +		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!
>