lkml.org 
[lkml]   [2018]   [Feb]   [26]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
Date
SubjectRe: [PATCH V2 1/2] [media] Add Rockchip RK1608 driver
On Mon, Feb 26, 2018 at 9:16 AM, Wen Nuan <leo.wen@rock-chips.com> wrote:

> +#include <linux/of_gpio.h>

Please do not use this old API in new drivers.

Just use GPIO descriptors and only
#include <linux/gpio/consumer.h>

There is documentation in
Documentation/gpio/consumer.txt

> +static int rk1608_pin_power(struct rk1608_state *pdata, int on)
> +{
> + unsigned int grf_val = 0;
> +
> + if (on) {
> + clk_prepare_enable(pdata->pd_cif);
> + clk_prepare_enable(pdata->aclk_cif);
> + clk_prepare_enable(pdata->hclk_cif);
> + clk_prepare_enable(pdata->cif_clk_in);
> + clk_prepare_enable(pdata->cif_clk_out);
> + clk_prepare_enable(pdata->clk_mipi_24m);
> + clk_prepare_enable(pdata->hclk_mipiphy);
> + clk_set_rate(pdata->cif_clk_out, RK1608_MCLK_RATE);
> +
> + clk_prepare_enable(pdata->mipi_clk);
> + clk_set_rate(pdata->mipi_clk, RK1608_MCLK_RATE);
> +
> + gpio_direction_output(pdata->rst1, 0);

Instead of this old GPIO API, put GPIO descriptors
struct gpio_desc *rst1; etc in your state container and
use gpiod_direction_output() etc.

> + pdata->grf_gpio2b_iomux = ioremap((resource_size_t)
> + (GRF_BASE_ADDR +
> + GRF_GPIO2B_IOMUX), 4);
> + grf_val = __raw_readl(pdata->grf_gpio2b_iomux);
> + __raw_writel(((grf_val) | (1 << 6) | (1 << (6 + 16))),
> + pdata->grf_gpio2b_iomux);
> +
> + pdata->grf_io_vsel = ioremap((resource_size_t)
> + (GRF_BASE_ADDR + GRF_IO_VSEL), 4);
> + grf_val = __raw_readl(pdata->grf_io_vsel);
> + __raw_writel(((grf_val) | (1 << 1) | (1 << (1 + 16))),
> + pdata->grf_io_vsel);

You are doing pin control on the side of the pin control subsystem
it looks like?

I think David Wu and Heiko Stubner needs to have a look at what you
are doing here to suggest other solutions.

Apart from that, why use __raw_writel instead of just writel()?

> +static int rk1608_parse_dt_property(struct rk1608_state *pdata)
> +{
(...)
> + enum of_gpio_flags flags;

Don't use this.

> + ret = of_get_named_gpio_flags(node, "rockchip,reset1", 0, &flags);

You should not use custom properties for GPIO names,
they need names like this:

gpios-reset1 = <&gpioN 1 GPIO_ACTIVE_HIGH>

See
Documentation/devicetree/bindings/gpio/gpio.txt

> + if (ret <= 0)
> + dev_err(dev, "can not find reset1 error %d\n", ret);
> + pdata->rst1 = ret;
> +
> + ret = devm_gpio_request(dev, pdata->rst1, "rockchip-reset1");
> + if (ret) {
> + dev_err(dev, "gpio pdata->rst1 %d request error %d\n",
> + pdata->rst1, ret);
> + return ret;
> + }
> + gpio_set_value(pdata->rst1, 0);
> + ret = gpio_direction_output(pdata->rst1, 0);
> + if (ret) {
> + dev_err(dev, "gpio %d direction output error %d\n",
> + pdata->rst1, ret);
> + return ret;
> + }

As you see this become tedious and repetitive.

Instead use:

pdata->rst1 = gpiod_get(dev, "reset1", GPIOD_OUT_LOW);
if (IS_ERR(pdata->rst1))
return PTR_ERR(pdata->rst1);

After this point you know that you have a valid handle on the
GPIO line and it is initialized low.

> +struct rk1608_state {
(...)
> + void __iomem *grf_gpio2b_iomux;

I suspect this should be done with pin control.

> + void __iomem *grf_io_vsel;

And this should maybe use a regulator.

> + int powerdown1;
> + int rst1;
> + int powerdown0;
> + int rst0;
> + int power_count;
> + int reset_gpio;
> + int reset_active;
> + int irq_gpio;
> + int irq;
> + int sleepst_gpio;
> + int sleepst_irq;
> + int wakeup_gpio;
> + int wakeup_active;
> + int powerdown_gpio;
> + int powerdown_active;

Anything that us a GPIO should be a struct gpio_desc *.

You do not need to keep track of if they are active I guess,
but I haven't looked close. They should probably be bool
rather than int if necessary.

But notice that we have
gpiod_get_optional() and if you use that you can
just check if the gpio_desc * is NULL like

if (!pdata->wakeup_gpio)
gpiod_set_value() ...

Yours,
Linus Walleij

\
 
 \ /
  Last update: 2018-02-26 11:12    [W:1.991 / U:0.016 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site