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!
>
--
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
