linux-kernel - Re: [PATCH v2 4/6] clk: stm32f4: Add RTC clock

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <d805fde7-c8a8-15e0-b13d-b54b5c7cca6a@st.com>
Date:   Thu, 20 Oct 2016 09:50:34 +0200
From:   Gabriel Fernandez <gabriel.fernandez@...com>
To:     Stephen Boyd <sboyd@...eaurora.org>
CC:     Rob Herring <robh+dt@...nel.org>,
        Mark Rutland <mark.rutland@....com>,
        Russell King <linux@...linux.org.uk>,
        Maxime Coquelin <mcoquelin.stm32@...il.com>,
        Alexandre Torgue <alexandre.torgue@...com>,
        Michael Turquette <mturquette@...libre.com>,
        Nicolas Pitre <nico@...aro.org>, Arnd Bergmann <arnd@...db.de>,
        <daniel.thompson@...aro.org>, <devicetree@...r.kernel.org>,
        <linux-arm-kernel@...ts.infradead.org>,
        <linux-kernel@...r.kernel.org>, <linux-clk@...r.kernel.org>,
        <ludovic.barre@...com>, <olivier.bideau@...com>,
        <amelie.delaunay@...com>
Subject: Re: [PATCH v2 4/6] clk: stm32f4: Add RTC clock

Hi Stephen,

Thanks for reviewing.

Ok for all yours remarks


On 10/19/2016 10:45 PM, Stephen Boyd wrote:
> On 10/14, gabriel.fernandez@...com wrote:
>> @@ -310,6 +310,15 @@ static inline void enable_power_domain_write_protection(void)
>>   	regmap_update_bits(pdrm, 0x00, (1 << 8), (0 << 8));
>>   }
>>   
>> +static inline void sofware_reset_backup_domain(void)
>> +{
>> +	unsigned long val;
>> +
>> +	val = readl(base + STM32F4_RCC_BDCR);
>> +	writel(val |= (1 << 16), base + STM32F4_RCC_BDCR);
> Interesting C style here! Why set the bit in val that will then
> be cleared in the next function call? Please just don't do it. It
> would be better to do writel(val | BIT(16), ...)
>
>> +	writel(val & ~(1 << 16), base + STM32F4_RCC_BDCR);
>> +}
>> +
>>   struct stm32_rgate {
>>   	struct	clk_hw hw;
>>   	struct	clk_gate gate;
>> @@ -396,6 +405,113 @@ static struct clk_hw *clk_register_rgate(struct device *dev, const char *name,
>>   	return hw;
>>   }
>>   
>> +static int cclk_gate_enable(struct clk_hw *hw)
>> +{
>> +	int ret;
>> +
>> +	disable_power_domain_write_protection();
>> +
>> +	ret = clk_gate_ops.enable(hw);
>> +
>> +	enable_power_domain_write_protection();
>> +
>> +	return ret;
>> +}
>> +
>> +static void cclk_gate_disable(struct clk_hw *hw)
>> +{
>> +	disable_power_domain_write_protection();
>> +
>> +	clk_gate_ops.disable(hw);
>> +
>> +	enable_power_domain_write_protection();
>> +}
>> +
>> +static int cclk_gate_is_enabled(struct clk_hw *hw)
>> +{
>> +	return clk_gate_ops.is_enabled(hw);
>> +}
>> +
>> +static const struct clk_ops cclk_gate_ops = {
>> +	.enable		= cclk_gate_enable,
>> +	.disable	= cclk_gate_disable,
>> +	.is_enabled	= cclk_gate_is_enabled,
>> +};
>> +
>> +static u8 cclk_mux_get_parent(struct clk_hw *hw)
>> +{
>> +	return clk_mux_ops.get_parent(hw);
>> +}
>> +
>> +
> Weird double newline here. Please remove one.
>
>> +static int cclk_mux_set_parent(struct clk_hw *hw, u8 index)
>> +{
>> +	int ret;
>> +
>> +	disable_power_domain_write_protection();
>> +
>> +	sofware_reset_backup_domain();
>> +
>> +	ret = clk_mux_ops.set_parent(hw, index);
>> +
>> +	enable_power_domain_write_protection();
>> +
>> +	return ret;
>> +}
>> +
>> +
> Same.
>
>> +static const struct clk_ops cclk_mux_ops = {
>> +	.get_parent = cclk_mux_get_parent,
>> +	.set_parent = cclk_mux_set_parent,
>> +};
>> +
>> +static struct clk_hw *stm32_register_cclk(struct device *dev, const char *name,
>> +		const char * const *parent_names, int num_parents,
>> +		void __iomem *reg, u8 bit_idx, u8 shift, unsigned long flags,
>> +		spinlock_t *lock)
>> +{
>> +	struct clk_hw *hw;
>> +	struct clk_gate *gate;
>> +	struct clk_mux *mux;
>> +
>> +	gate = kzalloc(sizeof(struct clk_gate), GFP_KERNEL);
> sizeof(*gate) please.
>
>> +	if (!gate) {
>> +		hw = ERR_PTR(-EINVAL);
>> +		goto fail;
>> +	}
>> +
>> +	mux = kzalloc(sizeof(struct clk_mux), GFP_KERNEL);
> sizeof(*mux) please.
>
>> +	if (!mux) {
>> +		kfree(gate);
>> +		hw = ERR_PTR(-EINVAL);
>> +		goto fail;
>> +	}
>> +