Hi Hartmut,
On 12/10/2014 08:43 PM, Hartmut Knaack wrote:
> Ezequiel Garcia schrieb am 27.11.2014 um 16:39:
>> From: Phani Movva <Phani.Movva@xxxxxxxxxx>
>>
>> This commit adds support for Cosmic Circuits 10001 10-bit ADC device.
> Still some issues left, unfortunately things I pointed out in my last review.
Ouch, sorry about that.
[..]
>> +
>> +#define CC10001_ADC_DATA_MASK GENMASK(9, 0)
>> +#define CC10001_ADC_NUM_CHANNELS 8
>> +#define CC10001_ADC_CH_MASK GENMASK(CC10001_ADC_NUM_CHANNELS - 1, 0)
> This is only valid for one of the cases you make (wrong) use of CC10001_ADC_CH_MASK.
> As you seem to use this mask to separate channel selection values (0 - 7) in register
> CC10001_ADC_CONFIG, a GENMASK(2, 0) would be appropriate.
> In case of the indio_dev->channel_mask, you would actually need this
> GENMASK(CC10001_ADC_NUM_CHANNELS - 1, 0).
Right.
[..]
>> +static int cc10001_adc_probe(struct platform_device *pdev)
>> +{
>> + struct device_node *node = pdev->dev.of_node;
>> + struct cc10001_adc_device *adc_dev;
>> + unsigned long adc_clk_rate;
>> + struct resource *res;
>> + struct iio_dev *indio_dev;
>> + int ret;
>> +
>> + indio_dev = devm_iio_device_alloc(&pdev->dev, sizeof(*adc_dev));
>> + if (indio_dev == NULL)
>> + return -ENOMEM;
>> +
>> + adc_dev = iio_priv(indio_dev);
>> +
>> + adc_dev->channel_map = CC10001_ADC_CH_MASK;
> So, here you need a GENMASK(CC10001_ADC_NUM_CHANNELS - 1, 0).
Right.
>> + if (!of_property_read_u32(node, "cosmic,reserved-adc-channels", &ret))
>> + adc_dev->channel_map &= ~ret;
>> +
>> + adc_dev->reg = devm_regulator_get(&pdev->dev, "vref");
>> + if (IS_ERR(adc_dev->reg))
>> + return -EINVAL;
> Preferably pass up the real error code with PTR_ERR(adc_dev->reg).
OK.
Thanks for all your reviews!
--
Ezequiel
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
