On Thu, 1 Nov 2018 14:43:13 -0700
Enrico Granata <egranata@xxxxxxxxxx> wrote:
> From: Enrico Granata <egranata@xxxxxxxxxxxx>
>
> This driver allows interfacing with the Semtech sx9320, sx9321 and sx9324
> proximity sensors via the IIO subsystem.
>
> Signed-off-by: Enrico Granata <egranata@xxxxxxxxxxxx>
Various comments inline. Please make sure to proof read your patches
before sending. This one picked up some changes you definitely didn't
mean to make int the build files!
There is a mass of non standard ABI in here. It all needs documenting
in Documentation/ABI/testing/sysfs-bus-iio-* as appropriate.
Is there a datasheet available somewhere? It would make life easier.
I think this is a device with it's own clock / sequencer and that we need
to enable channels before their events work? That would explain
some of the complexity in here.. I'm not yet convinced the reference
counting logic makes sense so want more information on the constraints.
You may have it right, but without the datasheet (link below doesn't work)
it's hard to know what we 'need' to happen.
There is quite a lot of non standard ABI that probably just wants to be
removed.
> ---
> .../bindings/iio/proximity/sx932x.txt | 54 +
> drivers/iio/proximity/Kconfig | 36 +-
> drivers/iio/proximity/Makefile | 1 +
> drivers/iio/proximity/sx932x.c | 1462 +++++++++++++++++
> 4 files changed, 1542 insertions(+), 11 deletions(-)
> create mode 100644 Documentation/devicetree/bindings/iio/proximity/sx932x.txt
I know we have a number of wild card named drivers in tree (with weird historical
or compatibility reasons for most of them) but generally we don't use wild cards
in naming. It goes wrong far too often when the company things it'll be smart
to use other names in the range we thought was ours!
So pick a part and name it after that. sx9320.txt etc.
Also as you have a devicetree binding in here you should cc the relevant
maintainers and list. +cc Rob, Mark and List.
> create mode 100644 drivers/iio/proximity/sx932x.c
>
> diff --git a/Documentation/devicetree/bindings/iio/proximity/sx932x.txt b/Documentation/devicetree/bindings/iio/proximity/sx932x.txt
> new file mode 100644
> index 0000000000000..e1f1fefdbca08
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/proximity/sx932x.txt
> @@ -0,0 +1,54 @@
> +Semtech's SX9320/SX9321/SX9324 capacitive proximity button device driver
> +
> +Required properties:
> + - compatible: must be "semtech,sx9320 or sx9321 or sx9324"
One compatible per line and fully stated with prefix for each one.
> + - reg: i2c address where to find the device
> + - interrupt-parent : should be the phandle for the interrupt controller
> + - interrupts : the sole interrupt generated by the device
> + - semtech,register-prefix: the string prefixing all register names in the
I'm lost on what this one is.
> +
> + Refer to interrupt-controller/interrupts.txt for generic
> + interrupt client node bindings.
> +
> +Optional properties:
> + - semtech,adv_ctrl10: Initial value for register AdvCtrl10.
> + - semtech,afe_ctrl4: Initial value for register AfeCtrl4.
> + - semtech,afe_ctrl7: Initial value for register AfeCtrl7.
> + - semtech,afe_ctrl9: Initial value for register AfeCtrl9.
> + - semtech,prox_ctrl0: Initial value for register ProxCtrl0.
> + - semtech,prox_ctrl1: Initial value for register ProxCtrl1.
> + - semtech,prox_ctrl2: Initial value for register ProxCtrl2.
> + - semtech,prox_ctrl4: Initial value for register ProxCtrl4.
> + - semtech,prox_ctrl5: Initial value for register ProxCtrl5.
> + - semtech,prox_ctrl6: Initial value for register ProxCtrl6.
> + - semtech,prox_ctrl7: Initial value for register Prox_Trl7.
> + - semtech,adv_ctrl16: Initial value for register AdvCtrl16.
> + - semtech,adv_ctrl17: Initial value for register AdvCtrl17.
> + - semtech,adv_ctrl18: Initial value for register AdvCtrl18.
> + - semtech,gnrl_ctrl1: Initial value for register GnrlCtrl1.
Bindings should never include raw register addresses. If there are some
things in those registers that need controlling please break them out
as human readable elements so we know what is being set.
As these are optional I suspect you can just drop the lot.
> +
> + Refer to the data sheet for register definitions.
> +
> +Example:
> +
> +sx9310@28 {
Hmm. Not sure we have a generic name for a proximity sensor but
proximity@28 {
would seems sensible.
> + compatible = "semtech,sx9320";
> + reg = <0x28>;
> + interrupt-parent = <&gpio2>;
> + interrupts = <16 IRQ_TYPE_LEVEL_LOW>;
> +};
> +
> +Driver supports ACPI bindings. We can store registers configuration obtained
While interesting, doesn't belong in this binding file.
> +from per-model SAR sensor calibration. Refer to the data sheet for meaning.
> +
> +ACPI name is STH9320 or STH9321 or STH9324.
> +
> +Example:
> + chip drivers/i2c/generic
> + register "hid" = ""STH9321""
> + register "name" = ""SEMTECH SX9321""
> + register "desc" = ""SAR Proximity Sensor""
> + register "irq" = "ACPI_IRQ_LEVEL_LOW(GPP_A18_IRQ)"
> + register "device_present_gpio" = "GPP_B20"
> + device i2c 28 on end
> + end
> diff --git a/drivers/iio/proximity/Kconfig b/drivers/iio/proximity/Kconfig
> index b99367a89f810..3b979b8198d7c 100644
> --- a/drivers/iio/proximity/Kconfig
> +++ b/drivers/iio/proximity/Kconfig
> @@ -66,31 +66,45 @@ config SRF04
> To compile this driver as a module, choose M here: the
> module will be called srf04.
>
> -config SX9500
> - tristate "SX9500 Semtech proximity sensor"
> +config SRF08
> + tristate "Devantech SRF02/SRF08/SRF10 ultrasonic ranger sensor"
> + select IIO_BUFFER
> + select IIO_TRIGGERED_BUFFER
> + depends on I2C
> + help
> + Say Y here to build a driver for Devantech SRF02/SRF08/SRF10
> + ultrasonic ranger sensors with i2c interface.
> + This driver can be used to measure the distance of objects.
> +
> + To compile this driver as a module, choose M here: the
> + module will be called srf08.
??? Stray blocks here!
Please make sure you aren't killing off other drivers!
> +
> +config SX932X
> + tristate "sx932x Semtech proximity sensor"
> select IIO_BUFFER
> select IIO_TRIGGERED_BUFFER
> select REGMAP_I2C
> depends on I2C
> help
> - Say Y here to build a driver for Semtech's SX9500 capacitive
> - proximity/button sensor.
> + Say Y here to build a driver for Semtech's sx932x capacitive
> + proximity/button sensor. This driver supports Semtech's
> + sx9320, sx9321 and sx9324 sensors.
>
> To compile this driver as a module, choose M here: the
> - module will be called sx9500.
> + module will be called sx932x.
>
> -config SRF08
> - tristate "Devantech SRF02/SRF08/SRF10 ultrasonic ranger sensor"
> +config SX9500
> + tristate "SX9500 Semtech proximity sensor"
> select IIO_BUFFER
> select IIO_TRIGGERED_BUFFER
> + select REGMAP_I2C
> depends on I2C
> help
> - Say Y here to build a driver for Devantech SRF02/SRF08/SRF10
> - ultrasonic ranger sensors with i2c interface.
> - This driver can be used to measure the distance of objects.
> + Say Y here to build a driver for Semtech's SX9500 capacitive
> + proximity/button sensor.
>
> To compile this driver as a module, choose M here: the
> - module will be called srf08.
> + module will be called sx9500.
>
> config VL53L0X_I2C
> tristate "STMicroelectronics VL53L0X ToF ranger sensor (I2C)"
> diff --git a/drivers/iio/proximity/Makefile b/drivers/iio/proximity/Makefile
> index 6d031f903c4c2..65ac237d63dd6 100644
> --- a/drivers/iio/proximity/Makefile
> +++ b/drivers/iio/proximity/Makefile
> @@ -10,6 +10,7 @@ obj-$(CONFIG_LIDAR_LITE_V2) += pulsedlight-lidar-lite-v2.o
> obj-$(CONFIG_RFD77402) += rfd77402.o
> obj-$(CONFIG_SRF04) += srf04.o
> obj-$(CONFIG_SRF08) += srf08.o
> +obj-$(CONFIG_SX932X) += sx932x.o
> obj-$(CONFIG_SX9500) += sx9500.o
> obj-$(CONFIG_VL53L0X_I2C) += vl53l0x-i2c.o
>
> diff --git a/drivers/iio/proximity/sx932x.c b/drivers/iio/proximity/sx932x.c
> new file mode 100644
> index 0000000000000..069d3678f6bc9
> --- /dev/null
> +++ b/drivers/iio/proximity/sx932x.c
> @@ -0,0 +1,1462 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright 2018 Google LLC.
> + *
> + * Driver for Semtech's SX9320 capacitive proximity/button solution.
> + * Datasheet available at
> + * <http://www.semtech.com/images/datasheet/sx9320.pdf>.
Sadly not it seems...
> + * Based on SX9500 driver and Semtech driver using the input framework
> + * <https://my.syncplicity.com/share/teouwsim8niiaud/
> + * linux-driver-SX9320_NoSmartHSensing>.
I'm not 100% sure what this device actually is. It might be better off
as an input driver... Depends on just how much info it gives and how
that maps to input.
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms of the GNU General Public License version 2 as published by
> + * the Free Software Foundation.
If using an SPDX header it is generally considered that there is no need to
repeat the license statement.
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/slab.h>
> +#include <linux/module.h>
> +#include <linux/i2c.h>
> +#include <linux/irq.h>
> +#include <linux/acpi.h>
> +#include <linux/gpio/consumer.h>
> +#include <linux/regmap.h>
> +#include <linux/pm.h>
> +#include <linux/delay.h>
If no reason to do otherwise, it's preferred to have headers in alphabetical
order. Absolutely fine to have the separate block for iio headers though.
> +
> +#include <linux/iio/iio.h>
> +#include <linux/iio/buffer.h>
> +#include <linux/iio/sysfs.h>
> +#include <linux/iio/events.h>
> +#include <linux/iio/trigger.h>
> +#include <linux/iio/triggered_buffer.h>
> +#include <linux/iio/trigger_consumer.h>
> +
> +#define SX932X_DRIVER_NAME "sx932x"
After removing the wrong usage of this I've marked you should only have
the one place where it is used. Just put it directly there.
The define just makes it harder to know where this is used.
> +#define SX9320_ACPI_NAME "STH9320"
> +#define SX9321_ACPI_NAME "STH9321"
> +#define SX9324_ACPI_NAME "STH9324"
I'd expect to only see these in the binding, not up here as defines.
> +#define SX932X_DEV_NB 2
I'm relieved this isn't used! Please remove.
> +
> +#define SX932X_IRQ_NAME "sx932x_event"
Put it inline.
> +
> +#define SX932X_GPIO_INT "interrupt"
This is worrying. We shouldn't care that it is a gpio inside the
driver. Ah, we don't as this is unused. Please delete.
> +
> +/* Register definitions. */
> +#define SX932X_REG_IRQ_SRC 0x00
> +#define SX932X_REG_STAT0 0x01
> +#define SX932X_REG_STAT1 0x02
> +#define SX932X_REG_STAT2 0x03
> +#define SX932X_REG_STAT3 0x04
> +#define SX932X_REG_IRQ_MSK 0x05
> +#define SX932X_REG_IRQ_CFG0 0x06
> +#define SX932X_REG_IRQ_CFG2 0x07
> +
> +#define SX932X_REG_GNRL_CTRL0 0x10
> +#define SX932X_REG_GNRL_CTRL1 0x11
> +
> +#define SX932X_REG_I2C_ADDR 0x14
> +#define SX932X_REG_CLK_SPRD 0x15
> +
> +#define SX932X_REG_AFE_CTRL0 0x20
> +#define SX932X_REG_AFE_CTRL1 0x21
> +#define SX932X_REG_AFE_CTRL2 0x22
> +#define SX932X_REG_AFE_CTRL3 0x23
> +#define SX932X_REG_AFE_CTRL4 0x24
> +#define SX932X_REG_AFE_CTRL5 0x25
> +#define SX932X_REG_AFE_CTRL6 0x26
> +#define SX932X_REG_AFE_CTRL7 0x27
> +#define SX932X_REG_AFE_PH0 0x28
> +#define SX932X_REG_AFE_PH1 0x29
> +#define SX932X_REG_AFE_PH2 0x2a
> +#define SX932X_REG_AFE_PH3 0x2b
> +#define SX932X_REG_AFE_CTRL8 0x2c
> +#define SX932X_REG_AFE_CTRL9 0x2d
> +
> +#define SX932X_REG_PROX_CTRL0 0x30
> +#define SX932X_REG_PROX_CTRL1 0x31
> +#define SX932X_REG_PROX_CTRL2 0x32
> +#define SX932X_REG_PROX_CTRL3 0x33
> +#define SX932X_REG_PROX_CTRL4 0x34
> +#define SX932X_REG_PROX_CTRL5 0x35
> +#define SX932X_REG_PROX_CTRL6 0x36
> +#define SX932X_REG_PROX_CTRL7 0x37
> +
> +#define SX932X_REG_ADV_CTRL0 0x40
> +#define SX932X_REG_ADV_CTRL1 0x41
> +#define SX932X_REG_ADV_CTRL2 0x42
> +#define SX932X_REG_ADV_CTRL3 0x43
> +#define SX932X_REG_ADV_CTRL4 0x44
> +#define SX932X_REG_ADV_CTRL5 0x45
> +#define SX932X_REG_ADV_CTRL6 0x46
> +#define SX932X_REG_ADV_CTRL7 0x47
> +#define SX932X_REG_ADV_CTRL8 0x48
> +#define SX932X_REG_ADV_CTRL9 0x49
> +#define SX932X_REG_ADV_CTRL10 0x4a
> +#define SX932X_REG_ADV_CTRL11 0x4b
> +#define SX932X_REG_ADV_CTRL12 0x4c
> +#define SX932X_REG_ADV_CTRL13 0x4d
> +#define SX932X_REG_ADV_CTRL14 0x4e
> +#define SX932X_REG_ADV_CTRL15 0x4f
> +#define SX932X_REG_ADV_CTRL16 0x50
> +#define SX932X_REG_ADV_CTRL17 0x51
> +#define SX932X_REG_ADV_CTRL18 0x52
> +#define SX932X_REG_ADV_CTRL19 0x53
> +#define SX932X_REG_ADV_CTRL20 0x54
> +
> +#define SX932X_REG_PHASE_SEL 0x60
> +
> +#define SX932X_REG_USE_MSB 0x61
> +#define SX932X_REG_USE_LSB 0x62
> +
> +#define SX932X_REG_AVG_MSB 0x63
> +#define SX932X_REG_AVG_LSB 0x64
> +
> +#define SX932X_REG_DIFF_MSB 0x65
> +#define SX932X_REG_DIFF_LSB 0x66
> +
> +#define SX932X_REG_OFFSET_MSB 0x67
> +#define SX932X_REG_OFFSET_LSB 0x68
> +
> +#define SX932X_REG_SAR_MSB 0x69
> +#define SX932X_REG_SAR_LSB 0x6a
> +
> +#define SX932X_REG_RESET 0x9f
> +/* Write this to REG_RESET to do a soft reset. */
> +#define SX932X_SOFT_RESET 0xde
> +
> +#define SX932X_REG_WHOAMI 0xfa
> +#define SX9320_WHOAMI 0x20
> +#define SX9321_WHOAMI 0x21
> +#define SX9324_WHOAMI 0x23
Very odd indenting. Please fix.
> +#define SX932X_REG_REVISION 0xfe
> +
> +
One blank line is almost always enough for readability.
Adding more just reduces what is on my screen at one time and makes
the code harder to review.
> +/* Sensor Readback */
This comment seems unlikely to add much.
> +
> +/*
> + * These serve for identifying IRQ source in the IRQ_SRC register, and
> + * also for masking the IRQs in the IRQ_MSK register.
> + */
> +#define SX932X_RESET_IRQ BIT(7)
> +#define SX932X_CLOSE_IRQ BIT(6)
> +#define SX932X_FAR_IRQ BIT(5)
> +#define SX932X_COMPDONE_IRQ BIT(4)
> +#define SX932X_CONVDONE_IRQ BIT(3)
I would prefer it to be obvious which register these are used for from
the naming. There are various ways of doing this, but a nice naming convention
is something like.
#define SX9320_IRQ_MASK_REG xxx
#define SX9320_IRQ_MASK_RESET BIT(7)
#define SX9320_IRQ_MASK_CLOSE
etc. Note the indenting to make it clear which is the register name and
which the field name. Also note no wild cards in the prefix. Just pick
a device and name everything common after that one. If there are differences
then make those clear by using the relevant prefix.
> +
> +#define SX932X_SCAN_PERIOD_MASK GENMASK(7, 4)
> +#define SX932X_SCAN_PERIOD_SHIFT 4
> +
> +#define SX932X_COMPSTAT_MASK GENMASK(3, 0)
> +
> +/* 4 channels, as defined in STAT0: PH0, PH1, PH2 and PH3. */
> +#define SX932X_NUM_CHANNELS 4
> +#define SX932X_CHAN_MASK GENMASK(1, 0)
> +
> +struct sx932x_reg_config {
> + const char *property;
> + u8 reg_addr;
> + u8 def_value;
> +};
> +
> +struct sx932x_data {
> + struct mutex mutex;
Locks should always have clear comments explaining exactly what
they are protecting.
> + struct i2c_client *client;
> + const struct sx932x_reg_config *reg_config;
> + struct iio_trigger *trig;
> + struct regmap *regmap;
> + /*
> + * Last reading of the proximity status for each channel.
> + * We only send an event to user space when this changes.
> + */
> + bool prox_stat[SX932X_NUM_CHANNELS];
> + bool event_enabled[SX932X_NUM_CHANNELS];
> + bool trigger_enabled;
> + u16 *buffer;
> + /* Remember which phases are enabled during a suspend. */
> + unsigned int suspend_reg_gnrl_ctrl1;
> + struct completion completion;
> + int data_rdy_users, close_far_users;
> + int channel_users[SX932X_NUM_CHANNELS];
> +};
> +
> +static const struct iio_event_spec sx932x_events[] = {
> + {
> + .type = IIO_EV_TYPE_THRESH,
> + .dir = IIO_EV_DIR_EITHER,
> + .mask_separate = BIT(IIO_EV_INFO_ENABLE),
> + },
> +};
> +
> +#define SX932X_RAW_CHANNEL(idx, name, addr) \
> + { \
> + .type = IIO_PROXIMITY, \
> + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \
> + .info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ), \
> + .indexed = 1, \
> + .channel = idx, \
> + .address = addr, \
> + .event_spec = sx932x_events, \
> + .num_event_specs = ARRAY_SIZE(sx932x_events), \
> + .extend_name = name, \
The extend name field is almost always a bad idea as it breaks generic
userspace code.
> + .scan_index = idx, \
> + .scan_type = { \
> + .sign = 's', \
> + .realbits = 16, \
> + .storagebits = 16, \
> + .shift = 0, \
Shift default will be 0 anyway so no need to specify.
> + }, \
> + }
> +
> +#define SX932X_PROCESSED_CHANNEL(idx, name, addr) \
> + { \
> + .type = IIO_PROXIMITY, \
> + .info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED), \
> + .info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ), \
> + .indexed = 1, \
> + .channel = idx, \
> + .address = addr, \
> + .event_spec = sx932x_events, \
> + .num_event_specs = ARRAY_SIZE(sx932x_events), \
> + .extend_name = name, \
> + .scan_index = idx, \
> + .scan_type = { \
> + .sign = 's', \
> + .realbits = 16, \
> + .storagebits = 16, \
> + .shift = 0, \
> + }, \
> + }
This doesn't seem to be used...
> +
> +static const struct iio_chan_spec sx932x_channels[] = {
> + SX932X_RAW_CHANNEL(0, "UC0", SX932X_REG_USE_MSB),
These channels are somewhat 'unusual'. What do they actually give
you?
> + SX932X_RAW_CHANNEL(1, "UC1", SX932X_REG_USE_MSB),
> + SX932X_RAW_CHANNEL(2, "UC2", SX932X_REG_USE_MSB),
> + SX932X_RAW_CHANNEL(3, "UC3", SX932X_REG_USE_MSB),
> +
> + SX932X_RAW_CHANNEL(4, "AC0", SX932X_REG_AVG_MSB),
> + SX932X_RAW_CHANNEL(5, "AC1", SX932X_REG_AVG_MSB),
> + SX932X_RAW_CHANNEL(6, "AC2", SX932X_REG_AVG_MSB),
> + SX932X_RAW_CHANNEL(7, "AC3", SX932X_REG_AVG_MSB),
> +
> + SX932X_RAW_CHANNEL(8, "DC0", SX932X_REG_DIFF_MSB),
> + SX932X_RAW_CHANNEL(9, "DC1", SX932X_REG_DIFF_MSB),
> + SX932X_RAW_CHANNEL(10, "DC2", SX932X_REG_DIFF_MSB),
> + SX932X_RAW_CHANNEL(11, "DC3", SX932X_REG_DIFF_MSB),
> +
> + SX932X_RAW_CHANNEL(12, "CO0", SX932X_REG_OFFSET_MSB),
> + SX932X_RAW_CHANNEL(13, "CO1", SX932X_REG_OFFSET_MSB),
> + SX932X_RAW_CHANNEL(14, "CO2", SX932X_REG_OFFSET_MSB),
> + SX932X_RAW_CHANNEL(15, "CO3", SX932X_REG_OFFSET_MSB),
> +
> + IIO_CHAN_SOFT_TIMESTAMP(16),
> +};
> +
> +static const struct {
> + int val;
> + int val2;
> +} sx932x_samp_freq_table[] = {
> + {500, 0}, /* 0000: Min (no idle time) */
> + {66, 666666}, /* 0001: 15 ms */
> + {33, 333333}, /* 0010: 30 ms (Typ.) */
> + {22, 222222}, /* 0011: 45 ms */
> + {16, 666666}, /* 0100: 60 ms */
> + {11, 111111}, /* 0101: 90 ms */
> + {8, 333333}, /* 0110: 120 ms */
> + {5, 0}, /* 0111: 200 ms */
> + {2, 500000}, /* 1000: 400 ms */
> + {1, 666666}, /* 1001: 600 ms */
> + {1, 250000}, /* 1010: 800 ms */
> + {1, 0}, /* 1011: 1 s */
> + {0, 500000}, /* 1100: 2 s */
> + {8, 333333}, /* 1101: 3 s */
> + {0, 250000}, /* 1110: 4 s */
> + {0, 200000}, /* 1111: 5 s */
> +};
> +static const unsigned int sx932x_scan_period_table[] = {
> + 2, 15, 30, 45, 60, 90, 120, 200, 400, 800, 1000, 2000, 3000, 4000, 5000,
> +};
> +
> +static IIO_CONST_ATTR_SAMP_FREQ_AVAIL(
> + "500.0 66.666666 33.333333 22.222222 16.666666 "
> + "11.111111 8.333333 5.0 2.500000 1.666666 1.250000 "
> + "1.0 0.500000 8.333333 0.250000 0.200000");
> +
> +
> +static const struct regmap_range sx932x_writable_reg_ranges[] = {
> + regmap_reg_range(SX932X_REG_IRQ_MSK, SX932X_REG_IRQ_CFG2),
> + regmap_reg_range(SX932X_REG_GNRL_CTRL0, SX932X_REG_GNRL_CTRL1),
> + /* Leave i2c and clock spreading as unavailable */
> + regmap_reg_range(SX932X_REG_AFE_CTRL0, SX932X_REG_AFE_CTRL9),
> + regmap_reg_range(SX932X_REG_PROX_CTRL0, SX932X_REG_PROX_CTRL7),
> + regmap_reg_range(SX932X_REG_ADV_CTRL0, SX932X_REG_ADV_CTRL20),
> + regmap_reg_range(SX932X_REG_PHASE_SEL, SX932X_REG_PHASE_SEL),
> + regmap_reg_range(SX932X_REG_OFFSET_MSB, SX932X_REG_OFFSET_LSB),
> + regmap_reg_range(SX932X_REG_RESET, SX932X_REG_RESET),
> +};
> +
> +static const struct regmap_access_table sx932x_writeable_regs = {
> + .yes_ranges = sx932x_writable_reg_ranges,
> + .n_yes_ranges = ARRAY_SIZE(sx932x_writable_reg_ranges),
> +};
> +
> +/*
> + * All allocated registers are readable, so we just list unallocated
> + * ones.
Whilst it might be shorter, I would prefer a positive list to the
negative one based on 1 more than a register... Easier to audit
if the datasheet is available.
> + */
> +static const struct regmap_range sx932x_non_readable_reg_ranges[] = {
> + regmap_reg_range(SX932X_REG_IRQ_CFG2 + 1, SX932X_REG_GNRL_CTRL0 - 1),
> + regmap_reg_range(SX932X_REG_GNRL_CTRL1 + 1, SX932X_REG_AFE_CTRL0 - 1),
> + regmap_reg_range(SX932X_REG_AFE_CTRL9 + 1, SX932X_REG_PROX_CTRL0 - 1),
> + regmap_reg_range(SX932X_REG_PROX_CTRL7 + 1, SX932X_REG_ADV_CTRL0 - 1),
> + regmap_reg_range(SX932X_REG_ADV_CTRL20 + 1, SX932X_REG_PHASE_SEL - 1),
> + regmap_reg_range(SX932X_REG_SAR_LSB + 1, SX932X_REG_RESET - 1),
> + regmap_reg_range(SX932X_REG_RESET + 1, SX932X_REG_WHOAMI - 1),
> + regmap_reg_range(SX932X_REG_WHOAMI + 1, SX932X_REG_REVISION - 1),
> +};
> +
> +static const struct regmap_access_table sx932x_readable_regs = {
> + .no_ranges = sx932x_non_readable_reg_ranges,
> + .n_no_ranges = ARRAY_SIZE(sx932x_non_readable_reg_ranges),
> +};
> +
> +static const struct regmap_range sx932x_volatile_reg_ranges[] = {
> + regmap_reg_range(SX932X_REG_IRQ_SRC, SX932X_REG_STAT3),
> + regmap_reg_range(SX932X_REG_USE_MSB, SX932X_REG_DIFF_LSB),
> + regmap_reg_range(SX932X_REG_SAR_MSB, SX932X_REG_SAR_LSB),
> + regmap_reg_range(SX932X_REG_WHOAMI, SX932X_REG_WHOAMI),
> + regmap_reg_range(SX932X_REG_REVISION, SX932X_REG_REVISION),
> +};
> +
> +static const struct regmap_access_table sx932x_volatile_regs = {
> + .yes_ranges = sx932x_volatile_reg_ranges,
> + .n_yes_ranges = ARRAY_SIZE(sx932x_volatile_reg_ranges),
> +};
> +
> +static const struct regmap_config sx932x_regmap_config = {
> + .reg_bits = 8,
> + .val_bits = 8,
> +
> + .max_register = SX932X_REG_REVISION,
> + .cache_type = REGCACHE_RBTREE,
> +
> + .wr_table = &sx932x_writeable_regs,
> + .rd_table = &sx932x_readable_regs,
> + .volatile_table = &sx932x_volatile_regs,
> +};
> +
> +static int sx932x_inc_users(struct sx932x_data *data, int *counter,
> + unsigned int reg, unsigned int bitmask)
> +{
> + ++(*counter);
> + if (*counter != 1)
> + /* Bit is already active, nothing to do. */
> + return 0;
Hmm. I want a full description of what this reference counting is actually
doing. It is possible to work out, but I'm not sure what the hardware
constraints actually are.
There are 3 things that can be going on that might result in a channel needing
to be enabled
1. Buffered capture of that channel
2. Events on that channel
3. An individual read of that channel.
1 and 3 are sort of prevented from happening at the same time (I'm not sure this
intended or not).
Anyhow, looks like 3 simple masks to me rather than reference counting.
In paths were it 'might' make sense to disable a given channel then
the OR of these can be checked to see if someone else is using it.
It might be that what you have simpler, but I'm not convinced yet.
> +
> + return regmap_update_bits(data->regmap, reg, bitmask, bitmask);
> +}
> +
> +static int sx932x_dec_users(struct sx932x_data *data, int *counter,
> + unsigned int reg, unsigned int bitmask)
> +{
> + --(*counter);
> + if (*counter != 0)
> + /* There are more users, do not deactivate. */
> + return 0;
> +
> + return regmap_update_bits(data->regmap, reg, bitmask, 0);
> +}
> +
> +static int sx932x_inc_chan_users(struct sx932x_data *data, int chan)
> +{
> + return sx932x_inc_users(data, &data->channel_users[chan],
> + SX932X_REG_PROX_CTRL0, BIT(chan));
> +}
> +
> +static int sx932x_dec_chan_users(struct sx932x_data *data, int chan)
> +{
> + return sx932x_dec_users(data, &data->channel_users[chan],
> + SX932X_REG_PROX_CTRL0, BIT(chan));
> +}
> +
> +static int sx932x_inc_data_rdy_users(struct sx932x_data *data)
> +{
> + return sx932x_inc_users(data, &data->data_rdy_users,
> + SX932X_REG_IRQ_MSK, SX932X_CONVDONE_IRQ);
> +}
> +
> +static int sx932x_dec_data_rdy_users(struct sx932x_data *data)
> +{
> + return sx932x_dec_users(data, &data->data_rdy_users,
> + SX932X_REG_IRQ_MSK, SX932X_CONVDONE_IRQ);
> +}
> +
> +static int sx932x_inc_close_far_users(struct sx932x_data *data)
> +{
> + return sx932x_inc_users(data, &data->close_far_users,
> + SX932X_REG_IRQ_MSK,
> + SX932X_CLOSE_IRQ | SX932X_FAR_IRQ);
> +}
> +
> +static int sx932x_dec_close_far_users(struct sx932x_data *data)
> +{
> + return sx932x_dec_users(data, &data->close_far_users,
> + SX932X_REG_IRQ_MSK,
> + SX932X_CLOSE_IRQ | SX932X_FAR_IRQ);
Given we have separate irqs for close and far why not support separately
enabled interrupts for the two?
> +}
> +
> +static int sx932x_read_prox_data(struct sx932x_data *data,
> + const struct iio_chan_spec *chan,
> + int *val)
> +{
> + int ret;
> + __be16 regval;
> + const int regid = chan->channel & 3;
> +
> + ret = regmap_write(data->regmap, SX932X_REG_PHASE_SEL, regid);
> + if (ret < 0)
> + return ret;
> +
> + ret = regmap_bulk_read(data->regmap, chan->address, ®val, 2);
> + if (ret < 0)
> + return ret;
> +
> + *val = sign_extend32(be16_to_cpu(regval), 15);
This should only be in the read_raw case. Where buffers are in use
we should just pass it up be16 and let userspace deal with it.
Also should not be sign extended for that case - or indeed converted
to a 32 bit (to be truncated when put into the buffer).
> +
> + return 0;
> +}
> +
> +static int sx932x_read_stat_data(struct sx932x_data *data,
> + const struct iio_chan_spec
> + *chan,
> + int *val)
> +{
> + int ret;
> +
> + ret = regmap_read(data->regmap, chan->address, val);
> + if (ret < 0)
> + return ret;
regmap read always I think returns 0 or < 0 so just return regmap_read(...)
> +
> + return 0;
> +}
> +
> +/*
> + * If we have no interrupt support, we have to wait for a scan period
> + * after enabling a channel to get a result.
> + */
> +static int sx932x_wait_for_sample(struct sx932x_data *data)
> +{
> + int ret;
> + unsigned int val;
> +
> + ret = regmap_read(data->regmap, SX932X_REG_PROX_CTRL0, &val);
> + if (ret < 0)
> + return ret;
> +
> + val = (val & SX932X_SCAN_PERIOD_MASK) >> SX932X_SCAN_PERIOD_SHIFT;
> +
> + msleep(sx932x_scan_period_table[val]);
> +
> + return 0;
> +}
> +
> +static int sx932x_read_stat(struct sx932x_data *data,
> + const struct iio_chan_spec *chan,
> + int *val)
I don't think this is ever actually called...
> +{
> + int ret;
> +
> + mutex_lock(&data->mutex);
> +
> + ret = sx932x_inc_chan_users(data, chan->channel);
> + if (ret < 0)
> + goto out;
> +
> + ret = sx932x_inc_data_rdy_users(data);
> + if (ret < 0)
> + goto out_dec_chan;
> +
> + mutex_unlock(&data->mutex);
> +
> + if (data->client->irq > 0)
> + ret = wait_for_completion_interruptible(&data->completion);
> + else
> + ret = sx932x_wait_for_sample(data);
> +
> + mutex_lock(&data->mutex);
> +
> + if (ret < 0)
> + goto out_dec_data_rdy;
> +
> + ret = sx932x_read_stat_data(data, chan, val);
> + if (ret < 0)
> + goto out_dec_data_rdy;
> +
> + ret = sx932x_dec_data_rdy_users(data);
> + if (ret < 0)
> + goto out_dec_chan;
> +
> + ret = sx932x_dec_chan_users(data, chan->channel);
> + if (ret < 0)
> + goto out;
> +
> + ret = IIO_VAL_INT;
> +
> + goto out;
> +
> +out_dec_data_rdy:
> + sx932x_dec_data_rdy_users(data);
> +out_dec_chan:
> + sx932x_dec_chan_users(data, chan->channel);
> +out:
> + mutex_unlock(&data->mutex);
> + reinit_completion(&data->completion);
> +
> + return ret;
> +}
> +
> +static int sx932x_read_proximity(struct sx932x_data *data,
> + const struct iio_chan_spec *chan,
> + int *val)
> +{
> + int ret;
> +
> + mutex_lock(&data->mutex);
> +
> + ret = sx932x_inc_chan_users(data, chan->channel);
> + if (ret < 0)
> + goto out;
> +
> + ret = sx932x_inc_data_rdy_users(data);
> + if (ret < 0)
> + goto out_dec_chan;
> +
> + mutex_unlock(&data->mutex);
> +
> + if (data->client->irq > 0)
> + ret = wait_for_completion_interruptible(&data->completion);
> + else
> + ret = sx932x_wait_for_sample(data);
> +
> + mutex_lock(&data->mutex);
> +
> + if (ret < 0)
> + goto out_dec_data_rdy;
> +
> + ret = sx932x_read_prox_data(data, chan, val);
> + if (ret < 0)
> + goto out_dec_data_rdy;
> +
> + ret = sx932x_dec_data_rdy_users(data);
> + if (ret < 0)
> + goto out_dec_chan;
> +
> + ret = sx932x_dec_chan_users(data, chan->channel);
> + if (ret < 0)
> + goto out;
> +
> + ret = IIO_VAL_INT;
> +
> + goto out;
Separate the error path so you don't need to jump cleanup.
That makes for messy and hard to read code to just save a few lines.
> +
> +out_dec_data_rdy:
> + sx932x_dec_data_rdy_users(data);
> +out_dec_chan:
> + sx932x_dec_chan_users(data, chan->channel);
> +out:
> + mutex_unlock(&data->mutex);
> + reinit_completion(&data->completion);
> +
> + return ret;
> +}
> +
> +static int sx932x_read_samp_freq(struct sx932x_data *data,
> + int *val, int *val2)
> +{
> + int ret;
> + unsigned int regval;
> +
> + mutex_lock(&data->mutex);
> + ret = regmap_read(data->regmap, SX932X_REG_PROX_CTRL0, ®val);
> + mutex_unlock(&data->mutex);
> +
> + if (ret < 0)
> + return ret;
> +
> + regval = (regval & SX932X_SCAN_PERIOD_MASK) >> SX932X_SCAN_PERIOD_SHIFT;
> + *val = sx932x_samp_freq_table[regval].val;
> + *val2 = sx932x_samp_freq_table[regval].val2;
> +
> + return IIO_VAL_INT_PLUS_MICRO;
> +}
> +
> +static int sx932x_read_raw(struct iio_dev *indio_dev,
> + const struct iio_chan_spec *chan,
> + int *val, int *val2, long mask)
> +{
> + struct sx932x_data *data = iio_priv(indio_dev);
> + int ret;
> +
> + switch (chan->type) {
> + case IIO_PROXIMITY:
> + switch (mask) {
> + case IIO_CHAN_INFO_RAW:
> + mutex_lock(&indio_dev->mlock);
> +
Use the iio_device_claim_direct_mode helper.
Whilst you have this correct I think, the helper save us
checking that the locking right :)
> + if (iio_buffer_enabled(indio_dev)) {
> + mutex_unlock(&indio_dev->mlock);
> + return -EBUSY;
> + }
> + ret = sx932x_read_proximity(data, chan, val);
> + mutex_unlock(&indio_dev->mlock);
> + return ret;
> + case IIO_CHAN_INFO_PROCESSED:
This bothers me. A given channel should only ever be either raw
(if you can provide scale and offset to allow userspace to get to the
processed value) or processed. Never both. I don't think you ever
actually use this path though so just clean it up and all the
functions that are only called from here..
> + mutex_lock(&indio_dev->mlock);
> +
> + if (iio_buffer_enabled(indio_dev)) {
> + mutex_unlock(&indio_dev->mlock);
> + return -EBUSY;
> + }
> + ret = sx932x_read_stat(data, chan, val);
> + mutex_unlock(&indio_dev->mlock);
> + return ret;
> + case IIO_CHAN_INFO_SAMP_FREQ:
> + return sx932x_read_samp_freq(data, val, val2);
> + default:
> + return -EINVAL;
> + }
> + default:
> + return -EINVAL;
> + }
> +}
> +
> +static int sx932x_set_samp_freq(struct sx932x_data *data,
> + int val, int val2)
> +{
> + int i, ret;
> +
> + for (i = 0; i < ARRAY_SIZE(sx932x_samp_freq_table); i++)
> + if (val == sx932x_samp_freq_table[i].val &&
> + val2 == sx932x_samp_freq_table[i].val2)
> + break;
> +
> + if (i == ARRAY_SIZE(sx932x_samp_freq_table))
> + return -EINVAL;
> +
> + mutex_lock(&data->mutex);
> +
> + ret = regmap_update_bits(data->regmap, SX932X_REG_PROX_CTRL0,
> + SX932X_SCAN_PERIOD_MASK,
> + i << SX932X_SCAN_PERIOD_SHIFT);
> +
> + mutex_unlock(&data->mutex);
> +
> + return ret;
> +}
> +
> +static int sx932x_write_raw(struct iio_dev *indio_dev,
> + const struct iio_chan_spec *chan,
> + int val, int val2, long mask)
> +{
> + struct sx932x_data *data = iio_priv(indio_dev);
> +
> + switch (chan->type) {
> + case IIO_PROXIMITY:
> + switch (mask) {
> + case IIO_CHAN_INFO_SAMP_FREQ:
> + return sx932x_set_samp_freq(data, val, val2);
> + default:
> + return -EINVAL;
> + }
> + default:
> + return -EINVAL;
> + }
> +}
> +
> +static irqreturn_t sx932x_irq_handler(int irq, void *private)
> +{
> + struct iio_dev *indio_dev = private;
> + struct sx932x_data *data = iio_priv(indio_dev);
> +
> + if (data->trigger_enabled)
> + iio_trigger_poll(data->trig);
Is it possible to have an interrupt that doesn't include the
dataready signal? I'm guessing no...
> +
> + /*
> + * Even if no event is enabled, we need to wake the thread to
> + * clear the interrupt state by reading SX932X_REG_IRQ_SRC. It
> + * is not possible to do that here because regmap_read takes a
> + * mutex.
> + */
> + return IRQ_WAKE_THREAD;
> +}
> +
> +static void sx932x_push_events(struct iio_dev *indio_dev)
> +{
> + int ret;
> + unsigned int val, chan;
> + struct sx932x_data *data = iio_priv(indio_dev);
> +
> + ret = regmap_read(data->regmap, SX932X_REG_STAT0, &val);
> + if (ret < 0) {
> + dev_err(&data->client->dev, "i2c transfer error in irq\n");
> + return;
> + }
> +
> + for (chan = 0; chan < SX932X_NUM_CHANNELS; chan++) {
> + int dir;
> + u64 ev;
> + bool new_prox = val & BIT(chan);
> +
> + if (!data->event_enabled[chan])
> + continue;
> + if (new_prox == data->prox_stat[chan])
> + /* No change on this channel. */
So we are looking at a level state register and edge interrupt?
Perhaps add a comment to make this clear.
> + continue;
> +
> + dir = new_prox ? IIO_EV_DIR_FALLING : IIO_EV_DIR_RISING;
> + ev = IIO_UNMOD_EVENT_CODE(IIO_PROXIMITY, chan,
> + IIO_EV_TYPE_THRESH, dir);
> + iio_push_event(indio_dev, ev, iio_get_time_ns(indio_dev));
Hmm. Should this be a single timestamp for all events? In theory we could
have more turn up here that weren't true when the interrupt fires so I guess
this is better.
> + data->prox_stat[chan] = new_prox;
> + }
> +}
> +
> +static irqreturn_t sx932x_irq_thread_handler(int irq, void *private)
> +{
> + struct iio_dev *indio_dev = private;
> + struct sx932x_data *data = iio_priv(indio_dev);
> + int ret;
> + unsigned int val;
> +
> + mutex_lock(&data->mutex);
> +
> + ret = regmap_read(data->regmap, SX932X_REG_IRQ_SRC, &val);
> + if (ret < 0) {
> + dev_err(&data->client->dev, "i2c transfer error in irq\n");
> + goto out;
> + }
> +
> + if (val & (SX932X_CLOSE_IRQ | SX932X_FAR_IRQ))
> + sx932x_push_events(indio_dev);
> +
> + if (val & SX932X_CONVDONE_IRQ)
> + complete(&data->completion);
> +
> +out:
> + mutex_unlock(&data->mutex);
> +
> + return IRQ_HANDLED;
> +}
> +
> +static int sx932x_read_event_config(struct iio_dev *indio_dev,
> + const struct iio_chan_spec *chan,
> + enum iio_event_type type,
> + enum iio_event_direction dir)
> +{
> + struct sx932x_data *data = iio_priv(indio_dev);
> +
> + if (chan->type != IIO_PROXIMITY || type != IIO_EV_TYPE_THRESH ||
> + dir != IIO_EV_DIR_EITHER)
> + return -EINVAL;
Given that you don't register this for other options, that check cannot
ever fail as far as I can see. I would drop it.
> +
> + return data->event_enabled[chan->channel & SX932X_CHAN_MASK];
> +}
> +
> +static int sx932x_write_event_config(struct iio_dev *indio_dev,
> + const struct iio_chan_spec *chan,
> + enum iio_event_type type,
> + enum iio_event_direction dir,
> + int state)
> +{
> + struct sx932x_data *data = iio_priv(indio_dev);
> + int ret;
> +
> + if (chan->type != IIO_PROXIMITY || type != IIO_EV_TYPE_THRESH ||
> + dir != IIO_EV_DIR_EITHER)
> + return -EINVAL;
> +
> + mutex_lock(&data->mutex);
> +
> + if (state == 1) {
> + ret = sx932x_inc_chan_users(data, chan->channel);
Ah. I'm starting to understand what is going on here. You need to enable
the channels if you are either reading them, or you have their events enabled.
That doesn't need full reference counting. Just have a pair of masks and
track the | of the two of them.
> + if (ret < 0)
> + goto out_unlock;
> + ret = sx932x_inc_close_far_users(data);
> + if (ret < 0)
> + goto out_undo_chan;
> + } else {
> + ret = sx932x_dec_chan_users(data, chan->channel);
> + if (ret < 0)
> + goto out_unlock;
> + ret = sx932x_dec_close_far_users(data);
> + if (ret < 0)
> + goto out_undo_chan;
> + }
> +
> + data->event_enabled[chan->channel & SX932X_CHAN_MASK] = state;
> + goto out_unlock;
> +
> +out_undo_chan:
> + if (state == 1)
> + sx932x_dec_chan_users(data, chan->channel);
> + else
> + sx932x_inc_chan_users(data, chan->channel);
> +out_unlock:
> + mutex_unlock(&data->mutex);
> + return ret;
> +}
> +
> +static int sx932x_update_scan_mode(struct iio_dev *indio_dev,
> + const unsigned long *scan_mask)
> +{
> + struct sx932x_data *data = iio_priv(indio_dev);
> +
> + mutex_lock(&data->mutex);
> + kfree(data->buffer);
> + data->buffer = kzalloc(indio_dev->scan_bytes, GFP_KERNEL);
What's the maximum? My suspicion is that it's not that large so
you should just put it in the data structure directly, sized
to take the maximum and not worry about this complexity.
> + mutex_unlock(&data->mutex);
> +
> + if (data->buffer == NULL)
> + return -ENOMEM;
> +
> + return 0;
> +}
> +
> +static ssize_t sx932x_uid_show(struct device *dev,
> + struct device_attribute *attr,
> + char *buf)
> +{
> + struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> +#ifdef CONFIG_ACPI
> + struct acpi_device *adev = ACPI_COMPANION(dev);
> +
> + if (adev != NULL)
> + return scnprintf(buf, PAGE_SIZE, "%s\n", acpi_device_uid(adev));
> +#endif
> + return scnprintf(buf, PAGE_SIZE, "%d\n", indio_dev->id);
No to this one. There are lots of ways of finding out an ACPI uid,
we don't need it exposed as ABI in IIO.
> +}
> +
> +static IIO_DEVICE_ATTR(uid, 0444, sx932x_uid_show, NULL, 0);
> +
> +struct sx932x_ctrl_attribute {
> + struct device_attribute dev_attr;
> + u8 ctrl;
> +};
> +
> +static inline struct sx932x_ctrl_attribute *sx932x_to_ctrl_attr(
> + struct device_attribute *attr)
> +{
> + return container_of(attr, struct sx932x_ctrl_attribute, dev_attr);
> +}
> +
> +static ssize_t sx932x_show_ctrl_attr(struct device *dev,
> + struct device_attribute *dev_attr,
> + char *buf)
> +{
> + struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> + struct sx932x_ctrl_attribute *attr = sx932x_to_ctrl_attr(dev_attr);
> + struct sx932x_data *data = iio_priv(indio_dev);
> + unsigned int val;
> + int ret;
> +
> + mutex_lock(&data->mutex);
> + ret = regmap_read(data->regmap, attr->ctrl, &val);
> + mutex_unlock(&data->mutex);
> + if (ret)
> + return ret;
> +
> + return scnprintf(buf, PAGE_SIZE, "%02x\n", val);
> +}
> +
> +static ssize_t sx932x_store_ctrl_attr(struct device *dev,
> + struct device_attribute *dev_attr,
> + const char *buf, size_t len)
> +{
> + struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> + struct sx932x_ctrl_attribute *attr = sx932x_to_ctrl_attr(dev_attr);
> + struct sx932x_data *data = iio_priv(indio_dev);
> + u8 val;
> + int ret;
> +
> + if (kstrtou8(buf, 0, &val))
> + return -EINVAL;
> +
> + mutex_lock(&data->mutex);
> + ret = regmap_write(data->regmap, attr->ctrl, val);
> + mutex_unlock(&data->mutex);
> + if (ret)
> + return ret;
> +
> + return len;
> +}
> +
> +#define SX932X_CTRL_ATTR_FILL(_name, _mode, _show, _store, _ctrl, _offset) \
> + { .dev_attr = __ATTR(_name, _mode, _show, _store), \
> + .ctrl = _ctrl + _offset }
> +
> +#define SX932X_CTRL_ATTR(_name, _mode, _show, _store, _ctrl, _offset) \
> + struct sx932x_ctrl_attribute sx932x_ctrl_attr_##_name = \
> + SX932X_CTRL_ATTR_FILL( \
> + _name, _mode, _show, _store, _ctrl, _offset)
> +
> +#define SX932X_ADV_CTRL_RW(_ctrl) \
> + SX932X_CTRL_ATTR(adv_ctrl##_ctrl##_raw, 0644, \
> + sx932x_show_ctrl_attr, sx932x_store_ctrl_attr, \
> + _ctrl, SX932X_REG_ADV_CTRL0)
> +
> +static SX932X_ADV_CTRL_RW(0);
> +static SX932X_ADV_CTRL_RW(1);
> +static SX932X_ADV_CTRL_RW(2);
> +static SX932X_ADV_CTRL_RW(3);
> +static SX932X_ADV_CTRL_RW(4);
> +static SX932X_ADV_CTRL_RW(5);
> +static SX932X_ADV_CTRL_RW(6);
> +static SX932X_ADV_CTRL_RW(7);
> +static SX932X_ADV_CTRL_RW(8);
> +static SX932X_ADV_CTRL_RW(9);
> +static SX932X_ADV_CTRL_RW(10);
> +static SX932X_ADV_CTRL_RW(11);
> +static SX932X_ADV_CTRL_RW(12);
> +static SX932X_ADV_CTRL_RW(13);
> +static SX932X_ADV_CTRL_RW(14);
> +static SX932X_ADV_CTRL_RW(15);
> +static SX932X_ADV_CTRL_RW(16);
> +static SX932X_ADV_CTRL_RW(17);
> +static SX932X_ADV_CTRL_RW(18);
> +static SX932X_ADV_CTRL_RW(19);
> +static SX932X_ADV_CTRL_RW(20);
> +
> +static struct attribute *sx932x_attributes[] = {
> + &iio_const_attr_sampling_frequency_available.dev_attr.attr,
> + &iio_dev_attr_uid.dev_attr.attr,
> + &sx932x_ctrl_attr_adv_ctrl0_raw.dev_attr.attr,
> + &sx932x_ctrl_attr_adv_ctrl1_raw.dev_attr.attr,
> + &sx932x_ctrl_attr_adv_ctrl2_raw.dev_attr.attr,
> + &sx932x_ctrl_attr_adv_ctrl3_raw.dev_attr.attr,
> + &sx932x_ctrl_attr_adv_ctrl4_raw.dev_attr.attr,
> + &sx932x_ctrl_attr_adv_ctrl5_raw.dev_attr.attr,
> + &sx932x_ctrl_attr_adv_ctrl6_raw.dev_attr.attr,
> + &sx932x_ctrl_attr_adv_ctrl7_raw.dev_attr.attr,
> + &sx932x_ctrl_attr_adv_ctrl8_raw.dev_attr.attr,
> + &sx932x_ctrl_attr_adv_ctrl9_raw.dev_attr.attr,
> + &sx932x_ctrl_attr_adv_ctrl10_raw.dev_attr.attr,
> + &sx932x_ctrl_attr_adv_ctrl11_raw.dev_attr.attr,
> + &sx932x_ctrl_attr_adv_ctrl12_raw.dev_attr.attr,
> + &sx932x_ctrl_attr_adv_ctrl13_raw.dev_attr.attr,
> + &sx932x_ctrl_attr_adv_ctrl14_raw.dev_attr.attr,
> + &sx932x_ctrl_attr_adv_ctrl15_raw.dev_attr.attr,
> + &sx932x_ctrl_attr_adv_ctrl16_raw.dev_attr.attr,
> + &sx932x_ctrl_attr_adv_ctrl17_raw.dev_attr.attr,
> + &sx932x_ctrl_attr_adv_ctrl18_raw.dev_attr.attr,
> + &sx932x_ctrl_attr_adv_ctrl19_raw.dev_attr.attr,
> + &sx932x_ctrl_attr_adv_ctrl20_raw.dev_attr.attr,
I had a feeling that would be somewhere. Do not have raw registers
exposed in sysfs. If you really want them for debug reasons, read only
access in debugfs is reasonable - possibly even write access though probably
not...
They cannot form part of a remotely useable or generic userspace ABI.
> + NULL,
> +};
> +
> +static const struct attribute_group sx932x_attribute_group = {
> + .attrs = sx932x_attributes,
> +};
> +
> +static const struct iio_info sx932x_info = {
> + .attrs = &sx932x_attribute_group,
> + .read_raw = &sx932x_read_raw,
> + .write_raw = &sx932x_write_raw,
> + .read_event_config = &sx932x_read_event_config,
> + .write_event_config = &sx932x_write_event_config,
> + .update_scan_mode = &sx932x_update_scan_mode,
> +};
> +
> +static int sx932x_set_trigger_state(struct iio_trigger *trig,
> + bool state)
> +{
> + struct iio_dev *indio_dev = iio_trigger_get_drvdata(trig);
> + struct sx932x_data *data = iio_priv(indio_dev);
> + int ret;
> +
> + mutex_lock(&data->mutex);
> +
> + if (state)
> + ret = sx932x_inc_data_rdy_users(data);
It feels like you are doing this twice - once in the trigger setup
and once in the buffer enable. That doesn't seem likely to be
correct.
> + else
> + ret = sx932x_dec_data_rdy_users(data);
> + if (ret < 0)
> + goto out;
> +
> + data->trigger_enabled = state;
> +
> +out:
> + mutex_unlock(&data->mutex);
> +
> + return ret;
> +}
> +
> +static const struct iio_trigger_ops sx932x_trigger_ops = {
> + .set_trigger_state = sx932x_set_trigger_state,
> +};
> +
> +static irqreturn_t sx932x_trigger_handler(int irq, void *private)
> +{
> + struct iio_poll_func *pf = private;
> + struct iio_dev *indio_dev = pf->indio_dev;
> + struct sx932x_data *data = iio_priv(indio_dev);
> + int val, bit, ret, i = 0;
> +
> + mutex_lock(&data->mutex);
> +
> + for_each_set_bit(bit, indio_dev->active_scan_mask,
> + indio_dev->masklength) {
> + ret = sx932x_read_prox_data(data, &indio_dev->channels[bit],
> + &val);
so there is an endian conversion in this call that we would normally leave
to userspace if possible..
> + if (ret < 0)
> + goto out;
> +
> + data->buffer[i++] = val;
> + }
> +
> + iio_push_to_buffers_with_timestamp(indio_dev, data->buffer,
> + iio_get_time_ns(indio_dev));
> +out:
> + mutex_unlock(&data->mutex);
> +
> + iio_trigger_notify_done(indio_dev->trig);
> +
> + return IRQ_HANDLED;
> +}
> +
> +static int sx932x_buffer_preenable(struct iio_dev *indio_dev)
> +{
> + struct sx932x_data *data = iio_priv(indio_dev);
> + int ret = 0, i;
> +
> + mutex_lock(&data->mutex);
> +
> + for (i = 0; i < SX932X_NUM_CHANNELS; i++)
> + if (test_bit(i, indio_dev->active_scan_mask)) {
for_each_set_bit
> + ret = sx932x_inc_chan_users(data, i);
> + if (ret)
> + break;
> + }
> +
> + if (ret)
> + for (i = i - 1; i >= 0; i--)
> + if (test_bit(i, indio_dev->active_scan_mask))
> + sx932x_dec_chan_users(data, i);
Pull this error block out of the main flow via a goto.
> +
> + mutex_unlock(&data->mutex);
> +
> + return ret;
> +}
> +
> +static int sx932x_buffer_predisable(struct iio_dev *indio_dev)
> +{
> + struct sx932x_data *data = iio_priv(indio_dev);
> + int ret = 0, i;
> +
> + iio_triggered_buffer_predisable(indio_dev);
> +
> + mutex_lock(&data->mutex);
> +
> + for (i = 0; i < SX932X_NUM_CHANNELS; i++)
> + if (test_bit(i, indio_dev->active_scan_mask)) {
for_each_set_bit
> + ret = sx932x_dec_chan_users(data, i);
> + if (ret)
> + break;
> + }
> +
> + if (ret)
> + for (i = i - 1; i >= 0; i--)
> + if (test_bit(i, indio_dev->active_scan_mask))
> + sx932x_inc_chan_users(data, i);
I'm finding it hard to understand why this complexity is needed.
IIO handles muxing multiple readers explicitly with all the necessary
reference counting. What case are we missing that this covers?
(Ah, I think I found it above. Events need the channels enabled..)
> +
> + mutex_unlock(&data->mutex);
> +
> + return ret;
> +}
> +
> +static const struct iio_buffer_setup_ops sx932x_buffer_setup_ops = {
> + .preenable = sx932x_buffer_preenable,
> + .postenable = iio_triggered_buffer_postenable,
> + .predisable = sx932x_buffer_predisable,
> +};
> +#define SX932X_REG_CONFIG(_property_prefix, _property_name, _reg_addr, \
> + _def_value) \
> + { \
> + .property = _property_prefix ",reg_" _property_name, \
This property prefix stuff will hopefully go away once we've dropped the
DT binding. As I've said the ACPI binding is hideous if there are actual
instances of what you describe. I would prefer to loose the abstraction here
and have that all named explicitly where we use it.
> + .reg_addr = SX932X_REG_##_reg_addr, .def_value = _def_value, \
> + }
> +
> +#define SX932X_DEFAULT_REG_CONFIGS(_property_prefix) \
> + { \
> + { \
> + .property = NULL, \
> + .reg_addr = SX932X_REG_ADV_CTRL11, \
> + .def_value = (5 << 4) | 2, \
Do we know what these magic values are? Please use defines to make the clear.
It would be much nicer to see the device configured element by element in code
rather than a register dump like this.
> + }, \
Fix the odd indenting.
> + { \
> + .property = NULL, \
> + .reg_addr = SX932X_REG_ADV_CTRL12, \
> + .def_value = (11 << 4) | 5, \
> + }, \
> + SX932X_REG_CONFIG(_property_prefix, "adv_ctrl10", \
> + ADV_CTRL10, (5 << 4) | 12), \
> + SX932X_REG_CONFIG(_property_prefix, "afe_ctrl4", \
> + AFE_CTRL4, 7), \
> + SX932X_REG_CONFIG(_property_prefix, "afe_ctrl7", \
> + AFE_CTRL7, 7), \
> + SX932X_REG_CONFIG(_property_prefix, "afe_ctrl9", \
> + AFE_CTRL9, 15), \
> + SX932X_REG_CONFIG(_property_prefix, "prox_ctrl0", \
> + PROX_CTRL0, (2 << 3) | 2), \
> + SX932X_REG_CONFIG(_property_prefix, "prox_ctrl1", \
> + PROX_CTRL1, (2 << 3) | 2), \
> + SX932X_REG_CONFIG(_property_prefix, "prox_ctrl2", \
> + PROX_CTRL2, 0x80 | 16), \
> + SX932X_REG_CONFIG(_property_prefix, "prox_ctrl4", \
> + PROX_CTRL4, (1 << 3) | 4), \
> + SX932X_REG_CONFIG(_property_prefix, "prox_ctrl5", \
> + PROX_CTRL5, (1 << 4) | 2), \
> + SX932X_REG_CONFIG(_property_prefix, "prox_ctrl6", \
> + PROX_CTRL6, 60), \
> + SX932X_REG_CONFIG(_property_prefix, "prox_ctrl7", \
> + PROX_CTRL7, 88), \
> + SX932X_REG_CONFIG(_property_prefix, "adv_ctrl16", \
> + ADV_CTRL16, (3 << 4) | (2 << 2)), \
> + SX932X_REG_CONFIG(_property_prefix, "adv_ctrl17", \
> + ADV_CTRL17, (5 << 4) | 6), \
> + SX932X_REG_CONFIG(_property_prefix, "adv_ctrl18", \
> + ADV_CTRL18, (3 << 4) | 3), \
> + SX932X_REG_CONFIG(_property_prefix, "gnrl_ctrl1", \
> + GNRL_CTRL1, 0x2f), \
> + { \
> + .property = NULL, \
> + .reg_addr = 0, \
> + .def_value = 0, \
> + }, \
> + }
> +
> +#ifdef CONFIG_ACPI
ACPI and DT will typically be configured on for example a defconfig arm64 build.
> +static const struct sx932x_reg_config sx9320_reg_config[] =
> + SX932X_DEFAULT_REG_CONFIGS(SX9320_ACPI_NAME);
> +static const struct sx932x_reg_config sx9321_reg_config[] =
> + SX932X_DEFAULT_REG_CONFIGS(SX9321_ACPI_NAME);
> +static const struct sx932x_reg_config sx9324_reg_config[] =
> + SX932X_DEFAULT_REG_CONFIGS(SX9324_ACPI_NAME);
> +#else
> +static const struct sx932x_reg_config sx9320_reg_config[] =
> + SX932X_DEFAULT_REG_CONFIGS("semtech,");
> +static const struct sx932x_reg_config sx9321_reg_config[] =
> + SX932X_DEFAULT_REG_CONFIGS("semtech,");
> +static const struct sx932x_reg_config sx9324_reg_config[] =
> + SX932X_DEFAULT_REG_CONFIGS("semtech,");
> +#endif
> +static const struct sx932x_reg_config *sx932x_reg_configs[] = {
> + sx9320_reg_config, sx9321_reg_config, sx9324_reg_config
> +};
> +static int sx932x_read_register_property(struct device *dev,
> + const struct sx932x_reg_config *cfg,
> + u8 *value)
> +{
This looks like a lot of 'magic' register fun. Are the lots of random
ACPI DSDT files out there with different values? Can we figure out
a good common default?
This is a truely horrible mess. Note that I'm definitely not keen on
propagating this into DT, but if we have cope with ACPI on existing
systems I guess we can...
> +#ifdef CONFIG_ACPI
> + const union acpi_object *acpi_value = NULL;
> +#endif
It's perfectly possible to have both ACPI and OF built, so you need to handle
that case nicely.
> + int ret;
> +
> + if ((dev == NULL) || (cfg->property == NULL)) {
if (!dev || !cfg->property) {
> + *value = cfg->def_value;
> + return 0;
> + }
> +
> +#ifdef CONFIG_ACPI
> + ret = acpi_dev_get_property(ACPI_COMPANION(dev), cfg->property,
> + ACPI_TYPE_INTEGER, &acpi_value);
> +#else
> + ret = of_property_read_u8(dev->of_node, cfg->property, value);
> +#endif
> + if (ret)
> + *value = cfg->def_value;
> +#ifdef CONFIG_ACPI
> + *value = acpi_value ? (u8)acpi_value->integer.value : cfg->def_value;
> +#endif
> + return 0;
> +}
> +
> +static int sx932x_load_config(struct device *dev, struct sx932x_data *data)
> +{
> + u8 val;
> + int i, ret;
> + const struct sx932x_reg_config *cfg;
> +
> + for (i = 0; data->reg_config[i].reg_addr != 0; i++) {
> + cfg = &data->reg_config[i];
> + ret = sx932x_read_register_property(dev, cfg, &val);
> + if (ret < 0)
> + return ret;
> + ret = regmap_write(data->regmap, cfg->reg_addr, val);
> + if (ret < 0)
> + return ret;
> + }
> + return 0;
> +}
> +
> +/* Activate all channels and perform an initial compensation. */
> +static int sx932x_init_compensation(struct iio_dev *indio_dev)
> +{
> + struct sx932x_data *data = iio_priv(indio_dev);
> + int i, ret;
> + bool success = false;
> + unsigned int val;
> +
> + for (i = 100; i >= 0; i--) {
> + usleep_range(10000, 20000);
msleep(10);
> + ret = regmap_read(data->regmap, SX932X_REG_STAT2, &val);
> + if (ret < 0)
> + goto out;
> + if (!(val & SX932X_COMPSTAT_MASK)) {
> + success = true;
return 0; instead.
> + break;
> + }
> + }
> +
> + if (success) {
> + dev_info(&data->client->dev,
> + "initial compensation success");
No need to report the good path. Which also menas you can just return
directly where you currently set success.
> + } else {
> + dev_err(&data->client->dev,
> + "initial compensation timed out: 0x%02x", val);
> + ret = -ETIMEDOUT;
return -ETIMEDOUT;
> + }
> +
> +out:
> + return ret;
> +}
> +
> +static int sx932x_init_device(struct iio_dev *indio_dev)
> +{
> + struct device *dev = &indio_dev->dev;
> + struct sx932x_data *data = iio_priv(indio_dev);
> + int ret;
> + unsigned int val;
> +
> + ret = regmap_read(data->regmap, SX932X_REG_WHOAMI, &val);
> + if (ret < 0 || (val != SX9320_WHOAMI && val != SX9321_WHOAMI &&
> + val != SX9324_WHOAMI)) {
If ret is less than 0, return that error it may be more meaningful.
Then check the value separately.
> + dev_err(&data->client->dev,
> + "Unable to identify the chip: %d - 0x%02x", ret, val);
> + return -ENODEV;
> + }
> +
> + ret = regmap_write(data->regmap, SX932X_REG_IRQ_MSK, 0);
> + if (ret < 0)
> + return ret;
> +
> + ret = regmap_write(data->regmap, SX932X_REG_RESET,
> + SX932X_SOFT_RESET);
> + if (ret < 0)
> + return ret;
> +
> + // wait for the reset to be completed
/* */
> + usleep_range(10000, 20000);
msleep given the large values would be fine I think.
> +
> + ret = regmap_write(data->regmap, SX932X_REG_RESET, 0);
> + if (ret < 0)
> + return ret;
> +
> + ret = regmap_read(data->regmap, SX932X_REG_IRQ_SRC, &val);
> + if (ret < 0)
> + return ret;
If this is a read to clear register, please add a comment saying so.
Right now it just looks odd to read a value and do nothing with it.
> +
> + ret = sx932x_load_config(dev, data);
> + if (ret < 0)
> + return ret;
> +
> + return sx932x_init_compensation(indio_dev);
> +}
> +
> +static int sx932x_probe(struct i2c_client *client,
> + const struct i2c_device_id *i2c_id)
> +{
> + int ret;
> + struct iio_dev *indio_dev;
> + const struct acpi_device_id *acpi_id;
> + struct sx932x_data *data;
> +
> + indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*data));
> + if (indio_dev == NULL)
> + return -ENOMEM;
> +
> + data = iio_priv(indio_dev);
> + data->client = client;
> + if (i2c_id) {
> + data->reg_config = sx932x_reg_configs[i2c_id->driver_data];
> + } else if (ACPI_HANDLE(&client->dev)) {
> + acpi_id = acpi_match_device(
> + client->dev.driver->acpi_match_table,
> + &client->dev);
> + if (!acpi_id) {
> + dev_err(&client->dev, "No driver data\n");
> + return -EINVAL;
> + }
> + data->reg_config = sx932x_reg_configs[acpi_id->driver_data];
> + } else {
> + return -ENODEV;
> + }
> + mutex_init(&data->mutex);
> + init_completion(&data->completion);
> + data->trigger_enabled = false;
That will actually be the default and is what people would expect (data is
kzalloc'd).
> +
> + data->regmap = devm_regmap_init_i2c(client, &sx932x_regmap_config);
> + if (IS_ERR(data->regmap))
> + return PTR_ERR(data->regmap);
blank line here would help readability slightly.
> + ACPI_COMPANION_SET(&indio_dev->dev, ACPI_COMPANION(&client->dev));
> + indio_dev->dev.parent = &client->dev;
> + indio_dev->name = SX932X_DRIVER_NAME;
Please use the actual part name, not the driver name.
> + indio_dev->channels = sx932x_channels;
> + indio_dev->num_channels = ARRAY_SIZE(sx932x_channels);
> + indio_dev->info = &sx932x_info;
> + indio_dev->modes = INDIO_DIRECT_MODE;
> + i2c_set_clientdata(client, indio_dev);
> +
> + ret = sx932x_init_device(indio_dev);
> + if (ret < 0) {
> + dev_err(&client->dev, "tried to init sx932x - error %d\n",
> + ret);
> + return ret;
> + }
> +
> + if (client->irq <= 0)
> + dev_warn(&client->dev, "no valid irq found\n");
If it's not been provided, that was probably deliberate and the driver
works fine like that (if not this should be an error) so drop this
print.
> + else {
> + ret = devm_request_threaded_irq(&client->dev, client->irq,
> + sx932x_irq_handler, sx932x_irq_thread_handler,
> + IRQF_TRIGGER_LOW | IRQF_ONESHOT,
> + SX932X_IRQ_NAME, indio_dev);
> + if (ret < 0)
> + return ret;
> +
> + data->trig = devm_iio_trigger_alloc(&client->dev,
> + "%s-dev%d", indio_dev->name, indio_dev->id);
> + if (!data->trig)
> + return -ENOMEM;
> +
> + data->trig->dev.parent = &client->dev;
> + data->trig->ops = &sx932x_trigger_ops;
> + iio_trigger_set_drvdata(data->trig, indio_dev);
> +
> + ret = iio_trigger_register(data->trig);
devm_iio_trigger_register
> + if (ret)
> + return ret;
> + }
> +
> + ret = devm_iio_triggered_buffer_setup(&client->dev, indio_dev,
> + NULL, sx932x_trigger_handler,
> + &sx932x_buffer_setup_ops);
> + if (ret < 0)
> + goto out_trigger_unregister;
> +
> + ret = devm_iio_device_register(&client->dev, indio_dev);
> + if (ret < 0)
> + goto out_trigger_unregister;
> +
> + return 0;
> +
> +out_trigger_unregister:
> + if (client->irq > 0)
> + iio_trigger_unregister(data->trig);
> +
> + return ret;
> +}
> +
> +static int sx932x_remove(struct i2c_client *client)
> +{
> + struct iio_dev *indio_dev = i2c_get_clientdata(client);
> + struct sx932x_data *data = iio_priv(indio_dev);
> +
> + if (client->irq > 0)
> + iio_trigger_unregister(data->trig);
> + kfree(data->buffer);
This feels like something that could have been done with managed
interfaces. This is also racy. You can't manually unwind a few
random items by hand and use the devm_ interfaces for the rest as
the unwind order is not obviously correct (should be reverse of
the set up order).
> + return 0;
> +}
> +
> +static int __maybe_unused sx932x_suspend(struct device *dev)
> +{
> + struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(dev));
> + struct sx932x_data *data = iio_priv(indio_dev);
> + int ret;
> +
> + mutex_lock(&data->mutex);
> + ret = regmap_read(data->regmap, SX932X_REG_GNRL_CTRL1,
> + &data->suspend_reg_gnrl_ctrl1);
> +
> + if (ret < 0)
> + goto out;
> +
> + // disable all phases, send the device to sleep
/* style comments */
> + ret = regmap_write(data->regmap, SX932X_REG_GNRL_CTRL1, 0);
> +
> +out:
> + mutex_unlock(&data->mutex);
> + return ret;
> +}
> +
> +static int __maybe_unused sx932x_resume(struct device *dev)
> +{
> + struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(dev));
> + struct sx932x_data *data = iio_priv(indio_dev);
> + int ret;
> +
> + mutex_lock(&data->mutex);
> + ret = regmap_write(data->regmap, SX932X_REG_GNRL_CTRL1,
> + data->suspend_reg_gnrl_ctrl1);
> + mutex_unlock(&data->mutex);
> +
> + return ret;
> +}
> +
> +static const struct dev_pm_ops sx932x_pm_ops = {
> + SET_SYSTEM_SLEEP_PM_OPS(sx932x_suspend, sx932x_resume)
> +};
> +#ifdef CONFIG_ACPI
> +static const struct acpi_device_id sx932x_acpi_match[] = {
> + { SX9320_ACPI_NAME, 0 },
> + { SX9321_ACPI_NAME, 1 },
> + { SX9324_ACPI_NAME, 2 },
> + {},
> +};
> +MODULE_DEVICE_TABLE(acpi, sx932x_acpi_match);
> +#endif
> +#ifdef CONFIG_OF
Don't protect of bindings like this. They can be used via
the obscure ACPI route that uses the DT bindings.
Just let them always be built.
> +static const struct of_device_id sx932x_of_match[] = {
> + { .compatible = "semtech,sx9320" },
> + { .compatible = "semtech,sx9321" },
> + { .compatible = "semtech,sx9324" },
> + {}
> +};
> +MODULE_DEVICE_TABLE(of, sx932x_of_match);
> +#endif
> +static const struct i2c_device_id sx932x_id[] = {
> + { "sx9320", 0 },
> + { "sx9321", 1 },
> + { "sx9324", 2 },
> + {},
> +};
> +MODULE_DEVICE_TABLE(i2c, sx932x_id);
> +
> +static struct i2c_driver sx932x_driver = {
> + .driver = {
> + .name = SX932X_DRIVER_NAME,
> + .acpi_match_table = ACPI_PTR(sx932x_acpi_match),
> + .of_match_table = of_match_ptr(sx932x_of_match),
> + .pm = &sx932x_pm_ops,
> + },
> + .probe = sx932x_probe,
> + .remove = sx932x_remove,
> + .id_table = sx932x_id,
> +};
> +module_i2c_driver(sx932x_driver);
> +
> +MODULE_AUTHOR("Enrico Granata <egranata@xxxxxxxxxxxx>");
> +MODULE_AUTHOR("Gwendal Grignou <gwendal@xxxxxxxxxxxx>");
> +MODULE_AUTHOR("Pat Plunkett <patplunkett@xxxxxxxxxx");
> +MODULE_DESCRIPTION("Driver for Semtech SX9320/SX9321/SX9324 proximity sensor");
> +MODULE_LICENSE("GPL v2");
