* Re: [PATCH v2 3/3] media: i2c: ak7375: Add regulator management
2022-07-11 14:40 ` [PATCH v2 3/3] media: i2c: ak7375: Add regulator management Yassine Oudjana
@ 2022-07-11 17:31 ` Jacopo Mondi
2022-07-14 13:56 ` Yassine Oudjana
2022-07-20 7:36 ` Christophe JAILLET
2022-07-13 7:39 ` Jacopo Mondi
` (2 subsequent siblings)
3 siblings, 2 replies; 17+ messages in thread
From: Jacopo Mondi @ 2022-07-11 17:31 UTC (permalink / raw)
To: Yassine Oudjana
Cc: Mauro Carvalho Chehab, Rob Herring, Krzysztof Kozlowski,
Tianshu Qiu, Bingbu Cao, Yassine Oudjana, linux-media,
devicetree, linux-kernel
Hi Yassine,
On Mon, Jul 11, 2022 at 06:40:39PM +0400, Yassine Oudjana wrote:
> From: Yassine Oudjana <y.oudjana@protonmail.com>
>
> Make the driver get needed regulators on probe and enable/disable
> them on runtime PM callbacks.
>
> Signed-off-by: Yassine Oudjana <y.oudjana@protonmail.com>
> ---
> Changes since v1:
> - Reorganize variable declaration
> - Change the power-on delay range to 3000-3500 microseconds.
>
> drivers/media/i2c/ak7375.c | 39 ++++++++++++++++++++++++++++++++++++++
> 1 file changed, 39 insertions(+)
>
> diff --git a/drivers/media/i2c/ak7375.c b/drivers/media/i2c/ak7375.c
> index 40b1a4aa846c..c2b2542a0056 100644
> --- a/drivers/media/i2c/ak7375.c
> +++ b/drivers/media/i2c/ak7375.c
> @@ -6,6 +6,7 @@
> #include <linux/i2c.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>
>
> @@ -23,17 +24,32 @@
> */
> #define AK7375_CTRL_STEPS 64
> #define AK7375_CTRL_DELAY_US 1000
> +/*
> + * The vcm takes around 3 ms to power on and start taking
> + * I2C messages. This value was found experimentally due to
> + * lack of documentation.
> + */
> +#define AK7375_POWER_DELAY_US 3000
>
> #define AK7375_REG_POSITION 0x0
> #define AK7375_REG_CONT 0x2
> #define AK7375_MODE_ACTIVE 0x0
> #define AK7375_MODE_STANDBY 0x40
>
> +static const char * const ak7375_supply_names[] = {
> + "vdd",
> + "vio",
> +};
> +
> +#define AK7375_NUM_SUPPLIES ARRAY_SIZE(ak7375_supply_names)
> +
> /* ak7375 device structure */
> struct ak7375_device {
> struct v4l2_ctrl_handler ctrls_vcm;
> struct v4l2_subdev sd;
> struct v4l2_ctrl *focus;
> + struct regulator_bulk_data supplies[AK7375_NUM_SUPPLIES];
> +
> /* active or standby mode */
> bool active;
> };
> @@ -133,12 +149,24 @@ static int ak7375_probe(struct i2c_client *client)
> {
> struct ak7375_device *ak7375_dev;
> int ret;
> + int i;
>
> ak7375_dev = devm_kzalloc(&client->dev, sizeof(*ak7375_dev),
> GFP_KERNEL);
> if (!ak7375_dev)
> return -ENOMEM;
>
> + for (i = 0; i < AK7375_NUM_SUPPLIES; i++)
> + ak7375_dev->supplies[i].supply = ak7375_supply_names[i];
> +
> + ret = devm_regulator_bulk_get(&client->dev, AK7375_NUM_SUPPLIES,
> + ak7375_dev->supplies);
> + if (ret) {
> + dev_err(&client->dev, "Failed to get regulators: %pe",
> + ERR_PTR(ret));
Why are you using %pe here ? Your return value is not a pointer
(Also, missing \n at the end of the string)
From Documentation/core-api/printk-formats.rst:
%pe -ENOSPC
For printing error pointers (i.e. a pointer for which IS_ERR() is true)
as a symbolic error name. Error values for which no symbolic name is
known are printed in decimal, while a non-ERR_PTR passed as the
argument to %pe gets treated as ordinary %p.
> + return ret;
> + }
> +
> v4l2_i2c_subdev_init(&ak7375_dev->sd, client, &ak7375_ops);
> ak7375_dev->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
> ak7375_dev->sd.internal_ops = &ak7375_int_ops;
> @@ -210,6 +238,10 @@ static int __maybe_unused ak7375_vcm_suspend(struct device *dev)
> if (ret)
> dev_err(dev, "%s I2C failure: %d\n", __func__, ret);
>
> + ret = regulator_bulk_disable(AK7375_NUM_SUPPLIES, ak7375_dev->supplies);
> + if (ret)
> + return ret;
> +
> ak7375_dev->active = false;
>
> return 0;
> @@ -230,6 +262,13 @@ static int __maybe_unused ak7375_vcm_resume(struct device *dev)
> if (ak7375_dev->active)
> return 0;
>
> + ret = regulator_bulk_enable(AK7375_NUM_SUPPLIES, ak7375_dev->supplies);
> + if (ret)
> + return ret;
> +
> + /* Wait for vcm to become ready */
> + usleep_range(AK7375_POWER_DELAY_US, AK7375_POWER_DELAY_US + 500);
> +
> ret = ak7375_i2c_write(ak7375_dev, AK7375_REG_CONT,
> AK7375_MODE_ACTIVE, 1);
> if (ret) {
> --
> 2.37.0
>
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [PATCH v2 3/3] media: i2c: ak7375: Add regulator management
2022-07-11 17:31 ` Jacopo Mondi
@ 2022-07-14 13:56 ` Yassine Oudjana
2022-07-14 15:56 ` Jacopo Mondi
2022-07-20 7:36 ` Christophe JAILLET
1 sibling, 1 reply; 17+ messages in thread
From: Yassine Oudjana @ 2022-07-14 13:56 UTC (permalink / raw)
To: Jacopo Mondi
Cc: Mauro Carvalho Chehab, Rob Herring, Krzysztof Kozlowski,
Tianshu Qiu, Bingbu Cao, Yassine Oudjana, linux-media,
devicetree, linux-kernel
On Mon, Jul 11 2022 at 19:31:23 +0200, Jacopo Mondi <jacopo@jmondi.org>
wrote:
> Hi Yassine,
>
> On Mon, Jul 11, 2022 at 06:40:39PM +0400, Yassine Oudjana wrote:
>> From: Yassine Oudjana <y.oudjana@protonmail.com>
>>
>> Make the driver get needed regulators on probe and enable/disable
>> them on runtime PM callbacks.
>>
>> Signed-off-by: Yassine Oudjana <y.oudjana@protonmail.com>
>> ---
>> Changes since v1:
>> - Reorganize variable declaration
>> - Change the power-on delay range to 3000-3500 microseconds.
>>
>> drivers/media/i2c/ak7375.c | 39
>> ++++++++++++++++++++++++++++++++++++++
>> 1 file changed, 39 insertions(+)
>>
>> diff --git a/drivers/media/i2c/ak7375.c b/drivers/media/i2c/ak7375.c
>> index 40b1a4aa846c..c2b2542a0056 100644
>> --- a/drivers/media/i2c/ak7375.c
>> +++ b/drivers/media/i2c/ak7375.c
>> @@ -6,6 +6,7 @@
>> #include <linux/i2c.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>
>>
>> @@ -23,17 +24,32 @@
>> */
>> #define AK7375_CTRL_STEPS 64
>> #define AK7375_CTRL_DELAY_US 1000
>> +/*
>> + * The vcm takes around 3 ms to power on and start taking
>> + * I2C messages. This value was found experimentally due to
>> + * lack of documentation.
>> + */
>> +#define AK7375_POWER_DELAY_US 3000
>>
>> #define AK7375_REG_POSITION 0x0
>> #define AK7375_REG_CONT 0x2
>> #define AK7375_MODE_ACTIVE 0x0
>> #define AK7375_MODE_STANDBY 0x40
>>
>> +static const char * const ak7375_supply_names[] = {
>> + "vdd",
>> + "vio",
>> +};
>> +
>> +#define AK7375_NUM_SUPPLIES ARRAY_SIZE(ak7375_supply_names)
>> +
>> /* ak7375 device structure */
>> struct ak7375_device {
>> struct v4l2_ctrl_handler ctrls_vcm;
>> struct v4l2_subdev sd;
>> struct v4l2_ctrl *focus;
>> + struct regulator_bulk_data supplies[AK7375_NUM_SUPPLIES];
>> +
>> /* active or standby mode */
>> bool active;
>> };
>> @@ -133,12 +149,24 @@ static int ak7375_probe(struct i2c_client
>> *client)
>> {
>> struct ak7375_device *ak7375_dev;
>> int ret;
>> + int i;
>>
>> ak7375_dev = devm_kzalloc(&client->dev, sizeof(*ak7375_dev),
>> GFP_KERNEL);
>> if (!ak7375_dev)
>> return -ENOMEM;
>>
>> + for (i = 0; i < AK7375_NUM_SUPPLIES; i++)
>> + ak7375_dev->supplies[i].supply = ak7375_supply_names[i];
>> +
>> + ret = devm_regulator_bulk_get(&client->dev, AK7375_NUM_SUPPLIES,
>> + ak7375_dev->supplies);
>> + if (ret) {
>> + dev_err(&client->dev, "Failed to get regulators: %pe",
>> + ERR_PTR(ret));
>
> Why are you using %pe here ? Your return value is not a pointer
In order to have it print a symbolic error name instead of a value
with CONFIG_SYMBOLIC_ERRNAME=y. There is no format code for an
error integer (or at least I couldn't find one mentioned anywhere
in the docs), so instead I use %pe then wrap `ret` in ERR_PTR().
> (Also, missing \n at the end of the string)
That wasn't intentional. I'll fix it.
>
> From Documentation/core-api/printk-formats.rst:
> %pe -ENOSPC
>
> For printing error pointers (i.e. a pointer for which IS_ERR() is
> true)
> as a symbolic error name. Error values for which no symbolic name is
> known are printed in decimal, while a non-ERR_PTR passed as the
> argument to %pe gets treated as ordinary %p.
>
>> + return ret;
>> + }
>> +
>> v4l2_i2c_subdev_init(&ak7375_dev->sd, client, &ak7375_ops);
>> ak7375_dev->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
>> ak7375_dev->sd.internal_ops = &ak7375_int_ops;
>> @@ -210,6 +238,10 @@ static int __maybe_unused
>> ak7375_vcm_suspend(struct device *dev)
>> if (ret)
>> dev_err(dev, "%s I2C failure: %d\n", __func__, ret);
>>
>> + ret = regulator_bulk_disable(AK7375_NUM_SUPPLIES,
>> ak7375_dev->supplies);
>> + if (ret)
>> + return ret;
>> +
>> ak7375_dev->active = false;
>>
>> return 0;
>> @@ -230,6 +262,13 @@ static int __maybe_unused
>> ak7375_vcm_resume(struct device *dev)
>> if (ak7375_dev->active)
>> return 0;
>>
>> + ret = regulator_bulk_enable(AK7375_NUM_SUPPLIES,
>> ak7375_dev->supplies);
>> + if (ret)
>> + return ret;
>> +
>> + /* Wait for vcm to become ready */
>> + usleep_range(AK7375_POWER_DELAY_US, AK7375_POWER_DELAY_US + 500);
>> +
>> ret = ak7375_i2c_write(ak7375_dev, AK7375_REG_CONT,
>> AK7375_MODE_ACTIVE, 1);
>> if (ret) {
>> --
>> 2.37.0
>>
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [PATCH v2 3/3] media: i2c: ak7375: Add regulator management
2022-07-14 13:56 ` Yassine Oudjana
@ 2022-07-14 15:56 ` Jacopo Mondi
0 siblings, 0 replies; 17+ messages in thread
From: Jacopo Mondi @ 2022-07-14 15:56 UTC (permalink / raw)
To: Yassine Oudjana
Cc: Mauro Carvalho Chehab, Rob Herring, Krzysztof Kozlowski,
Tianshu Qiu, Bingbu Cao, Yassine Oudjana, linux-media,
devicetree, linux-kernel
Hi Yassine,
On Thu, Jul 14, 2022 at 05:56:29PM +0400, Yassine Oudjana wrote:
>
> On Mon, Jul 11 2022 at 19:31:23 +0200, Jacopo Mondi <jacopo@jmondi.org>
> wrote:
> > Hi Yassine,
> >
> > On Mon, Jul 11, 2022 at 06:40:39PM +0400, Yassine Oudjana wrote:
> > > From: Yassine Oudjana <y.oudjana@protonmail.com>
> > >
> > > Make the driver get needed regulators on probe and enable/disable
> > > them on runtime PM callbacks.
> > >
> > > Signed-off-by: Yassine Oudjana <y.oudjana@protonmail.com>
> > > ---
> > > Changes since v1:
> > > - Reorganize variable declaration
> > > - Change the power-on delay range to 3000-3500 microseconds.
> > >
> > > drivers/media/i2c/ak7375.c | 39
> > > ++++++++++++++++++++++++++++++++++++++
> > > 1 file changed, 39 insertions(+)
> > >
> > > diff --git a/drivers/media/i2c/ak7375.c b/drivers/media/i2c/ak7375.c
> > > index 40b1a4aa846c..c2b2542a0056 100644
> > > --- a/drivers/media/i2c/ak7375.c
> > > +++ b/drivers/media/i2c/ak7375.c
> > > @@ -6,6 +6,7 @@
> > > #include <linux/i2c.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>
> > >
> > > @@ -23,17 +24,32 @@
> > > */
> > > #define AK7375_CTRL_STEPS 64
> > > #define AK7375_CTRL_DELAY_US 1000
> > > +/*
> > > + * The vcm takes around 3 ms to power on and start taking
> > > + * I2C messages. This value was found experimentally due to
> > > + * lack of documentation.
> > > + */
> > > +#define AK7375_POWER_DELAY_US 3000
> > >
> > > #define AK7375_REG_POSITION 0x0
> > > #define AK7375_REG_CONT 0x2
> > > #define AK7375_MODE_ACTIVE 0x0
> > > #define AK7375_MODE_STANDBY 0x40
> > >
> > > +static const char * const ak7375_supply_names[] = {
> > > + "vdd",
> > > + "vio",
> > > +};
> > > +
> > > +#define AK7375_NUM_SUPPLIES ARRAY_SIZE(ak7375_supply_names)
> > > +
> > > /* ak7375 device structure */
> > > struct ak7375_device {
> > > struct v4l2_ctrl_handler ctrls_vcm;
> > > struct v4l2_subdev sd;
> > > struct v4l2_ctrl *focus;
> > > + struct regulator_bulk_data supplies[AK7375_NUM_SUPPLIES];
> > > +
> > > /* active or standby mode */
> > > bool active;
> > > };
> > > @@ -133,12 +149,24 @@ static int ak7375_probe(struct i2c_client
> > > *client)
> > > {
> > > struct ak7375_device *ak7375_dev;
> > > int ret;
> > > + int i;
> > >
> > > ak7375_dev = devm_kzalloc(&client->dev, sizeof(*ak7375_dev),
> > > GFP_KERNEL);
> > > if (!ak7375_dev)
> > > return -ENOMEM;
> > >
> > > + for (i = 0; i < AK7375_NUM_SUPPLIES; i++)
> > > + ak7375_dev->supplies[i].supply = ak7375_supply_names[i];
> > > +
> > > + ret = devm_regulator_bulk_get(&client->dev, AK7375_NUM_SUPPLIES,
> > > + ak7375_dev->supplies);
> > > + if (ret) {
> > > + dev_err(&client->dev, "Failed to get regulators: %pe",
> > > + ERR_PTR(ret));
> >
> > Why are you using %pe here ? Your return value is not a pointer
>
> In order to have it print a symbolic error name instead of a value
> with CONFIG_SYMBOLIC_ERRNAME=y. There is no format code for an
> error integer (or at least I couldn't find one mentioned anywhere
> in the docs), so instead I use %pe then wrap `ret` in ERR_PTR().
>
Ah nice, sorry I didn't realize. I grepped around in drivers/media and
saw it only used with pointer values, but I actually missed two
drivers that wrap ret in ERR_PTR() like you're doing here.
> > (Also, missing \n at the end of the string)
>
> That wasn't intentional. I'll fix it.
>
That's indeed minor.
Let me reply to the other email about delays..
> >
> > From Documentation/core-api/printk-formats.rst:
> > %pe -ENOSPC
> >
> > For printing error pointers (i.e. a pointer for which IS_ERR() is true)
> > as a symbolic error name. Error values for which no symbolic name is
> > known are printed in decimal, while a non-ERR_PTR passed as the
> > argument to %pe gets treated as ordinary %p.
> >
> > > + return ret;
> > > + }
> > > +
> > > v4l2_i2c_subdev_init(&ak7375_dev->sd, client, &ak7375_ops);
> > > ak7375_dev->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
> > > ak7375_dev->sd.internal_ops = &ak7375_int_ops;
> > > @@ -210,6 +238,10 @@ static int __maybe_unused
> > > ak7375_vcm_suspend(struct device *dev)
> > > if (ret)
> > > dev_err(dev, "%s I2C failure: %d\n", __func__, ret);
> > >
> > > + ret = regulator_bulk_disable(AK7375_NUM_SUPPLIES,
> > > ak7375_dev->supplies);
> > > + if (ret)
> > > + return ret;
> > > +
> > > ak7375_dev->active = false;
> > >
> > > return 0;
> > > @@ -230,6 +262,13 @@ static int __maybe_unused
> > > ak7375_vcm_resume(struct device *dev)
> > > if (ak7375_dev->active)
> > > return 0;
> > >
> > > + ret = regulator_bulk_enable(AK7375_NUM_SUPPLIES,
> > > ak7375_dev->supplies);
> > > + if (ret)
> > > + return ret;
> > > +
> > > + /* Wait for vcm to become ready */
> > > + usleep_range(AK7375_POWER_DELAY_US, AK7375_POWER_DELAY_US + 500);
> > > +
> > > ret = ak7375_i2c_write(ak7375_dev, AK7375_REG_CONT,
> > > AK7375_MODE_ACTIVE, 1);
> > > if (ret) {
> > > --
> > > 2.37.0
> > >
>
>
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2 3/3] media: i2c: ak7375: Add regulator management
2022-07-11 17:31 ` Jacopo Mondi
2022-07-14 13:56 ` Yassine Oudjana
@ 2022-07-20 7:36 ` Christophe JAILLET
1 sibling, 0 replies; 17+ messages in thread
From: Christophe JAILLET @ 2022-07-20 7:36 UTC (permalink / raw)
To: Jacopo Mondi, Yassine Oudjana
Cc: Mauro Carvalho Chehab, Rob Herring, Krzysztof Kozlowski,
Tianshu Qiu, Bingbu Cao, Yassine Oudjana, linux-media,
devicetree, linux-kernel
Le 11/07/2022 à 19:31, Jacopo Mondi a écrit :
> Hi Yassine,
>
> On Mon, Jul 11, 2022 at 06:40:39PM +0400, Yassine Oudjana wrote:
>> From: Yassine Oudjana <y.oudjana@protonmail.com>
>>
>> Make the driver get needed regulators on probe and enable/disable
>> them on runtime PM callbacks.
>>
>> Signed-off-by: Yassine Oudjana <y.oudjana@protonmail.com>
>> ---
>> Changes since v1:
>> - Reorganize variable declaration
>> - Change the power-on delay range to 3000-3500 microseconds.
>>
>> drivers/media/i2c/ak7375.c | 39 ++++++++++++++++++++++++++++++++++++++
>> 1 file changed, 39 insertions(+)
>>
>> diff --git a/drivers/media/i2c/ak7375.c b/drivers/media/i2c/ak7375.c
>> index 40b1a4aa846c..c2b2542a0056 100644
>> --- a/drivers/media/i2c/ak7375.c
>> +++ b/drivers/media/i2c/ak7375.c
>> @@ -6,6 +6,7 @@
>> #include <linux/i2c.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>
>>
>> @@ -23,17 +24,32 @@
>> */
>> #define AK7375_CTRL_STEPS 64
>> #define AK7375_CTRL_DELAY_US 1000
>> +/*
>> + * The vcm takes around 3 ms to power on and start taking
>> + * I2C messages. This value was found experimentally due to
>> + * lack of documentation.
>> + */
>> +#define AK7375_POWER_DELAY_US 3000
>>
>> #define AK7375_REG_POSITION 0x0
>> #define AK7375_REG_CONT 0x2
>> #define AK7375_MODE_ACTIVE 0x0
>> #define AK7375_MODE_STANDBY 0x40
>>
>> +static const char * const ak7375_supply_names[] = {
>> + "vdd",
>> + "vio",
>> +};
>> +
>> +#define AK7375_NUM_SUPPLIES ARRAY_SIZE(ak7375_supply_names)
>> +
>> /* ak7375 device structure */
>> struct ak7375_device {
>> struct v4l2_ctrl_handler ctrls_vcm;
>> struct v4l2_subdev sd;
>> struct v4l2_ctrl *focus;
>> + struct regulator_bulk_data supplies[AK7375_NUM_SUPPLIES];
>> +
>> /* active or standby mode */
>> bool active;
>> };
>> @@ -133,12 +149,24 @@ static int ak7375_probe(struct i2c_client *client)
>> {
>> struct ak7375_device *ak7375_dev;
>> int ret;
>> + int i;
>>
>> ak7375_dev = devm_kzalloc(&client->dev, sizeof(*ak7375_dev),
>> GFP_KERNEL);
>> if (!ak7375_dev)
>> return -ENOMEM;
>>
>> + for (i = 0; i < AK7375_NUM_SUPPLIES; i++)
>> + ak7375_dev->supplies[i].supply = ak7375_supply_names[i];
>> +
>> + ret = devm_regulator_bulk_get(&client->dev, AK7375_NUM_SUPPLIES,
>> + ak7375_dev->supplies);
>> + if (ret) {
>> + dev_err(&client->dev, "Failed to get regulators: %pe",
>> + ERR_PTR(ret));
>
> Why are you using %pe here ? Your return value is not a pointer
> (Also, missing \n at the end of the string)
>
> From Documentation/core-api/printk-formats.rst:
> %pe -ENOSPC
>
> For printing error pointers (i.e. a pointer for which IS_ERR() is true)
> as a symbolic error name. Error values for which no symbolic name is
> known are printed in decimal, while a non-ERR_PTR passed as the
> argument to %pe gets treated as ordinary %p.
>
Nit: using:
+ return dev_err_probe(&client->dev, ret, "Failed to get regulators);
would be even simpler.
CJ
>> + return ret;
>> + }
>> +
>> v4l2_i2c_subdev_init(&ak7375_dev->sd, client, &ak7375_ops);
>> ak7375_dev->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
>> ak7375_dev->sd.internal_ops = &ak7375_int_ops;
>> @@ -210,6 +238,10 @@ static int __maybe_unused ak7375_vcm_suspend(struct device *dev)
>> if (ret)
>> dev_err(dev, "%s I2C failure: %d\n", __func__, ret);
>>
>> + ret = regulator_bulk_disable(AK7375_NUM_SUPPLIES, ak7375_dev->supplies);
>> + if (ret)
>> + return ret;
>> +
>> ak7375_dev->active = false;
>>
>> return 0;
>> @@ -230,6 +262,13 @@ static int __maybe_unused ak7375_vcm_resume(struct device *dev)
>> if (ak7375_dev->active)
>> return 0;
>>
>> + ret = regulator_bulk_enable(AK7375_NUM_SUPPLIES, ak7375_dev->supplies);
>> + if (ret)
>> + return ret;
>> +
>> + /* Wait for vcm to become ready */
>> + usleep_range(AK7375_POWER_DELAY_US, AK7375_POWER_DELAY_US + 500);
>> +
>> ret = ak7375_i2c_write(ak7375_dev, AK7375_REG_CONT,
>> AK7375_MODE_ACTIVE, 1);
>> if (ret) {
>> --
>> 2.37.0
>>
>
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2 3/3] media: i2c: ak7375: Add regulator management
2022-07-11 14:40 ` [PATCH v2 3/3] media: i2c: ak7375: Add regulator management Yassine Oudjana
2022-07-11 17:31 ` Jacopo Mondi
@ 2022-07-13 7:39 ` Jacopo Mondi
2022-07-14 14:06 ` Yassine Oudjana
2022-07-23 11:33 ` Sakari Ailus
2022-11-28 6:47 ` Umang Jain
3 siblings, 1 reply; 17+ messages in thread
From: Jacopo Mondi @ 2022-07-13 7:39 UTC (permalink / raw)
To: Yassine Oudjana
Cc: Mauro Carvalho Chehab, Rob Herring, Krzysztof Kozlowski,
Tianshu Qiu, Bingbu Cao, Yassine Oudjana, linux-media,
devicetree, linux-kernel
Hi Yassine
On Mon, Jul 11, 2022 at 06:40:39PM +0400, Yassine Oudjana wrote:
> From: Yassine Oudjana <y.oudjana@protonmail.com>
>
> Make the driver get needed regulators on probe and enable/disable
> them on runtime PM callbacks.
>
> Signed-off-by: Yassine Oudjana <y.oudjana@protonmail.com>
Have you seen this ?
https://github.com/ArduCAM/IMX519_AK7375/blob/main/AK7375/0002-media-i2c-ak7375-driver-add-optional-regulator-suppo.patch#L172
It claims
+ * Initialisation delay between VDD low->high and the moment
+ * when the i2c command is available.
+ * From the datasheet, it should be 10ms + 2ms (max power
+ * up sequence duration)
10ms seems like a long time, it would be nice to have the datasheet to
cross-check.
Thanks
j
> ---
> Changes since v1:
> - Reorganize variable declaration
> - Change the power-on delay range to 3000-3500 microseconds.
>
> drivers/media/i2c/ak7375.c | 39 ++++++++++++++++++++++++++++++++++++++
> 1 file changed, 39 insertions(+)
>
> diff --git a/drivers/media/i2c/ak7375.c b/drivers/media/i2c/ak7375.c
> index 40b1a4aa846c..c2b2542a0056 100644
> --- a/drivers/media/i2c/ak7375.c
> +++ b/drivers/media/i2c/ak7375.c
> @@ -6,6 +6,7 @@
> #include <linux/i2c.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>
>
> @@ -23,17 +24,32 @@
> */
> #define AK7375_CTRL_STEPS 64
> #define AK7375_CTRL_DELAY_US 1000
> +/*
> + * The vcm takes around 3 ms to power on and start taking
> + * I2C messages. This value was found experimentally due to
> + * lack of documentation.
> + */
> +#define AK7375_POWER_DELAY_US 3000
>
> #define AK7375_REG_POSITION 0x0
> #define AK7375_REG_CONT 0x2
> #define AK7375_MODE_ACTIVE 0x0
> #define AK7375_MODE_STANDBY 0x40
>
> +static const char * const ak7375_supply_names[] = {
> + "vdd",
> + "vio",
> +};
> +
> +#define AK7375_NUM_SUPPLIES ARRAY_SIZE(ak7375_supply_names)
> +
> /* ak7375 device structure */
> struct ak7375_device {
> struct v4l2_ctrl_handler ctrls_vcm;
> struct v4l2_subdev sd;
> struct v4l2_ctrl *focus;
> + struct regulator_bulk_data supplies[AK7375_NUM_SUPPLIES];
> +
> /* active or standby mode */
> bool active;
> };
> @@ -133,12 +149,24 @@ static int ak7375_probe(struct i2c_client *client)
> {
> struct ak7375_device *ak7375_dev;
> int ret;
> + int i;
>
> ak7375_dev = devm_kzalloc(&client->dev, sizeof(*ak7375_dev),
> GFP_KERNEL);
> if (!ak7375_dev)
> return -ENOMEM;
>
> + for (i = 0; i < AK7375_NUM_SUPPLIES; i++)
> + ak7375_dev->supplies[i].supply = ak7375_supply_names[i];
> +
> + ret = devm_regulator_bulk_get(&client->dev, AK7375_NUM_SUPPLIES,
> + ak7375_dev->supplies);
> + if (ret) {
> + dev_err(&client->dev, "Failed to get regulators: %pe",
> + ERR_PTR(ret));
> + return ret;
> + }
> +
> v4l2_i2c_subdev_init(&ak7375_dev->sd, client, &ak7375_ops);
> ak7375_dev->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
> ak7375_dev->sd.internal_ops = &ak7375_int_ops;
> @@ -210,6 +238,10 @@ static int __maybe_unused ak7375_vcm_suspend(struct device *dev)
> if (ret)
> dev_err(dev, "%s I2C failure: %d\n", __func__, ret);
>
> + ret = regulator_bulk_disable(AK7375_NUM_SUPPLIES, ak7375_dev->supplies);
> + if (ret)
> + return ret;
> +
> ak7375_dev->active = false;
>
> return 0;
> @@ -230,6 +262,13 @@ static int __maybe_unused ak7375_vcm_resume(struct device *dev)
> if (ak7375_dev->active)
> return 0;
>
> + ret = regulator_bulk_enable(AK7375_NUM_SUPPLIES, ak7375_dev->supplies);
> + if (ret)
> + return ret;
> +
> + /* Wait for vcm to become ready */
> + usleep_range(AK7375_POWER_DELAY_US, AK7375_POWER_DELAY_US + 500);
> +
> ret = ak7375_i2c_write(ak7375_dev, AK7375_REG_CONT,
> AK7375_MODE_ACTIVE, 1);
> if (ret) {
> --
> 2.37.0
>
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [PATCH v2 3/3] media: i2c: ak7375: Add regulator management
2022-07-13 7:39 ` Jacopo Mondi
@ 2022-07-14 14:06 ` Yassine Oudjana
2022-07-14 15:58 ` Jacopo Mondi
0 siblings, 1 reply; 17+ messages in thread
From: Yassine Oudjana @ 2022-07-14 14:06 UTC (permalink / raw)
To: Jacopo Mondi
Cc: Lee Jackson, Mauro Carvalho Chehab, Rob Herring,
Krzysztof Kozlowski, Tianshu Qiu, Bingbu Cao, Yassine Oudjana,
linux-media, devicetree, linux-kernel
On Wed, Jul 13 2022 at 09:39:51 +0200, Jacopo Mondi <jacopo@jmondi.org>
wrote:
> Hi Yassine
>
> On Mon, Jul 11, 2022 at 06:40:39PM +0400, Yassine Oudjana wrote:
>> From: Yassine Oudjana <y.oudjana@protonmail.com>
>>
>> Make the driver get needed regulators on probe and enable/disable
>> them on runtime PM callbacks.
>>
>> Signed-off-by: Yassine Oudjana <y.oudjana@protonmail.com>
>
> Have you seen this ?
> https://github.com/ArduCAM/IMX519_AK7375/blob/main/AK7375/0002-media-i2c-ak7375-driver-add-optional-regulator-suppo.patch#L172
>
> It claims
> + * Initialisation delay between VDD low->high and the moment
> + * when the i2c command is available.
> + * From the datasheet, it should be 10ms + 2ms (max power
> + * up sequence duration)
>
> 10ms seems like a long time, it would be nice to have the datasheet to
> cross-check.
It does seem quite long. I couldn't find a datasheet anywhere
so the value I discovered is the best I have. I've added the
author of that patch to CC; maybe they have some info to
contribute.
>
> Thanks
> j
>
>> ---
>> Changes since v1:
>> - Reorganize variable declaration
>> - Change the power-on delay range to 3000-3500 microseconds.
>>
>> drivers/media/i2c/ak7375.c | 39
>> ++++++++++++++++++++++++++++++++++++++
>> 1 file changed, 39 insertions(+)
>>
>> diff --git a/drivers/media/i2c/ak7375.c b/drivers/media/i2c/ak7375.c
>> index 40b1a4aa846c..c2b2542a0056 100644
>> --- a/drivers/media/i2c/ak7375.c
>> +++ b/drivers/media/i2c/ak7375.c
>> @@ -6,6 +6,7 @@
>> #include <linux/i2c.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>
>>
>> @@ -23,17 +24,32 @@
>> */
>> #define AK7375_CTRL_STEPS 64
>> #define AK7375_CTRL_DELAY_US 1000
>> +/*
>> + * The vcm takes around 3 ms to power on and start taking
>> + * I2C messages. This value was found experimentally due to
>> + * lack of documentation.
>> + */
>> +#define AK7375_POWER_DELAY_US 3000
>>
>> #define AK7375_REG_POSITION 0x0
>> #define AK7375_REG_CONT 0x2
>> #define AK7375_MODE_ACTIVE 0x0
>> #define AK7375_MODE_STANDBY 0x40
>>
>> +static const char * const ak7375_supply_names[] = {
>> + "vdd",
>> + "vio",
>> +};
>> +
>> +#define AK7375_NUM_SUPPLIES ARRAY_SIZE(ak7375_supply_names)
>> +
>> /* ak7375 device structure */
>> struct ak7375_device {
>> struct v4l2_ctrl_handler ctrls_vcm;
>> struct v4l2_subdev sd;
>> struct v4l2_ctrl *focus;
>> + struct regulator_bulk_data supplies[AK7375_NUM_SUPPLIES];
>> +
>> /* active or standby mode */
>> bool active;
>> };
>> @@ -133,12 +149,24 @@ static int ak7375_probe(struct i2c_client
>> *client)
>> {
>> struct ak7375_device *ak7375_dev;
>> int ret;
>> + int i;
>>
>> ak7375_dev = devm_kzalloc(&client->dev, sizeof(*ak7375_dev),
>> GFP_KERNEL);
>> if (!ak7375_dev)
>> return -ENOMEM;
>>
>> + for (i = 0; i < AK7375_NUM_SUPPLIES; i++)
>> + ak7375_dev->supplies[i].supply = ak7375_supply_names[i];
>> +
>> + ret = devm_regulator_bulk_get(&client->dev, AK7375_NUM_SUPPLIES,
>> + ak7375_dev->supplies);
>> + if (ret) {
>> + dev_err(&client->dev, "Failed to get regulators: %pe",
>> + ERR_PTR(ret));
>> + return ret;
>> + }
>> +
>> v4l2_i2c_subdev_init(&ak7375_dev->sd, client, &ak7375_ops);
>> ak7375_dev->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
>> ak7375_dev->sd.internal_ops = &ak7375_int_ops;
>> @@ -210,6 +238,10 @@ static int __maybe_unused
>> ak7375_vcm_suspend(struct device *dev)
>> if (ret)
>> dev_err(dev, "%s I2C failure: %d\n", __func__, ret);
>>
>> + ret = regulator_bulk_disable(AK7375_NUM_SUPPLIES,
>> ak7375_dev->supplies);
>> + if (ret)
>> + return ret;
>> +
>> ak7375_dev->active = false;
>>
>> return 0;
>> @@ -230,6 +262,13 @@ static int __maybe_unused
>> ak7375_vcm_resume(struct device *dev)
>> if (ak7375_dev->active)
>> return 0;
>>
>> + ret = regulator_bulk_enable(AK7375_NUM_SUPPLIES,
>> ak7375_dev->supplies);
>> + if (ret)
>> + return ret;
>> +
>> + /* Wait for vcm to become ready */
>> + usleep_range(AK7375_POWER_DELAY_US, AK7375_POWER_DELAY_US + 500);
>> +
>> ret = ak7375_i2c_write(ak7375_dev, AK7375_REG_CONT,
>> AK7375_MODE_ACTIVE, 1);
>> if (ret) {
>> --
>> 2.37.0
>>
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [PATCH v2 3/3] media: i2c: ak7375: Add regulator management
2022-07-14 14:06 ` Yassine Oudjana
@ 2022-07-14 15:58 ` Jacopo Mondi
2022-07-20 2:58 ` Lee Jackson
0 siblings, 1 reply; 17+ messages in thread
From: Jacopo Mondi @ 2022-07-14 15:58 UTC (permalink / raw)
To: Yassine Oudjana
Cc: Lee Jackson, Mauro Carvalho Chehab, Rob Herring,
Krzysztof Kozlowski, Tianshu Qiu, Bingbu Cao, Yassine Oudjana,
linux-media, devicetree, linux-kernel
Hello Yassine
On Thu, Jul 14, 2022 at 06:06:32PM +0400, Yassine Oudjana wrote:
>
> On Wed, Jul 13 2022 at 09:39:51 +0200, Jacopo Mondi <jacopo@jmondi.org>
> wrote:
> > Hi Yassine
> >
> > On Mon, Jul 11, 2022 at 06:40:39PM +0400, Yassine Oudjana wrote:
> > > From: Yassine Oudjana <y.oudjana@protonmail.com>
> > >
> > > Make the driver get needed regulators on probe and enable/disable
> > > them on runtime PM callbacks.
> > >
> > > Signed-off-by: Yassine Oudjana <y.oudjana@protonmail.com>
> >
> > Have you seen this ?
> > https://github.com/ArduCAM/IMX519_AK7375/blob/main/AK7375/0002-media-i2c-ak7375-driver-add-optional-regulator-suppo.patch#L172
> >
> > It claims
> > + * Initialisation delay between VDD low->high and the moment
> > + * when the i2c command is available.
> > + * From the datasheet, it should be 10ms + 2ms (max power
> > + * up sequence duration)
> >
> > 10ms seems like a long time, it would be nice to have the datasheet to
> > cross-check.
>
> It does seem quite long. I couldn't find a datasheet anywhere
> so the value I discovered is the best I have. I've added the
> author of that patch to CC; maybe they have some info to
> contribute.
>
I have now tested these patches with an Arducam IMX519 camera.
Using a 3msec delay I get failures in the establishing i2c
communications (I only tested 2 times though).
With 10milliseconds (which I concur is a lot) I get stable results.
Let's see if we can get more info from who has the manual.
Thanks
j
> >
> > Thanks
> > j
> >
> > > ---
> > > Changes since v1:
> > > - Reorganize variable declaration
> > > - Change the power-on delay range to 3000-3500 microseconds.
> > >
> > > drivers/media/i2c/ak7375.c | 39
> > > ++++++++++++++++++++++++++++++++++++++
> > > 1 file changed, 39 insertions(+)
> > >
> > > diff --git a/drivers/media/i2c/ak7375.c b/drivers/media/i2c/ak7375.c
> > > index 40b1a4aa846c..c2b2542a0056 100644
> > > --- a/drivers/media/i2c/ak7375.c
> > > +++ b/drivers/media/i2c/ak7375.c
> > > @@ -6,6 +6,7 @@
> > > #include <linux/i2c.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>
> > >
> > > @@ -23,17 +24,32 @@
> > > */
> > > #define AK7375_CTRL_STEPS 64
> > > #define AK7375_CTRL_DELAY_US 1000
> > > +/*
> > > + * The vcm takes around 3 ms to power on and start taking
> > > + * I2C messages. This value was found experimentally due to
> > > + * lack of documentation.
> > > + */
> > > +#define AK7375_POWER_DELAY_US 3000
> > >
> > > #define AK7375_REG_POSITION 0x0
> > > #define AK7375_REG_CONT 0x2
> > > #define AK7375_MODE_ACTIVE 0x0
> > > #define AK7375_MODE_STANDBY 0x40
> > >
> > > +static const char * const ak7375_supply_names[] = {
> > > + "vdd",
> > > + "vio",
> > > +};
> > > +
> > > +#define AK7375_NUM_SUPPLIES ARRAY_SIZE(ak7375_supply_names)
> > > +
> > > /* ak7375 device structure */
> > > struct ak7375_device {
> > > struct v4l2_ctrl_handler ctrls_vcm;
> > > struct v4l2_subdev sd;
> > > struct v4l2_ctrl *focus;
> > > + struct regulator_bulk_data supplies[AK7375_NUM_SUPPLIES];
> > > +
> > > /* active or standby mode */
> > > bool active;
> > > };
> > > @@ -133,12 +149,24 @@ static int ak7375_probe(struct i2c_client
> > > *client)
> > > {
> > > struct ak7375_device *ak7375_dev;
> > > int ret;
> > > + int i;
> > >
> > > ak7375_dev = devm_kzalloc(&client->dev, sizeof(*ak7375_dev),
> > > GFP_KERNEL);
> > > if (!ak7375_dev)
> > > return -ENOMEM;
> > >
> > > + for (i = 0; i < AK7375_NUM_SUPPLIES; i++)
> > > + ak7375_dev->supplies[i].supply = ak7375_supply_names[i];
> > > +
> > > + ret = devm_regulator_bulk_get(&client->dev, AK7375_NUM_SUPPLIES,
> > > + ak7375_dev->supplies);
> > > + if (ret) {
> > > + dev_err(&client->dev, "Failed to get regulators: %pe",
> > > + ERR_PTR(ret));
> > > + return ret;
> > > + }
> > > +
> > > v4l2_i2c_subdev_init(&ak7375_dev->sd, client, &ak7375_ops);
> > > ak7375_dev->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
> > > ak7375_dev->sd.internal_ops = &ak7375_int_ops;
> > > @@ -210,6 +238,10 @@ static int __maybe_unused
> > > ak7375_vcm_suspend(struct device *dev)
> > > if (ret)
> > > dev_err(dev, "%s I2C failure: %d\n", __func__, ret);
> > >
> > > + ret = regulator_bulk_disable(AK7375_NUM_SUPPLIES,
> > > ak7375_dev->supplies);
> > > + if (ret)
> > > + return ret;
> > > +
> > > ak7375_dev->active = false;
> > >
> > > return 0;
> > > @@ -230,6 +262,13 @@ static int __maybe_unused
> > > ak7375_vcm_resume(struct device *dev)
> > > if (ak7375_dev->active)
> > > return 0;
> > >
> > > + ret = regulator_bulk_enable(AK7375_NUM_SUPPLIES,
> > > ak7375_dev->supplies);
> > > + if (ret)
> > > + return ret;
> > > +
> > > + /* Wait for vcm to become ready */
> > > + usleep_range(AK7375_POWER_DELAY_US, AK7375_POWER_DELAY_US + 500);
> > > +
> > > ret = ak7375_i2c_write(ak7375_dev, AK7375_REG_CONT,
> > > AK7375_MODE_ACTIVE, 1);
> > > if (ret) {
> > > --
> > > 2.37.0
> > >
>
>
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [PATCH v2 3/3] media: i2c: ak7375: Add regulator management
2022-07-14 15:58 ` Jacopo Mondi
@ 2022-07-20 2:58 ` Lee Jackson
0 siblings, 0 replies; 17+ messages in thread
From: Lee Jackson @ 2022-07-20 2:58 UTC (permalink / raw)
To: jacopo
Cc: bingbu.cao, devicetree, info, krzysztof.kozlowski+dt,
linux-kernel, linux-media, mchehab, robh+dt, tian.shu.qiu,
y.oudjana, yassine.oudjana
Hello All,
This is Lee Jackson from Arducam, please check the reply following.
On Thu, 14 Jul 2022 17:58:31 +0200
Jacopo Mondi <jacopo@jmondi.org> wrote:
> Hello Yassine
>
> On Thu, Jul 14, 2022 at 06:06:32PM +0400, Yassine Oudjana wrote:
> >
> > On Wed, Jul 13 2022 at 09:39:51 +0200, Jacopo Mondi
> > <jacopo@jmondi.org> wrote:
> > > Hi Yassine
> > >
> > > On Mon, Jul 11, 2022 at 06:40:39PM +0400, Yassine Oudjana wrote:
> > > > From: Yassine Oudjana <y.oudjana@protonmail.com>
> > > >
> > > > Make the driver get needed regulators on probe and
> > > > enable/disable them on runtime PM callbacks.
> > > >
> > > > Signed-off-by: Yassine Oudjana <y.oudjana@protonmail.com>
> > >
> > > Have you seen this ?
> > > https://github.com/ArduCAM/IMX519_AK7375/blob/main/AK7375/0002-media-i2c-ak7375-driver-add-optional-regulator-suppo.patch#L172
> > >
> > > It claims
> > > + * Initialisation delay between VDD low->high and the
> > > moment
> > > + * when the i2c command is available.
> > > + * From the datasheet, it should be 10ms + 2ms (max power
> > > + * up sequence duration)
> > >
> > > 10ms seems like a long time, it would be nice to have the
> > > datasheet to cross-check.
> >
> > It does seem quite long. I couldn't find a datasheet anywhere
> > so the value I discovered is the best I have. I've added the
> > author of that patch to CC; maybe they have some info to
> > contribute.
> >
>
> I have now tested these patches with an Arducam IMX519 camera.
> Using a 3msec delay I get failures in the establishing i2c
> communications (I only tested 2 times though).
>
> With 10milliseconds (which I concur is a lot) I get stable results.
> Let's see if we can get more info from who has the manual.
>
> Thanks
> j
>
We have the datasheet for the ak7371 and as far as we know they are not
much different, here is the power up sequence for the ak7371:
https://www.arducam.com/wp-content/uploads/2022/07/ak7371-power-sequence.png
(We're not very familiar with working on the linux kernel mailing list
yet, that's one reason we didn't submit ak7375patch, please let me know
if I'm doing something wrong.)
> > >
> > > Thanks
> > > j
> > >
> > > > ---
> > > > Changes since v1:
> > > > - Reorganize variable declaration
> > > > - Change the power-on delay range to 3000-3500 microseconds.
> > > >
> > > > drivers/media/i2c/ak7375.c | 39
> > > > ++++++++++++++++++++++++++++++++++++++
> > > > 1 file changed, 39 insertions(+)
> > > >
> > > > diff --git a/drivers/media/i2c/ak7375.c
> > > > b/drivers/media/i2c/ak7375.c index 40b1a4aa846c..c2b2542a0056
> > > > 100644 --- a/drivers/media/i2c/ak7375.c
> > > > +++ b/drivers/media/i2c/ak7375.c
> > > > @@ -6,6 +6,7 @@
> > > > #include <linux/i2c.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>
> > > >
> > > > @@ -23,17 +24,32 @@
> > > > */
> > > > #define AK7375_CTRL_STEPS 64
> > > > #define AK7375_CTRL_DELAY_US 1000
> > > > +/*
> > > > + * The vcm takes around 3 ms to power on and start taking
> > > > + * I2C messages. This value was found experimentally due to
> > > > + * lack of documentation.
> > > > + */
> > > > +#define AK7375_POWER_DELAY_US 3000
> > > >
> > > > #define AK7375_REG_POSITION 0x0
> > > > #define AK7375_REG_CONT 0x2
> > > > #define AK7375_MODE_ACTIVE 0x0
> > > > #define AK7375_MODE_STANDBY 0x40
> > > >
> > > > +static const char * const ak7375_supply_names[] = {
> > > > + "vdd",
> > > > + "vio",
> > > > +};
> > > > +
> > > > +#define AK7375_NUM_SUPPLIES ARRAY_SIZE(ak7375_supply_names)
> > > > +
> > > > /* ak7375 device structure */
> > > > struct ak7375_device {
> > > > struct v4l2_ctrl_handler ctrls_vcm;
> > > > struct v4l2_subdev sd;
> > > > struct v4l2_ctrl *focus;
> > > > + struct regulator_bulk_data
> > > > supplies[AK7375_NUM_SUPPLIES]; +
> > > > /* active or standby mode */
> > > > bool active;
> > > > };
> > > > @@ -133,12 +149,24 @@ static int ak7375_probe(struct i2c_client
> > > > *client)
> > > > {
> > > > struct ak7375_device *ak7375_dev;
> > > > int ret;
> > > > + int i;
> > > >
> > > > ak7375_dev = devm_kzalloc(&client->dev,
> > > > sizeof(*ak7375_dev), GFP_KERNEL);
> > > > if (!ak7375_dev)
> > > > return -ENOMEM;
> > > >
> > > > + for (i = 0; i < AK7375_NUM_SUPPLIES; i++)
> > > > + ak7375_dev->supplies[i].supply =
> > > > ak7375_supply_names[i]; +
> > > > + ret = devm_regulator_bulk_get(&client->dev,
> > > > AK7375_NUM_SUPPLIES,
> > > > + ak7375_dev->supplies);
> > > > + if (ret) {
> > > > + dev_err(&client->dev, "Failed to get
> > > > regulators: %pe",
> > > > + ERR_PTR(ret));
> > > > + return ret;
> > > > + }
> > > > +
> > > > v4l2_i2c_subdev_init(&ak7375_dev->sd, client,
> > > > &ak7375_ops); ak7375_dev->sd.flags |=
> > > > V4L2_SUBDEV_FL_HAS_DEVNODE; ak7375_dev->sd.internal_ops =
> > > > &ak7375_int_ops; @@ -210,6 +238,10 @@ static int __maybe_unused
> > > > ak7375_vcm_suspend(struct device *dev)
> > > > if (ret)
> > > > dev_err(dev, "%s I2C failure: %d\n",
> > > > __func__, ret);
> > > >
> > > > + ret = regulator_bulk_disable(AK7375_NUM_SUPPLIES,
> > > > ak7375_dev->supplies);
> > > > + if (ret)
> > > > + return ret;
> > > > +
> > > > ak7375_dev->active = false;
> > > >
> > > > return 0;
> > > > @@ -230,6 +262,13 @@ static int __maybe_unused
> > > > ak7375_vcm_resume(struct device *dev)
> > > > if (ak7375_dev->active)
> > > > return 0;
> > > >
> > > > + ret = regulator_bulk_enable(AK7375_NUM_SUPPLIES,
> > > > ak7375_dev->supplies);
> > > > + if (ret)
> > > > + return ret;
> > > > +
> > > > + /* Wait for vcm to become ready */
> > > > + usleep_range(AK7375_POWER_DELAY_US,
> > > > AK7375_POWER_DELAY_US + 500); +
> > > > ret = ak7375_i2c_write(ak7375_dev, AK7375_REG_CONT,
> > > > AK7375_MODE_ACTIVE, 1);
> > > > if (ret) {
> > > > --
> > > > 2.37.0
> > > >
> >
> >
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2 3/3] media: i2c: ak7375: Add regulator management
2022-07-11 14:40 ` [PATCH v2 3/3] media: i2c: ak7375: Add regulator management Yassine Oudjana
2022-07-11 17:31 ` Jacopo Mondi
2022-07-13 7:39 ` Jacopo Mondi
@ 2022-07-23 11:33 ` Sakari Ailus
2022-11-28 6:47 ` Umang Jain
3 siblings, 0 replies; 17+ messages in thread
From: Sakari Ailus @ 2022-07-23 11:33 UTC (permalink / raw)
To: Yassine Oudjana
Cc: Mauro Carvalho Chehab, Rob Herring, Krzysztof Kozlowski,
Tianshu Qiu, Bingbu Cao, Yassine Oudjana, Jacopo Mondi,
linux-media, devicetree, linux-kernel
Hi Yassine,
Thanks for the patch.
On Mon, Jul 11, 2022 at 06:40:39PM +0400, Yassine Oudjana wrote:
> From: Yassine Oudjana <y.oudjana@protonmail.com>
>
> Make the driver get needed regulators on probe and enable/disable
> them on runtime PM callbacks.
>
> Signed-off-by: Yassine Oudjana <y.oudjana@protonmail.com>
> ---
> Changes since v1:
> - Reorganize variable declaration
> - Change the power-on delay range to 3000-3500 microseconds.
>
> drivers/media/i2c/ak7375.c | 39 ++++++++++++++++++++++++++++++++++++++
> 1 file changed, 39 insertions(+)
>
> diff --git a/drivers/media/i2c/ak7375.c b/drivers/media/i2c/ak7375.c
> index 40b1a4aa846c..c2b2542a0056 100644
> --- a/drivers/media/i2c/ak7375.c
> +++ b/drivers/media/i2c/ak7375.c
> @@ -6,6 +6,7 @@
> #include <linux/i2c.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>
>
> @@ -23,17 +24,32 @@
> */
> #define AK7375_CTRL_STEPS 64
> #define AK7375_CTRL_DELAY_US 1000
> +/*
> + * The vcm takes around 3 ms to power on and start taking
> + * I2C messages. This value was found experimentally due to
> + * lack of documentation.
> + */
> +#define AK7375_POWER_DELAY_US 3000
>
> #define AK7375_REG_POSITION 0x0
> #define AK7375_REG_CONT 0x2
> #define AK7375_MODE_ACTIVE 0x0
> #define AK7375_MODE_STANDBY 0x40
>
> +static const char * const ak7375_supply_names[] = {
> + "vdd",
> + "vio",
> +};
> +
> +#define AK7375_NUM_SUPPLIES ARRAY_SIZE(ak7375_supply_names)
Please drop this, and use ARRAY_SIZE() directly.
> +
> /* ak7375 device structure */
> struct ak7375_device {
> struct v4l2_ctrl_handler ctrls_vcm;
> struct v4l2_subdev sd;
> struct v4l2_ctrl *focus;
> + struct regulator_bulk_data supplies[AK7375_NUM_SUPPLIES];
> +
> /* active or standby mode */
> bool active;
> };
> @@ -133,12 +149,24 @@ static int ak7375_probe(struct i2c_client *client)
> {
> struct ak7375_device *ak7375_dev;
> int ret;
> + int i;
unsigned int, please.
--
Kind regards,
Sakari Ailus
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [PATCH v2 3/3] media: i2c: ak7375: Add regulator management
2022-07-11 14:40 ` [PATCH v2 3/3] media: i2c: ak7375: Add regulator management Yassine Oudjana
` (2 preceding siblings ...)
2022-07-23 11:33 ` Sakari Ailus
@ 2022-11-28 6:47 ` Umang Jain
3 siblings, 0 replies; 17+ messages in thread
From: Umang Jain @ 2022-11-28 6:47 UTC (permalink / raw)
To: Yassine Oudjana, Mauro Carvalho Chehab, Rob Herring,
Krzysztof Kozlowski, Tianshu Qiu, Bingbu Cao
Cc: Yassine Oudjana, Jacopo Mondi, linux-media, devicetree, linux-kernel
Hi Yassine,
I have tested this VCM with autofocus algorithm plumbed in libcamera [1]
On 7/11/22 8:10 PM, Yassine Oudjana wrote:
> From: Yassine Oudjana <y.oudjana@protonmail.com>
>
> Make the driver get needed regulators on probe and enable/disable
> them on runtime PM callbacks.
>
> Signed-off-by: Yassine Oudjana <y.oudjana@protonmail.com>
Tested-by: Umang Jain <umang.jain@ideasonboard.com>
[1] https://patchwork.libcamera.org/project/libcamera/list/?series=3174
> ---
> Changes since v1:
> - Reorganize variable declaration
> - Change the power-on delay range to 3000-3500 microseconds.
>
> drivers/media/i2c/ak7375.c | 39 ++++++++++++++++++++++++++++++++++++++
> 1 file changed, 39 insertions(+)
>
> diff --git a/drivers/media/i2c/ak7375.c b/drivers/media/i2c/ak7375.c
> index 40b1a4aa846c..c2b2542a0056 100644
> --- a/drivers/media/i2c/ak7375.c
> +++ b/drivers/media/i2c/ak7375.c
> @@ -6,6 +6,7 @@
> #include <linux/i2c.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>
>
> @@ -23,17 +24,32 @@
> */
> #define AK7375_CTRL_STEPS 64
> #define AK7375_CTRL_DELAY_US 1000
> +/*
> + * The vcm takes around 3 ms to power on and start taking
> + * I2C messages. This value was found experimentally due to
> + * lack of documentation.
> + */
> +#define AK7375_POWER_DELAY_US 3000
>
> #define AK7375_REG_POSITION 0x0
> #define AK7375_REG_CONT 0x2
> #define AK7375_MODE_ACTIVE 0x0
> #define AK7375_MODE_STANDBY 0x40
>
> +static const char * const ak7375_supply_names[] = {
> + "vdd",
> + "vio",
> +};
> +
> +#define AK7375_NUM_SUPPLIES ARRAY_SIZE(ak7375_supply_names)
> +
> /* ak7375 device structure */
> struct ak7375_device {
> struct v4l2_ctrl_handler ctrls_vcm;
> struct v4l2_subdev sd;
> struct v4l2_ctrl *focus;
> + struct regulator_bulk_data supplies[AK7375_NUM_SUPPLIES];
> +
> /* active or standby mode */
> bool active;
> };
> @@ -133,12 +149,24 @@ static int ak7375_probe(struct i2c_client *client)
> {
> struct ak7375_device *ak7375_dev;
> int ret;
> + int i;
>
> ak7375_dev = devm_kzalloc(&client->dev, sizeof(*ak7375_dev),
> GFP_KERNEL);
> if (!ak7375_dev)
> return -ENOMEM;
>
> + for (i = 0; i < AK7375_NUM_SUPPLIES; i++)
> + ak7375_dev->supplies[i].supply = ak7375_supply_names[i];
> +
> + ret = devm_regulator_bulk_get(&client->dev, AK7375_NUM_SUPPLIES,
> + ak7375_dev->supplies);
> + if (ret) {
> + dev_err(&client->dev, "Failed to get regulators: %pe",
> + ERR_PTR(ret));
> + return ret;
> + }
> +
> v4l2_i2c_subdev_init(&ak7375_dev->sd, client, &ak7375_ops);
> ak7375_dev->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
> ak7375_dev->sd.internal_ops = &ak7375_int_ops;
> @@ -210,6 +238,10 @@ static int __maybe_unused ak7375_vcm_suspend(struct device *dev)
> if (ret)
> dev_err(dev, "%s I2C failure: %d\n", __func__, ret);
>
> + ret = regulator_bulk_disable(AK7375_NUM_SUPPLIES, ak7375_dev->supplies);
> + if (ret)
> + return ret;
> +
> ak7375_dev->active = false;
>
> return 0;
> @@ -230,6 +262,13 @@ static int __maybe_unused ak7375_vcm_resume(struct device *dev)
> if (ak7375_dev->active)
> return 0;
>
> + ret = regulator_bulk_enable(AK7375_NUM_SUPPLIES, ak7375_dev->supplies);
> + if (ret)
> + return ret;
> +
> + /* Wait for vcm to become ready */
> + usleep_range(AK7375_POWER_DELAY_US, AK7375_POWER_DELAY_US + 500);
> +
> ret = ak7375_i2c_write(ak7375_dev, AK7375_REG_CONT,
> AK7375_MODE_ACTIVE, 1);
> if (ret) {
>
^ permalink raw reply [flat|nested] 17+ messages in thread
