Hi Sowjanya,
On Fri, Jul 31, 2020 at 09:34:15AM -0700, Sowjanya Komatineni wrote:
>
> On 7/31/20 9:26 AM, Sakari Ailus wrote:
> > Hi Sowjanya,
> >
> > Thanks for the patch.
> >
> > On Mon, Jul 20, 2020 at 10:01:34AM -0700, Sowjanya Komatineni wrote:
> > > IMX274 has VANA analog 2.8V supply, VDIG digital core 1.8V supply,
> > > and VDDL digital io 1.2V supply which are optional based on camera
> > > module design.
> > >
> > > IMX274 also need external 24Mhz clock and is optional based on
> > > camera module design.
> > The sensor appears to be able to use other frequencies, too. Could you
> > check in the driver the frequency is correct? This should be found in DT
> > bindings, too.
>
> External input clock is not in DT. So added it as part of this series.
>
> We are mostly using 24Mhz I/P with IMX274 on designs we have and also on
> leopard module which has onboard XTAL for 24Mhz
Yes. This information still should be found in DT as the xtal isn't part of
the sensor.
>
> > > This patch adds support for IMX274 power on and off to enable and
> > > disable these supplies and external clock.
> > >
> > > Signed-off-by: Sowjanya Komatineni <skomatineni@xxxxxxxxxx>
> > > ---
> > > drivers/media/i2c/imx274.c | 132 +++++++++++++++++++++++++++++++++++++++++++--
> > > 1 file changed, 129 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/drivers/media/i2c/imx274.c b/drivers/media/i2c/imx274.c
> > > index 55869ff..7157b1d 100644
> > > --- a/drivers/media/i2c/imx274.c
> > > +++ b/drivers/media/i2c/imx274.c
> > > @@ -19,6 +19,7 @@
> > > #include <linux/module.h>
> > > #include <linux/of_gpio.h>
> > > #include <linux/regmap.h>
> > > +#include <linux/regulator/consumer.h>
> > > #include <linux/slab.h>
> > > #include <linux/v4l2-mediabus.h>
> > > #include <linux/videodev2.h>
> > > @@ -131,6 +132,15 @@
> > > #define IMX274_TABLE_WAIT_MS 0
> > > #define IMX274_TABLE_END 1
> > > +/* regulator supplies */
> > > +static const char * const imx274_supply_names[] = {
> > > + "VDDL", /* IF (1.2V) supply */
> > > + "VDIG", /* Digital Core (1.8V) supply */
> > > + "VANA", /* Analog (2.8V) supply */
> > > +};
> > > +
> > > +#define IMX274_NUM_SUPPLIES ARRAY_SIZE(imx274_supply_names)
> > Please use ARRAY_SIZE() directly.
> >
> > > +
> > > /*
> > > * imx274 I2C operation related structure
> > > */
> > > @@ -501,6 +511,8 @@ struct imx274_ctrls {
> > > * @frame_rate: V4L2 frame rate structure
> > > * @regmap: Pointer to regmap structure
> > > * @reset_gpio: Pointer to reset gpio
> > > + * @supplies: imx274 analog and digital supplies
> > > + * @inck: input clock to imx274
> > > * @lock: Mutex structure
> > > * @mode: Parameters for the selected readout mode
> > > */
> > > @@ -514,6 +526,8 @@ struct stimx274 {
> > > struct v4l2_fract frame_interval;
> > > struct regmap *regmap;
> > > struct gpio_desc *reset_gpio;
> > > + struct regulator *supplies[IMX274_NUM_SUPPLIES];
> > > + struct clk *inck;
> > > struct mutex lock; /* mutex lock for operations */
> > > const struct imx274_mode *mode;
> > > };
> > > @@ -767,6 +781,99 @@ static void imx274_reset(struct stimx274 *priv, int rst)
> > > usleep_range(IMX274_RESET_DELAY1, IMX274_RESET_DELAY2);
> > > }
> > > +/*
> > > + * imx274_power_on - Function called to power on the sensor
> > > + * @imx274: Pointer to device structure
> > > + */
> > > +static int imx274_power_on(struct device *dev)
> > > +{
> > > + struct i2c_client *client = to_i2c_client(dev);
> > > + struct v4l2_subdev *sd = i2c_get_clientdata(client);
> > > + struct stimx274 *imx274 = to_imx274(sd);
> > > + int i, ret;
> > > +
> > > + ret = clk_prepare_enable(imx274->inck);
> > > + if (ret) {
> > > + dev_err(&imx274->client->dev,
> > > + "Failed to enable input clock: %d\n", ret);
> > > + return ret;
> > > + }
> > > +
> > Could you use regulator_bulk_enable() instead? Same for disable.
>
> Using regulator_bulk_enable() makes these regulators mandatory.
How? regulator_bulk_enable() simply does call regulator_enable() on all the
regulators.
--
Sakari Ailus
