From: Biju Das <biju.das.jz@bp.renesas.com>
To: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Cc: "Corey Minyard" <cminyard@mvista.com>,
"Alexandre Belloni" <alexandre.belloni@bootlin.com>,
"Krzysztof Kozlowski" <krzysztof.kozlowski+dt@linaro.org>,
"Geert Uytterhoeven" <geert+renesas@glider.be>,
"Antonio Borneo" <antonio.borneo@foss.st.com>,
"dri-devel@lists.freedesktop.org"
<dri-devel@lists.freedesktop.org>,
"linux-i2c@vger.kernel.org" <linux-i2c@vger.kernel.org>,
"Andrzej Hajda" <andrzej.hajda@intel.com>,
"Trent Piepho" <tpiepho@gmail.com>,
"Marek Behún" <kabel@kernel.org>,
"linux-renesas-soc@vger.kernel.org"
<linux-renesas-soc@vger.kernel.org>,
"Robert Foss" <rfoss@kernel.org>,
"Jonas Karlman" <jonas@kwiboo.se>,
"Kieran Bingham" <kieran.bingham@ideasonboard.com>,
"Jernej Skrabec" <jernej.skrabec@gmail.com>,
"Geert Uytterhoeven" <geert@linux-m68k.org>,
"Uwe Kleine-König" <u.kleine-koenig@pengutronix.de>,
"linux-media@vger.kernel.org" <linux-media@vger.kernel.org>,
"Conor Dooley" <conor+dt@kernel.org>,
"Alessandro Zummo" <a.zummo@towertech.it>,
"Jiasheng Jiang" <jiasheng@iscas.ac.cn>,
"Abhinav Kumar" <quic_abhinavk@quicinc.com>,
"Fabrizio Castro" <fabrizio.castro.jz@renesas.com>,
"Rob Herring" <robh+dt@kernel.org>,
"Ahmad Fatoum" <a.fatoum@pengutronix.de>,
"Mauro Carvalho Chehab" <mchehab@kernel.org>,
"Neil Armstrong" <neil.armstrong@linaro.org>,
"Wolfram Sang" <wsa@kernel.org>,
"Mark Brown" <broonie@kernel.org>,
"Hans Verkuil" <hverkuil-cisco@xs4all.nl>
Subject: RE: [PATCH v5 01/11] i2c: Enhance i2c_new_ancillary_device API
Date: Mon, 12 Jun 2023 12:42:33 +0000 [thread overview]
Message-ID: <OS0PR01MB5922D335D53C7B0FA021B3218654A@OS0PR01MB5922.jpnprd01.prod.outlook.com> (raw)
In-Reply-To: <20230612122353.GA22391@pendragon.ideasonboard.com>
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.jz@bp.renesas.com/
[2]
https://patchwork.kernel.org/project/linux-renesas-soc/patch/20230602142426.438375-7-biju.das.jz@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.pinchart@ideasonboard.com>
> > > Cc: Wolfram Sang <wsa@kernel.org>; Geert Uytterhoeven <geert@linux-
> > > m68k.org>; Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>;
> > > Rob Herring <robh+dt@kernel.org>; Andrzej Hajda
> > > <andrzej.hajda@intel.com>; Neil Armstrong
> > > <neil.armstrong@linaro.org>; Robert Foss <rfoss@kernel.org>; David
> > > Airlie <airlied@gmail.com>; Daniel Vetter <daniel@ffwll.ch>; Kieran
> > > Bingham <kieran.bingham@ideasonboard.com>;
> > > Mauro Carvalho Chehab <mchehab@kernel.org>; Hans Verkuil <hverkuil-
> > > cisco@xs4all.nl>; Alessandro Zummo <a.zummo@towertech.it>; Alexandre
> > > Belloni <alexandre.belloni@bootlin.com>; Jonas Karlman
> > > <jonas@kwiboo.se>; Jernej Skrabec <jernej.skrabec@gmail.com>; Uwe
> > > Kleine-König <u.kleine- koenig@pengutronix.de>; Corey Minyard
> > > <cminyard@mvista.com>; Marek Behún <kabel@kernel.org>; Jiasheng
> > > Jiang <jiasheng@iscas.ac.cn>; Antonio Borneo
> > > <antonio.borneo@foss.st.com>; Abhinav Kumar
> > > <quic_abhinavk@quicinc.com>; Ahmad Fatoum <a.fatoum@pengutronix.de>;
> > > dri-devel@lists.freedesktop.org; linux-i2c@vger.kernel.org;
> > > linux-media@vger.kernel.org; Geert Uytterhoeven
> > > <geert+renesas@glider.be>; Fabrizio Castro
> > > <fabrizio.castro.jz@renesas.com>; linux-renesas-soc@vger.kernel.org;
> > > Mark Brown <broonie@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
next prev parent reply other threads:[~2023-06-12 12:42 UTC|newest]
Thread overview: 61+ messages / expand[flat|nested] mbox.gz Atom feed top
[not found] <20230522101849.297499-1-biju.das.jz@bp.renesas.com>
2023-05-22 10:18 ` [PATCH v5 01/11] i2c: Enhance i2c_new_ancillary_device API Biju Das
2023-05-23 9:50 ` Hans Verkuil
2023-05-25 16:49 ` Geert Uytterhoeven
2023-05-29 8:05 ` Laurent Pinchart
2023-05-29 9:00 ` Biju Das
2023-05-31 8:59 ` Laurent Pinchart
2023-05-31 9:34 ` Biju Das
2023-05-31 11:41 ` Laurent Pinchart
2023-05-31 12:53 ` Biju Das
2023-05-31 13:35 ` Laurent Pinchart
2023-05-31 13:44 ` Biju Das
2023-06-02 7:40 ` Biju Das
2023-05-31 13:37 ` Geert Uytterhoeven
2023-05-31 13:47 ` Biju Das
2023-05-31 12:51 ` Geert Uytterhoeven
2023-05-31 13:37 ` Laurent Pinchart
2023-05-31 13:39 ` Geert Uytterhoeven
2023-06-05 9:30 ` Wolfram Sang
2023-06-07 8:53 ` Wolfram Sang
2023-06-07 10:58 ` Biju Das
2023-06-08 6:41 ` Biju Das
2023-06-08 10:39 ` Laurent Pinchart
2023-06-08 11:00 ` Biju Das
2023-06-08 12:50 ` Laurent Pinchart
2023-06-08 12:57 ` Biju Das
2023-06-12 9:53 ` Biju Das
2023-06-12 12:23 ` Laurent Pinchart
2023-06-12 12:42 ` Biju Das [this message]
2023-06-12 12:54 ` Laurent Pinchart
2023-06-12 13:08 ` Geert Uytterhoeven
2023-06-12 13:19 ` Laurent Pinchart
2023-06-12 12:44 ` Geert Uytterhoeven
2023-06-12 13:02 ` Laurent Pinchart
2023-06-12 12:35 ` Wolfram Sang
2023-06-12 12:42 ` Geert Uytterhoeven
2023-06-12 12:48 ` Wolfram Sang
2023-06-12 13:00 ` Geert Uytterhoeven
2023-06-12 20:43 ` Wolfram Sang
2023-06-13 7:24 ` Biju Das
2023-06-13 17:57 ` Biju Das
2023-06-13 19:31 ` Biju Das
2023-06-14 8:13 ` Laurent Pinchart
2023-06-14 8:21 ` Biju Das
2023-06-14 9:18 ` Geert Uytterhoeven
2023-06-14 11:04 ` Biju Das
2023-06-15 9:00 ` Geert Uytterhoeven
2023-06-14 9:54 ` Laurent Pinchart
2023-06-14 11:30 ` Biju Das
2023-06-15 9:26 ` Laurent Pinchart
2023-06-20 8:06 ` Biju Das
2023-06-13 7:25 ` Geert Uytterhoeven
2023-06-13 10:45 ` Biju Das
2023-06-13 14:51 ` Geert Uytterhoeven
2023-06-13 16:11 ` Biju Das
2023-06-14 7:53 ` Geert Uytterhoeven
2023-06-14 8:02 ` Biju Das
2023-06-15 8:07 ` Geert Uytterhoeven
2023-06-15 9:23 ` Laurent Pinchart
2023-06-16 6:32 ` Wolfram Sang
2023-06-19 8:17 ` Biju Das
2023-06-12 13:00 ` Biju Das
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=OS0PR01MB5922D335D53C7B0FA021B3218654A@OS0PR01MB5922.jpnprd01.prod.outlook.com \
--to=biju.das.jz@bp.renesas.com \
--cc=a.fatoum@pengutronix.de \
--cc=a.zummo@towertech.it \
--cc=alexandre.belloni@bootlin.com \
--cc=andrzej.hajda@intel.com \
--cc=antonio.borneo@foss.st.com \
--cc=broonie@kernel.org \
--cc=cminyard@mvista.com \
--cc=conor+dt@kernel.org \
--cc=dri-devel@lists.freedesktop.org \
--cc=fabrizio.castro.jz@renesas.com \
--cc=geert+renesas@glider.be \
--cc=geert@linux-m68k.org \
--cc=hverkuil-cisco@xs4all.nl \
--cc=jernej.skrabec@gmail.com \
--cc=jiasheng@iscas.ac.cn \
--cc=jonas@kwiboo.se \
--cc=kabel@kernel.org \
--cc=kieran.bingham@ideasonboard.com \
--cc=krzysztof.kozlowski+dt@linaro.org \
--cc=laurent.pinchart@ideasonboard.com \
--cc=linux-i2c@vger.kernel.org \
--cc=linux-media@vger.kernel.org \
--cc=linux-renesas-soc@vger.kernel.org \
--cc=mchehab@kernel.org \
--cc=neil.armstrong@linaro.org \
--cc=quic_abhinavk@quicinc.com \
--cc=rfoss@kernel.org \
--cc=robh+dt@kernel.org \
--cc=tpiepho@gmail.com \
--cc=u.kleine-koenig@pengutronix.de \
--cc=wsa@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).