On Sun, Jun 18, 2023 at 06:49:16PM +0200, Jakob Hauser wrote:
> On 17.06.23 16:15, Stephan Gerhold wrote:
> > On Sat, Jun 17, 2023 at 02:29:34AM +0200, Jakob Hauser wrote:
>
> ...
>
> > > + regulators {
> > > + safe_ldo_reg: SAFE_LDO {
> > > + regulator-name = "SAFE_LDO";
> > > + regulator-min-microvolt = <4900000>;
> > > + regulator-max-microvolt = <4900000>;
> > > + regulator-always-on;
> > > + };
> > > + ldo_reg: LDO {
> > > + regulator-name = "LDO";
> > > + regulator-min-microvolt = <2800000>;
> > > + regulator-max-microvolt = <2800000>;
> > > + };
> > > + buck_reg: BUCK {
> > > + regulator-name = "BUCK";
> > > + regulator-min-microvolt = <1200000>;
> > > + regulator-max-microvolt = <1200000>;
> > > + };
> >
> > The "regulator-name"s here don't really seem useful, since they're just
> > the same as the ones already declared in the driver. Can you drop them?
> > Alternatively you could assign more useful board-specific names, such as
> > the CAM_SENSOR_A2.8V that was used downstream.
> >
> > Also, I think it would be slightly clearer to prefix the regulator
> > labels (safe_ldo_reg, ldo_reg etc) with rt5033_. Perhaps
> > "rt5033_ldo_reg" or "rt5033_reg_ldo"?
>
> ...
> About the "regulator-name"s I wasn't really aware. I don't have a strong
> opinion on this.
>
> With the downstream names, it would look like this:
>
> regulators {
> rt5033_reg_safe_ldo: SAFE_LDO {
> regulator-name = "RT5033SafeLDO";
> regulator-min-microvolt = <4900000>;
> regulator-max-microvolt = <4900000>;
> regulator-always-on;
> };
> rt5033_reg_ldo: LDO {
> regulator-name = "CAM_SENSOR_A2.8V";
> regulator-min-microvolt = <2800000>;
> regulator-max-microvolt = <2800000>;
> };
> rt5033_reg_buck: BUCK {
> regulator-name = "CAM_SENSOR_CORE_1.25V";
> regulator-min-microvolt = <1200000>;
> regulator-max-microvolt = <1200000>;
> };
>
> Dropping them would look like this:
>
> regulators {
> rt5033_reg_safe_ldo: SAFE_LDO {
> regulator-min-microvolt = <4900000>;
> regulator-max-microvolt = <4900000>;
> regulator-always-on;
> };
> rt5033_reg_ldo: LDO {
> regulator-min-microvolt = <2800000>;
> regulator-max-microvolt = <2800000>;
> };
> rt5033_reg_buck: BUCK {
> regulator-min-microvolt = <1200000>;
> regulator-max-microvolt = <1200000>;
> };
>
> I would rather drop them. The first name "RT5033SafeLDO" doesn't add much
> information. The other two I'm not fully sure if they provide the cam sensor
> only or if there might be other users as well. Also it add an additional set
> of names. When dropping them, the generic names SAFE_LDO, LDO and BUCK are
> taken from the rt5033-regulator driver.
>
> Unfortunately, I added the example in the dt-bindings with the generic
> names. So this question might come up again when someone else adds
> rt5033-regulators to another device.
>
> For the phandle labels I'd go for rt5033_reg_..., I already changed them in
> the examples above.
>
Sounds good to me, thanks!
Stephan
