| 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: <53D05A4F.5080905@gmail.com>
Date: Thu, 24 Jul 2014 06:28:55 +0530
From: Varka Bhadram <varkabhadram@...il.com>
To: Grygorii Strashko <grygorii.strashko@...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
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
--
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