| 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: <56420606.4070505@metafoo.de>
Date: Tue, 10 Nov 2015 15:58:14 +0100
From: Lars-Peter Clausen <lars@...afoo.de>
To: Haibo Chen <haibo.chen@...escale.com>, robh+dt@...nel.org,
pawel.moll@....com, mark.rutland@....com,
ijc+devicetree@...lion.org.uk, galak@...eaurora.org,
shawnguo@...nel.org, kernel@...gutronix.de, linux@....linux.org.uk,
jic23@...nel.org, knaack.h@....de, pmeerw@...erw.net
CC: devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
linux-arm-kernel@...ts.infradead.org, linux-iio@...r.kernel.org
Subject: Re: [PATCH v2 1/4] iio: adc: add IMX7D ADC driver support
On 11/09/2015 02:28 PM, Haibo Chen wrote:
> Freescale i.MX7D soc contains a new ADC IP. This patch add this ADC
> driver support, and the driver only support ADC software trigger.
>
> Signed-off-by: Haibo Chen <haibo.chen@...escale.com>
Looks pretty good, a few comments inline.
[...]
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/interrupt.h>
> +#include <linux/delay.h>
> +#include <linux/kernel.h>
> +#include <linux/slab.h>
> +#include <linux/io.h>
> +#include <linux/clk.h>
> +#include <linux/completion.h>
> +#include <linux/of.h>
> +#include <linux/of_irq.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/of_platform.h>
> +#include <linux/err.h>
I don't think you need all of these.
[...]
> +static void imx7d_adc_feature_config(struct imx7d_adc *info)
> +{
> + info->adc_feature.clk_pre_div = IMX7D_ADC_ANALOG_CLK_PRE_DIV_4;
> + info->adc_feature.avg_num = IMX7D_ADC_AVERAGE_NUM_32;
> + info->adc_feature.core_time_unit = 1;
> + info->adc_feature.average_en = true;
What's the plan for these? Right now they are always initialized to the same
static value.
> +}
[...]
> +static int imx7d_adc_read_raw(struct iio_dev *indio_dev,
> + struct iio_chan_spec const *chan,
> + int *val,
> + int *val2,
> + long mask)
> +{
> + struct imx7d_adc *info = iio_priv(indio_dev);
> +
> + u32 channel;
> + long ret;
> +
> + switch (mask) {
> + case IIO_CHAN_INFO_RAW:
> + mutex_lock(&indio_dev->mlock);
> + reinit_completion(&info->completion);
> +
> + channel = (chan->channel) & 0x0f;
> + info->channel = channel;
> + imx7d_adc_channel_set(info);
How about just passing channel directy adc_channel_set() instead of doing it
implicitly through the info struct?
[...]
> +static irqreturn_t imx7d_adc_isr(int irq, void *dev_id)
> +{
> + struct imx7d_adc *info = (struct imx7d_adc *)dev_id;
> + int status;
> +
> + status = readl(info->regs + IMX7D_REG_ADC_INT_STATUS);
> + if (status & IMX7D_REG_ADC_INT_STATUS_CHANNEL_INT_STATUS) {
> + info->value = imx7d_adc_read_data(info);
> + complete(&info->completion);
> + }
> + writel(0, info->regs + IMX7D_REG_ADC_INT_STATUS);
Is the hardware really this broken? If the interrupt happens between reading
the status register and clearing it here it will be missed.
> +
> + return IRQ_HANDLED;
You should only return IRQ_HANDLED if you actually handled are interrupt.
> +}
[...]
> +
> +static int imx7d_adc_probe(struct platform_device *pdev)
> +{
> + struct imx7d_adc *info;
> + struct iio_dev *indio_dev;
> + struct resource *mem;
> + int irq;
> + int ret;
> + u32 channels;
> +
> + indio_dev = devm_iio_device_alloc(&pdev->dev, sizeof(struct imx7d_adc));
> + if (!indio_dev) {
> + dev_err(&pdev->dev, "Failed allocating iio device\n");
> + return -ENOMEM;
> + }
> +
> + info = iio_priv(indio_dev);
> + info->dev = &pdev->dev;
> +
> + mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + info->regs = devm_ioremap_resource(&pdev->dev, mem);
> + if (IS_ERR(info->regs)) {
> + ret = PTR_ERR(info->regs);
> + dev_err(&pdev->dev, "failed to remap adc memory: %d\n", ret);
> + return ret;
> + }
> +
> + irq = platform_get_irq(pdev, 0);
> + if (irq < 0) {
> + dev_err(&pdev->dev, "no irq resource?\n");
> + return irq;
> + }
> +
> + ret = devm_request_irq(info->dev, irq,
> + imx7d_adc_isr, 0,
> + dev_name(&pdev->dev), info);
This is too early. Completion is not initialized, clocks are not enabled, etc...
> + if (ret < 0) {
> + dev_err(&pdev->dev, "failed requesting irq, irq = %d\n", irq);
> + return ret;
> + }
> +
[...]
> + ret = of_property_read_u32(pdev->dev.of_node,
Extra space.
> + "num-channels", &channels);
What decides how many channels are in use?
> + if (ret)
> + channels = ARRAY_SIZE(imx7d_adc_iio_channels);
> +
> + 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 = &imx7d_adc_iio_info;
> + indio_dev->modes = INDIO_DIRECT_MODE;
> + indio_dev->channels = imx7d_adc_iio_channels;
> + indio_dev->num_channels = (int)channels;
I don't think you need the case here.
>[...]
--
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