| Message ID | 20250513142353.2572563-1-vladimir.zapolskiy@linaro.org |
|---|---|
| Headers | show |
| Series | media: qcom: camss: a number of cleanups and fixes | expand |
On 13/05/2025 15:23, Vladimir Zapolskiy wrote: > Add myself as a review of Qualcomm CAMSS subsystem patches. > > Signed-off-by: Vladimir Zapolskiy <vladimir.zapolskiy@linaro.org> > --- > MAINTAINERS | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/MAINTAINERS b/MAINTAINERS > index 6dbdf02d6b0c..9b973c0128fa 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -20135,6 +20135,7 @@ QUALCOMM CAMERA SUBSYSTEM DRIVER > M: Robert Foss <rfoss@kernel.org> > M: Todor Tomov <todor.too@gmail.com> > M: Bryan O'Donoghue <bryan.odonoghue@linaro.org> > +R: Vladimir Zapolskiy <vladimir.zapolskiy@linaro.org> > L: linux-media@vger.kernel.org > S: Maintained > F: Documentation/admin-guide/media/qcom_camss.rst Acked-by: Bryan O'Donoghue <bod@kernel.org>
On 13/05/2025 15:23, Vladimir Zapolskiy wrote: > For sake of simplicity it makes sense to register async notifier > for all type of subdevices, both CAMSS components and sensors. > > The case of sensors not connected to CAMSS is extraordinary and > degenerate, it does not deserve any specific optimization. > > Signed-off-by: Vladimir Zapolskiy <vladimir.zapolskiy@linaro.org> > --- > drivers/media/platform/qcom/camss/camss.c | 30 ++++++----------------- > 1 file changed, 8 insertions(+), 22 deletions(-) > > diff --git a/drivers/media/platform/qcom/camss/camss.c b/drivers/media/platform/qcom/camss/camss.c > index 976b70cc6d6a..4e91e4b6ef52 100644 > --- a/drivers/media/platform/qcom/camss/camss.c > +++ b/drivers/media/platform/qcom/camss/camss.c > @@ -3556,7 +3556,6 @@ static int camss_probe(struct platform_device *pdev) > { > struct device *dev = &pdev->dev; > struct camss *camss; > - int num_subdevs; > int ret; > > camss = devm_kzalloc(dev, sizeof(*camss), GFP_KERNEL); > @@ -3627,11 +3626,9 @@ static int camss_probe(struct platform_device *pdev) > > pm_runtime_enable(dev); > > - num_subdevs = camss_of_parse_ports(camss); > - if (num_subdevs < 0) { > - ret = num_subdevs; > + ret = camss_of_parse_ports(camss); > + if (ret < 0) > goto err_v4l2_device_unregister; > - } > > ret = camss_register_entities(camss); > if (ret < 0) > @@ -3647,23 +3644,12 @@ static int camss_probe(struct platform_device *pdev) > goto err_register_subdevs; > } > > - if (num_subdevs) { > - camss->notifier.ops = &camss_subdev_notifier_ops; > - > - ret = v4l2_async_nf_register(&camss->notifier); > - if (ret) { > - dev_err(dev, > - "Failed to register async subdev nodes: %d\n", > - ret); > - goto err_media_device_unregister; > - } > - } else { > - ret = v4l2_device_register_subdev_nodes(&camss->v4l2_dev); > - if (ret < 0) { > - dev_err(dev, "Failed to register subdev nodes: %d\n", > - ret); > - goto err_media_device_unregister; > - } > + camss->notifier.ops = &camss_subdev_notifier_ops; > + ret = v4l2_async_nf_register(&camss->notifier); > + if (ret) { > + dev_err(dev, > + "Failed to register async subdev nodes: %d\n", ret); > + goto err_media_device_unregister; > } > > return 0; If I've understood the intent here, don't think this is right. For cases where we want to run CSID TPG or standalone TPG we would not necessarily have a sensor connected. --- bod
Hi Bryan. On 5/13/25 18:46, Bryan O'Donoghue wrote: > On 13/05/2025 15:23, Vladimir Zapolskiy wrote: >> For sake of simplicity it makes sense to register async notifier >> for all type of subdevices, both CAMSS components and sensors. >> >> The case of sensors not connected to CAMSS is extraordinary and >> degenerate, it does not deserve any specific optimization. >> >> Signed-off-by: Vladimir Zapolskiy <vladimir.zapolskiy@linaro.org> >> --- >> drivers/media/platform/qcom/camss/camss.c | 30 ++++++----------------- >> 1 file changed, 8 insertions(+), 22 deletions(-) >> >> diff --git a/drivers/media/platform/qcom/camss/camss.c b/drivers/media/platform/qcom/camss/camss.c >> index 976b70cc6d6a..4e91e4b6ef52 100644 >> --- a/drivers/media/platform/qcom/camss/camss.c >> +++ b/drivers/media/platform/qcom/camss/camss.c >> @@ -3556,7 +3556,6 @@ static int camss_probe(struct platform_device *pdev) >> { >> struct device *dev = &pdev->dev; >> struct camss *camss; >> - int num_subdevs; >> int ret; >> >> camss = devm_kzalloc(dev, sizeof(*camss), GFP_KERNEL); >> @@ -3627,11 +3626,9 @@ static int camss_probe(struct platform_device *pdev) >> >> pm_runtime_enable(dev); >> >> - num_subdevs = camss_of_parse_ports(camss); >> - if (num_subdevs < 0) { >> - ret = num_subdevs; >> + ret = camss_of_parse_ports(camss); >> + if (ret < 0) >> goto err_v4l2_device_unregister; >> - } >> >> ret = camss_register_entities(camss); >> if (ret < 0) >> @@ -3647,23 +3644,12 @@ static int camss_probe(struct platform_device *pdev) >> goto err_register_subdevs; >> } >> >> - if (num_subdevs) { >> - camss->notifier.ops = &camss_subdev_notifier_ops; >> - >> - ret = v4l2_async_nf_register(&camss->notifier); >> - if (ret) { >> - dev_err(dev, >> - "Failed to register async subdev nodes: %d\n", >> - ret); >> - goto err_media_device_unregister; >> - } >> - } else { >> - ret = v4l2_device_register_subdev_nodes(&camss->v4l2_dev); >> - if (ret < 0) { >> - dev_err(dev, "Failed to register subdev nodes: %d\n", >> - ret); >> - goto err_media_device_unregister; >> - } >> + camss->notifier.ops = &camss_subdev_notifier_ops; >> + ret = v4l2_async_nf_register(&camss->notifier); >> + if (ret) { >> + dev_err(dev, >> + "Failed to register async subdev nodes: %d\n", ret); >> + goto err_media_device_unregister; >> } >> >> return 0; > > If I've understood the intent here, don't think this is right. Please elaborate, so far it's not completely clear. > For cases where we want to run CSID TPG or standalone TPG we would not > necessarily have a sensor connected. > That's correct, and this is not broken by any means. As you mention it there might be so many usecases, but fortunately all of them are covered by the code, which is noticeably simpler than the original one, and which opens the path for even further code optimization and simplification, as you find it in this changeset. -- Best wishes, Vladimir
On Tue, May 13, 2025 at 5:47 PM Bryan O'Donoghue <bryan.odonoghue@linaro.org> wrote: > > On 13/05/2025 15:23, Vladimir Zapolskiy wrote: > > For sake of simplicity it makes sense to register async notifier > > for all type of subdevices, both CAMSS components and sensors. > > > > The case of sensors not connected to CAMSS is extraordinary and > > degenerate, it does not deserve any specific optimization. > > > > Signed-off-by: Vladimir Zapolskiy <vladimir.zapolskiy@linaro.org> > > --- > > drivers/media/platform/qcom/camss/camss.c | 30 ++++++----------------- > > 1 file changed, 8 insertions(+), 22 deletions(-) > > > > diff --git a/drivers/media/platform/qcom/camss/camss.c b/drivers/media/platform/qcom/camss/camss.c > > index 976b70cc6d6a..4e91e4b6ef52 100644 > > --- a/drivers/media/platform/qcom/camss/camss.c > > +++ b/drivers/media/platform/qcom/camss/camss.c > > @@ -3556,7 +3556,6 @@ static int camss_probe(struct platform_device *pdev) > > { > > struct device *dev = &pdev->dev; > > struct camss *camss; > > - int num_subdevs; > > int ret; > > > > camss = devm_kzalloc(dev, sizeof(*camss), GFP_KERNEL); > > @@ -3627,11 +3626,9 @@ static int camss_probe(struct platform_device *pdev) > > > > pm_runtime_enable(dev); > > > > - num_subdevs = camss_of_parse_ports(camss); > > - if (num_subdevs < 0) { > > - ret = num_subdevs; > > + ret = camss_of_parse_ports(camss); > > + if (ret < 0) > > goto err_v4l2_device_unregister; > > - } > > > > ret = camss_register_entities(camss); > > if (ret < 0) > > @@ -3647,23 +3644,12 @@ static int camss_probe(struct platform_device *pdev) > > goto err_register_subdevs; > > } > > > > - if (num_subdevs) { > > - camss->notifier.ops = &camss_subdev_notifier_ops; > > - > > - ret = v4l2_async_nf_register(&camss->notifier); > > - if (ret) { > > - dev_err(dev, > > - "Failed to register async subdev nodes: %d\n", > > - ret); > > - goto err_media_device_unregister; > > - } > > - } else { > > - ret = v4l2_device_register_subdev_nodes(&camss->v4l2_dev); > > - if (ret < 0) { > > - dev_err(dev, "Failed to register subdev nodes: %d\n", > > - ret); > > - goto err_media_device_unregister; > > - } > > + camss->notifier.ops = &camss_subdev_notifier_ops; > > + ret = v4l2_async_nf_register(&camss->notifier); > > + if (ret) { > > + dev_err(dev, > > + "Failed to register async subdev nodes: %d\n", ret); > > + goto err_media_device_unregister; > > } > > > > return 0; > > If I've understood the intent here, don't think this is right. > > For cases where we want to run CSID TPG or standalone TPG we would not > necessarily have a sensor connected. I understand it will work because Vladimir moved the media device registering earlier in the probe, so the media pipeline will be ready, even if no subdev sensor has been registered. Regards, Loic