Re: [PATCH v2 04/17] smiapp: Split off sub-device registration into two
Hi,
On Mon, Sep 19, 2016 at 11:50:02PM +0300, Sakari Ailus wrote:
> Hi Sebastian,
>
> Sebastian Reichel wrote:
> > Hi,
> >
> > On Thu, Sep 15, 2016 at 02:22:18PM +0300, Sakari Ailus wrote:
> > > Remove the loop in sub-device registration and create each sub-device
> > > explicitly instead.
> >
> > Reviewed-By: Sebastian Reichel
>
> Thanks!
>
> >
> > > +static int smiapp_register_subdevs(struct smiapp_sensor *sensor)
> > > +{
> > > + int rval;
> > > +
> > > + if (sensor->scaler) {
> > > + rval = smiapp_register_subdev(
> > > + sensor, sensor->binner, sensor->scaler,
> > > + SMIAPP_PAD_SRC, SMIAPP_PAD_SINK,
> > > + MEDIA_LNK_FL_ENABLED | MEDIA_LNK_FL_IMMUTABLE);
> > > + if (rval < 0)
> > > return rval;
> > > - }
> > > }
> > >
> > > - return 0;
> > > + return smiapp_register_subdev(
> > > + sensor, sensor->pixel_array, sensor->binner,
> > > + SMIAPP_PA_PAD_SRC, SMIAPP_PAD_SINK,
> > > + MEDIA_LNK_FL_ENABLED | MEDIA_LNK_FL_IMMUTABLE);
> > > }
> >
> > I haven't looked at the remaining code, but is sensor->scaler
> > stuff being cleaned up properly if the binner part fails?
>
> That's a very good question. I don't think it is. But that's how
> the code has always been
Yes, it's not a regression introduced by this patch, that's why I
gave Reviewed-By ;)
> --- there are issues left to be resolved if registered() fails for
> a reason or another. For instance, removing and reloading the
> omap3-isp module will cause a failure in the smiapp driver unless
> it's unloaded as well.
>
> I think I prefer to fix that in a different patch(set) as this one
> is quite large already.
ok.
-- Sebastian
signature.asc
Description: PGP signature
Re: [PATCH v2 04/17] smiapp: Split off sub-device registration into two
Hi Sebastian,
Sebastian Reichel wrote:
Hi,
On Thu, Sep 15, 2016 at 02:22:18PM +0300, Sakari Ailus wrote:
Remove the loop in sub-device registration and create each sub-device
explicitly instead.
Reviewed-By: Sebastian Reichel
Thanks!
+static int smiapp_register_subdevs(struct smiapp_sensor *sensor)
+{
+ int rval;
+
+ if (sensor->scaler) {
+ rval = smiapp_register_subdev(
+ sensor, sensor->binner, sensor->scaler,
+ SMIAPP_PAD_SRC, SMIAPP_PAD_SINK,
+ MEDIA_LNK_FL_ENABLED | MEDIA_LNK_FL_IMMUTABLE);
+ if (rval < 0)
return rval;
- }
}
- return 0;
+ return smiapp_register_subdev(
+ sensor, sensor->pixel_array, sensor->binner,
+ SMIAPP_PA_PAD_SRC, SMIAPP_PAD_SINK,
+ MEDIA_LNK_FL_ENABLED | MEDIA_LNK_FL_IMMUTABLE);
}
I haven't looked at the remaining code, but is sensor->scaler
stuff being cleaned up properly if the binner part fails?
That's a very good question. I don't think it is. But that's how the
code has always been --- there are issues left to be resolved if
registered() fails for a reason or another. For instance, removing and
reloading the omap3-isp module will cause a failure in the smiapp driver
unless it's unloaded as well.
I think I prefer to fix that in a different patch(set) as this one is
quite large already.
--
Kind regards,
Sakari Ailus
sakari.ai...@linux.intel.com
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 04/17] smiapp: Split off sub-device registration into two
Hi,
On Thu, Sep 15, 2016 at 02:22:18PM +0300, Sakari Ailus wrote:
> Remove the loop in sub-device registration and create each sub-device
> explicitly instead.
Reviewed-By: Sebastian Reichel
> +static int smiapp_register_subdevs(struct smiapp_sensor *sensor)
> +{
> + int rval;
> +
> + if (sensor->scaler) {
> + rval = smiapp_register_subdev(
> + sensor, sensor->binner, sensor->scaler,
> + SMIAPP_PAD_SRC, SMIAPP_PAD_SINK,
> + MEDIA_LNK_FL_ENABLED | MEDIA_LNK_FL_IMMUTABLE);
> + if (rval < 0)
> return rval;
> - }
> }
>
> - return 0;
> + return smiapp_register_subdev(
> + sensor, sensor->pixel_array, sensor->binner,
> + SMIAPP_PA_PAD_SRC, SMIAPP_PAD_SINK,
> + MEDIA_LNK_FL_ENABLED | MEDIA_LNK_FL_IMMUTABLE);
> }
I haven't looked at the remaining code, but is sensor->scaler
stuff being cleaned up properly if the binner part fails?
-- Sebastian
signature.asc
Description: PGP signature
[PATCH v2 04/17] smiapp: Split off sub-device registration into two
Remove the loop in sub-device registration and create each sub-device
explicitly instead.
Signed-off-by: Sakari Ailus
---
drivers/media/i2c/smiapp/smiapp-core.c | 82 +++---
1 file changed, 45 insertions(+), 37 deletions(-)
diff --git a/drivers/media/i2c/smiapp/smiapp-core.c
b/drivers/media/i2c/smiapp/smiapp-core.c
index 0a03f30..9022ffc 100644
--- a/drivers/media/i2c/smiapp/smiapp-core.c
+++ b/drivers/media/i2c/smiapp/smiapp-core.c
@@ -2475,54 +2475,62 @@ static const struct v4l2_subdev_ops smiapp_ops;
static const struct v4l2_subdev_internal_ops smiapp_internal_ops;
static const struct media_entity_operations smiapp_entity_ops;
-static int smiapp_register_subdevs(struct smiapp_sensor *sensor)
+static int smiapp_register_subdev(struct smiapp_sensor *sensor,
+ struct smiapp_subdev *ssd,
+ struct smiapp_subdev *sink_ssd,
+ u16 source_pad, u16 sink_pad, u32 link_flags)
{
struct i2c_client *client = v4l2_get_subdevdata(&sensor->src->sd);
- struct smiapp_subdev *ssds[] = {
- sensor->scaler,
- sensor->binner,
- sensor->pixel_array,
- };
- unsigned int i;
int rval;
- for (i = 0; i < SMIAPP_SUBDEVS - 1; i++) {
- struct smiapp_subdev *this = ssds[i + 1];
- struct smiapp_subdev *last = ssds[i];
+ if (!sink_ssd)
+ return 0;
- if (!last)
- continue;
+ rval = media_entity_pads_init(&ssd->sd.entity,
+ ssd->npads, ssd->pads);
+ if (rval) {
+ dev_err(&client->dev,
+ "media_entity_pads_init failed\n");
+ return rval;
+ }
- rval = media_entity_pads_init(&this->sd.entity,
-this->npads, this->pads);
- if (rval) {
- dev_err(&client->dev,
- "media_entity_pads_init failed\n");
- return rval;
- }
+ rval = v4l2_device_register_subdev(sensor->src->sd.v4l2_dev,
+ &ssd->sd);
+ if (rval) {
+ dev_err(&client->dev,
+ "v4l2_device_register_subdev failed\n");
+ return rval;
+ }
- rval = v4l2_device_register_subdev(sensor->src->sd.v4l2_dev,
- &this->sd);
- if (rval) {
- dev_err(&client->dev,
- "v4l2_device_register_subdev failed\n");
- return rval;
- }
+ rval = media_create_pad_link(&ssd->sd.entity, source_pad,
+&sink_ssd->sd.entity, sink_pad,
+link_flags);
+ if (rval) {
+ dev_err(&client->dev,
+ "media_create_pad_link failed\n");
+ return rval;
+ }
- rval = media_create_pad_link(&this->sd.entity,
-this->source_pad,
-&last->sd.entity,
-last->sink_pad,
-MEDIA_LNK_FL_ENABLED |
-MEDIA_LNK_FL_IMMUTABLE);
- if (rval) {
- dev_err(&client->dev,
- "media_create_pad_link failed\n");
+ return 0;
+}
+
+static int smiapp_register_subdevs(struct smiapp_sensor *sensor)
+{
+ int rval;
+
+ if (sensor->scaler) {
+ rval = smiapp_register_subdev(
+ sensor, sensor->binner, sensor->scaler,
+ SMIAPP_PAD_SRC, SMIAPP_PAD_SINK,
+ MEDIA_LNK_FL_ENABLED | MEDIA_LNK_FL_IMMUTABLE);
+ if (rval < 0)
return rval;
- }
}
- return 0;
+ return smiapp_register_subdev(
+ sensor, sensor->pixel_array, sensor->binner,
+ SMIAPP_PA_PAD_SRC, SMIAPP_PAD_SINK,
+ MEDIA_LNK_FL_ENABLED | MEDIA_LNK_FL_IMMUTABLE);
}
static void smiapp_cleanup(struct smiapp_sensor *sensor)
--
2.1.4
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
