| 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
| ||
|
Date: Wed, 23 Jul 2014 21:01:10 +0300
From: Grygorii Strashko <grygorii.strashko@...com>
To: Varka Bhadram <varkabhadram@...il.com>,
Thomas Gleixner <tglx@...utronix.de>,
<santosh.shilimkar@...com>, Jason Cooper <jason@...edaemon.net>
CC: Rob Herring <robh+dt@...nel.org>,
Kumar Gala <galak@...eaurora.org>, <ivan.khoronzhuk@...com>,
<m-karicheri2@...com>, <devicetree@...r.kernel.org>,
<linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2] irqchip: add keystone irq controller ip driver
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.
>
> (...)
>
>> +}
>> +
>> +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.
>
> (...)
>
>> +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....
>
Regards,
-grygorii
--
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