Re: [RFC PATCH v2 3/3] mmc: renesas_sdhi: Add support for RZ/V2H(P) SoC

[Wolfram Sang]

Hi Prabhakar,

> Based on the feedback from Rob I have now changed it to below, i.e.
> the regulator now probes based on regulator-compatible property value
> "vqmmc-r9a09g057-regulator" instead of regulator node name as the
> driver has of_match in regulator_desc.

I like this a lot! One minor comment.

> static struct regulator_desc r9a09g057_vqmmc_regulator = {
>     .of_match    = of_match_ptr("vqmmc-r9a09g057-regulator"),
>     .owner        = THIS_MODULE,
>     .type        = REGULATOR_VOLTAGE,
>     .ops        = &r9a09g057_regulator_voltage_ops,
>     .volt_table    = r9a09g057_vqmmc_voltages,
>     .n_voltages    = ARRAY_SIZE(r9a09g057_vqmmc_voltages),
> };
> 
> SoC DTSI:
>         sdhi1: mmc@15c10000 {
>             compatible = "renesas,sdhi-r9a09g057";
>             reg = <0x0 0x15c10000 0 0x10000>;
>             interrupts = <GIC_SPI 737 IRQ_TYPE_LEVEL_HIGH>,
>                      <GIC_SPI 738 IRQ_TYPE_LEVEL_HIGH>;
>             clocks = <&cpg CPG_MOD 167>,
>                  <&cpg CPG_MOD 169>,
>                  <&cpg CPG_MOD 168>,
>                  <&cpg CPG_MOD 170>;
>             clock-names = "core", "clkh", "cd", "aclk";
>             resets = <&cpg 168>;
>             power-domains = <&cpg>;
>             status = "disabled";
> 
>             vqmmc_sdhi1: vqmmc-regulator {
>                 regulator-compatible = "vqmmc-r9a09g057-regulator";
>                 regulator-name = "vqmmc-regulator";

This should have "sdhi<X>" somewhere in the name?

>                 regulator-min-microvolt = <1800000>;
>                 regulator-max-microvolt = <3300000>;
>                 status = "disabled";
>             };
>         };
> 
> Board DTS:
> 
> &sdhi1 {
>     pinctrl-0 = <&sdhi1_pins>;
>     pinctrl-1 = <&sdhi1_pins>;
>     pinctrl-names = "default", "state_uhs";
>     vmmc-supply = <&reg_3p3v>;
>     vqmmc-supply = <&vqmmc_sdhi1>;
>     bus-width = <4>;
>     sd-uhs-sdr50;
>     sd-uhs-sdr104;
>     status = "okay";
> };
> 
> &vqmmc_sdhi1 {
>     status = "okay";
> };

Again, I like this. It looks like proper HW description to me.

> Based on the feedback provided Geert ie to use set_pwr callback to set
> PWEN bit and handle IOVS bit in voltage switch callback by dropping
> the regulator altogether. In this case we will have to introduce just
> a single "use-internal-regulator" property and if set make the vqmmc
> regulator optional?

Let's discuss with Geert. But I am quite convinced of your approach
above.

> > > Let me know if I have missed something obvious here.
> >
> > Nope, all good.

Don't give up, I think we are close...

All the best,

   Wolfram

Attachment: signature.asc
Description: PGP signature