On Fri, Apr 10, 2020 at 4:52 AM Andy Shevchenko
<andy.shevchenko@xxxxxxxxx> wrote:
>
> One side note. Whenever you prepare patches do the following:
> - use -v<n> to git-format-patch to versioning it by using standard
> template (<n> is a version number)
> - use --thread to make messages in one bunch
> - resend entire series
> - do not send patches more often than once per 24 hours
>
Thanks for the feedback
> On Thu, Apr 9, 2020 at 2:54 AM Daniel Campello <campello@xxxxxxxxxxxx> wrote:
> >
> > Add SEMTECH SX9310/9311 driver.
> >
> > The device has the following entry points:
> >
> > Usual frequency:
> > - sampling_frequency
> > - sampling_frequency_available
> >
> > Instant reading of current values for different sensors:
> > - in_proximity0_raw
> > - in_proximity1_raw
> > - in_proximity2_raw
> > - in_proximity3_comb_raw
> > and associated events in events/
> >
> > Signed-off-by: Gwendal Grignou <gwendal@xxxxxxxxxxxx>
> > Signed-off-by: Enrico Granata <egranata@xxxxxxxxxxxx>
>
> This is not understandable. Are they who helped you develop the code
> (we have a special tag, i.e. Co-developed-by in addition to SoB), or
> just people in the middle? Then the question is, how come author is
> you and not Gwendal?
>
This patch was initially developed by Gwendal and Enrico (here:
crrev.com/c/1089826).
> ...
>
> > +What: /sys/bus/iio/devices/iio:deviceX/in_proximity3_comb_raw
>
> > +Description:
> > + Proximity measurement indicating that some object is
> > + near the combined sensor. The combined sensor presents
> > + proximity measurements constructed by hardware by
> > + combining measurements taken from a given set of
> > + physical sensors.
>
> I'm wondering if we rather have some standard tag across sensors for
> combined values.
> It's particular to proximity sensors only? Would it stay like this
> forever? Won't we come to the very heavy and noisy ABI if more sensors
> are gaining something like this?
>
> ...
>
> > + * Copyright 2018 Google LLC.
>
> I remember you did changes in this year...
> Is your changes copyrighted by G company?
>
> > + * Reworked April 2019 by Evan Green <evgreen@xxxxxxxxxxxx>
>
> in April
>
> > + * and January 2020 by Daniel Campello <campello@xxxxxxxxxxxx>
>
> in January
>
> And since it looks like a sentence, put period at the end of it.
>
> ...
>
> > +#include <linux/acpi.h>
> > +#include <linux/delay.h>
> > +#include <linux/i2c.h>
> > +#include <linux/irq.h>
> > +#include <linux/kernel.h>
> > +#include <linux/module.h>
> > +#include <linux/pm.h>
> > +#include <linux/regmap.h>
> > +#include <linux/slab.h>
>
> ...
>
> > +#define SX9310_REG_IRQ_MSK 0x03
>
> Is MSK abbreviation in datasheet? Please spell it how it's in datasheet.
Yes, they indead use MSK.
>
> > +#define SX9310_CONVDONE_IRQ BIT(3)
> > +#define SX9310_FAR_IRQ BIT(5)
> > +#define SX9310_CLOSE_IRQ BIT(6)
>
> > +#define SX9310_EVENT_IRQ (SX9310_FAR_IRQ | SX9310_CLOSE_IRQ)
>
> Is it listed in hardware, or simple an addition to have it easier to
> handle in the code?
It was an addition, removed in v9
>
> ...
>
> > +#define SX9310_REG_PROX_CTRL0_EN_MASK 0x0F
>
> GENMASK()
>
> ...
>
> > +#define SX9310_REG_PROX_CTRL2_COMBMODE_ALL 0x80
>
> BIT() ?
>
> > +#define SX9310_REG_PROX_CTRL2_SHIELDEN_DYNAMIC 0x04
>
> BIT() ?
>
> If it is not a one bit value, better to use (value << shift) and
> perhaps mention other possibilities.
> But for current case looks like anyway BIT() macro can be suitable.
>
> You can revisit all the rest definitions, but it's up to you, just try
> to be close to the datasheet.
>
> ...
>
> > +struct sx9310_data {
>
> > + struct i2c_client *client;
>
> > +};
>
> ...
>
> > +static ssize_t sx9310_show_samp_freq_avail(struct device *dev,
> > + struct device_attribute *attr,
> > + char *buf)
> > +{
> > + size_t len = 0;
> > + int i;
> > +
> > + for (i = 0; i < ARRAY_SIZE(sx9310_samp_freq_table); i++)
> > + len += scnprintf(buf + len, PAGE_SIZE - len, "%d.%d ",
> > + sx9310_samp_freq_table[i].val,
> > + sx9310_samp_freq_table[i].val2);
> > + buf[len - 1] = '\n';
> > + return len;
> > +}
> > +static IIO_DEV_ATTR_SAMP_FREQ_AVAIL(sx9310_show_samp_freq_avail);
>
> Jonathan, what is the best practice now with this kind of output? I
> think that IIO core provides a unified format to out this.
>
> ...
>
> > +static int sx9310_read_prox_data(struct sx9310_data *data,
> > + const struct iio_chan_spec *chan, __be16 *val)
> > +{
> > + int ret;
> > +
> > + ret = regmap_write(data->regmap, SX9310_REG_SENSOR_SEL, chan->channel);
>
> > + if (ret < 0)
>
> Do you need all these ' < 0' checks? Revisit code and drop where it's
> not needed, like here.
>
> > + return ret;
> > +
> > + return regmap_bulk_read(data->regmap, chan->address, val,
>
> > + sizeof(__be16));
>
> sizeof(*val)
>
> > +}
>
> ...
>
> > +static int sx9310_read_proximity(struct sx9310_data *data,
> > + const struct iio_chan_spec *chan, int *val)
> > +{
>
> > + int ret = 0;
>
> Unneede assignment.
>
> > + if (ret < 0)
> > + goto out_disable_irq;
>
> And if the path was non-IRQ one do we need to call ..._disable_irq()?
>
> > + if (ret < 0)
> > + goto out_disable_irq;
>
> Ditto.
>
> > + ret = sx9310_disable_irq(data, SX9310_CONVDONE_IRQ);
> > + if (ret < 0)
> > + goto out_put_channel;
>
> Ditto.
>
> > + return IIO_VAL_INT;
> > +
> > +out_disable_irq:
> > + sx9310_disable_irq(data, SX9310_CONVDONE_IRQ);
> > +out_put_channel:
> > + sx9310_put_read_channel(data, chan->channel);
> > +out:
> > + mutex_unlock(&data->mutex);
> > +
> > + return ret;
> > +}
>
> ...
>
> > +static int sx9310_read_samp_freq(struct sx9310_data *data, int *val, int *val2)
> > +{
> > + unsigned int regval;
>
> > + int ret = regmap_read(data->regmap, SX9310_REG_PROX_CTRL0, ®val);
>
> Slightly better to assign exactly before use, i.e. below before if (ret).
> For now it's okay, but rationale is that if you need to inject some
> code in between in the future, this will become an additional burden.
>
> > + if (ret < 0)
> > + return ret;
>
> > +}
>
> ...
>
> > + for_each_set_bit(chan, &data->chan_event, SX9310_NUM_CHANNELS) {
> > + int dir;
> > + u64 ev;
>
> > + bool new_prox = val & BIT(chan);
>
> Similar: slightly better to assign before use.
>
> > +
> > + if (new_prox == data->prox_stat[chan])
> > + /* No change on this channel. */
> > + continue;
>
> > + }
>
> ...
>
> > +#define SX_INIT(_reg, _def) \
> > + { \
> > + .reg = SX9310_REG_##_reg, \
> > + .def = _def, \
> > + }
>
> I think this macro makes it harder to read, and better simple to put
> these initializers directly into below structure, but it's up to you.
>
> ...
>
> > + ret = regmap_read_poll_timeout(data->regmap, SX9310_REG_STAT1, val,
> > + !(val & SX9310_COMPSTAT_MASK), 20000,
> > + 2000000);
> > + if (ret == -ETIMEDOUT)
> > + dev_err(&data->client->dev,
> > + "initial compensation timed out: 0x%02x", val);
> > +
>
> > + regmap_write(data->regmap, SX9310_REG_PROX_CTRL0, ctrl0);
>
> Even in timeout case?
>
> > + return ret;
>
> ...
>
> > + if ((long)device_get_match_data(dev) != whoami)
>
> unsigned int long ddata;
>
> ddata = (uintptr_t)device_get_match_data(dev);
> if (ddata != whoami)
> ...
>
> > + dev_err(dev, "WHOAMI does not match device data: %d", whoami);
>
> If it's error, why not bail out here?
> Or i.o.w. what is the usefulness of the driver data?
>
> > + switch (whoami) {
> > + case SX9310_WHOAMI_VALUE:
> > + indio_dev->name = "sx9310";
> > + break;
> > + case SX9311_WHOAMI_VALUE:
> > + indio_dev->name = "sx9311";
> > + break;
> > + default:
> > + dev_err(dev, "unexpected WHOAMI response: %u", whoami);
> > + return -ENODEV;
> > + }
>
> ...
>
> > + data->trig = devm_iio_trigger_alloc(
> > + dev, "%s-dev%d", indio_dev->name, indio_dev->id);
>
> Indentation issues.
>
> ...
>
> > + mutex_lock(&data->mutex);
> > + ret = regmap_read(data->regmap, SX9310_REG_PROX_CTRL0,
> > + &data->suspend_ctrl0);
>
> > +
>
> Blank line in a wrong position?
>
> > + if (ret)
> > + goto out;
> > +
>
> ...
>
> > + mutex_lock(&data->mutex);
> > + ret = regmap_write(data->regmap, SX9310_REG_PAUSE, 1);
> > + if (ret)
> > + goto out;
> > +
> > + ret = regmap_write(data->regmap, SX9310_REG_PROX_CTRL0,
> > + data->suspend_ctrl0);
> > +
> > +out:
> > + mutex_unlock(&data->mutex);
> > +
>
> > + enable_irq(data->client->irq);
>
> So, you enable IRQ despite the error. Why?
>
> ...
>
> > +static const struct dev_pm_ops sx9310_pm_ops = {
> > + SET_SYSTEM_SLEEP_PM_OPS(sx9310_suspend, sx9310_resume)
> > +};
>
> ...
>
> > + .acpi_match_table = ACPI_PTR(sx9310_acpi_match),
> > + .of_match_table = of_match_ptr(sx9310_of_match),
>
> Drop these macros. They more harmful than useful, i.e. you will get
> compiler warning.
> If you would like to use them, you have to guard ID tables with ugly ifdeffery.
>
>
> --
> With Best Regards,
> Andy Shevchenko
Thank you for the review,
Daniel Campello
