From: Andy Shevchenko <andy.shevchenko@gmail.com> To: Robert Foss <robert.foss@linaro.org> Cc: Dongchun Zhu <dongchun.zhu@mediatek.com>, Fabio Estevam <festevam@gmail.com>, Andy Shevchenko <andriy.shevchenko@linux.intel.com>, Sakari Ailus <sakari.ailus@iki.fi>, Tomasz Figa <tfiga@chromium.org>, Linux Media Mailing List <linux-media@vger.kernel.org>, devicetree <devicetree@vger.kernel.org>, Linux Kernel Mailing List <linux-kernel@vger.kernel.org>, linux-arm Mailing List <linux-arm-kernel@lists.infradead.org> Subject: Re: [PATCH v3 2/3] media: ov8856: Add devicetree support Date: Tue, 31 Mar 2020 17:01:00 +0300 [thread overview] Message-ID: <CAHp75VfFKY6nzb8aEWmop73v2haZ0P5+aTsKDEU8M=uUPn0u3g@mail.gmail.com> (raw) In-Reply-To: <20200331133346.372517-3-robert.foss@linaro.org> On Tue, Mar 31, 2020 at 4:36 PM Robert Foss <robert.foss@linaro.org> wrote: > > Add devicetree match table, and enable ov8856_probe() > to initialize power, clocks and reset pins. ... > +static int __ov8856_power_on(struct ov8856 *ov8856) > +{ > + struct i2c_client *client = v4l2_get_subdevdata(&ov8856->sd); > + int ret; > + > + ret = clk_prepare_enable(ov8856->xvclk); > + if (ret < 0) { > + dev_err(&client->dev, "failed to enable xvclk\n"); > + return ret; > + } > + > + if (is_acpi_node(ov8856->dev->fwnode)) Use dev_fwnode(). > + return 0; > + > + if (ov8856->reset_gpio) { > + gpiod_set_value_cansleep(ov8856->reset_gpio, GPIOD_OUT_HIGH); This is wrong. You have to fix it to use either 0 or 1. > + usleep_range(1000, 2000); > + } > + > + ret = regulator_bulk_enable(ARRAY_SIZE(ov8856_supply_names), > + ov8856->supplies); > + if (ret < 0) { Do you need all ' < 0' parts all over the series? > + dev_err(&client->dev, "failed to enable regulators\n"); > + goto disable_clk; > + } ... > + gpiod_set_value_cansleep(ov8856->reset_gpio, GPIOD_OUT_LOW); Ditto. ... > + gpiod_set_value_cansleep(ov8856->reset_gpio, GPIOD_OUT_HIGH); Ditto. ... > + gpiod_set_value_cansleep(ov8856->reset_gpio, GPIOD_OUT_HIGH); Ditto. ... > - ret = fwnode_property_read_u32(fwnode, "clock-frequency", &mclk); > - if (ret) > - return ret; Where is it gone? Why? > + 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. > + return PTR_ERR(ov8856->xvclk); > + } > + ov8856->reset_gpio = devm_gpiod_get_optional(dev, "reset", > + GPIOD_OUT_HIGH); Here parameter is correct. The question is, what the value should be HIGH or LOW? Basically HIGH means to assert the signal. > + if (IS_ERR(ov8856->reset_gpio)) { > + dev_dbg(dev, "failed to get reset-gpio\n"); Noise. Enable GPIO debug to see this kind of messages. > + return PTR_ERR(ov8856->reset_gpio); > + } ... > + 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. > + return ret; > } -- With Best Regards, Andy Shevchenko
WARNING: multiple messages have this Message-ID (diff)
From: Andy Shevchenko <andy.shevchenko@gmail.com> To: Robert Foss <robert.foss@linaro.org> 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>, Andy Shevchenko <andriy.shevchenko@linux.intel.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: Tue, 31 Mar 2020 17:01:00 +0300 [thread overview] Message-ID: <CAHp75VfFKY6nzb8aEWmop73v2haZ0P5+aTsKDEU8M=uUPn0u3g@mail.gmail.com> (raw) In-Reply-To: <20200331133346.372517-3-robert.foss@linaro.org> On Tue, Mar 31, 2020 at 4:36 PM Robert Foss <robert.foss@linaro.org> wrote: > > Add devicetree match table, and enable ov8856_probe() > to initialize power, clocks and reset pins. ... > +static int __ov8856_power_on(struct ov8856 *ov8856) > +{ > + struct i2c_client *client = v4l2_get_subdevdata(&ov8856->sd); > + int ret; > + > + ret = clk_prepare_enable(ov8856->xvclk); > + if (ret < 0) { > + dev_err(&client->dev, "failed to enable xvclk\n"); > + return ret; > + } > + > + if (is_acpi_node(ov8856->dev->fwnode)) Use dev_fwnode(). > + return 0; > + > + if (ov8856->reset_gpio) { > + gpiod_set_value_cansleep(ov8856->reset_gpio, GPIOD_OUT_HIGH); This is wrong. You have to fix it to use either 0 or 1. > + usleep_range(1000, 2000); > + } > + > + ret = regulator_bulk_enable(ARRAY_SIZE(ov8856_supply_names), > + ov8856->supplies); > + if (ret < 0) { Do you need all ' < 0' parts all over the series? > + dev_err(&client->dev, "failed to enable regulators\n"); > + goto disable_clk; > + } ... > + gpiod_set_value_cansleep(ov8856->reset_gpio, GPIOD_OUT_LOW); Ditto. ... > + gpiod_set_value_cansleep(ov8856->reset_gpio, GPIOD_OUT_HIGH); Ditto. ... > + gpiod_set_value_cansleep(ov8856->reset_gpio, GPIOD_OUT_HIGH); Ditto. ... > - ret = fwnode_property_read_u32(fwnode, "clock-frequency", &mclk); > - if (ret) > - return ret; Where is it gone? Why? > + 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. > + return PTR_ERR(ov8856->xvclk); > + } > + ov8856->reset_gpio = devm_gpiod_get_optional(dev, "reset", > + GPIOD_OUT_HIGH); Here parameter is correct. The question is, what the value should be HIGH or LOW? Basically HIGH means to assert the signal. > + if (IS_ERR(ov8856->reset_gpio)) { > + dev_dbg(dev, "failed to get reset-gpio\n"); Noise. Enable GPIO debug to see this kind of messages. > + return PTR_ERR(ov8856->reset_gpio); > + } ... > + 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. > + 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
next prev parent reply other threads:[~2020-03-31 14:01 UTC|newest] Thread overview: 76+ messages / expand[flat|nested] mbox.gz Atom feed top 2020-03-31 13:33 [PATCH v3 0/3] media: ov8856: Add devicetree support Robert Foss 2020-03-31 13:33 ` Robert Foss 2020-03-31 13:33 ` [PATCH v6 1/3] media: dt-bindings: ov8856: Document YAML bindings Robert Foss 2020-03-31 13:33 ` Robert Foss 2020-03-31 15:12 ` Marco Felsch 2020-03-31 15:12 ` Marco Felsch 2020-04-02 9:57 ` Robert Foss 2020-04-02 9:57 ` Robert Foss 2020-04-03 19:21 ` Marco Felsch 2020-04-03 19:21 ` Marco Felsch 2020-04-01 8:07 ` Maxime Ripard 2020-04-01 8:07 ` Maxime Ripard 2020-04-02 10:10 ` Robert Foss 2020-04-02 10:10 ` Robert Foss 2020-04-03 23:27 ` Sakari Ailus 2020-04-03 23:27 ` Sakari Ailus 2020-04-04 9:34 ` Maxime Ripard 2020-04-04 9:34 ` Maxime Ripard 2020-04-06 8:25 ` Robert Foss 2020-04-06 8:25 ` Robert Foss 2020-04-06 8:35 ` Sakari Ailus 2020-04-06 8:35 ` Sakari Ailus 2020-04-07 8:36 ` Maxime Ripard 2020-04-07 8:36 ` Maxime Ripard 2020-04-07 11:29 ` Robert Foss 2020-04-07 11:29 ` Robert Foss 2020-04-07 12:32 ` Maxime Ripard 2020-04-07 12:32 ` Maxime Ripard 2020-04-07 15:47 ` Robert Foss 2020-04-07 15:47 ` Robert Foss 2020-04-07 16:39 ` Sakari Ailus 2020-04-07 16:39 ` Sakari Ailus 2020-04-07 16:46 ` Tomasz Figa 2020-04-07 16:46 ` Tomasz Figa 2020-04-07 17:20 ` Sakari Ailus 2020-04-07 17:20 ` Sakari Ailus 2020-04-08 12:21 ` Maxime Ripard 2020-04-08 12:21 ` Maxime Ripard 2020-04-08 12:35 ` Tomasz Figa 2020-04-08 12:35 ` Tomasz Figa 2020-04-08 13:43 ` Maxime Ripard 2020-04-08 13:43 ` Maxime Ripard 2020-04-08 15:28 ` Sakari Ailus 2020-04-08 15:28 ` Sakari Ailus 2020-04-08 15:30 ` Sakari Ailus 2020-04-08 15:30 ` Sakari Ailus 2020-04-08 16:34 ` Andy Shevchenko 2020-04-08 16:34 ` Andy Shevchenko 2020-04-15 10:18 ` Maxime Ripard 2020-04-15 10:18 ` Maxime Ripard 2020-04-15 11:10 ` Robert Foss 2020-04-15 11:10 ` Robert Foss 2020-04-15 16:16 ` Sakari Ailus 2020-04-15 16:16 ` Sakari Ailus 2020-04-20 15:02 ` Maxime Ripard 2020-04-20 15:02 ` Maxime Ripard 2020-04-09 8:32 ` Robert Foss 2020-04-09 8:32 ` Robert Foss 2020-04-07 16:20 ` Sakari Ailus 2020-04-07 16:20 ` Sakari Ailus 2020-04-04 9:23 ` Maxime Ripard 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 13:33 ` Robert Foss 2020-03-31 14:01 ` Andy Shevchenko [this message] 2020-03-31 14:01 ` Andy Shevchenko 2020-04-06 13:37 ` Robert Foss 2020-04-06 13:37 ` Robert Foss 2020-04-06 15:06 ` Andy Shevchenko 2020-04-06 15:06 ` Andy Shevchenko 2020-04-06 15:25 ` Robert Foss 2020-04-06 15:25 ` Robert Foss 2020-04-03 23:33 ` Sakari Ailus 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 2020-03-31 13:33 ` 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='CAHp75VfFKY6nzb8aEWmop73v2haZ0P5+aTsKDEU8M=uUPn0u3g@mail.gmail.com' \ --to=andy.shevchenko@gmail.com \ --cc=andriy.shevchenko@linux.intel.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=robert.foss@linaro.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: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.