* [v2 2/3] media: ov8856: Add devicetree support
@ 2020-03-13 11:03 ` Robert Foss
0 siblings, 0 replies; 34+ messages in thread
From: Robert Foss @ 2020-03-13 11:03 UTC (permalink / raw)
To: Dongchun Zhu, Fabio Estevam, Andy Shevchenko, Sakari Ailus,
Tomasz Figa, linux-media, devicetree, linux-kernel,
linux-arm-kernel
Cc: Robert Foss
Add devicetree match table, and enable ov8856_probe()
to initialize power, clocks and reset pins.
Signed-off-by: Robert Foss <robert.foss@linaro.org>
---
- Changes since v1:
* Fabio: Change n_shutdown_gpio name to reset_gpio
* Fabio: Invert reset_gpio due to GPIO_ACTIVE_HIGH -> GPIO_ACTIVE_LOW change
* Fabio: Remove empty line
* Fabio: Remove real error from devm_gpiod_get() failures
* Andy & Sakari: Make XVCLK optional since to not break ACPI
* Sakari: ARRAY_SIZE() directly instead of through OV8856_NUM_SUPPLIES
* Sakari: Use XVCLK rate as provided by DT
drivers/media/i2c/ov8856.c | 109 ++++++++++++++++++++++++++++++++++++-
1 file changed, 107 insertions(+), 2 deletions(-)
diff --git a/drivers/media/i2c/ov8856.c b/drivers/media/i2c/ov8856.c
index 8655842af275..db61eed223e8 100644
--- a/drivers/media/i2c/ov8856.c
+++ b/drivers/media/i2c/ov8856.c
@@ -3,10 +3,13 @@
#include <asm/unaligned.h>
#include <linux/acpi.h>
+#include <linux/clk.h>
#include <linux/delay.h>
+#include <linux/gpio/consumer.h>
#include <linux/i2c.h>
#include <linux/module.h>
#include <linux/pm_runtime.h>
+#include <linux/regulator/consumer.h>
#include <media/v4l2-ctrls.h>
#include <media/v4l2-device.h>
#include <media/v4l2-fwnode.h>
@@ -19,6 +22,8 @@
#define OV8856_LINK_FREQ_180MHZ 180000000ULL
#define OV8856_SCLK 144000000ULL
#define OV8856_MCLK 19200000
+#define OV8856_XVCLK_19_2 19200000
+#define OV8856_XVCLK_24 24000000
#define OV8856_DATA_LANES 4
#define OV8856_RGB_DEPTH 10
@@ -64,6 +69,12 @@
#define to_ov8856(_sd) container_of(_sd, struct ov8856, sd)
+static const char * const ov8856_supply_names[] = {
+ "dovdd", /* Digital I/O power */
+ "avdd", /* Analog power */
+ "dvdd", /* Digital core power */
+};
+
enum {
OV8856_LINK_FREQ_720MBPS,
OV8856_LINK_FREQ_360MBPS,
@@ -566,6 +577,10 @@ struct ov8856 {
struct media_pad pad;
struct v4l2_ctrl_handler ctrl_handler;
+ struct clk *xvclk;
+ struct gpio_desc *reset_gpio;
+ struct regulator_bulk_data supplies[ARRAY_SIZE(ov8856_supply_names)];
+
/* V4L2 Controls */
struct v4l2_ctrl *link_freq;
struct v4l2_ctrl *pixel_rate;
@@ -908,6 +923,46 @@ static int ov8856_set_stream(struct v4l2_subdev *sd, int enable)
return ret;
}
+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);
+
+ return 0;
+
+disable_clk:
+ clk_disable_unprepare(ov8856->xvclk);
+
+ return ret;
+}
+
+static void __ov8856_power_off(struct ov8856 *ov8856)
+{
+ gpiod_set_value_cansleep(ov8856->reset_gpio, GPIOD_OUT_HIGH);
+ regulator_bulk_disable(ARRAY_SIZE(ov8856_supply_names),
+ ov8856->supplies);
+ clk_disable_unprepare(ov8856->xvclk);
+}
+
static int __maybe_unused ov8856_suspend(struct device *dev)
{
struct i2c_client *client = to_i2c_client(dev);
@@ -1175,7 +1230,7 @@ static int ov8856_remove(struct i2c_client *client)
static int ov8856_probe(struct i2c_client *client)
{
struct ov8856 *ov8856;
- int ret;
+ int i, ret;
ret = ov8856_check_hwcfg(&client->dev);
if (ret) {
@@ -1189,10 +1244,50 @@ static int ov8856_probe(struct i2c_client *client)
return -ENOMEM;
v4l2_i2c_subdev_init(&ov8856->sd, client, &ov8856_subdev_ops);
+ ov8856->xvclk = devm_clk_get(&client->dev, "xvclk");
+ if (PTR_ERR(ov8856->xvclk) == -ENOENT) {
+ dev_info(&client->dev, "xvclk clock not defined, continuing...\n");
+ 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);
+ if (ret < 0) {
+ dev_err(&client->dev, "failed to set xvclk rate (24MHz)\n");
+ return ret;
+ }
+
+ ov8856->reset_gpio = devm_gpiod_get(&client->dev, "reset",
+ GPIOD_OUT_HIGH);
+ if (IS_ERR(ov8856->reset_gpio)) {
+ dev_err(&client->dev, "failed to get reset-gpios\n");
+ return PTR_ERR(ov8856->reset_gpio);
+ }
+
+ for (i = 0; i < ARRAY_SIZE(ov8856_supply_names); i++)
+ ov8856->supplies[i].supply = ov8856_supply_names[i];
+
+ ret = devm_regulator_bulk_get(&client->dev,
+ ARRAY_SIZE(ov8856_supply_names),
+ ov8856->supplies);
+ if (ret) {
+ dev_warn(&client->dev, "failed to get regulators\n");
+ return ret;
+ }
+
+ ret = __ov8856_power_on(ov8856);
+ if (ret) {
+ dev_warn(&client->dev, "failed to power on\n");
+ return ret;
+ }
+
ret = ov8856_identify_module(ov8856);
if (ret) {
dev_err(&client->dev, "failed to find sensor: %d", ret);
- return ret;
+ goto probe_power_off;
}
mutex_init(&ov8856->mutex);
@@ -1238,6 +1333,9 @@ static int ov8856_probe(struct i2c_client *client)
v4l2_ctrl_handler_free(ov8856->sd.ctrl_handler);
mutex_destroy(&ov8856->mutex);
+probe_power_off:
+ __ov8856_power_off(ov8856);
+
return ret;
}
@@ -1254,11 +1352,18 @@ static const struct acpi_device_id ov8856_acpi_ids[] = {
MODULE_DEVICE_TABLE(acpi, ov8856_acpi_ids);
#endif
+static const struct of_device_id ov8856_of_match[] = {
+ { .compatible = "ovti,ov8856" },
+ { /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, ov8856_of_match);
+
static struct i2c_driver ov8856_i2c_driver = {
.driver = {
.name = "ov8856",
.pm = &ov8856_pm_ops,
.acpi_match_table = ACPI_PTR(ov8856_acpi_ids),
+ .of_match_table = ov8856_of_match,
},
.probe_new = ov8856_probe,
.remove = ov8856_remove,
--
2.20.1
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related [flat|nested] 34+ messages in thread* Re: [v2 2/3] media: ov8856: Add devicetree support
2020-03-13 11:03 ` Robert Foss
@ 2020-03-13 12:17 ` Sakari Ailus
-1 siblings, 0 replies; 34+ messages in thread
From: Sakari Ailus @ 2020-03-13 12:17 UTC (permalink / raw)
To: Robert Foss
Cc: Dongchun Zhu, Fabio Estevam, Andy Shevchenko, Tomasz Figa,
linux-media, devicetree, linux-kernel, linux-arm-kernel
Hi Robert,
On Fri, Mar 13, 2020 at 12:03:49PM +0100, Robert Foss wrote:
> Add devicetree match table, and enable ov8856_probe()
> to initialize power, clocks and reset pins.
>
> Signed-off-by: Robert Foss <robert.foss@linaro.org>
> ---
>
> - Changes since v1:
> * Fabio: Change n_shutdown_gpio name to reset_gpio
> * Fabio: Invert reset_gpio due to GPIO_ACTIVE_HIGH -> GPIO_ACTIVE_LOW change
> * Fabio: Remove empty line
> * Fabio: Remove real error from devm_gpiod_get() failures
> * Andy & Sakari: Make XVCLK optional since to not break ACPI
> * Sakari: ARRAY_SIZE() directly instead of through OV8856_NUM_SUPPLIES
> * Sakari: Use XVCLK rate as provided by DT
>
> drivers/media/i2c/ov8856.c | 109 ++++++++++++++++++++++++++++++++++++-
> 1 file changed, 107 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/media/i2c/ov8856.c b/drivers/media/i2c/ov8856.c
> index 8655842af275..db61eed223e8 100644
> --- a/drivers/media/i2c/ov8856.c
> +++ b/drivers/media/i2c/ov8856.c
> @@ -3,10 +3,13 @@
>
> #include <asm/unaligned.h>
> #include <linux/acpi.h>
> +#include <linux/clk.h>
> #include <linux/delay.h>
> +#include <linux/gpio/consumer.h>
> #include <linux/i2c.h>
> #include <linux/module.h>
> #include <linux/pm_runtime.h>
> +#include <linux/regulator/consumer.h>
> #include <media/v4l2-ctrls.h>
> #include <media/v4l2-device.h>
> #include <media/v4l2-fwnode.h>
> @@ -19,6 +22,8 @@
> #define OV8856_LINK_FREQ_180MHZ 180000000ULL
> #define OV8856_SCLK 144000000ULL
> #define OV8856_MCLK 19200000
> +#define OV8856_XVCLK_19_2 19200000
Please use a single macro to refer to 19,2 MHz clock.
> +#define OV8856_XVCLK_24 24000000
This doesn't seem to be needed
> #define OV8856_DATA_LANES 4
> #define OV8856_RGB_DEPTH 10
>
> @@ -64,6 +69,12 @@
>
> #define to_ov8856(_sd) container_of(_sd, struct ov8856, sd)
>
> +static const char * const ov8856_supply_names[] = {
> + "dovdd", /* Digital I/O power */
> + "avdd", /* Analog power */
> + "dvdd", /* Digital core power */
> +};
> +
> enum {
> OV8856_LINK_FREQ_720MBPS,
> OV8856_LINK_FREQ_360MBPS,
> @@ -566,6 +577,10 @@ struct ov8856 {
> struct media_pad pad;
> struct v4l2_ctrl_handler ctrl_handler;
>
> + struct clk *xvclk;
> + struct gpio_desc *reset_gpio;
> + struct regulator_bulk_data supplies[ARRAY_SIZE(ov8856_supply_names)];
> +
> /* V4L2 Controls */
> struct v4l2_ctrl *link_freq;
> struct v4l2_ctrl *pixel_rate;
> @@ -908,6 +923,46 @@ static int ov8856_set_stream(struct v4l2_subdev *sd, int enable)
> return ret;
> }
>
> +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.
> +
> + return 0;
> +
> +disable_clk:
How about the GPIO here?
> + clk_disable_unprepare(ov8856->xvclk);
> +
> + return ret;
> +}
> +
> +static void __ov8856_power_off(struct ov8856 *ov8856)
> +{
> + gpiod_set_value_cansleep(ov8856->reset_gpio, GPIOD_OUT_HIGH);
> + regulator_bulk_disable(ARRAY_SIZE(ov8856_supply_names),
> + ov8856->supplies);
> + clk_disable_unprepare(ov8856->xvclk);
> +}
You'll need to call the two in the driver's suspend and resume functions.
> +
> static int __maybe_unused ov8856_suspend(struct device *dev)
> {
> struct i2c_client *client = to_i2c_client(dev);
> @@ -1175,7 +1230,7 @@ static int ov8856_remove(struct i2c_client *client)
> static int ov8856_probe(struct i2c_client *client)
> {
> struct ov8856 *ov8856;
> - int ret;
> + int i, ret;
unsigned int?
>
> ret = ov8856_check_hwcfg(&client->dev);
> if (ret) {
> @@ -1189,10 +1244,50 @@ static int ov8856_probe(struct i2c_client *client)
> return -ENOMEM;
>
> v4l2_i2c_subdev_init(&ov8856->sd, client, &ov8856_subdev_ops);
> + ov8856->xvclk = devm_clk_get(&client->dev, "xvclk");
> + if (PTR_ERR(ov8856->xvclk) == -ENOENT) {
> + dev_info(&client->dev, "xvclk clock not defined, continuing...\n");
How about dev_dbg()?
> + 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.
> + if (ret < 0) {
> + dev_err(&client->dev, "failed to set xvclk rate (24MHz)\n");
> + return ret;
> + }
> +
> + ov8856->reset_gpio = devm_gpiod_get(&client->dev, "reset",
> + GPIOD_OUT_HIGH);
Indentation.
What if no gpio is defined?
> + if (IS_ERR(ov8856->reset_gpio)) {
> + dev_err(&client->dev, "failed to get reset-gpios\n");
> + return PTR_ERR(ov8856->reset_gpio);
> + }
> +
> + for (i = 0; i < ARRAY_SIZE(ov8856_supply_names); i++)
> + ov8856->supplies[i].supply = ov8856_supply_names[i];
> +
> + ret = devm_regulator_bulk_get(&client->dev,
> + ARRAY_SIZE(ov8856_supply_names),
> + ov8856->supplies);
What happens if there are no regulators?
> + if (ret) {
> + dev_warn(&client->dev, "failed to get regulators\n");
> + return ret;
> + }
> +
> + ret = __ov8856_power_on(ov8856);
> + if (ret) {
> + dev_warn(&client->dev, "failed to power on\n");
> + return ret;
> + }
> +
> ret = ov8856_identify_module(ov8856);
> if (ret) {
> dev_err(&client->dev, "failed to find sensor: %d", ret);
> - return ret;
> + goto probe_power_off;
> }
>
> mutex_init(&ov8856->mutex);
> @@ -1238,6 +1333,9 @@ static int ov8856_probe(struct i2c_client *client)
> v4l2_ctrl_handler_free(ov8856->sd.ctrl_handler);
> mutex_destroy(&ov8856->mutex);
>
> +probe_power_off:
> + __ov8856_power_off(ov8856);
> +
Also remember to power off the device in remove().
> return ret;
> }
>
> @@ -1254,11 +1352,18 @@ static const struct acpi_device_id ov8856_acpi_ids[] = {
> MODULE_DEVICE_TABLE(acpi, ov8856_acpi_ids);
> #endif
>
> +static const struct of_device_id ov8856_of_match[] = {
> + { .compatible = "ovti,ov8856" },
> + { /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(of, ov8856_of_match);
> +
> static struct i2c_driver ov8856_i2c_driver = {
> .driver = {
> .name = "ov8856",
> .pm = &ov8856_pm_ops,
> .acpi_match_table = ACPI_PTR(ov8856_acpi_ids),
> + .of_match_table = ov8856_of_match,
> },
> .probe_new = ov8856_probe,
> .remove = ov8856_remove,
--
Regards,
Sakari Ailus
^ permalink raw reply [flat|nested] 34+ messages in thread* Re: [v2 2/3] media: ov8856: Add devicetree support
@ 2020-03-13 12:17 ` Sakari Ailus
0 siblings, 0 replies; 34+ messages in thread
From: Sakari Ailus @ 2020-03-13 12:17 UTC (permalink / raw)
To: Robert Foss
Cc: devicetree, linux-kernel, Tomasz Figa, Dongchun Zhu,
Andy Shevchenko, Fabio Estevam, linux-arm-kernel, linux-media
Hi Robert,
On Fri, Mar 13, 2020 at 12:03:49PM +0100, Robert Foss wrote:
> Add devicetree match table, and enable ov8856_probe()
> to initialize power, clocks and reset pins.
>
> Signed-off-by: Robert Foss <robert.foss@linaro.org>
> ---
>
> - Changes since v1:
> * Fabio: Change n_shutdown_gpio name to reset_gpio
> * Fabio: Invert reset_gpio due to GPIO_ACTIVE_HIGH -> GPIO_ACTIVE_LOW change
> * Fabio: Remove empty line
> * Fabio: Remove real error from devm_gpiod_get() failures
> * Andy & Sakari: Make XVCLK optional since to not break ACPI
> * Sakari: ARRAY_SIZE() directly instead of through OV8856_NUM_SUPPLIES
> * Sakari: Use XVCLK rate as provided by DT
>
> drivers/media/i2c/ov8856.c | 109 ++++++++++++++++++++++++++++++++++++-
> 1 file changed, 107 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/media/i2c/ov8856.c b/drivers/media/i2c/ov8856.c
> index 8655842af275..db61eed223e8 100644
> --- a/drivers/media/i2c/ov8856.c
> +++ b/drivers/media/i2c/ov8856.c
> @@ -3,10 +3,13 @@
>
> #include <asm/unaligned.h>
> #include <linux/acpi.h>
> +#include <linux/clk.h>
> #include <linux/delay.h>
> +#include <linux/gpio/consumer.h>
> #include <linux/i2c.h>
> #include <linux/module.h>
> #include <linux/pm_runtime.h>
> +#include <linux/regulator/consumer.h>
> #include <media/v4l2-ctrls.h>
> #include <media/v4l2-device.h>
> #include <media/v4l2-fwnode.h>
> @@ -19,6 +22,8 @@
> #define OV8856_LINK_FREQ_180MHZ 180000000ULL
> #define OV8856_SCLK 144000000ULL
> #define OV8856_MCLK 19200000
> +#define OV8856_XVCLK_19_2 19200000
Please use a single macro to refer to 19,2 MHz clock.
> +#define OV8856_XVCLK_24 24000000
This doesn't seem to be needed
> #define OV8856_DATA_LANES 4
> #define OV8856_RGB_DEPTH 10
>
> @@ -64,6 +69,12 @@
>
> #define to_ov8856(_sd) container_of(_sd, struct ov8856, sd)
>
> +static const char * const ov8856_supply_names[] = {
> + "dovdd", /* Digital I/O power */
> + "avdd", /* Analog power */
> + "dvdd", /* Digital core power */
> +};
> +
> enum {
> OV8856_LINK_FREQ_720MBPS,
> OV8856_LINK_FREQ_360MBPS,
> @@ -566,6 +577,10 @@ struct ov8856 {
> struct media_pad pad;
> struct v4l2_ctrl_handler ctrl_handler;
>
> + struct clk *xvclk;
> + struct gpio_desc *reset_gpio;
> + struct regulator_bulk_data supplies[ARRAY_SIZE(ov8856_supply_names)];
> +
> /* V4L2 Controls */
> struct v4l2_ctrl *link_freq;
> struct v4l2_ctrl *pixel_rate;
> @@ -908,6 +923,46 @@ static int ov8856_set_stream(struct v4l2_subdev *sd, int enable)
> return ret;
> }
>
> +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.
> +
> + return 0;
> +
> +disable_clk:
How about the GPIO here?
> + clk_disable_unprepare(ov8856->xvclk);
> +
> + return ret;
> +}
> +
> +static void __ov8856_power_off(struct ov8856 *ov8856)
> +{
> + gpiod_set_value_cansleep(ov8856->reset_gpio, GPIOD_OUT_HIGH);
> + regulator_bulk_disable(ARRAY_SIZE(ov8856_supply_names),
> + ov8856->supplies);
> + clk_disable_unprepare(ov8856->xvclk);
> +}
You'll need to call the two in the driver's suspend and resume functions.
> +
> static int __maybe_unused ov8856_suspend(struct device *dev)
> {
> struct i2c_client *client = to_i2c_client(dev);
> @@ -1175,7 +1230,7 @@ static int ov8856_remove(struct i2c_client *client)
> static int ov8856_probe(struct i2c_client *client)
> {
> struct ov8856 *ov8856;
> - int ret;
> + int i, ret;
unsigned int?
>
> ret = ov8856_check_hwcfg(&client->dev);
> if (ret) {
> @@ -1189,10 +1244,50 @@ static int ov8856_probe(struct i2c_client *client)
> return -ENOMEM;
>
> v4l2_i2c_subdev_init(&ov8856->sd, client, &ov8856_subdev_ops);
> + ov8856->xvclk = devm_clk_get(&client->dev, "xvclk");
> + if (PTR_ERR(ov8856->xvclk) == -ENOENT) {
> + dev_info(&client->dev, "xvclk clock not defined, continuing...\n");
How about dev_dbg()?
> + 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.
> + if (ret < 0) {
> + dev_err(&client->dev, "failed to set xvclk rate (24MHz)\n");
> + return ret;
> + }
> +
> + ov8856->reset_gpio = devm_gpiod_get(&client->dev, "reset",
> + GPIOD_OUT_HIGH);
Indentation.
What if no gpio is defined?
> + if (IS_ERR(ov8856->reset_gpio)) {
> + dev_err(&client->dev, "failed to get reset-gpios\n");
> + return PTR_ERR(ov8856->reset_gpio);
> + }
> +
> + for (i = 0; i < ARRAY_SIZE(ov8856_supply_names); i++)
> + ov8856->supplies[i].supply = ov8856_supply_names[i];
> +
> + ret = devm_regulator_bulk_get(&client->dev,
> + ARRAY_SIZE(ov8856_supply_names),
> + ov8856->supplies);
What happens if there are no regulators?
> + if (ret) {
> + dev_warn(&client->dev, "failed to get regulators\n");
> + return ret;
> + }
> +
> + ret = __ov8856_power_on(ov8856);
> + if (ret) {
> + dev_warn(&client->dev, "failed to power on\n");
> + return ret;
> + }
> +
> ret = ov8856_identify_module(ov8856);
> if (ret) {
> dev_err(&client->dev, "failed to find sensor: %d", ret);
> - return ret;
> + goto probe_power_off;
> }
>
> mutex_init(&ov8856->mutex);
> @@ -1238,6 +1333,9 @@ static int ov8856_probe(struct i2c_client *client)
> v4l2_ctrl_handler_free(ov8856->sd.ctrl_handler);
> mutex_destroy(&ov8856->mutex);
>
> +probe_power_off:
> + __ov8856_power_off(ov8856);
> +
Also remember to power off the device in remove().
> return ret;
> }
>
> @@ -1254,11 +1352,18 @@ static const struct acpi_device_id ov8856_acpi_ids[] = {
> MODULE_DEVICE_TABLE(acpi, ov8856_acpi_ids);
> #endif
>
> +static const struct of_device_id ov8856_of_match[] = {
> + { .compatible = "ovti,ov8856" },
> + { /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(of, ov8856_of_match);
> +
> static struct i2c_driver ov8856_i2c_driver = {
> .driver = {
> .name = "ov8856",
> .pm = &ov8856_pm_ops,
> .acpi_match_table = ACPI_PTR(ov8856_acpi_ids),
> + .of_match_table = ov8856_of_match,
> },
> .probe_new = ov8856_probe,
> .remove = ov8856_remove,
--
Regards,
Sakari Ailus
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 34+ messages in thread* Re: [v2 2/3] media: ov8856: Add devicetree support
2020-03-13 12:17 ` Sakari Ailus
@ 2020-03-26 11:56 ` Robert Foss
-1 siblings, 0 replies; 34+ messages in thread
From: Robert Foss @ 2020-03-26 11:56 UTC (permalink / raw)
To: Sakari Ailus
Cc: Dongchun Zhu, Fabio Estevam, Andy Shevchenko, Tomasz Figa,
linux-media,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
linux-kernel,
moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE
Hey Sakari,
On Fri, 13 Mar 2020 at 13:18, Sakari Ailus <sakari.ailus@iki.fi> wrote:
>
> Hi Robert,
>
> On Fri, Mar 13, 2020 at 12:03:49PM +0100, Robert Foss wrote:
> > Add devicetree match table, and enable ov8856_probe()
> > to initialize power, clocks and reset pins.
> >
> > Signed-off-by: Robert Foss <robert.foss@linaro.org>
> > ---
> >
> > - Changes since v1:
> > * Fabio: Change n_shutdown_gpio name to reset_gpio
> > * Fabio: Invert reset_gpio due to GPIO_ACTIVE_HIGH -> GPIO_ACTIVE_LOW change
> > * Fabio: Remove empty line
> > * Fabio: Remove real error from devm_gpiod_get() failures
> > * Andy & Sakari: Make XVCLK optional since to not break ACPI
> > * Sakari: ARRAY_SIZE() directly instead of through OV8856_NUM_SUPPLIES
> > * Sakari: Use XVCLK rate as provided by DT
> >
> > drivers/media/i2c/ov8856.c | 109 ++++++++++++++++++++++++++++++++++++-
> > 1 file changed, 107 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/media/i2c/ov8856.c b/drivers/media/i2c/ov8856.c
> > index 8655842af275..db61eed223e8 100644
> > --- a/drivers/media/i2c/ov8856.c
> > +++ b/drivers/media/i2c/ov8856.c
> > @@ -3,10 +3,13 @@
> >
> > #include <asm/unaligned.h>
> > #include <linux/acpi.h>
> > +#include <linux/clk.h>
> > #include <linux/delay.h>
> > +#include <linux/gpio/consumer.h>
> > #include <linux/i2c.h>
> > #include <linux/module.h>
> > #include <linux/pm_runtime.h>
> > +#include <linux/regulator/consumer.h>
> > #include <media/v4l2-ctrls.h>
> > #include <media/v4l2-device.h>
> > #include <media/v4l2-fwnode.h>
> > @@ -19,6 +22,8 @@
> > #define OV8856_LINK_FREQ_180MHZ 180000000ULL
> > #define OV8856_SCLK 144000000ULL
> > #define OV8856_MCLK 19200000
> > +#define OV8856_XVCLK_19_2 19200000
>
> Please use a single macro to refer to 19,2 MHz clock.
Alright, I'll combine the two into a macro for both.
>
> > +#define OV8856_XVCLK_24 24000000
>
> This doesn't seem to be needed
As long as we don't set the clock to 24Mhz we don't.
I'm assuming that you're saying that the 24Mhz clock rate isn't needed
for the modes used currently.
Removing this macro in v3.
>
> > #define OV8856_DATA_LANES 4
> > #define OV8856_RGB_DEPTH 10
> >
> > @@ -64,6 +69,12 @@
> >
> > #define to_ov8856(_sd) container_of(_sd, struct ov8856, sd)
> >
> > +static const char * const ov8856_supply_names[] = {
> > + "dovdd", /* Digital I/O power */
> > + "avdd", /* Analog power */
> > + "dvdd", /* Digital core power */
> > +};
> > +
> > enum {
> > OV8856_LINK_FREQ_720MBPS,
> > OV8856_LINK_FREQ_360MBPS,
> > @@ -566,6 +577,10 @@ struct ov8856 {
> > struct media_pad pad;
> > struct v4l2_ctrl_handler ctrl_handler;
> >
> > + struct clk *xvclk;
> > + struct gpio_desc *reset_gpio;
> > + struct regulator_bulk_data supplies[ARRAY_SIZE(ov8856_supply_names)];
> > +
> > /* V4L2 Controls */
> > struct v4l2_ctrl *link_freq;
> > struct v4l2_ctrl *pixel_rate;
> > @@ -908,6 +923,46 @@ static int ov8856_set_stream(struct v4l2_subdev *sd, int enable)
> > return ret;
> > }
> >
> > +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.
>
> > +
> > + return 0;
> > +
> > +disable_clk:
>
> How about the GPIO here?
Ack
>
> > + clk_disable_unprepare(ov8856->xvclk);
> > +
> > + return ret;
> > +}
> > +
> > +static void __ov8856_power_off(struct ov8856 *ov8856)
> > +{
> > + gpiod_set_value_cansleep(ov8856->reset_gpio, GPIOD_OUT_HIGH);
> > + regulator_bulk_disable(ARRAY_SIZE(ov8856_supply_names),
> > + ov8856->supplies);
> > + clk_disable_unprepare(ov8856->xvclk);
> > +}
>
> You'll need to call the two in the driver's suspend and resume functions.
Ack
>
> > +
> > static int __maybe_unused ov8856_suspend(struct device *dev)
> > {
> > struct i2c_client *client = to_i2c_client(dev);
> > @@ -1175,7 +1230,7 @@ static int ov8856_remove(struct i2c_client *client)
> > static int ov8856_probe(struct i2c_client *client)
> > {
> > struct ov8856 *ov8856;
> > - int ret;
> > + int i, ret;
>
> unsigned int?
Ack
>
> >
> > ret = ov8856_check_hwcfg(&client->dev);
> > if (ret) {
> > @@ -1189,10 +1244,50 @@ static int ov8856_probe(struct i2c_client *client)
> > return -ENOMEM;
> >
> > v4l2_i2c_subdev_init(&ov8856->sd, client, &ov8856_subdev_ops);
> > + ov8856->xvclk = devm_clk_get(&client->dev, "xvclk");
> > + if (PTR_ERR(ov8856->xvclk) == -ENOENT) {
> > + dev_info(&client->dev, "xvclk clock not defined, continuing...\n");
>
> How about dev_dbg()?
Ack.
>
> > + 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.
>
> > + if (ret < 0) {
> > + dev_err(&client->dev, "failed to set xvclk rate (24MHz)\n");
> > + return ret;
> > + }
> > +
> > + ov8856->reset_gpio = devm_gpiod_get(&client->dev, "reset",
> > + GPIOD_OUT_HIGH);
>
> Indentation.
Ack.
>
> What if no gpio is defined?
As per Andys comments, I'll switch to the optional version of devm_gpiod_get().
>
> > + if (IS_ERR(ov8856->reset_gpio)) {
> > + dev_err(&client->dev, "failed to get reset-gpios\n");
> > + return PTR_ERR(ov8856->reset_gpio);
> > + }
> > +
> > + for (i = 0; i < ARRAY_SIZE(ov8856_supply_names); i++)
> > + ov8856->supplies[i].supply = ov8856_supply_names[i];
> > +
> > + ret = devm_regulator_bulk_get(&client->dev,
> > + ARRAY_SIZE(ov8856_supply_names),
> > + ov8856->supplies);
>
> What happens if there are no regulators?
Like Andy mentioned, we should be alright, since
devm_regulator_bulk_get() creates dummy regulators if one isn't found.
>
> > + if (ret) {
> > + dev_warn(&client->dev, "failed to get regulators\n");
> > + return ret;
> > + }
> > +
> > + ret = __ov8856_power_on(ov8856);
> > + if (ret) {
> > + dev_warn(&client->dev, "failed to power on\n");
> > + return ret;
> > + }
> > +
> > ret = ov8856_identify_module(ov8856);
> > if (ret) {
> > dev_err(&client->dev, "failed to find sensor: %d", ret);
> > - return ret;
> > + goto probe_power_off;
> > }
> >
> > mutex_init(&ov8856->mutex);
> > @@ -1238,6 +1333,9 @@ static int ov8856_probe(struct i2c_client *client)
> > v4l2_ctrl_handler_free(ov8856->sd.ctrl_handler);
> > mutex_destroy(&ov8856->mutex);
> >
> > +probe_power_off:
> > + __ov8856_power_off(ov8856);
> > +
>
> Also remember to power off the device in remove().
>
Ack
> > return ret;
> > }
> >
> > @@ -1254,11 +1352,18 @@ static const struct acpi_device_id ov8856_acpi_ids[] = {
> > MODULE_DEVICE_TABLE(acpi, ov8856_acpi_ids);
> > #endif
> >
> > +static const struct of_device_id ov8856_of_match[] = {
> > + { .compatible = "ovti,ov8856" },
> > + { /* sentinel */ }
> > +};
> > +MODULE_DEVICE_TABLE(of, ov8856_of_match);
> > +
> > static struct i2c_driver ov8856_i2c_driver = {
> > .driver = {
> > .name = "ov8856",
> > .pm = &ov8856_pm_ops,
> > .acpi_match_table = ACPI_PTR(ov8856_acpi_ids),
> > + .of_match_table = ov8856_of_match,
> > },
> > .probe_new = ov8856_probe,
> > .remove = ov8856_remove,
>
> --
> Regards,
>
> Sakari Ailus
^ permalink raw reply [flat|nested] 34+ messages in thread* Re: [v2 2/3] media: ov8856: Add devicetree support
@ 2020-03-26 11:56 ` Robert Foss
0 siblings, 0 replies; 34+ messages in thread
From: Robert Foss @ 2020-03-26 11:56 UTC (permalink / raw)
To: Sakari Ailus
Cc: open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
linux-kernel, Tomasz Figa, Dongchun Zhu, Andy Shevchenko,
Fabio Estevam,
moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE,
linux-media
Hey Sakari,
On Fri, 13 Mar 2020 at 13:18, Sakari Ailus <sakari.ailus@iki.fi> wrote:
>
> Hi Robert,
>
> On Fri, Mar 13, 2020 at 12:03:49PM +0100, Robert Foss wrote:
> > Add devicetree match table, and enable ov8856_probe()
> > to initialize power, clocks and reset pins.
> >
> > Signed-off-by: Robert Foss <robert.foss@linaro.org>
> > ---
> >
> > - Changes since v1:
> > * Fabio: Change n_shutdown_gpio name to reset_gpio
> > * Fabio: Invert reset_gpio due to GPIO_ACTIVE_HIGH -> GPIO_ACTIVE_LOW change
> > * Fabio: Remove empty line
> > * Fabio: Remove real error from devm_gpiod_get() failures
> > * Andy & Sakari: Make XVCLK optional since to not break ACPI
> > * Sakari: ARRAY_SIZE() directly instead of through OV8856_NUM_SUPPLIES
> > * Sakari: Use XVCLK rate as provided by DT
> >
> > drivers/media/i2c/ov8856.c | 109 ++++++++++++++++++++++++++++++++++++-
> > 1 file changed, 107 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/media/i2c/ov8856.c b/drivers/media/i2c/ov8856.c
> > index 8655842af275..db61eed223e8 100644
> > --- a/drivers/media/i2c/ov8856.c
> > +++ b/drivers/media/i2c/ov8856.c
> > @@ -3,10 +3,13 @@
> >
> > #include <asm/unaligned.h>
> > #include <linux/acpi.h>
> > +#include <linux/clk.h>
> > #include <linux/delay.h>
> > +#include <linux/gpio/consumer.h>
> > #include <linux/i2c.h>
> > #include <linux/module.h>
> > #include <linux/pm_runtime.h>
> > +#include <linux/regulator/consumer.h>
> > #include <media/v4l2-ctrls.h>
> > #include <media/v4l2-device.h>
> > #include <media/v4l2-fwnode.h>
> > @@ -19,6 +22,8 @@
> > #define OV8856_LINK_FREQ_180MHZ 180000000ULL
> > #define OV8856_SCLK 144000000ULL
> > #define OV8856_MCLK 19200000
> > +#define OV8856_XVCLK_19_2 19200000
>
> Please use a single macro to refer to 19,2 MHz clock.
Alright, I'll combine the two into a macro for both.
>
> > +#define OV8856_XVCLK_24 24000000
>
> This doesn't seem to be needed
As long as we don't set the clock to 24Mhz we don't.
I'm assuming that you're saying that the 24Mhz clock rate isn't needed
for the modes used currently.
Removing this macro in v3.
>
> > #define OV8856_DATA_LANES 4
> > #define OV8856_RGB_DEPTH 10
> >
> > @@ -64,6 +69,12 @@
> >
> > #define to_ov8856(_sd) container_of(_sd, struct ov8856, sd)
> >
> > +static const char * const ov8856_supply_names[] = {
> > + "dovdd", /* Digital I/O power */
> > + "avdd", /* Analog power */
> > + "dvdd", /* Digital core power */
> > +};
> > +
> > enum {
> > OV8856_LINK_FREQ_720MBPS,
> > OV8856_LINK_FREQ_360MBPS,
> > @@ -566,6 +577,10 @@ struct ov8856 {
> > struct media_pad pad;
> > struct v4l2_ctrl_handler ctrl_handler;
> >
> > + struct clk *xvclk;
> > + struct gpio_desc *reset_gpio;
> > + struct regulator_bulk_data supplies[ARRAY_SIZE(ov8856_supply_names)];
> > +
> > /* V4L2 Controls */
> > struct v4l2_ctrl *link_freq;
> > struct v4l2_ctrl *pixel_rate;
> > @@ -908,6 +923,46 @@ static int ov8856_set_stream(struct v4l2_subdev *sd, int enable)
> > return ret;
> > }
> >
> > +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.
>
> > +
> > + return 0;
> > +
> > +disable_clk:
>
> How about the GPIO here?
Ack
>
> > + clk_disable_unprepare(ov8856->xvclk);
> > +
> > + return ret;
> > +}
> > +
> > +static void __ov8856_power_off(struct ov8856 *ov8856)
> > +{
> > + gpiod_set_value_cansleep(ov8856->reset_gpio, GPIOD_OUT_HIGH);
> > + regulator_bulk_disable(ARRAY_SIZE(ov8856_supply_names),
> > + ov8856->supplies);
> > + clk_disable_unprepare(ov8856->xvclk);
> > +}
>
> You'll need to call the two in the driver's suspend and resume functions.
Ack
>
> > +
> > static int __maybe_unused ov8856_suspend(struct device *dev)
> > {
> > struct i2c_client *client = to_i2c_client(dev);
> > @@ -1175,7 +1230,7 @@ static int ov8856_remove(struct i2c_client *client)
> > static int ov8856_probe(struct i2c_client *client)
> > {
> > struct ov8856 *ov8856;
> > - int ret;
> > + int i, ret;
>
> unsigned int?
Ack
>
> >
> > ret = ov8856_check_hwcfg(&client->dev);
> > if (ret) {
> > @@ -1189,10 +1244,50 @@ static int ov8856_probe(struct i2c_client *client)
> > return -ENOMEM;
> >
> > v4l2_i2c_subdev_init(&ov8856->sd, client, &ov8856_subdev_ops);
> > + ov8856->xvclk = devm_clk_get(&client->dev, "xvclk");
> > + if (PTR_ERR(ov8856->xvclk) == -ENOENT) {
> > + dev_info(&client->dev, "xvclk clock not defined, continuing...\n");
>
> How about dev_dbg()?
Ack.
>
> > + 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.
>
> > + if (ret < 0) {
> > + dev_err(&client->dev, "failed to set xvclk rate (24MHz)\n");
> > + return ret;
> > + }
> > +
> > + ov8856->reset_gpio = devm_gpiod_get(&client->dev, "reset",
> > + GPIOD_OUT_HIGH);
>
> Indentation.
Ack.
>
> What if no gpio is defined?
As per Andys comments, I'll switch to the optional version of devm_gpiod_get().
>
> > + if (IS_ERR(ov8856->reset_gpio)) {
> > + dev_err(&client->dev, "failed to get reset-gpios\n");
> > + return PTR_ERR(ov8856->reset_gpio);
> > + }
> > +
> > + for (i = 0; i < ARRAY_SIZE(ov8856_supply_names); i++)
> > + ov8856->supplies[i].supply = ov8856_supply_names[i];
> > +
> > + ret = devm_regulator_bulk_get(&client->dev,
> > + ARRAY_SIZE(ov8856_supply_names),
> > + ov8856->supplies);
>
> What happens if there are no regulators?
Like Andy mentioned, we should be alright, since
devm_regulator_bulk_get() creates dummy regulators if one isn't found.
>
> > + if (ret) {
> > + dev_warn(&client->dev, "failed to get regulators\n");
> > + return ret;
> > + }
> > +
> > + ret = __ov8856_power_on(ov8856);
> > + if (ret) {
> > + dev_warn(&client->dev, "failed to power on\n");
> > + return ret;
> > + }
> > +
> > ret = ov8856_identify_module(ov8856);
> > if (ret) {
> > dev_err(&client->dev, "failed to find sensor: %d", ret);
> > - return ret;
> > + goto probe_power_off;
> > }
> >
> > mutex_init(&ov8856->mutex);
> > @@ -1238,6 +1333,9 @@ static int ov8856_probe(struct i2c_client *client)
> > v4l2_ctrl_handler_free(ov8856->sd.ctrl_handler);
> > mutex_destroy(&ov8856->mutex);
> >
> > +probe_power_off:
> > + __ov8856_power_off(ov8856);
> > +
>
> Also remember to power off the device in remove().
>
Ack
> > return ret;
> > }
> >
> > @@ -1254,11 +1352,18 @@ static const struct acpi_device_id ov8856_acpi_ids[] = {
> > MODULE_DEVICE_TABLE(acpi, ov8856_acpi_ids);
> > #endif
> >
> > +static const struct of_device_id ov8856_of_match[] = {
> > + { .compatible = "ovti,ov8856" },
> > + { /* sentinel */ }
> > +};
> > +MODULE_DEVICE_TABLE(of, ov8856_of_match);
> > +
> > static struct i2c_driver ov8856_i2c_driver = {
> > .driver = {
> > .name = "ov8856",
> > .pm = &ov8856_pm_ops,
> > .acpi_match_table = ACPI_PTR(ov8856_acpi_ids),
> > + .of_match_table = ov8856_of_match,
> > },
> > .probe_new = ov8856_probe,
> > .remove = ov8856_remove,
>
> --
> Regards,
>
> Sakari Ailus
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 34+ messages in thread* Re: [v2 2/3] media: ov8856: Add devicetree support
2020-03-26 11:56 ` Robert Foss
@ 2020-03-26 14:47 ` Sakari Ailus
-1 siblings, 0 replies; 34+ messages in thread
From: Sakari Ailus @ 2020-03-26 14:47 UTC (permalink / raw)
To: Robert Foss
Cc: Dongchun Zhu, Fabio Estevam, Andy Shevchenko, Tomasz Figa,
linux-media,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
linux-kernel,
moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE
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.
...
> > > + 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().
As you get the actual rate, it could be somewhat off of the intended one.
--
Kind regards,
Sakari Ailus
^ permalink raw reply [flat|nested] 34+ messages in thread* Re: [v2 2/3] media: ov8856: Add devicetree support
@ 2020-03-26 14:47 ` Sakari Ailus
0 siblings, 0 replies; 34+ messages in thread
From: Sakari Ailus @ 2020-03-26 14:47 UTC (permalink / raw)
To: Robert Foss
Cc: open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
linux-kernel, Tomasz Figa, Dongchun Zhu, Andy Shevchenko,
Fabio Estevam,
moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE,
linux-media
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.
...
> > > + 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().
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
^ permalink raw reply [flat|nested] 34+ messages in thread* Re: [v2 2/3] media: ov8856: Add devicetree support
2020-03-26 14:47 ` Sakari Ailus
@ 2020-03-27 10:32 ` Robert Foss
-1 siblings, 0 replies; 34+ messages in thread
From: Robert Foss @ 2020-03-27 10:32 UTC (permalink / raw)
To: Sakari Ailus
Cc: Dongchun Zhu, Fabio Estevam, Andy Shevchenko, Tomasz Figa,
linux-media,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
linux-kernel,
moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE
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
^ permalink raw reply [flat|nested] 34+ messages in thread* Re: [v2 2/3] media: ov8856: Add devicetree support
@ 2020-03-27 10:32 ` Robert Foss
0 siblings, 0 replies; 34+ messages in thread
From: Robert Foss @ 2020-03-27 10:32 UTC (permalink / raw)
To: Sakari Ailus
Cc: open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
linux-kernel, Tomasz Figa, Dongchun Zhu, Andy Shevchenko,
Fabio Estevam,
moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE,
linux-media
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
^ permalink raw reply [flat|nested] 34+ messages in thread* Re: [v2 2/3] media: ov8856: Add devicetree support
2020-03-27 10:32 ` Robert Foss
@ 2020-03-27 13:37 ` Sakari Ailus
-1 siblings, 0 replies; 34+ messages in thread
From: Sakari Ailus @ 2020-03-27 13:37 UTC (permalink / raw)
To: Robert Foss
Cc: Dongchun Zhu, Fabio Estevam, Andy Shevchenko, Tomasz Figa,
linux-media,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
linux-kernel,
moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE
Hi Robert,
On Fri, Mar 27, 2020 at 11:32:29AM +0100, Robert Foss wrote:
> 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.
I guess the cleanest, easy way to make this right, albeit slightly
unoptimal in very rare cases where you have none of the above resources in
a DT system, is to bail out if you're running on an ACPI based system.
I.e. checking for e.g. is_acpi_node(dev->fwnode).
--
Sakari Ailus
^ permalink raw reply [flat|nested] 34+ messages in thread* Re: [v2 2/3] media: ov8856: Add devicetree support
@ 2020-03-27 13:37 ` Sakari Ailus
0 siblings, 0 replies; 34+ messages in thread
From: Sakari Ailus @ 2020-03-27 13:37 UTC (permalink / raw)
To: Robert Foss
Cc: open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
linux-kernel, Tomasz Figa, Dongchun Zhu, Andy Shevchenko,
Fabio Estevam,
moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE,
linux-media
Hi Robert,
On Fri, Mar 27, 2020 at 11:32:29AM +0100, Robert Foss wrote:
> 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.
I guess the cleanest, easy way to make this right, albeit slightly
unoptimal in very rare cases where you have none of the above resources in
a DT system, is to bail out if you're running on an ACPI based system.
I.e. checking for e.g. is_acpi_node(dev->fwnode).
--
Sakari Ailus
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [v2 2/3] media: ov8856: Add devicetree support
2020-03-13 11:03 ` Robert Foss
@ 2020-03-13 12:28 ` Andy Shevchenko
-1 siblings, 0 replies; 34+ messages in thread
From: Andy Shevchenko @ 2020-03-13 12:28 UTC (permalink / raw)
To: Robert Foss
Cc: Dongchun Zhu, Fabio Estevam, Sakari Ailus, Tomasz Figa,
linux-media, devicetree, linux-kernel, linux-arm-kernel
On Fri, Mar 13, 2020 at 12:03:49PM +0100, Robert Foss wrote:
> Add devicetree match table, and enable ov8856_probe()
> to initialize power, clocks and reset pins.
Thanks for an update.
My comments below.
...
> + ov8856->xvclk = devm_clk_get(&client->dev, "xvclk");
In many frameworks we have '_optional' variants of API. Please use it instead
of open coded approach.
> + if (PTR_ERR(ov8856->xvclk) == -ENOENT) {
> + dev_info(&client->dev, "xvclk clock not defined, continuing...\n");
> + 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);
> + if (ret < 0) {
> + dev_err(&client->dev, "failed to set xvclk rate (24MHz)\n");
> + return ret;
> + }
> +
> + ov8856->reset_gpio = devm_gpiod_get(&client->dev, "reset",
> + GPIOD_OUT_HIGH);
Same here.
> + if (IS_ERR(ov8856->reset_gpio)) {
> + dev_err(&client->dev, "failed to get reset-gpios\n");
> + return PTR_ERR(ov8856->reset_gpio);
> + }
> +
> + for (i = 0; i < ARRAY_SIZE(ov8856_supply_names); i++)
> + ov8856->supplies[i].supply = ov8856_supply_names[i];
> +
> + ret = devm_regulator_bulk_get(&client->dev,
> + ARRAY_SIZE(ov8856_supply_names),
> + ov8856->supplies);
Luckily regulator framework will create dummy ones if there is none found.
> + if (ret) {
> + dev_warn(&client->dev, "failed to get regulators\n");
> + return ret;
> + }
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 34+ messages in thread* Re: [v2 2/3] media: ov8856: Add devicetree support
@ 2020-03-13 12:28 ` Andy Shevchenko
0 siblings, 0 replies; 34+ messages in thread
From: Andy Shevchenko @ 2020-03-13 12:28 UTC (permalink / raw)
To: Robert Foss
Cc: devicetree, linux-kernel, Tomasz Figa, Sakari Ailus,
Dongchun Zhu, Fabio Estevam, linux-arm-kernel, linux-media
On Fri, Mar 13, 2020 at 12:03:49PM +0100, Robert Foss wrote:
> Add devicetree match table, and enable ov8856_probe()
> to initialize power, clocks and reset pins.
Thanks for an update.
My comments below.
...
> + ov8856->xvclk = devm_clk_get(&client->dev, "xvclk");
In many frameworks we have '_optional' variants of API. Please use it instead
of open coded approach.
> + if (PTR_ERR(ov8856->xvclk) == -ENOENT) {
> + dev_info(&client->dev, "xvclk clock not defined, continuing...\n");
> + 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);
> + if (ret < 0) {
> + dev_err(&client->dev, "failed to set xvclk rate (24MHz)\n");
> + return ret;
> + }
> +
> + ov8856->reset_gpio = devm_gpiod_get(&client->dev, "reset",
> + GPIOD_OUT_HIGH);
Same here.
> + if (IS_ERR(ov8856->reset_gpio)) {
> + dev_err(&client->dev, "failed to get reset-gpios\n");
> + return PTR_ERR(ov8856->reset_gpio);
> + }
> +
> + for (i = 0; i < ARRAY_SIZE(ov8856_supply_names); i++)
> + ov8856->supplies[i].supply = ov8856_supply_names[i];
> +
> + ret = devm_regulator_bulk_get(&client->dev,
> + ARRAY_SIZE(ov8856_supply_names),
> + ov8856->supplies);
Luckily regulator framework will create dummy ones if there is none found.
> + if (ret) {
> + dev_warn(&client->dev, "failed to get regulators\n");
> + 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
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [v2 2/3] media: ov8856: Add devicetree support
2020-03-13 11:03 ` Robert Foss
@ 2020-03-13 13:15 ` Fabio Estevam
-1 siblings, 0 replies; 34+ messages in thread
From: Fabio Estevam @ 2020-03-13 13:15 UTC (permalink / raw)
To: Robert Foss
Cc: Dongchun Zhu, Andy Shevchenko, Sakari Ailus, Tomasz Figa,
linux-media,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
linux-kernel,
moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE
Hi Robert,
On Fri, Mar 13, 2020 at 8:04 AM Robert Foss <robert.foss@linaro.org> 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);
The parameter of gpiod_set_value_cansleep() is typically 0 (inactive
state) or 1 (active state), so:
gpiod_set_value_cansleep(ov8856->reset_gpio, 1);
> +
> + 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);
and here it should be:
gpiod_set_value_cansleep(ov8856->reset_gpio, 0);
Also, don't you need a reset period between the two?
^ permalink raw reply [flat|nested] 34+ messages in thread* Re: [v2 2/3] media: ov8856: Add devicetree support
@ 2020-03-13 13:15 ` Fabio Estevam
0 siblings, 0 replies; 34+ messages in thread
From: Fabio Estevam @ 2020-03-13 13:15 UTC (permalink / raw)
To: Robert Foss
Cc: open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
linux-kernel, Tomasz Figa, Sakari Ailus, Dongchun Zhu,
Andy Shevchenko,
moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE,
linux-media
Hi Robert,
On Fri, Mar 13, 2020 at 8:04 AM Robert Foss <robert.foss@linaro.org> 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);
The parameter of gpiod_set_value_cansleep() is typically 0 (inactive
state) or 1 (active state), so:
gpiod_set_value_cansleep(ov8856->reset_gpio, 1);
> +
> + 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);
and here it should be:
gpiod_set_value_cansleep(ov8856->reset_gpio, 0);
Also, don't you need a reset period between the two?
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 34+ messages in thread* Re: [v2 2/3] media: ov8856: Add devicetree support
2020-03-13 13:15 ` Fabio Estevam
@ 2020-03-31 13:37 ` Robert Foss
-1 siblings, 0 replies; 34+ messages in thread
From: Robert Foss @ 2020-03-31 13:37 UTC (permalink / raw)
To: Fabio Estevam
Cc: Dongchun Zhu, Andy Shevchenko, Sakari Ailus, Tomasz Figa,
linux-media,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
linux-kernel,
moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE
On Thu, 26 Mar 2020 at 13:18, Robert Foss <robert.foss@linaro.org> wrote:
>
> Hey Fabio,
>
> On Fri, 13 Mar 2020 at 14:15, Fabio Estevam <festevam@gmail.com> wrote:
> >
> > Hi Robert,
> >
> > On Fri, Mar 13, 2020 at 8:04 AM Robert Foss <robert.foss@linaro.org> 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);
> >
> > The parameter of gpiod_set_value_cansleep() is typically 0 (inactive
> > state) or 1 (active state), so:
> >
> > gpiod_set_value_cansleep(ov8856->reset_gpio, 1);
>
> Ack
After testing this change, it breaks the driver during probing.
I had a quick look into GPIOD_OUT_HIGH & LOW definitions, and they
seem to never be 0 or 1.
https://elixir.bootlin.com/linux/latest/source/include/linux/gpio/consumer.h#L38
GPIOD_ASIS = 0,
GPIOD_IN = 1,
GPIOD_OUT_LOW = 3
GPIOD_OUT_HIGH = 7
^ permalink raw reply [flat|nested] 34+ messages in thread* Re: [v2 2/3] media: ov8856: Add devicetree support
@ 2020-03-31 13:37 ` Robert Foss
0 siblings, 0 replies; 34+ messages in thread
From: Robert Foss @ 2020-03-31 13:37 UTC (permalink / raw)
To: Fabio Estevam
Cc: open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
linux-kernel, Tomasz Figa, Sakari Ailus, Dongchun Zhu,
Andy Shevchenko,
moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE,
linux-media
On Thu, 26 Mar 2020 at 13:18, Robert Foss <robert.foss@linaro.org> wrote:
>
> Hey Fabio,
>
> On Fri, 13 Mar 2020 at 14:15, Fabio Estevam <festevam@gmail.com> wrote:
> >
> > Hi Robert,
> >
> > On Fri, Mar 13, 2020 at 8:04 AM Robert Foss <robert.foss@linaro.org> 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);
> >
> > The parameter of gpiod_set_value_cansleep() is typically 0 (inactive
> > state) or 1 (active state), so:
> >
> > gpiod_set_value_cansleep(ov8856->reset_gpio, 1);
>
> Ack
After testing this change, it breaks the driver during probing.
I had a quick look into GPIOD_OUT_HIGH & LOW definitions, and they
seem to never be 0 or 1.
https://elixir.bootlin.com/linux/latest/source/include/linux/gpio/consumer.h#L38
GPIOD_ASIS = 0,
GPIOD_IN = 1,
GPIOD_OUT_LOW = 3
GPIOD_OUT_HIGH = 7
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 34+ messages in thread* Re: [v2 2/3] media: ov8856: Add devicetree support
2020-03-31 13:37 ` Robert Foss
@ 2020-03-31 13:42 ` Fabio Estevam
-1 siblings, 0 replies; 34+ messages in thread
From: Fabio Estevam @ 2020-03-31 13:42 UTC (permalink / raw)
To: Robert Foss
Cc: Dongchun Zhu, Andy Shevchenko, Sakari Ailus, Tomasz Figa,
linux-media,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
linux-kernel,
moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE
Hi Robert,
On Tue, Mar 31, 2020 at 10:37 AM Robert Foss <robert.foss@linaro.org> wrote:
> After testing this change, it breaks the driver during probing.
Why exactly does it break probing? Maybe the GPIO polarity defined in
the device tree is wrong?
> I had a quick look into GPIOD_OUT_HIGH & LOW definitions, and they
> seem to never be 0 or 1.
If you do a grep in all gpiod_set_value_cansleep() usages in the
kernel tree, there is not a single case where GPIOD_OUT_HIGH or
GPIOD_OUT_LOW is passed as argument of gpiod_set_value_cansleep().
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [v2 2/3] media: ov8856: Add devicetree support
@ 2020-03-31 13:42 ` Fabio Estevam
0 siblings, 0 replies; 34+ messages in thread
From: Fabio Estevam @ 2020-03-31 13:42 UTC (permalink / raw)
To: Robert Foss
Cc: open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
linux-kernel, Tomasz Figa, Sakari Ailus, Dongchun Zhu,
Andy Shevchenko,
moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE,
linux-media
Hi Robert,
On Tue, Mar 31, 2020 at 10:37 AM Robert Foss <robert.foss@linaro.org> wrote:
> After testing this change, it breaks the driver during probing.
Why exactly does it break probing? Maybe the GPIO polarity defined in
the device tree is wrong?
> I had a quick look into GPIOD_OUT_HIGH & LOW definitions, and they
> seem to never be 0 or 1.
If you do a grep in all gpiod_set_value_cansleep() usages in the
kernel tree, there is not a single case where GPIOD_OUT_HIGH or
GPIOD_OUT_LOW is passed as argument of gpiod_set_value_cansleep().
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [v2 2/3] media: ov8856: Add devicetree support
2020-03-31 13:42 ` Fabio Estevam
@ 2020-03-31 13:53 ` Andy Shevchenko
-1 siblings, 0 replies; 34+ messages in thread
From: Andy Shevchenko @ 2020-03-31 13:53 UTC (permalink / raw)
To: Fabio Estevam
Cc: Robert Foss, Dongchun Zhu, Andy Shevchenko, Sakari Ailus,
Tomasz Figa, linux-media,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
linux-kernel,
moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE
On Tue, Mar 31, 2020 at 4:43 PM Fabio Estevam <festevam@gmail.com> wrote:
> On Tue, Mar 31, 2020 at 10:37 AM Robert Foss <robert.foss@linaro.org> wrote:
>
> > After testing this change, it breaks the driver during probing.
>
> Why exactly does it break probing? Maybe the GPIO polarity defined in
> the device tree is wrong?
>
> > I had a quick look into GPIOD_OUT_HIGH & LOW definitions, and they
> > seem to never be 0 or 1.
>
> If you do a grep in all gpiod_set_value_cansleep() usages in the
> kernel tree, there is not a single case where GPIOD_OUT_HIGH or
> GPIOD_OUT_LOW is passed as argument of gpiod_set_value_cansleep().
+1. It simple reveals the problem that is somewhere else.
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [v2 2/3] media: ov8856: Add devicetree support
@ 2020-03-31 13:53 ` Andy Shevchenko
0 siblings, 0 replies; 34+ messages in thread
From: Andy Shevchenko @ 2020-03-31 13:53 UTC (permalink / raw)
To: Fabio Estevam
Cc: open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
linux-kernel, Robert Foss, Tomasz Figa, Sakari Ailus,
Dongchun Zhu, Andy Shevchenko,
moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE,
linux-media
On Tue, Mar 31, 2020 at 4:43 PM Fabio Estevam <festevam@gmail.com> wrote:
> On Tue, Mar 31, 2020 at 10:37 AM Robert Foss <robert.foss@linaro.org> wrote:
>
> > After testing this change, it breaks the driver during probing.
>
> Why exactly does it break probing? Maybe the GPIO polarity defined in
> the device tree is wrong?
>
> > I had a quick look into GPIOD_OUT_HIGH & LOW definitions, and they
> > seem to never be 0 or 1.
>
> If you do a grep in all gpiod_set_value_cansleep() usages in the
> kernel tree, there is not a single case where GPIOD_OUT_HIGH or
> GPIOD_OUT_LOW is passed as argument of gpiod_set_value_cansleep().
+1. It simple reveals the problem that is somewhere else.
--
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
^ permalink raw reply [flat|nested] 34+ messages in thread