Linux-ARM-Kernel Archive on lore.kernel.org
 help / color / Atom feed
From: Robert Foss <robert.foss@linaro.org>
To: Andy Shevchenko <andy.shevchenko@gmail.com>
Cc: devicetree <devicetree@vger.kernel.org>,
	Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
	Tomasz Figa <tfiga@chromium.org>,
	Sakari Ailus <sakari.ailus@iki.fi>,
	Dongchun Zhu <dongchun.zhu@mediatek.com>,
	Fabio Estevam <festevam@gmail.com>,
	linux-arm Mailing List <linux-arm-kernel@lists.infradead.org>,
	Linux Media Mailing List <linux-media@vger.kernel.org>
Subject: Re: [PATCH v3 2/3] media: ov8856: Add devicetree support
Date: Mon, 6 Apr 2020 17:25:58 +0200
Message-ID: <CAG3jFyvRD3MTQHKVgnneNutJ_e-eykrZHNF-GBe1F5c1TjYLqg@mail.gmail.com> (raw)
In-Reply-To: <20200406150626.GW3676135@smile.fi.intel.com>

On Mon, 6 Apr 2020 at 17:06, Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
>
> On Mon, Apr 06, 2020 at 03:37:24PM +0200, Robert Foss wrote:
> > On Tue, 31 Mar 2020 at 16:01, Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> > > On Tue, Mar 31, 2020 at 4:36 PM Robert Foss <robert.foss@linaro.org> wrote:
>
> ...
>
> > > > +       if (ret < 0) {
> > >
> > > Do you need all ' < 0' parts all over the series?
> >
> > Some checks are needed due to ACPI and DT support co-existing.
> > Maybe it would be better to just split the probing into an ACPI path
> > and a DT path.
> >
> > I'll have a look through the series for redundant retval checks.
>
> Drop where it is redundant.
>
> ...
>
> > > > -       ret = fwnode_property_read_u32(fwnode, "clock-frequency", &mclk);
> > > > -       if (ret)
> > > > -               return ret;
> > >
> > > Where is it gone? Why?
> >
> > It was replaced by a clk_get_rate call, which as Sakari pointed out,
> > isn't correct.
> > I'll rework the clock handling for v4.
>
> If it was in the driver it should stay -- properties is an ABI (between firmware and kernel).

Ack.

>
> > > > +       ov8856->xvclk = devm_clk_get_optional(dev, "xvclk");
> > > > +       if (IS_ERR(ov8856->xvclk)) {
> > >
> > > > +               dev_err(dev, "could not get xvclk clock (%ld)\n",
> > > > +                       PTR_ERR(ov8856->xvclk));
> > >
> > > Also you may use %pe here and in similar cases.
> >
> > Weirdly checkpatch complains about this.
> > But it builds and runs cleanly, so I'll add it in v4.
>
> %pe requires pointer, PTR_ERR converts pointer to integer.

Ack.

>
> ...
>
> > > > +       ret = devm_regulator_bulk_get(dev, ARRAY_SIZE(ov8856_supply_names),
> > > > +                                     ov8856->supplies);
> > > > +       if (ret) {
> > >
> > > > +               dev_warn(dev, "failed to get regulators\n");
> > >
> > > If it's a warning, why we return from here?
> > > Same question to all other places with same issue.
> >
> > The issue I was seeing was the driver having to return a EDEFER here,
> > so this warning sheds some light on which exact component is returning
> > an EDEFER.
> >
> > [   15.962623] ov8856 16-0010: Dropping the link to regulator.29
> > [   15.968464] ov8856 16-0010: failed to get regulators
> > [   15.973493] ov8856 16-0010: failed to get HW configuration: -517
> > [   15.979591] ov8856 16-0010: removing from PM domain titan_top_gdsc
> > [   15.985855] ov8856 16-0010: genpd_remove_device()
> > [   15.990672] i2c 16-0010: Driver ov8856 requests probe deferral
> >
> > Personally I found it helpful to speed up debugging, but I'll happily
> > remove it if you prefer no warning.
>
> My point is that you have it in align:
>  - if it is an error, print as an error and bail out, otherwise
>  - if it is a warning, print it and continue.

I see what you're saying now, let's remove it then :)
I guess in the specific case of EDEFER, it doesn't fit neatly into
either of those categories, in the sense that the way you continue is
to return and then try to probe again later.

There are some other locations where this is handled wrong, I'll align
them properly for v4.

>
> > > > +               return ret;
> > > >         }
>
> --
> With Best Regards,
> Andy Shevchenko
>
>

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

  reply index

Thread overview: 38+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-03-31 13:33 [PATCH v3 0/3] " Robert Foss
2020-03-31 13:33 ` [PATCH v6 1/3] media: dt-bindings: ov8856: Document YAML bindings Robert Foss
2020-03-31 15:12   ` Marco Felsch
2020-04-02  9:57     ` Robert Foss
2020-04-03 19:21       ` Marco Felsch
2020-04-01  8:07   ` Maxime Ripard
2020-04-02 10:10     ` Robert Foss
2020-04-03 23:27       ` Sakari Ailus
2020-04-04  9:34         ` Maxime Ripard
2020-04-06  8:25           ` Robert Foss
2020-04-06  8:35           ` Sakari Ailus
2020-04-07  8:36             ` Maxime Ripard
2020-04-07 11:29               ` Robert Foss
2020-04-07 12:32                 ` Maxime Ripard
2020-04-07 15:47                   ` Robert Foss
2020-04-07 16:39                     ` Sakari Ailus
2020-04-07 16:46                       ` Tomasz Figa
2020-04-07 17:20                         ` Sakari Ailus
2020-04-08 12:21                           ` Maxime Ripard
2020-04-08 12:35                             ` Tomasz Figa
2020-04-08 13:43                               ` Maxime Ripard
2020-04-08 15:28                                 ` Sakari Ailus
2020-04-08 15:30                                   ` Sakari Ailus
2020-04-08 16:34                                     ` Andy Shevchenko
2020-04-15 10:18                                     ` Maxime Ripard
2020-04-15 11:10                                       ` Robert Foss
2020-04-15 16:16                                       ` Sakari Ailus
2020-04-20 15:02                                         ` Maxime Ripard
2020-04-09  8:32                                   ` Robert Foss
2020-04-07 16:20                   ` Sakari Ailus
2020-04-04  9:23       ` Maxime Ripard
2020-03-31 13:33 ` [PATCH v3 2/3] media: ov8856: Add devicetree support Robert Foss
2020-03-31 14:01   ` Andy Shevchenko
2020-04-06 13:37     ` Robert Foss
2020-04-06 15:06       ` Andy Shevchenko
2020-04-06 15:25         ` Robert Foss [this message]
2020-04-03 23:33   ` Sakari Ailus
2020-03-31 13:33 ` [PATCH v3 3/3] media: ov8856: Implement sensor module revision identification Robert Foss

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=CAG3jFyvRD3MTQHKVgnneNutJ_e-eykrZHNF-GBe1F5c1TjYLqg@mail.gmail.com \
    --to=robert.foss@linaro.org \
    --cc=andy.shevchenko@gmail.com \
    --cc=devicetree@vger.kernel.org \
    --cc=dongchun.zhu@mediatek.com \
    --cc=festevam@gmail.com \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-media@vger.kernel.org \
    --cc=sakari.ailus@iki.fi \
    --cc=tfiga@chromium.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

Linux-ARM-Kernel Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-arm-kernel/0 linux-arm-kernel/git/0.git
	git clone --mirror https://lore.kernel.org/linux-arm-kernel/1 linux-arm-kernel/git/1.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-arm-kernel linux-arm-kernel/ https://lore.kernel.org/linux-arm-kernel \
		linux-arm-kernel@lists.infradead.org
	public-inbox-index linux-arm-kernel

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.infradead.lists.linux-arm-kernel


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git