Re: [PATCH 1/1] ARM: dts: Add PMIC support to arndale-octa

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




Hi Mark,

Sorry for the late reply as I was sick and still recovering.

On 11 December 2013 00:24, Mark Brown <broonie@xxxxxxxxxx> wrote:
> On Tue, Dec 10, 2013 at 11:59:18AM +0530, Sachin Kamat wrote:
>
>> Added PMIC node to Arndale-Octa board.
>
> Is there a bootloader posted for this anywhere yet?

One version available here from Insignal:
git://git.insignal.co.kr/insignal/arndale_octa-jb_mr1.1/u-boot


>
>> +                     regulators {
>> +                             ldo1_reg: LDO1 {
>> +                                     regulator-name = "vdd_ldo1";
>
> So, there is a label on the schematic for PVDD_LDO1 but it's not
> entirely obvious why that's there as throughout the rest of the
> schematic it's got the rather more constructive name PVDD_ALIVE_1V0.
> We should really be using the more readable versions of the names
> in the DT since the whole point in overriding the driver default
> names is to make it easier to associate the kernel diagnostics with
> the schematic, what the DT is doing here serves no purpose.
>
> The same comment applies to all the LDO supplies.

I will update with the appropriate labels.

>
>> +                             buck2_reg: BUCK2 {
>> +                                     regulator-name = "vdd_arm";
>> +                                     regulator-min-microvolt = <800000>;
>> +                                     regulator-max-microvolt = <1500000>;
>> +                                     regulator-always-on;
>> +                                     regulator-boot-on;
>> +                             };
>
> Why are you specifying boot-on?  It doesn't seem terribly plausible that
> the cores won't be powered on boot, or that we can't tell what the state
> is by reading the registers.
>
> Same for all the DCDCs.

Yes. Right. Will remove them.

>
>> +                             buck7_reg: BUCK7 {
>> +                                     regulator-name = "vdd_1.0v_ldo";
>> +                                     regulator-min-microvolt = <800000>;
>> +                                     regulator-max-microvolt = <1500000>;
>> +                                     regulator-always-on;
>> +                                     regulator-boot-on;
>> +                             };
>
> This doesn't correspond to the schematic which says that BUCK7 is
> supplying VIN_LLDO_1V4 (the input for some of the LDOs).  The voltage
> range specified there doesn't seem terribly sensible either, are you
> sure you haven't just got the underlying voltage range for the DCDC?
> I can't see why you'd want to raise the voltage over 1.4V (that's rather
> defeating the point) and while it'd be nice to have the core be able to
> vary supply to track the headroom needed by the LDOs none of the LDOs
> were configured with variable voltage so even if we implemented that
> it'd never be used.

That was definitely a mistake. It should be 1.4V Will fix it.

>
> Similar issues apply to the other bucks being used to drop the battery
> voltage for LDO supplies.

Will check and update others appropriately. Thanks for your review and
pointing out the errors.

-- 
With warm regards,
Sachin
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html




[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]
  Powered by Linux