Re: [PATCH v5 2/4] media: ov5675: add device-tree support and support runtime PM

[Quentin Schulz]

Hi Jacopo,

On 5/31/22 15:16, Jacopo Mondi wrote:
> Hi Quentin,
>     one more question
>
> On Wed, May 25, 2022 at 04:58:31PM +0200, Quentin Schulz wrote:
> > From: Quentin Schulz <quentin.schulz@xxxxxxxxxxxxxxxxxxxxx>
> >
> > Until now, this driver only supported ACPI. This adds support for
> > Device Tree too while enabling clock and regulators in runtime PM.
> >
> > Signed-off-by: Quentin Schulz <quentin.schulz@xxxxxxxxxxxxxxxxxxxxx>
> > ---
> >
> > v5:
> >   - fixed -Wdeclaration-after-statement for delay_us,
> >
> > v4:
> >   - added delays based on clock cycles as specified in datasheet for
> >   pre-power-off and post-power-on,
> >   - re-arranged clk handling, shutdown toggling and regulator handling to
> >   better match power up/down sequence defined in datasheet,
> >   - added comment on need for regulator being stable before releasing
> >   shutdown pin,
> >
> > v3:
> >   - added linux/mod_devicetable.h include,
> >   - moved delay for reset pulse right after the regulators are enabled,
> >   - removed check on is_acpi_node in favor of checks on presence of OF
> >   properties (e.g. devm_clk_get_optional returns NULL),
> >   - moved power management out of system suspend/resume into runtime PM
> >   callbacks,
> >   - removed ACPI specific comment since it's not specific to this driver,
> >   - changed devm_clk_get to devm_clk_get_optional,
> >   - remove OF use of clock-frequency (handled by devm_clk_get_optional
> >   directly),
> >   - removed name of clock (only one, so no need for anything explicit)
> >   when requesting a clock from OF,
> >   - wrapped lines to 80 chars,
> >
> > v2:
> >   - fixed unused-const-variable warning by removing of_match_ptr in
> >   of_match_table, reported by kernel test robot,
> >
> >   drivers/media/i2c/ov5675.c | 149 +++++++++++++++++++++++++++++++------
> >   1 file changed, 128 insertions(+), 21 deletions(-)
> >
> > diff --git a/drivers/media/i2c/ov5675.c b/drivers/media/i2c/ov5675.c
> > index 82ba9f56baec8..ea801edb8e408 100644
> > --- a/drivers/media/i2c/ov5675.c
> > +++ b/drivers/media/i2c/ov5675.c
> > @@ -3,10 +3,14 @@
> >
> >   #include <asm/unaligned.h>
> >   #include <linux/acpi.h>
> > +#include <linux/clk.h>
> >   #include <linux/delay.h>
> > +#include <linux/gpio/consumer.h>
> >   #include <linux/i2c.h>
> > +#include <linux/mod_devicetable.h>
> >   #include <linux/module.h>
> >   #include <linux/pm_runtime.h>
> > +#include <linux/regulator/consumer.h>
> >   #include <media/v4l2-ctrls.h>
> >   #include <media/v4l2-device.h>
> >   #include <media/v4l2-fwnode.h>
> > @@ -17,7 +21,7 @@
> >
> >   #define OV5675_LINK_FREQ_450MHZ		450000000ULL
> >   #define OV5675_SCLK			90000000LL
> > -#define OV5675_MCLK			19200000
> > +#define OV5675_XVCLK_19_2		19200000
> >   #define OV5675_DATA_LANES		2
> >   #define OV5675_RGB_DEPTH		10
> >
> > @@ -76,6 +80,14 @@
> >
> >   #define to_ov5675(_sd)			container_of(_sd, struct ov5675, sd)
> >
> > +static const char * const ov5675_supply_names[] = {
> > +	"avdd",		/* Analog power */
> > +	"dovdd",	/* Digital I/O power */
> > +	"dvdd",		/* Digital core power */
> > +};
> > +
> > +#define OV5675_NUM_SUPPLIES	ARRAY_SIZE(ov5675_supply_names)
> > +
> >   enum {
> >   	OV5675_LINK_FREQ_900MBPS,
> >   };
> > @@ -484,6 +496,9 @@ struct ov5675 {
> >   	struct v4l2_subdev sd;
> >   	struct media_pad pad;
> >   	struct v4l2_ctrl_handler ctrl_handler;
> > +	struct clk		*xvclk;
> > +	struct gpio_desc	*reset_gpio;
> > +	struct regulator_bulk_data supplies[OV5675_NUM_SUPPLIES];
> >
> >   	/* V4L2 Controls */
> >   	struct v4l2_ctrl *link_freq;
> > @@ -944,6 +959,56 @@ static int ov5675_set_stream(struct v4l2_subdev *sd, int enable)
> >   	return ret;
> >   }
> >
> > +static int ov5675_power_off(struct device *dev)
>
> Does this (and power_on) require __maybe_unused to avoid a warning
> when compiling without CONFIG_PM support ? Have you tried that ?

It does not, because they are called manually in the probe function and 
its error path (but thanks for the heads up, because I definitely forgot 
about this one).

I received some review outside of this mailing list telling me the error 
path is incorrect for pm_runtime so I'll have a look at it before 
sending a v6.

Cheers,
Quentin