On Sun, 12 Dec 2021 18:40:55 -0800
Gwendal Grignou <gwendal@xxxxxxxxxxxx> wrote:
> A simplified version of SX9324, it only have one pin and
> 2 phases (aka channels).
> Only one event is presented.
>
> Signed-off-by: Gwendal Grignou <gwendal@xxxxxxxxxxxx>
You don't use the modifier defined in the previous
patch...
> ---
> Changes since v2:
> - Fix issues reported during sx9324 driver review:
> - fix include with iwyu
> - Remove call to ACPI_PTR to prevent unused variable warning.
> - Fix panic when setting frequency to 0.
> - Add offset to decipher interrupt register
> - Fix default register value.
>
> Changes since v1:
> - Remove SX9360_DRIVER_NAME
> - Simplify whoami function
> - Define WHOAMI register value internally.
> - Handle negative values when setting sysfs parameters.
>
> drivers/iio/proximity/Kconfig | 14 +
> drivers/iio/proximity/Makefile | 1 +
> drivers/iio/proximity/sx9360.c | 807 +++++++++++++++++++++++++++++++++
> 3 files changed, 822 insertions(+)
> create mode 100644 drivers/iio/proximity/sx9360.c
>
....
> +static const struct iio_chan_spec sx9360_channels[] = {
> + {
> + .type = IIO_PROXIMITY,
> + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
> + BIT(IIO_CHAN_INFO_HARDWAREGAIN),
> + .info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ),
> + .info_mask_separate_available =
> + BIT(IIO_CHAN_INFO_HARDWAREGAIN),
> + .info_mask_shared_by_all_available =
> + BIT(IIO_CHAN_INFO_SAMP_FREQ),
> + .extend_name = "reference",
You defined the modifier for this and then didn't use it?
I've suggested in review of patch 1 you might want to use label though
via the read_label() callback.
> + .address = SX9360_REG_USEFUL_PHR_MSB,
> + .channel = 0,
> + .scan_index = 0,
> + .scan_type = {
> + .sign = 's',
> + .realbits = 12,
> + .storagebits = 16,
> + .endianness = IIO_BE,
> + },
> + },
> + {
> + .type = IIO_PROXIMITY,
> + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
> + BIT(IIO_CHAN_INFO_HARDWAREGAIN),
> + .info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ),
> + .info_mask_separate_available =
> + BIT(IIO_CHAN_INFO_HARDWAREGAIN),
> + .info_mask_shared_by_all_available =
> + BIT(IIO_CHAN_INFO_SAMP_FREQ),
> + .address = SX9360_REG_USEFUL_PHM_MSB,
> + .event_spec = sx_common_events,
> + .num_event_specs = ARRAY_SIZE(sx_common_events),
> + .channel = 1,
> + .scan_index = 1,
> + .scan_type = {
> + .sign = 's',
> + .realbits = 12,
> + .storagebits = 16,
> + .endianness = IIO_BE,
> + },
> + },
> + IIO_CHAN_SOFT_TIMESTAMP(2),
> +};
> +
...
> +
> +static int sx9360_read_samp_freq(struct sx_common_data *data,
> + int *val, int *val2)
> +{
> + int ret, divisor;
> + __be16 buf;
> +
> + ret = regmap_bulk_read(data->regmap, SX9360_REG_GNRL_CTRL1,
> + &buf, sizeof(buf));
> + if (ret < 0)
> + return ret;
> + divisor = be16_to_cpu(buf);
> + if (divisor == 0) {
> + *val = 0;
> + return IIO_VAL_INT;
> + }
> +
> + *val = SX9360_FOSC_HZ;
> + *val2 = be16_to_cpu(buf) * 8192;
*val2 = divisor * 8192;?
> +
> + return IIO_VAL_FRACTIONAL;
> +}
> +
...
> +static int sx9360_write_raw(struct iio_dev *indio_dev,
> + const struct iio_chan_spec *chan, int val, int val2,
> + long mask)
> +{
> + struct sx_common_data *data = iio_priv(indio_dev);
> +
> + switch (mask) {
> + case IIO_CHAN_INFO_SAMP_FREQ:
> + return sx9360_set_samp_freq(data, val, val2);
> + case IIO_CHAN_INFO_HARDWAREGAIN:
> + return sx9360_write_gain(data, chan, val);
> + }
> +
Slight preference for this as a default in the switch.
> + return -EINVAL;
> +}
...
> +static int sx9360_check_whoami(struct device *dev,
> + struct iio_dev *indio_dev)
Will fit on one line under 80 chars I think..
> +{
> + /*
> + * Only one sensor for this driver. Assuming the device tree
> + * is correct, just set the sensor name.
> + */
> + indio_dev->name = "sx9360";
> + return 0;
> +}
> +
> +
> +static int __maybe_unused sx9360_suspend(struct device *dev)
> +{
> + struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(dev));
I don't feel particularly strongly about this, as there are arguments
either way but this is the same as
struct iio_dev *indio_dev = dev_get_drvdata(dev);
> + struct sx_common_data *data = iio_priv(indio_dev);
> + unsigned int regval;
> + int ret;
> +
> + disable_irq_nosync(data->client->irq);
> +
> + mutex_lock(&data->mutex);
> + ret = regmap_read(data->regmap, SX9360_REG_GNRL_CTRL0, ®val);
> +
> + data->suspend_ctrl =
> + FIELD_GET(SX9360_REG_GNRL_CTRL0_PHEN_MASK, regval);
> +
> + if (ret < 0)
> + goto out;
> +
> + /* Disable all phases, send the device to sleep. */
> + ret = regmap_write(data->regmap, SX9360_REG_GNRL_CTRL0, 0);
> +
> +out:
> + mutex_unlock(&data->mutex);
> + return ret;
> +}
> +
