From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S933306AbbKRQSJ (ORCPT ); Wed, 18 Nov 2015 11:18:09 -0500 Received: from szxga03-in.huawei.com ([119.145.14.66]:44871 "EHLO szxga03-in.huawei.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S933147AbbKRQSG (ORCPT ); Wed, 18 Nov 2015 11:18:06 -0500 Subject: Re: [PATCH v5 20/32] scsi: hisi_sas: add v1 hw interrupt init To: Rob Herring References: <1447779059-136143-1-git-send-email-john.garry@huawei.com> <1447779059-136143-21-git-send-email-john.garry@huawei.com> CC: "James E.J. Bottomley" , Pawel Moll , Mark Rutland , Ian Campbell , Kumar Gala , "Arnd Bergmann" , , "linux-kernel@vger.kernel.org" , "devicetree@vger.kernel.org" , Linuxarm , John Garry , , Wei Xu , Zhangfei Gao From: John Garry Message-ID: <564CA396.9010709@huawei.com> Date: Wed, 18 Nov 2015 16:13:10 +0000 User-Agent: Mozilla/5.0 (Windows NT 6.1; rv:38.0) Gecko/20100101 Thunderbird/38.3.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset="utf-8"; format=flowed Content-Transfer-Encoding: 7bit X-Originating-IP: [10.202.137.235] X-CFilter-Loop: Reflected X-Mirapoint-Virus-RAPID-Raw: score=unknown(0), refid=str=0001.0A090201.564CA3B5.0092,ss=1,re=0.000,recu=0.000,reip=0.000,cl=1,cld=1,fgs=0, ip=0.0.0.0, so=2013-05-26 15:14:31, dmn=2013-03-21 17:37:32 X-Mirapoint-Loop-Id: fda170a7d0375334e6cdbbe99fa42518 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 18/11/2015 15:26, Rob Herring wrote: > On Tue, Nov 17, 2015 at 10:50 AM, John Garry wrote: >> Add code to interrupts, so now we can get a phy up >> interrupt when a disk is connected. > So I started looking at why you are using of_irq_count which drivers > shouldn't need to. In patch 5 you use it to allocate memory to store > the irq names, then use them here... right > >> +static const char phy_int_names[HISI_SAS_PHY_INT_NR][32] = { >> + {"Phy Up"}, >> +}; >> +static irq_handler_t phy_interrupts[HISI_SAS_PHY_INT_NR] = { >> + int_phyup_v1_hw, >> +}; >> + >> +static int interrupt_init_v1_hw(struct hisi_hba *hisi_hba) >> +{ >> + struct device *dev = &hisi_hba->pdev->dev; >> + struct device_node *np = dev->of_node; >> + char *int_names = hisi_hba->int_names; >> + int i, j, irq, rc, idx; >> + >> + if (!np) >> + return -ENOENT; >> + >> + for (i = 0; i < hisi_hba->n_phy; i++) { >> + struct hisi_sas_phy *phy = &hisi_hba->phy[i]; >> + >> + idx = i * HISI_SAS_PHY_INT_NR; >> + for (j = 0; j < HISI_SAS_PHY_INT_NR; j++, idx++) { >> + irq = irq_of_parse_and_map(np, idx); > It is also preferred that drivers don't use this either. You should > use platform_get_irq() instead. > >> + if (!irq) { >> + dev_err(dev, >> + "irq init: fail map phy interrupt %d\n", >> + idx); >> + return -ENOENT; >> + } >> + >> + (void)snprintf(&int_names[idx * HISI_SAS_NAME_LEN], >> + HISI_SAS_NAME_LEN, >> + "%s %s:%d", dev_name(dev), >> + phy_int_names[j], i); >> + rc = devm_request_irq(dev, irq, phy_interrupts[j], 0, >> + &int_names[idx * HISI_SAS_NAME_LEN], >> + phy); > There's no requirement for the name to match the name in the DT or > even that the name needs to be unique. It is desirable to be unique as we have so many instances of the same types of interrupt sources: for hip05 chipset with v1 controller we have the following interrupts: 8 phyup, 8 abnormal, 8 broadcast, 32 completion, 2 fatal > If you really want the DT names used, then just call > of_property_read_string_index() on interrupt-names here. There is no > point to copy the string. We would not want to add so many strings, no? > Rob thanks, John