Hi all,
On Fri, Jun 21, 2024 at 9:54 AM Wolfram Sang
<wsa+renesas@xxxxxxxxxxxxxxxxxxxx> wrote:
> > 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";
renesas,r9a09g057-vqmmc-regulator?
> > regulator-name = "vqmmc-regulator";
>
> This should have "sdhi<X>" somewhere in the name?
Indeed.
>
> > 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 = <®_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...
The above indeed starts looking good to me.
IIUIC, the use of the special vqmmc-r9a09g057-regulator is still
optional, and the subnode can be left disabled? E.g. the board
designer may still use a (different) GPIO to control the regulator,
using "regulator-gpio"?
Which brings me to another question: as both the SDmIOVS and SDmPWEN
pins can be configured as GPIOs, why not ignore the special handling
using the SDm_SD_STATUS register, and use "regulator-gpio" instead?
We usually do the same for CD/WP, using "{cd,wp}-gpios" instead.
Exceptions are old SH/R-Mobile and R-Car Gen1 boards:
arch/arm/boot/dts/renesas/r8a73a4-ape6evm.dts: groups =
"sdhi0_data4", "sdhi0_ctrl", "sdhi0_cd";
arch/arm/boot/dts/renesas/r8a7740-armadillo800eva.dts:
groups = "sdhi0_data4", "sdhi0_ctrl", "sdhi0_wp";
arch/arm/boot/dts/renesas/r8a7778-bockw.dts: groups =
"sdhi0_cd", "sdhi0_wp";
arch/arm/boot/dts/renesas/r8a7779-marzen.dts: groups =
"sdhi0_data4", "sdhi0_ctrl", "sdhi0_cd";
arch/arm/boot/dts/renesas/sh73a0-kzm9g.dts: groups =
"sdhi0_data4", "sdhi0_ctrl", "sdhi0_cd", "sdhi0_wp";
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@xxxxxxxxxxxxxx
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
