On Wed, Nov 07, 2018 at 01:30:10PM +0100, Simon Horman wrote: > 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 <ykaneko0...@gmail.com> > > wrote: > > > From: Takeshi Kihara <takeshi.kihara...@renesas.com> > > > > > > This patch adds I2C-DVFS device node for the R8A77990 SoC. > > > > > > Signed-off-by: Takeshi Kihara <takeshi.kihara...@renesas.com> > > > Signed-off-by: Yoshihiro Kaneko <ykaneko0...@gmail.com> > > > > 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?
Further examination by Wolfram Sang has shown that the code in question is only used on the r8a7740. I don't think there is any compatibility issue for r8a77990 when using the current driver. > > > > > + 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"; > > > + }; > > > + >