From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751050AbbLUGdc (ORCPT ); Mon, 21 Dec 2015 01:33:32 -0500 Received: from szxga02-in.huawei.com ([119.145.14.65]:51203 "EHLO szxga02-in.huawei.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750774AbbLUGda (ORCPT ); Mon, 21 Dec 2015 01:33:30 -0500 Subject: Re: [PATCH v3 5/5] hisilicon/dts: Add hi655x pmic dts node To: Mark Brown References: <1450184056-79851-1-git-send-email-puck.chen@hisilicon.com> <1450184056-79851-6-git-send-email-puck.chen@hisilicon.com> <56722B9F.8090301@hisilicon.com> <20151218175813.GE5727@sirena.org.uk> <56776B91.3030004@hisilicon.com> CC: , , , , , , , , , , , , , , , , , , , , , From: chenfeng Message-ID: <56779A20.9070802@hisilicon.com> Date: Mon, 21 Dec 2015 14:20:16 +0800 User-Agent: Mozilla/5.0 (Windows NT 6.1; rv:38.0) Gecko/20100101 Thunderbird/38.2.0 MIME-Version: 1.0 In-Reply-To: <56776B91.3030004@hisilicon.com> Content-Type: text/plain; charset="windows-1252" Content-Transfer-Encoding: 7bit X-Originating-IP: [10.142.192.172] X-CFilter-Loop: Reflected X-Mirapoint-Virus-RAPID-Raw: score=unknown(0), refid=str=0001.0A020206.56779B56.00E6,ss=1,re=0.000,recu=0.000,reip=0.000,cl=1,cld=1,fgs=0, ip=0.0.0.0, so=2013-06-18 04:22:30, dmn=2013-03-21 17:37:32 X-Mirapoint-Loop-Id: 0a445765aaa7c0c1af7a8096541a2d0c Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 2015/12/21 11:01, chenfeng wrote: > Mark, > > On 2015/12/19 1:58, Mark Brown wrote: >> On Thu, Dec 17, 2015 at 11:27:27AM +0800, chenfeng wrote: >> >>> +- regulator-vset-regs: Voltage set register offset. >>> +- regulator-vset-mask: voltage set control mask. >>> +- regulator-n-vol: The num of support voltages. >>> +- regulator-vset-table: The table of support voltages. >> >>>> Why is this in the binding? This is a binding for a specific device, >>>> there is no point in putting all these data tables in the DT - it just >>>> bloats the DT and makes it harder for us to enhance our support for this >>>> device in the future. >> >>> You mentioned in previous version,I I have some questions for it. >> >>> This regulator-vset-regs etc are vendor specific describe. The hi655x PMIC >> >> There's nothing vendor specific about the way this is written... >> >>> is a series of chips. They all have this value, but the offset may be different. >>> And we can generate the dts file from excel which is defined by SOC. >> >>> I think the dts is designed to distinguish different platform. If we hard code this >>> in files, it may be also different to use as common in next chip version. >> >> If your tooling can generate DT files it can generate C code just as >> well and it seems unlikely you're going to be able to build new boards >> without being able to do firmware updates here. Especially for the >> sorts of systems that use DT the set of scenarios where you're able to >> update the DT but not the kernel seems like it will be extremely >> limited. I don't really buy the argument that there's any practical >> difference in the ability to update the kernel and DT and to the extent >> there is one it seems better to keep the ABI we have to support smaller >> by having the DT be minimal. >> >> This also allows us to map things more efficiently than we can with just >> a table of voltages. For example a good selection of the regulators in >> your example DT appear to be linear ranges and so should be mapped as >> such so we can do direct calcuations rather than having to iterate >> through a table to map voltages into selectors. That gets especially >> serious for higher resolution regulators like most DCDCs (and modern >> LDOs for that matter). >> > Thanks, > I see, I will change the table of voltages into driver. > like this, > static const unsigned int voltages[] = { > 1500000, 1800000, 2400000, 2500000, > 2600000, 2700000, 2850000, 3000000, > }; > > And there will be two open-code function for is-enable and disable in the regulator driver. > Since we need use the status and disable register on PM chip. Only enable reg in the regulator desc. > > Do you agree with this? > While doing this in driver code, I found that it seems all the vendor chip have the voltage table. So I am wondering can we add this into the regulator framework. We can add in the function of_get_regulation_constraints to get the vset table. I am not sure this is right or not. > >