linux-kernel - Re: [PATCH v7 3/3] clk: stm32h7: Add stm32h743 clock driver

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]
Date:   Thu, 20 Jul 2017 14:51:06 +0300
From:   Vladimir Zapolskiy <vz@...ia.com>
To:     Gabriel FERNANDEZ <gabriel.fernandez@...com>,
        Rob Herring <robh+dt@...nel.org>,
        Stephen Boyd <sboyd@...eaurora.org>
Cc:     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" <daniel.thompson@...aro.org>,
        "andrea.merello@...il.com" <andrea.merello@...il.com>,
        "radoslaw.pietrzyk@...il.com" <radoslaw.pietrzyk@...il.com>,
        Lee Jones <lee.jones@...aro.org>,
        Sylvain Lemieux <slemieux.tyco@...il.com>,
        "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
        "linux-arm-kernel@...ts.infradead.org" 
        <linux-arm-kernel@...ts.infradead.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linux-clk@...r.kernel.org" <linux-clk@...r.kernel.org>,
        Ludovic BARRE <ludovic.barre@...com>,
        Olivier BIDEAU <olivier.bideau@...com>,
        Amelie DELAUNAY <amelie.delaunay@...com>,
        "gabriel.fernandez.st@...il.com" <gabriel.fernandez.st@...il.com>,
        Arvind Yadav <arvind.yadav.cs@...il.com>
Subject: Re: [PATCH v7 3/3] clk: stm32h7: Add stm32h743 clock driver

Hi Gabriel,

On 07/20/2017 11:31 AM, Gabriel FERNANDEZ wrote:
> Hi Vladimir,
> 
> 
> On 07/19/2017 11:20 PM, Vladimir Zapolskiy wrote:
>> Hello Gabriel,
>>
>> On 07/19/2017 05:25 PM, gabriel.fernandez@...com wrote:
>>> From: Gabriel Fernandez <gabriel.fernandez@...com>
>>>
>>> This patch enables clocks for STM32H743 boards.
>>>
>>> Signed-off-by: Gabriel Fernandez <gabriel.fernandez@...com>
>>>
>>> for MFD changes:
>>> Acked-by: Lee Jones <lee.jones@...aro.org>
>>>
>>> for DT-Bindings
>>> Acked-by: Rob Herring <robh@...nel.org>
>>> ---
>>>   .../devicetree/bindings/clock/st,stm32h7-rcc.txt   |   82 ++
>> I'll provide some review comments about device tree bindings only.
>>
>>>   drivers/clk/Makefile                               |    1 +
>>>   drivers/clk/clk-stm32h7.c                          | 1409 ++++++++++++++++++++
>>>   include/dt-bindings/clock/stm32h7-clks.h           |  165 +++
>>>   include/dt-bindings/mfd/stm32h7-rcc.h              |  136 ++
>>>   5 files changed, 1793 insertions(+)
>>>   create mode 100644 Documentation/devicetree/bindings/clock/st,stm32h7-rcc.txt
>>>   create mode 100644 drivers/clk/clk-stm32h7.c
>>>   create mode 100644 include/dt-bindings/clock/stm32h7-clks.h
>>>   create mode 100644 include/dt-bindings/mfd/stm32h7-rcc.h
>>>
>>> diff --git a/Documentation/devicetree/bindings/clock/st,stm32h7-rcc.txt b/Documentation/devicetree/bindings/clock/st,stm32h7-rcc.txt
>>> new file mode 100644
>>> index 0000000..442c50c
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/clock/st,stm32h7-rcc.txt
>>> @@ -0,0 +1,82 @@
>>> +STMicroelectronics STM32H7 Reset and Clock Controller
>>> +=====================================================
>>> +
>>> +The RCC IP is both a reset and a clock controller.
>>> +
>>> +Please refer to clock-bindings.txt for common clock controller binding usage.
>>> +Please also refer to reset.txt for common reset controller binding usage.
>>> +
>>> +Required properties:
>>> +- compatible: Should be:
>>> +  "st,stm32h743-rcc"
>>> +
>>> +- reg: should be register base and length as documented in the
>>> +  datasheet
>>> +
>>> +- #reset-cells: 1, see below
>>> +
>>> +- #clock-cells : from common clock binding; shall be set to 1
>>> +
>>> +- clocks: External oscillator clock phandle
>>> +  - high speed external clock signal (HSE)
>>> +  - low speed external clock signal (LSE)
>>> +  - external I2S clock (I2S_CKIN)
>>> +
>>> +Optional properties:
>>> +- st,syscfg: phandle for pwrcfg, mandatory to disable/enable backup domain
>>> +  write protection (RTC clock).
>>> +
>>> +Example:
>>> +
>>> +	rcc: rcc@...24400 {
>> 'rcc' as a generic device node name is awkward.
>>
>> I believe the main function of the device is clock controller (unlikely
>> a generic reset controller can be converted into a clock controller),
>> the locations of the document and device driver also indicate that
>> primarily it is a clock controller, so I suggest to replace device node
>> name with 'clock-controller' like below:
> I prefer to keep rcc node name, to be coherent with the other ST 
> platforms (STM32F4/F7)

the thing is, a device node name is expected to comply with ePAPR or
the devicetree specification, which says

	The name of a node should be somewhat generic, reflecting
	the function of the device and not its precise programming model.

If devicetree and CCF maintainers are fine with 'rcc', I won't object,
my role is just to emphasize the found issue and recommend to use another
and more common name 'clock-controller', it is a simple and fortunately
backward compatible change to other ST platforms as well.

>> 	rcc: clock-controller@...24400 {
>>
>>> +		#reset-cells = <1>;
>>> +		#clock-cells = <2>
>> Missing trailing semicolon       ^^^
> ok

--
With best wishes,
Vladimir