| Message ID | 20230105134729.59542-7-m.tretter@pengutronix.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | media: imx-pxp: add support for i.MX7D | expand |
Hi Michael, Thank you for the patch. On Thu, Jan 05, 2023 at 02:47:27PM +0100, Michael Tretter wrote: > Unfortunately, the PXP_HW_VERSION register reports the PXP on the i.MX7D > and on the i.MX6ULL as version 3.0, although the PXP versions on these > SoCs have significant differences. > > Use the compatible to configure the ctrl0 register as required dependent > on the platform. > > Signed-off-by: Michael Tretter <m.tretter@pengutronix.de> > --- > drivers/media/platform/nxp/imx-pxp.c | 27 +++++++++++++++++++++++++-- > 1 file changed, 25 insertions(+), 2 deletions(-) > > diff --git a/drivers/media/platform/nxp/imx-pxp.c b/drivers/media/platform/nxp/imx-pxp.c > index 1d649b9cadad..4e182f80a36b 100644 > --- a/drivers/media/platform/nxp/imx-pxp.c > +++ b/drivers/media/platform/nxp/imx-pxp.c > @@ -19,6 +19,7 @@ > #include <linux/iopoll.h> > #include <linux/module.h> > #include <linux/of.h> > +#include <linux/of_device.h> > #include <linux/sched.h> > #include <linux/slab.h> > > @@ -191,6 +192,11 @@ static struct pxp_fmt *find_format(struct v4l2_format *f) > return &formats[k]; > } > > +struct pxp_ctx; Please add a blank line here. > +struct pxp_pdata { > + u32 (*data_path_ctrl0)(struct pxp_ctx *ctx); > +}; > + > struct pxp_dev { > struct v4l2_device v4l2_dev; > struct video_device vfd; > @@ -199,6 +205,7 @@ struct pxp_dev { > void __iomem *mmio; > > u32 hw_version; > + const struct pxp_pdata *pdata; > > atomic_t num_inst; > struct mutex dev_mutex; > @@ -726,7 +733,7 @@ static void pxp_setup_csc(struct pxp_ctx *ctx) > } > } > > -static u32 pxp_data_path_ctrl0(struct pxp_ctx *ctx) > +static u32 pxp_imx6ull_data_path_ctrl0(struct pxp_ctx *ctx) > { > u32 ctrl0; > > @@ -756,6 +763,16 @@ static u32 pxp_data_path_ctrl0(struct pxp_ctx *ctx) > return ctrl0; > } > > +static u32 pxp_data_path_ctrl0(struct pxp_ctx *ctx) > +{ > + struct pxp_dev *dev = ctx->dev; > + > + if (dev->pdata && dev->pdata->data_path_ctrl0) > + return dev->pdata->data_path_ctrl0(ctx); > + > + return pxp_imx6ull_data_path_ctrl0(ctx); Do you need this fallback, given that all compatible strings give you valid pdata ? I'd rather be explicit. This function then becomes so small that I would inline it in the caller. > +} > + > static void pxp_set_data_path(struct pxp_ctx *ctx) > { > struct pxp_dev *dev = ctx->dev; > @@ -1711,6 +1728,8 @@ static int pxp_probe(struct platform_device *pdev) > if (!dev) > return -ENOMEM; > > + dev->pdata = of_device_get_match_data(&pdev->dev); > + > dev->clk = devm_clk_get(&pdev->dev, "axi"); > if (IS_ERR(dev->clk)) { > ret = PTR_ERR(dev->clk); > @@ -1811,8 +1830,12 @@ static int pxp_remove(struct platform_device *pdev) > return 0; > } > > +static const struct pxp_pdata pxp_imx6ull_pdata = { > + .data_path_ctrl0 = pxp_imx6ull_data_path_ctrl0, > +}; > + > static const struct of_device_id pxp_dt_ids[] = { > - { .compatible = "fsl,imx6ull-pxp", .data = NULL }, > + { .compatible = "fsl,imx6ull-pxp", .data = &pxp_imx6ull_pdata }, > { }, > }; > MODULE_DEVICE_TABLE(of, pxp_dt_ids);
On Fri, 06 Jan 2023 14:30:52 +0200, Laurent Pinchart wrote: > On Thu, Jan 05, 2023 at 02:47:27PM +0100, Michael Tretter wrote: > > Unfortunately, the PXP_HW_VERSION register reports the PXP on the i.MX7D > > and on the i.MX6ULL as version 3.0, although the PXP versions on these > > SoCs have significant differences. > > > > Use the compatible to configure the ctrl0 register as required dependent > > on the platform. > > > > Signed-off-by: Michael Tretter <m.tretter@pengutronix.de> > > --- > > drivers/media/platform/nxp/imx-pxp.c | 27 +++++++++++++++++++++++++-- > > 1 file changed, 25 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/media/platform/nxp/imx-pxp.c b/drivers/media/platform/nxp/imx-pxp.c > > index 1d649b9cadad..4e182f80a36b 100644 > > --- a/drivers/media/platform/nxp/imx-pxp.c > > +++ b/drivers/media/platform/nxp/imx-pxp.c > > @@ -19,6 +19,7 @@ > > #include <linux/iopoll.h> > > #include <linux/module.h> > > #include <linux/of.h> > > +#include <linux/of_device.h> > > #include <linux/sched.h> > > #include <linux/slab.h> > > > > @@ -191,6 +192,11 @@ static struct pxp_fmt *find_format(struct v4l2_format *f) > > return &formats[k]; > > } > > > > +struct pxp_ctx; > > Please add a blank line here. > > > +struct pxp_pdata { > > + u32 (*data_path_ctrl0)(struct pxp_ctx *ctx); > > +}; > > + > > struct pxp_dev { > > struct v4l2_device v4l2_dev; > > struct video_device vfd; > > @@ -199,6 +205,7 @@ struct pxp_dev { > > void __iomem *mmio; > > > > u32 hw_version; > > + const struct pxp_pdata *pdata; > > > > atomic_t num_inst; > > struct mutex dev_mutex; > > @@ -726,7 +733,7 @@ static void pxp_setup_csc(struct pxp_ctx *ctx) > > } > > } > > > > -static u32 pxp_data_path_ctrl0(struct pxp_ctx *ctx) > > +static u32 pxp_imx6ull_data_path_ctrl0(struct pxp_ctx *ctx) > > { > > u32 ctrl0; > > > > @@ -756,6 +763,16 @@ static u32 pxp_data_path_ctrl0(struct pxp_ctx *ctx) > > return ctrl0; > > } > > > > +static u32 pxp_data_path_ctrl0(struct pxp_ctx *ctx) > > +{ > > + struct pxp_dev *dev = ctx->dev; > > + > > + if (dev->pdata && dev->pdata->data_path_ctrl0) > > + return dev->pdata->data_path_ctrl0(ctx); > > + > > + return pxp_imx6ull_data_path_ctrl0(ctx); > > Do you need this fallback, given that all compatible strings give you > valid pdata ? I'd rather be explicit. > > This function then becomes so small that I would inline it in the > caller. I was a bit paranoid that there may be cases in which pdata is not set. I will change this to assume that pdata is always valid and just be explicit. Michael > > > +} > > + > > static void pxp_set_data_path(struct pxp_ctx *ctx) > > { > > struct pxp_dev *dev = ctx->dev; > > @@ -1711,6 +1728,8 @@ static int pxp_probe(struct platform_device *pdev) > > if (!dev) > > return -ENOMEM; > > > > + dev->pdata = of_device_get_match_data(&pdev->dev); > > + > > dev->clk = devm_clk_get(&pdev->dev, "axi"); > > if (IS_ERR(dev->clk)) { > > ret = PTR_ERR(dev->clk); > > @@ -1811,8 +1830,12 @@ static int pxp_remove(struct platform_device *pdev) > > return 0; > > } > > > > +static const struct pxp_pdata pxp_imx6ull_pdata = { > > + .data_path_ctrl0 = pxp_imx6ull_data_path_ctrl0, > > +}; > > + > > static const struct of_device_id pxp_dt_ids[] = { > > - { .compatible = "fsl,imx6ull-pxp", .data = NULL }, > > + { .compatible = "fsl,imx6ull-pxp", .data = &pxp_imx6ull_pdata }, > > { }, > > }; > > MODULE_DEVICE_TABLE(of, pxp_dt_ids);
On Fri, Jan 06, 2023 at 03:11:41PM +0100, Michael Tretter wrote: > On Fri, 06 Jan 2023 14:30:52 +0200, Laurent Pinchart wrote: > > On Thu, Jan 05, 2023 at 02:47:27PM +0100, Michael Tretter wrote: > > > Unfortunately, the PXP_HW_VERSION register reports the PXP on the i.MX7D > > > and on the i.MX6ULL as version 3.0, although the PXP versions on these > > > SoCs have significant differences. > > > > > > Use the compatible to configure the ctrl0 register as required dependent > > > on the platform. > > > > > > Signed-off-by: Michael Tretter <m.tretter@pengutronix.de> > > > --- > > > drivers/media/platform/nxp/imx-pxp.c | 27 +++++++++++++++++++++++++-- > > > 1 file changed, 25 insertions(+), 2 deletions(-) > > > > > > diff --git a/drivers/media/platform/nxp/imx-pxp.c b/drivers/media/platform/nxp/imx-pxp.c > > > index 1d649b9cadad..4e182f80a36b 100644 > > > --- a/drivers/media/platform/nxp/imx-pxp.c > > > +++ b/drivers/media/platform/nxp/imx-pxp.c > > > @@ -19,6 +19,7 @@ > > > #include <linux/iopoll.h> > > > #include <linux/module.h> > > > #include <linux/of.h> > > > +#include <linux/of_device.h> > > > #include <linux/sched.h> > > > #include <linux/slab.h> > > > > > > @@ -191,6 +192,11 @@ static struct pxp_fmt *find_format(struct v4l2_format *f) > > > return &formats[k]; > > > } > > > > > > +struct pxp_ctx; > > > > Please add a blank line here. > > > > > +struct pxp_pdata { > > > + u32 (*data_path_ctrl0)(struct pxp_ctx *ctx); > > > +}; > > > + > > > struct pxp_dev { > > > struct v4l2_device v4l2_dev; > > > struct video_device vfd; > > > @@ -199,6 +205,7 @@ struct pxp_dev { > > > void __iomem *mmio; > > > > > > u32 hw_version; > > > + const struct pxp_pdata *pdata; > > > > > > atomic_t num_inst; > > > struct mutex dev_mutex; > > > @@ -726,7 +733,7 @@ static void pxp_setup_csc(struct pxp_ctx *ctx) > > > } > > > } > > > > > > -static u32 pxp_data_path_ctrl0(struct pxp_ctx *ctx) > > > +static u32 pxp_imx6ull_data_path_ctrl0(struct pxp_ctx *ctx) > > > { > > > u32 ctrl0; > > > > > > @@ -756,6 +763,16 @@ static u32 pxp_data_path_ctrl0(struct pxp_ctx *ctx) > > > return ctrl0; > > > } > > > > > > +static u32 pxp_data_path_ctrl0(struct pxp_ctx *ctx) > > > +{ > > > + struct pxp_dev *dev = ctx->dev; > > > + > > > + if (dev->pdata && dev->pdata->data_path_ctrl0) > > > + return dev->pdata->data_path_ctrl0(ctx); > > > + > > > + return pxp_imx6ull_data_path_ctrl0(ctx); > > > > Do you need this fallback, given that all compatible strings give you > > valid pdata ? I'd rather be explicit. > > > > This function then becomes so small that I would inline it in the > > caller. > > I was a bit paranoid that there may be cases in which pdata is not set. I will > change this to assume that pdata is always valid and just be explicit. If you're worried that future addition of platform support could add a compatible string with NULL .data, you could catch that in probe(). I'm not worried personally, as it would crash on first use, so the developer submitting the patch should notice. > > > +} > > > + > > > static void pxp_set_data_path(struct pxp_ctx *ctx) > > > { > > > struct pxp_dev *dev = ctx->dev; > > > @@ -1711,6 +1728,8 @@ static int pxp_probe(struct platform_device *pdev) > > > if (!dev) > > > return -ENOMEM; > > > > > > + dev->pdata = of_device_get_match_data(&pdev->dev); > > > + > > > dev->clk = devm_clk_get(&pdev->dev, "axi"); > > > if (IS_ERR(dev->clk)) { > > > ret = PTR_ERR(dev->clk); > > > @@ -1811,8 +1830,12 @@ static int pxp_remove(struct platform_device *pdev) > > > return 0; > > > } > > > > > > +static const struct pxp_pdata pxp_imx6ull_pdata = { > > > + .data_path_ctrl0 = pxp_imx6ull_data_path_ctrl0, > > > +}; > > > + > > > static const struct of_device_id pxp_dt_ids[] = { > > > - { .compatible = "fsl,imx6ull-pxp", .data = NULL }, > > > + { .compatible = "fsl,imx6ull-pxp", .data = &pxp_imx6ull_pdata }, > > > { }, > > > }; > > > MODULE_DEVICE_TABLE(of, pxp_dt_ids);
diff --git a/drivers/media/platform/nxp/imx-pxp.c b/drivers/media/platform/nxp/imx-pxp.c index 1d649b9cadad..4e182f80a36b 100644 --- a/drivers/media/platform/nxp/imx-pxp.c +++ b/drivers/media/platform/nxp/imx-pxp.c @@ -19,6 +19,7 @@ #include <linux/iopoll.h> #include <linux/module.h> #include <linux/of.h> +#include <linux/of_device.h> #include <linux/sched.h> #include <linux/slab.h> @@ -191,6 +192,11 @@ static struct pxp_fmt *find_format(struct v4l2_format *f) return &formats[k]; } +struct pxp_ctx; +struct pxp_pdata { + u32 (*data_path_ctrl0)(struct pxp_ctx *ctx); +}; + struct pxp_dev { struct v4l2_device v4l2_dev; struct video_device vfd; @@ -199,6 +205,7 @@ struct pxp_dev { void __iomem *mmio; u32 hw_version; + const struct pxp_pdata *pdata; atomic_t num_inst; struct mutex dev_mutex; @@ -726,7 +733,7 @@ static void pxp_setup_csc(struct pxp_ctx *ctx) } } -static u32 pxp_data_path_ctrl0(struct pxp_ctx *ctx) +static u32 pxp_imx6ull_data_path_ctrl0(struct pxp_ctx *ctx) { u32 ctrl0; @@ -756,6 +763,16 @@ static u32 pxp_data_path_ctrl0(struct pxp_ctx *ctx) return ctrl0; } +static u32 pxp_data_path_ctrl0(struct pxp_ctx *ctx) +{ + struct pxp_dev *dev = ctx->dev; + + if (dev->pdata && dev->pdata->data_path_ctrl0) + return dev->pdata->data_path_ctrl0(ctx); + + return pxp_imx6ull_data_path_ctrl0(ctx); +} + static void pxp_set_data_path(struct pxp_ctx *ctx) { struct pxp_dev *dev = ctx->dev; @@ -1711,6 +1728,8 @@ static int pxp_probe(struct platform_device *pdev) if (!dev) return -ENOMEM; + dev->pdata = of_device_get_match_data(&pdev->dev); + dev->clk = devm_clk_get(&pdev->dev, "axi"); if (IS_ERR(dev->clk)) { ret = PTR_ERR(dev->clk); @@ -1811,8 +1830,12 @@ static int pxp_remove(struct platform_device *pdev) return 0; } +static const struct pxp_pdata pxp_imx6ull_pdata = { + .data_path_ctrl0 = pxp_imx6ull_data_path_ctrl0, +}; + static const struct of_device_id pxp_dt_ids[] = { - { .compatible = "fsl,imx6ull-pxp", .data = NULL }, + { .compatible = "fsl,imx6ull-pxp", .data = &pxp_imx6ull_pdata }, { }, }; MODULE_DEVICE_TABLE(of, pxp_dt_ids);
Unfortunately, the PXP_HW_VERSION register reports the PXP on the i.MX7D and on the i.MX6ULL as version 3.0, although the PXP versions on these SoCs have significant differences. Use the compatible to configure the ctrl0 register as required dependent on the platform. Signed-off-by: Michael Tretter <m.tretter@pengutronix.de> --- drivers/media/platform/nxp/imx-pxp.c | 27 +++++++++++++++++++++++++-- 1 file changed, 25 insertions(+), 2 deletions(-)