| Message ID | f468c4ac-0629-41b5-b5d1-e26f70e44800@moroto.mountain (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] media: ipu-bridge: fix error code in ipu_bridge_init() | expand |
On Fri, May 10, 2024 at 06:43:31PM +0300, Dan Carpenter wrote:
> Return -EINVAL if "bridge->n_sensors == 0". Don't return success.
LGTM, but I leave the main Q "Is it really the error case?" to the maintainers.
I would imagine the use case where either from the following may happen:
1) the sensors are all new and not listed as supported;
2) there no sensors connected for real.
In both cases I don't see this as a critical error that we can't enumerate
the bridge itself.
Hello On 10/05/2024 16:55, Andy Shevchenko wrote: > On Fri, May 10, 2024 at 06:43:31PM +0300, Dan Carpenter wrote: >> Return -EINVAL if "bridge->n_sensors == 0". Don't return success. > LGTM, but I leave the main Q "Is it really the error case?" to the maintainers. > I would imagine the use case where either from the following may happen: > 1) the sensors are all new and not listed as supported; > 2) there no sensors connected for real. > > In both cases I don't see this as a critical error that we can't enumerate > the bridge itself. > I have no strong feelings on this really. The CIO2 driver, before the bridge was a thing, didn't treat a lack of connected endpoints as an error case and still completed probe if the cio2_parse_firmware() function doesn't find any connected endpoints...but perhaps it should have behaved this way all along. Is there value in having the cio2 device probed, but useless? I can't think of any at the moment. The patch contents themselves look good to me.
On Tue, May 14, 2024 at 11:14:18AM +0100, Dan Scally wrote: > Hello > > On 10/05/2024 16:55, Andy Shevchenko wrote: > > On Fri, May 10, 2024 at 06:43:31PM +0300, Dan Carpenter wrote: > > > Return -EINVAL if "bridge->n_sensors == 0". Don't return success. > > LGTM, but I leave the main Q "Is it really the error case?" to the maintainers. > > I would imagine the use case where either from the following may happen: > > 1) the sensors are all new and not listed as supported; > > 2) there no sensors connected for real. > > > > In both cases I don't see this as a critical error that we can't enumerate > > the bridge itself. > > > I have no strong feelings on this really. The CIO2 driver, before the bridge > was a thing, didn't treat a lack of connected endpoints as an error case and > still completed probe if the cio2_parse_firmware() function doesn't find any > connected endpoints...but perhaps it should have behaved this way all along. > Is there value in having the cio2 device probed, but useless? I can't think > of any at the moment. > > > The patch contents themselves look good to me. Let's just leave it as-is if everyone is happy with the current behavior. When someone complains, we'll fix it. regards, dan carpenter
Hi Dan, On Fri, May 10, 2024 at 06:43:31PM +0300, Dan Carpenter wrote: > Return -EINVAL if "bridge->n_sensors == 0". Don't return success. > > Fixes: 881ca25978c6 ("media: ipu3-cio2: rename cio2 bridge to ipu bridge and move out of ipu3") > Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org> > --- > v2: style change > > drivers/media/pci/intel/ipu-bridge.c | 7 ++++++- > 1 file changed, 6 insertions(+), 1 deletion(-) > > diff --git a/drivers/media/pci/intel/ipu-bridge.c b/drivers/media/pci/intel/ipu-bridge.c > index 61750cc98d70..44a9d9c15b05 100644 > --- a/drivers/media/pci/intel/ipu-bridge.c > +++ b/drivers/media/pci/intel/ipu-bridge.c > @@ -839,9 +839,14 @@ int ipu_bridge_init(struct device *dev, > bridge->data_lanes[i] = i + 1; > > ret = ipu_bridge_connect_sensors(bridge); > - if (ret || bridge->n_sensors == 0) > + if (ret) > goto err_unregister_ipu; > > + if (bridge->n_sensors == 0) { > + ret = -EINVAL; > + goto err_unregister_ipu; > + } This would return an error if there are no sensors. Neither IPU3-CIO2 or IPU6 ISYS drivers should be of any functional use without sensors. But the power states of the devices could be affected by this: the drivers should power off these devices but without drivers they maybe left powered on. I haven't made any measurements though. > + > dev_info(dev, "Connected %d cameras\n", bridge->n_sensors); > > fwnode = software_node_fwnode(&bridge->ipu_hid_node);
On Tue, May 14, 2024 at 02:33:31PM +0000, Sakari Ailus wrote: > On Fri, May 10, 2024 at 06:43:31PM +0300, Dan Carpenter wrote: ... > Neither IPU3-CIO2 or IPU6 ISYS drivers should be of any functional use > without sensors. But the power states of the devices could be affected by > this: the drivers should power off these devices but without drivers they > maybe left powered on. I haven't made any measurements though. FWIW, Hans mentioned AtomISPv2 case with somewhat 7W consumption on top of the idling machine. That's why we have a stub driver in PDx86 exactly for the purpose of turning it off when not used. Hans may correct me if I'm wrong in numbers or elsewhere.
Hi, On 5/14/24 5:21 PM, Andy Shevchenko wrote: > On Tue, May 14, 2024 at 02:33:31PM +0000, Sakari Ailus wrote: >> On Fri, May 10, 2024 at 06:43:31PM +0300, Dan Carpenter wrote: > > ... > >> Neither IPU3-CIO2 or IPU6 ISYS drivers should be of any functional use >> without sensors. But the power states of the devices could be affected by >> this: the drivers should power off these devices but without drivers they >> maybe left powered on. I haven't made any measurements though. > > FWIW, Hans mentioned AtomISPv2 case with somewhat 7W consumption on top of > the idling machine. That's why we have a stub driver in PDx86 exactly for > the purpose of turning it off when not used. I'm not sure if I ever mentioned the 7W, that seems a lot. But in the atomisp case the SoC will never reach S0i3 when the ISP is not properly turned off. And in this case the ISP is special and just letting PCI / ACPI put it in D3 is not enough it needs some special writes on the IO-Sideband-Fabric to be turned off. I don't know if something similar applies to the IPU3 / IPU6, but the bridge code is used by the atomisp code now too. So at a minimum if an error gets returned when there are no sensors then this must be unique enough that the atomisp code can check for it. Maybe -ENODEV ? Regards, Hans
diff --git a/drivers/media/pci/intel/ipu-bridge.c b/drivers/media/pci/intel/ipu-bridge.c index 61750cc98d70..44a9d9c15b05 100644 --- a/drivers/media/pci/intel/ipu-bridge.c +++ b/drivers/media/pci/intel/ipu-bridge.c @@ -839,9 +839,14 @@ int ipu_bridge_init(struct device *dev, bridge->data_lanes[i] = i + 1; ret = ipu_bridge_connect_sensors(bridge); - if (ret || bridge->n_sensors == 0) + if (ret) goto err_unregister_ipu; + if (bridge->n_sensors == 0) { + ret = -EINVAL; + goto err_unregister_ipu; + } + dev_info(dev, "Connected %d cameras\n", bridge->n_sensors); fwnode = software_node_fwnode(&bridge->ipu_hid_node);
Return -EINVAL if "bridge->n_sensors == 0". Don't return success. Fixes: 881ca25978c6 ("media: ipu3-cio2: rename cio2 bridge to ipu bridge and move out of ipu3") Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org> --- v2: style change drivers/media/pci/intel/ipu-bridge.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)