Re: [PATCH v3 2/4] iio: proximity: Add sx9360 support

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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, &regval);
> > +
> > +     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;
> > +}
> > +
>



[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]


  Powered by Linux