This patch switches the driver away from legacy gpio/of_gpio API to
gpiod API, and removes one of the last uses of of_get_gpio_flags().
Signed-off-by: Dmitry Torokhov <[email protected]>
---
drivers/media/i2c/s5k6a3.c | 30 +++++++++++-------------------
1 file changed, 11 insertions(+), 19 deletions(-)
diff --git a/drivers/media/i2c/s5k6a3.c b/drivers/media/i2c/s5k6a3.c
index a4efd6d10b43..ef6673b10580 100644
--- a/drivers/media/i2c/s5k6a3.c
+++ b/drivers/media/i2c/s5k6a3.c
@@ -9,12 +9,12 @@
#include <linux/clk.h>
#include <linux/delay.h>
#include <linux/device.h>
+#include <linux/err.h>
#include <linux/errno.h>
-#include <linux/gpio.h>
+#include <linux/gpio/consumer.h>
#include <linux/i2c.h>
#include <linux/kernel.h>
#include <linux/module.h>
-#include <linux/of_gpio.h>
#include <linux/pm_runtime.h>
#include <linux/regulator/consumer.h>
#include <linux/slab.h>
@@ -59,7 +59,7 @@ struct s5k6a3 {
struct v4l2_subdev subdev;
struct media_pad pad;
struct regulator_bulk_data supplies[S5K6A3_NUM_SUPPLIES];
- int gpio_reset;
+ struct gpio_desc *gpio_reset;
struct mutex lock;
struct v4l2_mbus_framefmt format;
struct clk *clock;
@@ -216,11 +216,11 @@ static int __s5k6a3_power_on(struct s5k6a3 *sensor)
goto error_clk;
}
- gpio_set_value(sensor->gpio_reset, 1);
+ gpiod_set_value_cansleep(sensor->gpio_reset, 0);
usleep_range(600, 800);
- gpio_set_value(sensor->gpio_reset, 0);
+ gpiod_set_value_cansleep(sensor->gpio_reset, 1);
usleep_range(600, 800);
- gpio_set_value(sensor->gpio_reset, 1);
+ gpiod_set_value_cansleep(sensor->gpio_reset, 0);
/* Delay needed for the sensor initialization */
msleep(20);
@@ -240,7 +240,7 @@ static int __s5k6a3_power_off(struct s5k6a3 *sensor)
{
int i;
- gpio_set_value(sensor->gpio_reset, 0);
+ gpiod_set_value_cansleep(sensor->gpio_reset, 1);
for (i = S5K6A3_NUM_SUPPLIES - 1; i >= 0; i--)
regulator_disable(sensor->supplies[i].consumer);
@@ -285,32 +285,24 @@ static int s5k6a3_probe(struct i2c_client *client)
struct device *dev = &client->dev;
struct s5k6a3 *sensor;
struct v4l2_subdev *sd;
- int gpio, i, ret;
+ int i, ret;
sensor = devm_kzalloc(dev, sizeof(*sensor), GFP_KERNEL);
if (!sensor)
return -ENOMEM;
mutex_init(&sensor->lock);
- sensor->gpio_reset = -EINVAL;
- sensor->clock = ERR_PTR(-EINVAL);
sensor->dev = dev;
sensor->clock = devm_clk_get(sensor->dev, S5K6A3_CLK_NAME);
if (IS_ERR(sensor->clock))
return PTR_ERR(sensor->clock);
- gpio = of_get_gpio_flags(dev->of_node, 0, NULL);
- if (!gpio_is_valid(gpio))
- return gpio;
-
- ret = devm_gpio_request_one(dev, gpio, GPIOF_OUT_INIT_LOW,
- S5K6A3_DRV_NAME);
- if (ret < 0)
+ sensor->gpio_reset = devm_gpiod_get(dev, NULL, GPIOD_OUT_HIGH);
+ ret = PTR_ERR_OR_ZERO(sensor->gpio_reset);
+ if (ret)
return ret;
- sensor->gpio_reset = gpio;
-
if (of_property_read_u32(dev->of_node, "clock-frequency",
&sensor->clock_frequency)) {
sensor->clock_frequency = S5K6A3_DEFAULT_CLK_FREQ;
--
2.38.1.431.g37b22c650d-goog
This patch switches the driver away from legacy gpio/of_gpio API to
gpiod API, and removes use of of_get_named_gpio_flags() which I want to
make private to gpiolib.
Reviewed-by: Linus Walleij <[email protected]>
Signed-off-by: Dmitry Torokhov <[email protected]>
---
drivers/media/i2c/s5k5baf.c | 64 +++++++++++--------------------------
1 file changed, 18 insertions(+), 46 deletions(-)
diff --git a/drivers/media/i2c/s5k5baf.c b/drivers/media/i2c/s5k5baf.c
index 5c2253ab3b6f..960fbf6428ea 100644
--- a/drivers/media/i2c/s5k5baf.c
+++ b/drivers/media/i2c/s5k5baf.c
@@ -13,11 +13,10 @@
#include <linux/clk.h>
#include <linux/delay.h>
#include <linux/firmware.h>
-#include <linux/gpio.h>
+#include <linux/gpio/consumer.h>
#include <linux/i2c.h>
#include <linux/media.h>
#include <linux/module.h>
-#include <linux/of_gpio.h>
#include <linux/of_graph.h>
#include <linux/regulator/consumer.h>
#include <linux/slab.h>
@@ -228,11 +227,6 @@ static const char * const s5k5baf_supply_names[] = {
};
#define S5K5BAF_NUM_SUPPLIES ARRAY_SIZE(s5k5baf_supply_names)
-struct s5k5baf_gpio {
- int gpio;
- int level;
-};
-
enum s5k5baf_gpio_id {
STBY,
RSET,
@@ -284,7 +278,7 @@ struct s5k5baf_fw {
};
struct s5k5baf {
- struct s5k5baf_gpio gpios[NUM_GPIOS];
+ struct gpio_desc *gpios[NUM_GPIOS];
enum v4l2_mbus_type bus_type;
u8 nlanes;
struct regulator_bulk_data supplies[S5K5BAF_NUM_SUPPLIES];
@@ -936,16 +930,12 @@ static void s5k5baf_hw_set_test_pattern(struct s5k5baf *state, int id)
static void s5k5baf_gpio_assert(struct s5k5baf *state, int id)
{
- struct s5k5baf_gpio *gpio = &state->gpios[id];
-
- gpio_set_value(gpio->gpio, gpio->level);
+ gpiod_set_value_cansleep(state->gpios[id], 1);
}
static void s5k5baf_gpio_deassert(struct s5k5baf *state, int id)
{
- struct s5k5baf_gpio *gpio = &state->gpios[id];
-
- gpio_set_value(gpio->gpio, !gpio->level);
+ gpiod_set_value_cansleep(state->gpios[id], 0);
}
static int s5k5baf_power_on(struct s5k5baf *state)
@@ -1799,44 +1789,30 @@ static const struct v4l2_subdev_ops s5k5baf_subdev_ops = {
static int s5k5baf_configure_gpios(struct s5k5baf *state)
{
- static const char * const name[] = { "S5K5BAF_STBY", "S5K5BAF_RST" };
+ static const char * const name[] = { "stbyn", "rstn" };
+ static const char * const label[] = { "S5K5BAF_STBY", "S5K5BAF_RST" };
struct i2c_client *c = v4l2_get_subdevdata(&state->sd);
- struct s5k5baf_gpio *g = state->gpios;
+ struct gpio_desc *gpio;
int ret, i;
for (i = 0; i < NUM_GPIOS; ++i) {
- int flags = GPIOF_DIR_OUT;
- if (g[i].level)
- flags |= GPIOF_INIT_HIGH;
- ret = devm_gpio_request_one(&c->dev, g[i].gpio, flags, name[i]);
- if (ret < 0) {
- v4l2_err(c, "failed to request gpio %s\n", name[i]);
+ gpio = devm_gpiod_get(&c->dev, name[i], GPIOD_OUT_HIGH);
+ ret = PTR_ERR_OR_ZERO(gpio);
+ if (ret) {
+ v4l2_err(c, "failed to request gpio %s: %d\n",
+ name[i], ret);
return ret;
}
- }
- return 0;
-}
-
-static int s5k5baf_parse_gpios(struct s5k5baf_gpio *gpios, struct device *dev)
-{
- static const char * const names[] = {
- "stbyn-gpios",
- "rstn-gpios",
- };
- struct device_node *node = dev->of_node;
- enum of_gpio_flags flags;
- int ret, i;
- for (i = 0; i < NUM_GPIOS; ++i) {
- ret = of_get_named_gpio_flags(node, names[i], 0, &flags);
- if (ret < 0) {
- dev_err(dev, "no %s GPIO pin provided\n", names[i]);
+ ret = gpiod_set_consumer_name(gpio, label[i]);
+ if (ret) {
+ v4l2_err(c, "failed to set up name for gpio %s: %d\n",
+ name[i], ret);
return ret;
}
- gpios[i].gpio = ret;
- gpios[i].level = !(flags & OF_GPIO_ACTIVE_LOW);
- }
+ state->gpios[i] = gpio;
+ }
return 0;
}
@@ -1860,10 +1836,6 @@ static int s5k5baf_parse_device_node(struct s5k5baf *state, struct device *dev)
state->mclk_frequency);
}
- ret = s5k5baf_parse_gpios(state->gpios, dev);
- if (ret < 0)
- return ret;
-
node_ep = of_graph_get_next_endpoint(node, NULL);
if (!node_ep) {
dev_err(dev, "no endpoint defined at node %pOF\n", node);
--
2.38.1.431.g37b22c650d-goog
On Tue, Nov 15, 2022 at 11:11 PM Dmitry Torokhov
<[email protected]> wrote:
> This patch switches the driver away from legacy gpio/of_gpio API to
> gpiod API, and removes one of the last uses of of_get_gpio_flags().
>
> Signed-off-by: Dmitry Torokhov <[email protected]>
Reviewed-by: Linus Walleij <[email protected]>
> - sensor->gpio_reset = -EINVAL;
> - sensor->clock = ERR_PTR(-EINVAL);
Looks unrelated but makes sense.
> sensor->dev = dev;
>
> sensor->clock = devm_clk_get(sensor->dev, S5K6A3_CLK_NAME);
Given that it is initialized unconditionally two lines down :P
Yours,
Linus Walleij
Hi Dmitry,
On Tue, Nov 15, 2022 at 02:11:42PM -0800, Dmitry Torokhov wrote:
> This patch switches the driver away from legacy gpio/of_gpio API to
> gpiod API, and removes one of the last uses of of_get_gpio_flags().
>
> Signed-off-by: Dmitry Torokhov <[email protected]>
> ---
> drivers/media/i2c/s5k6a3.c | 30 +++++++++++-------------------
> 1 file changed, 11 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/media/i2c/s5k6a3.c b/drivers/media/i2c/s5k6a3.c
> index a4efd6d10b43..ef6673b10580 100644
> --- a/drivers/media/i2c/s5k6a3.c
> +++ b/drivers/media/i2c/s5k6a3.c
> @@ -9,12 +9,12 @@
> #include <linux/clk.h>
> #include <linux/delay.h>
> #include <linux/device.h>
> +#include <linux/err.h>
> #include <linux/errno.h>
> -#include <linux/gpio.h>
> +#include <linux/gpio/consumer.h>
> #include <linux/i2c.h>
> #include <linux/kernel.h>
> #include <linux/module.h>
> -#include <linux/of_gpio.h>
> #include <linux/pm_runtime.h>
> #include <linux/regulator/consumer.h>
> #include <linux/slab.h>
> @@ -59,7 +59,7 @@ struct s5k6a3 {
> struct v4l2_subdev subdev;
> struct media_pad pad;
> struct regulator_bulk_data supplies[S5K6A3_NUM_SUPPLIES];
> - int gpio_reset;
> + struct gpio_desc *gpio_reset;
> struct mutex lock;
> struct v4l2_mbus_framefmt format;
> struct clk *clock;
> @@ -216,11 +216,11 @@ static int __s5k6a3_power_on(struct s5k6a3 *sensor)
> goto error_clk;
> }
>
> - gpio_set_value(sensor->gpio_reset, 1);
> + gpiod_set_value_cansleep(sensor->gpio_reset, 0);
> usleep_range(600, 800);
> - gpio_set_value(sensor->gpio_reset, 0);
> + gpiod_set_value_cansleep(sensor->gpio_reset, 1);
> usleep_range(600, 800);
> - gpio_set_value(sensor->gpio_reset, 1);
> + gpiod_set_value_cansleep(sensor->gpio_reset, 0);
>
> /* Delay needed for the sensor initialization */
> msleep(20);
> @@ -240,7 +240,7 @@ static int __s5k6a3_power_off(struct s5k6a3 *sensor)
> {
> int i;
>
> - gpio_set_value(sensor->gpio_reset, 0);
> + gpiod_set_value_cansleep(sensor->gpio_reset, 1);
>
> for (i = S5K6A3_NUM_SUPPLIES - 1; i >= 0; i--)
> regulator_disable(sensor->supplies[i].consumer);
> @@ -285,32 +285,24 @@ static int s5k6a3_probe(struct i2c_client *client)
> struct device *dev = &client->dev;
> struct s5k6a3 *sensor;
> struct v4l2_subdev *sd;
> - int gpio, i, ret;
> + int i, ret;
>
> sensor = devm_kzalloc(dev, sizeof(*sensor), GFP_KERNEL);
> if (!sensor)
> return -ENOMEM;
>
> mutex_init(&sensor->lock);
> - sensor->gpio_reset = -EINVAL;
> - sensor->clock = ERR_PTR(-EINVAL);
> sensor->dev = dev;
>
> sensor->clock = devm_clk_get(sensor->dev, S5K6A3_CLK_NAME);
> if (IS_ERR(sensor->clock))
> return PTR_ERR(sensor->clock);
>
> - gpio = of_get_gpio_flags(dev->of_node, 0, NULL);
> - if (!gpio_is_valid(gpio))
> - return gpio;
> -
> - ret = devm_gpio_request_one(dev, gpio, GPIOF_OUT_INIT_LOW,
> - S5K6A3_DRV_NAME);
> - if (ret < 0)
> + sensor->gpio_reset = devm_gpiod_get(dev, NULL, GPIOD_OUT_HIGH);
> + ret = PTR_ERR_OR_ZERO(sensor->gpio_reset);
> + if (ret)
> return ret;
Patch looks good to me!
Reviewed-by: Tommaso Merciai <[email protected]>
For the future I think would be nice use "reset-gpios" name in dts.
Then call:
devm_gpiod_get(dev, "reset", GPIOD_OUT_HIGH);
But this is a todo :)
Regards,
Tommaso
>
> - sensor->gpio_reset = gpio;
> -
> if (of_property_read_u32(dev->of_node, "clock-frequency",
> &sensor->clock_frequency)) {
> sensor->clock_frequency = S5K6A3_DEFAULT_CLK_FREQ;
> --
> 2.38.1.431.g37b22c650d-goog
>
--
Tommaso Merciai
Embedded Linux Engineer
[email protected]
__________________________________
Amarula Solutions SRL
Via Le Canevare 30, 31100 Treviso, Veneto, IT
T. +39 042 243 5310
[email protected]
http://www.amarulasolutions.com
Hi Dmitry,
On Tue, Nov 15, 2022 at 02:11:43PM -0800, Dmitry Torokhov wrote:
> This patch switches the driver away from legacy gpio/of_gpio API to
> gpiod API, and removes use of of_get_named_gpio_flags() which I want to
> make private to gpiolib.
>
> Reviewed-by: Linus Walleij <[email protected]>
> Signed-off-by: Dmitry Torokhov <[email protected]>
> ---
> drivers/media/i2c/s5k5baf.c | 64 +++++++++++--------------------------
> 1 file changed, 18 insertions(+), 46 deletions(-)
>
> diff --git a/drivers/media/i2c/s5k5baf.c b/drivers/media/i2c/s5k5baf.c
> index 5c2253ab3b6f..960fbf6428ea 100644
> --- a/drivers/media/i2c/s5k5baf.c
> +++ b/drivers/media/i2c/s5k5baf.c
> @@ -13,11 +13,10 @@
> #include <linux/clk.h>
> #include <linux/delay.h>
> #include <linux/firmware.h>
> -#include <linux/gpio.h>
> +#include <linux/gpio/consumer.h>
> #include <linux/i2c.h>
> #include <linux/media.h>
> #include <linux/module.h>
> -#include <linux/of_gpio.h>
> #include <linux/of_graph.h>
> #include <linux/regulator/consumer.h>
> #include <linux/slab.h>
> @@ -228,11 +227,6 @@ static const char * const s5k5baf_supply_names[] = {
> };
> #define S5K5BAF_NUM_SUPPLIES ARRAY_SIZE(s5k5baf_supply_names)
>
> -struct s5k5baf_gpio {
> - int gpio;
> - int level;
> -};
> -
> enum s5k5baf_gpio_id {
> STBY,
> RSET,
> @@ -284,7 +278,7 @@ struct s5k5baf_fw {
> };
>
> struct s5k5baf {
> - struct s5k5baf_gpio gpios[NUM_GPIOS];
> + struct gpio_desc *gpios[NUM_GPIOS];
> enum v4l2_mbus_type bus_type;
> u8 nlanes;
> struct regulator_bulk_data supplies[S5K5BAF_NUM_SUPPLIES];
> @@ -936,16 +930,12 @@ static void s5k5baf_hw_set_test_pattern(struct s5k5baf *state, int id)
>
> static void s5k5baf_gpio_assert(struct s5k5baf *state, int id)
> {
> - struct s5k5baf_gpio *gpio = &state->gpios[id];
> -
> - gpio_set_value(gpio->gpio, gpio->level);
> + gpiod_set_value_cansleep(state->gpios[id], 1);
> }
>
> static void s5k5baf_gpio_deassert(struct s5k5baf *state, int id)
> {
> - struct s5k5baf_gpio *gpio = &state->gpios[id];
> -
> - gpio_set_value(gpio->gpio, !gpio->level);
> + gpiod_set_value_cansleep(state->gpios[id], 0);
> }
>
> static int s5k5baf_power_on(struct s5k5baf *state)
> @@ -1799,44 +1789,30 @@ static const struct v4l2_subdev_ops s5k5baf_subdev_ops = {
>
> static int s5k5baf_configure_gpios(struct s5k5baf *state)
> {
> - static const char * const name[] = { "S5K5BAF_STBY", "S5K5BAF_RST" };
> + static const char * const name[] = { "stbyn", "rstn" };
> + static const char * const label[] = { "S5K5BAF_STBY", "S5K5BAF_RST" };
> struct i2c_client *c = v4l2_get_subdevdata(&state->sd);
> - struct s5k5baf_gpio *g = state->gpios;
> + struct gpio_desc *gpio;
> int ret, i;
>
> for (i = 0; i < NUM_GPIOS; ++i) {
> - int flags = GPIOF_DIR_OUT;
> - if (g[i].level)
> - flags |= GPIOF_INIT_HIGH;
> - ret = devm_gpio_request_one(&c->dev, g[i].gpio, flags, name[i]);
> - if (ret < 0) {
> - v4l2_err(c, "failed to request gpio %s\n", name[i]);
> + gpio = devm_gpiod_get(&c->dev, name[i], GPIOD_OUT_HIGH);
> + ret = PTR_ERR_OR_ZERO(gpio);
> + if (ret) {
> + v4l2_err(c, "failed to request gpio %s: %d\n",
> + name[i], ret);
> return ret;
> }
> - }
> - return 0;
> -}
> -
> -static int s5k5baf_parse_gpios(struct s5k5baf_gpio *gpios, struct device *dev)
> -{
> - static const char * const names[] = {
> - "stbyn-gpios",
> - "rstn-gpios",
> - };
> - struct device_node *node = dev->of_node;
> - enum of_gpio_flags flags;
> - int ret, i;
>
> - for (i = 0; i < NUM_GPIOS; ++i) {
> - ret = of_get_named_gpio_flags(node, names[i], 0, &flags);
> - if (ret < 0) {
> - dev_err(dev, "no %s GPIO pin provided\n", names[i]);
> + ret = gpiod_set_consumer_name(gpio, label[i]);
> + if (ret) {
> + v4l2_err(c, "failed to set up name for gpio %s: %d\n",
> + name[i], ret);
> return ret;
> }
> - gpios[i].gpio = ret;
> - gpios[i].level = !(flags & OF_GPIO_ACTIVE_LOW);
> - }
>
> + state->gpios[i] = gpio;
> + }
> return 0;
> }
>
> @@ -1860,10 +1836,6 @@ static int s5k5baf_parse_device_node(struct s5k5baf *state, struct device *dev)
> state->mclk_frequency);
> }
>
> - ret = s5k5baf_parse_gpios(state->gpios, dev);
> - if (ret < 0)
> - return ret;
> -
> node_ep = of_graph_get_next_endpoint(node, NULL);
> if (!node_ep) {
> dev_err(dev, "no endpoint defined at node %pOF\n", node);
> --
> 2.38.1.431.g37b22c650d-goog
>
Looks good to me.
Reviewed-by: Tommaso Merciai <[email protected]>
Regards,
Tommaso
--
Tommaso Merciai
Embedded Linux Engineer
[email protected]
__________________________________
Amarula Solutions SRL
Via Le Canevare 30, 31100 Treviso, Veneto, IT
T. +39 042 243 5310
[email protected]
http://www.amarulasolutions.com