linux-mips
[Top] [All Lists]

Re: [PATCH v2 22/26] MFD: Add JZ4740 ADC driver

To: Lars-Peter Clausen <lars@metafoo.de>
Subject: Re: [PATCH v2 22/26] MFD: Add JZ4740 ADC driver
From: Samuel Ortiz <sameo@linux.intel.com>
Date: Mon, 5 Jul 2010 16:53:08 +0200
Cc: Ralf Baechle <ralf@linux-mips.org>, linux-mips@linux-mips.org, linux-kernel@vger.kernel.org
In-reply-to: <1276924111-11158-23-git-send-email-lars@metafoo.de>
Original-recipient: rfc822;linux-mips@linux-mips.org
References: <1276924111-11158-1-git-send-email-lars@metafoo.de> <1276924111-11158-23-git-send-email-lars@metafoo.de>
Sender: linux-mips-bounce@linux-mips.org
User-agent: Mutt/1.5.20 (2009-06-14)
Hi Lars,

Sorry for the review delay. This one got lost in my inbox :-/

On Sat, Jun 19, 2010 at 07:08:27AM +0200, Lars-Peter Clausen wrote:
> This patch adds a MFD driver for the JZ4740 ADC unit. The driver is used to
> demultiplex IRQs and synchronize access to shared registers between the 
> battery,
> hwmon and (future) touchscreen driver.
The driver looks pretty clean. I only have a couple nitpicks:

> +static void jz4740_adc_irq_demux(unsigned int irq, struct irq_desc *desc)
> +{
> +     struct jz4740_adc *adc = get_irq_desc_data(desc);
> +     uint8_t status;
> +     unsigned int i;
> +
> +     status = readb(adc->base + JZ_REG_ADC_STATUS);
> +
> +     for (i = 0; i < 5; ++i) {
> +             if (status & BIT(i))
> +                     generic_handle_irq(adc->irq_base + i);
for_each_set_bit() could nicely replace your loop here.

> +static inline void jz4740_adc_clk_enable(struct jz4740_adc *adc)
> +{
> +     unsigned long flags;
> +
> +     spin_lock_irqsave(&adc->lock, flags);
> +     if (adc->clk_ref++ == 0)
> +             clk_enable(adc->clk);
> +     spin_unlock_irqrestore(&adc->lock, flags);
> +}
I'm not familiar with your platform clock framework, but shouldn't the
refcounting be handled there instead of spread over all your drivers ?

> +static int __devinit jz4740_adc_probe(struct platform_device *pdev)
> +{
> +     int ret;
> +     struct jz4740_adc *adc;
> +     struct resource *mem_base;
> +     int irq;
> +
> +     adc = kmalloc(sizeof(*adc), GFP_KERNEL);
> +
> +     adc->irq = platform_get_irq(pdev, 0);
> +     if (adc->irq < 0) {
> +             ret = adc->irq;
> +             dev_err(&pdev->dev, "Failed to get platform irq: %d\n", ret);
> +             goto err_free;
> +     }
> +
> +     adc->irq_base = platform_get_irq(pdev, 1);
> +     if (adc->irq_base < 0) {
> +             ret = adc->irq_base;
> +             dev_err(&pdev->dev, "Failed to get irq base: %d\n", ret);
> +             goto err_free;
> +     }
> +
> +     mem_base = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +     if (!mem_base) {
> +             ret = -ENOENT;
> +             dev_err(&pdev->dev, "Failed to get platform mmio resource\n");
> +             goto err_free;
> +     }
> +
> +     /* Only request the shared registers for the MFD driver */
> +     adc->mem = request_mem_region(mem_base->start, JZ_REG_ADC_STATUS,
> +                                     pdev->name);
> +     if (!adc->mem) {
> +             ret = -EBUSY;
> +             dev_err(&pdev->dev, "Failed to request mmio memory region\n");
> +             goto err_free;
> +     }
> +
> +     adc->base = ioremap_nocache(adc->mem->start, resource_size(adc->mem));
> +     if (!adc->base) {
> +             ret = -EBUSY;
> +             dev_err(&pdev->dev, "Failed to ioremap mmio memory\n");
> +             goto err_release_mem_region;
> +     }
> +
> +     adc->clk = clk_get(&pdev->dev, "adc");
> +     if (IS_ERR(adc->clk)) {
> +             ret = PTR_ERR(adc->clk);
> +             dev_err(&pdev->dev, "Failed to get clock: %d\n", ret);
> +             goto err_iounmap;
> +     }
> +
> +     spin_lock_init(&adc->lock);
> +
> +     adc->clk_ref = 0;
> +
> +     platform_set_drvdata(pdev, adc);
> +
> +     for (irq = adc->irq_base; irq < adc->irq_base + 5; ++irq) {
> +             set_irq_chip_data(irq, adc);
> +             set_irq_chip_and_handler(irq, &jz4740_adc_irq_chip,
> +                 handle_level_irq);
> +     }
> +
> +     set_irq_data(adc->irq, adc);
> +     set_irq_chained_handler(adc->irq, jz4740_adc_irq_demux);
> +
> +     writeb(0x00, adc->base + JZ_REG_ADC_ENABLE);
> +     writeb(0xff, adc->base + JZ_REG_ADC_CTRL);
> +
> +     mfd_add_devices(&pdev->dev, 0, jz4740_adc_cells,
> +             ARRAY_SIZE(jz4740_adc_cells), mem_base, adc->irq_base);
> +
> +     return 0;
Please return the mfd_add_devices return value.

Cheers,
Samuel.

-- 
Intel Open Source Technology Centre
http://oss.intel.com/

<Prev in Thread] Current Thread [Next in Thread>