On 01/23/2013 05:58 AM, Naveen Krishna Chatradhi wrote: > This patch add an ADC IP found on EXYNOS5 series socs from Samsung. > Also adds the Documentation for device tree bindings. > > Signed-off-by: Naveen Krishna Chatradhi <ch.naveen@xxxxxxxxxxx> > --- > Changes since v1: > > 1. Fixed comments from Lars > 2. Added support for ADC on EXYNOS5410 > > Changes since v2: > > 1. Changed the instance name for (struct iio_dev *) to indio_dev > 2. Changed devm_request_irq to request_irq > > Few doubts regarding the mappings and child device handling. > Kindly, suggest me better methods. > Hi, The patch looks mostly good now. As for the mappings, the problem is that we currently do not have any device tree bindings for IIO. So a proper solution would be to add dt bindings for IIO. [...] > diff --git a/drivers/iio/adc/exynos5_adc.c b/drivers/iio/adc/exynos5_adc.c > new file mode 100644 > index 0000000..8982675 > --- /dev/null > +++ b/drivers/iio/adc/exynos5_adc.c [...] > + > +/** ADC core in EXYNOS5410 has 10 channels, > + * ADC core in EXYNOS5250 has 8 channels > +*/ Minor nitpick, according to Documentation/Codingsytle multi-line comments should look like this: /* * Text * Text */ [...] > +static int exynos5_adc_probe(struct platform_device *pdev) > +{ > + struct exynos5_adc *info = NULL; > + struct device_node *np = pdev->dev.of_node; > + struct iio_dev *indio_dev = NULL; > + struct resource *mem; > + int ret = -ENODEV; > + int irq; > + > + if (!np) > + return ret; > + > + indio_dev = iio_device_alloc(sizeof(struct exynos5_adc)); > + if (!indio_dev) { > + dev_err(&pdev->dev, "failed allocating iio device\n"); > + return -ENOMEM; > + } > + > + info = iio_priv(indio_dev); > + > + mem = platform_get_resource(pdev, IORESOURCE_MEM, 0); > + > + info->regs = devm_request_and_ioremap(&pdev->dev, mem); While devm_request_and_ioremap takes care of the printing and error you still need to check whether regs is non NULL. > + > + irq = platform_get_irq(pdev, 0); > + if (irq < 0) { > + dev_err(&pdev->dev, "no irq resource?\n"); > + ret = irq; > + goto err_iio; > + } > + > + info->irq = irq; > + > + ret = request_irq(info->irq, exynos5_adc_isr, > + 0, dev_name(&pdev->dev), info); > + if (ret < 0) { > + dev_err(&pdev->dev, "failed requesting irq, irq = %d\n", > + info->irq); > + goto err_iio; > + } > + > + info->clk = devm_clk_get(&pdev->dev, "adc"); > + if (IS_ERR(info->clk)) { > + dev_err(&pdev->dev, "failed getting clock, err = %ld\n", > + PTR_ERR(info->clk)); > + ret = PTR_ERR(info->clk); > + goto err_irq; > + } > + > + info->vdd = devm_regulator_get(&pdev->dev, "vdd"); > + if (IS_ERR(info->vdd)) { > + dev_err(&pdev->dev, "failed getting regulator, err = %ld\n", > + PTR_ERR(info->vdd)); > + ret = PTR_ERR(info->vdd); > + goto err_irq; > + } > + > + info->version = exynos5_adc_get_version(pdev); > + > + platform_set_drvdata(pdev, indio_dev); > + > + init_completion(&info->completion); Since the completion is used in the IRQ handler it should be initialized before the IRQ is requested. > + > + indio_dev->name = dev_name(&pdev->dev); > + indio_dev->dev.parent = &pdev->dev; > + indio_dev->dev.of_node = pdev->dev.of_node; > + indio_dev->info = &exynos5_adc_iio_info; > + indio_dev->modes = INDIO_DIRECT_MODE; > + indio_dev->channels = exynos5_adc_iio_channels; > + indio_dev->num_channels = ARRAY_SIZE(exynos5_adc_iio_channels); Shouldn't the number of channels depend on the ADC version? E.g. 8 for v1 and 10 for v2? > + > + info->map = exynos5_adc_iio_maps; > + > + ret = iio_map_array_register(indio_dev, info->map); > + if (ret) { > + dev_err(&indio_dev->dev, "failed mapping iio, err: %d\n", ret); > + goto err_irq; > + } > + > + ret = iio_device_register(indio_dev); > + if (ret) > + goto err_map; > + > + ret = regulator_enable(info->vdd); > + if (ret) > + goto err_iio_dev; You never disable the regulator again. I think you need to do that before it is freed. > + > + clk_prepare_enable(info->clk); > + > + exynos5_adc_hw_init(info); > + > + ret = of_platform_populate(np, exynos5_adc_match, NULL, &pdev->dev); > + if (ret < 0) { > + dev_err(&pdev->dev, "failed adding child nodes\n"); > + goto err_of_populate; > + } > + > + return 0; > + > +err_of_populate: > + device_for_each_child(&pdev->dev, NULL, exynos5_adc_remove_devices); > +err_iio_dev: > + iio_device_unregister(indio_dev); > +err_map: > + iio_map_array_unregister(indio_dev, info->map); > +err_irq: > + free_irq(info->irq, info); > +err_iio: > + iio_device_free(indio_dev); > + return ret; > +} > + [..] -- 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