RE: [PATCH v5 01/11] i2c: Enhance i2c_new_ancillary_device API

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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";
		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 */
	};

Cheers,
Biju

> -----Original Message-----
> From: Biju Das
> Sent: Thursday, June 8, 2023 1:57 PM
> To: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>
> Cc: Wolfram Sang <wsa@xxxxxxxxxx>; Geert Uytterhoeven <geert@linux-
> m68k.org>; Krzysztof Kozlowski <krzysztof.kozlowski+dt@xxxxxxxxxx>; Rob
> Herring <robh+dt@xxxxxxxxxx>; Andrzej Hajda <andrzej.hajda@xxxxxxxxx>;
> Neil Armstrong <neil.armstrong@xxxxxxxxxx>; Robert Foss
> <rfoss@xxxxxxxxxx>; David Airlie <airlied@xxxxxxxxx>; Daniel Vetter
> <daniel@xxxxxxxx>; Kieran Bingham <kieran.bingham@xxxxxxxxxxxxxxxx>;
> Mauro Carvalho Chehab <mchehab@xxxxxxxxxx>; Hans Verkuil <hverkuil-
> cisco@xxxxxxxxx>; Alessandro Zummo <a.zummo@xxxxxxxxxxxx>; Alexandre
> Belloni <alexandre.belloni@xxxxxxxxxxx>; Jonas Karlman <jonas@xxxxxxxxx>;
> Jernej Skrabec <jernej.skrabec@xxxxxxxxx>; Uwe Kleine-König <u.kleine-
> koenig@xxxxxxxxxxxxxx>; Corey Minyard <cminyard@xxxxxxxxxx>; Marek Behún
> <kabel@xxxxxxxxxx>; Jiasheng Jiang <jiasheng@xxxxxxxxxxx>; Antonio Borneo
> <antonio.borneo@xxxxxxxxxxx>; Abhinav Kumar <quic_abhinavk@xxxxxxxxxxx>;
> Ahmad Fatoum <a.fatoum@xxxxxxxxxxxxxx>; dri-devel@xxxxxxxxxxxxxxxxxxxxx;
> linux-i2c@xxxxxxxxxxxxxxx; linux-media@xxxxxxxxxxxxxxx; Geert
> Uytterhoeven <geert+renesas@xxxxxxxxx>; Fabrizio Castro
> <fabrizio.castro.jz@xxxxxxxxxxx>; linux-renesas-soc@xxxxxxxxxxxxxxx; Mark
> Brown <broonie@xxxxxxxxxx>
> 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.
> 
> Cheers,
> Biju
> >
> > > > > 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




[Index of Archives]     [Linux Samsung SOC]     [Linux Wireless]     [Linux Kernel]     [ATH6KL]     [Linux Bluetooth]     [Linux Netdev]     [Kernel Newbies]     [IDE]     [Security]     [Git]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux ATA RAID]     [Samba]     [Device Mapper]

  Powered by Linux