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  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
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