Messages in this thread | | | From | Linus Walleij <> | Date | Mon, 26 Feb 2018 11:12:30 +0100 | Subject | Re: [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
| |