Messages in this thread | | | Date | Thu, 24 Jul 2014 06:28:55 +0530 | From | Varka Bhadram <> | Subject | Re: [PATCH v2] irqchip: add keystone irq controller ip driver |
| |
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 <linux/irq.h> >>> +#include <linux/bitops.h> >>> +#include <linux/module.h> >>> +#include <linux/moduleparam.h> >>> +#include <linux/irqdomain.h> >>> +#include <linux/irqchip/chained_irq.h> >>> +#include <linux/of.h> >>> +#include <linux/of_platform.h> >>> +#include <linux/mfd/syscon.h> >>> +#include <linux/regmap.h> >> Includes in alphabetical order... >> >> Give one line gap before local includes.. >> >> ... >> #include <linux/regmap.h> >> >> #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
| |