* [PATCH v5 0/3] IMX274 fixes and power on and off implementation @ 2020-09-02 2:04 Sowjanya Komatineni 2020-09-02 2:04 ` [PATCH v5 1/3] media: i2c: imx274: Fix Y_OUT_SIZE register setting Sowjanya Komatineni ` (2 more replies) 0 siblings, 3 replies; 18+ messages in thread From: Sowjanya Komatineni @ 2020-09-02 2:04 UTC (permalink / raw) To: skomatineni, thierry.reding, jonathanh, sakari.ailus, hverkuil, jacopo+renesas, luca, leonl, robh+dt, lgirdwood, broonie Cc: linux-media, devicetree, linux-kernel This patch series includes - Fix for proper Y_OUT_SIZE register configuration. - Power on/off sequence implementation through runtime PM. - dt-binding doc to add optional input clock and supplies Delta between patch versions: [v5]: Includes below v4 feedback - dt-bindings patch to add optional clock and supplies and rebased on below json-schema patch. https://patchwork.kernel.org/patch/11732875/ - Other minor v4 feedbacks. [v4]: Includes below v3 feedback - Implemented power on/off through Runtime PM. - Use regulator bulk APIs. - Use lower case for supply names. [v3]: Includes below v2 feedback - Removed explicit clk_set_rate from driver as default external input clock rate can be configured through DT. [v2]: Includes below changes based on v1 feedback - External input clock name changed from xclk to inck. - implementation change for get regulators to store all in array. - To keep in reset low prior to regulators power on. Sowjanya Komatineni (3): media: i2c: imx274: Fix Y_OUT_SIZE register setting dt-bindings: media: imx274: Add optional input clock and supplies media: i2c: imx274: Add IMX274 power on and off sequence .../devicetree/bindings/media/i2c/sony,imx274.yaml | 21 ++++ drivers/media/i2c/imx274.c | 136 ++++++++++++++++++++- 2 files changed, 153 insertions(+), 4 deletions(-) -- 2.7.4 ^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH v5 1/3] media: i2c: imx274: Fix Y_OUT_SIZE register setting 2020-09-02 2:04 [PATCH v5 0/3] IMX274 fixes and power on and off implementation Sowjanya Komatineni @ 2020-09-02 2:04 ` Sowjanya Komatineni 2020-09-02 2:04 ` [PATCH v5 2/3] dt-bindings: media: imx274: Add optional input clock and supplies Sowjanya Komatineni 2020-09-02 2:04 ` [PATCH v5 3/3] media: i2c: imx274: Add IMX274 power on and off sequence Sowjanya Komatineni 2 siblings, 0 replies; 18+ messages in thread From: Sowjanya Komatineni @ 2020-09-02 2:04 UTC (permalink / raw) To: skomatineni, thierry.reding, jonathanh, sakari.ailus, hverkuil, jacopo+renesas, luca, leonl, robh+dt, lgirdwood, broonie Cc: linux-media, devicetree, linux-kernel As per Sony IMX274 Y_OUT_SIZE should be the height of effective image output from the sensor which are the actual total lines sent over MIPI CSI to receiver. So, Y_OUT_SIZE should be same as crop height and this patch fixes it. Signed-off-by: Sowjanya Komatineni <skomatineni@nvidia.com> --- drivers/media/i2c/imx274.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/media/i2c/imx274.c b/drivers/media/i2c/imx274.c index 6011cec..a4b9dfd 100644 --- a/drivers/media/i2c/imx274.c +++ b/drivers/media/i2c/imx274.c @@ -1163,7 +1163,7 @@ static int imx274_apply_trimming(struct stimx274 *imx274) (-imx274->crop.top / 2) : (imx274->crop.top / 2); v_cut = (IMX274_MAX_HEIGHT - imx274->crop.height) / 2; write_v_size = imx274->crop.height + 22; - y_out_size = imx274->crop.height + 14; + y_out_size = imx274->crop.height; err = imx274_write_mbreg(imx274, IMX274_HMAX_REG_LSB, hmax, 2); if (!err) -- 2.7.4 ^ permalink raw reply related [flat|nested] 18+ messages in thread
* [PATCH v5 2/3] dt-bindings: media: imx274: Add optional input clock and supplies 2020-09-02 2:04 [PATCH v5 0/3] IMX274 fixes and power on and off implementation Sowjanya Komatineni 2020-09-02 2:04 ` [PATCH v5 1/3] media: i2c: imx274: Fix Y_OUT_SIZE register setting Sowjanya Komatineni @ 2020-09-02 2:04 ` Sowjanya Komatineni 2020-09-02 6:46 ` Luca Ceresoli 2020-09-03 12:55 ` Jacopo Mondi 2020-09-02 2:04 ` [PATCH v5 3/3] media: i2c: imx274: Add IMX274 power on and off sequence Sowjanya Komatineni 2 siblings, 2 replies; 18+ messages in thread From: Sowjanya Komatineni @ 2020-09-02 2:04 UTC (permalink / raw) To: skomatineni, thierry.reding, jonathanh, sakari.ailus, hverkuil, jacopo+renesas, luca, leonl, robh+dt, lgirdwood, broonie Cc: linux-media, devicetree, linux-kernel This patch adds IMX274 optional external clock input and voltage supplies to device tree bindings. Reviewed-by: Luca Ceresoli <luca@lucaceresoli.net> Signed-off-by: Sowjanya Komatineni <skomatineni@nvidia.com> --- .../devicetree/bindings/media/i2c/sony,imx274.yaml | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/Documentation/devicetree/bindings/media/i2c/sony,imx274.yaml b/Documentation/devicetree/bindings/media/i2c/sony,imx274.yaml index 7ae47a6..57e7176 100644 --- a/Documentation/devicetree/bindings/media/i2c/sony,imx274.yaml +++ b/Documentation/devicetree/bindings/media/i2c/sony,imx274.yaml @@ -25,6 +25,27 @@ properties: reset-gpios: maxItems: 1 + clocks: + maxItems: 1 + description: Reference to the sensor input clock + + clock-names: + maxItems: 1 + items: + - const: inck + + vana-supply: + description: + Analog voltage supply, 2.8 volts + + vdig-supply: + description: + Digital IO voltage supply, 1.8 volts + + vddl-supply: + description: + Digital core voltage supply, 1.2 volts + port: type: object description: | -- 2.7.4 ^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH v5 2/3] dt-bindings: media: imx274: Add optional input clock and supplies 2020-09-02 2:04 ` [PATCH v5 2/3] dt-bindings: media: imx274: Add optional input clock and supplies Sowjanya Komatineni @ 2020-09-02 6:46 ` Luca Ceresoli 2020-09-02 6:52 ` Luca Ceresoli 2020-09-03 12:55 ` Jacopo Mondi 1 sibling, 1 reply; 18+ messages in thread From: Luca Ceresoli @ 2020-09-02 6:46 UTC (permalink / raw) To: Sowjanya Komatineni, thierry.reding, jonathanh, sakari.ailus, hverkuil, jacopo+renesas, leonl, robh+dt, lgirdwood, broonie Cc: linux-media, devicetree, linux-kernel Hi Sowjanya, On 02/09/20 04:04, Sowjanya Komatineni wrote: > This patch adds IMX274 optional external clock input and voltage > supplies to device tree bindings. > > Reviewed-by: Luca Ceresoli <luca@lucaceresoli.net> > Signed-off-by: Sowjanya Komatineni <skomatineni@nvidia.com> > --- > .../devicetree/bindings/media/i2c/sony,imx274.yaml | 21 +++++++++++++++++++++ > 1 file changed, 21 insertions(+) > > diff --git a/Documentation/devicetree/bindings/media/i2c/sony,imx274.yaml b/Documentation/devicetree/bindings/media/i2c/sony,imx274.yaml > index 7ae47a6..57e7176 100644 > --- a/Documentation/devicetree/bindings/media/i2c/sony,imx274.yaml > +++ b/Documentation/devicetree/bindings/media/i2c/sony,imx274.yaml > @@ -25,6 +25,27 @@ properties: > reset-gpios: > maxItems: 1 > > + clocks: > + maxItems: 1 > + description: Reference to the sensor input clock > + > + clock-names: > + maxItems: 1 > + items: > + - const: inck I think this can be simpler: clock-names: maxItems: 1 items: - const: inck As in: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/clock/idt,versaclock5.yaml?h=v5.9-rc3#n90 -- Luca ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v5 2/3] dt-bindings: media: imx274: Add optional input clock and supplies 2020-09-02 6:46 ` Luca Ceresoli @ 2020-09-02 6:52 ` Luca Ceresoli 0 siblings, 0 replies; 18+ messages in thread From: Luca Ceresoli @ 2020-09-02 6:52 UTC (permalink / raw) To: Sowjanya Komatineni, thierry.reding, jonathanh, sakari.ailus, hverkuil, jacopo+renesas, leonl, robh+dt, lgirdwood, broonie Cc: linux-media, devicetree, linux-kernel Hi again... On 02/09/20 08:46, Luca Ceresoli wrote: > Hi Sowjanya, > > On 02/09/20 04:04, Sowjanya Komatineni wrote: >> This patch adds IMX274 optional external clock input and voltage >> supplies to device tree bindings. >> >> Reviewed-by: Luca Ceresoli <luca@lucaceresoli.net> >> Signed-off-by: Sowjanya Komatineni <skomatineni@nvidia.com> >> --- >> .../devicetree/bindings/media/i2c/sony,imx274.yaml | 21 +++++++++++++++++++++ >> 1 file changed, 21 insertions(+) >> >> diff --git a/Documentation/devicetree/bindings/media/i2c/sony,imx274.yaml b/Documentation/devicetree/bindings/media/i2c/sony,imx274.yaml >> index 7ae47a6..57e7176 100644 >> --- a/Documentation/devicetree/bindings/media/i2c/sony,imx274.yaml >> +++ b/Documentation/devicetree/bindings/media/i2c/sony,imx274.yaml >> @@ -25,6 +25,27 @@ properties: >> reset-gpios: >> maxItems: 1 >> >> + clocks: >> + maxItems: 1 >> + description: Reference to the sensor input clock >> + >> + clock-names: >> + maxItems: 1 >> + items: >> + - const: inck > > I think this can be simpler: > > clock-names: > maxItems: 1 > items: > - const: inck Which is equal to the original... copy-paste-and-forgot. Here's the simplified form, for real: clocks: maxItems: 1 clock-names: const: inck As in: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/clock/idt,versaclock5.yaml?h=v5.9-rc3#n90 -- Luca ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v5 2/3] dt-bindings: media: imx274: Add optional input clock and supplies 2020-09-02 2:04 ` [PATCH v5 2/3] dt-bindings: media: imx274: Add optional input clock and supplies Sowjanya Komatineni 2020-09-02 6:46 ` Luca Ceresoli @ 2020-09-03 12:55 ` Jacopo Mondi 2020-09-03 16:05 ` Sowjanya Komatineni 1 sibling, 1 reply; 18+ messages in thread From: Jacopo Mondi @ 2020-09-03 12:55 UTC (permalink / raw) To: Sowjanya Komatineni Cc: thierry.reding, jonathanh, sakari.ailus, hverkuil, jacopo+renesas, luca, leonl, robh+dt, lgirdwood, broonie, linux-media, devicetree, linux-kernel Hello Sowjanya, On Tue, Sep 01, 2020 at 07:04:37PM -0700, Sowjanya Komatineni wrote: > This patch adds IMX274 optional external clock input and voltage > supplies to device tree bindings. > > Reviewed-by: Luca Ceresoli <luca@lucaceresoli.net> > Signed-off-by: Sowjanya Komatineni <skomatineni@nvidia.com> > --- > .../devicetree/bindings/media/i2c/sony,imx274.yaml | 21 +++++++++++++++++++++ > 1 file changed, 21 insertions(+) > > diff --git a/Documentation/devicetree/bindings/media/i2c/sony,imx274.yaml b/Documentation/devicetree/bindings/media/i2c/sony,imx274.yaml > index 7ae47a6..57e7176 100644 > --- a/Documentation/devicetree/bindings/media/i2c/sony,imx274.yaml > +++ b/Documentation/devicetree/bindings/media/i2c/sony,imx274.yaml > @@ -25,6 +25,27 @@ properties: > reset-gpios: > maxItems: 1 > I just sent an update to my json-schema conversion of this bindings document (not yet on patchwork, sorry) and Sakari pointed me to the fact in between my v2 and my v4 this patch from you went in: 4ea3273d24b ("dt-bindings: media: imx274: Add optional input clock and supplies") I should probably now update my bindings conversion patch, basically taking in what you've done here, but I would have one question. > + clocks: > + maxItems: 1 > + description: Reference to the sensor input clock > + > + clock-names: > + maxItems: 1 > + items: > + - const: inck > + > + vana-supply: > + description: > + Analog voltage supply, 2.8 volts > + > + vdig-supply: > + description: > + Digital IO voltage supply, 1.8 volts > + > + vddl-supply: > + description: > + Digital core voltage supply, 1.2 volts 4ea3273d24b introduced these regulators as VANA-supply, VDIG-supply and VDDL-supply (please note the upper-case names). This version uses lower-case ones instead. Is this intentional ? The driver currently does not parse any of these if I'm not mistaken, but as the bindings in textual form defines an ABI which should be preserved during the conversion to json-schema, should these be kept in upper-case ? Thanks j > + > port: > type: object > description: | > -- > 2.7.4 > ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v5 2/3] dt-bindings: media: imx274: Add optional input clock and supplies 2020-09-03 12:55 ` Jacopo Mondi @ 2020-09-03 16:05 ` Sowjanya Komatineni 2020-09-03 16:35 ` Jacopo Mondi 0 siblings, 1 reply; 18+ messages in thread From: Sowjanya Komatineni @ 2020-09-03 16:05 UTC (permalink / raw) To: Jacopo Mondi Cc: thierry.reding, jonathanh, sakari.ailus, hverkuil, jacopo+renesas, luca, leonl, robh+dt, lgirdwood, broonie, linux-media, devicetree, linux-kernel On 9/3/20 5:55 AM, Jacopo Mondi wrote: > Hello Sowjanya, > > On Tue, Sep 01, 2020 at 07:04:37PM -0700, Sowjanya Komatineni wrote: >> This patch adds IMX274 optional external clock input and voltage >> supplies to device tree bindings. >> >> Reviewed-by: Luca Ceresoli <luca@lucaceresoli.net> >> Signed-off-by: Sowjanya Komatineni <skomatineni@nvidia.com> >> --- >> .../devicetree/bindings/media/i2c/sony,imx274.yaml | 21 +++++++++++++++++++++ >> 1 file changed, 21 insertions(+) >> >> diff --git a/Documentation/devicetree/bindings/media/i2c/sony,imx274.yaml b/Documentation/devicetree/bindings/media/i2c/sony,imx274.yaml >> index 7ae47a6..57e7176 100644 >> --- a/Documentation/devicetree/bindings/media/i2c/sony,imx274.yaml >> +++ b/Documentation/devicetree/bindings/media/i2c/sony,imx274.yaml >> @@ -25,6 +25,27 @@ properties: >> reset-gpios: >> maxItems: 1 >> > I just sent an update to my json-schema conversion of this bindings > document (not yet on patchwork, sorry) and Sakari pointed me to the > fact in between my v2 and my v4 this patch from you went in: > 4ea3273d24b ("dt-bindings: media: imx274: Add optional input clock and supplies") > > I should probably now update my bindings conversion patch, basically > taking in what you've done here, but I would have one question. > >> + clocks: >> + maxItems: 1 >> + description: Reference to the sensor input clock >> + >> + clock-names: >> + maxItems: 1 >> + items: >> + - const: inck >> + >> + vana-supply: >> + description: >> + Analog voltage supply, 2.8 volts >> + >> + vdig-supply: >> + description: >> + Digital IO voltage supply, 1.8 volts >> + >> + vddl-supply: >> + description: >> + Digital core voltage supply, 1.2 volts > 4ea3273d24b introduced these regulators as VANA-supply, VDIG-supply > and VDDL-supply (please note the upper-case names). This version uses > lower-case ones instead. Is this intentional ? The driver currently > does not parse any of these if I'm not mistaken, but as the bindings > in textual form defines an ABI which should be preserved during the > conversion to json-schema, should these be kept in upper-case ? > > Thanks > j Yes, based on feedback lower case was recommended. So, changed to use lower-case names. These properties were not used by driver currently and from my prior series only dt-binding got merged as no feedback was received on it for all prior versions. So, should be ok to change to lower-case as there properties are introduced now and driver update using these properties is under review >> + >> port: >> type: object >> description: | >> -- >> 2.7.4 >> ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v5 2/3] dt-bindings: media: imx274: Add optional input clock and supplies 2020-09-03 16:05 ` Sowjanya Komatineni @ 2020-09-03 16:35 ` Jacopo Mondi 2020-09-03 16:40 ` Sowjanya Komatineni 0 siblings, 1 reply; 18+ messages in thread From: Jacopo Mondi @ 2020-09-03 16:35 UTC (permalink / raw) To: Sowjanya Komatineni Cc: thierry.reding, jonathanh, sakari.ailus, hverkuil, jacopo+renesas, luca, leonl, robh+dt, lgirdwood, broonie, linux-media, devicetree, linux-kernel Hi Sowjanya, On Thu, Sep 03, 2020 at 09:05:27AM -0700, Sowjanya Komatineni wrote: > > On 9/3/20 5:55 AM, Jacopo Mondi wrote: > > Hello Sowjanya, > > > > On Tue, Sep 01, 2020 at 07:04:37PM -0700, Sowjanya Komatineni wrote: > > > This patch adds IMX274 optional external clock input and voltage > > > supplies to device tree bindings. > > > > > > Reviewed-by: Luca Ceresoli <luca@lucaceresoli.net> > > > Signed-off-by: Sowjanya Komatineni <skomatineni@nvidia.com> > > > --- > > > .../devicetree/bindings/media/i2c/sony,imx274.yaml | 21 +++++++++++++++++++++ > > > 1 file changed, 21 insertions(+) > > > > > > diff --git a/Documentation/devicetree/bindings/media/i2c/sony,imx274.yaml b/Documentation/devicetree/bindings/media/i2c/sony,imx274.yaml > > > index 7ae47a6..57e7176 100644 > > > --- a/Documentation/devicetree/bindings/media/i2c/sony,imx274.yaml > > > +++ b/Documentation/devicetree/bindings/media/i2c/sony,imx274.yaml > > > @@ -25,6 +25,27 @@ properties: > > > reset-gpios: > > > maxItems: 1 > > > > > I just sent an update to my json-schema conversion of this bindings > > document (not yet on patchwork, sorry) and Sakari pointed me to the > > fact in between my v2 and my v4 this patch from you went in: > > 4ea3273d24b ("dt-bindings: media: imx274: Add optional input clock and supplies") > > > > I should probably now update my bindings conversion patch, basically > > taking in what you've done here, but I would have one question. > > > > > + clocks: > > > + maxItems: 1 > > > + description: Reference to the sensor input clock > > > + > > > + clock-names: > > > + maxItems: 1 > > > + items: > > > + - const: inck > > > + > > > + vana-supply: > > > + description: > > > + Analog voltage supply, 2.8 volts > > > + > > > + vdig-supply: > > > + description: > > > + Digital IO voltage supply, 1.8 volts > > > + > > > + vddl-supply: > > > + description: > > > + Digital core voltage supply, 1.2 volts > > 4ea3273d24b introduced these regulators as VANA-supply, VDIG-supply > > and VDDL-supply (please note the upper-case names). This version uses > > lower-case ones instead. Is this intentional ? The driver currently > > does not parse any of these if I'm not mistaken, but as the bindings > > in textual form defines an ABI which should be preserved during the > > conversion to json-schema, should these be kept in upper-case ? > > > > Thanks > > j > > Yes, based on feedback lower case was recommended. So, changed to use > lower-case names. > > These properties were not used by driver currently and from my prior series > only dt-binding got merged as no feedback was received on it for all prior > versions. > > So, should be ok to change to lower-case as there properties are introduced > now and driver update using these properties is under review > Well, I see that patch went in v5.9-rc1, so it will be part of v5.9. If the bindings update goes in in v5.10 (or whatever comes after v5.9) then we have a problem, as the DTB created for v5.9 won't work anymore on any later version, and that should not happen. Alternatively, a fix for the next -rc release could be fast-tracked, but you would need to synchronize with the dt maintainers for that and make a patch for the existing .txt bindings file. If the name change happens in the yaml file and one release is made with the old names, then we're stuck with those forever and ever, if I got the situation right. Please check with the dt and media maintainers, or they can comment here if they glance through these lines. Thanks j > > > + > > > port: > > > type: object > > > description: | > > > -- > > > 2.7.4 > > > ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v5 2/3] dt-bindings: media: imx274: Add optional input clock and supplies 2020-09-03 16:35 ` Jacopo Mondi @ 2020-09-03 16:40 ` Sowjanya Komatineni 2020-09-08 9:33 ` Sakari Ailus 0 siblings, 1 reply; 18+ messages in thread From: Sowjanya Komatineni @ 2020-09-03 16:40 UTC (permalink / raw) To: Jacopo Mondi Cc: thierry.reding, jonathanh, sakari.ailus, hverkuil, jacopo+renesas, luca, leonl, robh+dt, lgirdwood, broonie, linux-media, devicetree, linux-kernel On 9/3/20 9:35 AM, Jacopo Mondi wrote: > Hi Sowjanya, > > On Thu, Sep 03, 2020 at 09:05:27AM -0700, Sowjanya Komatineni wrote: >> On 9/3/20 5:55 AM, Jacopo Mondi wrote: >>> Hello Sowjanya, >>> >>> On Tue, Sep 01, 2020 at 07:04:37PM -0700, Sowjanya Komatineni wrote: >>>> This patch adds IMX274 optional external clock input and voltage >>>> supplies to device tree bindings. >>>> >>>> Reviewed-by: Luca Ceresoli <luca@lucaceresoli.net> >>>> Signed-off-by: Sowjanya Komatineni <skomatineni@nvidia.com> >>>> --- >>>> .../devicetree/bindings/media/i2c/sony,imx274.yaml | 21 +++++++++++++++++++++ >>>> 1 file changed, 21 insertions(+) >>>> >>>> diff --git a/Documentation/devicetree/bindings/media/i2c/sony,imx274.yaml b/Documentation/devicetree/bindings/media/i2c/sony,imx274.yaml >>>> index 7ae47a6..57e7176 100644 >>>> --- a/Documentation/devicetree/bindings/media/i2c/sony,imx274.yaml >>>> +++ b/Documentation/devicetree/bindings/media/i2c/sony,imx274.yaml >>>> @@ -25,6 +25,27 @@ properties: >>>> reset-gpios: >>>> maxItems: 1 >>>> >>> I just sent an update to my json-schema conversion of this bindings >>> document (not yet on patchwork, sorry) and Sakari pointed me to the >>> fact in between my v2 and my v4 this patch from you went in: >>> 4ea3273d24b ("dt-bindings: media: imx274: Add optional input clock and supplies") >>> >>> I should probably now update my bindings conversion patch, basically >>> taking in what you've done here, but I would have one question. >>> >>>> + clocks: >>>> + maxItems: 1 >>>> + description: Reference to the sensor input clock >>>> + >>>> + clock-names: >>>> + maxItems: 1 >>>> + items: >>>> + - const: inck >>>> + >>>> + vana-supply: >>>> + description: >>>> + Analog voltage supply, 2.8 volts >>>> + >>>> + vdig-supply: >>>> + description: >>>> + Digital IO voltage supply, 1.8 volts >>>> + >>>> + vddl-supply: >>>> + description: >>>> + Digital core voltage supply, 1.2 volts >>> 4ea3273d24b introduced these regulators as VANA-supply, VDIG-supply >>> and VDDL-supply (please note the upper-case names). This version uses >>> lower-case ones instead. Is this intentional ? The driver currently >>> does not parse any of these if I'm not mistaken, but as the bindings >>> in textual form defines an ABI which should be preserved during the >>> conversion to json-schema, should these be kept in upper-case ? >>> >>> Thanks >>> j >> Yes, based on feedback lower case was recommended. So, changed to use >> lower-case names. >> >> These properties were not used by driver currently and from my prior series >> only dt-binding got merged as no feedback was received on it for all prior >> versions. >> >> So, should be ok to change to lower-case as there properties are introduced >> now and driver update using these properties is under review >> > Well, I see that patch went in v5.9-rc1, so it will be part of v5.9. > > If the bindings update goes in in v5.10 (or whatever comes after v5.9) > then we have a problem, as the DTB created for v5.9 won't work anymore > on any later version, and that should not happen. Alternatively, a fix > for the next -rc release could be fast-tracked, but you would > need to synchronize with the dt maintainers for that and make a patch > for the existing .txt bindings file. > > If the name change happens in the yaml file and one release is made > with the old names, then we're stuck with those forever and ever, if I > got the situation right. > > Please check with the dt and media maintainers, or they can comment > here if they glance through these lines. > > Thanks > j Hi Leon Luo, I used upper case for regulator supply names in all prior 4 versions of IMX274 patch series as I see some other media i2c drivers doing it and dt-binding patch from v3 got merged in 5.9-rc1 which was using upper-case. Later received feedback from Sakari requesting to use lower-case names so updated to use lower case name now in v5. Not sure if we have timeline to squeeze in patch to change names to lower-case before they get into 5.10. Can you please comment? Sakari, Can you also help understand why can't we keep upper case for regulator supplies? I see some other media i2c drivers using upper case as well. Thanks Sowjanya >>>> + >>>> port: >>>> type: object >>>> description: | >>>> -- >>>> 2.7.4 >>>> ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v5 2/3] dt-bindings: media: imx274: Add optional input clock and supplies 2020-09-03 16:40 ` Sowjanya Komatineni @ 2020-09-08 9:33 ` Sakari Ailus 2020-09-08 14:34 ` Jacopo Mondi 0 siblings, 1 reply; 18+ messages in thread From: Sakari Ailus @ 2020-09-08 9:33 UTC (permalink / raw) To: Sowjanya Komatineni Cc: Jacopo Mondi, thierry.reding, jonathanh, hverkuil, jacopo+renesas, luca, leonl, robh+dt, lgirdwood, broonie, linux-media, devicetree, linux-kernel On Thu, Sep 03, 2020 at 09:40:57AM -0700, Sowjanya Komatineni wrote: > > On 9/3/20 9:35 AM, Jacopo Mondi wrote: > > Hi Sowjanya, > > > > On Thu, Sep 03, 2020 at 09:05:27AM -0700, Sowjanya Komatineni wrote: > > > On 9/3/20 5:55 AM, Jacopo Mondi wrote: > > > > Hello Sowjanya, > > > > > > > > On Tue, Sep 01, 2020 at 07:04:37PM -0700, Sowjanya Komatineni wrote: > > > > > This patch adds IMX274 optional external clock input and voltage > > > > > supplies to device tree bindings. > > > > > > > > > > Reviewed-by: Luca Ceresoli <luca@lucaceresoli.net> > > > > > Signed-off-by: Sowjanya Komatineni <skomatineni@nvidia.com> > > > > > --- > > > > > .../devicetree/bindings/media/i2c/sony,imx274.yaml | 21 +++++++++++++++++++++ > > > > > 1 file changed, 21 insertions(+) > > > > > > > > > > diff --git a/Documentation/devicetree/bindings/media/i2c/sony,imx274.yaml b/Documentation/devicetree/bindings/media/i2c/sony,imx274.yaml > > > > > index 7ae47a6..57e7176 100644 > > > > > --- a/Documentation/devicetree/bindings/media/i2c/sony,imx274.yaml > > > > > +++ b/Documentation/devicetree/bindings/media/i2c/sony,imx274.yaml > > > > > @@ -25,6 +25,27 @@ properties: > > > > > reset-gpios: > > > > > maxItems: 1 > > > > > > > > > I just sent an update to my json-schema conversion of this bindings > > > > document (not yet on patchwork, sorry) and Sakari pointed me to the > > > > fact in between my v2 and my v4 this patch from you went in: > > > > 4ea3273d24b ("dt-bindings: media: imx274: Add optional input clock and supplies") > > > > > > > > I should probably now update my bindings conversion patch, basically > > > > taking in what you've done here, but I would have one question. > > > > > > > > > + clocks: > > > > > + maxItems: 1 > > > > > + description: Reference to the sensor input clock > > > > > + > > > > > + clock-names: > > > > > + maxItems: 1 > > > > > + items: > > > > > + - const: inck > > > > > + > > > > > + vana-supply: > > > > > + description: > > > > > + Analog voltage supply, 2.8 volts > > > > > + > > > > > + vdig-supply: > > > > > + description: > > > > > + Digital IO voltage supply, 1.8 volts > > > > > + > > > > > + vddl-supply: > > > > > + description: > > > > > + Digital core voltage supply, 1.2 volts > > > > 4ea3273d24b introduced these regulators as VANA-supply, VDIG-supply > > > > and VDDL-supply (please note the upper-case names). This version uses > > > > lower-case ones instead. Is this intentional ? The driver currently > > > > does not parse any of these if I'm not mistaken, but as the bindings > > > > in textual form defines an ABI which should be preserved during the > > > > conversion to json-schema, should these be kept in upper-case ? > > > > > > > > Thanks > > > > j > > > Yes, based on feedback lower case was recommended. So, changed to use > > > lower-case names. > > > > > > These properties were not used by driver currently and from my prior series > > > only dt-binding got merged as no feedback was received on it for all prior > > > versions. > > > > > > So, should be ok to change to lower-case as there properties are introduced > > > now and driver update using these properties is under review > > > > > Well, I see that patch went in v5.9-rc1, so it will be part of v5.9. > > > > If the bindings update goes in in v5.10 (or whatever comes after v5.9) > > then we have a problem, as the DTB created for v5.9 won't work anymore > > on any later version, and that should not happen. Alternatively, a fix > > for the next -rc release could be fast-tracked, but you would > > need to synchronize with the dt maintainers for that and make a patch > > for the existing .txt bindings file. > > > > If the name change happens in the yaml file and one release is made > > with the old names, then we're stuck with those forever and ever, if I > > got the situation right. > > > > Please check with the dt and media maintainers, or they can comment > > here if they glance through these lines. > > > > Thanks > > j > > Hi Leon Luo, > > I used upper case for regulator supply names in all prior 4 versions of > IMX274 patch series as I see some other media i2c drivers doing it and > dt-binding patch from v3 got merged in 5.9-rc1 which was using upper-case. > > Later received feedback from Sakari requesting to use lower-case names so > updated to use lower case name now in v5. > > Not sure if we have timeline to squeeze in patch to change names to > lower-case before they get into 5.10. > > Can you please comment? We can merge patches through the fixes branch if needed. That is not an issue. > > Sakari, > > Can you also help understand why can't we keep upper case for regulator > supplies? > > I see some other media i2c drivers using upper case as well. The vast majority of bindings use lower case, that's it, simply. -- Regards, Sakari Ailus ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v5 2/3] dt-bindings: media: imx274: Add optional input clock and supplies 2020-09-08 9:33 ` Sakari Ailus @ 2020-09-08 14:34 ` Jacopo Mondi 2020-09-08 18:10 ` Sowjanya Komatineni 0 siblings, 1 reply; 18+ messages in thread From: Jacopo Mondi @ 2020-09-08 14:34 UTC (permalink / raw) To: Sakari Ailus Cc: Sowjanya Komatineni, thierry.reding, jonathanh, hverkuil, jacopo+renesas, luca, leonl, robh+dt, lgirdwood, broonie, linux-media, devicetree, linux-kernel Hi Sakari, Sowjanya, On Tue, Sep 08, 2020 at 12:33:41PM +0300, Sakari Ailus wrote: > On Thu, Sep 03, 2020 at 09:40:57AM -0700, Sowjanya Komatineni wrote: > > > > On 9/3/20 9:35 AM, Jacopo Mondi wrote: > > > Hi Sowjanya, > > > > > > On Thu, Sep 03, 2020 at 09:05:27AM -0700, Sowjanya Komatineni wrote: > > > > On 9/3/20 5:55 AM, Jacopo Mondi wrote: > > > > > Hello Sowjanya, > > > > > > > > > > On Tue, Sep 01, 2020 at 07:04:37PM -0700, Sowjanya Komatineni wrote: > > > > > > This patch adds IMX274 optional external clock input and voltage > > > > > > supplies to device tree bindings. > > > > > > > > > > > > Reviewed-by: Luca Ceresoli <luca@lucaceresoli.net> > > > > > > Signed-off-by: Sowjanya Komatineni <skomatineni@nvidia.com> > > > > > > --- > > > > > > .../devicetree/bindings/media/i2c/sony,imx274.yaml | 21 +++++++++++++++++++++ > > > > > > 1 file changed, 21 insertions(+) > > > > > > > > > > > > diff --git a/Documentation/devicetree/bindings/media/i2c/sony,imx274.yaml b/Documentation/devicetree/bindings/media/i2c/sony,imx274.yaml > > > > > > index 7ae47a6..57e7176 100644 > > > > > > --- a/Documentation/devicetree/bindings/media/i2c/sony,imx274.yaml > > > > > > +++ b/Documentation/devicetree/bindings/media/i2c/sony,imx274.yaml > > > > > > @@ -25,6 +25,27 @@ properties: > > > > > > reset-gpios: > > > > > > maxItems: 1 > > > > > > > > > > > I just sent an update to my json-schema conversion of this bindings > > > > > document (not yet on patchwork, sorry) and Sakari pointed me to the > > > > > fact in between my v2 and my v4 this patch from you went in: > > > > > 4ea3273d24b ("dt-bindings: media: imx274: Add optional input clock and supplies") > > > > > > > > > > I should probably now update my bindings conversion patch, basically > > > > > taking in what you've done here, but I would have one question. > > > > > > > > > > > + clocks: > > > > > > + maxItems: 1 > > > > > > + description: Reference to the sensor input clock > > > > > > + > > > > > > + clock-names: > > > > > > + maxItems: 1 > > > > > > + items: > > > > > > + - const: inck > > > > > > + > > > > > > + vana-supply: > > > > > > + description: > > > > > > + Analog voltage supply, 2.8 volts > > > > > > + > > > > > > + vdig-supply: > > > > > > + description: > > > > > > + Digital IO voltage supply, 1.8 volts > > > > > > + > > > > > > + vddl-supply: > > > > > > + description: > > > > > > + Digital core voltage supply, 1.2 volts > > > > > 4ea3273d24b introduced these regulators as VANA-supply, VDIG-supply > > > > > and VDDL-supply (please note the upper-case names). This version uses > > > > > lower-case ones instead. Is this intentional ? The driver currently > > > > > does not parse any of these if I'm not mistaken, but as the bindings > > > > > in textual form defines an ABI which should be preserved during the > > > > > conversion to json-schema, should these be kept in upper-case ? > > > > > > > > > > Thanks > > > > > j > > > > Yes, based on feedback lower case was recommended. So, changed to use > > > > lower-case names. > > > > > > > > These properties were not used by driver currently and from my prior series > > > > only dt-binding got merged as no feedback was received on it for all prior > > > > versions. > > > > > > > > So, should be ok to change to lower-case as there properties are introduced > > > > now and driver update using these properties is under review > > > > > > > Well, I see that patch went in v5.9-rc1, so it will be part of v5.9. > > > > > > If the bindings update goes in in v5.10 (or whatever comes after v5.9) > > > then we have a problem, as the DTB created for v5.9 won't work anymore > > > on any later version, and that should not happen. Alternatively, a fix > > > for the next -rc release could be fast-tracked, but you would > > > need to synchronize with the dt maintainers for that and make a patch > > > for the existing .txt bindings file. > > > > > > If the name change happens in the yaml file and one release is made > > > with the old names, then we're stuck with those forever and ever, if I > > > got the situation right. > > > > > > Please check with the dt and media maintainers, or they can comment > > > here if they glance through these lines. > > > > > > Thanks > > > j > > > > Hi Leon Luo, > > > > I used upper case for regulator supply names in all prior 4 versions of > > IMX274 patch series as I see some other media i2c drivers doing it and > > dt-binding patch from v3 got merged in 5.9-rc1 which was using upper-case. > > > > Later received feedback from Sakari requesting to use lower-case names so > > updated to use lower case name now in v5. > > > > Not sure if we have timeline to squeeze in patch to change names to > > lower-case before they get into 5.10. > > > > Can you please comment? > > We can merge patches through the fixes branch if needed. That is not an > issue. > Good! So I'll make a v5 of the json-schema bindings soon that includes the lower-case supplies and clock names and let's merge it as a fix in this release cycle. Sowjanya is this ok with you ? Sakari, I'll then trust you to fast-track the patch if no other issues! Thanks j > > > > Sakari, > > > > Can you also help understand why can't we keep upper case for regulator > > supplies? > > > > I see some other media i2c drivers using upper case as well. > > The vast majority of bindings use lower case, that's it, simply. > > -- > Regards, > > Sakari Ailus ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v5 2/3] dt-bindings: media: imx274: Add optional input clock and supplies 2020-09-08 14:34 ` Jacopo Mondi @ 2020-09-08 18:10 ` Sowjanya Komatineni 0 siblings, 0 replies; 18+ messages in thread From: Sowjanya Komatineni @ 2020-09-08 18:10 UTC (permalink / raw) To: Jacopo Mondi, Sakari Ailus Cc: thierry.reding, jonathanh, hverkuil, jacopo+renesas, luca, leonl, robh+dt, lgirdwood, broonie, linux-media, devicetree, linux-kernel On 9/8/20 7:34 AM, Jacopo Mondi wrote: > Hi Sakari, Sowjanya, > > On Tue, Sep 08, 2020 at 12:33:41PM +0300, Sakari Ailus wrote: >> On Thu, Sep 03, 2020 at 09:40:57AM -0700, Sowjanya Komatineni wrote: >>> On 9/3/20 9:35 AM, Jacopo Mondi wrote: >>>> Hi Sowjanya, >>>> >>>> On Thu, Sep 03, 2020 at 09:05:27AM -0700, Sowjanya Komatineni wrote: >>>>> On 9/3/20 5:55 AM, Jacopo Mondi wrote: >>>>>> Hello Sowjanya, >>>>>> >>>>>> On Tue, Sep 01, 2020 at 07:04:37PM -0700, Sowjanya Komatineni wrote: >>>>>>> This patch adds IMX274 optional external clock input and voltage >>>>>>> supplies to device tree bindings. >>>>>>> >>>>>>> Reviewed-by: Luca Ceresoli <luca@lucaceresoli.net> >>>>>>> Signed-off-by: Sowjanya Komatineni <skomatineni@nvidia.com> >>>>>>> --- >>>>>>> .../devicetree/bindings/media/i2c/sony,imx274.yaml | 21 +++++++++++++++++++++ >>>>>>> 1 file changed, 21 insertions(+) >>>>>>> >>>>>>> diff --git a/Documentation/devicetree/bindings/media/i2c/sony,imx274.yaml b/Documentation/devicetree/bindings/media/i2c/sony,imx274.yaml >>>>>>> index 7ae47a6..57e7176 100644 >>>>>>> --- a/Documentation/devicetree/bindings/media/i2c/sony,imx274.yaml >>>>>>> +++ b/Documentation/devicetree/bindings/media/i2c/sony,imx274.yaml >>>>>>> @@ -25,6 +25,27 @@ properties: >>>>>>> reset-gpios: >>>>>>> maxItems: 1 >>>>>>> >>>>>> I just sent an update to my json-schema conversion of this bindings >>>>>> document (not yet on patchwork, sorry) and Sakari pointed me to the >>>>>> fact in between my v2 and my v4 this patch from you went in: >>>>>> 4ea3273d24b ("dt-bindings: media: imx274: Add optional input clock and supplies") >>>>>> >>>>>> I should probably now update my bindings conversion patch, basically >>>>>> taking in what you've done here, but I would have one question. >>>>>> >>>>>>> + clocks: >>>>>>> + maxItems: 1 >>>>>>> + description: Reference to the sensor input clock >>>>>>> + >>>>>>> + clock-names: >>>>>>> + maxItems: 1 >>>>>>> + items: >>>>>>> + - const: inck >>>>>>> + >>>>>>> + vana-supply: >>>>>>> + description: >>>>>>> + Analog voltage supply, 2.8 volts >>>>>>> + >>>>>>> + vdig-supply: >>>>>>> + description: >>>>>>> + Digital IO voltage supply, 1.8 volts >>>>>>> + >>>>>>> + vddl-supply: >>>>>>> + description: >>>>>>> + Digital core voltage supply, 1.2 volts >>>>>> 4ea3273d24b introduced these regulators as VANA-supply, VDIG-supply >>>>>> and VDDL-supply (please note the upper-case names). This version uses >>>>>> lower-case ones instead. Is this intentional ? The driver currently >>>>>> does not parse any of these if I'm not mistaken, but as the bindings >>>>>> in textual form defines an ABI which should be preserved during the >>>>>> conversion to json-schema, should these be kept in upper-case ? >>>>>> >>>>>> Thanks >>>>>> j >>>>> Yes, based on feedback lower case was recommended. So, changed to use >>>>> lower-case names. >>>>> >>>>> These properties were not used by driver currently and from my prior series >>>>> only dt-binding got merged as no feedback was received on it for all prior >>>>> versions. >>>>> >>>>> So, should be ok to change to lower-case as there properties are introduced >>>>> now and driver update using these properties is under review >>>>> >>>> Well, I see that patch went in v5.9-rc1, so it will be part of v5.9. >>>> >>>> If the bindings update goes in in v5.10 (or whatever comes after v5.9) >>>> then we have a problem, as the DTB created for v5.9 won't work anymore >>>> on any later version, and that should not happen. Alternatively, a fix >>>> for the next -rc release could be fast-tracked, but you would >>>> need to synchronize with the dt maintainers for that and make a patch >>>> for the existing .txt bindings file. >>>> >>>> If the name change happens in the yaml file and one release is made >>>> with the old names, then we're stuck with those forever and ever, if I >>>> got the situation right. >>>> >>>> Please check with the dt and media maintainers, or they can comment >>>> here if they glance through these lines. >>>> >>>> Thanks >>>> j >>> Hi Leon Luo, >>> >>> I used upper case for regulator supply names in all prior 4 versions of >>> IMX274 patch series as I see some other media i2c drivers doing it and >>> dt-binding patch from v3 got merged in 5.9-rc1 which was using upper-case. >>> >>> Later received feedback from Sakari requesting to use lower-case names so >>> updated to use lower case name now in v5. >>> >>> Not sure if we have timeline to squeeze in patch to change names to >>> lower-case before they get into 5.10. >>> >>> Can you please comment? >> We can merge patches through the fixes branch if needed. That is not an >> issue. >> > Good! So I'll make a v5 of the json-schema bindings soon that includes > the lower-case supplies and clock names and let's merge it as a fix in > this release cycle. > > Sowjanya is this ok with you ? Yes fine for me. > Sakari, I'll then trust you to fast-track the patch if no other > issues! > > Thanks > j > >>> Sakari, >>> >>> Can you also help understand why can't we keep upper case for regulator >>> supplies? >>> >>> I see some other media i2c drivers using upper case as well. >> The vast majority of bindings use lower case, that's it, simply. >> >> -- >> Regards, >> >> Sakari Ailus ^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH v5 3/3] media: i2c: imx274: Add IMX274 power on and off sequence 2020-09-02 2:04 [PATCH v5 0/3] IMX274 fixes and power on and off implementation Sowjanya Komatineni 2020-09-02 2:04 ` [PATCH v5 1/3] media: i2c: imx274: Fix Y_OUT_SIZE register setting Sowjanya Komatineni 2020-09-02 2:04 ` [PATCH v5 2/3] dt-bindings: media: imx274: Add optional input clock and supplies Sowjanya Komatineni @ 2020-09-02 2:04 ` Sowjanya Komatineni 2020-09-03 15:03 ` Jacopo Mondi 2 siblings, 1 reply; 18+ messages in thread From: Sowjanya Komatineni @ 2020-09-02 2:04 UTC (permalink / raw) To: skomatineni, thierry.reding, jonathanh, sakari.ailus, hverkuil, jacopo+renesas, luca, leonl, robh+dt, lgirdwood, broonie Cc: linux-media, devicetree, linux-kernel IMX274 has VANA analog 2.8V supply, VDIG digital core 1.8V supply, and VDDL digital io 1.2V supply which are optional based on camera module design. IMX274 also need external 24Mhz clock and is optional based on camera module design. This patch adds support for IMX274 power on and off to enable and disable these supplies and external clock. Reviewed-by: Luca Ceresoli <luca@lucaceresoli.net> Signed-off-by: Sowjanya Komatineni <skomatineni@nvidia.com> --- drivers/media/i2c/imx274.c | 134 ++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 131 insertions(+), 3 deletions(-) diff --git a/drivers/media/i2c/imx274.c b/drivers/media/i2c/imx274.c index a4b9dfd..79bfac3c6 100644 --- a/drivers/media/i2c/imx274.c +++ b/drivers/media/i2c/imx274.c @@ -18,7 +18,9 @@ #include <linux/kernel.h> #include <linux/module.h> #include <linux/of_gpio.h> +#include <linux/pm_runtime.h> #include <linux/regmap.h> +#include <linux/regulator/consumer.h> #include <linux/slab.h> #include <linux/v4l2-mediabus.h> #include <linux/videodev2.h> @@ -131,6 +133,15 @@ #define IMX274_TABLE_WAIT_MS 0 #define IMX274_TABLE_END 1 +/* regulator supplies */ +static const char * const imx274_supply_names[] = { + "vddl", /* IF (1.2V) supply */ + "vdig", /* Digital Core (1.8V) supply */ + "vana", /* Analog (2.8V) supply */ +}; + +#define IMX274_NUM_SUPPLIES ARRAY_SIZE(imx274_supply_names) + /* * imx274 I2C operation related structure */ @@ -501,6 +512,8 @@ struct imx274_ctrls { * @frame_rate: V4L2 frame rate structure * @regmap: Pointer to regmap structure * @reset_gpio: Pointer to reset gpio + * @supplies: List of analog and digital supply regulators + * @inck: Pointer to sensor input clock * @lock: Mutex structure * @mode: Parameters for the selected readout mode */ @@ -514,6 +527,8 @@ struct stimx274 { struct v4l2_fract frame_interval; struct regmap *regmap; struct gpio_desc *reset_gpio; + struct regulator_bulk_data supplies[IMX274_NUM_SUPPLIES]; + struct clk *inck; struct mutex lock; /* mutex lock for operations */ const struct imx274_mode *mode; }; @@ -767,6 +782,75 @@ static void imx274_reset(struct stimx274 *priv, int rst) usleep_range(IMX274_RESET_DELAY1, IMX274_RESET_DELAY2); } +/* + * imx274_power_on - Function called to power on the sensor + * @imx274: Pointer to device structure + */ +static int imx274_power_on(struct device *dev) +{ + struct i2c_client *client = to_i2c_client(dev); + struct v4l2_subdev *sd = i2c_get_clientdata(client); + struct stimx274 *imx274 = to_imx274(sd); + int ret; + + /* keep sensor in reset before power on */ + imx274_reset(imx274, 0); + + ret = clk_prepare_enable(imx274->inck); + if (ret) { + dev_err(&imx274->client->dev, + "Failed to enable input clock: %d\n", ret); + return ret; + } + + ret = regulator_bulk_enable(IMX274_NUM_SUPPLIES, imx274->supplies); + if (ret) { + dev_err(&imx274->client->dev, + "Failed to enable regulators: %d\n", ret); + goto fail_reg; + } + + usleep_range(1, 2); + imx274_reset(imx274, 1); + + return 0; + +fail_reg: + regulator_bulk_disable(IMX274_NUM_SUPPLIES, imx274->supplies); + clk_disable_unprepare(imx274->inck); + return ret; +} + +/* + * imx274_power_off - Function called to power off the sensor + * @imx274: Pointer to device structure + */ +static int imx274_power_off(struct device *dev) +{ + struct i2c_client *client = to_i2c_client(dev); + struct v4l2_subdev *sd = i2c_get_clientdata(client); + struct stimx274 *imx274 = to_imx274(sd); + + imx274_reset(imx274, 0); + + regulator_bulk_disable(IMX274_NUM_SUPPLIES, imx274->supplies); + + clk_disable_unprepare(imx274->inck); + + return 0; +} + +static int imx274_get_regulators(struct device *dev, struct stimx274 *imx274) +{ + unsigned int i; + + for (i = 0; i < IMX274_NUM_SUPPLIES; i++) + imx274->supplies[i].supply = imx274_supply_names[i]; + + return devm_regulator_bulk_get(dev, IMX274_NUM_SUPPLIES, + imx274->supplies); +} + /** * imx274_s_ctrl - This is used to set the imx274 V4L2 controls * @ctrl: V4L2 control to be set @@ -781,6 +865,9 @@ static int imx274_s_ctrl(struct v4l2_ctrl *ctrl) struct stimx274 *imx274 = to_imx274(sd); int ret = -EINVAL; + if (!pm_runtime_get_if_in_use(&imx274->client->dev)) + return 0; + dev_dbg(&imx274->client->dev, "%s : s_ctrl: %s, value: %d\n", __func__, ctrl->name, ctrl->val); @@ -811,6 +898,8 @@ static int imx274_s_ctrl(struct v4l2_ctrl *ctrl) break; } + pm_runtime_put(&imx274->client->dev); + return ret; } @@ -1327,6 +1416,13 @@ static int imx274_s_stream(struct v4l2_subdev *sd, int on) mutex_lock(&imx274->lock); if (on) { + ret = pm_runtime_get_sync(&imx274->client->dev); + if (ret < 0) { + pm_runtime_put_noidle(&imx274->client->dev); + mutex_unlock(&imx274->lock); + return ret; + } + /* load mode registers */ ret = imx274_mode_regs(imx274); if (ret) @@ -1362,6 +1458,7 @@ static int imx274_s_stream(struct v4l2_subdev *sd, int on) ret = imx274_write_table(imx274, imx274_stop); if (ret) goto fail; + pm_runtime_put(&imx274->client->dev); } mutex_unlock(&imx274->lock); @@ -1369,6 +1466,7 @@ static int imx274_s_stream(struct v4l2_subdev *sd, int on) return 0; fail: + pm_runtime_put(&imx274->client->dev); mutex_unlock(&imx274->lock); dev_err(&imx274->client->dev, "s_stream failed\n"); return ret; @@ -1834,6 +1932,14 @@ static int imx274_probe(struct i2c_client *client) mutex_init(&imx274->lock); + imx274->inck = devm_clk_get_optional(&client->dev, "inck"); + ret = imx274_get_regulators(&client->dev, imx274); + if (ret) { + dev_err(&client->dev, + "Failed to get power regulators, err: %d\n", ret); + return ret; + } + /* initialize format */ imx274->mode = &imx274_modes[IMX274_DEFAULT_BINNING]; imx274->crop.width = IMX274_MAX_WIDTH; @@ -1881,15 +1987,23 @@ static int imx274_probe(struct i2c_client *client) goto err_me; } - /* pull sensor out of reset */ - imx274_reset(imx274, 1); + /* power on the sensor */ + ret = imx274_power_on(&client->dev); + if (ret < 0) { + dev_err(&client->dev, + "%s : imx274 power on failed\n", __func__); + goto err_me; + } + + pm_runtime_set_active(&client->dev); + pm_runtime_enable(&client->dev); /* initialize controls */ ret = v4l2_ctrl_handler_init(&imx274->ctrls.handler, 4); if (ret < 0) { dev_err(&client->dev, "%s : ctrl handler init Failed\n", __func__); - goto err_me; + goto err_disable_rpm; } imx274->ctrls.handler.lock = &imx274->lock; @@ -1951,11 +2065,16 @@ static int imx274_probe(struct i2c_client *client) goto err_ctrls; } + pm_runtime_idle(&client->dev); + dev_info(&client->dev, "imx274 : imx274 probe success !\n"); return 0; err_ctrls: v4l2_ctrl_handler_free(&imx274->ctrls.handler); +err_disable_rpm: + pm_runtime_disable(&client->dev); + pm_runtime_set_suspended(&client->dev); err_me: media_entity_cleanup(&sd->entity); err_regmap: @@ -1973,14 +2092,23 @@ static int imx274_remove(struct i2c_client *client) v4l2_async_unregister_subdev(sd); v4l2_ctrl_handler_free(&imx274->ctrls.handler); + + pm_runtime_disable(&client->dev); + pm_runtime_set_suspended(&client->dev); + media_entity_cleanup(&sd->entity); mutex_destroy(&imx274->lock); return 0; } +static const struct dev_pm_ops imx274_pm_ops = { + SET_RUNTIME_PM_OPS(imx274_power_off, imx274_power_on, NULL) +}; + static struct i2c_driver imx274_i2c_driver = { .driver = { .name = DRIVER_NAME, + .pm = &imx274_pm_ops, .of_match_table = imx274_of_id_table, }, .probe_new = imx274_probe, -- 2.7.4 ^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH v5 3/3] media: i2c: imx274: Add IMX274 power on and off sequence 2020-09-02 2:04 ` [PATCH v5 3/3] media: i2c: imx274: Add IMX274 power on and off sequence Sowjanya Komatineni @ 2020-09-03 15:03 ` Jacopo Mondi 2020-09-03 16:25 ` Sowjanya Komatineni 0 siblings, 1 reply; 18+ messages in thread From: Jacopo Mondi @ 2020-09-03 15:03 UTC (permalink / raw) To: Sowjanya Komatineni Cc: thierry.reding, jonathanh, sakari.ailus, hverkuil, jacopo+renesas, luca, leonl, robh+dt, lgirdwood, broonie, linux-media, devicetree, linux-kernel Hello, On Tue, Sep 01, 2020 at 07:04:38PM -0700, Sowjanya Komatineni wrote: > IMX274 has VANA analog 2.8V supply, VDIG digital core 1.8V supply, > and VDDL digital io 1.2V supply which are optional based on camera > module design. > > IMX274 also need external 24Mhz clock and is optional based on > camera module design. > > This patch adds support for IMX274 power on and off to enable and > disable these supplies and external clock. > > Reviewed-by: Luca Ceresoli <luca@lucaceresoli.net> > Signed-off-by: Sowjanya Komatineni <skomatineni@nvidia.com> > --- > drivers/media/i2c/imx274.c | 134 ++++++++++++++++++++++++++++++++++++++++++++- > 1 file changed, 131 insertions(+), 3 deletions(-) > > diff --git a/drivers/media/i2c/imx274.c b/drivers/media/i2c/imx274.c > index a4b9dfd..79bfac3c6 100644 > --- a/drivers/media/i2c/imx274.c > +++ b/drivers/media/i2c/imx274.c > @@ -18,7 +18,9 @@ > #include <linux/kernel.h> > #include <linux/module.h> > #include <linux/of_gpio.h> > +#include <linux/pm_runtime.h> > #include <linux/regmap.h> > +#include <linux/regulator/consumer.h> > #include <linux/slab.h> > #include <linux/v4l2-mediabus.h> > #include <linux/videodev2.h> > @@ -131,6 +133,15 @@ > #define IMX274_TABLE_WAIT_MS 0 > #define IMX274_TABLE_END 1 > > +/* regulator supplies */ > +static const char * const imx274_supply_names[] = { > + "vddl", /* IF (1.2V) supply */ > + "vdig", /* Digital Core (1.8V) supply */ > + "vana", /* Analog (2.8V) supply */ > +}; > + > +#define IMX274_NUM_SUPPLIES ARRAY_SIZE(imx274_supply_names) > + > /* > * imx274 I2C operation related structure > */ > @@ -501,6 +512,8 @@ struct imx274_ctrls { > * @frame_rate: V4L2 frame rate structure > * @regmap: Pointer to regmap structure > * @reset_gpio: Pointer to reset gpio > + * @supplies: List of analog and digital supply regulators > + * @inck: Pointer to sensor input clock > * @lock: Mutex structure > * @mode: Parameters for the selected readout mode > */ > @@ -514,6 +527,8 @@ struct stimx274 { > struct v4l2_fract frame_interval; > struct regmap *regmap; > struct gpio_desc *reset_gpio; > + struct regulator_bulk_data supplies[IMX274_NUM_SUPPLIES]; > + struct clk *inck; > struct mutex lock; /* mutex lock for operations */ > const struct imx274_mode *mode; > }; > @@ -767,6 +782,75 @@ static void imx274_reset(struct stimx274 *priv, int rst) > usleep_range(IMX274_RESET_DELAY1, IMX274_RESET_DELAY2); > } > > +/* > + * imx274_power_on - Function called to power on the sensor > + * @imx274: Pointer to device structure > + */ Can I say this does not bring much value ? :) Also the parameter name is wrong > +static int imx274_power_on(struct device *dev) > +{ > + struct i2c_client *client = to_i2c_client(dev); > + struct v4l2_subdev *sd = i2c_get_clientdata(client); > + struct stimx274 *imx274 = to_imx274(sd); > + int ret; > + > + /* keep sensor in reset before power on */ > + imx274_reset(imx274, 0); > + > + ret = clk_prepare_enable(imx274->inck); > + if (ret) { > + dev_err(&imx274->client->dev, > + "Failed to enable input clock: %d\n", ret); > + return ret; > + } > + > + ret = regulator_bulk_enable(IMX274_NUM_SUPPLIES, imx274->supplies); > + if (ret) { > + dev_err(&imx274->client->dev, > + "Failed to enable regulators: %d\n", ret); > + goto fail_reg; > + } > + > + usleep_range(1, 2); usleep_range() allows you to provide an interval in which your timeout can be coalesced with others. Giving a [1usec, 2usec] range kind of defeat the purpose. And most than everything, does sleeping for 2usec serve any real purpose ? > + imx274_reset(imx274, 1); > + > + return 0; > + > +fail_reg: > + regulator_bulk_disable(IMX274_NUM_SUPPLIES, imx274->supplies); regulator_bulk_enable() disables all the regulators that were enabled before the one that failed, so I don't think you need this one here > + clk_disable_unprepare(imx274->inck); > + return ret; > +} > + > +/* > + * imx274_power_off - Function called to power off the sensor > + * @imx274: Pointer to device structure > + */ Same as the above one > +static int imx274_power_off(struct device *dev) > +{ > + struct i2c_client *client = to_i2c_client(dev); > + struct v4l2_subdev *sd = i2c_get_clientdata(client); > + struct stimx274 *imx274 = to_imx274(sd); > + > + imx274_reset(imx274, 0); > + Is reset before power-off necessary ? > + regulator_bulk_disable(IMX274_NUM_SUPPLIES, imx274->supplies); > + > + clk_disable_unprepare(imx274->inck); > + > + return 0; > +} > + > +static int imx274_get_regulators(struct device *dev, struct stimx274 *imx274) For symmetry with the regulators API I would call this imx274_regulators_get(). Up to you :) > +{ > + unsigned int i; > + > + for (i = 0; i < IMX274_NUM_SUPPLIES; i++) > + imx274->supplies[i].supply = imx274_supply_names[i]; > + > + return devm_regulator_bulk_get(dev, IMX274_NUM_SUPPLIES, > + imx274->supplies); ^ not sure if it's my email client but you might have a wrong indent here Also, the regulators are optional in the bindings, how do the regulators API cope with that ? I had a look around and they seems to assume regulators are provided. I might be mistaken though > +} > + > /** > * imx274_s_ctrl - This is used to set the imx274 V4L2 controls > * @ctrl: V4L2 control to be set > @@ -781,6 +865,9 @@ static int imx274_s_ctrl(struct v4l2_ctrl *ctrl) > struct stimx274 *imx274 = to_imx274(sd); > int ret = -EINVAL; > > + if (!pm_runtime_get_if_in_use(&imx274->client->dev)) > + return 0; > + Right, but then you should call __v4l2_ctrl_handler_setup() in the s_stream(1) call path to have controls updated (after pm_runtime_get_sync() call for power on). I had a look at it seems only exposure is updated. > dev_dbg(&imx274->client->dev, > "%s : s_ctrl: %s, value: %d\n", __func__, > ctrl->name, ctrl->val); > @@ -811,6 +898,8 @@ static int imx274_s_ctrl(struct v4l2_ctrl *ctrl) > break; > } > > + pm_runtime_put(&imx274->client->dev); > + > return ret; > } > > @@ -1327,6 +1416,13 @@ static int imx274_s_stream(struct v4l2_subdev *sd, int on) > mutex_lock(&imx274->lock); > > if (on) { > + ret = pm_runtime_get_sync(&imx274->client->dev); > + if (ret < 0) { > + pm_runtime_put_noidle(&imx274->client->dev); > + mutex_unlock(&imx274->lock); > + return ret; > + } > + > /* load mode registers */ > ret = imx274_mode_regs(imx274); > if (ret) > @@ -1362,6 +1458,7 @@ static int imx274_s_stream(struct v4l2_subdev *sd, int on) > ret = imx274_write_table(imx274, imx274_stop); > if (ret) > goto fail; > + pm_runtime_put(&imx274->client->dev); > } > > mutex_unlock(&imx274->lock); > @@ -1369,6 +1466,7 @@ static int imx274_s_stream(struct v4l2_subdev *sd, int on) > return 0; > > fail: > + pm_runtime_put(&imx274->client->dev); > mutex_unlock(&imx274->lock); > dev_err(&imx274->client->dev, "s_stream failed\n"); > return ret; > @@ -1834,6 +1932,14 @@ static int imx274_probe(struct i2c_client *client) > > mutex_init(&imx274->lock); > > + imx274->inck = devm_clk_get_optional(&client->dev, "inck"); clk_get_optional() might return error. I would check this with IS_ERR > + ret = imx274_get_regulators(&client->dev, imx274); > + if (ret) { > + dev_err(&client->dev, > + "Failed to get power regulators, err: %d\n", ret); > + return ret; > + } > + > /* initialize format */ > imx274->mode = &imx274_modes[IMX274_DEFAULT_BINNING]; > imx274->crop.width = IMX274_MAX_WIDTH; > @@ -1881,15 +1987,23 @@ static int imx274_probe(struct i2c_client *client) > goto err_me; > } > > - /* pull sensor out of reset */ > - imx274_reset(imx274, 1); > + /* power on the sensor */ > + ret = imx274_power_on(&client->dev); > + if (ret < 0) { > + dev_err(&client->dev, > + "%s : imx274 power on failed\n", __func__); > + goto err_me; > + } Doesn't pm_runtime_get calls the poweron function for you ? But anyway, I don't see the device being probed for, in example, querying it's VID/PID for identification during the driver's probe routine. Do you need to power on ? > + > + pm_runtime_set_active(&client->dev); > + pm_runtime_enable(&client->dev); > > /* initialize controls */ > ret = v4l2_ctrl_handler_init(&imx274->ctrls.handler, 4); > if (ret < 0) { > dev_err(&client->dev, > "%s : ctrl handler init Failed\n", __func__); > - goto err_me; > + goto err_disable_rpm; > } > > imx274->ctrls.handler.lock = &imx274->lock; > @@ -1951,11 +2065,16 @@ static int imx274_probe(struct i2c_client *client) > goto err_ctrls; > } > > + pm_runtime_idle(&client->dev); > + > dev_info(&client->dev, "imx274 : imx274 probe success !\n"); > return 0; > > err_ctrls: > v4l2_ctrl_handler_free(&imx274->ctrls.handler); > +err_disable_rpm: > + pm_runtime_disable(&client->dev); > + pm_runtime_set_suspended(&client->dev); > err_me: > media_entity_cleanup(&sd->entity); > err_regmap: > @@ -1973,14 +2092,23 @@ static int imx274_remove(struct i2c_client *client) > > v4l2_async_unregister_subdev(sd); > v4l2_ctrl_handler_free(&imx274->ctrls.handler); > + > + pm_runtime_disable(&client->dev); > + pm_runtime_set_suspended(&client->dev); > + > media_entity_cleanup(&sd->entity); > mutex_destroy(&imx274->lock); > return 0; > } > > +static const struct dev_pm_ops imx274_pm_ops = { > + SET_RUNTIME_PM_OPS(imx274_power_off, imx274_power_on, NULL) > +}; > + > static struct i2c_driver imx274_i2c_driver = { > .driver = { > .name = DRIVER_NAME, > + .pm = &imx274_pm_ops, > .of_match_table = imx274_of_id_table, > }, > .probe_new = imx274_probe, > -- > 2.7.4 > ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v5 3/3] media: i2c: imx274: Add IMX274 power on and off sequence 2020-09-03 15:03 ` Jacopo Mondi @ 2020-09-03 16:25 ` Sowjanya Komatineni 2020-09-04 8:55 ` Jacopo Mondi 0 siblings, 1 reply; 18+ messages in thread From: Sowjanya Komatineni @ 2020-09-03 16:25 UTC (permalink / raw) To: Jacopo Mondi Cc: thierry.reding, jonathanh, sakari.ailus, hverkuil, jacopo+renesas, luca, leonl, robh+dt, lgirdwood, broonie, linux-media, devicetree, linux-kernel On 9/3/20 8:03 AM, Jacopo Mondi wrote: > Hello, > > On Tue, Sep 01, 2020 at 07:04:38PM -0700, Sowjanya Komatineni wrote: >> IMX274 has VANA analog 2.8V supply, VDIG digital core 1.8V supply, >> and VDDL digital io 1.2V supply which are optional based on camera >> module design. >> >> IMX274 also need external 24Mhz clock and is optional based on >> camera module design. >> >> This patch adds support for IMX274 power on and off to enable and >> disable these supplies and external clock. >> >> Reviewed-by: Luca Ceresoli <luca@lucaceresoli.net> >> Signed-off-by: Sowjanya Komatineni <skomatineni@nvidia.com> >> --- >> drivers/media/i2c/imx274.c | 134 ++++++++++++++++++++++++++++++++++++++++++++- >> 1 file changed, 131 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/media/i2c/imx274.c b/drivers/media/i2c/imx274.c >> index a4b9dfd..79bfac3c6 100644 >> --- a/drivers/media/i2c/imx274.c >> +++ b/drivers/media/i2c/imx274.c >> @@ -18,7 +18,9 @@ >> #include <linux/kernel.h> >> #include <linux/module.h> >> #include <linux/of_gpio.h> >> +#include <linux/pm_runtime.h> >> #include <linux/regmap.h> >> +#include <linux/regulator/consumer.h> >> #include <linux/slab.h> >> #include <linux/v4l2-mediabus.h> >> #include <linux/videodev2.h> >> @@ -131,6 +133,15 @@ >> #define IMX274_TABLE_WAIT_MS 0 >> #define IMX274_TABLE_END 1 >> >> +/* regulator supplies */ >> +static const char * const imx274_supply_names[] = { >> + "vddl", /* IF (1.2V) supply */ >> + "vdig", /* Digital Core (1.8V) supply */ >> + "vana", /* Analog (2.8V) supply */ >> +}; >> + >> +#define IMX274_NUM_SUPPLIES ARRAY_SIZE(imx274_supply_names) >> + >> /* >> * imx274 I2C operation related structure >> */ >> @@ -501,6 +512,8 @@ struct imx274_ctrls { >> * @frame_rate: V4L2 frame rate structure >> * @regmap: Pointer to regmap structure >> * @reset_gpio: Pointer to reset gpio >> + * @supplies: List of analog and digital supply regulators >> + * @inck: Pointer to sensor input clock >> * @lock: Mutex structure >> * @mode: Parameters for the selected readout mode >> */ >> @@ -514,6 +527,8 @@ struct stimx274 { >> struct v4l2_fract frame_interval; >> struct regmap *regmap; >> struct gpio_desc *reset_gpio; >> + struct regulator_bulk_data supplies[IMX274_NUM_SUPPLIES]; >> + struct clk *inck; >> struct mutex lock; /* mutex lock for operations */ >> const struct imx274_mode *mode; >> }; >> @@ -767,6 +782,75 @@ static void imx274_reset(struct stimx274 *priv, int rst) >> usleep_range(IMX274_RESET_DELAY1, IMX274_RESET_DELAY2); >> } >> >> +/* >> + * imx274_power_on - Function called to power on the sensor >> + * @imx274: Pointer to device structure >> + */ > Can I say this does not bring much value ? :) > Also the parameter name is wrong > >> +static int imx274_power_on(struct device *dev) >> +{ >> + struct i2c_client *client = to_i2c_client(dev); >> + struct v4l2_subdev *sd = i2c_get_clientdata(client); >> + struct stimx274 *imx274 = to_imx274(sd); >> + int ret; >> + >> + /* keep sensor in reset before power on */ >> + imx274_reset(imx274, 0); >> + >> + ret = clk_prepare_enable(imx274->inck); >> + if (ret) { >> + dev_err(&imx274->client->dev, >> + "Failed to enable input clock: %d\n", ret); >> + return ret; >> + } >> + >> + ret = regulator_bulk_enable(IMX274_NUM_SUPPLIES, imx274->supplies); >> + if (ret) { >> + dev_err(&imx274->client->dev, >> + "Failed to enable regulators: %d\n", ret); >> + goto fail_reg; >> + } >> + >> + usleep_range(1, 2); > usleep_range() allows you to provide an interval in which your timeout > can be coalesced with others. Giving a [1usec, 2usec] range kind of > defeat the purpose. And most than everything, does sleeping for 2usec > serve any real purpose ? Following delay recommendation from DS for power on sequence. > > >> + imx274_reset(imx274, 1); >> + >> + return 0; >> + >> +fail_reg: >> + regulator_bulk_disable(IMX274_NUM_SUPPLIES, imx274->supplies); > regulator_bulk_enable() disables all the regulators that were enabled > before the one that failed, so I don't think you need this one here > >> + clk_disable_unprepare(imx274->inck); >> + return ret; >> +} >> + >> +/* >> + * imx274_power_off - Function called to power off the sensor >> + * @imx274: Pointer to device structure >> + */ > Same as the above one > >> +static int imx274_power_off(struct device *dev) >> +{ >> + struct i2c_client *client = to_i2c_client(dev); >> + struct v4l2_subdev *sd = i2c_get_clientdata(client); >> + struct stimx274 *imx274 = to_imx274(sd); >> + >> + imx274_reset(imx274, 0); >> + > Is reset before power-off necessary ? Its recommended power off sequence as per data sheet. Safe to keep sensor in reset before powering down one regulator at a time. > >> + regulator_bulk_disable(IMX274_NUM_SUPPLIES, imx274->supplies); >> + >> + clk_disable_unprepare(imx274->inck); >> + >> + return 0; >> +} >> + >> +static int imx274_get_regulators(struct device *dev, struct stimx274 *imx274) > For symmetry with the regulators API I would call this > imx274_regulators_get(). Up to you :) > >> +{ >> + unsigned int i; >> + >> + for (i = 0; i < IMX274_NUM_SUPPLIES; i++) >> + imx274->supplies[i].supply = imx274_supply_names[i]; >> + >> + return devm_regulator_bulk_get(dev, IMX274_NUM_SUPPLIES, >> + imx274->supplies); > ^ not sure if it's my email > client but you might have a > wrong indent here > > Also, the regulators are optional in the bindings, how do the > regulators API cope with that ? I had a look around and they seems to > assume regulators are provided. I might be mistaken though Yes these are optional regulators and based on feedback from sakari changed to use regulator_bulk_get() here. regulator_bulk_get() uses NORMAL_GET and in case if supplies is not found it will use dummy regulator. >> +} >> + >> /** >> * imx274_s_ctrl - This is used to set the imx274 V4L2 controls >> * @ctrl: V4L2 control to be set >> @@ -781,6 +865,9 @@ static int imx274_s_ctrl(struct v4l2_ctrl *ctrl) >> struct stimx274 *imx274 = to_imx274(sd); >> int ret = -EINVAL; >> >> + if (!pm_runtime_get_if_in_use(&imx274->client->dev)) >> + return 0; >> + > Right, but then you should call __v4l2_ctrl_handler_setup() in the > s_stream(1) call path to have controls updated (after > pm_runtime_get_sync() call for power on). I had a look at it seems > only exposure is updated. Existing driver does v4l2_ctrl_handler_setup() in probe(). So, sensor power on happens prior to that in probe() and then powers down during idle. > >> dev_dbg(&imx274->client->dev, >> "%s : s_ctrl: %s, value: %d\n", __func__, >> ctrl->name, ctrl->val); >> @@ -811,6 +898,8 @@ static int imx274_s_ctrl(struct v4l2_ctrl *ctrl) >> break; >> } >> >> + pm_runtime_put(&imx274->client->dev); >> + >> return ret; >> } >> >> @@ -1327,6 +1416,13 @@ static int imx274_s_stream(struct v4l2_subdev *sd, int on) >> mutex_lock(&imx274->lock); >> >> if (on) { >> + ret = pm_runtime_get_sync(&imx274->client->dev); >> + if (ret < 0) { >> + pm_runtime_put_noidle(&imx274->client->dev); >> + mutex_unlock(&imx274->lock); >> + return ret; >> + } >> + >> /* load mode registers */ >> ret = imx274_mode_regs(imx274); >> if (ret) >> @@ -1362,6 +1458,7 @@ static int imx274_s_stream(struct v4l2_subdev *sd, int on) >> ret = imx274_write_table(imx274, imx274_stop); >> if (ret) >> goto fail; >> + pm_runtime_put(&imx274->client->dev); >> } >> >> mutex_unlock(&imx274->lock); >> @@ -1369,6 +1466,7 @@ static int imx274_s_stream(struct v4l2_subdev *sd, int on) >> return 0; >> >> fail: >> + pm_runtime_put(&imx274->client->dev); >> mutex_unlock(&imx274->lock); >> dev_err(&imx274->client->dev, "s_stream failed\n"); >> return ret; >> @@ -1834,6 +1932,14 @@ static int imx274_probe(struct i2c_client *client) >> >> mutex_init(&imx274->lock); >> >> + imx274->inck = devm_clk_get_optional(&client->dev, "inck"); > clk_get_optional() might return error. I would check this with IS_ERR > >> + ret = imx274_get_regulators(&client->dev, imx274); >> + if (ret) { >> + dev_err(&client->dev, >> + "Failed to get power regulators, err: %d\n", ret); >> + return ret; >> + } >> + >> /* initialize format */ >> imx274->mode = &imx274_modes[IMX274_DEFAULT_BINNING]; >> imx274->crop.width = IMX274_MAX_WIDTH; >> @@ -1881,15 +1987,23 @@ static int imx274_probe(struct i2c_client *client) >> goto err_me; >> } >> >> - /* pull sensor out of reset */ >> - imx274_reset(imx274, 1); >> + /* power on the sensor */ >> + ret = imx274_power_on(&client->dev); >> + if (ret < 0) { >> + dev_err(&client->dev, >> + "%s : imx274 power on failed\n", __func__); >> + goto err_me; >> + } > Doesn't pm_runtime_get calls the poweron function for you ? > > But anyway, I don't see the device being probed for, in example, > querying it's VID/PID for identification during the driver's probe > routine. Do you need to power on ? existing driver does v4l2_ctrl handler setup and loads sensor default control values during probe. So doing sensor power_on here prior to setup. Power off happens during idle. > >> + >> + pm_runtime_set_active(&client->dev); >> + pm_runtime_enable(&client->dev); >> >> /* initialize controls */ >> ret = v4l2_ctrl_handler_init(&imx274->ctrls.handler, 4); >> if (ret < 0) { >> dev_err(&client->dev, >> "%s : ctrl handler init Failed\n", __func__); >> - goto err_me; >> + goto err_disable_rpm; >> } >> >> imx274->ctrls.handler.lock = &imx274->lock; >> @@ -1951,11 +2065,16 @@ static int imx274_probe(struct i2c_client *client) >> goto err_ctrls; >> } >> >> + pm_runtime_idle(&client->dev); >> + >> dev_info(&client->dev, "imx274 : imx274 probe success !\n"); >> return 0; >> >> err_ctrls: >> v4l2_ctrl_handler_free(&imx274->ctrls.handler); >> +err_disable_rpm: >> + pm_runtime_disable(&client->dev); >> + pm_runtime_set_suspended(&client->dev); >> err_me: >> media_entity_cleanup(&sd->entity); >> err_regmap: >> @@ -1973,14 +2092,23 @@ static int imx274_remove(struct i2c_client *client) >> >> v4l2_async_unregister_subdev(sd); >> v4l2_ctrl_handler_free(&imx274->ctrls.handler); >> + >> + pm_runtime_disable(&client->dev); >> + pm_runtime_set_suspended(&client->dev); >> + >> media_entity_cleanup(&sd->entity); >> mutex_destroy(&imx274->lock); >> return 0; >> } >> >> +static const struct dev_pm_ops imx274_pm_ops = { >> + SET_RUNTIME_PM_OPS(imx274_power_off, imx274_power_on, NULL) >> +}; >> + >> static struct i2c_driver imx274_i2c_driver = { >> .driver = { >> .name = DRIVER_NAME, >> + .pm = &imx274_pm_ops, >> .of_match_table = imx274_of_id_table, >> }, >> .probe_new = imx274_probe, >> -- >> 2.7.4 >> ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v5 3/3] media: i2c: imx274: Add IMX274 power on and off sequence 2020-09-03 16:25 ` Sowjanya Komatineni @ 2020-09-04 8:55 ` Jacopo Mondi [not found] ` <5ebe8d22-86fb-7bf2-ab19-e729caf8d88f@nvidia.com> 0 siblings, 1 reply; 18+ messages in thread From: Jacopo Mondi @ 2020-09-04 8:55 UTC (permalink / raw) To: Sowjanya Komatineni Cc: thierry.reding, jonathanh, sakari.ailus, hverkuil, jacopo+renesas, luca, leonl, robh+dt, lgirdwood, broonie, linux-media, devicetree, linux-kernel Hello Sowjanya, On Thu, Sep 03, 2020 at 09:25:44AM -0700, Sowjanya Komatineni wrote: > > On 9/3/20 8:03 AM, Jacopo Mondi wrote: > > Hello, > > > > On Tue, Sep 01, 2020 at 07:04:38PM -0700, Sowjanya Komatineni wrote: > > > IMX274 has VANA analog 2.8V supply, VDIG digital core 1.8V supply, > > > and VDDL digital io 1.2V supply which are optional based on camera > > > module design. > > > > > > IMX274 also need external 24Mhz clock and is optional based on > > > camera module design. > > > > > > This patch adds support for IMX274 power on and off to enable and > > > disable these supplies and external clock. > > > > > > Reviewed-by: Luca Ceresoli <luca@lucaceresoli.net> > > > Signed-off-by: Sowjanya Komatineni <skomatineni@nvidia.com> > > > --- > > > drivers/media/i2c/imx274.c | 134 ++++++++++++++++++++++++++++++++++++++++++++- > > > 1 file changed, 131 insertions(+), 3 deletions(-) > > > > > > diff --git a/drivers/media/i2c/imx274.c b/drivers/media/i2c/imx274.c > > > index a4b9dfd..79bfac3c6 100644 > > > --- a/drivers/media/i2c/imx274.c > > > +++ b/drivers/media/i2c/imx274.c > > > @@ -18,7 +18,9 @@ > > > #include <linux/kernel.h> > > > #include <linux/module.h> > > > #include <linux/of_gpio.h> > > > +#include <linux/pm_runtime.h> > > > #include <linux/regmap.h> > > > +#include <linux/regulator/consumer.h> > > > #include <linux/slab.h> > > > #include <linux/v4l2-mediabus.h> > > > #include <linux/videodev2.h> > > > @@ -131,6 +133,15 @@ > > > #define IMX274_TABLE_WAIT_MS 0 > > > #define IMX274_TABLE_END 1 > > > > > > +/* regulator supplies */ > > > +static const char * const imx274_supply_names[] = { > > > + "vddl", /* IF (1.2V) supply */ > > > + "vdig", /* Digital Core (1.8V) supply */ > > > + "vana", /* Analog (2.8V) supply */ > > > +}; > > > + > > > +#define IMX274_NUM_SUPPLIES ARRAY_SIZE(imx274_supply_names) > > > + > > > /* > > > * imx274 I2C operation related structure > > > */ > > > @@ -501,6 +512,8 @@ struct imx274_ctrls { > > > * @frame_rate: V4L2 frame rate structure > > > * @regmap: Pointer to regmap structure > > > * @reset_gpio: Pointer to reset gpio > > > + * @supplies: List of analog and digital supply regulators > > > + * @inck: Pointer to sensor input clock > > > * @lock: Mutex structure > > > * @mode: Parameters for the selected readout mode > > > */ > > > @@ -514,6 +527,8 @@ struct stimx274 { > > > struct v4l2_fract frame_interval; > > > struct regmap *regmap; > > > struct gpio_desc *reset_gpio; > > > + struct regulator_bulk_data supplies[IMX274_NUM_SUPPLIES]; > > > + struct clk *inck; > > > struct mutex lock; /* mutex lock for operations */ > > > const struct imx274_mode *mode; > > > }; > > > @@ -767,6 +782,75 @@ static void imx274_reset(struct stimx274 *priv, int rst) > > > usleep_range(IMX274_RESET_DELAY1, IMX274_RESET_DELAY2); > > > } > > > > > > +/* > > > + * imx274_power_on - Function called to power on the sensor > > > + * @imx274: Pointer to device structure > > > + */ > > Can I say this does not bring much value ? :) > > Also the parameter name is wrong > > > > > +static int imx274_power_on(struct device *dev) > > > +{ > > > + struct i2c_client *client = to_i2c_client(dev); > > > + struct v4l2_subdev *sd = i2c_get_clientdata(client); > > > + struct stimx274 *imx274 = to_imx274(sd); > > > + int ret; > > > + > > > + /* keep sensor in reset before power on */ > > > + imx274_reset(imx274, 0); > > > + > > > + ret = clk_prepare_enable(imx274->inck); > > > + if (ret) { > > > + dev_err(&imx274->client->dev, > > > + "Failed to enable input clock: %d\n", ret); > > > + return ret; > > > + } > > > + > > > + ret = regulator_bulk_enable(IMX274_NUM_SUPPLIES, imx274->supplies); > > > + if (ret) { > > > + dev_err(&imx274->client->dev, > > > + "Failed to enable regulators: %d\n", ret); > > > + goto fail_reg; > > > + } > > > + > > > + usleep_range(1, 2); > > usleep_range() allows you to provide an interval in which your timeout > > can be coalesced with others. Giving a [1usec, 2usec] range kind of > > defeat the purpose. And most than everything, does sleeping for 2usec > > serve any real purpose ? > > Following delay recommendation from DS for power on sequence. > 2 useconds ? Seems very short :) > > > > > > > + imx274_reset(imx274, 1); > > > + > > > + return 0; > > > + > > > +fail_reg: > > > + regulator_bulk_disable(IMX274_NUM_SUPPLIES, imx274->supplies); > > regulator_bulk_enable() disables all the regulators that were enabled > > before the one that failed, so I don't think you need this one here > > > > > + clk_disable_unprepare(imx274->inck); > > > + return ret; > > > +} > > > + > > > +/* > > > + * imx274_power_off - Function called to power off the sensor > > > + * @imx274: Pointer to device structure > > > + */ > > Same as the above one > > > > > +static int imx274_power_off(struct device *dev) > > > +{ > > > + struct i2c_client *client = to_i2c_client(dev); > > > + struct v4l2_subdev *sd = i2c_get_clientdata(client); > > > + struct stimx274 *imx274 = to_imx274(sd); > > > + > > > + imx274_reset(imx274, 0); > > > + > > Is reset before power-off necessary ? > > Its recommended power off sequence as per data sheet. > > Safe to keep sensor in reset before powering down one regulator at a time. > Fair enough then! > > > > > + regulator_bulk_disable(IMX274_NUM_SUPPLIES, imx274->supplies); > > > + > > > + clk_disable_unprepare(imx274->inck); > > > + > > > + return 0; > > > +} > > > + > > > +static int imx274_get_regulators(struct device *dev, struct stimx274 *imx274) > > For symmetry with the regulators API I would call this > > imx274_regulators_get(). Up to you :) > > > > > +{ > > > + unsigned int i; > > > + > > > + for (i = 0; i < IMX274_NUM_SUPPLIES; i++) > > > + imx274->supplies[i].supply = imx274_supply_names[i]; > > > + > > > + return devm_regulator_bulk_get(dev, IMX274_NUM_SUPPLIES, > > > + imx274->supplies); > > ^ not sure if it's my email > > client but you might have a > > wrong indent here > > > > Also, the regulators are optional in the bindings, how do the > > regulators API cope with that ? I had a look around and they seems to > > assume regulators are provided. I might be mistaken though > > Yes these are optional regulators and based on feedback from sakari changed > to use regulator_bulk_get() here. > > regulator_bulk_get() uses NORMAL_GET and in case if supplies is not found it > will use dummy regulator. > Ah thanks, I had a look at the regulator_get() implementation and missed that. So we're safe here! > > > +} > > > + > > > /** > > > * imx274_s_ctrl - This is used to set the imx274 V4L2 controls > > > * @ctrl: V4L2 control to be set > > > @@ -781,6 +865,9 @@ static int imx274_s_ctrl(struct v4l2_ctrl *ctrl) > > > struct stimx274 *imx274 = to_imx274(sd); > > > int ret = -EINVAL; > > > > > > + if (!pm_runtime_get_if_in_use(&imx274->client->dev)) > > > + return 0; > > > + > > Right, but then you should call __v4l2_ctrl_handler_setup() in the > > s_stream(1) call path to have controls updated (after > > pm_runtime_get_sync() call for power on). I had a look at it seems > > only exposure is updated. > > Existing driver does v4l2_ctrl_handler_setup() in probe(). So, sensor power > on happens prior to that in probe() and then powers down during idle. > mmm, my point is that with this patch if a control is set while the sensor is powered off it does not get applied, as you return 0 here. This mean the newly set control values have to be applied as soon as it possible, or at least before starting to stream. If you have a look at the imx219 driver, in the s_stream() call path there's a call to v4l2_ctrl_handler_setup() (in imx219_start_streaming()). I think you should do the same unless I mis-interpreted your reply. > > > > > dev_dbg(&imx274->client->dev, > > > "%s : s_ctrl: %s, value: %d\n", __func__, > > > ctrl->name, ctrl->val); > > > @@ -811,6 +898,8 @@ static int imx274_s_ctrl(struct v4l2_ctrl *ctrl) > > > break; > > > } > > > > > > + pm_runtime_put(&imx274->client->dev); > > > + > > > return ret; > > > } > > > > > > @@ -1327,6 +1416,13 @@ static int imx274_s_stream(struct v4l2_subdev *sd, int on) > > > mutex_lock(&imx274->lock); > > > > > > if (on) { > > > + ret = pm_runtime_get_sync(&imx274->client->dev); > > > + if (ret < 0) { > > > + pm_runtime_put_noidle(&imx274->client->dev); > > > + mutex_unlock(&imx274->lock); > > > + return ret; > > > + } > > > + > > > /* load mode registers */ > > > ret = imx274_mode_regs(imx274); > > > if (ret) > > > @@ -1362,6 +1458,7 @@ static int imx274_s_stream(struct v4l2_subdev *sd, int on) > > > ret = imx274_write_table(imx274, imx274_stop); > > > if (ret) > > > goto fail; > > > + pm_runtime_put(&imx274->client->dev); > > > } > > > > > > mutex_unlock(&imx274->lock); > > > @@ -1369,6 +1466,7 @@ static int imx274_s_stream(struct v4l2_subdev *sd, int on) > > > return 0; > > > > > > fail: > > > + pm_runtime_put(&imx274->client->dev); > > > mutex_unlock(&imx274->lock); > > > dev_err(&imx274->client->dev, "s_stream failed\n"); > > > return ret; > > > @@ -1834,6 +1932,14 @@ static int imx274_probe(struct i2c_client *client) > > > > > > mutex_init(&imx274->lock); > > > > > > + imx274->inck = devm_clk_get_optional(&client->dev, "inck"); > > clk_get_optional() might return error. I would check this with IS_ERR > > > > > + ret = imx274_get_regulators(&client->dev, imx274); > > > + if (ret) { > > > + dev_err(&client->dev, > > > + "Failed to get power regulators, err: %d\n", ret); > > > + return ret; > > > + } > > > + > > > /* initialize format */ > > > imx274->mode = &imx274_modes[IMX274_DEFAULT_BINNING]; > > > imx274->crop.width = IMX274_MAX_WIDTH; > > > @@ -1881,15 +1987,23 @@ static int imx274_probe(struct i2c_client *client) > > > goto err_me; > > > } > > > > > > - /* pull sensor out of reset */ > > > - imx274_reset(imx274, 1); > > > + /* power on the sensor */ > > > + ret = imx274_power_on(&client->dev); > > > + if (ret < 0) { > > > + dev_err(&client->dev, > > > + "%s : imx274 power on failed\n", __func__); > > > + goto err_me; > > > + } > > Doesn't pm_runtime_get calls the poweron function for you ? > > > > But anyway, I don't see the device being probed for, in example, > > querying it's VID/PID for identification during the driver's probe > > routine. Do you need to power on ? > > existing driver does v4l2_ctrl handler setup and loads sensor default > control values during probe. Ouch, they're pretty evident and I've missed that. Although I think you can call pm_runtime_get_sync() to have resume executed, but this makes no difference in practice I guess. Thanks for the clarifications, there's just a few items left to address in my opinion. Thanks j > > So doing sensor power_on here prior to setup. Power off happens during idle. > > > > > > + > > > + pm_runtime_set_active(&client->dev); > > > + pm_runtime_enable(&client->dev); > > > > > > /* initialize controls */ > > > ret = v4l2_ctrl_handler_init(&imx274->ctrls.handler, 4); > > > if (ret < 0) { > > > dev_err(&client->dev, > > > "%s : ctrl handler init Failed\n", __func__); > > > - goto err_me; > > > + goto err_disable_rpm; > > > } > > > > > > imx274->ctrls.handler.lock = &imx274->lock; > > > @@ -1951,11 +2065,16 @@ static int imx274_probe(struct i2c_client *client) > > > goto err_ctrls; > > > } > > > > > > + pm_runtime_idle(&client->dev); > > > + > > > dev_info(&client->dev, "imx274 : imx274 probe success !\n"); > > > return 0; > > > > > > err_ctrls: > > > v4l2_ctrl_handler_free(&imx274->ctrls.handler); > > > +err_disable_rpm: > > > + pm_runtime_disable(&client->dev); > > > + pm_runtime_set_suspended(&client->dev); > > > err_me: > > > media_entity_cleanup(&sd->entity); > > > err_regmap: > > > @@ -1973,14 +2092,23 @@ static int imx274_remove(struct i2c_client *client) > > > > > > v4l2_async_unregister_subdev(sd); > > > v4l2_ctrl_handler_free(&imx274->ctrls.handler); > > > + > > > + pm_runtime_disable(&client->dev); > > > + pm_runtime_set_suspended(&client->dev); > > > + > > > media_entity_cleanup(&sd->entity); > > > mutex_destroy(&imx274->lock); > > > return 0; > > > } > > > > > > +static const struct dev_pm_ops imx274_pm_ops = { > > > + SET_RUNTIME_PM_OPS(imx274_power_off, imx274_power_on, NULL) > > > +}; > > > + > > > static struct i2c_driver imx274_i2c_driver = { > > > .driver = { > > > .name = DRIVER_NAME, > > > + .pm = &imx274_pm_ops, > > > .of_match_table = imx274_of_id_table, > > > }, > > > .probe_new = imx274_probe, > > > -- > > > 2.7.4 > > > ^ permalink raw reply [flat|nested] 18+ messages in thread
[parent not found: <5ebe8d22-86fb-7bf2-ab19-e729caf8d88f@nvidia.com>]
* Re: [PATCH v5 3/3] media: i2c: imx274: Add IMX274 power on and off sequence [not found] ` <5ebe8d22-86fb-7bf2-ab19-e729caf8d88f@nvidia.com> @ 2020-09-07 7:48 ` Jacopo Mondi 2020-09-08 18:13 ` Sowjanya Komatineni 0 siblings, 1 reply; 18+ messages in thread From: Jacopo Mondi @ 2020-09-07 7:48 UTC (permalink / raw) To: Sowjanya Komatineni Cc: thierry.reding, jonathanh, sakari.ailus, hverkuil, jacopo+renesas, luca, leonl, robh+dt, lgirdwood, broonie, linux-media, devicetree, linux-kernel Hello, On Fri, Sep 04, 2020 at 10:04:10AM -0700, Sowjanya Komatineni wrote: > > On 9/4/20 1:55 AM, Jacopo Mondi wrote: > > > > usleep_range() allows you to provide an interval in which your timeout > > > > can be coalesced with others. Giving a [1usec, 2usec] range kind of > > > > defeat the purpose. And most than everything, does sleeping for 2usec > > > > serve any real purpose ? > > > Following delay recommendation from DS for power on sequence. > > > > > 2 useconds ? Seems very short:) > > > As per IMX274 datasheet for power on sequence, 100ns is the min wait time > after the last power supply of 1v8/1v2/2v8 is ON before releasing RESET > high. ook.. well, it's actually reasonable, it's just the time for the regulators to ramp up, I initially thought it was the time for the chip to exit reset. Let me be a bit more picky and ask if you have considered busy waiting on such a small sleep interval by using udelay. Again, as this happens at chip power on only, the impact on the system of mis-using usleep_range() is negligible, but according to documentation: SLEEPING FOR "A FEW" USECS ( < ~10us? ): * Use udelay - Why not usleep? On slower systems, (embedded, OR perhaps a speed- stepped PC!) the overhead of setting up the hrtimers for usleep *may* not be worth it. Such an evaluation will obviously depend on your specific situation, but it is something to be aware of. Up to you, really! Thanks j ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v5 3/3] media: i2c: imx274: Add IMX274 power on and off sequence 2020-09-07 7:48 ` Jacopo Mondi @ 2020-09-08 18:13 ` Sowjanya Komatineni 0 siblings, 0 replies; 18+ messages in thread From: Sowjanya Komatineni @ 2020-09-08 18:13 UTC (permalink / raw) To: Jacopo Mondi Cc: thierry.reding, jonathanh, sakari.ailus, hverkuil, jacopo+renesas, luca, leonl, robh+dt, lgirdwood, broonie, linux-media, devicetree, linux-kernel On 9/7/20 12:48 AM, Jacopo Mondi wrote: > Hello, > > On Fri, Sep 04, 2020 at 10:04:10AM -0700, Sowjanya Komatineni wrote: >> On 9/4/20 1:55 AM, Jacopo Mondi wrote: >>>>> usleep_range() allows you to provide an interval in which your timeout >>>>> can be coalesced with others. Giving a [1usec, 2usec] range kind of >>>>> defeat the purpose. And most than everything, does sleeping for 2usec >>>>> serve any real purpose ? >>>> Following delay recommendation from DS for power on sequence. >>>> >>> 2 useconds ? Seems very short:) >>> >> As per IMX274 datasheet for power on sequence, 100ns is the min wait time >> after the last power supply of 1v8/1v2/2v8 is ON before releasing RESET >> high. > ook.. well, it's actually reasonable, it's just the time for the > regulators to ramp up, I initially thought it was the time for the > chip to exit reset. > > Let me be a bit more picky and ask if you have considered busy waiting > on such a small sleep interval by using udelay. Again, as this happens > at chip power on only, the impact on the system of mis-using > usleep_range() is negligible, but according to documentation: > > SLEEPING FOR "A FEW" USECS ( < ~10us? ): > * Use udelay > > - Why not usleep? > On slower systems, (embedded, OR perhaps a speed- > stepped PC!) the overhead of setting up the hrtimers > for usleep *may* not be worth it. Such an evaluation > will obviously depend on your specific situation, but > it is something to be aware of. > > Up to you, really! > > Thanks > j Thanks Jacopo. Will update in v6 to use udelay. ^ permalink raw reply [flat|nested] 18+ messages in thread
end of thread, other threads:[~2020-09-08 18:13 UTC | newest] Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-09-02 2:04 [PATCH v5 0/3] IMX274 fixes and power on and off implementation Sowjanya Komatineni 2020-09-02 2:04 ` [PATCH v5 1/3] media: i2c: imx274: Fix Y_OUT_SIZE register setting Sowjanya Komatineni 2020-09-02 2:04 ` [PATCH v5 2/3] dt-bindings: media: imx274: Add optional input clock and supplies Sowjanya Komatineni 2020-09-02 6:46 ` Luca Ceresoli 2020-09-02 6:52 ` Luca Ceresoli 2020-09-03 12:55 ` Jacopo Mondi 2020-09-03 16:05 ` Sowjanya Komatineni 2020-09-03 16:35 ` Jacopo Mondi 2020-09-03 16:40 ` Sowjanya Komatineni 2020-09-08 9:33 ` Sakari Ailus 2020-09-08 14:34 ` Jacopo Mondi 2020-09-08 18:10 ` Sowjanya Komatineni 2020-09-02 2:04 ` [PATCH v5 3/3] media: i2c: imx274: Add IMX274 power on and off sequence Sowjanya Komatineni 2020-09-03 15:03 ` Jacopo Mondi 2020-09-03 16:25 ` Sowjanya Komatineni 2020-09-04 8:55 ` Jacopo Mondi [not found] ` <5ebe8d22-86fb-7bf2-ab19-e729caf8d88f@nvidia.com> 2020-09-07 7:48 ` Jacopo Mondi 2020-09-08 18:13 ` Sowjanya Komatineni
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).