From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753624AbbFVQwJ (ORCPT ); Mon, 22 Jun 2015 12:52:09 -0400 Received: from eusmtp01.atmel.com ([212.144.249.243]:8024 "EHLO eusmtp01.atmel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753865AbbFVQvv (ORCPT ); Mon, 22 Jun 2015 12:51:51 -0400 Message-ID: <55883CE9.2060904@atmel.com> Date: Mon, 22 Jun 2015 18:50:49 +0200 From: Nicolas Ferre Organization: atmel User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.7.0 MIME-Version: 1.0 To: Boris Brezillon CC: , Alexandre Belloni , Ludovic Desroches , Josh Wu , , Subject: Re: [PATCH] clk: at91: add generated clock driver References: <1434547409-12232-1-git-send-email-nicolas.ferre@atmel.com> <20150618172534.57b09c0b@bbrezillon> In-Reply-To: <20150618172534.57b09c0b@bbrezillon> Content-Type: text/plain; charset="windows-1252" Content-Transfer-Encoding: 8bit X-Originating-IP: [10.161.30.18] Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Le 18/06/2015 17:25, Boris Brezillon a écrit : > Hi Nicolas, Thanks for your review Boris. I'll address your remarks by sending another revision of this series in several days... Bye, > I haven't run checkpatch on your patch, but there seems to be a few > things to fix ;-). > > On Wed, 17 Jun 2015 15:23:29 +0200 > Nicolas Ferre wrote: > > > > >> + >> +static long clk_generated_determine_rate(struct clk_hw *hw, >> + unsigned long rate, >> + unsigned long *best_parent_rate, >> + struct clk_hw **best_parent_hw) > > The ->determine_rate() prototype has changed (see [1]). > > >> +/* No modification of hardware as we have the flag CLK_SET_RATE_GATE set */ >> +static int clk_generated_set_rate(struct clk_hw *hw, >> + unsigned long rate, >> + unsigned long parent_rate) >> +{ >> + struct clk_generated *gck = to_clk_generated(hw); >> + u32 div; >> + >> + if (!rate) >> + return -EINVAL; >> + >> + if (gck->id < PERIPHERAL_ID_MIN || !gck->range.max) { >> + if (parent_rate == rate) { >> + gck->gckdiv = 0; >> + return 0; >> + } else { >> + return -EINVAL; >> + } >> + } > > Do you really need the above check ? > AFAICT, periph ids inferior to 2 are invalid ids, so they should be > detected at probe time. > The !gck->range.max will be caught by the rate > gck->range.max test. > And finally, the case parent_rate == rate will just give you a div of 1, > which in turns give a gckdiv of 0. > >> + >> + if (rate > gck->range.max) >> + return -EINVAL; >> + >> + div = DIV_ROUND_CLOSEST(parent_rate, rate); >> + if (div > GENERATED_MAX_DIV + 1 || !div) >> + return -EINVAL; >> + >> + gck->gckdiv = div - 1; >> + return 0; >> +} >> + >> +static const struct clk_ops generated_ops = { >> + .enable = clk_generated_enable, >> + .disable = clk_generated_disable, >> + .is_enabled = clk_generated_is_enabled, >> + .recalc_rate = clk_generated_recalc_rate, >> + .determine_rate = clk_generated_determine_rate, >> + .get_parent = clk_generated_get_parent, >> + .set_parent = clk_generated_set_parent, >> + .set_rate = clk_generated_set_rate, >> +}; >> + >> +/** >> + * clk_generated_startup - Initialize a given clock to its default parent and >> + * divisor parameter. >> + * >> + * @gck: Generated clock to set the startup parameters for. >> + * >> + * Take parameters from the hardware and update local clock configuration >> + * accordingly. >> + */ >> +static void clk_generated_startup(struct clk_generated *gck) >> +{ >> + struct at91_pmc *pmc = gck->pmc; >> + u32 tmp; >> + >> + pmc_lock(pmc); >> + pmc_write(pmc, AT91_PMC_PCR, (gck->id & AT91_PMC_PCR_PID_MASK)); >> + tmp = pmc_read(pmc, AT91_PMC_PCR); >> + pmc_unlock(pmc); >> + >> + gck->parent_id = (tmp & AT91_PMC_PCR_GCKCSS_MASK) >> AT91_PMC_PCR_GCKCSS_OFFSET; >> + gck->gckdiv = (tmp & AT91_PMC_PCR_GCKDIV_MASK) >> AT91_PMC_PCR_GCKDIV_OFFSET; >> +} >> + >> +static struct clk * __init >> +at91_clk_register_generated(struct at91_pmc *pmc, const char *name, >> + const char **parent_names, u8 num_parents, >> + u8 id, const struct clk_range *range) >> +{ >> + struct clk_generated *gck; >> + struct clk *clk = NULL; >> + struct clk_init_data init; >> + >> + gck = kzalloc(sizeof(*gck), GFP_KERNEL); >> + if (!gck) >> + return ERR_PTR(-ENOMEM); >> + >> + init.name = name; >> + init.ops = &generated_ops; >> + init.parent_names = parent_names; >> + init.num_parents = num_parents; >> + init.flags = CLK_SET_RATE_GATE | CLK_SET_PARENT_GATE; >> + >> + gck->id = id; >> + gck->hw.init = &init; >> + gck->pmc = pmc; >> + gck->range = *range; >> + >> + clk = clk_register(NULL, &gck->hw); >> + if (IS_ERR(clk)) >> + kfree(gck); >> + else >> + clk_generated_startup(gck); >> + >> + return clk; >> +} >> + >> +void __init of_sama5d2_clk_generated_setup(struct device_node *np, >> + struct at91_pmc *pmc) >> +{ >> + int i; >> + int num; >> + u32 id; >> + const char *name; >> + struct clk *clk; >> + int num_parents; >> + const char *parent_names[GENERATED_SOURCE_MAX]; >> + struct device_node *gcknp; >> + struct clk_range range = CLK_RANGE(0, 0); >> + >> + num_parents = of_count_phandle_with_args(np, "clocks", "#clock-cells"); > > Use the of_clk_get_parent_count() [2]. > > That's all I got for now. > > Thanks, > > Boris > > > [1]http://lxr.free-electrons.com/source/include/linux/clk-provider.h#L178 > [2]http://lxr.free-electrons.com/source/include/linux/clk-provider.h#L626 > -- Nicolas Ferre -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in Please read the FAQ at http://www.tux.org/lkml/ From mboxrd@z Thu Jan 1 00:00:00 1970 From: nicolas.ferre@atmel.com (Nicolas Ferre) Date: Mon, 22 Jun 2015 18:50:49 +0200 Subject: [PATCH] clk: at91: add generated clock driver In-Reply-To: <20150618172534.57b09c0b@bbrezillon> References: <1434547409-12232-1-git-send-email-nicolas.ferre@atmel.com> <20150618172534.57b09c0b@bbrezillon> Message-ID: <55883CE9.2060904@atmel.com> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org Le 18/06/2015 17:25, Boris Brezillon a ?crit : > Hi Nicolas, Thanks for your review Boris. I'll address your remarks by sending another revision of this series in several days... Bye, > I haven't run checkpatch on your patch, but there seems to be a few > things to fix ;-). > > On Wed, 17 Jun 2015 15:23:29 +0200 > Nicolas Ferre wrote: > > > > >> + >> +static long clk_generated_determine_rate(struct clk_hw *hw, >> + unsigned long rate, >> + unsigned long *best_parent_rate, >> + struct clk_hw **best_parent_hw) > > The ->determine_rate() prototype has changed (see [1]). > > >> +/* No modification of hardware as we have the flag CLK_SET_RATE_GATE set */ >> +static int clk_generated_set_rate(struct clk_hw *hw, >> + unsigned long rate, >> + unsigned long parent_rate) >> +{ >> + struct clk_generated *gck = to_clk_generated(hw); >> + u32 div; >> + >> + if (!rate) >> + return -EINVAL; >> + >> + if (gck->id < PERIPHERAL_ID_MIN || !gck->range.max) { >> + if (parent_rate == rate) { >> + gck->gckdiv = 0; >> + return 0; >> + } else { >> + return -EINVAL; >> + } >> + } > > Do you really need the above check ? > AFAICT, periph ids inferior to 2 are invalid ids, so they should be > detected at probe time. > The !gck->range.max will be caught by the rate > gck->range.max test. > And finally, the case parent_rate == rate will just give you a div of 1, > which in turns give a gckdiv of 0. > >> + >> + if (rate > gck->range.max) >> + return -EINVAL; >> + >> + div = DIV_ROUND_CLOSEST(parent_rate, rate); >> + if (div > GENERATED_MAX_DIV + 1 || !div) >> + return -EINVAL; >> + >> + gck->gckdiv = div - 1; >> + return 0; >> +} >> + >> +static const struct clk_ops generated_ops = { >> + .enable = clk_generated_enable, >> + .disable = clk_generated_disable, >> + .is_enabled = clk_generated_is_enabled, >> + .recalc_rate = clk_generated_recalc_rate, >> + .determine_rate = clk_generated_determine_rate, >> + .get_parent = clk_generated_get_parent, >> + .set_parent = clk_generated_set_parent, >> + .set_rate = clk_generated_set_rate, >> +}; >> + >> +/** >> + * clk_generated_startup - Initialize a given clock to its default parent and >> + * divisor parameter. >> + * >> + * @gck: Generated clock to set the startup parameters for. >> + * >> + * Take parameters from the hardware and update local clock configuration >> + * accordingly. >> + */ >> +static void clk_generated_startup(struct clk_generated *gck) >> +{ >> + struct at91_pmc *pmc = gck->pmc; >> + u32 tmp; >> + >> + pmc_lock(pmc); >> + pmc_write(pmc, AT91_PMC_PCR, (gck->id & AT91_PMC_PCR_PID_MASK)); >> + tmp = pmc_read(pmc, AT91_PMC_PCR); >> + pmc_unlock(pmc); >> + >> + gck->parent_id = (tmp & AT91_PMC_PCR_GCKCSS_MASK) >> AT91_PMC_PCR_GCKCSS_OFFSET; >> + gck->gckdiv = (tmp & AT91_PMC_PCR_GCKDIV_MASK) >> AT91_PMC_PCR_GCKDIV_OFFSET; >> +} >> + >> +static struct clk * __init >> +at91_clk_register_generated(struct at91_pmc *pmc, const char *name, >> + const char **parent_names, u8 num_parents, >> + u8 id, const struct clk_range *range) >> +{ >> + struct clk_generated *gck; >> + struct clk *clk = NULL; >> + struct clk_init_data init; >> + >> + gck = kzalloc(sizeof(*gck), GFP_KERNEL); >> + if (!gck) >> + return ERR_PTR(-ENOMEM); >> + >> + init.name = name; >> + init.ops = &generated_ops; >> + init.parent_names = parent_names; >> + init.num_parents = num_parents; >> + init.flags = CLK_SET_RATE_GATE | CLK_SET_PARENT_GATE; >> + >> + gck->id = id; >> + gck->hw.init = &init; >> + gck->pmc = pmc; >> + gck->range = *range; >> + >> + clk = clk_register(NULL, &gck->hw); >> + if (IS_ERR(clk)) >> + kfree(gck); >> + else >> + clk_generated_startup(gck); >> + >> + return clk; >> +} >> + >> +void __init of_sama5d2_clk_generated_setup(struct device_node *np, >> + struct at91_pmc *pmc) >> +{ >> + int i; >> + int num; >> + u32 id; >> + const char *name; >> + struct clk *clk; >> + int num_parents; >> + const char *parent_names[GENERATED_SOURCE_MAX]; >> + struct device_node *gcknp; >> + struct clk_range range = CLK_RANGE(0, 0); >> + >> + num_parents = of_count_phandle_with_args(np, "clocks", "#clock-cells"); > > Use the of_clk_get_parent_count() [2]. > > That's all I got for now. > > Thanks, > > Boris > > > [1]http://lxr.free-electrons.com/source/include/linux/clk-provider.h#L178 > [2]http://lxr.free-electrons.com/source/include/linux/clk-provider.h#L626 > -- Nicolas Ferre