Hi Linus,
On Wed, Jun 01, 2022 at 03:48:35PM +0200, Linus Walleij wrote:
> The driver has an option to pass in GPIO numbers from platform
> data but this is not used in the kernel so delete this and the
> whole platform data mechanism.
>
> Get GPIO descriptors using the standard API and simplify the code,
> gpiolib will handle any inversions.
>
> Cc: Sylwester Nawrocki <s.nawrocki@xxxxxxxxxxx>
> Cc: Andrzej Hajda <andrzej.hajda@xxxxxxxxx>
> Cc: Krzysztof Kozlowski <krzk@xxxxxxxxxx>
> Cc: Alim Akhtar <alim.akhtar@xxxxxxxxxxx>
> Signed-off-by: Linus Walleij <linus.walleij@xxxxxxxxxx>
> ---
> ChangeLog v2->v3:
> - COMMIT and send out what is actually in my tree (and
> compiling).
> ChangeLog v1->v2:
> - Fix compile bug by sending out the patch actually in my
> git tree.
> ---
> drivers/media/i2c/s5k4ecgx.c | 132 ++++++++---------------------------
> include/media/i2c/s5k4ecgx.h | 33 ---------
> 2 files changed, 31 insertions(+), 134 deletions(-)
> delete mode 100644 include/media/i2c/s5k4ecgx.h
>
> diff --git a/drivers/media/i2c/s5k4ecgx.c b/drivers/media/i2c/s5k4ecgx.c
> index af9a305242cd..04e8d7b130ad 100644
> --- a/drivers/media/i2c/s5k4ecgx.c
> +++ b/drivers/media/i2c/s5k4ecgx.c
> @@ -15,7 +15,7 @@
> #include <linux/ctype.h>
> #include <linux/delay.h>
> #include <linux/firmware.h>
> -#include <linux/gpio.h>
> +#include <linux/gpio/consumer.h>
> #include <linux/i2c.h>
> #include <linux/module.h>
> #include <linux/regulator/consumer.h>
> @@ -23,7 +23,6 @@
> #include <asm/unaligned.h>
>
> #include <media/media-entity.h>
> -#include <media/i2c/s5k4ecgx.h>
> #include <media/v4l2-ctrls.h>
> #include <media/v4l2-device.h>
> #include <media/v4l2-mediabus.h>
> @@ -171,12 +170,6 @@ static const char * const s5k4ecgx_supply_names[] = {
>
> #define S5K4ECGX_NUM_SUPPLIES ARRAY_SIZE(s5k4ecgx_supply_names)
>
> -enum s5k4ecgx_gpio_id {
> - STBY,
> - RSET,
> - GPIO_NUM,
> -};
> -
> struct s5k4ecgx {
> struct v4l2_subdev sd;
> struct media_pad pad;
> @@ -190,7 +183,8 @@ struct s5k4ecgx {
> u8 set_params;
>
> struct regulator_bulk_data supplies[S5K4ECGX_NUM_SUPPLIES];
> - struct s5k4ecgx_gpio gpio[GPIO_NUM];
> + struct gpio_desc *stby;
> + struct gpio_desc *reset;
> };
>
> static inline struct s5k4ecgx *to_s5k4ecgx(struct v4l2_subdev *sd)
> @@ -454,15 +448,6 @@ static int s5k4ecgx_init_sensor(struct v4l2_subdev *sd)
> return ret;
> }
>
> -static int s5k4ecgx_gpio_set_value(struct s5k4ecgx *priv, int id, u32 val)
> -{
> - if (!gpio_is_valid(priv->gpio[id].gpio))
> - return 0;
> - gpio_set_value(priv->gpio[id].gpio, val);
> -
> - return 1;
> -}
> -
> static int __s5k4ecgx_power_on(struct s5k4ecgx *priv)
> {
> int ret;
> @@ -472,23 +457,22 @@ static int __s5k4ecgx_power_on(struct s5k4ecgx *priv)
> return ret;
> usleep_range(30, 50);
>
> - /* The polarity of STBY is controlled by TSP */
> - if (s5k4ecgx_gpio_set_value(priv, STBY, priv->gpio[STBY].level))
> - usleep_range(30, 50);
> -
> - if (s5k4ecgx_gpio_set_value(priv, RSET, priv->gpio[RSET].level))
> - usleep_range(30, 50);
> + /* De-assert standby and reset */
> + gpiod_set_value(priv->stby, 0);
> + usleep_range(30, 50);
> + gpiod_set_value(priv->reset, 0);
> + usleep_range(30, 50);
>
> return 0;
> }
>
> static int __s5k4ecgx_power_off(struct s5k4ecgx *priv)
> {
> - if (s5k4ecgx_gpio_set_value(priv, RSET, !priv->gpio[RSET].level))
> - usleep_range(30, 50);
> -
> - if (s5k4ecgx_gpio_set_value(priv, STBY, !priv->gpio[STBY].level))
> - usleep_range(30, 50);
> + /* Assert reset and standby */
> + gpiod_set_value(priv->reset, 1);
> + usleep_range(30, 50);
> + gpiod_set_value(priv->stby, 1);
> + usleep_range(30, 50);
>
> priv->streaming = 0;
>
> @@ -840,68 +824,6 @@ static const struct v4l2_subdev_ops s5k4ecgx_ops = {
> .video = &s5k4ecgx_video_ops,
> };
>
> -/*
> - * GPIO setup
> - */
> -static int s5k4ecgx_config_gpio(int nr, int val, const char *name)
> -{
> - unsigned long flags = val ? GPIOF_OUT_INIT_HIGH : GPIOF_OUT_INIT_LOW;
> - int ret;
> -
> - if (!gpio_is_valid(nr))
> - return 0;
> - ret = gpio_request_one(nr, flags, name);
> - if (!ret)
> - gpio_export(nr, 0);
> -
> - return ret;
> -}
> -
> -static void s5k4ecgx_free_gpios(struct s5k4ecgx *priv)
> -{
> - int i;
> -
> - for (i = 0; i < ARRAY_SIZE(priv->gpio); i++) {
> - if (!gpio_is_valid(priv->gpio[i].gpio))
> - continue;
> - gpio_free(priv->gpio[i].gpio);
> - priv->gpio[i].gpio = -EINVAL;
> - }
> -}
> -
> -static int s5k4ecgx_config_gpios(struct s5k4ecgx *priv,
> - const struct s5k4ecgx_platform_data *pdata)
> -{
> - const struct s5k4ecgx_gpio *gpio = &pdata->gpio_stby;
> - int ret;
> -
> - priv->gpio[STBY].gpio = -EINVAL;
> - priv->gpio[RSET].gpio = -EINVAL;
> -
> - ret = s5k4ecgx_config_gpio(gpio->gpio, gpio->level, "S5K4ECGX_STBY");
> -
> - if (ret) {
> - s5k4ecgx_free_gpios(priv);
> - return ret;
> - }
> - priv->gpio[STBY] = *gpio;
> - if (gpio_is_valid(gpio->gpio))
> - gpio_set_value(gpio->gpio, 0);
> -
> - gpio = &pdata->gpio_reset;
> -
> - ret = s5k4ecgx_config_gpio(gpio->gpio, gpio->level, "S5K4ECGX_RST");
> - if (ret) {
> - s5k4ecgx_free_gpios(priv);
> - return ret;
> - }
> - priv->gpio[RSET] = *gpio;
> - if (gpio_is_valid(gpio->gpio))
> - gpio_set_value(gpio->gpio, 0);
> -
> - return 0;
> -}
> -
> static int s5k4ecgx_init_v4l2_ctrls(struct s5k4ecgx *priv)
> {
> const struct v4l2_ctrl_ops *ops = &s5k4ecgx_ctrl_ops;
> @@ -965,11 +887,22 @@ static int s5k4ecgx_probe(struct i2c_client *client,
> if (ret)
> return ret;
>
> - ret = s5k4ecgx_config_gpios(priv, pdata);
> - if (ret) {
> - dev_err(&client->dev, "Failed to set gpios\n");
> - goto out_err1;
> + /* Request GPIO lines asserted */
> + priv->stby = devm_gpiod_get(&client->dev, "standby", GPIOD_OUT_HIGH);
> + if (IS_ERR(priv->stby)) {
> + v4l2_err(sd, "failed to request gpio S5K4ECGX_STBY");
> + ret = PTR_ERR(priv->stby);
> + goto out_err;
> + }
> + gpiod_set_consumer_name(priv->stby, "S5K4ECGX_STBY");
> + priv->reset = devm_gpiod_get(&client->dev, "reset", GPIOD_OUT_HIGH);
> + if (IS_ERR(priv->reset)) {
> + v4l2_err(sd, "failed to request gpio S5K4ECGX_RST");
> + ret = PTR_ERR(priv->reset);
> + goto out_err;
> }
> + gpiod_set_consumer_name(priv->reset, "S5K4ECGX_RST");
> +
> for (i = 0; i < S5K4ECGX_NUM_SUPPLIES; i++)
> priv->supplies[i].supply = s5k4ecgx_supply_names[i];
>
> @@ -977,20 +910,18 @@ static int s5k4ecgx_probe(struct i2c_client *client,
> priv->supplies);
> if (ret) {
> dev_err(&client->dev, "Failed to get regulators\n");
> - goto out_err2;
> + goto out_err;
> }
> ret = s5k4ecgx_init_v4l2_ctrls(priv);
> if (ret)
> - goto out_err2;
> + goto out_err;
>
> priv->curr_pixfmt = &s5k4ecgx_formats[0];
> priv->curr_frmsize = &s5k4ecgx_prev_sizes[0];
>
> return 0;
>
> -out_err2:
> - s5k4ecgx_free_gpios(priv);
> -out_err1:
> +out_err:
> media_entity_cleanup(&priv->sd.entity);
>
> return ret;
> @@ -1002,7 +933,6 @@ static int s5k4ecgx_remove(struct i2c_client *client)
> struct s5k4ecgx *priv = to_s5k4ecgx(sd);
>
> mutex_destroy(&priv->lock);
> - s5k4ecgx_free_gpios(priv);
> v4l2_device_unregister_subdev(sd);
> v4l2_ctrl_handler_free(&priv->handler);
> media_entity_cleanup(&sd->entity);
> diff --git a/include/media/i2c/s5k4ecgx.h b/include/media/i2c/s5k4ecgx.h
> deleted file mode 100644
> index 92202eb35249..000000000000
> --- a/include/media/i2c/s5k4ecgx.h
> +++ /dev/null
> @@ -1,33 +0,0 @@
> -/* SPDX-License-Identifier: GPL-2.0-or-later */
> -/*
> - * S5K4ECGX image sensor header file
> - *
> - * Copyright (C) 2012, Linaro
> - * Copyright (C) 2012, Samsung Electronics Co., Ltd.
> - */
> -
> -#ifndef S5K4ECGX_H
> -#define S5K4ECGX_H
> -
> -/**
> - * struct s5k4ecgx_gpio - data structure describing a GPIO
> - * @gpio: GPIO number
> - * @level: indicates active state of the @gpio
> - */
> -struct s5k4ecgx_gpio {
> - int gpio;
> - int level;
> -};
> -
> -/**
> - * struct s5k4ecgx_platform_data - s5k4ecgx driver platform data
> - * @gpio_reset: GPIO driving RESET pin
> - * @gpio_stby: GPIO driving STBY pin
> - */
> -
> -struct s5k4ecgx_platform_data {
> - struct s5k4ecgx_gpio gpio_reset;
> - struct s5k4ecgx_gpio gpio_stby;
> -};
> -
> -#endif /* S5K4ECGX_H */
> --
> 2.36.1
>
Looks good to me.
I test also the compilation locally all seems works
fine.
make ARCH=arm64 CROSS_COMPILE=aarch64-linux-gnu- drivers/media/i2c/s5k4ecgx.ko
UPD include/config/kernel.release
UPD include/generated/utsrelease.h
CALL scripts/checksyscalls.sh
CALL scripts/atomic/check-atomics.sh
CC [M] drivers/media/i2c/s5k4ecgx.o
MODPOST modules-only.symvers
GEN Module.symvers
CC [M] drivers/media/i2c/s5k4ecgx.mod.o
LD [M] drivers/media/i2c/s5k4ecgx.ko
Unfortunately I can't test it on real target.
Reviewed-by: Tommaso Merciai <tommaso.merciai@xxxxxxxxxxxxxxxxxxxx>
Thanks,
Tommaso
--
Tommaso Merciai
Embedded Linux Engineer
tommaso.merciai@xxxxxxxxxxxxxxxxxxxx
__________________________________
Amarula Solutions SRL
Via Le Canevare 30, 31100 Treviso, Veneto, IT
T. +39 042 243 5310
info@xxxxxxxxxxxxxxxxxxxx
www.amarulasolutions.com
