* [PATCH 0/3] media: i2c: ak7375: Add regulator management
@ 2022-07-11 4:28 Yassine Oudjana
2022-07-11 4:28 ` [PATCH 1/3] media: dt-bindings: ak7375: Convert to DT schema Yassine Oudjana
` (2 more replies)
0 siblings, 3 replies; 9+ messages in thread
From: Yassine Oudjana @ 2022-07-11 4:28 UTC (permalink / raw)
To: Mauro Carvalho Chehab, Rob Herring, Krzysztof Kozlowski,
Tianshu Qiu, Bingbu Cao
Cc: Yassine Oudjana, Yassine Oudjana, linux-media, devicetree, linux-kernel
From: Yassine Oudjana <y.oudjana@protonmail.com>
This series adds needed regulator management for the AK7375 VCM.
A DT schema conversion is made before adding new properties.
Yassine Oudjana (3):
media: dt-bindings: ak7375: Convert to DT schema
media: dt-bindings: ak7375: Add supplies
media: i2c: ak7375: Add regulator management
.../devicetree/bindings/media/i2c/ak7375.txt | 8 ---
.../devicetree/bindings/media/i2c/ak7375.yaml | 52 +++++++++++++++++++
MAINTAINERS | 2 +-
drivers/media/i2c/ak7375.c | 39 ++++++++++++++
4 files changed, 92 insertions(+), 9 deletions(-)
delete mode 100644 Documentation/devicetree/bindings/media/i2c/ak7375.txt
create mode 100644 Documentation/devicetree/bindings/media/i2c/ak7375.yaml
--
2.37.0
^ permalink raw reply [flat|nested] 9+ messages in thread* [PATCH 1/3] media: dt-bindings: ak7375: Convert to DT schema 2022-07-11 4:28 [PATCH 0/3] media: i2c: ak7375: Add regulator management Yassine Oudjana @ 2022-07-11 4:28 ` Yassine Oudjana 2022-07-11 11:42 ` Krzysztof Kozlowski 2022-07-11 4:28 ` [PATCH 2/3] media: dt-bindings: ak7375: Add supplies Yassine Oudjana 2022-07-11 4:28 ` [PATCH 3/3] media: i2c: ak7375: Add regulator management Yassine Oudjana 2 siblings, 1 reply; 9+ messages in thread From: Yassine Oudjana @ 2022-07-11 4:28 UTC (permalink / raw) To: Mauro Carvalho Chehab, Rob Herring, Krzysztof Kozlowski, Tianshu Qiu, Bingbu Cao Cc: Yassine Oudjana, Yassine Oudjana, linux-media, devicetree, linux-kernel From: Yassine Oudjana <y.oudjana@protonmail.com> Convert DT bindings document for AKM AK7375 VCM to DT schema format and add an example. Signed-off-by: Yassine Oudjana <y.oudjana@protonmail.com> --- .../devicetree/bindings/media/i2c/ak7375.txt | 8 ---- .../devicetree/bindings/media/i2c/ak7375.yaml | 41 +++++++++++++++++++ MAINTAINERS | 2 +- 3 files changed, 42 insertions(+), 9 deletions(-) delete mode 100644 Documentation/devicetree/bindings/media/i2c/ak7375.txt create mode 100644 Documentation/devicetree/bindings/media/i2c/ak7375.yaml diff --git a/Documentation/devicetree/bindings/media/i2c/ak7375.txt b/Documentation/devicetree/bindings/media/i2c/ak7375.txt deleted file mode 100644 index aa3e24b41241..000000000000 --- a/Documentation/devicetree/bindings/media/i2c/ak7375.txt +++ /dev/null @@ -1,8 +0,0 @@ -Asahi Kasei Microdevices AK7375 voice coil lens driver - -AK7375 is a camera voice coil lens. - -Mandatory properties: - -- compatible: "asahi-kasei,ak7375" -- reg: I2C slave address diff --git a/Documentation/devicetree/bindings/media/i2c/ak7375.yaml b/Documentation/devicetree/bindings/media/i2c/ak7375.yaml new file mode 100644 index 000000000000..4fc216846ae7 --- /dev/null +++ b/Documentation/devicetree/bindings/media/i2c/ak7375.yaml @@ -0,0 +1,41 @@ +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/media/i2c/ak7375.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Asahi Kasei Microdevices AK7375 voice coil lens actuator + +maintainers: + - Tianshu Qiu <tian.shu.qiu@intel.com> + +description: + AK7375 is a voice coil motor (VCM) camera lens actuator that + is controlled over I2C. + +properties: + compatible: + const: asahi-kasei,ak7375 + + reg: + maxItems: 1 + +required: + - compatible + - reg + +additionalProperties: false + +examples: + - | + i2c { + #address-cells = <1>; + #size-cells = <0>; + + ak7375: camera-lens@c { + compatible = "asahi-kasei,ak7375"; + reg = <0x0c>; + }; + }; + +... diff --git a/MAINTAINERS b/MAINTAINERS index a97fef8c131d..19faf9ce8258 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -3063,7 +3063,7 @@ M: Tianshu Qiu <tian.shu.qiu@intel.com> L: linux-media@vger.kernel.org S: Maintained T: git git://linuxtv.org/media_tree.git -F: Documentation/devicetree/bindings/media/i2c/ak7375.txt +F: Documentation/devicetree/bindings/media/i2c/ak7375.yaml F: drivers/media/i2c/ak7375.c ASAHI KASEI AK8974 DRIVER -- 2.37.0 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH 1/3] media: dt-bindings: ak7375: Convert to DT schema 2022-07-11 4:28 ` [PATCH 1/3] media: dt-bindings: ak7375: Convert to DT schema Yassine Oudjana @ 2022-07-11 11:42 ` Krzysztof Kozlowski 0 siblings, 0 replies; 9+ messages in thread From: Krzysztof Kozlowski @ 2022-07-11 11:42 UTC (permalink / raw) To: Yassine Oudjana, Mauro Carvalho Chehab, Rob Herring, Krzysztof Kozlowski, Tianshu Qiu, Bingbu Cao Cc: Yassine Oudjana, linux-media, devicetree, linux-kernel On 11/07/2022 06:28, Yassine Oudjana wrote: > From: Yassine Oudjana <y.oudjana@protonmail.com> > > Convert DT bindings document for AKM AK7375 VCM to DT schema > format and add an example. > > Signed-off-by: Yassine Oudjana <y.oudjana@protonmail.com> > --- > .../devicetree/bindings/media/i2c/ak7375.txt | 8 ---- > .../devicetree/bindings/media/i2c/ak7375.yaml | 41 +++++++++++++++++++ > MAINTAINERS | 2 +- > 3 files changed, 42 insertions(+), 9 deletions(-) > delete mode 100644 Documentation/devicetree/bindings/media/i2c/ak7375.txt > create mode 100644 Documentation/devicetree/bindings/media/i2c/ak7375.yaml > > diff --git a/Documentation/devicetree/bindings/media/i2c/ak7375.txt b/Documentation/devicetree/bindings/media/i2c/ak7375.txt > deleted file mode 100644 > index aa3e24b41241..000000000000 > --- a/Documentation/devicetree/bindings/media/i2c/ak7375.txt > +++ /dev/null > @@ -1,8 +0,0 @@ > -Asahi Kasei Microdevices AK7375 voice coil lens driver > - > -AK7375 is a camera voice coil lens. > - > -Mandatory properties: > - > -- compatible: "asahi-kasei,ak7375" > -- reg: I2C slave address > diff --git a/Documentation/devicetree/bindings/media/i2c/ak7375.yaml b/Documentation/devicetree/bindings/media/i2c/ak7375.yaml > new file mode 100644 > index 000000000000..4fc216846ae7 > --- /dev/null > +++ b/Documentation/devicetree/bindings/media/i2c/ak7375.yaml > @@ -0,0 +1,41 @@ > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/media/i2c/ak7375.yaml# Include vendor prefix in the file name, so: asahi-kasei,ak7375.yaml Best regards, Krzysztof ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH 2/3] media: dt-bindings: ak7375: Add supplies 2022-07-11 4:28 [PATCH 0/3] media: i2c: ak7375: Add regulator management Yassine Oudjana 2022-07-11 4:28 ` [PATCH 1/3] media: dt-bindings: ak7375: Convert to DT schema Yassine Oudjana @ 2022-07-11 4:28 ` Yassine Oudjana 2022-07-11 11:42 ` Krzysztof Kozlowski 2022-07-11 4:28 ` [PATCH 3/3] media: i2c: ak7375: Add regulator management Yassine Oudjana 2 siblings, 1 reply; 9+ messages in thread From: Yassine Oudjana @ 2022-07-11 4:28 UTC (permalink / raw) To: Mauro Carvalho Chehab, Rob Herring, Krzysztof Kozlowski, Tianshu Qiu, Bingbu Cao Cc: Yassine Oudjana, Yassine Oudjana, linux-media, devicetree, linux-kernel From: Yassine Oudjana <y.oudjana@protonmail.com> Add supply properties to describe regulators needed to power the AK7375 VCM. Signed-off-by: Yassine Oudjana <y.oudjana@protonmail.com> --- .../devicetree/bindings/media/i2c/ak7375.yaml | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/Documentation/devicetree/bindings/media/i2c/ak7375.yaml b/Documentation/devicetree/bindings/media/i2c/ak7375.yaml index 4fc216846ae7..baa91de55927 100644 --- a/Documentation/devicetree/bindings/media/i2c/ak7375.yaml +++ b/Documentation/devicetree/bindings/media/i2c/ak7375.yaml @@ -20,9 +20,17 @@ properties: reg: maxItems: 1 + vdd-supply: + description: VDD supply + + vio-supply: + description: I/O pull-up supply + required: - compatible - reg + - vdd-supply + - vio-supply additionalProperties: false @@ -35,6 +43,9 @@ examples: ak7375: camera-lens@c { compatible = "asahi-kasei,ak7375"; reg = <0x0c>; + + vdd-supply = <&vreg_l23a_2p8>; + vio-supply = <&vreg_lvs1a_1p8>; }; }; -- 2.37.0 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH 2/3] media: dt-bindings: ak7375: Add supplies 2022-07-11 4:28 ` [PATCH 2/3] media: dt-bindings: ak7375: Add supplies Yassine Oudjana @ 2022-07-11 11:42 ` Krzysztof Kozlowski 0 siblings, 0 replies; 9+ messages in thread From: Krzysztof Kozlowski @ 2022-07-11 11:42 UTC (permalink / raw) To: Yassine Oudjana, Mauro Carvalho Chehab, Rob Herring, Krzysztof Kozlowski, Tianshu Qiu, Bingbu Cao Cc: Yassine Oudjana, linux-media, devicetree, linux-kernel On 11/07/2022 06:28, Yassine Oudjana wrote: > From: Yassine Oudjana <y.oudjana@protonmail.com> > > Add supply properties to describe regulators needed to power > the AK7375 VCM. > > Signed-off-by: Yassine Oudjana <y.oudjana@protonmail.com> > --- Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> Best regards, Krzysztof ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH 3/3] media: i2c: ak7375: Add regulator management 2022-07-11 4:28 [PATCH 0/3] media: i2c: ak7375: Add regulator management Yassine Oudjana 2022-07-11 4:28 ` [PATCH 1/3] media: dt-bindings: ak7375: Convert to DT schema Yassine Oudjana 2022-07-11 4:28 ` [PATCH 2/3] media: dt-bindings: ak7375: Add supplies Yassine Oudjana @ 2022-07-11 4:28 ` Yassine Oudjana 2022-07-11 13:34 ` Jacopo Mondi 2 siblings, 1 reply; 9+ messages in thread From: Yassine Oudjana @ 2022-07-11 4:28 UTC (permalink / raw) To: Mauro Carvalho Chehab, Rob Herring, Krzysztof Kozlowski, Tianshu Qiu, Bingbu Cao Cc: Yassine Oudjana, Yassine Oudjana, linux-media, devicetree, linux-kernel 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> --- 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..59d5cb00e3ba 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. 2 ms is added as a safety margin. + */ +#define AK7375_POWER_DELAY_US 5000 #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; }; @@ -132,6 +148,7 @@ static int ak7375_init_controls(struct ak7375_device *dev_vcm) static int ak7375_probe(struct i2c_client *client) { struct ak7375_device *ak7375_dev; + int i; int ret; ak7375_dev = devm_kzalloc(&client->dev, sizeof(*ak7375_dev), @@ -139,6 +156,17 @@ static int ak7375_probe(struct i2c_client *client) 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 + 10); + ret = ak7375_i2c_write(ak7375_dev, AK7375_REG_CONT, AK7375_MODE_ACTIVE, 1); if (ret) { -- 2.37.0 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH 3/3] media: i2c: ak7375: Add regulator management 2022-07-11 4:28 ` [PATCH 3/3] media: i2c: ak7375: Add regulator management Yassine Oudjana @ 2022-07-11 13:34 ` Jacopo Mondi 2022-07-11 14:08 ` Yassine Oudjana 0 siblings, 1 reply; 9+ messages in thread From: Jacopo Mondi @ 2022-07-11 13:34 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 Hello Yassine On Mon, Jul 11, 2022 at 08:28:39AM +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> > --- > 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..59d5cb00e3ba 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. 2 ms is added as a safety margin. > + */ > +#define AK7375_POWER_DELAY_US 5000 > > #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; > }; > @@ -132,6 +148,7 @@ static int ak7375_init_controls(struct ak7375_device *dev_vcm) > static int ak7375_probe(struct i2c_client *client) > { > struct ak7375_device *ak7375_dev; > + int i; I would have moved this one down to maintain variable declaration in the in-famous reverse-xmas-tree ordering. Up to you. > int ret; > > ak7375_dev = devm_kzalloc(&client->dev, sizeof(*ak7375_dev), > @@ -139,6 +156,17 @@ static int ak7375_probe(struct i2c_client *client) > 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 + 10); > + Isn't 10usec a very small delay to be given to usleep_range() for a delay of at least 3msec ? Also assuming 5msec just to be safe seems a little arbitrary. Adding 2 milliseconds in the wakeup path introduces a non-negligible delay. It's likely a detail, but according to Documentation/timers/timers-howto.rst Since usleep_range is built on top of hrtimers, the wakeup will be very precise (ish), thus a simple usleep function would likely introduce a large number of undesired interrupts. With the introduction of a range, the scheduler is free to coalesce your wakeup with any other wakeup that may have happened for other reasons, or at the worst case, fire an interrupt for your upper bound. The larger a range you supply, the greater a chance that you will not trigger an interrupt; this should be balanced with what is an acceptable upper bound on delay / performance for your specific code path. Exact tolerances here are very situation specific, thus it is left to the caller to determine a reasonable range. If you have a min of 3msec I would try with a range of (3000, 3500). What do you think ? > ret = ak7375_i2c_write(ak7375_dev, AK7375_REG_CONT, > AK7375_MODE_ACTIVE, 1); > if (ret) { > -- > 2.37.0 > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 3/3] media: i2c: ak7375: Add regulator management 2022-07-11 13:34 ` Jacopo Mondi @ 2022-07-11 14:08 ` Yassine Oudjana 2022-07-11 16:25 ` Jacopo Mondi 0 siblings, 1 reply; 9+ messages in thread From: Yassine Oudjana @ 2022-07-11 14:08 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 15:34:23 +0200, Jacopo Mondi <jacopo@jmondi.org> wrote: > Hello Yassine > > On Mon, Jul 11, 2022 at 08:28:39AM +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> >> --- >> 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..59d5cb00e3ba 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. 2 ms is added as a safety margin. >> + */ >> +#define AK7375_POWER_DELAY_US 5000 >> >> #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; >> }; >> @@ -132,6 +148,7 @@ static int ak7375_init_controls(struct >> ak7375_device *dev_vcm) >> static int ak7375_probe(struct i2c_client *client) >> { >> struct ak7375_device *ak7375_dev; >> + int i; > > I would have moved this one down to maintain variable declaration > in the in-famous reverse-xmas-tree ordering. Up to you. I'm used to declaring variables in the order of first use, but I don't really mind it either way. I'll move it down. > >> int ret; >> >> ak7375_dev = devm_kzalloc(&client->dev, sizeof(*ak7375_dev), >> @@ -139,6 +156,17 @@ static int ak7375_probe(struct i2c_client >> *client) >> 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 + 10); >> + > > Isn't 10usec a very small delay to be given to usleep_range() for a > delay of at least 3msec ? Also assuming 5msec just to be safe seems a > little arbitrary. Adding 2 milliseconds in the wakeup path introduces > a non-negligible delay. I must admit that I didn't give it too much thought. I just did it similar to the other delay used in this driver (AK7375_CTRL_DELAY_US). As for adding 2ms, I don't know what the worst case wake-up time is since I don't have a datasheet on hand, so I just wanted to stay safe. Also, this driver doesn't really recover if it fails to resume (which is what used to happen before adding a delay). Rounding up to 5ms felt good enough. > > It's likely a detail, but according to > Documentation/timers/timers-howto.rst > > Since usleep_range is built on top of hrtimers, the > wakeup will be very precise (ish), thus a simple > usleep function would likely introduce a large number > of undesired interrupts. > > With the introduction of a range, the scheduler is > free to coalesce your wakeup with any other wakeup > that may have happened for other reasons, or at the > worst case, fire an interrupt for your upper bound. > > The larger a range you supply, the greater a chance > that you will not trigger an interrupt; this should > be balanced with what is an acceptable upper bound on > delay / performance for your specific code path. Exact > tolerances here are very situation specific, thus it > is left to the caller to determine a reasonable range. > > If you have a min of 3msec I would try with a range of (3000, 3500). > What do you think ? Seems good. I haven't yet had it fail to power on within 3ms of turning on regulators so I guess there is no reason to worry about it. >> >> ret = ak7375_i2c_write(ak7375_dev, AK7375_REG_CONT, >> AK7375_MODE_ACTIVE, 1); >> if (ret) { >> -- >> 2.37.0 > Thanks for the review, Yassine ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 3/3] media: i2c: ak7375: Add regulator management 2022-07-11 14:08 ` Yassine Oudjana @ 2022-07-11 16:25 ` Jacopo Mondi 0 siblings, 0 replies; 9+ messages in thread From: Jacopo Mondi @ 2022-07-11 16:25 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:08:53PM +0400, Yassine Oudjana wrote: > > On Mon, Jul 11 2022 at 15:34:23 +0200, Jacopo Mondi <jacopo@jmondi.org> > wrote: > > Hello Yassine > > > > On Mon, Jul 11, 2022 at 08:28:39AM +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> > > > --- > > > 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..59d5cb00e3ba 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. 2 ms is added as a safety margin. > > > + */ > > > +#define AK7375_POWER_DELAY_US 5000 > > > > > > #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; > > > }; > > > @@ -132,6 +148,7 @@ static int ak7375_init_controls(struct > > > ak7375_device *dev_vcm) > > > static int ak7375_probe(struct i2c_client *client) > > > { > > > struct ak7375_device *ak7375_dev; > > > + int i; > > > > I would have moved this one down to maintain variable declaration > > in the in-famous reverse-xmas-tree ordering. Up to you. > > I'm used to declaring variables in the order of first use, > but I don't really mind it either way. I'll move it down. > > > > > > int ret; > > > > > > ak7375_dev = devm_kzalloc(&client->dev, sizeof(*ak7375_dev), > > > @@ -139,6 +156,17 @@ static int ak7375_probe(struct i2c_client > > > *client) > > > 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 + 10); > > > + > > > > Isn't 10usec a very small delay to be given to usleep_range() for a > > delay of at least 3msec ? Also assuming 5msec just to be safe seems a > > little arbitrary. Adding 2 milliseconds in the wakeup path introduces > > a non-negligible delay. > > I must admit that I didn't give it too much thought. I just > did it similar to the other delay used in this driver > (AK7375_CTRL_DELAY_US). As for adding 2ms, I don't know what > the worst case wake-up time is since I don't have a datasheet > on hand, so I just wanted to stay safe. Also, this driver Oh sorry, I missed in the comment the value was found experimentally an it's not documented.. > doesn't really recover if it fails to resume (which is what > used to happen before adding a delay). Rounding up to 5ms > felt good enough. > > > > > It's likely a detail, but according to > > Documentation/timers/timers-howto.rst > > > > Since usleep_range is built on top of hrtimers, the > > wakeup will be very precise (ish), thus a simple > > usleep function would likely introduce a large number > > of undesired interrupts. > > > > With the introduction of a range, the scheduler is > > free to coalesce your wakeup with any other wakeup > > that may have happened for other reasons, or at the > > worst case, fire an interrupt for your upper bound. > > > > The larger a range you supply, the greater a chance > > that you will not trigger an interrupt; this should > > be balanced with what is an acceptable upper bound on > > delay / performance for your specific code path. Exact > > tolerances here are very situation specific, thus it > > is left to the caller to determine a reasonable range. > > > > If you have a min of 3msec I would try with a range of (3000, 3500). > > What do you think ? > > Seems good. I haven't yet had it fail to power on within 3ms of > turning on regulators so I guess there is no reason to worry about it. > Ok then :) There's anyway a comment that says the value comes from practical experience, so if anything bad happens, it's easy to track it down to that Thanks j > > > > > > ret = ak7375_i2c_write(ak7375_dev, AK7375_REG_CONT, > > > AK7375_MODE_ACTIVE, 1); > > > if (ret) { > > > -- > > > 2.37.0 > > > > Thanks for the review, > Yassine > > ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2022-07-11 16:25 UTC | newest] Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-07-11 4:28 [PATCH 0/3] media: i2c: ak7375: Add regulator management Yassine Oudjana 2022-07-11 4:28 ` [PATCH 1/3] media: dt-bindings: ak7375: Convert to DT schema Yassine Oudjana 2022-07-11 11:42 ` Krzysztof Kozlowski 2022-07-11 4:28 ` [PATCH 2/3] media: dt-bindings: ak7375: Add supplies Yassine Oudjana 2022-07-11 11:42 ` Krzysztof Kozlowski 2022-07-11 4:28 ` [PATCH 3/3] media: i2c: ak7375: Add regulator management Yassine Oudjana 2022-07-11 13:34 ` Jacopo Mondi 2022-07-11 14:08 ` Yassine Oudjana 2022-07-11 16:25 ` Jacopo Mondi
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.