Re: [PATCH v3 3/3] media: i2c: imx274: Add IMX274 power on and off sequence

[Sowjanya Komatineni <skomatineni@nvidia.com>]

On 8/27/20 3:55 PM, Sowjanya Komatineni wrote:
>
> On 8/13/20 3:01 PM, Sakari Ailus wrote:
>> 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@nvidia.com>
>>>>> ---
>>>>>    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.
>>
> regulator_bulk APIs uses regulator_bulk_data and so had to retrieve 
> regulators from DT with devm_regulator_bulk_get() which don't use 
> optional regulator get.
>
Sorry, please ignore my above comment. Yes, will fix in v4 to use 
bulk_enable()