From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-lf1-f45.google.com (mail-lf1-f45.google.com [209.85.167.45]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id F3DFA1A840C for ; Fri, 20 Dec 2024 09:11:34 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=209.85.167.45 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1734685896; cv=none; b=POx5EAQEu7rcHK86G8afJaGwhPs+opweFsTPyGzS7eyyEnpomXtPIXEPU+rMndBYG3yS0ekZFEoisluUqdIP57bOQ0tySmLNaSqqszTIImUYq2uhAXeBvVirA8wS2rpWvrY//5n3i7ztOQi6uCa/iUOnIHfsAb1XJ+Vme6s3cpE= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1734685896; c=relaxed/simple; bh=huyKTAD5TizUaeXmtPHdaMmKMkdO1y4kWXduIO+yhSw=; h=Message-ID:Date:MIME-Version:Subject:To:Cc:References:From: In-Reply-To:Content-Type; b=ULNMmN+8Uc5YnyQqFMR1LEbx0GNHw2AuxpurfkRBwCaMF02q0PAKoX3DI33EnDIQ+npR3/4yoJOC8pcH+3ZSPOtyiyk0oqkID29hOJ/lG8zVJuMBevxgNh86BQsYRLoixjWMHogNm9uhBBcG9O5CmIkLf5Nn7UxhdIw44oZTGVk= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=none (p=none dis=none) header.from=cogentembedded.com; spf=pass smtp.mailfrom=cogentembedded.com; dkim=pass (2048-bit key) header.d=cogentembedded-com.20230601.gappssmtp.com header.i=@cogentembedded-com.20230601.gappssmtp.com header.b=kkKIr3qm; arc=none smtp.client-ip=209.85.167.45 Authentication-Results: smtp.subspace.kernel.org; dmarc=none (p=none dis=none) header.from=cogentembedded.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=cogentembedded.com Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=cogentembedded-com.20230601.gappssmtp.com header.i=@cogentembedded-com.20230601.gappssmtp.com header.b="kkKIr3qm" Received: by mail-lf1-f45.google.com with SMTP id 2adb3069b0e04-5401bd6cdb4so1770104e87.2 for ; Fri, 20 Dec 2024 01:11:34 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=cogentembedded-com.20230601.gappssmtp.com; s=20230601; t=1734685893; x=1735290693; darn=vger.kernel.org; h=content-transfer-encoding:in-reply-to:from:content-language :references:cc:to:subject:user-agent:mime-version:date:message-id :from:to:cc:subject:date:message-id:reply-to; bh=yPPsIlWtoiJzR9IRsKw7Ht2hqc4uWrmKnxf2DzBaodQ=; b=kkKIr3qmu6jI2MxvE3szsucxusogeDdblF/KG9+r4KghGU2iKNRunYFhknj5GH9XrV 4pApE7XuUIfPShPh5eGAqF4oDIg0naFVd6miDDVff/E3vkxw+WhfuCcK8wjFxt3clG0Z i4yWgr5DXLWic77706zuWxamCLhbo/TTfEBvWD3317giJEOzwJ7NjCFLz/Ucnb23QRw3 jcqStDn+5znO62H57gfb2HC4G32ibleC8gkyLWHqEwAhHZ78O8V6SIjD7I3bcNtj3Q/8 EKNccfwSGYkA5WpfRFb5QTeFz03f61KbYGXYj7Qrig8VsWSedOW0yTo6d9MpnNZVWbX4 KOWQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1734685893; x=1735290693; h=content-transfer-encoding:in-reply-to:from:content-language :references:cc:to:subject:user-agent:mime-version:date:message-id :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=yPPsIlWtoiJzR9IRsKw7Ht2hqc4uWrmKnxf2DzBaodQ=; b=Vf1Xd/0ch1yoEQaaUjnNqOz67k2GiXOFyAi5yQZopT+pPIlFg0w1jPsKVENmxUYMdK n/ZHCxvvGi834Mf+Z3KFZF9BJMylYaOLePU887pFHjk9Cnm6bFpfY/aXWg4NuLQlruuw RvmwuInFageYy4WcRusL/EWAn1fQkj2eg8l2fLCCjE6R+vU1QrVDm+opMq7yMTfWuYEj V9PYs2x0tgiD02aff37rTnN4rynVHqYuDqpsUabdQIRtExQin6uog9kbx5F3QSR5LEcp MEDmqVsqmz7DQn/OFv/Ph9RAtGC1GcL87CGujSoihjfy7Z3R5uOu6kbQIfHRl9Oomn6f 57qA== X-Forwarded-Encrypted: i=1; AJvYcCX6U4+SXU21fa28Rk8EVZIVeb6BQJfA+4NHaXIxWLNXnwyyRMg5qWK/b8qnIkroGcQ1EvVXCpUvCRU0p8o=@vger.kernel.org X-Gm-Message-State: AOJu0YymSRdfMtwDvCcUHJzp4vQftRU8Do+1/IN0t5Hrzan1qtPiIrOL ikUpTffVlztrWYEcKD4Cgto2OgWEojba7LOZroK8ej3zfEyUY8LNehRoxOEPUUBAmJow/5MYs1r 6 X-Gm-Gg: ASbGncuITZDc3MLfxFZZ5IAcylkdcLOAPUF7ufXSEUVLAyJ92nxChTsbGHRKsC95k4n 5h6OlDW+ae5gOofL/RknrRnvXZt8pobLDzPLTO4hY/8dcIW1W39BG4ADNl78h42MkpiWVtCBekK SoV0o5A951q07nfN2vLkBCpYwjy39MPCDAbHN2yyZbEG+FkNyBwMbUUYdxqHg707/oLVaaXXWdU 84jc2IKJqQHNRPVCLGno9Cvp8A7pH6G5ca6iN6I73Hq2oNuQit873r/SgKrxhmKAwPWbyatQHiI X-Google-Smtp-Source: AGHT+IEfNicnTZkEytBBHq6sbMYBDDsi515rs7GJGYu4kBt4EJniWkEN+7wvu29GWJPtgOjQQ8cCFg== X-Received: by 2002:a05:6512:1387:b0:542:213f:7901 with SMTP id 2adb3069b0e04-54229582367mr581669e87.44.1734685893124; Fri, 20 Dec 2024 01:11:33 -0800 (PST) Received: from [192.168.0.104] ([91.198.101.25]) by smtp.gmail.com with ESMTPSA id 2adb3069b0e04-542235ffe72sm431810e87.77.2024.12.20.01.11.31 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Fri, 20 Dec 2024 01:11:32 -0800 (PST) Message-ID: <0e95c4dc-e155-4860-b918-13e47bf9b9c6@cogentembedded.com> Date: Fri, 20 Dec 2024 14:11:26 +0500 Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH net-next 1/2] net: renesas: rswitch: use per-port irq handlers To: Michal Swiatkowski Cc: Yoshihiro Shimoda , Andrew Lunn , "David S. Miller" , Eric Dumazet , Jakub Kicinski , Paolo Abeni , Geert Uytterhoeven , netdev@vger.kernel.org, linux-renesas-soc@vger.kernel.org, linux-kernel@vger.kernel.org, Michael Dege , Christian Mardmoeller , Dennis Ostermann References: <20241220041659.2985492-1-nikita.yoush@cogentembedded.com> <20241220041659.2985492-2-nikita.yoush@cogentembedded.com> Content-Language: en-US, ru-RU From: Nikita Yushchenko In-Reply-To: Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit >> + ret = request_irq(rdev->irq, rswitch_gwca_data_irq, IRQF_SHARED, > It wasn't shared previously, maybe some notes in commit message about > that. It can be shared between several ports. I will try to rephrase the commit message to make this stated explicitly. >> + err = of_property_read_u32(rdev->np_port, "irq-index", &irq_index); >> + if (err == 0) { > Usually if (!err) is used. Ok, will fix it. > >> + if (irq_index < GWCA_NUM_IRQS) >> + rdev->irq_index = irq_index; >> + else >> + dev_warn(&rdev->priv->pdev->dev, >> + "%pOF: irq-index out of range\n", >> + rdev->np_port); > Why not return here? It is a little counter intuitive, maybe: > if (err) { > dev_warn(); > return -ERR; > } It is meant to be optional, not having it defined shall not be an error > if (irq_index < NUM_IRQS) { > dev_warn(); > return -ERR; > } Ok - although if erroring out, I think it shall be dev_err. >> + } >> + >> + name = kasprintf(GFP_KERNEL, GWCA_IRQ_RESOURCE_NAME, rdev->irq_index); > > In case with not returning you are using invalid rdev_irq_index here > (probably 0, so may it be fine, I am only wondering). Yes, the field is zero-initialized and that zero is a sane default. > >> + if (!name) >> + return -ENOMEM; >> + err = platform_get_irq_byname(rdev->priv->pdev, name); >> + kfree(name); >> + if (err < 0) >> + return err; >> + rdev->irq = err; > > If you will be changing sth here consider: > rdev->irq = platform() > if (rdev->irq < 0) > return rdev->irq; Ok >> + err = rswitch_port_get_irq(rdev); >> + if (err < 0) > You are returning 0 in case of success, the netdev code style is to > check it like that: if (!err) I tried to follow the style already existing in the driver. Several checks just above and below are written this way. Shall I add this one check written differently? > >> + goto out_get_irq; > If you will use the label name according to what does happen under label > you will not have to add another one. Feel free to leave it as it is, as > you have the same scheme across driver with is completle fine. You can > check Przemek's answer according "came from" convention [1]. Again, following existing style here. My personal opinion is that "came from" labels are more reliable against future changes than other label styles. But if there is maintainer requirement here then definitely I will follow. Nikita