| Message ID | 20191215165924.28314-7-wens@kernel.org (mailing list archive) |
|---|---|
| State | Mainlined |
| Headers | show |
| Series | media: sun4i-csi: A10/A20 CSI1 and R40 CSI0 support | expand |
On Mon, Dec 16, 2019 at 12:59:16AM +0800, Chen-Yu Tsai wrote: > From: Chen-Yu Tsai <wens@csie.org> > > The A10/A20 Allwinner SoCs have two camera sensor interface blocks, > named CSI0 and CSI1. The two have the same register layouts with > slightly different features: > > - CSI0 has an image signal processor (ISP); CSI1 doesn't > > - CSI0 can support up to four separate channels under CCIR656; > CSI1 can only support one > > - CSI0 can support up to 16-bit wide bus with YUV422; > CSI1 can support up to 24-bit wide bus with YUV444 > > For now the driver doesn't support wide busses, nor CCIR656. So the > only relevant difference is whether a clock needs to be taken and > enabled for the ISP. > > Add structs to record the differences, tie them to the compatible > strings, and deal with the ISP clock. Support for the new CSI1 > hardware block is added as well. > > Signed-off-by: Chen-Yu Tsai <wens@csie.org> Acked-by: Maxime Ripard <mripard@kernel.org> Thanks! Maxime
Hi Chen-Yu, Thanks for the patchset. On Mon, Dec 16, 2019 at 12:59:16AM +0800, Chen-Yu Tsai wrote: > From: Chen-Yu Tsai <wens@csie.org> > > The A10/A20 Allwinner SoCs have two camera sensor interface blocks, > named CSI0 and CSI1. The two have the same register layouts with > slightly different features: > > - CSI0 has an image signal processor (ISP); CSI1 doesn't > > - CSI0 can support up to four separate channels under CCIR656; > CSI1 can only support one > > - CSI0 can support up to 16-bit wide bus with YUV422; > CSI1 can support up to 24-bit wide bus with YUV444 > > For now the driver doesn't support wide busses, nor CCIR656. So the > only relevant difference is whether a clock needs to be taken and > enabled for the ISP. > > Add structs to record the differences, tie them to the compatible > strings, and deal with the ISP clock. Support for the new CSI1 > hardware block is added as well. > > Signed-off-by: Chen-Yu Tsai <wens@csie.org> > --- > .../platform/sunxi/sun4i-csi/sun4i_csi.c | 35 ++++++++++++++++--- > .../platform/sunxi/sun4i-csi/sun4i_csi.h | 2 ++ > 2 files changed, 32 insertions(+), 5 deletions(-) > > diff --git a/drivers/media/platform/sunxi/sun4i-csi/sun4i_csi.c b/drivers/media/platform/sunxi/sun4i-csi/sun4i_csi.c > index b8b07c1de2a8..be2466930a49 100644 > --- a/drivers/media/platform/sunxi/sun4i-csi/sun4i_csi.c > +++ b/drivers/media/platform/sunxi/sun4i-csi/sun4i_csi.c > @@ -29,6 +29,12 @@ > > #include "sun4i_csi.h" > > +struct sun4i_csi_traits { > + unsigned int channels; > + unsigned int max_width; > + bool has_isp; > +}; > + > static const struct media_entity_operations sun4i_csi_video_entity_ops = { > .link_validate = v4l2_subdev_link_validate, > }; > @@ -156,6 +162,10 @@ static int sun4i_csi_probe(struct platform_device *pdev) > subdev = &csi->subdev; > vdev = &csi->vdev; > > + csi->traits = of_device_get_match_data(&pdev->dev); > + if (!csi->traits) > + return -EINVAL; > + > /* > * On Allwinner SoCs, some high memory bandwidth devices do DMA > * directly over the memory bus (called MBUS), instead of the > @@ -199,10 +209,12 @@ static int sun4i_csi_probe(struct platform_device *pdev) > return PTR_ERR(csi->bus_clk); > } > > - csi->isp_clk = devm_clk_get(&pdev->dev, "isp"); > - if (IS_ERR(csi->isp_clk)) { > - dev_err(&pdev->dev, "Couldn't get our ISP clock\n"); > - return PTR_ERR(csi->isp_clk); > + if (csi->traits->has_isp) { > + csi->isp_clk = devm_clk_get(&pdev->dev, "isp"); > + if (IS_ERR(csi->isp_clk)) { > + dev_err(&pdev->dev, "Couldn't get our ISP clock\n"); > + return PTR_ERR(csi->isp_clk); > + } > } > > csi->ram_clk = devm_clk_get(&pdev->dev, "ram"); > @@ -280,8 +292,21 @@ static int sun4i_csi_remove(struct platform_device *pdev) > return 0; > } > > +struct sun4i_csi_traits sun4i_a10_csi1_traits = { > + .channels = 1, > + .max_width = 24, > + .has_isp = false, > +}; > + > +struct sun4i_csi_traits sun7i_a20_csi0_traits = { These two should be static const, right? > + .channels = 4, > + .max_width = 16, > + .has_isp = true, > +}; > + > static const struct of_device_id sun4i_csi_of_match[] = { > - { .compatible = "allwinner,sun7i-a20-csi0" }, > + { .compatible = "allwinner,sun4i-a10-csi1", .data = &sun4i_a10_csi1_traits }, > + { .compatible = "allwinner,sun7i-a20-csi0", .data = &sun7i_a20_csi0_traits }, > { /* Sentinel */ } > }; > MODULE_DEVICE_TABLE(of, sun4i_csi_of_match); > diff --git a/drivers/media/platform/sunxi/sun4i-csi/sun4i_csi.h b/drivers/media/platform/sunxi/sun4i-csi/sun4i_csi.h > index 88d39b3554c4..0f67ff652c2e 100644 > --- a/drivers/media/platform/sunxi/sun4i-csi/sun4i_csi.h > +++ b/drivers/media/platform/sunxi/sun4i-csi/sun4i_csi.h > @@ -108,6 +108,8 @@ struct sun4i_csi { > /* Device resources */ > struct device *dev; > > + const struct sun4i_csi_traits *traits; > + > void __iomem *regs; > struct clk *bus_clk; > struct clk *isp_clk;
On Thu, Jan 2, 2020 at 7:33 PM Sakari Ailus <sakari.ailus@linux.intel.com> wrote: > > Hi Chen-Yu, > > Thanks for the patchset. > > On Mon, Dec 16, 2019 at 12:59:16AM +0800, Chen-Yu Tsai wrote: > > From: Chen-Yu Tsai <wens@csie.org> > > > > The A10/A20 Allwinner SoCs have two camera sensor interface blocks, > > named CSI0 and CSI1. The two have the same register layouts with > > slightly different features: > > > > - CSI0 has an image signal processor (ISP); CSI1 doesn't > > > > - CSI0 can support up to four separate channels under CCIR656; > > CSI1 can only support one > > > > - CSI0 can support up to 16-bit wide bus with YUV422; > > CSI1 can support up to 24-bit wide bus with YUV444 > > > > For now the driver doesn't support wide busses, nor CCIR656. So the > > only relevant difference is whether a clock needs to be taken and > > enabled for the ISP. > > > > Add structs to record the differences, tie them to the compatible > > strings, and deal with the ISP clock. Support for the new CSI1 > > hardware block is added as well. > > > > Signed-off-by: Chen-Yu Tsai <wens@csie.org> > > --- > > .../platform/sunxi/sun4i-csi/sun4i_csi.c | 35 ++++++++++++++++--- > > .../platform/sunxi/sun4i-csi/sun4i_csi.h | 2 ++ > > 2 files changed, 32 insertions(+), 5 deletions(-) > > > > diff --git a/drivers/media/platform/sunxi/sun4i-csi/sun4i_csi.c b/drivers/media/platform/sunxi/sun4i-csi/sun4i_csi.c > > index b8b07c1de2a8..be2466930a49 100644 > > --- a/drivers/media/platform/sunxi/sun4i-csi/sun4i_csi.c > > +++ b/drivers/media/platform/sunxi/sun4i-csi/sun4i_csi.c > > @@ -29,6 +29,12 @@ > > > > #include "sun4i_csi.h" > > > > +struct sun4i_csi_traits { > > + unsigned int channels; > > + unsigned int max_width; > > + bool has_isp; > > +}; > > + > > static const struct media_entity_operations sun4i_csi_video_entity_ops = { > > .link_validate = v4l2_subdev_link_validate, > > }; > > @@ -156,6 +162,10 @@ static int sun4i_csi_probe(struct platform_device *pdev) > > subdev = &csi->subdev; > > vdev = &csi->vdev; > > > > + csi->traits = of_device_get_match_data(&pdev->dev); > > + if (!csi->traits) > > + return -EINVAL; > > + > > /* > > * On Allwinner SoCs, some high memory bandwidth devices do DMA > > * directly over the memory bus (called MBUS), instead of the > > @@ -199,10 +209,12 @@ static int sun4i_csi_probe(struct platform_device *pdev) > > return PTR_ERR(csi->bus_clk); > > } > > > > - csi->isp_clk = devm_clk_get(&pdev->dev, "isp"); > > - if (IS_ERR(csi->isp_clk)) { > > - dev_err(&pdev->dev, "Couldn't get our ISP clock\n"); > > - return PTR_ERR(csi->isp_clk); > > + if (csi->traits->has_isp) { > > + csi->isp_clk = devm_clk_get(&pdev->dev, "isp"); > > + if (IS_ERR(csi->isp_clk)) { > > + dev_err(&pdev->dev, "Couldn't get our ISP clock\n"); > > + return PTR_ERR(csi->isp_clk); > > + } > > } > > > > csi->ram_clk = devm_clk_get(&pdev->dev, "ram"); > > @@ -280,8 +292,21 @@ static int sun4i_csi_remove(struct platform_device *pdev) > > return 0; > > } > > > > +struct sun4i_csi_traits sun4i_a10_csi1_traits = { > > + .channels = 1, > > + .max_width = 24, > > + .has_isp = false, > > +}; > > + > > +struct sun4i_csi_traits sun7i_a20_csi0_traits = { > > These two should be static const, right? You're right. I'll wait for the remaining reviews before sending v2. ChenYu > > + .channels = 4, > > + .max_width = 16, > > + .has_isp = true, > > +}; > > + > > static const struct of_device_id sun4i_csi_of_match[] = { > > - { .compatible = "allwinner,sun7i-a20-csi0" }, > > + { .compatible = "allwinner,sun4i-a10-csi1", .data = &sun4i_a10_csi1_traits }, > > + { .compatible = "allwinner,sun7i-a20-csi0", .data = &sun7i_a20_csi0_traits }, > > { /* Sentinel */ } > > }; > > MODULE_DEVICE_TABLE(of, sun4i_csi_of_match); > > diff --git a/drivers/media/platform/sunxi/sun4i-csi/sun4i_csi.h b/drivers/media/platform/sunxi/sun4i-csi/sun4i_csi.h > > index 88d39b3554c4..0f67ff652c2e 100644 > > --- a/drivers/media/platform/sunxi/sun4i-csi/sun4i_csi.h > > +++ b/drivers/media/platform/sunxi/sun4i-csi/sun4i_csi.h > > @@ -108,6 +108,8 @@ struct sun4i_csi { > > /* Device resources */ > > struct device *dev; > > > > + const struct sun4i_csi_traits *traits; > > + > > void __iomem *regs; > > struct clk *bus_clk; > > struct clk *isp_clk; > > -- > Kind regards, > > Sakari Ailus
diff --git a/drivers/media/platform/sunxi/sun4i-csi/sun4i_csi.c b/drivers/media/platform/sunxi/sun4i-csi/sun4i_csi.c index b8b07c1de2a8..be2466930a49 100644 --- a/drivers/media/platform/sunxi/sun4i-csi/sun4i_csi.c +++ b/drivers/media/platform/sunxi/sun4i-csi/sun4i_csi.c @@ -29,6 +29,12 @@ #include "sun4i_csi.h" +struct sun4i_csi_traits { + unsigned int channels; + unsigned int max_width; + bool has_isp; +}; + static const struct media_entity_operations sun4i_csi_video_entity_ops = { .link_validate = v4l2_subdev_link_validate, }; @@ -156,6 +162,10 @@ static int sun4i_csi_probe(struct platform_device *pdev) subdev = &csi->subdev; vdev = &csi->vdev; + csi->traits = of_device_get_match_data(&pdev->dev); + if (!csi->traits) + return -EINVAL; + /* * On Allwinner SoCs, some high memory bandwidth devices do DMA * directly over the memory bus (called MBUS), instead of the @@ -199,10 +209,12 @@ static int sun4i_csi_probe(struct platform_device *pdev) return PTR_ERR(csi->bus_clk); } - csi->isp_clk = devm_clk_get(&pdev->dev, "isp"); - if (IS_ERR(csi->isp_clk)) { - dev_err(&pdev->dev, "Couldn't get our ISP clock\n"); - return PTR_ERR(csi->isp_clk); + if (csi->traits->has_isp) { + csi->isp_clk = devm_clk_get(&pdev->dev, "isp"); + if (IS_ERR(csi->isp_clk)) { + dev_err(&pdev->dev, "Couldn't get our ISP clock\n"); + return PTR_ERR(csi->isp_clk); + } } csi->ram_clk = devm_clk_get(&pdev->dev, "ram"); @@ -280,8 +292,21 @@ static int sun4i_csi_remove(struct platform_device *pdev) return 0; } +struct sun4i_csi_traits sun4i_a10_csi1_traits = { + .channels = 1, + .max_width = 24, + .has_isp = false, +}; + +struct sun4i_csi_traits sun7i_a20_csi0_traits = { + .channels = 4, + .max_width = 16, + .has_isp = true, +}; + static const struct of_device_id sun4i_csi_of_match[] = { - { .compatible = "allwinner,sun7i-a20-csi0" }, + { .compatible = "allwinner,sun4i-a10-csi1", .data = &sun4i_a10_csi1_traits }, + { .compatible = "allwinner,sun7i-a20-csi0", .data = &sun7i_a20_csi0_traits }, { /* Sentinel */ } }; MODULE_DEVICE_TABLE(of, sun4i_csi_of_match); diff --git a/drivers/media/platform/sunxi/sun4i-csi/sun4i_csi.h b/drivers/media/platform/sunxi/sun4i-csi/sun4i_csi.h index 88d39b3554c4..0f67ff652c2e 100644 --- a/drivers/media/platform/sunxi/sun4i-csi/sun4i_csi.h +++ b/drivers/media/platform/sunxi/sun4i-csi/sun4i_csi.h @@ -108,6 +108,8 @@ struct sun4i_csi { /* Device resources */ struct device *dev; + const struct sun4i_csi_traits *traits; + void __iomem *regs; struct clk *bus_clk; struct clk *isp_clk;