From: Robert Foss <robert.foss@linaro.org> To: Sakari Ailus <sakari.ailus@iki.fi> Cc: Dongchun Zhu <dongchun.zhu@mediatek.com>, Fabio Estevam <festevam@gmail.com>, Andy Shevchenko <andriy.shevchenko@linux.intel.com>, Tomasz Figa <tfiga@chromium.org>, linux-media <linux-media@vger.kernel.org>, "open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS" <devicetree@vger.kernel.org>, linux-kernel <linux-kernel@vger.kernel.org>, "moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE" <linux-arm-kernel@lists.infradead.org> Subject: Re: [v2 2/3] media: ov8856: Add devicetree support Date: Fri, 27 Mar 2020 11:32:29 +0100 [thread overview] Message-ID: <CAG3jFyu=HOsWNeRFC2t4HjzYrFrLjsbXzAm4+zD50Xq48mqzcw@mail.gmail.com> (raw) In-Reply-To: <20200326144725.GA2394@valkosipuli.retiisi.org.uk> On Thu, 26 Mar 2020 at 15:47, Sakari Ailus <sakari.ailus@iki.fi> wrote: > > Hi Robert, > > On Thu, Mar 26, 2020 at 12:56:37PM +0100, Robert Foss wrote: > ... > > > > +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; > > > > + } > > > > + > > > > + gpiod_set_value_cansleep(ov8856->reset_gpio, GPIOD_OUT_HIGH); > > > > + > > > > + ret = regulator_bulk_enable(ARRAY_SIZE(ov8856_supply_names), > > > > + ov8856->supplies); > > > > + if (ret < 0) { > > > > + dev_err(&client->dev, "failed to enable regulators\n"); > > > > + goto disable_clk; > > > > + } > > > > + > > > > + gpiod_set_value_cansleep(ov8856->reset_gpio, GPIOD_OUT_LOW); > > > > + > > > > + usleep_range(1500, 1800); > > > > > > I think you could omit the delay on ACPI based systems. Or just bail out > > > early in that case. > > > > I'll add a check for reset_gpio being NULL, and skip the sleep for that case. > > There could also be a regulator but no GPIO. > > I think if you don't have either, then certainly there's no need for a > delay. Removing the delay if no action is taken makes sense, but I'm not sure how best to do it. If there are no regulators dummy ones are created automatically, which makes distinguishing between a little bit cumbersome. The regulator structs could of course all be inspected, and if all are dummy ones, the delay could be skipped. But is there a neater way of doing this? Manually inspecting the regs strikes me as a bit inelegant. > > ... > > > > > + ov8856->xvclk = NULL; > > > > + } else if (IS_ERR(ov8856->xvclk)) { > > > > + dev_err(&client->dev, "could not get xvclk clock (%ld)\n", > > > > + PTR_ERR(ov8856->xvclk)); > > > > + return PTR_ERR(ov8856->xvclk); > > > > + } > > > > + > > > > + ret = clk_set_rate(ov8856->xvclk, OV8856_XVCLK_24); > > > > > > This should either come from platform data, or perhaps it'd be even better > > > to get the clock rate and use assigned-clock-rates. I guess that's > > > preferred nowadays. > > > > I'm a bit unsure about what this would look like. > > > > Are you thinking something like the way ext_clk is used in smiapp_core.c? > > I went ahead and implemented support for retrieving and storing > > 'clock-rates' during the ov8856_check_hwcfg() call, and then setting > > the rate to the configured rate during probing. > > With assigned-clock-rates, you can simply use clk_get_rate(). Ah, I see. I'll switch to that approach then. > > As you get the actual rate, it could be somewhat off of the intended one. > > -- > Kind regards, > > Sakari Ailus
WARNING: multiple messages have this Message-ID (diff)
From: Robert Foss <robert.foss@linaro.org> To: Sakari Ailus <sakari.ailus@iki.fi> Cc: "open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS" <devicetree@vger.kernel.org>, linux-kernel <linux-kernel@vger.kernel.org>, Tomasz Figa <tfiga@chromium.org>, Dongchun Zhu <dongchun.zhu@mediatek.com>, Andy Shevchenko <andriy.shevchenko@linux.intel.com>, Fabio Estevam <festevam@gmail.com>, "moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE" <linux-arm-kernel@lists.infradead.org>, linux-media <linux-media@vger.kernel.org> Subject: Re: [v2 2/3] media: ov8856: Add devicetree support Date: Fri, 27 Mar 2020 11:32:29 +0100 [thread overview] Message-ID: <CAG3jFyu=HOsWNeRFC2t4HjzYrFrLjsbXzAm4+zD50Xq48mqzcw@mail.gmail.com> (raw) In-Reply-To: <20200326144725.GA2394@valkosipuli.retiisi.org.uk> On Thu, 26 Mar 2020 at 15:47, Sakari Ailus <sakari.ailus@iki.fi> wrote: > > Hi Robert, > > On Thu, Mar 26, 2020 at 12:56:37PM +0100, Robert Foss wrote: > ... > > > > +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; > > > > + } > > > > + > > > > + gpiod_set_value_cansleep(ov8856->reset_gpio, GPIOD_OUT_HIGH); > > > > + > > > > + ret = regulator_bulk_enable(ARRAY_SIZE(ov8856_supply_names), > > > > + ov8856->supplies); > > > > + if (ret < 0) { > > > > + dev_err(&client->dev, "failed to enable regulators\n"); > > > > + goto disable_clk; > > > > + } > > > > + > > > > + gpiod_set_value_cansleep(ov8856->reset_gpio, GPIOD_OUT_LOW); > > > > + > > > > + usleep_range(1500, 1800); > > > > > > I think you could omit the delay on ACPI based systems. Or just bail out > > > early in that case. > > > > I'll add a check for reset_gpio being NULL, and skip the sleep for that case. > > There could also be a regulator but no GPIO. > > I think if you don't have either, then certainly there's no need for a > delay. Removing the delay if no action is taken makes sense, but I'm not sure how best to do it. If there are no regulators dummy ones are created automatically, which makes distinguishing between a little bit cumbersome. The regulator structs could of course all be inspected, and if all are dummy ones, the delay could be skipped. But is there a neater way of doing this? Manually inspecting the regs strikes me as a bit inelegant. > > ... > > > > > + ov8856->xvclk = NULL; > > > > + } else if (IS_ERR(ov8856->xvclk)) { > > > > + dev_err(&client->dev, "could not get xvclk clock (%ld)\n", > > > > + PTR_ERR(ov8856->xvclk)); > > > > + return PTR_ERR(ov8856->xvclk); > > > > + } > > > > + > > > > + ret = clk_set_rate(ov8856->xvclk, OV8856_XVCLK_24); > > > > > > This should either come from platform data, or perhaps it'd be even better > > > to get the clock rate and use assigned-clock-rates. I guess that's > > > preferred nowadays. > > > > I'm a bit unsure about what this would look like. > > > > Are you thinking something like the way ext_clk is used in smiapp_core.c? > > I went ahead and implemented support for retrieving and storing > > 'clock-rates' during the ov8856_check_hwcfg() call, and then setting > > the rate to the configured rate during probing. > > With assigned-clock-rates, you can simply use clk_get_rate(). Ah, I see. I'll switch to that approach then. > > As you get the actual rate, it could be somewhat off of the intended one. > > -- > Kind regards, > > Sakari Ailus _______________________________________________ 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-27 10:32 UTC|newest] Thread overview: 34+ messages / expand[flat|nested] mbox.gz Atom feed top 2020-03-13 11:03 [v2 0/3] media: ov8856: Add devicetree support Robert Foss 2020-03-13 11:03 ` Robert Foss 2020-03-13 11:03 ` [v2 1/3] media: dt-bindings: ov8856: Document YAML bindings Robert Foss 2020-03-13 11:03 ` Robert Foss 2020-03-13 12:19 ` Sakari Ailus 2020-03-13 12:19 ` Sakari Ailus 2020-03-13 22:00 ` Rob Herring 2020-03-13 22:00 ` Rob Herring 2020-03-13 11:03 ` [v2 2/3] media: ov8856: Add devicetree support Robert Foss 2020-03-13 11:03 ` Robert Foss 2020-03-13 12:17 ` Sakari Ailus 2020-03-13 12:17 ` Sakari Ailus 2020-03-26 11:56 ` Robert Foss 2020-03-26 11:56 ` Robert Foss 2020-03-26 14:47 ` Sakari Ailus 2020-03-26 14:47 ` Sakari Ailus 2020-03-27 10:32 ` Robert Foss [this message] 2020-03-27 10:32 ` Robert Foss 2020-03-27 13:37 ` Sakari Ailus 2020-03-27 13:37 ` Sakari Ailus 2020-03-13 12:28 ` Andy Shevchenko 2020-03-13 12:28 ` Andy Shevchenko 2020-03-13 13:15 ` Fabio Estevam 2020-03-13 13:15 ` Fabio Estevam 2020-03-31 13:37 ` Robert Foss 2020-03-31 13:37 ` Robert Foss 2020-03-31 13:42 ` Fabio Estevam 2020-03-31 13:42 ` Fabio Estevam 2020-03-31 13:53 ` Andy Shevchenko 2020-03-31 13:53 ` Andy Shevchenko 2020-03-13 11:03 ` [v2 3/3] media: ov8856: Implement sensor module revision identification Robert Foss 2020-03-13 11:03 ` Robert Foss 2020-03-13 12:43 ` Sakari Ailus 2020-03-13 12:43 ` Sakari Ailus
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='CAG3jFyu=HOsWNeRFC2t4HjzYrFrLjsbXzAm4+zD50Xq48mqzcw@mail.gmail.com' \ --to=robert.foss@linaro.org \ --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=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.