On Mon, Feb 26, 2018 at 9:16 AM, Wen Nuan <leo.wen@xxxxxxxxxxxxxx> 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
