| Message ID | 20230105134729.59542-3-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:23PM +0100, Michael Tretter wrote: > Different versions of the Pixel Pipeline have different blocks and their > routing may be different. Read the PXP_HW_VERSION register to determine > the version of the PXP and print it to the log for debugging purposes. Is there a specific reason you chose to read the version register instead of basing the decision on the compatible string ? > Signed-off-by: Michael Tretter <m.tretter@pengutronix.de> > --- > drivers/media/platform/nxp/imx-pxp.c | 18 ++++++++++++++++++ > 1 file changed, 18 insertions(+) > > diff --git a/drivers/media/platform/nxp/imx-pxp.c b/drivers/media/platform/nxp/imx-pxp.c > index 689ae5e6ac62..05abe40558b0 100644 > --- a/drivers/media/platform/nxp/imx-pxp.c > +++ b/drivers/media/platform/nxp/imx-pxp.c > @@ -10,6 +10,7 @@ > * Pawel Osciak, <pawel@osciak.com> > * Marek Szyprowski, <m.szyprowski@samsung.com> > */ > +#include <linux/bitfield.h> > #include <linux/clk.h> > #include <linux/delay.h> > #include <linux/dma-mapping.h> > @@ -52,6 +53,11 @@ MODULE_PARM_DESC(debug, "activates debug info"); > #define MEM2MEM_HFLIP (1 << 0) > #define MEM2MEM_VFLIP (1 << 1) > > +#define PXP_VERSION_MAJOR(version) \ > + FIELD_GET(BM_PXP_VERSION_MAJOR, version) > +#define PXP_VERSION_MINOR(version) \ > + FIELD_GET(BM_PXP_VERSION_MINOR, version) > + > #define dprintk(dev, fmt, arg...) \ > v4l2_dbg(1, debug, &dev->v4l2_dev, "%s: " fmt, __func__, ## arg) > > @@ -192,6 +198,8 @@ struct pxp_dev { > struct clk *clk; > void __iomem *mmio; > > + u32 hw_version; > + > atomic_t num_inst; > struct mutex dev_mutex; > spinlock_t irqlock; > @@ -1660,6 +1668,11 @@ static int pxp_soft_reset(struct pxp_dev *dev) > return 0; > } > > +static u32 pxp_read_version(struct pxp_dev *dev) > +{ > + return readl(dev->mmio + HW_PXP_VERSION); > +} > + > static int pxp_probe(struct platform_device *pdev) > { > struct pxp_dev *dev; > @@ -1705,6 +1718,11 @@ static int pxp_probe(struct platform_device *pdev) > goto err_clk; > } > > + dev->hw_version = pxp_read_version(dev); > + dev_info(&pdev->dev, "PXP Version %d.%d\n", As the version can't be negative, I'd use %u.%u. > + PXP_VERSION_MAJOR(dev->hw_version), > + PXP_VERSION_MINOR(dev->hw_version)); > + The driver now prints two messages at probe time, it would be nice to combine them, or remove the other one. That's a candidate for a future patch though. > ret = v4l2_device_register(&pdev->dev, &dev->v4l2_dev); > if (ret) > goto err_clk;
On Fri, Jan 06, 2023 at 01:47:32PM +0200, Laurent Pinchart wrote: > Hi Michael, > > Thank you for the patch. > > On Thu, Jan 05, 2023 at 02:47:23PM +0100, Michael Tretter wrote: > > Different versions of the Pixel Pipeline have different blocks and their > > routing may be different. Read the PXP_HW_VERSION register to determine > > the version of the PXP and print it to the log for debugging purposes. > > Is there a specific reason you chose to read the version register > instead of basing the decision on the compatible string ? Reading the rest of the series, you use the compatible strings later, and never use the hw_version field. I'm tempted to propose dropping this patch. > > Signed-off-by: Michael Tretter <m.tretter@pengutronix.de> > > --- > > drivers/media/platform/nxp/imx-pxp.c | 18 ++++++++++++++++++ > > 1 file changed, 18 insertions(+) > > > > diff --git a/drivers/media/platform/nxp/imx-pxp.c b/drivers/media/platform/nxp/imx-pxp.c > > index 689ae5e6ac62..05abe40558b0 100644 > > --- a/drivers/media/platform/nxp/imx-pxp.c > > +++ b/drivers/media/platform/nxp/imx-pxp.c > > @@ -10,6 +10,7 @@ > > * Pawel Osciak, <pawel@osciak.com> > > * Marek Szyprowski, <m.szyprowski@samsung.com> > > */ > > +#include <linux/bitfield.h> > > #include <linux/clk.h> > > #include <linux/delay.h> > > #include <linux/dma-mapping.h> > > @@ -52,6 +53,11 @@ MODULE_PARM_DESC(debug, "activates debug info"); > > #define MEM2MEM_HFLIP (1 << 0) > > #define MEM2MEM_VFLIP (1 << 1) > > > > +#define PXP_VERSION_MAJOR(version) \ > > + FIELD_GET(BM_PXP_VERSION_MAJOR, version) > > +#define PXP_VERSION_MINOR(version) \ > > + FIELD_GET(BM_PXP_VERSION_MINOR, version) > > + > > #define dprintk(dev, fmt, arg...) \ > > v4l2_dbg(1, debug, &dev->v4l2_dev, "%s: " fmt, __func__, ## arg) > > > > @@ -192,6 +198,8 @@ struct pxp_dev { > > struct clk *clk; > > void __iomem *mmio; > > > > + u32 hw_version; > > + > > atomic_t num_inst; > > struct mutex dev_mutex; > > spinlock_t irqlock; > > @@ -1660,6 +1668,11 @@ static int pxp_soft_reset(struct pxp_dev *dev) > > return 0; > > } > > > > +static u32 pxp_read_version(struct pxp_dev *dev) > > +{ > > + return readl(dev->mmio + HW_PXP_VERSION); > > +} > > + > > static int pxp_probe(struct platform_device *pdev) > > { > > struct pxp_dev *dev; > > @@ -1705,6 +1718,11 @@ static int pxp_probe(struct platform_device *pdev) > > goto err_clk; > > } > > > > + dev->hw_version = pxp_read_version(dev); > > + dev_info(&pdev->dev, "PXP Version %d.%d\n", > > As the version can't be negative, I'd use %u.%u. > > > + PXP_VERSION_MAJOR(dev->hw_version), > > + PXP_VERSION_MINOR(dev->hw_version)); > > + > > The driver now prints two messages at probe time, it would be nice to > combine them, or remove the other one. That's a candidate for a future > patch though. > > > ret = v4l2_device_register(&pdev->dev, &dev->v4l2_dev); > > if (ret) > > goto err_clk;
On Fri, 06 Jan 2023 14:28:47 +0200, Laurent Pinchart wrote: > On Fri, Jan 06, 2023 at 01:47:32PM +0200, Laurent Pinchart wrote: > > Thank you for the patch. > > > > On Thu, Jan 05, 2023 at 02:47:23PM +0100, Michael Tretter wrote: > > > Different versions of the Pixel Pipeline have different blocks and their > > > routing may be different. Read the PXP_HW_VERSION register to determine > > > the version of the PXP and print it to the log for debugging purposes. > > > > Is there a specific reason you chose to read the version register > > instead of basing the decision on the compatible string ? > > Reading the rest of the series, you use the compatible strings later, > and never use the hw_version field. I'm tempted to propose dropping this > patch. My first try was to use the version register, but it turned out that the version is the same at least on i.MX6ULL and i.MX7D. I kept it to avoid that others fall into the same trap. > > > > Signed-off-by: Michael Tretter <m.tretter@pengutronix.de> > > > --- > > > drivers/media/platform/nxp/imx-pxp.c | 18 ++++++++++++++++++ > > > 1 file changed, 18 insertions(+) > > > > > > diff --git a/drivers/media/platform/nxp/imx-pxp.c b/drivers/media/platform/nxp/imx-pxp.c > > > index 689ae5e6ac62..05abe40558b0 100644 > > > --- a/drivers/media/platform/nxp/imx-pxp.c > > > +++ b/drivers/media/platform/nxp/imx-pxp.c > > > @@ -10,6 +10,7 @@ > > > * Pawel Osciak, <pawel@osciak.com> > > > * Marek Szyprowski, <m.szyprowski@samsung.com> > > > */ > > > +#include <linux/bitfield.h> > > > #include <linux/clk.h> > > > #include <linux/delay.h> > > > #include <linux/dma-mapping.h> > > > @@ -52,6 +53,11 @@ MODULE_PARM_DESC(debug, "activates debug info"); > > > #define MEM2MEM_HFLIP (1 << 0) > > > #define MEM2MEM_VFLIP (1 << 1) > > > > > > +#define PXP_VERSION_MAJOR(version) \ > > > + FIELD_GET(BM_PXP_VERSION_MAJOR, version) > > > +#define PXP_VERSION_MINOR(version) \ > > > + FIELD_GET(BM_PXP_VERSION_MINOR, version) > > > + > > > #define dprintk(dev, fmt, arg...) \ > > > v4l2_dbg(1, debug, &dev->v4l2_dev, "%s: " fmt, __func__, ## arg) > > > > > > @@ -192,6 +198,8 @@ struct pxp_dev { > > > struct clk *clk; > > > void __iomem *mmio; > > > > > > + u32 hw_version; > > > + > > > atomic_t num_inst; > > > struct mutex dev_mutex; > > > spinlock_t irqlock; > > > @@ -1660,6 +1668,11 @@ static int pxp_soft_reset(struct pxp_dev *dev) > > > return 0; > > > } > > > > > > +static u32 pxp_read_version(struct pxp_dev *dev) > > > +{ > > > + return readl(dev->mmio + HW_PXP_VERSION); > > > +} > > > + > > > static int pxp_probe(struct platform_device *pdev) > > > { > > > struct pxp_dev *dev; > > > @@ -1705,6 +1718,11 @@ static int pxp_probe(struct platform_device *pdev) > > > goto err_clk; > > > } > > > > > > + dev->hw_version = pxp_read_version(dev); > > > + dev_info(&pdev->dev, "PXP Version %d.%d\n", > > > > As the version can't be negative, I'd use %u.%u. > > > > > + PXP_VERSION_MAJOR(dev->hw_version), > > > + PXP_VERSION_MINOR(dev->hw_version)); > > > + > > > > The driver now prints two messages at probe time, it would be nice to > > combine them, or remove the other one. That's a candidate for a future > > patch though. I would reduce the level to dev_debug. Then the version is not always printed, but it can be easily enabled if necessary for the bringup on another platform. Michael > > > > > ret = v4l2_device_register(&pdev->dev, &dev->v4l2_dev); > > > if (ret) > > > goto err_clk;
On Fri, Jan 06, 2023 at 03:01:41PM +0100, Michael Tretter wrote: > On Fri, 06 Jan 2023 14:28:47 +0200, Laurent Pinchart wrote: > > On Fri, Jan 06, 2023 at 01:47:32PM +0200, Laurent Pinchart wrote: > > > Thank you for the patch. > > > > > > On Thu, Jan 05, 2023 at 02:47:23PM +0100, Michael Tretter wrote: > > > > Different versions of the Pixel Pipeline have different blocks and their > > > > routing may be different. Read the PXP_HW_VERSION register to determine > > > > the version of the PXP and print it to the log for debugging purposes. > > > > > > Is there a specific reason you chose to read the version register > > > instead of basing the decision on the compatible string ? > > > > Reading the rest of the series, you use the compatible strings later, > > and never use the hw_version field. I'm tempted to propose dropping this > > patch. > > My first try was to use the version register, but it turned out that the > version is the same at least on i.MX6ULL and i.MX7D. I kept it to avoid that > others fall into the same trap. > > > > > > > Signed-off-by: Michael Tretter <m.tretter@pengutronix.de> > > > > --- > > > > drivers/media/platform/nxp/imx-pxp.c | 18 ++++++++++++++++++ > > > > 1 file changed, 18 insertions(+) > > > > > > > > diff --git a/drivers/media/platform/nxp/imx-pxp.c b/drivers/media/platform/nxp/imx-pxp.c > > > > index 689ae5e6ac62..05abe40558b0 100644 > > > > --- a/drivers/media/platform/nxp/imx-pxp.c > > > > +++ b/drivers/media/platform/nxp/imx-pxp.c > > > > @@ -10,6 +10,7 @@ > > > > * Pawel Osciak, <pawel@osciak.com> > > > > * Marek Szyprowski, <m.szyprowski@samsung.com> > > > > */ > > > > +#include <linux/bitfield.h> > > > > #include <linux/clk.h> > > > > #include <linux/delay.h> > > > > #include <linux/dma-mapping.h> > > > > @@ -52,6 +53,11 @@ MODULE_PARM_DESC(debug, "activates debug info"); > > > > #define MEM2MEM_HFLIP (1 << 0) > > > > #define MEM2MEM_VFLIP (1 << 1) > > > > > > > > +#define PXP_VERSION_MAJOR(version) \ > > > > + FIELD_GET(BM_PXP_VERSION_MAJOR, version) > > > > +#define PXP_VERSION_MINOR(version) \ > > > > + FIELD_GET(BM_PXP_VERSION_MINOR, version) > > > > + > > > > #define dprintk(dev, fmt, arg...) \ > > > > v4l2_dbg(1, debug, &dev->v4l2_dev, "%s: " fmt, __func__, ## arg) > > > > > > > > @@ -192,6 +198,8 @@ struct pxp_dev { > > > > struct clk *clk; > > > > void __iomem *mmio; > > > > > > > > + u32 hw_version; > > > > + > > > > atomic_t num_inst; > > > > struct mutex dev_mutex; > > > > spinlock_t irqlock; > > > > @@ -1660,6 +1668,11 @@ static int pxp_soft_reset(struct pxp_dev *dev) > > > > return 0; > > > > } > > > > > > > > +static u32 pxp_read_version(struct pxp_dev *dev) > > > > +{ > > > > + return readl(dev->mmio + HW_PXP_VERSION); > > > > +} > > > > + > > > > static int pxp_probe(struct platform_device *pdev) > > > > { > > > > struct pxp_dev *dev; > > > > @@ -1705,6 +1718,11 @@ static int pxp_probe(struct platform_device *pdev) > > > > goto err_clk; > > > > } > > > > > > > > + dev->hw_version = pxp_read_version(dev); > > > > + dev_info(&pdev->dev, "PXP Version %d.%d\n", > > > > > > As the version can't be negative, I'd use %u.%u. > > > > > > > + PXP_VERSION_MAJOR(dev->hw_version), > > > > + PXP_VERSION_MINOR(dev->hw_version)); > > > > + > > > > > > The driver now prints two messages at probe time, it would be nice to > > > combine them, or remove the other one. That's a candidate for a future > > > patch though. > > I would reduce the level to dev_debug. Then the version is not always printed, > but it can be easily enabled if necessary for the bringup on another platform. Fine with me. > > > > ret = v4l2_device_register(&pdev->dev, &dev->v4l2_dev); > > > > if (ret) > > > > goto err_clk;
diff --git a/drivers/media/platform/nxp/imx-pxp.c b/drivers/media/platform/nxp/imx-pxp.c index 689ae5e6ac62..05abe40558b0 100644 --- a/drivers/media/platform/nxp/imx-pxp.c +++ b/drivers/media/platform/nxp/imx-pxp.c @@ -10,6 +10,7 @@ * Pawel Osciak, <pawel@osciak.com> * Marek Szyprowski, <m.szyprowski@samsung.com> */ +#include <linux/bitfield.h> #include <linux/clk.h> #include <linux/delay.h> #include <linux/dma-mapping.h> @@ -52,6 +53,11 @@ MODULE_PARM_DESC(debug, "activates debug info"); #define MEM2MEM_HFLIP (1 << 0) #define MEM2MEM_VFLIP (1 << 1) +#define PXP_VERSION_MAJOR(version) \ + FIELD_GET(BM_PXP_VERSION_MAJOR, version) +#define PXP_VERSION_MINOR(version) \ + FIELD_GET(BM_PXP_VERSION_MINOR, version) + #define dprintk(dev, fmt, arg...) \ v4l2_dbg(1, debug, &dev->v4l2_dev, "%s: " fmt, __func__, ## arg) @@ -192,6 +198,8 @@ struct pxp_dev { struct clk *clk; void __iomem *mmio; + u32 hw_version; + atomic_t num_inst; struct mutex dev_mutex; spinlock_t irqlock; @@ -1660,6 +1668,11 @@ static int pxp_soft_reset(struct pxp_dev *dev) return 0; } +static u32 pxp_read_version(struct pxp_dev *dev) +{ + return readl(dev->mmio + HW_PXP_VERSION); +} + static int pxp_probe(struct platform_device *pdev) { struct pxp_dev *dev; @@ -1705,6 +1718,11 @@ static int pxp_probe(struct platform_device *pdev) goto err_clk; } + dev->hw_version = pxp_read_version(dev); + dev_info(&pdev->dev, "PXP Version %d.%d\n", + PXP_VERSION_MAJOR(dev->hw_version), + PXP_VERSION_MINOR(dev->hw_version)); + ret = v4l2_device_register(&pdev->dev, &dev->v4l2_dev); if (ret) goto err_clk;
Different versions of the Pixel Pipeline have different blocks and their routing may be different. Read the PXP_HW_VERSION register to determine the version of the PXP and print it to the log for debugging purposes. Signed-off-by: Michael Tretter <m.tretter@pengutronix.de> --- drivers/media/platform/nxp/imx-pxp.c | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+)