Re: [PATCH 07/55] media: rkisp1: Save info pointer in rkisp1_device

[Dafna Hirschfeld <dafna@fastmail.com>]

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

_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip