Re: [PATCH v3 1/2] dt-bindings: input/touchscreen: add bindings for msg26xx
Hi Vincent, On Tue, Feb 09, 2021 at 07:58:33PM +0100, Vincent Knecht wrote: > Le mardi 09 février 2021 à 10:13 -0600, Rob Herring a écrit : > > On Thu, Jan 21, 2021 at 06:43:47PM +0100, Vincent Knecht wrote: > > > This adds dts bindings for the mstar msg26xx touchscreen. > > > > > > Signed-off-by: Vincent Knecht > > > --- > > > Changed in v3: > > > - added `touchscreen-size-x: true` and `touchscreen-size-y: true` > > > properties > > > Changed in v2: > > > - changed M-Star to MStar in title line > > > - changed reset gpio to active-low in example section > > > --- > > > .../input/touchscreen/mstar,msg26xx.yaml | 69 +++ > > > 1 file changed, 69 insertions(+) > > > create mode 100644 > > > Documentation/devicetree/bindings/input/touchscreen/mstar,msg26xx.yaml > > > > > > diff --git > > > a/Documentation/devicetree/bindings/input/touchscreen/mstar,msg26xx.yaml > > > b/Documentation/devicetree/bindings/input/touchscreen/mstar,msg26xx.yaml > > > new file mode 100644 > > > index ..5d26a1008bf1 > > > --- /dev/null > > > +++ > > > b/Documentation/devicetree/bindings/input/touchscreen/mstar,msg26xx.yaml > > > @@ -0,0 +1,69 @@ > > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > > > +%YAML 1.2 > > > +--- > > > +$id: http://devicetree.org/schemas/input/touchscreen/mstar,msg26xx.yaml# > > > +$schema: http://devicetree.org/meta-schemas/core.yaml# > > > + > > > +title: MStar msg26xx touchscreen controller Bindings > > > + > > > +maintainers: > > > + - Vincent Knecht > > > + > > > +allOf: > > > + - $ref: touchscreen.yaml# > > > + > > > +properties: > > > + compatible: > > > + const: mstar,msg26xx > > > > Don't use wildcards in compatible strings. > > Thank you for the input... > > Let's say I set it to "mstar,msg2638", is it better to rename the driver file > and functions too ? > According to downstream source file naming, msg2638 is the model I have and > test this driver with. This is ultimately Dmitry's call, but it's fairly common to use wildcards for driver names and function calls if the driver is known to work across all devices that fit in the wildcard (see iqs5xx and many others). The risk with wildcards, however, is that vendors can introduce different devices later with similar part numbers. Therefore, some subsystems (e.g. iio) tend to frown upon wildcards for that reason. You should try and make the driver cover as many devices as possible. But if the driver is only known to work for one device then I don't think you can use a wildcard in the name unless you support all other devices (just my opinion). In either case, however, compatible strings must be unique just as with a part number in a schematic or bill of materials. As such, it is perfectly fine to have multiple compatible strings in a single driver. > > > There's a possibility this driver works as-is or with minor mods for msg2633 > too, > and a more remote one for msg21xx and msg22xx... > Kind regards, Jeff LaBundy
Re: [PATCH][next] Input: iqs5xx: Ensure error_bl is initialized on error exit path
Hi Colin,
On Thu, Jan 28, 2021 at 12:19:03PM +, Colin King wrote:
> From: Colin Ian King
>
> Currently if the call to qs5xx_fw_file_parse fails the error return
> exit path will read the uninitialized variable error_bl. Fix this
> by ensuring error_bl is initialized to zero.
>
> Addresses-Coverity: ("Uninitialized scalar variable")
> Fixes: 2539da6677b6 ("Input: iqs5xx - preserve bootloader errors")
> Signed-off-by: Colin Ian King
This was fixed in [1]; it just needs pushed.
[1] https://patchwork.kernel.org/patch/12043701
> ---
> drivers/input/touchscreen/iqs5xx.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/input/touchscreen/iqs5xx.c
> b/drivers/input/touchscreen/iqs5xx.c
> index 05e0c6ff217b..54f30038dca4 100644
> --- a/drivers/input/touchscreen/iqs5xx.c
> +++ b/drivers/input/touchscreen/iqs5xx.c
> @@ -852,7 +852,7 @@ static int iqs5xx_fw_file_parse(struct i2c_client *client,
> static int iqs5xx_fw_file_write(struct i2c_client *client, const char
> *fw_file)
> {
> struct iqs5xx_private *iqs5xx = i2c_get_clientdata(client);
> - int error, error_bl;
> + int error, error_bl = 0;
> u8 *pmap;
>
> if (iqs5xx->bl_status == IQS5XX_BL_STATUS_NONE)
> --
> 2.29.2
>
Kind regards,
Jeff LaBundy
Re: [PATCH v9 1/4] dt-bindings: mfd: Fix schema warnings for pwm-leds
Hi Alexander,
Thanks again for your work on this.
On Fri, Jan 15, 2021 at 10:42:39AM +0100, Alexander Dahl wrote:
> Hello Jeff,
>
> On Thu, Jan 14, 2021 at 09:50:50PM -0600, Jeff LaBundy wrote:
> > On Thu, Jan 14, 2021 at 10:03:12AM +, Lee Jones wrote:
> > > On Mon, 28 Dec 2020, Alexander Dahl wrote:
> > >
> > > > The node names for devices using the pwm-leds driver follow a certain
> > > > naming scheme (now). Parent node name is not enforced, but recommended
> > > > by DT project.
> > > >
> > > > DTC Documentation/devicetree/bindings/mfd/iqs62x.example.dt.yaml
> > > > CHECK Documentation/devicetree/bindings/mfd/iqs62x.example.dt.yaml
> > > > /home/alex/build/linux/Documentation/devicetree/bindings/mfd/iqs62x.example.dt.yaml:
> > > > pwmleds: 'panel' does not match any of the regexes:
> > > > '^led(-[0-9a-f]+)?$', 'pinctrl-[0-9]+'
> > > > From schema:
> > > > /home/alex/src/linux/leds/Documentation/devicetree/bindings/leds/leds-pwm.yaml
> > > >
> > > > Signed-off-by: Alexander Dahl
> > > > Acked-by: Jeff LaBundy
> > > > Acked-by: Rob Herring
> > > > ---
> > > >
> > > > Notes:
> > > > v8 -> v9:
> > > > * added forgotten Acked-by (Jeff LaBundy)
> > > > * rebased on v5.11-rc1
> > > >
> > > > v7 -> v8:
> > > > * rebased on recent pavel/for-next (post v5.10-rc1)
> > > > * added Acked-by (Rob Herring)
> > > >
> > > > v6 -> v7:
> > > > * added warning message to commit message (Krzysztof Kozlowski)
> > > >
> > > > v6:
> > > > * added this patch to series
> > > >
> > > > Documentation/devicetree/bindings/mfd/iqs62x.yaml | 5 +++--
> > > > 1 file changed, 3 insertions(+), 2 deletions(-)
> > >
> > > Failed to apply:
> > >
> > > Applying: dt-bindings: mfd: Fix schema warnings for pwm-leds
> > > Using index info to reconstruct a base tree...
> > > M Documentation/devicetree/bindings/mfd/iqs62x.yaml
> > > /home/lee/projects/linux/kernel/.git/worktrees/mfd/rebase-apply/patch:34:
> > > indent with spaces.
> > > led-1 {
> > > /home/lee/projects/linux/kernel/.git/worktrees/mfd/rebase-apply/patch:35:
> > > indent with spaces.
> > > label = "panel";
> > > warning: 2 lines add whitespace errors.
> > > Falling back to patching base and 3-way merge...
> > > Auto-merging Documentation/devicetree/bindings/mfd/iqs62x.yaml
> > > CONFLICT (content): Merge conflict in
> > > Documentation/devicetree/bindings/mfd/iqs62x.yaml
> > > Recorded preimage for 'Documentation/devicetree/bindings/mfd/iqs62x.yaml'
> >
> > It looks like the following patch already beat this to the punch:
> >
> > 8237e8382498 ("dt-bindings: mfd: Correct the node name of the panel LED")
>
> Which tree is that commit on? This one?
>
> https://git.kernel.org/pub/scm/linux/kernel/git/lee/mfd.git/
That's correct.
>
> > That patch does not retain the LED's label or rename the parent node to
> > led-controller, however. The label hardly matters for this example, but
> > perhaps we still want the parent node change to follow leds-pwm.yaml.
>
> Should I rework the patch then to have that change only?
That seems like a reasonable compromise.
>
> Greets
> Alex
>
> --
> /"\ ASCII RIBBON | »With the first link, the chain is forged. The first
> \ / CAMPAIGN | speech censured, the first thought forbidden, the
> X AGAINST | first freedom denied, chains us all irrevocably.«
> / \ HTML MAIL| (Jean-Luc Picard, quoting Judge Aaron Satie)
Kind regards,
Jeff LaBundy
[PATCH v2 6/6] mfd: iqs62x: Do not change clock frequency during ATI
After a reset event, the device automatically triggers ATI at the
default core clock frequency (16 MHz). Soon afterward, the driver
loads firmware which may attempt to lower the frequency.
If this initial ATI cycle is still in progress when the frequency
is changed, however, the device incorrectly reports channels 0, 1
and 2 to be in a state of touch once ATI finally completes.
To solve this problem, wait until ATI is complete before lowering
the frequency. Because this particular ATI cycle occurs following
a reset event, its duration is predictable and a simple delay can
suffice.
Signed-off-by: Jeff LaBundy
Acked-for-MFD-by: Lee Jones
---
Changes in v2:
- Added Acked-for-MFD-by trailer
drivers/mfd/iqs62x.c | 9 +
1 file changed, 9 insertions(+)
diff --git a/drivers/mfd/iqs62x.c b/drivers/mfd/iqs62x.c
index 9b5c389..d1fc38a 100644
--- a/drivers/mfd/iqs62x.c
+++ b/drivers/mfd/iqs62x.c
@@ -81,6 +81,7 @@
#define IQS62X_FW_REC_TYPE_MASK3
#define IQS62X_FW_REC_TYPE_DATA4
+#define IQS62X_ATI_STARTUP_MS 350
#define IQS62X_FILT_SETTLE_MS 250
struct iqs62x_fw_rec {
@@ -111,6 +112,14 @@ static int iqs62x_dev_init(struct iqs62x_core *iqs62x)
int ret;
list_for_each_entry(fw_blk, &iqs62x->fw_blk_head, list) {
+ /*
+* In case ATI is in progress, wait for it to complete before
+* lowering the core clock frequency.
+*/
+ if (fw_blk->addr == IQS62X_SYS_SETTINGS &&
+ *fw_blk->data & IQS62X_SYS_SETTINGS_CLK_DIV)
+ msleep(IQS62X_ATI_STARTUP_MS);
+
if (fw_blk->mask)
ret = regmap_update_bits(iqs62x->regmap, fw_blk->addr,
fw_blk->mask, *fw_blk->data);
--
2.7.4
[PATCH v2 5/6] mfd: iqs62x: Do not poll during ATI
After loading firmware, the driver triggers ATI (calibration) with
the newly loaded register configuration in place. Next, the driver
polls a register field to ensure ATI completed in a timely fashion
and that the device is ready to sense.
However, communicating with the device over I2C while ATI is under-
way may induce noise in the device and cause ATI to fail. As such,
the vendor recommends not to poll the device during ATI.
To solve this problem, let the device naturally signal to the host
that ATI is complete by way of an interrupt. A completion prevents
the sub-devices from being registered until this happens.
The former logic that scaled ATI timeout and filter settling delay
is not carried forward with the new implementation, as it produces
overly conservative delays at lower clock rates. Instead, a single
pair of delays that covers all cases is used.
Signed-off-by: Jeff LaBundy
---
Changes in v2:
- Removed superfluous newlines throughout all iqs62x_dev_desc structs
drivers/mfd/iqs62x.c | 125 +
include/linux/mfd/iqs62x.h | 11 ++--
2 files changed, 73 insertions(+), 63 deletions(-)
diff --git a/drivers/mfd/iqs62x.c b/drivers/mfd/iqs62x.c
index 07c9725..9b5c389 100644
--- a/drivers/mfd/iqs62x.c
+++ b/drivers/mfd/iqs62x.c
@@ -36,7 +36,6 @@
#define IQS62X_PROD_NUM0x00
#define IQS62X_SYS_FLAGS 0x10
-#define IQS62X_SYS_FLAGS_IN_ATIBIT(2)
#define IQS620_HALL_FLAGS 0x16
#define IQS621_HALL_FLAGS 0x19
@@ -60,6 +59,7 @@
#define IQS62X_SYS_SETTINGS_ACK_RESET BIT(6)
#define IQS62X_SYS_SETTINGS_EVENT_MODE BIT(5)
#define IQS62X_SYS_SETTINGS_CLK_DIVBIT(4)
+#define IQS62X_SYS_SETTINGS_COMM_ATI BIT(3)
#define IQS62X_SYS_SETTINGS_REDO_ATI BIT(1)
#define IQS62X_PWR_SETTINGS0xD2
@@ -81,9 +81,7 @@
#define IQS62X_FW_REC_TYPE_MASK3
#define IQS62X_FW_REC_TYPE_DATA4
-#define IQS62X_ATI_POLL_SLEEP_US 1
-#define IQS62X_ATI_POLL_TIMEOUT_US 50
-#define IQS62X_ATI_STABLE_DELAY_MS 150
+#define IQS62X_FILT_SETTLE_MS 250
struct iqs62x_fw_rec {
u8 type;
@@ -111,7 +109,6 @@ static int iqs62x_dev_init(struct iqs62x_core *iqs62x)
struct iqs62x_fw_blk *fw_blk;
unsigned int val;
int ret;
- u8 clk_div = 1;
list_for_each_entry(fw_blk, &iqs62x->fw_blk_head, list) {
if (fw_blk->mask)
@@ -181,28 +178,32 @@ static int iqs62x_dev_init(struct iqs62x_core *iqs62x)
return ret;
}
- ret = regmap_read(iqs62x->regmap, IQS62X_SYS_SETTINGS, &val);
- if (ret)
- return ret;
-
- if (val & IQS62X_SYS_SETTINGS_CLK_DIV)
- clk_div = iqs62x->dev_desc->clk_div;
-
- ret = regmap_write(iqs62x->regmap, IQS62X_SYS_SETTINGS, val |
- IQS62X_SYS_SETTINGS_ACK_RESET |
- IQS62X_SYS_SETTINGS_EVENT_MODE |
- IQS62X_SYS_SETTINGS_REDO_ATI);
- if (ret)
- return ret;
-
- ret = regmap_read_poll_timeout(iqs62x->regmap, IQS62X_SYS_FLAGS, val,
- !(val & IQS62X_SYS_FLAGS_IN_ATI),
- IQS62X_ATI_POLL_SLEEP_US,
- IQS62X_ATI_POLL_TIMEOUT_US * clk_div);
+ /*
+* Place the device in streaming mode at first so as not to miss the
+* limited number of interrupts that would otherwise occur after ATI
+* completes. The device is subsequently placed in event mode by the
+* interrupt handler.
+*
+* In the meantime, mask interrupts during ATI to prevent the device
+* from soliciting I2C traffic until the noise-sensitive ATI process
+* is complete.
+*/
+ ret = regmap_update_bits(iqs62x->regmap, IQS62X_SYS_SETTINGS,
+IQS62X_SYS_SETTINGS_ACK_RESET |
+IQS62X_SYS_SETTINGS_EVENT_MODE |
+IQS62X_SYS_SETTINGS_COMM_ATI |
+IQS62X_SYS_SETTINGS_REDO_ATI,
+IQS62X_SYS_SETTINGS_ACK_RESET |
+IQS62X_SYS_SETTINGS_REDO_ATI);
if (ret)
return ret;
- msleep(IQS62X_ATI_STABLE_DELAY_MS * clk_div);
+ /*
+* The following delay gives the device time to deassert its RDY output
+* in case a communication window was open while the REDO_ATI field was
+* written. This prevents an interrupt from being serviced prematurely.
+*/
+ usleep_range(5000, 5100);
return 0;
}
@@ -433,6 +434,11 @@ const struct iqs62x_eve
[PATCH v2 2/6] mfd: iqs62x: Remove unused bit mask
The register write that performed a mandatory soft reset during probe was removed during development of the driver, however the IQS62X_SYS_SETTINGS_SOFT_RESET bit mask was left behind. Remove it to keep stray macros out of the driver. Signed-off-by: Jeff LaBundy Acked-for-MFD-by: Lee Jones --- Changes in v2: - Added Acked-for-MFD-by trailer drivers/mfd/iqs62x.c | 1 - 1 file changed, 1 deletion(-) diff --git a/drivers/mfd/iqs62x.c b/drivers/mfd/iqs62x.c index ec4c790..ff968dc 100644 --- a/drivers/mfd/iqs62x.c +++ b/drivers/mfd/iqs62x.c @@ -57,7 +57,6 @@ #define IQS620_TEMP_CAL_OFFS 0xC4 #define IQS62X_SYS_SETTINGS0xD0 -#define IQS62X_SYS_SETTINGS_SOFT_RESET BIT(7) #define IQS62X_SYS_SETTINGS_ACK_RESET BIT(6) #define IQS62X_SYS_SETTINGS_EVENT_MODE BIT(5) #define IQS62X_SYS_SETTINGS_CLK_DIVBIT(4) -- 2.7.4
[PATCH v2 4/6] mfd: iqs62x: Increase interrupt handler return delay
The time the device takes to deassert its RDY output following an I2C stop condition scales with the core clock frequency. To prevent level-triggered interrupts from being reasserted after the interrupt handler returns, increase the time before returning to account for the worst-case delay (~90 us) plus margin. Signed-off-by: Jeff LaBundy Acked-for-MFD-by: Lee Jones --- Changes in v2: - Added Acked-for-MFD-by trailer drivers/mfd/iqs62x.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/mfd/iqs62x.c b/drivers/mfd/iqs62x.c index 7a1ff7c..07c9725 100644 --- a/drivers/mfd/iqs62x.c +++ b/drivers/mfd/iqs62x.c @@ -533,7 +533,7 @@ static irqreturn_t iqs62x_irq(int irq, void *context) * ensure the device's RDY output has been deasserted by the time the * interrupt handler returns. */ - usleep_range(50, 100); + usleep_range(150, 200); return IRQ_HANDLED; } -- 2.7.4
[PATCH v2 3/6] mfd: iqs62x: Rename regmap_config struct
The regmap member of the driver's private data is called 'regmap',
but the regmap_config struct is called 'iqs62x_map_config'. Rename
the latter to 'iqs62x_regmap_config' for consistency.
Signed-off-by: Jeff LaBundy
Acked-for-MFD-by: Lee Jones
---
Changes in v2:
- Added Acked-for-MFD-by trailer
drivers/mfd/iqs62x.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/mfd/iqs62x.c b/drivers/mfd/iqs62x.c
index ff968dc..7a1ff7c 100644
--- a/drivers/mfd/iqs62x.c
+++ b/drivers/mfd/iqs62x.c
@@ -866,7 +866,7 @@ static const struct iqs62x_dev_desc iqs62x_devs[] = {
},
};
-static const struct regmap_config iqs62x_map_config = {
+static const struct regmap_config iqs62x_regmap_config = {
.reg_bits = 8,
.val_bits = 8,
.max_register = IQS62X_MAX_REG,
@@ -892,7 +892,7 @@ static int iqs62x_probe(struct i2c_client *client)
INIT_LIST_HEAD(&iqs62x->fw_blk_head);
init_completion(&iqs62x->fw_done);
- iqs62x->regmap = devm_regmap_init_i2c(client, &iqs62x_map_config);
+ iqs62x->regmap = devm_regmap_init_i2c(client, &iqs62x_regmap_config);
if (IS_ERR(iqs62x->regmap)) {
ret = PTR_ERR(iqs62x->regmap);
dev_err(&client->dev, "Failed to initialize register map: %d\n",
--
2.7.4
[PATCH v2 1/6] mfd: iqs62x: Remove superfluous whitespace above fallthroughs
Previously, all instances of the /* fall through */ comment were
preceded by a newline to improve readability.
Now that /* fall through */ comments have been replaced with the
fallthrough pseudo-keyword, the leftover whitespace looks out of
place and can simply be removed.
Fixes: df561f6688fe ("treewide: Use fallthrough pseudo-keyword")
Signed-off-by: Jeff LaBundy
Acked-for-MFD-by: Lee Jones
---
Changes in v2:
- Added Acked-for-MFD-by trailer
drivers/mfd/iqs62x.c | 3 ---
1 file changed, 3 deletions(-)
diff --git a/drivers/mfd/iqs62x.c b/drivers/mfd/iqs62x.c
index 761b4ef..ec4c790 100644
--- a/drivers/mfd/iqs62x.c
+++ b/drivers/mfd/iqs62x.c
@@ -135,7 +135,6 @@ static int iqs62x_dev_init(struct iqs62x_core *iqs62x)
if (val & IQS620_PROX_SETTINGS_4_SAR_EN)
iqs62x->ui_sel = IQS62X_UI_SAR1;
-
fallthrough;
case IQS621_PROD_NUM:
@@ -469,7 +468,6 @@ static irqreturn_t iqs62x_irq(int irq, void *context)
switch (event_reg) {
case IQS62X_EVENT_UI_LO:
event_data.ui_data = get_unaligned_le16(&event_map[i]);
-
fallthrough;
case IQS62X_EVENT_UI_HI:
@@ -490,7 +488,6 @@ static irqreturn_t iqs62x_irq(int irq, void *context)
case IQS62X_EVENT_HYST:
event_map[i] <<= iqs62x->dev_desc->hyst_shift;
-
fallthrough;
case IQS62X_EVENT_WHEEL:
--
2.7.4
[PATCH v2 0/6] mfd: iqs62x: Minor cosmetic and functional improvements
This series includes a variety of minor cosmetic and functional improvements to the core support for the Azoteq IQS620A, IQS621, IQS622, IQS624 and IQS625 multi-function sensors. The first three patches are purely cosmetic, while the remaining three incorporate learnings during recent work with other Azoteq devices that have similar functions. Jeff LaBundy (6): mfd: iqs62x: Remove superfluous whitespace above fallthroughs mfd: iqs62x: Remove unused bit mask mfd: iqs62x: Rename regmap_config struct mfd: iqs62x: Increase interrupt handler return delay mfd: iqs62x: Do not poll during ATI mfd: iqs62x: Do not change clock frequency during ATI drivers/mfd/iqs62x.c | 144 + include/linux/mfd/iqs62x.h | 11 ++-- 2 files changed, 85 insertions(+), 70 deletions(-) -- 2.7.4
Re: [PATCH 5/6] mfd: iqs62x: Do not poll during ATI
Hi Lee,
Thank you for taking a look at the series.
On Thu, Jan 14, 2021 at 10:17:11AM +, Lee Jones wrote:
> On Sun, 03 Jan 2021, Jeff LaBundy wrote:
>
> > After loading firmware, the driver triggers ATI (calibration) with
> > the newly loaded register configuration in place. Next, the driver
> > polls a register field to ensure ATI completed in a timely fashion
> > and that the device is ready to sense.
> >
> > However, communicating with the device over I2C while ATI is under-
>
> This doesn't line-up with all of the others! ;)
:)
>
> > way may induce noise in the device and cause ATI to fail. As such,
> > the vendor recommends not to poll the device during ATI.
> >
> > To solve this problem, let the device naturally signal to the host
> > that ATI is complete by way of an interrupt. A completion prevents
> > the sub-devices from being registered until this happens.
> >
> > The former logic that scaled ATI timeout and filter settling delay
> > is not carried forward with the new implementation, as it produces
> > overly conservative delays at lower clock rates. Instead, a single
> > pair of delays that covers all cases is used.
> >
> > Signed-off-by: Jeff LaBundy
> > ---
> > drivers/mfd/iqs62x.c | 103
> > ++---
> > include/linux/mfd/iqs62x.h | 6 ++-
> > 2 files changed, 73 insertions(+), 36 deletions(-)
>
> [...]
>
> > @@ -567,6 +600,12 @@ static void iqs62x_firmware_load(const struct firmware
> > *fw, void *context)
> > goto err_out;
> > }
> >
> > + if (!wait_for_completion_timeout(&iqs62x->ati_done,
> > +msecs_to_jiffies(2000))) {
> > + dev_err(&client->dev, "Failed to complete ATI\n");
> > + goto err_out;
> > + }
> > +
> > ret = devm_mfd_add_devices(&client->dev, PLATFORM_DEVID_NONE,
> >iqs62x->dev_desc->sub_devs,
> >iqs62x->dev_desc->num_sub_devs,
> > @@ -763,7 +802,6 @@ static const struct iqs62x_dev_desc iqs62x_devs[] = {
> > .prox_settings = IQS620_PROX_SETTINGS_4,
> > .hall_flags = IQS620_HALL_FLAGS,
> >
> > - .clk_div= 4,
>
> No unnecessary double-line spacings please.
Not a problem, v2 on the way. There are a couple nearby instances of
double-spacing within these same structs so I'll nuke those as well.
[...]
Kind regards,
Jeff LaBundy
Re: [PATCH v9 1/4] dt-bindings: mfd: Fix schema warnings for pwm-leds
Hi Alexander,
On Thu, Jan 14, 2021 at 10:03:12AM +, Lee Jones wrote:
> On Mon, 28 Dec 2020, Alexander Dahl wrote:
>
> > The node names for devices using the pwm-leds driver follow a certain
> > naming scheme (now). Parent node name is not enforced, but recommended
> > by DT project.
> >
> > DTC Documentation/devicetree/bindings/mfd/iqs62x.example.dt.yaml
> > CHECK Documentation/devicetree/bindings/mfd/iqs62x.example.dt.yaml
> > /home/alex/build/linux/Documentation/devicetree/bindings/mfd/iqs62x.example.dt.yaml:
> > pwmleds: 'panel' does not match any of the regexes: '^led(-[0-9a-f]+)?$',
> > 'pinctrl-[0-9]+'
> > From schema:
> > /home/alex/src/linux/leds/Documentation/devicetree/bindings/leds/leds-pwm.yaml
> >
> > Signed-off-by: Alexander Dahl
> > Acked-by: Jeff LaBundy
> > Acked-by: Rob Herring
> > ---
> >
> > Notes:
> > v8 -> v9:
> > * added forgotten Acked-by (Jeff LaBundy)
> > * rebased on v5.11-rc1
> >
> > v7 -> v8:
> > * rebased on recent pavel/for-next (post v5.10-rc1)
> > * added Acked-by (Rob Herring)
> >
> > v6 -> v7:
> > * added warning message to commit message (Krzysztof Kozlowski)
> >
> > v6:
> > * added this patch to series
> >
> > Documentation/devicetree/bindings/mfd/iqs62x.yaml | 5 +++--
> > 1 file changed, 3 insertions(+), 2 deletions(-)
>
> Failed to apply:
>
> Applying: dt-bindings: mfd: Fix schema warnings for pwm-leds
> Using index info to reconstruct a base tree...
> M Documentation/devicetree/bindings/mfd/iqs62x.yaml
> /home/lee/projects/linux/kernel/.git/worktrees/mfd/rebase-apply/patch:34:
> indent with spaces.
> led-1 {
> /home/lee/projects/linux/kernel/.git/worktrees/mfd/rebase-apply/patch:35:
> indent with spaces.
> label = "panel";
> warning: 2 lines add whitespace errors.
> Falling back to patching base and 3-way merge...
> Auto-merging Documentation/devicetree/bindings/mfd/iqs62x.yaml
> CONFLICT (content): Merge conflict in
> Documentation/devicetree/bindings/mfd/iqs62x.yaml
> Recorded preimage for 'Documentation/devicetree/bindings/mfd/iqs62x.yaml'
It looks like the following patch already beat this to the punch:
8237e8382498 ("dt-bindings: mfd: Correct the node name of the panel LED")
That patch does not retain the LED's label or rename the parent node to
led-controller, however. The label hardly matters for this example, but
perhaps we still want the parent node change to follow leds-pwm.yaml.
>
> --
> Lee Jones [李琼斯]
> Senior Technical Lead - Developer Services
> Linaro.org │ Open source software for Arm SoCs
> Follow Linaro: Facebook | Twitter | Blog
Kind regards,
Jeff LaBundy
Re: [PATCH] Input: da7280 - protect OF match table with CONFIG_OF
Hi Dmitry,
On Sun, Jan 03, 2021 at 05:58:41PM -0800, Dmitry Torokhov wrote:
> Hi Jeff,
>
> On Sat, Dec 19, 2020 at 08:01:09PM -0600, Jeff LaBundy wrote:
> > Hi Dmitry,
> >
> > On Fri, Dec 18, 2020 at 04:49:48PM +, Roy Im wrote:
> > > On Friday, December 18, 2020 3:50 PM, Dmitry Torokhov wrote:
> > >
> > > > The OF match table is only used when OF is enabled.
> > > >
> > > > Fixes: cd3f609823a5 ("Input: new da7280 haptic driver")
> > > > Reported-by: kernel test robot
> > > > Signed-off-by: Dmitry Torokhov
> > > > ---
> > > > drivers/input/misc/da7280.c | 2 ++
> > > > 1 file changed, 2 insertions(+)
> > > >
> > > > diff --git a/drivers/input/misc/da7280.c b/drivers/input/misc/da7280.c
> > > > index 2f698a8c1d65..b08610d6e575 100644
> > > > --- a/drivers/input/misc/da7280.c
> > > > +++ b/drivers/input/misc/da7280.c
> > > > @@ -1300,11 +1300,13 @@ static int __maybe_unused da7280_resume(struct
> > > > device *dev)
> > > > return retval;
> > > > }
> > > >
> > > > +#ifdef CONFIG_OF
> > > > static const struct of_device_id da7280_of_match[] = {
> > > > { .compatible = "dlg,da7280", },
> > > > { }
> > > > };
> > > > MODULE_DEVICE_TABLE(of, da7280_of_match);
> > > > +#endif
> >
> > Just for my own understanding, would it not work just as well
> > to include of_device.h? This includes mod_devicetable.h which
> > in turn defines the of_device_id struct (even if CONFIG_OF is
> > not set).
>
> The issue here is not that the structure is undefined, but the variable
> is unused. We could also fix this by not using of_match_ptr() when
> assigning the match table to the driver structure, making the variable
> referenced even if CONFIG_OF is off.
ACK. The call to of_match_ptr() is what I was missing; the other
drivers I was looking at do not use it which must be why the bot
has not complained.
>
> >
> > The reason for asking is because it seems many drivers do not
> > include these guards.
>
> It could be that they are either only compiled with OF, or they decided
> it is not worth saving a few bytes, or maybe they are used on ACPI-based
> systems with PRP0001 bindings in which case the match table in the
> driver might still be needed.
Makes perfect sense; thank you for the follow-up.
>
> Thanks.
>
> --
> Dmitry
Kind regards,
Jeff LaBundy
[PATCH 2/6] mfd: iqs62x: Remove unused bit mask
The register write that performed a mandatory soft reset during probe was removed during development of the driver, however the IQS62X_SYS_SETTINGS_SOFT_RESET bit mask was left behind. Remove it to keep stray macros out of the driver. Signed-off-by: Jeff LaBundy --- drivers/mfd/iqs62x.c | 1 - 1 file changed, 1 deletion(-) diff --git a/drivers/mfd/iqs62x.c b/drivers/mfd/iqs62x.c index ec4c790..ff968dc 100644 --- a/drivers/mfd/iqs62x.c +++ b/drivers/mfd/iqs62x.c @@ -57,7 +57,6 @@ #define IQS620_TEMP_CAL_OFFS 0xC4 #define IQS62X_SYS_SETTINGS0xD0 -#define IQS62X_SYS_SETTINGS_SOFT_RESET BIT(7) #define IQS62X_SYS_SETTINGS_ACK_RESET BIT(6) #define IQS62X_SYS_SETTINGS_EVENT_MODE BIT(5) #define IQS62X_SYS_SETTINGS_CLK_DIVBIT(4) -- 2.7.4
[PATCH 4/6] mfd: iqs62x: Increase interrupt handler return delay
The time the device takes to deassert its RDY output following an I2C stop condition scales with the core clock frequency. To prevent level-triggered interrupts from being reasserted after the interrupt handler returns, increase the time before returning to account for the worst-case delay (~90 us) plus margin. Signed-off-by: Jeff LaBundy --- drivers/mfd/iqs62x.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/mfd/iqs62x.c b/drivers/mfd/iqs62x.c index 7a1ff7c..07c9725 100644 --- a/drivers/mfd/iqs62x.c +++ b/drivers/mfd/iqs62x.c @@ -533,7 +533,7 @@ static irqreturn_t iqs62x_irq(int irq, void *context) * ensure the device's RDY output has been deasserted by the time the * interrupt handler returns. */ - usleep_range(50, 100); + usleep_range(150, 200); return IRQ_HANDLED; } -- 2.7.4
[PATCH 3/6] mfd: iqs62x: Rename regmap_config struct
The regmap member of the driver's private data is called 'regmap',
but the regmap_config struct is called 'iqs62x_map_config'. Rename
the latter to 'iqs62x_regmap_config' for consistency.
Signed-off-by: Jeff LaBundy
---
drivers/mfd/iqs62x.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/mfd/iqs62x.c b/drivers/mfd/iqs62x.c
index ff968dc..7a1ff7c 100644
--- a/drivers/mfd/iqs62x.c
+++ b/drivers/mfd/iqs62x.c
@@ -866,7 +866,7 @@ static const struct iqs62x_dev_desc iqs62x_devs[] = {
},
};
-static const struct regmap_config iqs62x_map_config = {
+static const struct regmap_config iqs62x_regmap_config = {
.reg_bits = 8,
.val_bits = 8,
.max_register = IQS62X_MAX_REG,
@@ -892,7 +892,7 @@ static int iqs62x_probe(struct i2c_client *client)
INIT_LIST_HEAD(&iqs62x->fw_blk_head);
init_completion(&iqs62x->fw_done);
- iqs62x->regmap = devm_regmap_init_i2c(client, &iqs62x_map_config);
+ iqs62x->regmap = devm_regmap_init_i2c(client, &iqs62x_regmap_config);
if (IS_ERR(iqs62x->regmap)) {
ret = PTR_ERR(iqs62x->regmap);
dev_err(&client->dev, "Failed to initialize register map: %d\n",
--
2.7.4
[PATCH 6/6] mfd: iqs62x: Do not change clock frequency during ATI
After a reset event, the device automatically triggers ATI at the
default core clock frequency (16 MHz). Soon afterward, the driver
loads firmware which may attempt to lower the frequency.
If this initial ATI cycle is still in progress when the frequency
is changed, however, the device incorrectly reports channels 0, 1
and 2 to be in a state of touch once ATI finally completes.
To solve this problem, wait until ATI is complete before lowering
the frequency. Because this particular ATI cycle occurs following
a reset event, its duration is predictable and a simple delay can
suffice.
Signed-off-by: Jeff LaBundy
---
drivers/mfd/iqs62x.c | 9 +
1 file changed, 9 insertions(+)
diff --git a/drivers/mfd/iqs62x.c b/drivers/mfd/iqs62x.c
index 9fdf32f..3b32542 100644
--- a/drivers/mfd/iqs62x.c
+++ b/drivers/mfd/iqs62x.c
@@ -81,6 +81,7 @@
#define IQS62X_FW_REC_TYPE_MASK3
#define IQS62X_FW_REC_TYPE_DATA4
+#define IQS62X_ATI_STARTUP_MS 350
#define IQS62X_FILT_SETTLE_MS 250
struct iqs62x_fw_rec {
@@ -111,6 +112,14 @@ static int iqs62x_dev_init(struct iqs62x_core *iqs62x)
int ret;
list_for_each_entry(fw_blk, &iqs62x->fw_blk_head, list) {
+ /*
+* In case ATI is in progress, wait for it to complete before
+* lowering the core clock frequency.
+*/
+ if (fw_blk->addr == IQS62X_SYS_SETTINGS &&
+ *fw_blk->data & IQS62X_SYS_SETTINGS_CLK_DIV)
+ msleep(IQS62X_ATI_STARTUP_MS);
+
if (fw_blk->mask)
ret = regmap_update_bits(iqs62x->regmap, fw_blk->addr,
fw_blk->mask, *fw_blk->data);
--
2.7.4
[PATCH 5/6] mfd: iqs62x: Do not poll during ATI
After loading firmware, the driver triggers ATI (calibration) with
the newly loaded register configuration in place. Next, the driver
polls a register field to ensure ATI completed in a timely fashion
and that the device is ready to sense.
However, communicating with the device over I2C while ATI is under-
way may induce noise in the device and cause ATI to fail. As such,
the vendor recommends not to poll the device during ATI.
To solve this problem, let the device naturally signal to the host
that ATI is complete by way of an interrupt. A completion prevents
the sub-devices from being registered until this happens.
The former logic that scaled ATI timeout and filter settling delay
is not carried forward with the new implementation, as it produces
overly conservative delays at lower clock rates. Instead, a single
pair of delays that covers all cases is used.
Signed-off-by: Jeff LaBundy
---
drivers/mfd/iqs62x.c | 103 ++---
include/linux/mfd/iqs62x.h | 6 ++-
2 files changed, 73 insertions(+), 36 deletions(-)
diff --git a/drivers/mfd/iqs62x.c b/drivers/mfd/iqs62x.c
index 07c9725..9fdf32f 100644
--- a/drivers/mfd/iqs62x.c
+++ b/drivers/mfd/iqs62x.c
@@ -36,7 +36,6 @@
#define IQS62X_PROD_NUM0x00
#define IQS62X_SYS_FLAGS 0x10
-#define IQS62X_SYS_FLAGS_IN_ATIBIT(2)
#define IQS620_HALL_FLAGS 0x16
#define IQS621_HALL_FLAGS 0x19
@@ -60,6 +59,7 @@
#define IQS62X_SYS_SETTINGS_ACK_RESET BIT(6)
#define IQS62X_SYS_SETTINGS_EVENT_MODE BIT(5)
#define IQS62X_SYS_SETTINGS_CLK_DIVBIT(4)
+#define IQS62X_SYS_SETTINGS_COMM_ATI BIT(3)
#define IQS62X_SYS_SETTINGS_REDO_ATI BIT(1)
#define IQS62X_PWR_SETTINGS0xD2
@@ -81,9 +81,7 @@
#define IQS62X_FW_REC_TYPE_MASK3
#define IQS62X_FW_REC_TYPE_DATA4
-#define IQS62X_ATI_POLL_SLEEP_US 1
-#define IQS62X_ATI_POLL_TIMEOUT_US 50
-#define IQS62X_ATI_STABLE_DELAY_MS 150
+#define IQS62X_FILT_SETTLE_MS 250
struct iqs62x_fw_rec {
u8 type;
@@ -111,7 +109,6 @@ static int iqs62x_dev_init(struct iqs62x_core *iqs62x)
struct iqs62x_fw_blk *fw_blk;
unsigned int val;
int ret;
- u8 clk_div = 1;
list_for_each_entry(fw_blk, &iqs62x->fw_blk_head, list) {
if (fw_blk->mask)
@@ -181,28 +178,32 @@ static int iqs62x_dev_init(struct iqs62x_core *iqs62x)
return ret;
}
- ret = regmap_read(iqs62x->regmap, IQS62X_SYS_SETTINGS, &val);
- if (ret)
- return ret;
-
- if (val & IQS62X_SYS_SETTINGS_CLK_DIV)
- clk_div = iqs62x->dev_desc->clk_div;
-
- ret = regmap_write(iqs62x->regmap, IQS62X_SYS_SETTINGS, val |
- IQS62X_SYS_SETTINGS_ACK_RESET |
- IQS62X_SYS_SETTINGS_EVENT_MODE |
- IQS62X_SYS_SETTINGS_REDO_ATI);
- if (ret)
- return ret;
-
- ret = regmap_read_poll_timeout(iqs62x->regmap, IQS62X_SYS_FLAGS, val,
- !(val & IQS62X_SYS_FLAGS_IN_ATI),
- IQS62X_ATI_POLL_SLEEP_US,
- IQS62X_ATI_POLL_TIMEOUT_US * clk_div);
+ /*
+* Place the device in streaming mode at first so as not to miss the
+* limited number of interrupts that would otherwise occur after ATI
+* completes. The device is subsequently placed in event mode by the
+* interrupt handler.
+*
+* In the meantime, mask interrupts during ATI to prevent the device
+* from soliciting I2C traffic until the noise-sensitive ATI process
+* is complete.
+*/
+ ret = regmap_update_bits(iqs62x->regmap, IQS62X_SYS_SETTINGS,
+IQS62X_SYS_SETTINGS_ACK_RESET |
+IQS62X_SYS_SETTINGS_EVENT_MODE |
+IQS62X_SYS_SETTINGS_COMM_ATI |
+IQS62X_SYS_SETTINGS_REDO_ATI,
+IQS62X_SYS_SETTINGS_ACK_RESET |
+IQS62X_SYS_SETTINGS_REDO_ATI);
if (ret)
return ret;
- msleep(IQS62X_ATI_STABLE_DELAY_MS * clk_div);
+ /*
+* The following delay gives the device time to deassert its RDY output
+* in case a communication window was open while the REDO_ATI field was
+* written. This prevents an interrupt from being serviced prematurely.
+*/
+ usleep_range(5000, 5100);
return 0;
}
@@ -433,6 +434,11 @@ const struct iqs62x_event_desc
iqs62x_events[IQS62X_NUM_EVENTS] = {
.mask =
[PATCH 0/6] mfd: iqs62x: Minor cosmetic and functional improvements
This series includes a variety of minor cosmetic and functional improvements to the core support for the Azoteq IQS620A, IQS621, IQS622, IQS624 and IQS625 multi-function sensors. The first three patches are purely cosmetic, while the remaining three incorporate learnings during recent work with other Azoteq devices that have similar functions. Jeff LaBundy (6): mfd: iqs62x: Remove superfluous whitespace above fallthroughs mfd: iqs62x: Remove unused bit mask mfd: iqs62x: Rename regmap_config struct mfd: iqs62x: Increase interrupt handler return delay mfd: iqs62x: Do not poll during ATI mfd: iqs62x: Do not change clock frequency during ATI drivers/mfd/iqs62x.c | 122 ++--- include/linux/mfd/iqs62x.h | 6 ++- 2 files changed, 85 insertions(+), 43 deletions(-) -- 2.7.4
[PATCH 1/6] mfd: iqs62x: Remove superfluous whitespace above fallthroughs
Previously, all instances of the /* fall through */ comment were
preceded by a newline to improve readability.
Now that /* fall through */ comments have been replaced with the
fallthrough pseudo-keyword, the leftover whitespace looks out of
place and can simply be removed.
Fixes: df561f6688fe ("treewide: Use fallthrough pseudo-keyword")
Signed-off-by: Jeff LaBundy
---
drivers/mfd/iqs62x.c | 3 ---
1 file changed, 3 deletions(-)
diff --git a/drivers/mfd/iqs62x.c b/drivers/mfd/iqs62x.c
index 761b4ef..ec4c790 100644
--- a/drivers/mfd/iqs62x.c
+++ b/drivers/mfd/iqs62x.c
@@ -135,7 +135,6 @@ static int iqs62x_dev_init(struct iqs62x_core *iqs62x)
if (val & IQS620_PROX_SETTINGS_4_SAR_EN)
iqs62x->ui_sel = IQS62X_UI_SAR1;
-
fallthrough;
case IQS621_PROD_NUM:
@@ -469,7 +468,6 @@ static irqreturn_t iqs62x_irq(int irq, void *context)
switch (event_reg) {
case IQS62X_EVENT_UI_LO:
event_data.ui_data = get_unaligned_le16(&event_map[i]);
-
fallthrough;
case IQS62X_EVENT_UI_HI:
@@ -490,7 +488,6 @@ static irqreturn_t iqs62x_irq(int irq, void *context)
case IQS62X_EVENT_HYST:
event_map[i] <<= iqs62x->dev_desc->hyst_shift;
-
fallthrough;
case IQS62X_EVENT_WHEEL:
--
2.7.4
Re: [PATCH] Input: da7280 - protect OF match table with CONFIG_OF
Hi Dmitry,
On Fri, Dec 18, 2020 at 04:49:48PM +, Roy Im wrote:
> On Friday, December 18, 2020 3:50 PM, Dmitry Torokhov wrote:
>
> > The OF match table is only used when OF is enabled.
> >
> > Fixes: cd3f609823a5 ("Input: new da7280 haptic driver")
> > Reported-by: kernel test robot
> > Signed-off-by: Dmitry Torokhov
> > ---
> > drivers/input/misc/da7280.c | 2 ++
> > 1 file changed, 2 insertions(+)
> >
> > diff --git a/drivers/input/misc/da7280.c b/drivers/input/misc/da7280.c
> > index 2f698a8c1d65..b08610d6e575 100644
> > --- a/drivers/input/misc/da7280.c
> > +++ b/drivers/input/misc/da7280.c
> > @@ -1300,11 +1300,13 @@ static int __maybe_unused da7280_resume(struct
> > device *dev)
> > return retval;
> > }
> >
> > +#ifdef CONFIG_OF
> > static const struct of_device_id da7280_of_match[] = {
> > { .compatible = "dlg,da7280", },
> > { }
> > };
> > MODULE_DEVICE_TABLE(of, da7280_of_match);
> > +#endif
Just for my own understanding, would it not work just as well
to include of_device.h? This includes mod_devicetable.h which
in turn defines the of_device_id struct (even if CONFIG_OF is
not set).
The reason for asking is because it seems many drivers do not
include these guards.
> >
> > static const struct i2c_device_id da7280_i2c_id[] = {
> > { "da7280", },
> > --
> > 2.29.2.729.g45daf8777d-goog
> >
> >
> > --
> > Dmitry
>
> Thanks!
>
> Acked-by: Roy Im
>
Kind regards,
Jeff LaBundy
Re: [PATCH v7 03/12] dt-bindings: mfd: Fix schema warnings for pwm-leds
Hi Alexander,
On Mon, Oct 05, 2020 at 10:34:42PM +0200, Alexander Dahl wrote:
> The node names for devices using the pwm-leds driver follow a certain
> naming scheme (now). Parent node name is not enforced, but recommended
> by DT project.
>
> DTC Documentation/devicetree/bindings/mfd/iqs62x.example.dt.yaml
> CHECK Documentation/devicetree/bindings/mfd/iqs62x.example.dt.yaml
> /home/alex/build/linux/Documentation/devicetree/bindings/mfd/iqs62x.example.dt.yaml:
> pwmleds: 'panel' does not match any of the regexes: '^led(-[0-9a-f]+)?$',
> 'pinctrl-[0-9]+'
> From schema:
> /home/alex/src/linux/leds/Documentation/devicetree/bindings/leds/leds-pwm.yaml
>
> Signed-off-by: Alexander Dahl
> ---
>
> Notes:
> v6 -> v7:
> * added warning message to commit message (Krzysztof Kozlowski)
>
> v6:
> * added this patch to series
>
> Documentation/devicetree/bindings/mfd/iqs62x.yaml | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/mfd/iqs62x.yaml
> b/Documentation/devicetree/bindings/mfd/iqs62x.yaml
> index 541b06d80e73..92dc48a8dfa7 100644
> --- a/Documentation/devicetree/bindings/mfd/iqs62x.yaml
> +++ b/Documentation/devicetree/bindings/mfd/iqs62x.yaml
> @@ -90,10 +90,11 @@ examples:
> };
> };
>
> -pwmleds {
> +led-controller {
> compatible = "pwm-leds";
>
> -panel {
> +led-1 {
> +label = "panel";
> pwms = <&iqs620a_pwm 0 100>;
> max-brightness = <255>;
> };
> --
> 2.20.1
>
I like the consistency this brings. My only feedback is that in the other
examples I found (common.yaml and leds-gpio.yaml), the children count off
from 0 (e.g. led-0) instead of 1 as your series appears to.
That's not a huge deal; it simply seems more consistent to count from the
first index allowed by the regex (0). The patch is still fine, and so:
Acked-by: Jeff LaBundy
Kind regards,
Jeff LaBundy
Re: [PATCH] iio: light: iqs621: remove usage of iio_priv_to_dev()
Hi Alexandru,
On Fri, May 22, 2020 at 09:54:42AM +0300, Alexandru Ardelean wrote:
> We may want to get rid of the iio_priv_to_dev() helper. That's a bit
> uncertain at this point. The reason is that we will hide some of the
> members of the iio_dev structure (to prevent drivers from accessing them
> directly), and that will also mean hiding the implementation of the
> iio_priv_to_dev() helper inside the IIO core.
>
> Hiding the implementation of iio_priv_to_dev() implies that some fast-paths
> may not be fast anymore, so a general idea is to try to get rid of the
> iio_priv_to_dev() altogether.
>
> For this driver, removing iio_priv_to_dev() means keeping a reference
> on the state struct.
>
> Signed-off-by: Alexandru Ardelean
> ---
> drivers/iio/light/iqs621-als.c | 6 --
> 1 file changed, 4 insertions(+), 2 deletions(-)
As with the iqs624 patch:
Acked-by: Jeff LaBundy
>
> diff --git a/drivers/iio/light/iqs621-als.c b/drivers/iio/light/iqs621-als.c
> index b2988a782bd0..1a056e2446ab 100644
> --- a/drivers/iio/light/iqs621-als.c
> +++ b/drivers/iio/light/iqs621-als.c
> @@ -36,6 +36,7 @@
>
> struct iqs621_als_private {
> struct iqs62x_core *iqs62x;
> + struct iio_dev *indio_dev;
> struct notifier_block notifier;
> struct mutex lock;
> bool light_en;
> @@ -103,7 +104,7 @@ static int iqs621_als_notifier(struct notifier_block
> *notifier,
>
> iqs621_als = container_of(notifier, struct iqs621_als_private,
> notifier);
> - indio_dev = iio_priv_to_dev(iqs621_als);
> + indio_dev = iqs621_als->indio_dev;
> timestamp = iio_get_time_ns(indio_dev);
>
> mutex_lock(&iqs621_als->lock);
> @@ -191,7 +192,7 @@ static int iqs621_als_notifier(struct notifier_block
> *notifier,
> static void iqs621_als_notifier_unregister(void *context)
> {
> struct iqs621_als_private *iqs621_als = context;
> - struct iio_dev *indio_dev = iio_priv_to_dev(iqs621_als);
> + struct iio_dev *indio_dev = iqs621_als->indio_dev;
> int ret;
>
> ret = blocking_notifier_chain_unregister(&iqs621_als->iqs62x->nh,
> @@ -551,6 +552,7 @@ static int iqs621_als_probe(struct platform_device *pdev)
>
> iqs621_als = iio_priv(indio_dev);
> iqs621_als->iqs62x = iqs62x;
> + iqs621_als->indio_dev = indio_dev;
>
> if (iqs62x->dev_desc->prod_num == IQS622_PROD_NUM) {
> ret = regmap_read(iqs62x->regmap, IQS622_IR_THRESH_TOUCH,
> --
> 2.25.1
>
Kind regards,
Jeff LaBundy
Re: [PATCH] iio: position: iqs624: remove usage of iio_priv_to_dev()
Hi Alexandru,
On Fri, May 22, 2020 at 09:53:22AM +0300, Alexandru Ardelean wrote:
> We may want to get rid of the iio_priv_to_dev() helper. That's a bit
> uncertain at this point. The reason is that we will hide some of the
> members of the iio_dev structure (to prevent drivers from accessing them
> directly), and that will also mean hiding the implementation of the
> iio_priv_to_dev() helper inside the IIO core.
>
> Hiding the implementation of iio_priv_to_dev() implies that some fast-paths
> may not be fast anymore, so a general idea is to try to get rid of the
> iio_priv_to_dev() altogether.
>
> For this driver, removing iio_priv_to_dev() also means keeping a reference
> on the state struct.
>
> Signed-off-by: Alexandru Ardelean
> ---
> drivers/iio/position/iqs624-pos.c | 6 --
> 1 file changed, 4 insertions(+), 2 deletions(-)
As a customer of iio, I find it handy that there is an "inverse" to iio_priv.
In this particular case it saves the container iio_dev from storing a pointer
to itself.
That being said, this patch is perfectly fine and I have no objection if this
is the route you and Jonathan opt to take. And so:
Acked-by: Jeff LaBundy
>
> diff --git a/drivers/iio/position/iqs624-pos.c
> b/drivers/iio/position/iqs624-pos.c
> index 77096c31c2ba..520dafbdc48f 100644
> --- a/drivers/iio/position/iqs624-pos.c
> +++ b/drivers/iio/position/iqs624-pos.c
> @@ -23,6 +23,7 @@
>
> struct iqs624_pos_private {
> struct iqs62x_core *iqs62x;
> + struct iio_dev *indio_dev;
> struct notifier_block notifier;
> struct mutex lock;
> bool angle_en;
> @@ -59,7 +60,7 @@ static int iqs624_pos_notifier(struct notifier_block
> *notifier,
>
> iqs624_pos = container_of(notifier, struct iqs624_pos_private,
> notifier);
> - indio_dev = iio_priv_to_dev(iqs624_pos);
> + indio_dev = iqs624_pos->indio_dev;
> timestamp = iio_get_time_ns(indio_dev);
>
> iqs62x = iqs624_pos->iqs62x;
> @@ -98,7 +99,7 @@ static int iqs624_pos_notifier(struct notifier_block
> *notifier,
> static void iqs624_pos_notifier_unregister(void *context)
> {
> struct iqs624_pos_private *iqs624_pos = context;
> - struct iio_dev *indio_dev = iio_priv_to_dev(iqs624_pos);
> + struct iio_dev *indio_dev = iqs624_pos->indio_dev;
> int ret;
>
> ret = blocking_notifier_chain_unregister(&iqs624_pos->iqs62x->nh,
> @@ -243,6 +244,7 @@ static int iqs624_pos_probe(struct platform_device *pdev)
>
> iqs624_pos = iio_priv(indio_dev);
> iqs624_pos->iqs62x = iqs62x;
> + iqs624_pos->indio_dev = indio_dev;
>
> indio_dev->modes = INDIO_DIRECT_MODE;
> indio_dev->dev.parent = &pdev->dev;
> --
> 2.25.1
>
Kind regards,
Jeff LaBundy
Re: linux-next: Tree for May 18 (input/misc/iqs269a.c & regmap)
Hi Randy et al, On Mon, May 18, 2020 at 08:42:43AM -0700, Randy Dunlap wrote: > On 5/18/20 3:57 AM, Stephen Rothwell wrote: > > Hi all, > > > > Changes since 20200515: > > > > on i386: > > > CONFIG_REGMAP_I2C=y > CONFIG_I2C=m > > WARNING: unmet direct dependencies detected for REGMAP_I2C > Depends on [m]: I2C [=m] > Selected by [y]: > - INPUT_IQS269A [=y] && !UML && INPUT [=y] && INPUT_MISC [=y] > > > ld: drivers/base/regmap/regmap-i2c.o: in function > `regmap_smbus_byte_reg_read': > regmap-i2c.c:(.text+0x192): undefined reference to `i2c_smbus_read_byte_data' > ld: drivers/base/regmap/regmap-i2c.o: in function > `regmap_smbus_byte_reg_write': > regmap-i2c.c:(.text+0x1d7): undefined reference to `i2c_smbus_write_byte_data' > ld: drivers/base/regmap/regmap-i2c.o: in function > `regmap_smbus_word_reg_read': > regmap-i2c.c:(.text+0x202): undefined reference to `i2c_smbus_read_word_data' > ld: drivers/base/regmap/regmap-i2c.o: in function > `regmap_smbus_word_read_swapped': > regmap-i2c.c:(.text+0x242): undefined reference to `i2c_smbus_read_word_data' > ld: drivers/base/regmap/regmap-i2c.o: in function > `regmap_smbus_word_write_swapped': > regmap-i2c.c:(.text+0x2a1): undefined reference to `i2c_smbus_write_word_data' > ld: drivers/base/regmap/regmap-i2c.o: in function > `regmap_smbus_word_reg_write': > regmap-i2c.c:(.text+0x2d7): undefined reference to `i2c_smbus_write_word_data' > ld: drivers/base/regmap/regmap-i2c.o: in function > `regmap_i2c_smbus_i2c_read_reg16': > regmap-i2c.c:(.text+0x310): undefined reference to `i2c_smbus_write_byte_data' > ld: regmap-i2c.c:(.text+0x323): undefined reference to `i2c_smbus_read_byte' > ld: drivers/base/regmap/regmap-i2c.o: in function > `regmap_i2c_smbus_i2c_write_reg16': > regmap-i2c.c:(.text+0x39c): undefined reference to > `i2c_smbus_write_i2c_block_data' > ld: drivers/base/regmap/regmap-i2c.o: in function > `regmap_i2c_smbus_i2c_write': > regmap-i2c.c:(.text+0x3db): undefined reference to > `i2c_smbus_write_i2c_block_data' > ld: drivers/base/regmap/regmap-i2c.o: in function `regmap_i2c_smbus_i2c_read': > regmap-i2c.c:(.text+0x427): undefined reference to > `i2c_smbus_read_i2c_block_data' > ld: drivers/base/regmap/regmap-i2c.o: in function `regmap_i2c_read': > regmap-i2c.c:(.text+0x49f): undefined reference to `i2c_transfer' > ld: drivers/base/regmap/regmap-i2c.o: in function `regmap_i2c_gather_write': > regmap-i2c.c:(.text+0x524): undefined reference to `i2c_transfer' > ld: drivers/base/regmap/regmap-i2c.o: in function `regmap_i2c_write': > regmap-i2c.c:(.text+0x56c): undefined reference to `i2c_transfer_buffer_flags' > ld: drivers/input/misc/iqs269a.o: in function `iqs269_i2c_driver_init': > iqs269a.c:(.init.text+0xb): undefined reference to `i2c_register_driver' > ld: drivers/input/misc/iqs269a.o: in function `iqs269_i2c_driver_exit': > iqs269a.c:(.exit.text+0x9): undefined reference to `i2c_del_driver' > > > > Full randconfig file is attached. A complete oversight on my part; during my testing I did not realize another module was selecting I2C for me. Valuable lesson learned :) The kbuild test robot set off the alarm bells earlier today and I've sent a patch [1] already. Many apologies for all of the noise. > > -- > ~Randy > Reported-by: Randy Dunlap [1] https://patchwork.kernel.org/patch/11555469/ Kind regards, Jeff LaBundy
