Re: [PATCH v2 2/3] media: staging: rkisp1: add rk3288 support

[Helen Koike <helen.koike@collabora.com>]

Hi Karthik,

Thanks for your patch.

On 4/6/20 4:30 AM, Karthik Poduval wrote:
> Add rk3288 support to the rkisp1 driver.
> 
> tested on tinkerbaord with ov5647 using command
> cam -c 1 -C -F cap
> 
> Reported-by: Karthik Poduval <karthik.poduval@gmail.com>

Please, see my comment from previous patch regarding this tag.

> Signed-off-by: Karthik Poduval <karthik.poduval@gmail.com>
> ---
>  .../bindings/media/rockchip-isp1.yaml          |  1 +
>  drivers/staging/media/rkisp1/rkisp1-dev.c      | 18 ++++++++++++++++++
>  2 files changed, 19 insertions(+)
> 
> diff --git a/drivers/staging/media/rkisp1/Documentation/devicetree/bindings/media/rockchip-isp1.yaml b/drivers/staging/media/rkisp1/Documentation/devicetree/bindings/media/rockchip-isp1.yaml
> index af246b71eac6..4c31294cf14b 100644
> --- a/drivers/staging/media/rkisp1/Documentation/devicetree/bindings/media/rockchip-isp1.yaml
> +++ b/drivers/staging/media/rkisp1/Documentation/devicetree/bindings/media/rockchip-isp1.yaml

Please, update bindings in a separated patchset.

> @@ -16,6 +16,7 @@ description: |
>  properties:
>    compatible:
>      const: rockchip,rk3399-cif-isp
> +    const: rockchip,rk3288-rkisp1

Please, keep consistency, or rk3288-cif-isp, or we change rk3399-cif-isp to be rk3399-rkisp1.

>  
>    reg:
>      maxItems: 1
> diff --git a/drivers/staging/media/rkisp1/rkisp1-dev.c b/drivers/staging/media/rkisp1/rkisp1-dev.c
> index b1b3c058e957..1df4f5906fd0 100644
> --- a/drivers/staging/media/rkisp1/rkisp1-dev.c
> +++ b/drivers/staging/media/rkisp1/rkisp1-dev.c
> @@ -403,6 +403,20 @@ static irqreturn_t rkisp1_isr(int irq, void *ctx)
>  	return IRQ_HANDLED;
>  }
>  
> +

Checkpatch error:
drivers/staging/media/rkisp1/rkisp1-dev.c:406: check: Please don't use multiple blank lines

> +static const char * const rk3288_isp_clks[] = {
> +	"clk_isp",
> +	"aclk_isp",
> +	"hclk_isp",
> +	"pclk_isp_in",
> +	"sclk_isp_jpe",
> +};

You should also update clock-names in the bindings as well.

Regards,
Helen

> +
> +static const struct rkisp1_match_data rk3288_isp_clk_data = {
> +	.clks = rk3288_isp_clks,
> +	.size = ARRAY_SIZE(rk3288_isp_clks),
> +};
> +
>  static const char * const rk3399_isp_clks[] = {
>  	"clk_isp",
>  	"aclk_isp",
> @@ -417,6 +431,10 @@ static const struct rkisp1_match_data rk3399_isp_clk_data = {
>  };
>  
>  static const struct of_device_id rkisp1_of_match[] = {
> +	{
> +		.compatible = "rockchip,rk3288-rkisp1",
> +		.data = &rk3288_isp_clk_data,
> +	},
>  	{
>  		.compatible = "rockchip,rk3399-cif-isp",
>  		.data = &rk3399_isp_clk_data,
>