| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <50FFDD2A.9040102@metafoo.de>
Date: Wed, 23 Jan 2013 13:52:58 +0100
From: Lars-Peter Clausen <lars@...afoo.de>
To: Naveen Krishna Chatradhi <ch.naveen@...sung.com>
CC: linux-iio@...r.kernel.org, linux-kernel@...r.kernel.org,
linux-samsung-soc@...r.kernel.org, dianders@...omium.org,
gregkh@...uxfoundation.org, naveenkrishna.ch@...il.com
Subject: Re: [PATCH] iio: adc: add exynos5 adc driver under iio framwork
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@...sung.com>
> ---
> 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-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists