| Message ID | 20220614191127.3420492-8-paul.elder@ideasonboard.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | media: rkisp1: Cleanups and add support for i.MX8MP | expand |
On 15.06.2022 04:10, Paul Elder wrote: >To make it possible to use the rkisp1_info after probe time (for >instance to make code conditional on the ISP version), save it in the >main rkisp1_device structure. To achieve this, also move the info >structure into the common header, and document it. > >While at it, drop a NULL check in rkisp1_probe() for the match data as >it can't happen. > >Signed-off-by: Paul Elder <paul.elder@ideasonboard.com> >Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> >--- > .../platform/rockchip/rkisp1/rkisp1-common.h | 22 +++++++++++++++++++ > .../platform/rockchip/rkisp1/rkisp1-dev.c | 15 +++---------- > 2 files changed, 25 insertions(+), 12 deletions(-) > >diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h b/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h >index a67fe7b1dfa1..50d31a254b03 100644 >--- a/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h >+++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h >@@ -91,6 +91,26 @@ enum rkisp1_isp_pad { > RKISP1_ISP_PAD_MAX > }; > >+/* >+ * struct rkisp1_info - Model-specific ISP Information >+ * >+ * @clks: array of ISP clock names >+ * @clk_size: number of entries in the @clks array >+ * @isrs: array of ISP interrupt descriptors >+ * @isr_size: number of entires in the @isrs array >+ * @isp_ver: ISP version >+ * >+ * This structure contains information about the ISP specific to a particular >+ * ISP model, version, or integration in a particular SoC. >+ */ >+struct rkisp1_info { >+ const char * const *clks; >+ unsigned int clk_size; >+ const struct rkisp1_isr_data *isrs; >+ unsigned int isr_size; >+ enum rkisp1_cif_isp_version isp_ver; >+}; >+ > /* > * struct rkisp1_sensor_async - A container for the v4l2_async_subdev to add to the notifier > * of the v4l2-async API >@@ -395,6 +415,7 @@ struct rkisp1_debug { > * @pipe: media pipeline > * @stream_lock: serializes {start/stop}_streaming callbacks between the capture devices. > * @debug: debug params to be exposed on debugfs >+ * @info: version-specific ISP information > */ > struct rkisp1_device { > void __iomem *base_addr; >@@ -413,6 +434,7 @@ struct rkisp1_device { > struct media_pipeline pipe; > struct mutex stream_lock; /* serialize {start/stop}_streaming cb between capture devices */ > struct rkisp1_debug debug; >+ const struct rkisp1_info *info; > }; > > /* >diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c b/drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c >index 258980ef4783..39ae35074062 100644 >--- a/drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c >+++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c >@@ -105,14 +105,6 @@ struct rkisp1_isr_data { > irqreturn_t (*isr)(int irq, void *ctx); > }; > >-struct rkisp1_info { >- const char * const *clks; >- unsigned int clk_size; >- const struct rkisp1_isr_data *isrs; >- unsigned int isr_size; >- enum rkisp1_cif_isp_version isp_ver; >-}; >- > /* ---------------------------------------------------------------------------- > * Sensor DT bindings > */ >@@ -469,14 +461,13 @@ static int rkisp1_probe(struct platform_device *pdev) > int ret, irq; > u32 cif_id; > >- info = of_device_get_match_data(&pdev->dev); >- if (!info) >- return -ENODEV; >- > rkisp1 = devm_kzalloc(dev, sizeof(*rkisp1), GFP_KERNEL); > if (!rkisp1) > return -ENOMEM; > >+ info = of_device_get_match_data(dev); Why did you omit the check 'if(!info)'? thanks, Dafna >+ rkisp1->info = info; >+ > dev_set_drvdata(dev, rkisp1); > rkisp1->dev = dev; > >-- >2.30.2 >
Hi Dafna, On Fri, Jun 24, 2022 at 05:34:00PM +0300, Dafna Hirschfeld wrote: > On 15.06.2022 04:10, Paul Elder wrote: > > To make it possible to use the rkisp1_info after probe time (for > > instance to make code conditional on the ISP version), save it in the > > main rkisp1_device structure. To achieve this, also move the info > > structure into the common header, and document it. > > > > While at it, drop a NULL check in rkisp1_probe() for the match data as > > it can't happen. > > > > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com> > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > --- > > .../platform/rockchip/rkisp1/rkisp1-common.h | 22 +++++++++++++++++++ > > .../platform/rockchip/rkisp1/rkisp1-dev.c | 15 +++---------- > > 2 files changed, 25 insertions(+), 12 deletions(-) > > > > diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h b/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h > > index a67fe7b1dfa1..50d31a254b03 100644 > > --- a/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h > > +++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h > > @@ -91,6 +91,26 @@ enum rkisp1_isp_pad { > > RKISP1_ISP_PAD_MAX > > }; > > > > +/* > > + * struct rkisp1_info - Model-specific ISP Information > > + * > > + * @clks: array of ISP clock names > > + * @clk_size: number of entries in the @clks array > > + * @isrs: array of ISP interrupt descriptors > > + * @isr_size: number of entires in the @isrs array > > + * @isp_ver: ISP version > > + * > > + * This structure contains information about the ISP specific to a particular > > + * ISP model, version, or integration in a particular SoC. > > + */ > > +struct rkisp1_info { > > + const char * const *clks; > > + unsigned int clk_size; > > + const struct rkisp1_isr_data *isrs; > > + unsigned int isr_size; > > + enum rkisp1_cif_isp_version isp_ver; > > +}; > > + > > /* > > * struct rkisp1_sensor_async - A container for the v4l2_async_subdev to add to the notifier > > * of the v4l2-async API > > @@ -395,6 +415,7 @@ struct rkisp1_debug { > > * @pipe: media pipeline > > * @stream_lock: serializes {start/stop}_streaming callbacks between the capture devices. > > * @debug: debug params to be exposed on debugfs > > + * @info: version-specific ISP information > > */ > > struct rkisp1_device { > > void __iomem *base_addr; > > @@ -413,6 +434,7 @@ struct rkisp1_device { > > struct media_pipeline pipe; > > struct mutex stream_lock; /* serialize {start/stop}_streaming cb between capture devices */ > > struct rkisp1_debug debug; > > + const struct rkisp1_info *info; > > }; > > > > /* > > diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c b/drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c > > index 258980ef4783..39ae35074062 100644 > > --- a/drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c > > +++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c > > @@ -105,14 +105,6 @@ struct rkisp1_isr_data { > > irqreturn_t (*isr)(int irq, void *ctx); > > }; > > > > -struct rkisp1_info { > > - const char * const *clks; > > - unsigned int clk_size; > > - const struct rkisp1_isr_data *isrs; > > - unsigned int isr_size; > > - enum rkisp1_cif_isp_version isp_ver; > > -}; > > - > > /* ---------------------------------------------------------------------------- > > * Sensor DT bindings > > */ > > @@ -469,14 +461,13 @@ static int rkisp1_probe(struct platform_device *pdev) > > int ret, irq; > > u32 cif_id; > > > > - info = of_device_get_match_data(&pdev->dev); > > - if (!info) > > - return -ENODEV; > > - > > rkisp1 = devm_kzalloc(dev, sizeof(*rkisp1), GFP_KERNEL); > > if (!rkisp1) > > return -ENOMEM; > > > > + info = of_device_get_match_data(dev); > > Why did you omit the check 'if(!info)'? Because it can't happen. The probe() function is only called if the platform device matches one of the of_device_id, so of_device_get_match_data() can't return NULL. > > + rkisp1->info = info; > > + > > dev_set_drvdata(dev, rkisp1); > > rkisp1->dev = dev; > >
On 24.06.2022 17:47, Laurent Pinchart wrote: >Hi Dafna, > >On Fri, Jun 24, 2022 at 05:34:00PM +0300, Dafna Hirschfeld wrote: >> On 15.06.2022 04:10, Paul Elder wrote: >> > To make it possible to use the rkisp1_info after probe time (for >> > instance to make code conditional on the ISP version), save it in the >> > main rkisp1_device structure. To achieve this, also move the info >> > structure into the common header, and document it. >> > >> > While at it, drop a NULL check in rkisp1_probe() for the match data as >> > it can't happen. >> > >> > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com> >> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> >> > --- >> > .../platform/rockchip/rkisp1/rkisp1-common.h | 22 +++++++++++++++++++ >> > .../platform/rockchip/rkisp1/rkisp1-dev.c | 15 +++---------- >> > 2 files changed, 25 insertions(+), 12 deletions(-) >> > >> > diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h b/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h >> > index a67fe7b1dfa1..50d31a254b03 100644 >> > --- a/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h >> > +++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h >> > @@ -91,6 +91,26 @@ enum rkisp1_isp_pad { >> > RKISP1_ISP_PAD_MAX >> > }; >> > >> > +/* >> > + * struct rkisp1_info - Model-specific ISP Information >> > + * >> > + * @clks: array of ISP clock names >> > + * @clk_size: number of entries in the @clks array >> > + * @isrs: array of ISP interrupt descriptors >> > + * @isr_size: number of entires in the @isrs array >> > + * @isp_ver: ISP version >> > + * >> > + * This structure contains information about the ISP specific to a particular >> > + * ISP model, version, or integration in a particular SoC. >> > + */ >> > +struct rkisp1_info { >> > + const char * const *clks; >> > + unsigned int clk_size; >> > + const struct rkisp1_isr_data *isrs; >> > + unsigned int isr_size; >> > + enum rkisp1_cif_isp_version isp_ver; >> > +}; >> > + >> > /* >> > * struct rkisp1_sensor_async - A container for the v4l2_async_subdev to add to the notifier >> > * of the v4l2-async API >> > @@ -395,6 +415,7 @@ struct rkisp1_debug { >> > * @pipe: media pipeline >> > * @stream_lock: serializes {start/stop}_streaming callbacks between the capture devices. >> > * @debug: debug params to be exposed on debugfs >> > + * @info: version-specific ISP information >> > */ >> > struct rkisp1_device { >> > void __iomem *base_addr; >> > @@ -413,6 +434,7 @@ struct rkisp1_device { >> > struct media_pipeline pipe; >> > struct mutex stream_lock; /* serialize {start/stop}_streaming cb between capture devices */ >> > struct rkisp1_debug debug; >> > + const struct rkisp1_info *info; >> > }; >> > >> > /* >> > diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c b/drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c >> > index 258980ef4783..39ae35074062 100644 >> > --- a/drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c >> > +++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c >> > @@ -105,14 +105,6 @@ struct rkisp1_isr_data { >> > irqreturn_t (*isr)(int irq, void *ctx); >> > }; >> > >> > -struct rkisp1_info { >> > - const char * const *clks; >> > - unsigned int clk_size; >> > - const struct rkisp1_isr_data *isrs; >> > - unsigned int isr_size; >> > - enum rkisp1_cif_isp_version isp_ver; >> > -}; >> > - >> > /* ---------------------------------------------------------------------------- >> > * Sensor DT bindings >> > */ >> > @@ -469,14 +461,13 @@ static int rkisp1_probe(struct platform_device *pdev) >> > int ret, irq; >> > u32 cif_id; >> > >> > - info = of_device_get_match_data(&pdev->dev); >> > - if (!info) >> > - return -ENODEV; >> > - >> > rkisp1 = devm_kzalloc(dev, sizeof(*rkisp1), GFP_KERNEL); >> > if (!rkisp1) >> > return -ENOMEM; >> > >> > + info = of_device_get_match_data(dev); >> >> Why did you omit the check 'if(!info)'? > >Because it can't happen. The probe() function is only called if the >platform device matches one of the of_device_id, so >of_device_get_match_data() can't return NULL. > I see now it is also written in the commit log, Reviewed-by: Dafna Hirschfeld <dafna@fastmail.com> >> > + rkisp1->info = info; >> > + >> > dev_set_drvdata(dev, rkisp1); >> > rkisp1->dev = dev; >> > > >-- >Regards, > >Laurent Pinchart > >_______________________________________________ >Linux-rockchip mailing list >Linux-rockchip@lists.infradead.org >http://lists.infradead.org/mailman/listinfo/linux-rockchip
diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h b/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h index a67fe7b1dfa1..50d31a254b03 100644 --- a/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h +++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h @@ -91,6 +91,26 @@ enum rkisp1_isp_pad { RKISP1_ISP_PAD_MAX }; +/* + * struct rkisp1_info - Model-specific ISP Information + * + * @clks: array of ISP clock names + * @clk_size: number of entries in the @clks array + * @isrs: array of ISP interrupt descriptors + * @isr_size: number of entires in the @isrs array + * @isp_ver: ISP version + * + * This structure contains information about the ISP specific to a particular + * ISP model, version, or integration in a particular SoC. + */ +struct rkisp1_info { + const char * const *clks; + unsigned int clk_size; + const struct rkisp1_isr_data *isrs; + unsigned int isr_size; + enum rkisp1_cif_isp_version isp_ver; +}; + /* * struct rkisp1_sensor_async - A container for the v4l2_async_subdev to add to the notifier * of the v4l2-async API @@ -395,6 +415,7 @@ struct rkisp1_debug { * @pipe: media pipeline * @stream_lock: serializes {start/stop}_streaming callbacks between the capture devices. * @debug: debug params to be exposed on debugfs + * @info: version-specific ISP information */ struct rkisp1_device { void __iomem *base_addr; @@ -413,6 +434,7 @@ struct rkisp1_device { struct media_pipeline pipe; struct mutex stream_lock; /* serialize {start/stop}_streaming cb between capture devices */ struct rkisp1_debug debug; + const struct rkisp1_info *info; }; /* diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c b/drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c index 258980ef4783..39ae35074062 100644 --- a/drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c +++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c @@ -105,14 +105,6 @@ struct rkisp1_isr_data { irqreturn_t (*isr)(int irq, void *ctx); }; -struct rkisp1_info { - const char * const *clks; - unsigned int clk_size; - const struct rkisp1_isr_data *isrs; - unsigned int isr_size; - enum rkisp1_cif_isp_version isp_ver; -}; - /* ---------------------------------------------------------------------------- * Sensor DT bindings */ @@ -469,14 +461,13 @@ static int rkisp1_probe(struct platform_device *pdev) int ret, irq; u32 cif_id; - info = of_device_get_match_data(&pdev->dev); - if (!info) - return -ENODEV; - rkisp1 = devm_kzalloc(dev, sizeof(*rkisp1), GFP_KERNEL); if (!rkisp1) return -ENOMEM; + info = of_device_get_match_data(dev); + rkisp1->info = info; + dev_set_drvdata(dev, rkisp1); rkisp1->dev = dev;