Re: [U-Boot] [PATCH v2 08/11] sandbox: Enable support for MC34708 PMIC in DTS

2018-05-13 Thread Simon Glass 
On 7 May 2018 at 06:26, Lukasz Majewski  wrote:
> This commit also provides the default values of the emulated MC34708 PMIC
> internal registers content.
>
> Signed-off-by: Lukasz Majewski 
>
> ---
>
> Changes in v2:
> - New patch
>
>  arch/sandbox/dts/sandbox.dts   |  4 
>  arch/sandbox/dts/sandbox64.dts |  4 
>  arch/sandbox/dts/sandbox_pmic.dtsi | 33 +
>  arch/sandbox/dts/test.dts  |  4 
>  4 files changed, 45 insertions(+)

Reviewed-by: Simon Glass 
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

Re: [U-Boot] [PATCH v2 08/11] sandbox: Enable support for MC34708 PMIC in DTS

2018-05-07 Thread Lukasz Majewski 
Hi Fabio,

> Hi Lukasz,
> 
> On Sun, May 6, 2018 at 5:26 PM, Lukasz Majewski  wrote:
> 
> > diff --git a/arch/sandbox/dts/sandbox.dts
> > b/arch/sandbox/dts/sandbox.dts index 1fb8225fbb..b187b6fac1 100644
> > --- a/arch/sandbox/dts/sandbox.dts
> > +++ b/arch/sandbox/dts/sandbox.dts
> > @@ -115,6 +115,10 @@
> > sandbox_pmic: sandbox_pmic {
> > reg = <0x40>;
> > };
> > +
> > +   mc34708_pmic: mc34708_pmic {
> > +   reg = <0x41>;
> > +   };  
> 
> I know you are following the current style of this file, but this
> looks incorrect.
> 
> According to Devicetree Specification v0.2 document:
> 
> "The name of a node should be somewhat generic, reflecting the
> function of the device and not its precise programming model."
> 
> Also, the reg property needs to have a corresponding unit address.
> 
> It would better to rewrite this as:
> 
> mc34708: pmic@41 {
>  reg = <0x41>
> };

Yes, you are right. I've blindly followed the current (wrong) code.

I'm now waiting for comments from Simon and will fix this in v3.

Thanks for review.


Best regards,

Lukasz Majewski

--

DENX Software Engineering GmbH,  Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: w...@denx.de


pgp82QpefYUeU.pgp
Description: OpenPGP digital signature
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

Re: [U-Boot] [PATCH v2 08/11] sandbox: Enable support for MC34708 PMIC in DTS

2018-05-06 Thread Fabio Estevam 
Hi Lukasz,

On Sun, May 6, 2018 at 5:26 PM, Lukasz Majewski  wrote:

> diff --git a/arch/sandbox/dts/sandbox.dts b/arch/sandbox/dts/sandbox.dts
> index 1fb8225fbb..b187b6fac1 100644
> --- a/arch/sandbox/dts/sandbox.dts
> +++ b/arch/sandbox/dts/sandbox.dts
> @@ -115,6 +115,10 @@
> sandbox_pmic: sandbox_pmic {
> reg = <0x40>;
> };
> +
> +   mc34708_pmic: mc34708_pmic {
> +   reg = <0x41>;
> +   };

I know you are following the current style of this file, but this
looks incorrect.

According to Devicetree Specification v0.2 document:

"The name of a node should be somewhat generic, reflecting the function
of the device and not its precise programming model."

Also, the reg property needs to have a corresponding unit address.

It would better to rewrite this as:

mc34708: pmic@41 {
 reg = <0x41>
};
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot