On Thu, Dec 16, 2021 at 8:16 AM Jonathan Cameron
<jic23@xxxxxxxxxxxxxxxxxxxxx> wrote:
>
> 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.
I see, I should have used .cannel2 instead of extend_name. Using label now.
>
>
> > + .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;?
Done
>
> > +
> > + 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.
Done, changed sx9360_read_avail() as well.
>
> > + 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..
Done.
>
> > +{
> > + /*
> > + * 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);
>
Done in both suspend and resume.
> > + 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;
> > +}
> > +
>
