From mboxrd@z Thu Jan 1 00:00:00 1970 From: Michael Turquette Subject: Re: [PATCH v2 6/7] spi: s3c64xx: restore removed comments Date: Mon, 11 Jul 2016 15:17:31 -0700 Message-ID: <146827545163.73491.5083923656346798965@resonance> References: <1467989201-14661-1-git-send-email-andi.shyti@samsung.com> <1467989201-14661-7-git-send-email-andi.shyti@samsung.com> <146799464881.73491.7507792367941552864@resonance> <57837606.8050906@samsung.com> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 8BIT Return-path: In-Reply-To: <57837606.8050906-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org> Sender: devicetree-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org To: Sylwester Nawrocki , Andi Shyti Cc: Rob Herring , Mark Rutland , Kukjin Kim , Krzysztof Kozlowski , Mark Brown , Tomasz Figa , Stephen Boyd , Chanwoo Choi , Jaehoon Chung , linux-clk-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org, linux-samsung-soc-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, linux-spi-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, Andi Shyti List-Id: devicetree@vger.kernel.org Quoting Sylwester Nawrocki (2016-07-11 03:33:42) > On 07/08/2016 06:17 PM, Michael Turquette wrote: > > Quoting Andi Shyti (2016-07-08 07:46:40) > > >> diff --git a/drivers/spi/spi-s3c64xx.c b/drivers/spi/spi-s3c64xx.c > >> index 6da663f..32b66f0 100644 > >> --- a/drivers/spi/spi-s3c64xx.c > >> +++ b/drivers/spi/spi-s3c64xx.c > >> @@ -639,6 +639,7 @@ static void s3c64xx_spi_config(struct s3c64xx_spi_driver_data *sdd) > >> writel(val, regs + S3C64XX_SPI_MODE_CFG); > >> > >> if (sdd->port_conf->clk_from_cmu) { > >> + /* There is half-multiplier before the SPI */ > >> clk_set_rate(sdd->src_clk, sdd->cur_speed * 2); > > > > Just a small comment, but if the fixed-factor divide-by-two clock was > > modeled in Linux, then this driver could call clk_set_rate on that clock > > with the "correct" rate. > > > > I guess that this driver would be the provider of that clock? > > Good point, however I'm not sure if it is worth to model this divider > with a clk object. It is an internal divider and the clock it provides > is only used internally within the SPI controller. Thus the spi-s3c64xx > driver would be provider on the only consumer of this clock. Sure, but it's only a few lines of code to this, and there are examples in the kernel already. > > Additionally, the "There is half-multiplier before the SPI" comment > seems to be obfuscating how the hardware really looks like to me. > It talks about multiplier (which reminds me of PLLs with a divider > in the feedback loop) while there is a simple divider which should > be considered as an integral part of the controller IP block. > > While we are at it, I'd propose to change this comment to something > like: > > /* The SCLK_SPI clock is divided internally by 2 */ It's your choice, but debug output would benefit from showing the real clock frequency at some point. Regards, Mike -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html