On Mon, Nov 05, 2018 at 09:52:48AM +0100, Geert Uytterhoeven wrote:
> Hi Kaneko-san,
>
> On Sat, Oct 20, 2018 at 11:35 PM Yoshihiro Kaneko <ykaneko0929@xxxxxxxxx> wrote:
> > From: Takeshi Kihara <takeshi.kihara.df@xxxxxxxxxxx>
> >
> > This patch adds I2C-DVFS device node for the R8A77990 SoC.
> >
> > Signed-off-by: Takeshi Kihara <takeshi.kihara.df@xxxxxxxxxxx>
> > Signed-off-by: Yoshihiro Kaneko <ykaneko0929@xxxxxxxxx>
>
> Thanks for your patch!
>
> > --- a/arch/arm64/boot/dts/renesas/r8a77990.dtsi
> > +++ b/arch/arm64/boot/dts/renesas/r8a77990.dtsi
> > @@ -22,7 +22,8 @@
> > i2c4 = &i2c4;
> > i2c5 = &i2c5;
> > i2c6 = &i2c6;
> > - i2c7 = &i2c7;
> > + i2c7 = &i2c_dvfs;
> > + i2c8 = &i2c7;
>
> Please don't change existing aliases.
> While this makes the use of i2c7 for PMIC access uniform across the
> range of R-Car Gen3 SoCs that have it, I think this is a bad idea, and that
> it is better not to rely on I2C aliases at all.
>
> I guess the BSP did this to configure the BD9571 PMIC for DDR backup
> mode using i2cset? Upstream has another method, avoiding the need for
> i2cset, cfr. Documentation/ABI/testing/sysfs-driver-bd9571mwv-regulator.
Dropping the above hung makes sense to me.
Out of interest, what would be the implication of removing
existing aliases?
>
> > };
> >
> > cpus {
> > @@ -337,6 +338,22 @@
> > reg = <0 0xe6060000 0 0x508>;
> > };
> >
> > + i2c_dvfs: i2c@e60b0000 {
> > + #address-cells = <1>;
> > + #size-cells = <0>;
> > + compatible = "renesas,iic-r8a77990",
>
> "renesas,iic-r8a77990" is not yet documented.
Thanks, as per my comment below I wonder if as well as documenting
"renesas,iic-r8a77990" we also to teach the driver about it.
>
> > + "renesas,rcar-gen3-iic",
> > + "renesas,rmobile-iic";
>
> Also, IIC on R-Car E3 does not have the automatic transmission registers.
> Does this affect claiming compatibility with the family-specific or generic
> compatible values?
My cursory reading of the driver indicates that only register that is
used by it but documented as not existing on E3 is ICSTART (offset 0x70).
It seems to me that we should confirm with Renesas that the register does
indeed not exist - as this patch comes from the BSP which implies it is
being used there. And if it does not exist we should try teaching the
driver not to use ICSTART via the "renesas,iic-r8a77990" compat string.
What do you think?
> > + reg = <0 0xe60b0000 0 0x34>;
>
> Why 0x34? Last (byte-sized) register documented is at 0x14 => 0x15?
I can't explain 0x34 but I agree 0x15 would match the documentation.
>
> > + interrupts = <GIC_SPI 173 IRQ_TYPE_LEVEL_HIGH>;
> > + clocks = <&cpg CPG_MOD 926>;
> > + power-domains = <&sysc R8A77990_PD_ALWAYS_ON>;
> > + resets = <&cpg 926>;
> > + dmas = <&dmac0 0x11>, <&dmac0 0x10>;
> > + dma-names = "tx", "rx";
> > + status = "disabled";
> > + };
> > +
