2020-07-24 21:34:06

by Stephen Boyd

[permalink] [raw]
Subject: [PATCH 0/3] Some sx9310 iio driver updates

These patches are only related because I'm looking at this driver. The
first one resends the DT binding for the driver that was merged in
v5.8-rc1 with a small change to update for proper regulators. The second
patch fixes a few printks that are missing newlines and should be
totally non-trivial to apply. The third patch changes whoami to unsigned
to avoid confusing. The fourth patch drops channel_users because it's
unused. The final patch adds support to enable the svdd and vdd supplies
so that this driver can work on a board I have where the svdd supply
isn't enabled at boot and needs to be turned on before this driver
starts to communicate with the chip.

Changes from v1:
* Two new patches for whoami and channel_uesrs
* Use bulk regulator APIs for supplies patch
* Support both regulators

Daniel Campello (1):
dt-bindings: iio: Add bindings for sx9310 sensor

Stephen Boyd (4):
iio: sx9310: Add newlines to printks
iio: sx9310: whoami is unsigned
iio: sx9310: Drop channel_users[]
iio: sx9310: Enable vdd and svdd regulators at probe

.../iio/proximity/semtech,sx9310.yaml | 60 +++++++++++++++++++
drivers/iio/proximity/sx9310.c | 46 +++++++++++---
2 files changed, 97 insertions(+), 9 deletions(-)
create mode 100644 Documentation/devicetree/bindings/iio/proximity/semtech,sx9310.yaml

Cc: Hartmut Knaack <[email protected]>
Cc: Lars-Peter Clausen <[email protected]>
Cc: Peter Meerwald-Stadler <[email protected]>
Cc: Rob Herring <[email protected]>
Cc: Douglas Anderson <[email protected]>
Cc: Daniel Campello <[email protected]>

base-commit: b3a9e3b9622ae10064826dccb4f7a52bd88c7407
--
Sent by a computer, using git, on the internet


2020-07-24 21:34:14

by Stephen Boyd

[permalink] [raw]
Subject: [PATCH v2 4/5] iio: sx9310: Drop channel_users[]

This struct member isn't used. Drop it.

Cc: Gwendal Grignou <[email protected]>
Cc: Daniel Campello <[email protected]>
Cc: Hartmut Knaack <[email protected]>
Cc: Lars-Peter Clausen <[email protected]>
Cc: Peter Meerwald-Stadler <[email protected]>
Cc: Douglas Anderson <[email protected]>
Fixes: 72ad02b15d63 ("iio: Add SEMTECH SX9310/9311 sensor driver")
Signed-off-by: Stephen Boyd <[email protected]>
---
drivers/iio/proximity/sx9310.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/iio/proximity/sx9310.c b/drivers/iio/proximity/sx9310.c
index fca871ad82ba..1e1f6bba50f6 100644
--- a/drivers/iio/proximity/sx9310.c
+++ b/drivers/iio/proximity/sx9310.c
@@ -142,8 +142,8 @@ struct sx9310_data {
/* Remember enabled channels and sample rate during suspend. */
unsigned int suspend_ctrl0;
struct completion completion;
- unsigned int chan_read, chan_event;
- int channel_users[SX9310_NUM_CHANNELS];
+ unsigned int chan_read;
+ unsigned int chan_event;
unsigned int whoami;
};

--
Sent by a computer, using git, on the internet

2020-07-24 21:34:15

by Stephen Boyd

[permalink] [raw]
Subject: [PATCH v2 1/5] dt-bindings: iio: Add bindings for sx9310 sensor

From: Daniel Campello <[email protected]>

Adds device tree bandings for sx9310 sensor.

Signed-off-by: Daniel Campello <[email protected]>
Cc: Hartmut Knaack <[email protected]>
Cc: Lars-Peter Clausen <[email protected]>
Cc: Peter Meerwald-Stadler <[email protected]>
Cc: Rob Herring <[email protected]>
Reviewed-by: Douglas Anderson <[email protected]>
[[email protected]: Add both regulators and make them optional]
Signed-off-by: Stephen Boyd <[email protected]>
---
.../iio/proximity/semtech,sx9310.yaml | 60 +++++++++++++++++++
1 file changed, 60 insertions(+)
create mode 100644 Documentation/devicetree/bindings/iio/proximity/semtech,sx9310.yaml

diff --git a/Documentation/devicetree/bindings/iio/proximity/semtech,sx9310.yaml b/Documentation/devicetree/bindings/iio/proximity/semtech,sx9310.yaml
new file mode 100644
index 000000000000..ba734ee868c7
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/proximity/semtech,sx9310.yaml
@@ -0,0 +1,60 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/iio/proximity/semtech,sx9310.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Semtech's SX9310 capacitive proximity sensor
+
+maintainers:
+ - Daniel Campello <[email protected]>
+
+description: |
+ Semtech's SX9310/SX9311 capacitive proximity/button solution.
+
+ Specifications about the devices can be found at:
+ https://www.semtech.com/products/smart-sensing/sar-sensors/sx9310
+
+properties:
+ compatible:
+ enum:
+ - semtech,sx9310
+ - semtech,sx9311
+
+ reg:
+ maxItems: 1
+
+ interrupts:
+ description:
+ The sole interrupt generated by the device used to announce the
+ preceding reading request has finished and that data is
+ available or that a close/far proximity event has happened.
+ maxItems: 1
+
+ vdd-supply:
+ description: Main power supply
+
+ svdd-supply:
+ description: Host interface power supply
+
+required:
+ - compatible
+ - reg
+
+additionalProperties: false
+
+examples:
+ - |
+ #include <dt-bindings/interrupt-controller/irq.h>
+ i2c {
+ #address-cells = <1>;
+ #size-cells = <0>;
+ proximity@28 {
+ compatible = "semtech,sx9310";
+ reg = <0x28>;
+ interrupt-parent = <&pio>;
+ interrupts = <5 IRQ_TYPE_LEVEL_LOW 5>;
+ vdd-supply = <&pp3300_a>;
+ svdd-supply = <&pp1800_prox>;
+ };
+ };
--
Sent by a computer, using git, on the internet

2020-07-24 21:34:19

by Stephen Boyd

[permalink] [raw]
Subject: [PATCH v2 5/5] iio: sx9310: Enable vdd and svdd regulators at probe

Enable the main power supply (vdd) and digital IO power supply (svdd)
during probe so that the i2c communication and device works properly on
boards that aggressively power gate these supplies.

Cc: Gwendal Grignou <[email protected]>
Cc: Daniel Campello <[email protected]>
Cc: Hartmut Knaack <[email protected]>
Cc: Lars-Peter Clausen <[email protected]>
Cc: Peter Meerwald-Stadler <[email protected]>
Cc: Douglas Anderson <[email protected]>
Signed-off-by: Stephen Boyd <[email protected]>
---
drivers/iio/proximity/sx9310.c | 27 +++++++++++++++++++++++++++
1 file changed, 27 insertions(+)

diff --git a/drivers/iio/proximity/sx9310.c b/drivers/iio/proximity/sx9310.c
index 1e1f6bba50f6..ad6ed100c7a6 100644
--- a/drivers/iio/proximity/sx9310.c
+++ b/drivers/iio/proximity/sx9310.c
@@ -19,6 +19,7 @@
#include <linux/of.h>
#include <linux/pm.h>
#include <linux/regmap.h>
+#include <linux/regulator/consumer.h>
#include <linux/slab.h>

#include <linux/iio/buffer.h>
@@ -899,12 +900,21 @@ static int sx9310_set_indio_dev_name(struct device *dev,
return 0;
}

+static void sx9310_regulator_disable(void *supplies)
+{
+ regulator_bulk_disable(2, supplies);
+}
+
static int sx9310_probe(struct i2c_client *client,
const struct i2c_device_id *id)
{
int ret;
struct iio_dev *indio_dev;
struct sx9310_data *data;
+ struct regulator_bulk_data supplies[] = {
+ { .supply = "vdd" },
+ { .supply = "svdd" },
+ };

indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*data));
if (indio_dev == NULL)
@@ -919,6 +929,23 @@ static int sx9310_probe(struct i2c_client *client,
if (IS_ERR(data->regmap))
return PTR_ERR(data->regmap);

+ ret = devm_regulator_bulk_get(&client->dev, ARRAY_SIZE(supplies), supplies);
+ if (ret)
+ return ret;
+
+ ret = regulator_bulk_enable(ARRAY_SIZE(supplies), supplies);
+ if (ret)
+ return ret;
+ /* Must wait for Tpor time after initial power up */
+ usleep_range(1000, 1100);
+
+ /* Update sx9310_regulator_disable() size if this bug is hit */
+ BUILD_BUG_ON(ARRAY_SIZE(supplies) != 2);
+ ret = devm_add_action_or_reset(&client->dev, sx9310_regulator_disable,
+ supplies);
+ if (ret)
+ return ret;
+
ret = regmap_read(data->regmap, SX9310_REG_WHOAMI, &data->whoami);
if (ret < 0) {
dev_err(&client->dev, "error in reading WHOAMI register: %d\n",
--
Sent by a computer, using git, on the internet

2020-07-24 21:34:50

by Stephen Boyd

[permalink] [raw]
Subject: [PATCH v2 3/5] iio: sx9310: whoami is unsigned

This is an unsigned value, actually it's a u8 but regmap doesn't handle
that easily. Let's make it unsigned throughout so that we don't need to
worry about signed vs. unsigned comparison behavior.

Cc: Gwendal Grignou <[email protected]>
Cc: Daniel Campello <[email protected]>
Cc: Hartmut Knaack <[email protected]>
Cc: Lars-Peter Clausen <[email protected]>
Cc: Peter Meerwald-Stadler <[email protected]>
Cc: Douglas Anderson <[email protected]>
Fixes: 72ad02b15d63 ("iio: Add SEMTECH SX9310/9311 sensor driver")
Signed-off-by: Stephen Boyd <[email protected]>
---
drivers/iio/proximity/sx9310.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/iio/proximity/sx9310.c b/drivers/iio/proximity/sx9310.c
index 84c3c9ae80dc..fca871ad82ba 100644
--- a/drivers/iio/proximity/sx9310.c
+++ b/drivers/iio/proximity/sx9310.c
@@ -144,7 +144,7 @@ struct sx9310_data {
struct completion completion;
unsigned int chan_read, chan_event;
int channel_users[SX9310_NUM_CHANNELS];
- int whoami;
+ unsigned int whoami;
};

static const struct iio_event_spec sx9310_events[] = {
@@ -864,7 +864,8 @@ static int sx9310_init_device(struct iio_dev *indio_dev)

static int sx9310_set_indio_dev_name(struct device *dev,
struct iio_dev *indio_dev,
- const struct i2c_device_id *id, int whoami)
+ const struct i2c_device_id *id,
+ unsigned int whoami)
{
const struct acpi_device_id *acpi_id;

--
Sent by a computer, using git, on the internet

2020-07-24 21:37:15

by Stephen Boyd

[permalink] [raw]
Subject: Re: [PATCH v2 1/5] dt-bindings: iio: Add bindings for sx9310 sensor

Quoting Stephen Boyd (2020-07-24 14:33:25)
> From: Daniel Campello <[email protected]>
>
> Adds device tree bandings for sx9310 sensor.
>
> Signed-off-by: Daniel Campello <[email protected]>
> Cc: Hartmut Knaack <[email protected]>
> Cc: Lars-Peter Clausen <[email protected]>
> Cc: Peter Meerwald-Stadler <[email protected]>
> Cc: Rob Herring <[email protected]>
> Reviewed-by: Douglas Anderson <[email protected]>
> [[email protected]: Add both regulators and make them optional]
> Signed-off-by: Stephen Boyd <[email protected]>
> ---

I forgot to Cc devicetree list. Will do next time around.

-Stephen

> .../iio/proximity/semtech,sx9310.yaml | 60 +++++++++++++++++++
> 1 file changed, 60 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/iio/proximity/semtech,sx9310.yaml
>
> diff --git a/Documentation/devicetree/bindings/iio/proximity/semtech,sx9310.yaml b/Documentation/devicetree/bindings/iio/proximity/semtech,sx9310.yaml
> new file mode 100644
> index 000000000000..ba734ee868c7
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/proximity/semtech,sx9310.yaml
> @@ -0,0 +1,60 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/iio/proximity/semtech,sx9310.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Semtech's SX9310 capacitive proximity sensor
> +
> +maintainers:
> + - Daniel Campello <[email protected]>
> +
> +description: |
> + Semtech's SX9310/SX9311 capacitive proximity/button solution.
> +
> + Specifications about the devices can be found at:
> + https://www.semtech.com/products/smart-sensing/sar-sensors/sx9310
> +
> +properties:
> + compatible:
> + enum:
> + - semtech,sx9310
> + - semtech,sx9311
> +
> + reg:
> + maxItems: 1
> +
> + interrupts:
> + description:
> + The sole interrupt generated by the device used to announce the
> + preceding reading request has finished and that data is
> + available or that a close/far proximity event has happened.
> + maxItems: 1
> +
> + vdd-supply:
> + description: Main power supply
> +
> + svdd-supply:
> + description: Host interface power supply
> +
> +required:
> + - compatible
> + - reg
> +
> +additionalProperties: false
> +
> +examples:
> + - |
> + #include <dt-bindings/interrupt-controller/irq.h>
> + i2c {
> + #address-cells = <1>;
> + #size-cells = <0>;
> + proximity@28 {
> + compatible = "semtech,sx9310";
> + reg = <0x28>;
> + interrupt-parent = <&pio>;
> + interrupts = <5 IRQ_TYPE_LEVEL_LOW 5>;
> + vdd-supply = <&pp3300_a>;
> + svdd-supply = <&pp1800_prox>;
> + };
> + };

2020-07-24 21:37:42

by Stephen Boyd

[permalink] [raw]
Subject: [PATCH v2 2/5] iio: sx9310: Add newlines to printks

Printks in the kernel have newlines at the end. Add them to the few
printks in this driver.

Cc: Gwendal Grignou <[email protected]>
Reviewed-by: Daniel Campello <[email protected]>
Cc: Hartmut Knaack <[email protected]>
Cc: Lars-Peter Clausen <[email protected]>
Cc: Peter Meerwald-Stadler <[email protected]>
Reviewed-by: Douglas Anderson <[email protected]>
Fixes: 72ad02b15d63 ("iio: Add SEMTECH SX9310/9311 sensor driver")
Signed-off-by: Stephen Boyd <[email protected]>
---
drivers/iio/proximity/sx9310.c | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/iio/proximity/sx9310.c b/drivers/iio/proximity/sx9310.c
index d161f3061e35..84c3c9ae80dc 100644
--- a/drivers/iio/proximity/sx9310.c
+++ b/drivers/iio/proximity/sx9310.c
@@ -824,7 +824,7 @@ static int sx9310_init_compensation(struct iio_dev *indio_dev)

if (i < 0) {
dev_err(&data->client->dev,
- "initial compensation timed out: 0x%02x", val);
+ "initial compensation timed out: 0x%02x\n", val);
ret = -ETIMEDOUT;
}

@@ -871,14 +871,14 @@ static int sx9310_set_indio_dev_name(struct device *dev,
/* id will be NULL when enumerated via ACPI */
if (id) {
if (id->driver_data != whoami)
- dev_err(dev, "WHOAMI does not match i2c_device_id: %s",
+ dev_err(dev, "WHOAMI does not match i2c_device_id: %s\n",
id->name);
} else if (ACPI_HANDLE(dev)) {
acpi_id = acpi_match_device(dev->driver->acpi_match_table, dev);
if (!acpi_id)
return -ENODEV;
if (acpi_id->driver_data != whoami)
- dev_err(dev, "WHOAMI does not match acpi_device_id: %s",
+ dev_err(dev, "WHOAMI does not match acpi_device_id: %s\n",
acpi_id->id);
} else
return -ENODEV;
@@ -891,7 +891,7 @@ static int sx9310_set_indio_dev_name(struct device *dev,
indio_dev->name = "sx9311";
break;
default:
- dev_err(dev, "unexpected WHOAMI response: %u", whoami);
+ dev_err(dev, "unexpected WHOAMI response: %u\n", whoami);
return -ENODEV;
}

@@ -920,7 +920,7 @@ static int sx9310_probe(struct i2c_client *client,

ret = regmap_read(data->regmap, SX9310_REG_WHOAMI, &data->whoami);
if (ret < 0) {
- dev_err(&client->dev, "error in reading WHOAMI register: %d",
+ dev_err(&client->dev, "error in reading WHOAMI register: %d\n",
ret);
return ret;
}
--
Sent by a computer, using git, on the internet

2020-07-24 21:57:03

by Doug Anderson

[permalink] [raw]
Subject: Re: [PATCH v2 3/5] iio: sx9310: whoami is unsigned

Hi,

On Fri, Jul 24, 2020 at 2:33 PM Stephen Boyd <[email protected]> wrote:
>
> This is an unsigned value, actually it's a u8 but regmap doesn't handle
> that easily. Let's make it unsigned throughout so that we don't need to
> worry about signed vs. unsigned comparison behavior.
>
> Cc: Gwendal Grignou <[email protected]>
> Cc: Daniel Campello <[email protected]>
> Cc: Hartmut Knaack <[email protected]>
> Cc: Lars-Peter Clausen <[email protected]>
> Cc: Peter Meerwald-Stadler <[email protected]>
> Cc: Douglas Anderson <[email protected]>
> Fixes: 72ad02b15d63 ("iio: Add SEMTECH SX9310/9311 sensor driver")
> Signed-off-by: Stephen Boyd <[email protected]>
> ---
> drivers/iio/proximity/sx9310.c | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)

Reviewed-by: Douglas Anderson <[email protected]>

2020-07-24 21:58:35

by Doug Anderson

[permalink] [raw]
Subject: Re: [PATCH v2 4/5] iio: sx9310: Drop channel_users[]

Hi,

On Fri, Jul 24, 2020 at 2:33 PM Stephen Boyd <[email protected]> wrote:
>
> This struct member isn't used. Drop it.
>
> Cc: Gwendal Grignou <[email protected]>
> Cc: Daniel Campello <[email protected]>
> Cc: Hartmut Knaack <[email protected]>
> Cc: Lars-Peter Clausen <[email protected]>
> Cc: Peter Meerwald-Stadler <[email protected]>
> Cc: Douglas Anderson <[email protected]>
> Fixes: 72ad02b15d63 ("iio: Add SEMTECH SX9310/9311 sensor driver")
> Signed-off-by: Stephen Boyd <[email protected]>
> ---
> drivers/iio/proximity/sx9310.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)

Reviewed-by: Douglas Anderson <[email protected]>

2020-07-24 22:04:02

by Doug Anderson

[permalink] [raw]
Subject: Re: [PATCH v2 5/5] iio: sx9310: Enable vdd and svdd regulators at probe

Hi,

On Fri, Jul 24, 2020 at 2:33 PM Stephen Boyd <[email protected]> wrote:
>
> Enable the main power supply (vdd) and digital IO power supply (svdd)
> during probe so that the i2c communication and device works properly on
> boards that aggressively power gate these supplies.
>
> Cc: Gwendal Grignou <[email protected]>
> Cc: Daniel Campello <[email protected]>
> Cc: Hartmut Knaack <[email protected]>
> Cc: Lars-Peter Clausen <[email protected]>
> Cc: Peter Meerwald-Stadler <[email protected]>
> Cc: Douglas Anderson <[email protected]>
> Signed-off-by: Stephen Boyd <[email protected]>
> ---
> drivers/iio/proximity/sx9310.c | 27 +++++++++++++++++++++++++++
> 1 file changed, 27 insertions(+)
>
> diff --git a/drivers/iio/proximity/sx9310.c b/drivers/iio/proximity/sx9310.c
> index 1e1f6bba50f6..ad6ed100c7a6 100644
> --- a/drivers/iio/proximity/sx9310.c
> +++ b/drivers/iio/proximity/sx9310.c
> @@ -19,6 +19,7 @@
> #include <linux/of.h>
> #include <linux/pm.h>
> #include <linux/regmap.h>
> +#include <linux/regulator/consumer.h>
> #include <linux/slab.h>
>
> #include <linux/iio/buffer.h>
> @@ -899,12 +900,21 @@ static int sx9310_set_indio_dev_name(struct device *dev,
> return 0;
> }
>
> +static void sx9310_regulator_disable(void *supplies)
> +{
> + regulator_bulk_disable(2, supplies);
> +}
> +
> static int sx9310_probe(struct i2c_client *client,
> const struct i2c_device_id *id)
> {
> int ret;
> struct iio_dev *indio_dev;
> struct sx9310_data *data;
> + struct regulator_bulk_data supplies[] = {
> + { .supply = "vdd" },
> + { .supply = "svdd" },
> + };
>
> indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*data));
> if (indio_dev == NULL)
> @@ -919,6 +929,23 @@ static int sx9310_probe(struct i2c_client *client,
> if (IS_ERR(data->regmap))
> return PTR_ERR(data->regmap);
>
> + ret = devm_regulator_bulk_get(&client->dev, ARRAY_SIZE(supplies), supplies);
> + if (ret)
> + return ret;
> +
> + ret = regulator_bulk_enable(ARRAY_SIZE(supplies), supplies);
> + if (ret)
> + return ret;
> + /* Must wait for Tpor time after initial power up */
> + usleep_range(1000, 1100);
> +
> + /* Update sx9310_regulator_disable() size if this bug is hit */
> + BUILD_BUG_ON(ARRAY_SIZE(supplies) != 2);
> + ret = devm_add_action_or_reset(&client->dev, sx9310_regulator_disable,
> + supplies);

...but, but... Aren't you storing a pointer to stack memory? How
does that work? I think you either need to store the "struct
regulator_bulk_data" in your private data or just make two normal
regulator calls.

-Doug

2020-07-25 00:17:41

by Daniel Campello

[permalink] [raw]
Subject: Re: [PATCH v2 3/5] iio: sx9310: whoami is unsigned

On Fri, Jul 24, 2020 at 3:33 PM Stephen Boyd <[email protected]> wrote:
>
> This is an unsigned value, actually it's a u8 but regmap doesn't handle
> that easily. Let's make it unsigned throughout so that we don't need to
> worry about signed vs. unsigned comparison behavior.
>
> Cc: Gwendal Grignou <[email protected]>
> Cc: Daniel Campello <[email protected]>
> Cc: Hartmut Knaack <[email protected]>
> Cc: Lars-Peter Clausen <[email protected]>
> Cc: Peter Meerwald-Stadler <[email protected]>
> Cc: Douglas Anderson <[email protected]>
> Fixes: 72ad02b15d63 ("iio: Add SEMTECH SX9310/9311 sensor driver")
> Signed-off-by: Stephen Boyd <[email protected]>

Reviewed-by: Daniel Campello <[email protected]>

2020-07-25 00:18:27

by Daniel Campello

[permalink] [raw]
Subject: Re: [PATCH v2 4/5] iio: sx9310: Drop channel_users[]

On Fri, Jul 24, 2020 at 3:33 PM Stephen Boyd <[email protected]> wrote:
>
> This struct member isn't used. Drop it.
>
> Cc: Gwendal Grignou <[email protected]>
> Cc: Daniel Campello <[email protected]>
> Cc: Hartmut Knaack <[email protected]>
> Cc: Lars-Peter Clausen <[email protected]>
> Cc: Peter Meerwald-Stadler <[email protected]>
> Cc: Douglas Anderson <[email protected]>
> Fixes: 72ad02b15d63 ("iio: Add SEMTECH SX9310/9311 sensor driver")
> Signed-off-by: Stephen Boyd <[email protected]>

Reviewed-by: Daniel Campello <[email protected]>

2020-07-25 00:39:13

by Stephen Boyd

[permalink] [raw]
Subject: Re: [PATCH v2 5/5] iio: sx9310: Enable vdd and svdd regulators at probe

Quoting Doug Anderson (2020-07-24 15:02:23)
> On Fri, Jul 24, 2020 at 2:33 PM Stephen Boyd <[email protected]> wrote:
> > diff --git a/drivers/iio/proximity/sx9310.c b/drivers/iio/proximity/sx9310.c
> > index 1e1f6bba50f6..ad6ed100c7a6 100644
> > --- a/drivers/iio/proximity/sx9310.c
> > +++ b/drivers/iio/proximity/sx9310.c
> > @@ -919,6 +929,23 @@ static int sx9310_probe(struct i2c_client *client,
> > if (IS_ERR(data->regmap))
> > return PTR_ERR(data->regmap);
> >
> > + ret = devm_regulator_bulk_get(&client->dev, ARRAY_SIZE(supplies), supplies);
> > + if (ret)
> > + return ret;
> > +
> > + ret = regulator_bulk_enable(ARRAY_SIZE(supplies), supplies);
> > + if (ret)
> > + return ret;
> > + /* Must wait for Tpor time after initial power up */
> > + usleep_range(1000, 1100);
> > +
> > + /* Update sx9310_regulator_disable() size if this bug is hit */
> > + BUILD_BUG_ON(ARRAY_SIZE(supplies) != 2);
> > + ret = devm_add_action_or_reset(&client->dev, sx9310_regulator_disable,
> > + supplies);
>
> ...but, but... Aren't you storing a pointer to stack memory? How
> does that work? I think you either need to store the "struct
> regulator_bulk_data" in your private data or just make two normal
> regulator calls.
>

Doh, no coffee today.

2020-07-26 11:59:34

by Jonathan Cameron

[permalink] [raw]
Subject: Re: [PATCH 0/3] Some sx9310 iio driver updates

On Fri, 24 Jul 2020 14:33:24 -0700
Stephen Boyd <[email protected]> wrote:

> These patches are only related because I'm looking at this driver. The
> first one resends the DT binding for the driver that was merged in
> v5.8-rc1 with a small change to update for proper regulators. The second
> patch fixes a few printks that are missing newlines and should be
> totally non-trivial to apply. The third patch changes whoami to unsigned
> to avoid confusing. The fourth patch drops channel_users because it's
> unused. The final patch adds support to enable the svdd and vdd supplies
> so that this driver can work on a board I have where the svdd supply
> isn't enabled at boot and needs to be turned on before this driver
> starts to communicate with the chip.
Hi Stephen,

I clearly made a mess of picking this driver up in the first place.

Anyhow, we now have two series in flight for the driver that, whilst
mostly independent (I think) will result in at least some fuzz.
If possible could you work with Daniel to send me one single series
with all the changes?

Thanks,

Jonathan

>
> Changes from v1:
> * Two new patches for whoami and channel_uesrs
> * Use bulk regulator APIs for supplies patch
> * Support both regulators
>
> Daniel Campello (1):
> dt-bindings: iio: Add bindings for sx9310 sensor
>
> Stephen Boyd (4):
> iio: sx9310: Add newlines to printks
> iio: sx9310: whoami is unsigned
> iio: sx9310: Drop channel_users[]
> iio: sx9310: Enable vdd and svdd regulators at probe
>
> .../iio/proximity/semtech,sx9310.yaml | 60 +++++++++++++++++++
> drivers/iio/proximity/sx9310.c | 46 +++++++++++---
> 2 files changed, 97 insertions(+), 9 deletions(-)
> create mode 100644 Documentation/devicetree/bindings/iio/proximity/semtech,sx9310.yaml
>
> Cc: Hartmut Knaack <[email protected]>
> Cc: Lars-Peter Clausen <[email protected]>
> Cc: Peter Meerwald-Stadler <[email protected]>
> Cc: Rob Herring <[email protected]>
> Cc: Douglas Anderson <[email protected]>
> Cc: Daniel Campello <[email protected]>
>
> base-commit: b3a9e3b9622ae10064826dccb4f7a52bd88c7407

2020-07-27 20:03:54

by Stephen Boyd

[permalink] [raw]
Subject: Re: [PATCH 0/3] Some sx9310 iio driver updates

Quoting Jonathan Cameron (2020-07-26 04:56:36)
> On Fri, 24 Jul 2020 14:33:24 -0700
> Stephen Boyd <[email protected]> wrote:
>
> > These patches are only related because I'm looking at this driver. The
> > first one resends the DT binding for the driver that was merged in
> > v5.8-rc1 with a small change to update for proper regulators. The second
> > patch fixes a few printks that are missing newlines and should be
> > totally non-trivial to apply. The third patch changes whoami to unsigned
> > to avoid confusing. The fourth patch drops channel_users because it's
> > unused. The final patch adds support to enable the svdd and vdd supplies
> > so that this driver can work on a board I have where the svdd supply
> > isn't enabled at boot and needs to be turned on before this driver
> > starts to communicate with the chip.
> Hi Stephen,
>
> I clearly made a mess of picking this driver up in the first place.
>
> Anyhow, we now have two series in flight for the driver that, whilst
> mostly independent (I think) will result in at least some fuzz.
> If possible could you work with Daniel to send me one single series
> with all the changes?
>

Sure. No problem for me to work with Daniel. Thanks.