Re: [PATCH v3 5/5] hisilicon/dts: Add hi655x pmic dts node

[chenfeng]

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?




--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/