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 devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Reply via email to