Hi Laurent,
On Tue, Sep 19, 2017 at 03:08:25PM +0300, Laurent Pinchart wrote:
> Hi Sakari,
>
> Thank you for the patch.
>
> On Friday, 15 September 2017 17:17:20 EEST Sakari Ailus wrote:
> > Parse async sub-devices by using
> > v4l2_subdev_fwnode_reference_parse_sensor_common().
> >
> > These types devices aren't directly related to the sensor, but are
> > nevertheless handled by the smiapp driver due to the relationship of these
> > component to the main part of the camera module --- the sensor.
> >
> > This does not yet address providing the user space with information on how
> > to associate the sensor or lens devices but the kernel now has the
> > necessary information to do that.
> >
> > Signed-off-by: Sakari Ailus <sakari.ailus@xxxxxxxxxxxxxxx>
> > Acked-by: Hans Verkuil <hans.verkuil@xxxxxxxxx>
> > Acked-by: Pavel Machek <pavel@xxxxxx>
>
> Something is bothering me here, not so much in the contents of the patch
> itself than in its nature. There are four patches in this series that add
> support for flash and lens devices to the smiapp, et8ek8, ov5670 and ov13858
> drivers. We have way more sensor drivers than that, and I don't really want to
> patch them all to support flash and lens. I believe a less intrusive approach,
> more focussed on the V4L2 core, is needed.
You could move this to the framework, yes. Nothing prevents that. Perhaps a
flag telling the framework whether this should be done? Just being
opportunistic might work as well.
>
> > ---
> > drivers/media/i2c/smiapp/smiapp-core.c | 38 ++++++++++++++++++++++++-------
> > drivers/media/i2c/smiapp/smiapp.h | 4 +++-
> > 2 files changed, 33 insertions(+), 9 deletions(-)
> >
> > diff --git a/drivers/media/i2c/smiapp/smiapp-core.c
> > b/drivers/media/i2c/smiapp/smiapp-core.c index 700f433261d0..a4735a96ea41
> > 100644
> > --- a/drivers/media/i2c/smiapp/smiapp-core.c
> > +++ b/drivers/media/i2c/smiapp/smiapp-core.c
> > @@ -31,7 +31,7 @@
> > #include <linux/regulator/consumer.h>
> > #include <linux/slab.h>
> > #include <linux/smiapp.h>
> > -#include <linux/v4l2-mediabus.h>
> > +#include <media/v4l2-async.h>
> > #include <media/v4l2-fwnode.h>
> > #include <media/v4l2-device.h>
> >
> > @@ -2887,17 +2887,24 @@ static int smiapp_probe(struct i2c_client *client,
> > v4l2_i2c_subdev_init(&sensor->src->sd, client, &smiapp_ops);
> > sensor->src->sd.internal_ops = &smiapp_internal_src_ops;
> >
> > + rval = v4l2_async_notifier_parse_fwnode_sensor_common(
> > + &client->dev, &sensor->notifier);
> > + if (rval < 0)
> > + return rval;
> > +
> > sensor->vana = devm_regulator_get(&client->dev, "vana");
> > if (IS_ERR(sensor->vana)) {
> > dev_err(&client->dev, "could not get regulator for vana\n");
> > - return PTR_ERR(sensor->vana);
> > + rval = PTR_ERR(sensor->vana);
> > + goto out_release_async_notifier;
> > }
> >
> > sensor->ext_clk = devm_clk_get(&client->dev, NULL);
> > if (IS_ERR(sensor->ext_clk)) {
> > dev_err(&client->dev, "could not get clock (%ld)\n",
> > PTR_ERR(sensor->ext_clk));
> > - return -EPROBE_DEFER;
> > + rval = -EPROBE_DEFER;
> > + goto out_release_async_notifier;
> > }
> >
> > rval = clk_set_rate(sensor->ext_clk, sensor->hwcfg->ext_clk);
> > @@ -2905,17 +2912,19 @@ static int smiapp_probe(struct i2c_client *client,
> > dev_err(&client->dev,
> > "unable to set clock freq to %u\n",
> > sensor->hwcfg->ext_clk);
> > - return rval;
> > + goto out_release_async_notifier;
> > }
> >
> > sensor->xshutdown = devm_gpiod_get_optional(&client->dev, "xshutdown",
> > GPIOD_OUT_LOW);
> > - if (IS_ERR(sensor->xshutdown))
> > - return PTR_ERR(sensor->xshutdown);
> > + if (IS_ERR(sensor->xshutdown)) {
> > + rval = PTR_ERR(sensor->xshutdown);
> > + goto out_release_async_notifier;
> > + }
> >
> > rval = smiapp_power_on(&client->dev);
> > if (rval < 0)
> > - return rval;
> > + goto out_release_async_notifier;
> >
> > rval = smiapp_identify_module(sensor);
> > if (rval) {
> > @@ -3092,9 +3101,14 @@ static int smiapp_probe(struct i2c_client *client,
> > if (rval < 0)
> > goto out_media_entity_cleanup;
> >
> > + rval = v4l2_async_subdev_notifier_register(&sensor->src->sd,
> > + &sensor->notifier);
> > + if (rval)
> > + goto out_media_entity_cleanup;
> > +
> > rval = v4l2_async_register_subdev(&sensor->src->sd);
> > if (rval < 0)
> > - goto out_media_entity_cleanup;
> > + goto out_unregister_async_notifier;
> >
> > pm_runtime_set_active(&client->dev);
> > pm_runtime_get_noresume(&client->dev);
> > @@ -3105,6 +3119,9 @@ static int smiapp_probe(struct i2c_client *client,
> >
> > return 0;
> >
> > +out_unregister_async_notifier:
> > + v4l2_async_notifier_unregister(&sensor->notifier);
> > +
> > out_media_entity_cleanup:
> > media_entity_cleanup(&sensor->src->sd.entity);
> >
> > @@ -3114,6 +3131,9 @@ static int smiapp_probe(struct i2c_client *client,
> > out_power_off:
> > smiapp_power_off(&client->dev);
> >
> > +out_release_async_notifier:
> > + v4l2_async_notifier_release(&sensor->notifier);
> > +
> > return rval;
> > }
> >
> > @@ -3124,6 +3144,8 @@ static int smiapp_remove(struct i2c_client *client)
> > unsigned int i;
> >
> > v4l2_async_unregister_subdev(subdev);
> > + v4l2_async_notifier_unregister(&sensor->notifier);
> > + v4l2_async_notifier_release(&sensor->notifier);
> >
> > pm_runtime_disable(&client->dev);
> > if (!pm_runtime_status_suspended(&client->dev))
> > diff --git a/drivers/media/i2c/smiapp/smiapp.h
> > b/drivers/media/i2c/smiapp/smiapp.h index f74d695018b9..be92cb5713f4 100644
> > --- a/drivers/media/i2c/smiapp/smiapp.h
> > +++ b/drivers/media/i2c/smiapp/smiapp.h
> > @@ -20,9 +20,10 @@
> > #define __SMIAPP_PRIV_H_
> >
> > #include <linux/mutex.h>
> > +#include <media/i2c/smiapp.h>
> > +#include <media/v4l2-async.h>
> > #include <media/v4l2-ctrls.h>
> > #include <media/v4l2-subdev.h>
> > -#include <media/i2c/smiapp.h>
> >
> > #include "smiapp-pll.h"
> > #include "smiapp-reg.h"
> > @@ -172,6 +173,7 @@ struct smiapp_subdev {
> > * struct smiapp_sensor - Main device structure
> > */
> > struct smiapp_sensor {
> > + struct v4l2_async_notifier notifier;
> > /*
> > * "mutex" is used to serialise access to all fields here
> > * except v4l2_ctrls at the end of the struct. "mutex" is also
>
>
> --
> Regards,
>
> Laurent Pinchart
>
--
Sakari Ailus
sakari.ailus@xxxxxxxxxxxxxxx
