From mboxrd@z Thu Jan 1 00:00:00 1970 From: Grygorii Strashko Date: Fri, 18 Sep 2020 16:13:33 +0300 Subject: [PATCH v2 00/30] Add DM support for omap PWM backlight In-Reply-To: <427243500.1438684.1600370614141@mail1.libero.it> References: <20200906120911.750-1-dariobin@libero.it> <427243500.1438684.1600370614141@mail1.libero.it> Message-ID: <46c659ca-9000-409c-a980-5c1957eb8829@ti.com> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de On 17/09/2020 22:23, Dario Binacchi wrote: > Hi Grygorii, > >> Il 17/09/2020 08:57 Grygorii Strashko ha scritto: >> >> >> Hi Dario, >> >> On 06/09/2020 15:08, Dario Binacchi wrote: >>> >>> The series was born from the need to manage the PWM backlight of the >>> display connected to my beaglebone board. To hit the target, I had to >>> develop drivers for PWM management which in turn relied on drivers for >>> managing timers and clocks, all developed according to the driver model. >>> My intention was to use the SoC-specific API only at strictly necessary >>> points in the code. My previous patches for migrating the AM335x display >>> driver to the driver model had required the implementation of additional >>> functions outside the concerns of the driver, (settings for dividing the >>> pixel clock rate, configuring the display DPLL rate, ....) not being >>> able to use the API of the related clock drivers. This series shouldn't >>> have repeated the same kind of mistake. Furthermore, I also wanted to fix >>> that kind of forced choice. Almost everything should have been accessible >>> via the driver model API. In the series there are also some patches that >>> could be submitted separately, but which I have however inserted to avoid >>> applying future patches to incorporate them. >>> With this last consideration, I hope I have convincingly justified the >>> large number of patches in the series. >>> >>> The patch enabling address translation into a CPU physical address from >>> device-tree even in case of crossing levels with #size-cells = <0>, is >>> crucial for the series. The previous implementation was unable to >>> perform the address translation required by the am33xx device tree. >>> I tried to apply in a conservative way as few changes as possible and >>> to verify the execution of all the tests already developed, as well as >>> the new ones I added for the new feature. >> >> Thank you for you patches. >> >> In my opinion it's better if you split this series as it is >> - too big >> - modifies different subsystems >> - contains as fixes as new features > > I agree with you. > I've been thinking about it for some time too. > Hope in the weekend. Anyway, next patches upload > will split this series. >> >> I'd recommend to separate >> - fixes first, like >> clk: remove a redundant header >> arch: sandbox: fix typo in clk.h >> fdt: translate address if #size-cells = <0> >> omap: timer: fix the rate setting >> dm: core: add a function to decode display timings >> .. >> - clk patches >> - pwm/backlight patches >> - video: omap: panel patches >> >> And I'd recommend not to port device tree bindings in u-boot as it's just duplication of >> kernel binding which u-boot shell follow. >> Just providing links to Kernel binding in commit messages should be enough. > > I have already added device-tree bindings in patches that have been accepted. No one has ever > pointed out what you recommend to me. Also, the doc/device-tree-bindings directory seems very > crowded. I have read that device-tree bindings are often evolving and I think that not copying > them in uboot does not favor their consultation. Also I wonder if it is enough to report in the > commit message the kernel file path or to refer to a particular file version by specifying the > commit sha1. Can you help me figure out what to do? > It depends, if you porting code to u-boot it's most probably that kernel bindings evolved already (>1 commit). In such case link on file may be preferable, i think. if it's new driver which just has been accepted to the Kernel with bindings - it could be sha1. Note. Common practice for u-boot is to accept new drivers only after their binding accepted in Linux Kernel. -- Best regards, grygorii