Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751598AbdFHWa4 (ORCPT ); Thu, 8 Jun 2017 18:30:56 -0400 Received: from mail-io0-f194.google.com ([209.85.223.194]:33754 "EHLO mail-io0-f194.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751428AbdFHWax (ORCPT ); Thu, 8 Jun 2017 18:30:53 -0400 MIME-Version: 1.0 X-Originating-IP: [90.68.25.6] In-Reply-To: References: <20170607103242.16008-1-enric.balletbo@collabora.com> <20170607103242.16008-4-enric.balletbo@collabora.com> <6359abe2-9a21-78a2-f5ea-87e2e3335af3@ti.com> <578f348c-509f-79d3-9770-73c9fcffe19c@ti.com> From: Javier Martinez Canillas Date: Fri, 9 Jun 2017 00:30:51 +0200 Message-ID: Subject: Re: [PATCH 4/4] mfd: tps65217: Instantiate sub-devices from device tree To: Enric Balletbo Serra Cc: Grygorii Strashko , Mark Brown , Dmitry Torokhov , Lee Jones , Rob Herring , Tony Lindgren , Enric Balletbo i Serra , "devicetree@vger.kernel.org" , "linux-arm-kernel@lists.infradead.org" , "linux-omap@vger.kernel.org" , linux-kernel , linux-leds@vger.kernel.org, linux-input@vger.kernel.org, Daniel Thompson , Jingoo Han , Richard Purdie , Jacek Anaszewski , Pavel Machek , Mark Rutland , Russell King Content-Type: text/plain; charset="UTF-8" Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 3311 Lines: 97 On Thu, Jun 8, 2017 at 11:35 PM, Enric Balletbo Serra wrote: > 2017-06-08 19:11 GMT+02:00 Grygorii Strashko : [snip] >>> >>> &tps { >>> charger { >>> interrupts = <0>, <1>; >>> interrupt-names = "USB", "AC"; >>> status = "okay"; >>> }; >>> >>> pwrbutton { >>> interrupts = <2>; >>> status = "okay"; >>> }; >>> }; >> >> Sry, but this make no sense - those IRQ configuration is static, >> so it should be defined in arch/arm/boot/dts/tps65217.dtsi at least. Agreed. > > I was describing the state-of-art not what should be. If you mean that > what doesn't make sense have these interrupts portions in the DT and > the resources in the driver I'm completely agree. So we have two > options: > > 1) Get rid of the irq resources from tps65217 MFD driver and > configure all with the DT (these patches) > 2) Get rid of the DT portions as doesn't make sense have them in two places. > > If we select 2) at least we have the problem that currently all > sub-devices are instantiated and there is no way to disable a > sub-device (the problem I'm trying to solve) hence I proposed 1) > There's also an option (3) AFAICT. To do (1) but instead of hardcoding in the DTS[i], to do it in the charger driver since it seems the sub-devices are only used for this particular MFD device, and so the IRQ numbers are always going to be the same (it's already hardcoded in the MFD driver anyways). So you can just move TPS65217_IRQ_{AC,USB} to the driver. Not sure what's general opinion of having drivers calling irq_create_mapping() to get the virtual IRQ's though... [snip] >> >> I can't find links on corresponding discussions, but mfd_add_devices() is >> preferred for MDF devices. Below is one commit i've found. Also you can compare number of >> drivers using mfd_add_devices() and of_platform_populate(). I'm not sure if we should use that as an argument, it may well be that's just the drivers being half converted to DT (which is pretty common TBH). >> > > I don't think this a valid argument, I can also provide you a commit > that does exactly the contrary, replaces mfd_add_devices for > of_platform_populate (commit bb03ffb96c72) > >> [snip] > >> --- >> commit 4531156db726d27e593d35800d43c74be4e393b9 >> Author: Keerthy >> Date: Mon Sep 19 13:09:05 2016 +0530 >> >> mfd: tps65218: Use mfd_add_devices instead of of_platform_populate >> >> mfd_add_devices enables parsing device tree nodes without compatibles >> for regulators and gpio modules. Replace of_platform_populate with >> mfd_add_devices. mfd_cell currently is populated with regulators, >> gpio and powerbutton. >> For tps65218 couldn't instead of using mfd_add_devices() for all the sub-devs, had used of_platform_populate() for the ones that have device nodes and mfd_add_devices() only for the "tps65218-regulator"? The commit talks about nodes without compatibles but's actually about sub-devices without an associated device node. For me it makes sense to use of_platform_populate() when the MFD has device nodes for their sub-devices and mfd_add_devices() when DT knows nothing about the sub-devices. Best regards, Javier