From: Appana Durga Kedareswara Rao <appana.durga.rao@xilinx.com>
To: Soren Brinkmann <sorenb@xilinx.com>
CC: Soren Brinkmann <sorenb@xilinx.com>,
        "robh+dt@kernel.org" <robh+dt@kernel.org>,
        "pawel.moll@arm.com" <pawel.moll@arm.com>,
        "mark.rutland@arm.com" <mark.rutland@arm.com>,
        "ijc+devicetree@hellion.org.uk" <ijc+devicetree@hellion.org.uk>,
        "galak@codeaurora.org" <galak@codeaurora.org>,
        Michal Simek <michals@xilinx.com>,
        "vinod.koul@intel.com" <vinod.koul@intel.com>,
        "dan.j.williams@intel.com" <dan.j.williams@intel.com>,
        "moritz.fischer@ettus.com" <moritz.fischer@ettus.com>,
        "laurent.pinchart@ideasonboard.com" 
	<laurent.pinchart@ideasonboard.com>,
        "luis@debethencourt.com" <luis@debethencourt.com>,
        Anirudha Sarangi <anirudh@xilinx.com>,
        Punnaiah Choudary Kalluri <punnaia@xilinx.com>,
        Shubhrajyoti Datta <shubhraj@xilinx.com>,
        "devicetree@vger.kernel.org" <devicetree@vger.kernel.org>,
        "linux-arm-kernel@lists.infradead.org" 
	<linux-arm-kernel@lists.infradead.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "dmaengine@vger.kernel.org" <dmaengine@vger.kernel.org>
Subject: RE: [PATCH v3 3/3] dmaengine: vdma: Add clock support
Thread-Topic: [PATCH v3 3/3] dmaengine: vdma: Add clock support
Thread-Index: AQHRm7oBw54uslpveEWBfoSSWvBadJ+UFkSAgACHnpD//4OpgIAAiGHQ
Date: Thu, 21 Apr 2016 17:13:18 +0000
Message-ID: <C246CAC1457055469EF09E3A7AC4E11A4A579A06@XAP-PVEXMBX01.xlnx.xilinx.com>
References: <1461235118-800-1-git-send-email-appanad@xilinx.com>
 <1461235118-800-4-git-send-email-appanad@xilinx.com>
 <20160421162153.GZ7128@xsjsorenbubuntu>
 <C246CAC1457055469EF09E3A7AC4E11A4A5799D9@XAP-PVEXMBX01.xlnx.xilinx.com>
 <20160421170215.GB7128@xsjsorenbubuntu>
In-Reply-To: <20160421170215.GB7128@xsjsorenbubuntu>
Accept-Language: en-US
Content-Language: en-US
Content-Type: text/plain; charset="utf-8"
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 21 Apr 2016 17:13:22.8715
 (UTC)
X-MS-Exchange-CrossTenant-Id: 657af505-d5df-48d0-8300-c31994686c5c
X-MS-Exchange-CrossTenant-OriginalAttributedTenantConnectingIp: TenantId=657af505-d5df-48d0-8300-c31994686c5c;Ip=[149.199.60.100];Helo=[xsj-pvapsmtpgw02]
X-MS-Exchange-CrossTenant-FromEntityHeader: HybridOnPrem
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SN1NAM02HT015
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org
Content-Transfer-Encoding: 8bit
X-MIME-Autoconverted: from base64 to 8bit by mail.home.local id u3LHDXjh011298
Content-Length: 4065
Lines: 94

Hi Soren,

> -----Original Message-----
> From: Sören Brinkmann [mailto:soren.brinkmann@xilinx.com]
> Sent: Thursday, April 21, 2016 10:32 PM
> To: Appana Durga Kedareswara Rao <appanad@xilinx.com>
> Cc: Soren Brinkmann <sorenb@xilinx.com>; robh+dt@kernel.org;
> pawel.moll@arm.com; mark.rutland@arm.com; ijc+devicetree@hellion.org.uk;
> galak@codeaurora.org; Michal Simek <michals@xilinx.com>;
> vinod.koul@intel.com; dan.j.williams@intel.com; moritz.fischer@ettus.com;
> laurent.pinchart@ideasonboard.com; luis@debethencourt.com; Anirudha
> Sarangi <anirudh@xilinx.com>; Punnaiah Choudary Kalluri
> <punnaia@xilinx.com>; Shubhrajyoti Datta <shubhraj@xilinx.com>;
> devicetree@vger.kernel.org; linux-arm-kernel@lists.infradead.org; linux-
> kernel@vger.kernel.org; dmaengine@vger.kernel.org
> Subject: Re: [PATCH v3 3/3] dmaengine: vdma: Add clock support
> 
> On Thu, 2016-04-21 at 09:32:44 -0700, Appana Durga Kedareswara Rao wrote:
> > Hi Soren,
> >
> > > -----Original Message-----
> > > From: Sören Brinkmann [mailto:soren.brinkmann@xilinx.com]
> > > Sent: Thursday, April 21, 2016 9:52 PM
> > > To: Appana Durga Kedareswara Rao <appanad@xilinx.com>
> > > Cc: robh+dt@kernel.org; pawel.moll@arm.com; mark.rutland@arm.com;
> > > ijc+devicetree@hellion.org.uk; galak@codeaurora.org; Michal Simek
> > > <michals@xilinx.com>; vinod.koul@intel.com; dan.j.williams@intel.com;
> > > Appana Durga Kedareswara Rao <appanad@xilinx.com>;
> > > moritz.fischer@ettus.com; laurent.pinchart@ideasonboard.com;
> > > luis@debethencourt.com; Anirudha Sarangi <anirudh@xilinx.com>; Punnaiah
> > > Choudary Kalluri <punnaia@xilinx.com>; Shubhrajyoti Datta
> > > <shubhraj@xilinx.com>; devicetree@vger.kernel.org; linux-arm-
> > > kernel@lists.infradead.org; linux-kernel@vger.kernel.org;
> > > dmaengine@vger.kernel.org
> > > Subject: Re: [PATCH v3 3/3] dmaengine: vdma: Add clock support
> > >
> > > On Thu, 2016-04-21 at 16:08:38 +0530, Kedareswara rao Appana wrote:
> [...]
> > > > @@ -1757,6 +1767,200 @@ static void xilinx_dma_chan_remove(struct
> > > xilinx_dma_chan *chan)
> > > >  	list_del(&chan->common.device_node);
> > > >  }
> > > >
> > > > +static int axidma_clk_init(struct platform_device *pdev, struct clk
> **axi_clk,
> > > > +			    struct clk **tx_clk, struct clk **rx_clk,
> > > > +			    struct clk **sg_clk, struct clk **tmp_clk) {
> > > > +	int err;
> > > > +
> > > > +	*tmp_clk = NULL;
> > > > +
> > > > +	*axi_clk = devm_clk_get(&pdev->dev, "s_axi_lite_aclk");
> > > > +	if (IS_ERR(*axi_clk)) {
> > > > +		err = PTR_ERR(*axi_clk);
> > > > +		dev_err(&pdev->dev, "failed to get axi_aclk (%u)\n", err);
> > > > +		return err;
> > > > +	}
> > > > +
> > > > +	*tx_clk = devm_clk_get(&pdev->dev, "m_axi_mm2s_aclk");
> > > > +	if (IS_ERR(*tx_clk))
> > > > +		*tx_clk = NULL;
> > > > +
> > > > +	*rx_clk = devm_clk_get(&pdev->dev, "m_axi_s2mm_aclk");
> > > > +	if (IS_ERR(*rx_clk))
> > > > +		*rx_clk = NULL;
> > > > +
> > > > +	*sg_clk = devm_clk_get(&pdev->dev, "m_axi_sg_aclk");
> > > > +	if (IS_ERR(*sg_clk))
> > > > +		*sg_clk = NULL;
> > > > +
> > > > +
> > > > +	err = clk_prepare_enable(*axi_clk);
> > >
> > > Should this be called if you know that the pointer might be NULL?
> >
> > It is a mandatory clock and if this clk is not there in DT I am already returning
> error...
> > I didn't get your question could you please elaborate???
> 
> But for all the optional clocks. They could all be NULL and you're calling
> clk_prepare_enable with a NULL pointer. That function is nice enough to
> do a NULL check for you, but I wonder whether these calls should happen at
> all when you already know that the pointer is not a valid clock.

I referred macb driver http://lxr.free-electrons.com/source/drivers/net/ethernet/cadence/macb.c 
There tx_clk is optional and in this driver they are calling clk_prepare_enable for the optional clocks.
Please let me know if you are ok to call the clk_prepare_enable() for optional clocks with a NULL pointer.
Will fix rest of the comments and will send next version of the patch...

Regards,
Kedar.

> 
> 	Sören