Hi Laurent, Thanks for the feedback.
> Subject: Re: [PATCH v5 01/11] i2c: Enhance i2c_new_ancillary_device API > > On Mon, Jun 12, 2023 at 09:53:02AM +0000, Biju Das wrote: > > Hi All, > > > > How do we proceed here between [1] and [2]? > > > > DT-Maintainers suggestion: > > [1] > > raa215300: pmic@12 { > > compatible = "renesas,raa215300"; > > reg = <0x12>, <0x6f>; > > reg-names = "main", "rtc"; > > > > clocks = <&x2>; > > clock-names = "xin"; > > /* Add Optional shared IRQ resource and share it to child and handle > > it both in parent and child */ }; > > > > Laurent/Wolfram suggestion to split it into two nodes and get rid of > this patch: > > [2] > > raa215300: pmic @12 { > > compatible = "renesas,raa215300"; > > reg = <0x12>; > > > > /* Add Optional shared IRQ */ > > renesas,raa215300-rtc = <&rtc_raa215300>; /* Parse the handle > and Enable RTC , if present.*/ > > }; > > > > rtc_raa215300: rtc@6f { > > compatible = "renesas,raa215300-isl1208"; > > Make this > > compatible = "renesas,raa215300-isl1208", "isil,isl1208"; > > Btw, it would be nice to convert > Documentation/devicetree/bindings/rtc/isil,isl1208.txt to YAML. It is already posted see [1] and [2] [1] https://patchwork.kernel.org/project/linux-renesas-soc/patch/20230602142426.438375-6-biju.das...@bp.renesas.com/ [2] https://patchwork.kernel.org/project/linux-renesas-soc/patch/20230602142426.438375-7-biju.das...@bp.renesas.com/ > > > reg = <0x6f>; > > > > /* Add Optional shared IRQ */ > > clocks = <&x2>; > > clock-names = "xin"; > > renesas,raa215300-pmic = <&pmic>; /* Parse the handle to get > PMIC > > version to check Oscillator bit is inverted or not */ > > This isn't nice. I would instead add a renesas,invert-xtoscb boolean > property. If you don't want different DT sources for different revisions > of the PMIC, I need to support all PMIC versions with same image, as PMIC is just a component on the SoM module. So SoM's have different PMIC versions. > one option is to perform the auto-detection in the boot > loader and update the DT dynamically there. Yes, this is an option. Bootloader updates "renesas,invert-xtoscb" property based on PMIC version. Not sure, From binding perspective, Documenting "renesas,invert-xtoscb" is OK for the relevant maintainers?? Cheers, Biju > > > }; > > > > > > -----Original Message----- > > > From: Biju Das > > > Sent: Thursday, June 8, 2023 1:57 PM > > > To: Laurent Pinchart <laurent.pinch...@ideasonboard.com> > > > Cc: Wolfram Sang <w...@kernel.org>; Geert Uytterhoeven <geert@linux- > > > m68k.org>; Krzysztof Kozlowski <krzysztof.kozlowski...@linaro.org>; > > > Rob Herring <robh...@kernel.org>; Andrzej Hajda > > > <andrzej.ha...@intel.com>; Neil Armstrong > > > <neil.armstr...@linaro.org>; Robert Foss <rf...@kernel.org>; David > > > Airlie <airl...@gmail.com>; Daniel Vetter <dan...@ffwll.ch>; Kieran > > > Bingham <kieran.bing...@ideasonboard.com>; > > > Mauro Carvalho Chehab <mche...@kernel.org>; Hans Verkuil <hverkuil- > > > ci...@xs4all.nl>; Alessandro Zummo <a.zu...@towertech.it>; Alexandre > > > Belloni <alexandre.bell...@bootlin.com>; Jonas Karlman > > > <jo...@kwiboo.se>; Jernej Skrabec <jernej.skra...@gmail.com>; Uwe > > > Kleine-König <u.kleine- koe...@pengutronix.de>; Corey Minyard > > > <cminy...@mvista.com>; Marek Behún <ka...@kernel.org>; Jiasheng > > > Jiang <jiash...@iscas.ac.cn>; Antonio Borneo > > > <antonio.bor...@foss.st.com>; Abhinav Kumar > > > <quic_abhin...@quicinc.com>; Ahmad Fatoum <a.fat...@pengutronix.de>; > > > dri-devel@lists.freedesktop.org; linux-...@vger.kernel.org; > > > linux-me...@vger.kernel.org; Geert Uytterhoeven > > > <geert+rene...@glider.be>; Fabrizio Castro > > > <fabrizio.castro...@renesas.com>; linux-renesas-...@vger.kernel.org; > > > Mark Brown <broo...@kernel.org> > > > Subject: RE: [PATCH v5 01/11] i2c: Enhance i2c_new_ancillary_device > > > API > > > > > > Hi Laurent, > > > > > > Thanks for the feedback. > > > > > > > Subject: Re: [PATCH v5 01/11] i2c: Enhance > > > > i2c_new_ancillary_device API > > > > > > > > Hi Biju, > > > > > > > > On Thu, Jun 08, 2023 at 11:00:19AM +0000, Biju Das wrote: > > > > > > Subject: Re: [PATCH v5 01/11] i2c: Enhance > > > > > > i2c_new_ancillary_device API On Thu, Jun 08, 2023 at > 06:41:35AM+0000, Biju Das wrote: > > > > > > > > Subject: RE: [PATCH v5 01/11] i2c: Enhance > > > > > > > > i2c_new_ancillary_device API > > > > > > > > > Subject: Re: [PATCH v5 01/11] i2c: Enhance > > > > > > > > > i2c_new_ancillary_device API > > > > > > > > > > > > > > > > > > Hi all, > > > > > > > > > > > > > > > > > > sorry for not being able to chime in earlier. > > > > > > > > > > > > > > > > > > > In Biju's particular use case, the i2c device responds > > > > > > > > > > to two addresses, which is the standard i2c ancillary > use case. > > > > > > > > > > However, what's special > > > > > > > > > > > > > > > > > > Not quite. ancillary is used when a *driver* needs to > > > > > > > > > take care of two addresses. We already have devices > > > > > > > > > bundling two features into the same chip. I recall at > > > > > > > > > least RTC + EEPROM somewhere. And so far, we have been > > > > > > > > > handling this by creating two nodes in DT and have proper > binding docs. > > > > > > > > > I think this is cleaner. First, you can see in DT > > > > > > > > > already what the compound device really consists of. In > > > > > > > > > this case, which RTC and RTC driver is exactly needed. > > > > > > > > > Second, the code added here adds complexity to the I2C > > > > > > > > > core with another layer of inderection for dummy devices. > > > > > > > > > > > > > > > > FYI, please see [1] and [2] > > > > > > > > > > > > > > > > As per DT maintainers, most of PMICs are described with > > > > > > > > one node, even though RTC is on separate address. > > > > > > > > According to them the DT schema allows multiple addresses > for children. > > > > > > > > But currently we lacks implementation for that. The > > > > > > > > enhancement to this API allows that. > > > > > > > > > > > > > > > > > > As some resources are shared (knowledge about the > > > > > > > > > > clocks), splitting this in two distinct devices in DT > > > > > > > > > > (which is what Biju's initial patch series did) would > > > > > > > > > > need phandles to link both nodes together. > > > > > > > > > > > > > > > > > > > > Do you have a better idea how to represent this? > > > > > > > > > > > > > > > > > > Not sure if I understood this chip correctly, but maybe: > > > > > > > > > The PMIC driver exposes a clock gate which can be > > > > > > > > > consumed by the RTC driver? > > > > > > > > > > > > > > Let me give me some details of this PMIC chip. > > > > > > > > > > > > > > PMIC device has 2 addresses "0x12:- PMIC" , "0x6f"- rtc. > > > > > > > > > > > > > > It has XIN, XOUT, INT# pins and a register for firmware > revisions. > > > > > > > > > > > > Is the firmware revision register accessed through address > > > > > > 0x12 > > > > > > (PMIC) or 0x6f (RTC) ? > > > > > > > > > > 0x12(PMIC). > > > > > > > > > > > > Based on the system design, > > > > > > > > > > > > > > If XIN and XOUT is connected to external crystal, Internal > > > > > > > oscillator is enabled for RTC. In this case we need to set > > > > > > > the oscillator bit to "0". > > > > > > > > > > > > > > If XIN is connected to external clock source, Internal > > > > > > > oscillator is disabled for RTC. In this case we need to set > > > > > > > the oscillator bit to "1". > > > > > > > > > > > > Same here, which address is the oscillator bit accessed through > ? > > > > > > > > > > RTC (0x6F)--> to set oscillator bit. > > > > > > > > And does the PMIC part depend on the oscillator bit being set > > > > correctly, or is that used for the RTC only ? > > > > > > PMIC part does not. It is used only in RTC. > > > > > > Based on PMIC revision, we need to set the oscillator bit in RTC > > > block for PMIC rev a0 and rest of the PMIC chips. > > > > > > On PMIC rev0, oscillator bit is inverted. > > > > > > > > > > If XIN and XOUT not connected RTC operation not possible. > > > > > > > > > > > > > > IRQ# (optional) functionality is shared between PMIC and RTC. > > > > > > > (PMIC fault for various bucks/LDOs/WDT/OTP/NVM and alarm > condition). > > > > > > > > > > > > IRQs can be shared between multiple devices so this shouldn't > > > > > > be a problem. > > > > > > > > > > OK. How do we represent this IRQ in DT? > > > > > > > > You can simply reference the same IRQ from the interrupts property > > > > of different DT nodes. > > > > > > > > > > > The board, I have doesn't populate IRQ# pin. If needed some > > > > > > > customers can populate IRQ# pin and use it for PMIC fault > > > > > > > and RTC alarm. > > > > > > > > > > > > > > Also, currently my board has PMIC rev a0 where oscillator > > > > > > > bit is inverted and internal oscillator is enabled (ie: XIN > > > > > > > and XOUT is connected to external crystal) > > -- > Regards, > > Laurent Pinchart