From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1758365AbaGXA7F (ORCPT ); Wed, 23 Jul 2014 20:59:05 -0400 Received: from mail-pa0-f42.google.com ([209.85.220.42]:33113 "EHLO mail-pa0-f42.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751954AbaGXA7D (ORCPT ); Wed, 23 Jul 2014 20:59:03 -0400 Message-ID: <53D05A4F.5080905@gmail.com> Date: Thu, 24 Jul 2014 06:28:55 +0530 From: Varka Bhadram User-Agent: Mozilla/5.0 (X11; Linux i686; rv:24.0) Gecko/20100101 Thunderbird/24.6.0 MIME-Version: 1.0 To: Grygorii Strashko , Thomas Gleixner , santosh.shilimkar@ti.com, Jason Cooper CC: Rob Herring , Kumar Gala , ivan.khoronzhuk@ti.com, m-karicheri2@ti.com, devicetree@vger.kernel.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH v2] irqchip: add keystone irq controller ip driver References: <1406126430-9978-1-git-send-email-grygorii.strashko@ti.com> <53CFD581.5040908@gmail.com> <53CFF866.6060703@ti.com> In-Reply-To: <53CFF866.6060703@ti.com> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Wednesday 23 July 2014 11:31 PM, Grygorii Strashko wrote: > Hi, > > On 07/23/2014 06:32 PM, Varka Bhadram wrote: >> On Wednesday 23 July 2014 08:10 PM, Grygorii Strashko wrote: >>> On Keystone SOCs, DSP cores can send interrupts to ARM >>> host using the IRQ controller IP. It provides 28 IRQ >>> signals to ARM. The IRQ handler running on HOST OS can >>> identify DSP signal source by analyzing SRCCx bits in >>> IPCARx registers. This is one of the component used by >>> the IPC mechanism used on Keystone SOCs. >> (...) >> >>> +Required Properties: >>> +- compatible: should be "ti,keystone-irq" >>> +- ti,syscon-dev : phandle and offset pair. The phandle to syscon used to >>> + access device control registers and the offset inside >>> + device control registers range. >>> +- interrupt-controller : Identifies the node as an interrupt controller >>> +- #interrupt-cells : Specifies the number of cells needed to encode >>> interrupt >>> + source should be 1. >>> +- interrupts: interrupt reference to primary interrupt controller >> proper indentation for the properties >> >> - compatible : Should be "ti,keystone-irq" >> - ti,syscon-dev : phandle and offset pair. The phandle to syscon >> used to >> access device control registers and the offset inside >> device control registers range. >> >>> + >>> +Please refer to interrupts.txt in this directory for details of the >>> common >>> +Interrupt Controllers bindings used by client devices. >>> + >> (...) >> >>> +#include >>> +#include >>> +#include >>> +#include >>> +#include >>> +#include >>> +#include >>> +#include >>> +#include >>> +#include >> Includes in alphabetical order... >> >> Give one line gap before local includes.. >> >> ... >> #include >> >> #include "irqchip.h" >> >>> +#include "irqchip.h" >>> + >>> + >>> +/* The source ID bits start from 4 to 31 (total 28 bits)*/ >>> +#define BIT_OFS 4 >>> +#define KEYSTONE_N_IRQ (32 - BIT_OFS) >>> + >>> +struct keystone_irq_device { >>> + struct device *dev; >>> + struct irq_chip chip; >>> + u32 mask; >>> + u32 irq; >>> + struct irq_domain *irqd; >>> + struct regmap *devctrl_regs; >>> + u32 devctrl_offset; >>> +}; >>> + >>> +static inline u32 keystone_irq_readl(struct keystone_irq_device *kirq) >>> +{ >>> + int ret; >>> + u32 val = 0; >>> + >>> + ret = regmap_read(kirq->devctrl_regs, kirq->devctrl_offset, &val); >>> + if (ret < 0) >>> + dev_dbg(kirq->dev, "irq read failed ret(%d)\n", ret); >>> + return val; >>> +} >>> + >>> +static inline void >>> +keystone_irq_writel(struct keystone_irq_device *kirq, u32 value) >>> +{ >>> + int ret; >>> + >>> + ret = regmap_write(kirq->devctrl_regs, kirq->devctrl_offset, value); >>> + if (ret < 0) >>> + dev_dbg(kirq->dev, "irq write failed ret(%d)\n", ret); >> It can be like >> >> if (!regmap_write(kirq->devctrl_regs, kirq->devctrl_offset, value)) >> dev_dbg(kirq->dev, "irq write failed \n"); >> >>> +} >>> + >>> + > Pls, Pay attention that I'd like to see ret code here in case of failure. What we have to do with ret code... ? In case of failure only this debug message will be printed. >> (...) >> >>> +} >>> + >>> +static int keystone_irq_map(struct irq_domain *h, unsigned int virq, >>> + irq_hw_number_t hw) >> should match open parenthesis: >> >> static int keystone_irq_map(struct irq_domain *h, unsigned int virq, >> irq_hw_number_t hw) >> >>> +{ >>> + struct keystone_irq_device *kirq = h->host_data; >>> + >>> + irq_set_chip_data(virq, kirq); >>> + irq_set_chip_and_handler(virq, &kirq->chip, handle_level_irq); >>> + set_irq_flags(virq, IRQF_VALID | IRQF_PROBE); >>> + return 0; >>> +} >>> + >>> +static struct irq_domain_ops keystone_irq_ops = { >>> + .map = keystone_irq_map, >>> + .xlate = irq_domain_xlate_onecell, >>> +}; >>> + >>> +static int keystone_irq_probe(struct platform_device *pdev) >>> +{ >>> + struct device *dev = &pdev->dev; >>> + struct device_node *np = dev->of_node; >>> + struct keystone_irq_device *kirq; >>> + int ret; >>> + >>> + if (np == NULL) >>> + return -EINVAL; >> return -ENODEV?????? > If probe is executed - the dev is present, but it was created in a wrong/unsupported way > or dev structure contains wrong data. Here we are trying to get the device tree node , but that is not present we may return the error code saying that NO DEVICE is present.... >> (...) >> >>> +static struct platform_driver keystone_irq_device_driver = { >>> + .probe = keystone_irq_probe, >>> + .remove = keystone_irq_remove, >>> + .driver = { >>> + .name = "keystone_irq", >>> + .owner = THIS_MODULE, >> No need to update it. Its done by module_platform_driver().. >> >>> + .of_match_table = of_match_ptr(keystone_irq_dt_ids), >> This driver is always populate through the dts file. So no need to use >> of_match_ptr.... >> -- -Varka Bhadram