| Message ID | 20200728151258.1222876-1-campello@chromium.org |
|---|---|
| Headers | show |
| Series | sx9310 iio driver updates | expand |
On Tue, Jul 28, 2020 at 6:14 PM Daniel Campello <campello@chromium.org> wrote: > > Fixes enable/disable irq handling at various points. The driver needs to > only enable/disable irqs if there is an actual irq handler installed. > - enable_irq(data->client->irq); > + if (!ret) > + enable_irq(data->client->irq); > > return ret; > } Can it be a usual pattern? if (ret) return ret; ... return 0;
On Tue, Jul 28, 2020 at 6:16 PM Daniel Campello <campello@chromium.org> wrote: > > Avoids unused warnings due to acpi/of table macros. > At the same time I would check if mod_devicetable.h is included. > Signed-off-by: Daniel Campello <campello@chromium.org> > Reported-by: kbuild test robot <lkp@intel.com>
On Tue, Jul 28, 2020 at 6:15 PM Daniel Campello <campello@chromium.org> wrote: > > Uses .probe_new in place of .probe. Also uses device_get_match_data() > for whoami matching. Good one! Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com> > Signed-off-by: Daniel Campello <campello@chromium.org> > --- > > drivers/iio/proximity/sx9310.c | 37 ++++++++++++---------------------- > 1 file changed, 13 insertions(+), 24 deletions(-) > > diff --git a/drivers/iio/proximity/sx9310.c b/drivers/iio/proximity/sx9310.c > index 0fb88ad66f7342..de52afd7c13333 100644 > --- a/drivers/iio/proximity/sx9310.c > +++ b/drivers/iio/proximity/sx9310.c > @@ -138,7 +138,7 @@ struct sx9310_data { > struct completion completion; > unsigned int chan_read, chan_event; > int channel_users[SX9310_NUM_CHANNELS]; > - int whoami; > + unsigned int whoami; > }; > > static const struct iio_event_spec sx9310_events[] = { > @@ -859,24 +859,15 @@ static int sx9310_init_device(struct iio_dev *indio_dev) > > static int sx9310_set_indio_dev_name(struct device *dev, > struct iio_dev *indio_dev, > - const struct i2c_device_id *id, int whoami) > + unsigned int whoami) > { > - const struct acpi_device_id *acpi_id; > - > - /* id will be NULL when enumerated via ACPI */ > - if (id) { > - if (id->driver_data != whoami) > - dev_err(dev, "WHOAMI does not match i2c_device_id: %s", > - id->name); > - } else if (ACPI_HANDLE(dev)) { > - acpi_id = acpi_match_device(dev->driver->acpi_match_table, dev); > - if (!acpi_id) > - return -ENODEV; > - if (acpi_id->driver_data != whoami) > - dev_err(dev, "WHOAMI does not match acpi_device_id: %s", > - acpi_id->id); > - } else > + unsigned int long ddata; > + > + ddata = (uintptr_t)device_get_match_data(dev); > + if (ddata != whoami) { > + dev_err(dev, "WHOAMI does not match device data: %u", whoami); > return -ENODEV; > + } > > switch (whoami) { > case SX9310_WHOAMI_VALUE: > @@ -893,8 +884,7 @@ static int sx9310_set_indio_dev_name(struct device *dev, > return 0; > } > > -static int sx9310_probe(struct i2c_client *client, > - const struct i2c_device_id *id) > +static int sx9310_probe(struct i2c_client *client) > { > int ret; > struct iio_dev *indio_dev; > @@ -920,8 +910,7 @@ static int sx9310_probe(struct i2c_client *client, > return ret; > } > > - ret = sx9310_set_indio_dev_name(&client->dev, indio_dev, id, > - data->whoami); > + ret = sx9310_set_indio_dev_name(&client->dev, indio_dev, data->whoami); > if (ret < 0) > return ret; > > @@ -1034,8 +1023,8 @@ static const struct acpi_device_id sx9310_acpi_match[] = { > MODULE_DEVICE_TABLE(acpi, sx9310_acpi_match); > > static const struct of_device_id sx9310_of_match[] = { > - { .compatible = "semtech,sx9310" }, > - { .compatible = "semtech,sx9311" }, > + { .compatible = "semtech,sx9310", (void *)SX9310_WHOAMI_VALUE }, > + { .compatible = "semtech,sx9311", (void *)SX9311_WHOAMI_VALUE }, > {}, > }; > MODULE_DEVICE_TABLE(of, sx9310_of_match); > @@ -1054,7 +1043,7 @@ static struct i2c_driver sx9310_driver = { > .of_match_table = sx9310_of_match, > .pm = &sx9310_pm_ops, > }, > - .probe = sx9310_probe, > + .probe_new = sx9310_probe, > .id_table = sx9310_id, > }; > module_i2c_driver(sx9310_driver); > -- > 2.28.0.rc0.142.g3c755180ce-goog >
On Tue, Jul 28, 2020 at 6:15 PM Daniel Campello <campello@chromium.org> wrote: > > Use __aligned(8) to ensure that the timestamp is correctly aligned > when we call push_to_buffers > > Signed-off-by: Daniel Campello <campello@chromium.org> > --- > > drivers/iio/proximity/sx9310.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/drivers/iio/proximity/sx9310.c b/drivers/iio/proximity/sx9310.c > index de52afd7c13333..fb5c16f2aa6b1a 100644 > --- a/drivers/iio/proximity/sx9310.c > +++ b/drivers/iio/proximity/sx9310.c > @@ -131,8 +131,8 @@ struct sx9310_data { > */ > bool prox_stat[SX9310_NUM_CHANNELS]; > bool trigger_enabled; > - __be16 buffer[SX9310_NUM_CHANNELS + > - 4]; /* 64-bit data + 64-bit timestamp */ > + /* 64-bit data + 64-bit timestamp buffer */ > + __be16 buffer[SX9310_NUM_CHANNELS + 4] __aligned(8); If the data amount (channels) is always the same, please, use struct approach. Otherwise put a comment explaining dynamic data.
On Tue, Jul 28, 2020 at 6:14 PM Daniel Campello <campello@chromium.org> wrote: > > Uses for_each_set_bit() macro to loop over channel bitmaps. > Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com> > Signed-off-by: Daniel Campello <campello@chromium.org> > --- > > drivers/iio/proximity/sx9310.c | 24 +++++++++++++----------- > 1 file changed, 13 insertions(+), 11 deletions(-) > > diff --git a/drivers/iio/proximity/sx9310.c b/drivers/iio/proximity/sx9310.c > index fb5c16f2aa6b1a..2465064971d0a7 100644 > --- a/drivers/iio/proximity/sx9310.c > +++ b/drivers/iio/proximity/sx9310.c > @@ -136,7 +136,8 @@ struct sx9310_data { > /* Remember enabled channels and sample rate during suspend. */ > unsigned int suspend_ctrl0; > struct completion completion; > - unsigned int chan_read, chan_event; > + unsigned long chan_read; > + unsigned long chan_event; > int channel_users[SX9310_NUM_CHANNELS]; > unsigned int whoami; > }; > @@ -279,15 +280,16 @@ static const struct regmap_config sx9310_regmap_config = { > }; > > static int sx9310_update_chan_en(struct sx9310_data *data, > - unsigned int chan_read, > - unsigned int chan_event) > + unsigned long chan_read, > + unsigned long chan_event) > { > int ret; > + unsigned long channels = chan_read | chan_event; > > - if ((data->chan_read | data->chan_event) != (chan_read | chan_event)) { > + if ((data->chan_read | data->chan_event) != channels) { > ret = regmap_update_bits(data->regmap, SX9310_REG_PROX_CTRL0, > SX9310_REG_PROX_CTRL0_SENSOREN_MASK, > - chan_read | chan_event); > + channels); > if (ret) > return ret; > } > @@ -538,13 +540,13 @@ static void sx9310_push_events(struct iio_dev *indio_dev) > return; > } > > - for (chan = 0; chan < SX9310_NUM_CHANNELS; chan++) { > + for_each_set_bit(chan, &data->chan_event, SX9310_NUM_CHANNELS) { > int dir; > u64 ev; > - bool new_prox = val & BIT(chan); > + bool new_prox; > + > + new_prox = val & BIT(chan); > > - if (!(data->chan_event & BIT(chan))) > - continue; > if (new_prox == data->prox_stat[chan]) > /* No change on this channel. */ > continue; > @@ -712,13 +714,13 @@ static irqreturn_t sx9310_trigger_handler(int irq, void *private) > static int sx9310_buffer_preenable(struct iio_dev *indio_dev) > { > struct sx9310_data *data = iio_priv(indio_dev); > - unsigned int channels = 0; > + unsigned long channels = 0; > int bit, ret; > > mutex_lock(&data->mutex); > for_each_set_bit(bit, indio_dev->active_scan_mask, > indio_dev->masklength) > - channels |= BIT(indio_dev->channels[bit].channel); > + __set_bit(indio_dev->channels[bit].channel, &channels); > > ret = sx9310_update_chan_en(data, channels, data->chan_event); > mutex_unlock(&data->mutex); > -- > 2.28.0.rc0.142.g3c755180ce-goog >
On Tue, Jul 28, 2020 at 6:15 PM Daniel Campello <campello@chromium.org> wrote: > > Simplify compensation stage by using regmap_read_poll_timeout(). Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com> > Signed-off-by: Daniel Campello <campello@chromium.org> > --- > > drivers/iio/proximity/sx9310.c | 25 ++++++++++--------------- > 1 file changed, 10 insertions(+), 15 deletions(-) > > diff --git a/drivers/iio/proximity/sx9310.c b/drivers/iio/proximity/sx9310.c > index 2465064971d0a7..3956fd679c6db9 100644 > --- a/drivers/iio/proximity/sx9310.c > +++ b/drivers/iio/proximity/sx9310.c > @@ -796,7 +796,7 @@ static const struct sx9310_reg_default sx9310_default_regs[] = { > static int sx9310_init_compensation(struct iio_dev *indio_dev) > { > struct sx9310_data *data = iio_priv(indio_dev); > - int i, ret; > + int ret; > unsigned int val; > unsigned int ctrl0; > > @@ -810,22 +810,17 @@ static int sx9310_init_compensation(struct iio_dev *indio_dev) > if (ret < 0) > return ret; > > - for (i = 100; i >= 0; i--) { > - msleep(20); > - ret = regmap_read(data->regmap, SX9310_REG_STAT1, &val); > - if (ret < 0) > - goto out; > - if (!(val & SX9310_COMPSTAT_MASK)) > - break; > - } > - > - if (i < 0) { > - dev_err(&data->client->dev, > - "initial compensation timed out: 0x%02x", val); > - ret = -ETIMEDOUT; > + ret = regmap_read_poll_timeout(data->regmap, SX9310_REG_STAT1, val, > + !(val & SX9310_REG_STAT1_COMPSTAT_MASK), > + 20000, 2000000); > + if (ret) { > + if (ret == -ETIMEDOUT) > + dev_err(&data->client->dev, > + "0x02 << 3l compensation timed out: 0x%02x", > + val); > + return ret; > } > > -out: > regmap_write(data->regmap, SX9310_REG_PROX_CTRL0, ctrl0); > return ret; > } > -- > 2.28.0.rc0.142.g3c755180ce-goog >
On Tue, Jul 28, 2020 at 6:14 PM Daniel Campello <campello@chromium.org> wrote: > > Checks for non-zero return values to signal error conditions. Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com> > Signed-off-by: Daniel Campello <campello@chromium.org> > --- > > drivers/iio/proximity/sx9310.c | 52 +++++++++++++++++----------------- > 1 file changed, 26 insertions(+), 26 deletions(-) > > diff --git a/drivers/iio/proximity/sx9310.c b/drivers/iio/proximity/sx9310.c > index 31234691a31abf..051c6515e62c18 100644 > --- a/drivers/iio/proximity/sx9310.c > +++ b/drivers/iio/proximity/sx9310.c > @@ -338,7 +338,7 @@ static int sx9310_read_prox_data(struct sx9310_data *data, > int ret; > > ret = regmap_write(data->regmap, SX9310_REG_SENSOR_SEL, chan->channel); > - if (ret < 0) > + if (ret) > return ret; > > return regmap_bulk_read(data->regmap, chan->address, val, sizeof(*val)); > @@ -354,7 +354,7 @@ static int sx9310_wait_for_sample(struct sx9310_data *data) > unsigned int val; > > ret = regmap_read(data->regmap, SX9310_REG_PROX_CTRL0, &val); > - if (ret < 0) > + if (ret) > return ret; > > val = (val & SX9310_REG_PROX_CTRL0_SCANPERIOD_MASK) >> > @@ -368,13 +368,13 @@ static int sx9310_wait_for_sample(struct sx9310_data *data) > static int sx9310_read_proximity(struct sx9310_data *data, > const struct iio_chan_spec *chan, int *val) > { > - int ret = 0; > + int ret; > __be16 rawval; > > mutex_lock(&data->mutex); > > ret = sx9310_get_read_channel(data, chan->channel); > - if (ret < 0) > + if (ret) > goto out; > > if (data->client->irq) { > @@ -394,11 +394,11 @@ static int sx9310_read_proximity(struct sx9310_data *data, > > mutex_lock(&data->mutex); > > - if (ret < 0) > + if (ret) > goto out_disable_irq; > > ret = sx9310_read_prox_data(data, chan, &rawval); > - if (ret < 0) > + if (ret) > goto out_disable_irq; > > *val = sign_extend32(be16_to_cpu(rawval), > @@ -411,7 +411,7 @@ static int sx9310_read_proximity(struct sx9310_data *data, > } > > ret = sx9310_put_read_channel(data, chan->channel); > - if (ret < 0) > + if (ret) > goto out; > > mutex_unlock(&data->mutex); > @@ -434,7 +434,7 @@ 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); > > - if (ret < 0) > + if (ret) > return ret; > > regval = (regval & SX9310_REG_PROX_CTRL0_SCANPERIOD_MASK) >> > @@ -535,7 +535,7 @@ static void sx9310_push_events(struct iio_dev *indio_dev) > > /* Read proximity state on all channels */ > ret = regmap_read(data->regmap, SX9310_REG_STAT0, &val); > - if (ret < 0) { > + if (ret) { > dev_err(&data->client->dev, "i2c transfer error in irq\n"); > return; > } > @@ -570,7 +570,7 @@ static irqreturn_t sx9310_irq_thread_handler(int irq, void *private) > mutex_lock(&data->mutex); > > ret = regmap_read(data->regmap, SX9310_REG_IRQ_SRC, &val); > - if (ret < 0) { > + if (ret) { > dev_err(&data->client->dev, "i2c transfer error in irq\n"); > goto out; > } > @@ -613,20 +613,20 @@ static int sx9310_write_event_config(struct iio_dev *indio_dev, > mutex_lock(&data->mutex); > if (state) { > ret = sx9310_get_event_channel(data, chan->channel); > - if (ret < 0) > + if (ret) > goto out_unlock; > if (!(data->chan_event & ~BIT(chan->channel))) { > ret = sx9310_enable_irq(data, eventirq); > - if (ret < 0) > + if (ret) > sx9310_put_event_channel(data, chan->channel); > } > } else { > ret = sx9310_put_event_channel(data, chan->channel); > - if (ret < 0) > + if (ret) > goto out_unlock; > if (!data->chan_event) { > ret = sx9310_disable_irq(data, eventirq); > - if (ret < 0) > + if (ret) > sx9310_get_event_channel(data, chan->channel); > } > } > @@ -665,7 +665,7 @@ static int sx9310_set_trigger_state(struct iio_trigger *trig, bool state) > ret = sx9310_enable_irq(data, SX9310_CONVDONE_IRQ); > else if (!data->chan_read) > ret = sx9310_disable_irq(data, SX9310_CONVDONE_IRQ); > - if (ret < 0) > + if (ret) > goto out; > > data->trigger_enabled = state; > @@ -694,7 +694,7 @@ static irqreturn_t sx9310_trigger_handler(int irq, void *private) > indio_dev->masklength) { > ret = sx9310_read_prox_data(data, &indio_dev->channels[bit], > &val); > - if (ret < 0) > + if (ret) > goto out; > > data->buffer[i++] = val; > @@ -801,13 +801,13 @@ static int sx9310_init_compensation(struct iio_dev *indio_dev) > unsigned int ctrl0; > > ret = regmap_read(data->regmap, SX9310_REG_PROX_CTRL0, &ctrl0); > - if (ret < 0) > + if (ret) > return ret; > > /* run the compensation phase on all channels */ > ret = regmap_write(data->regmap, SX9310_REG_PROX_CTRL0, > ctrl0 | SX9310_REG_PROX_CTRL0_SENSOREN_MASK); > - if (ret < 0) > + if (ret) > return ret; > > ret = regmap_read_poll_timeout(data->regmap, SX9310_REG_STAT1, val, > @@ -833,21 +833,21 @@ static int sx9310_init_device(struct iio_dev *indio_dev) > unsigned int i, val; > > ret = regmap_write(data->regmap, SX9310_REG_RESET, SX9310_SOFT_RESET); > - if (ret < 0) > + if (ret) > return ret; > > usleep_range(1000, 2000); /* power-up time is ~1ms. */ > > /* Clear reset interrupt state by reading SX9310_REG_IRQ_SRC. */ > ret = regmap_read(data->regmap, SX9310_REG_IRQ_SRC, &val); > - if (ret < 0) > + if (ret) > return ret; > > /* Program some sane defaults. */ > for (i = 0; i < ARRAY_SIZE(sx9310_default_regs); i++) { > initval = &sx9310_default_regs[i]; > ret = regmap_write(data->regmap, initval->reg, initval->def); > - if (ret < 0) > + if (ret) > return ret; > } > > @@ -901,14 +901,14 @@ static int sx9310_probe(struct i2c_client *client) > return PTR_ERR(data->regmap); > > ret = regmap_read(data->regmap, SX9310_REG_WHOAMI, &data->whoami); > - if (ret < 0) { > + if (ret) { > dev_err(&client->dev, "error in reading WHOAMI register: %d", > ret); > return ret; > } > > ret = sx9310_set_indio_dev_name(&client->dev, indio_dev, data->whoami); > - if (ret < 0) > + if (ret) > return ret; > > ACPI_COMPANION_SET(&indio_dev->dev, ACPI_COMPANION(&client->dev)); > @@ -920,7 +920,7 @@ static int sx9310_probe(struct i2c_client *client) > i2c_set_clientdata(client, indio_dev); > > ret = sx9310_init_device(indio_dev); > - if (ret < 0) > + if (ret) > return ret; > > if (client->irq) { > @@ -929,7 +929,7 @@ static int sx9310_probe(struct i2c_client *client) > sx9310_irq_thread_handler, > IRQF_TRIGGER_LOW | IRQF_ONESHOT, > "sx9310_event", indio_dev); > - if (ret < 0) > + if (ret) > return ret; > > data->trig = > @@ -951,7 +951,7 @@ static int sx9310_probe(struct i2c_client *client) > iio_pollfunc_store_time, > sx9310_trigger_handler, > &sx9310_buffer_setup_ops); > - if (ret < 0) > + if (ret) > return ret; > > return devm_iio_device_register(&client->dev, indio_dev); > -- > 2.28.0.rc0.142.g3c755180ce-goog >
On Tue, Jul 28, 2020 at 6:14 PM Daniel Campello <campello@chromium.org> wrote: > > Improves readability by storing &client->dev in a local variable. Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com> > Signed-off-by: Daniel Campello <campello@chromium.org> > --- > > drivers/iio/proximity/sx9310.c | 30 +++++++++++++++--------------- > 1 file changed, 15 insertions(+), 15 deletions(-) > > diff --git a/drivers/iio/proximity/sx9310.c b/drivers/iio/proximity/sx9310.c > index 051c6515e62c18..ba383710ef0dcf 100644 > --- a/drivers/iio/proximity/sx9310.c > +++ b/drivers/iio/proximity/sx9310.c > @@ -884,11 +884,12 @@ static int sx9310_set_indio_dev_name(struct device *dev, > static int sx9310_probe(struct i2c_client *client) > { > int ret; > + struct device *dev = &client->dev; > struct iio_dev *indio_dev; > struct sx9310_data *data; > > - indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*data)); > - if (indio_dev == NULL) > + indio_dev = devm_iio_device_alloc(dev, sizeof(*data)); > + if (!indio_dev) > return -ENOMEM; > > data = iio_priv(indio_dev); > @@ -902,17 +903,16 @@ static int sx9310_probe(struct i2c_client *client) > > ret = regmap_read(data->regmap, SX9310_REG_WHOAMI, &data->whoami); > if (ret) { > - dev_err(&client->dev, "error in reading WHOAMI register: %d", > - ret); > + dev_err(dev, "error in reading WHOAMI register: %d", ret); > return ret; > } > > - ret = sx9310_set_indio_dev_name(&client->dev, indio_dev, data->whoami); > + ret = sx9310_set_indio_dev_name(dev, indio_dev, data->whoami); > if (ret) > return ret; > > - ACPI_COMPANION_SET(&indio_dev->dev, ACPI_COMPANION(&client->dev)); > - indio_dev->dev.parent = &client->dev; > + ACPI_COMPANION_SET(&indio_dev->dev, ACPI_COMPANION(dev)); > + indio_dev->dev.parent = dev; > indio_dev->channels = sx9310_channels; > indio_dev->num_channels = ARRAY_SIZE(sx9310_channels); > indio_dev->info = &sx9310_info; > @@ -924,7 +924,7 @@ static int sx9310_probe(struct i2c_client *client) > return ret; > > if (client->irq) { > - ret = devm_request_threaded_irq(&client->dev, client->irq, > + ret = devm_request_threaded_irq(dev, client->irq, > sx9310_irq_handler, > sx9310_irq_thread_handler, > IRQF_TRIGGER_LOW | IRQF_ONESHOT, > @@ -932,29 +932,29 @@ static int sx9310_probe(struct i2c_client *client) > if (ret) > return ret; > > - data->trig = > - devm_iio_trigger_alloc(&client->dev, "%s-dev%d", > - indio_dev->name, indio_dev->id); > + data->trig = devm_iio_trigger_alloc(dev, "%s-dev%d", > + indio_dev->name, > + indio_dev->id); > if (!data->trig) > return -ENOMEM; > > - data->trig->dev.parent = &client->dev; > + data->trig->dev.parent = dev; > data->trig->ops = &sx9310_trigger_ops; > iio_trigger_set_drvdata(data->trig, indio_dev); > > - ret = devm_iio_trigger_register(&client->dev, data->trig); > + ret = devm_iio_trigger_register(dev, data->trig); > if (ret) > return ret; > } > > - ret = devm_iio_triggered_buffer_setup(&client->dev, indio_dev, > + ret = devm_iio_triggered_buffer_setup(dev, indio_dev, > iio_pollfunc_store_time, > sx9310_trigger_handler, > &sx9310_buffer_setup_ops); > if (ret) > return ret; > > - return devm_iio_device_register(&client->dev, indio_dev); > + return devm_iio_device_register(dev, indio_dev); > } > > static int __maybe_unused sx9310_suspend(struct device *dev) > -- > 2.28.0.rc0.142.g3c755180ce-goog >
On Tue, Jul 28, 2020 at 6:14 PM Daniel Campello <campello@chromium.org> wrote: > > Miscellaneous format fixes throughout the whole file. Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com> > Signed-off-by: Daniel Campello <campello@chromium.org> > --- > > drivers/iio/proximity/sx9310.c | 28 ++++++++++------------------ > 1 file changed, 10 insertions(+), 18 deletions(-) > > diff --git a/drivers/iio/proximity/sx9310.c b/drivers/iio/proximity/sx9310.c > index ba383710ef0dcf..3f981d28ee4056 100644 > --- a/drivers/iio/proximity/sx9310.c > +++ b/drivers/iio/proximity/sx9310.c > @@ -90,28 +90,21 @@ > #define SX9310_REG_SAR_CTRL2_SAROFFSET_DEFAULT 0x3c > > #define SX9310_REG_SENSOR_SEL 0x30 > - > #define SX9310_REG_USE_MSB 0x31 > #define SX9310_REG_USE_LSB 0x32 > - > #define SX9310_REG_AVG_MSB 0x33 > #define SX9310_REG_AVG_LSB 0x34 > - > #define SX9310_REG_DIFF_MSB 0x35 > #define SX9310_REG_DIFF_LSB 0x36 > - > #define SX9310_REG_OFFSET_MSB 0x37 > #define SX9310_REG_OFFSET_LSB 0x38 > - > #define SX9310_REG_SAR_MSB 0x39 > #define SX9310_REG_SAR_LSB 0x3a > - > #define SX9310_REG_I2C_ADDR 0x40 > #define SX9310_REG_PAUSE 0x41 > #define SX9310_REG_WHOAMI 0x42 > #define SX9310_WHOAMI_VALUE 0x01 > #define SX9311_WHOAMI_VALUE 0x02 > - > #define SX9310_REG_RESET 0x7f > #define SX9310_SOFT_RESET 0xde > > @@ -402,7 +395,7 @@ static int sx9310_read_proximity(struct sx9310_data *data, > goto out_disable_irq; > > *val = sign_extend32(be16_to_cpu(rawval), > - (chan->address == SX9310_REG_DIFF_MSB ? 11 : 15)); > + chan->address == SX9310_REG_DIFF_MSB ? 11 : 15); > > if (data->client->irq) { > ret = sx9310_disable_irq(data, SX9310_CONVDONE_IRQ); > @@ -432,8 +425,9 @@ static int sx9310_read_proximity(struct sx9310_data *data, > 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); > + int ret; > > + ret = regmap_read(data->regmap, SX9310_REG_PROX_CTRL0, ®val); > if (ret) > return ret; > > @@ -518,10 +512,9 @@ static irqreturn_t sx9310_irq_handler(int irq, void *private) > iio_trigger_poll(data->trig); > > /* > - * Even if no event is enabled, we need to wake the thread to > - * clear the interrupt state by reading SX9310_REG_IRQ_SRC. It > - * is not possible to do that here because regmap_read takes a > - * mutex. > + * Even if no event is enabled, we need to wake the thread to clear the > + * interrupt state by reading SX9310_REG_IRQ_SRC. > + * It is not possible to do that here because regmap_read takes a mutex. > */ > return IRQ_WAKE_THREAD; > } > @@ -638,7 +631,7 @@ static int sx9310_write_event_config(struct iio_dev *indio_dev, > > static struct attribute *sx9310_attributes[] = { > &iio_dev_attr_sampling_frequency_available.dev_attr.attr, > - NULL, > + NULL > }; > > static const struct attribute_group sx9310_attribute_group = { > @@ -969,7 +962,6 @@ static int __maybe_unused sx9310_suspend(struct device *dev) > mutex_lock(&data->mutex); > ret = regmap_read(data->regmap, SX9310_REG_PROX_CTRL0, > &data->suspend_ctrl0); > - > if (ret) > goto out; > > @@ -1015,21 +1007,21 @@ static const struct dev_pm_ops sx9310_pm_ops = { > static const struct acpi_device_id sx9310_acpi_match[] = { > { "STH9310", SX9310_WHOAMI_VALUE }, > { "STH9311", SX9311_WHOAMI_VALUE }, > - {}, > + {} > }; > MODULE_DEVICE_TABLE(acpi, sx9310_acpi_match); > > static const struct of_device_id sx9310_of_match[] = { > { .compatible = "semtech,sx9310", (void *)SX9310_WHOAMI_VALUE }, > { .compatible = "semtech,sx9311", (void *)SX9311_WHOAMI_VALUE }, > - {}, > + {} > }; > MODULE_DEVICE_TABLE(of, sx9310_of_match); > > static const struct i2c_device_id sx9310_id[] = { > { "sx9310", SX9310_WHOAMI_VALUE }, > { "sx9311", SX9311_WHOAMI_VALUE }, > - {}, > + {} > }; > MODULE_DEVICE_TABLE(i2c, sx9310_id); > > -- > 2.28.0.rc0.142.g3c755180ce-goog >
On Tue, Jul 28, 2020 at 6:14 PM Daniel Campello <campello@chromium.org> wrote: > > From: Stephen Boyd <swboyd@chromium.org> > > Printks in the kernel have newlines at the end. Add them to the few Printk()s > printks in this driver. printk()s > Reviewed-by: Daniel Campello <campello@chromium.org> > Reviewed-by: Douglas Anderson <dianders@chromium.org> > Fixes: 72ad02b15d63 ("iio: Add SEMTECH SX9310/9311 sensor driver") > Signed-off-by: Stephen Boyd <swboyd@chromium.org> > Signed-off-by: Daniel Campello <campello@chromium.org> It has ordering issues Should be Fixes: SoB: Stephen Rb: Douglas Rb: Daniel SoB: Daniel > --- > > drivers/iio/proximity/sx9310.c | 8 ++++---- > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/drivers/iio/proximity/sx9310.c b/drivers/iio/proximity/sx9310.c > index 3f981d28ee4056..4553ee83a016a3 100644 > --- a/drivers/iio/proximity/sx9310.c > +++ b/drivers/iio/proximity/sx9310.c > @@ -809,7 +809,7 @@ static int sx9310_init_compensation(struct iio_dev *indio_dev) > if (ret) { > if (ret == -ETIMEDOUT) > dev_err(&data->client->dev, > - "0x02 << 3l compensation timed out: 0x%02x", > + "0x02 << 3l compensation timed out: 0x%02x\n", Looks like ping-pong style in the series, i.e. you may fix this when you introduced this line. Check the rest (and not only printk()s) for the similar style and avoid the latter. > val); > return ret; > } > @@ -855,7 +855,7 @@ static int sx9310_set_indio_dev_name(struct device *dev, > > ddata = (uintptr_t)device_get_match_data(dev); > if (ddata != whoami) { > - dev_err(dev, "WHOAMI does not match device data: %u", whoami); > + dev_err(dev, "WHOAMI does not match device data: %u\n", whoami); > return -ENODEV; > } > > @@ -867,7 +867,7 @@ static int sx9310_set_indio_dev_name(struct device *dev, > indio_dev->name = "sx9311"; > break; > default: > - dev_err(dev, "unexpected WHOAMI response: %u", whoami); > + dev_err(dev, "unexpected WHOAMI response: %u\n", whoami); > return -ENODEV; > } > > @@ -896,7 +896,7 @@ static int sx9310_probe(struct i2c_client *client) > > ret = regmap_read(data->regmap, SX9310_REG_WHOAMI, &data->whoami); > if (ret) { > - dev_err(dev, "error in reading WHOAMI register: %d", ret); > + dev_err(dev, "error in reading WHOAMI register: %d\n", ret); > return ret; > } > > -- > 2.28.0.rc0.142.g3c755180ce-goog >
On Tue, Jul 28, 2020 at 6:14 PM Daniel Campello <campello@chromium.org> wrote: > > From: Stephen Boyd <swboyd@chromium.org> > > This struct member isn't used. Drop it. > > Fixes: 72ad02b15d63 ("iio: Add SEMTECH SX9310/9311 sensor driver") > Signed-off-by: Stephen Boyd <swboyd@chromium.org> > Reviewed-by: Douglas Anderson <dianders@chromium.org> > Reviewed-by: Daniel Campello <campello@chromium.org> > Signed-off-by: Daniel Campello <campello@chromium.org> Here is everything correct! Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com> > --- > > drivers/iio/proximity/sx9310.c | 1 - > 1 file changed, 1 deletion(-) > > diff --git a/drivers/iio/proximity/sx9310.c b/drivers/iio/proximity/sx9310.c > index 4553ee83a016a3..202018b726e32f 100644 > --- a/drivers/iio/proximity/sx9310.c > +++ b/drivers/iio/proximity/sx9310.c > @@ -131,7 +131,6 @@ struct sx9310_data { > struct completion completion; > unsigned long chan_read; > unsigned long chan_event; > - int channel_users[SX9310_NUM_CHANNELS]; > unsigned int whoami; > }; > > -- > 2.28.0.rc0.142.g3c755180ce-goog >
On Tue, Jul 28, 2020 at 6:14 PM Daniel Campello <campello@chromium.org> wrote: > > From: Stephen Boyd <swboyd@chromium.org> > > Enable the main power supply (vdd) and digital IO power supply (svdd) > during probe so that the i2c communication and device works properly on > boards that aggressively power gate these supplies. > > Reviewed-by: Douglas Anderson <dianders@chromium.org> > Signed-off-by: Stephen Boyd <swboyd@chromium.org> > Signed-off-by: Daniel Campello <campello@chromium.org> Again wrong order. After fixing, Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com> > --- > > drivers/iio/proximity/sx9310.c | 26 ++++++++++++++++++++++++++ > 1 file changed, 26 insertions(+) > > diff --git a/drivers/iio/proximity/sx9310.c b/drivers/iio/proximity/sx9310.c > index 202018b726e32f..24a2360b6314ef 100644 > --- a/drivers/iio/proximity/sx9310.c > +++ b/drivers/iio/proximity/sx9310.c > @@ -18,6 +18,7 @@ > #include <linux/module.h> > #include <linux/pm.h> > #include <linux/regmap.h> > +#include <linux/regulator/consumer.h> > #include <linux/slab.h> > > #include <linux/iio/buffer.h> > @@ -118,6 +119,7 @@ struct sx9310_data { > struct i2c_client *client; > struct iio_trigger *trig; > struct regmap *regmap; > + struct regulator_bulk_data supplies[2]; > /* > * Last reading of the proximity status for each channel. > * We only send an event to user space when this changes. > @@ -873,6 +875,13 @@ static int sx9310_set_indio_dev_name(struct device *dev, > return 0; > } > > +static void sx9310_regulator_disable(void *_data) > +{ > + struct sx9310_data *data = _data; > + > + regulator_bulk_disable(ARRAY_SIZE(data->supplies), data->supplies); > +} > + > static int sx9310_probe(struct i2c_client *client) > { > int ret; > @@ -886,6 +895,8 @@ static int sx9310_probe(struct i2c_client *client) > > data = iio_priv(indio_dev); > data->client = client; > + data->supplies[0].supply = "vdd"; > + data->supplies[1].supply = "svdd"; > mutex_init(&data->mutex); > init_completion(&data->completion); > > @@ -893,6 +904,21 @@ static int sx9310_probe(struct i2c_client *client) > if (IS_ERR(data->regmap)) > return PTR_ERR(data->regmap); > > + ret = devm_regulator_bulk_get(dev, ARRAY_SIZE(data->supplies), > + data->supplies); > + if (ret) > + return ret; > + > + ret = regulator_bulk_enable(ARRAY_SIZE(data->supplies), data->supplies); > + if (ret) > + return ret; > + /* Must wait for Tpor time after initial power up */ > + usleep_range(1000, 1100); > + > + ret = devm_add_action_or_reset(dev, sx9310_regulator_disable, data); > + if (ret) > + return ret; > + > ret = regmap_read(data->regmap, SX9310_REG_WHOAMI, &data->whoami); > if (ret) { > dev_err(dev, "error in reading WHOAMI register: %d\n", ret); > -- > 2.28.0.rc0.142.g3c755180ce-goog >
On Tue, 2020-07-28 at 21:19 +0300, Andy Shevchenko wrote: > On Tue, Jul 28, 2020 at 6:14 PM Daniel Campello <campello@chromium.org> wrote: > > From: Stephen Boyd <swboyd@chromium.org> > > > > Printks in the kernel have newlines at the end. Add them to the few > > Printk()s > > > printks in this driver. > > printk()s Random kernel pedantry. This patch should not need to be respun for any of these. > > Reviewed-by: Daniel Campello <campello@chromium.org> > > Reviewed-by: Douglas Anderson <dianders@chromium.org> > > Fixes: 72ad02b15d63 ("iio: Add SEMTECH SX9310/9311 sensor driver") > > Signed-off-by: Stephen Boyd <swboyd@chromium.org> > > Signed-off-by: Daniel Campello <campello@chromium.org> > > It has ordering issues > Should be > > Fixes: > SoB: Stephen > Rb: Douglas > Rb: Daniel > SoB: Daniel > > > > --- > > > > drivers/iio/proximity/sx9310.c | 8 ++++---- > > 1 file changed, 4 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/iio/proximity/sx9310.c b/drivers/iio/proximity/sx9310.c > > index 3f981d28ee4056..4553ee83a016a3 100644 > > --- a/drivers/iio/proximity/sx9310.c > > +++ b/drivers/iio/proximity/sx9310.c > > @@ -809,7 +809,7 @@ static int sx9310_init_compensation(struct iio_dev *indio_dev) > > if (ret) { > > if (ret == -ETIMEDOUT) > > dev_err(&data->client->dev, > > - "0x02 << 3l compensation timed out: 0x%02x", > > + "0x02 << 3l compensation timed out: 0x%02x\n", > > Looks like ping-pong style in the series, i.e. you may fix this when > you introduced this line. > > Check the rest (and not only printk()s) for the similar style and > avoid the latter. > > > val); > > return ret; > > } > > @@ -855,7 +855,7 @@ static int sx9310_set_indio_dev_name(struct device *dev, > > > > ddata = (uintptr_t)device_get_match_data(dev); > > if (ddata != whoami) { > > - dev_err(dev, "WHOAMI does not match device data: %u", whoami); > > + dev_err(dev, "WHOAMI does not match device data: %u\n", whoami); > > return -ENODEV; > > } > > > > @@ -867,7 +867,7 @@ static int sx9310_set_indio_dev_name(struct device *dev, > > indio_dev->name = "sx9311"; > > break; > > default: > > - dev_err(dev, "unexpected WHOAMI response: %u", whoami); > > + dev_err(dev, "unexpected WHOAMI response: %u\n", whoami); > > return -ENODEV; > > } > > > > @@ -896,7 +896,7 @@ static int sx9310_probe(struct i2c_client *client) > > > > ret = regmap_read(data->regmap, SX9310_REG_WHOAMI, &data->whoami); > > if (ret) { > > - dev_err(dev, "error in reading WHOAMI register: %d", ret); > > + dev_err(dev, "error in reading WHOAMI register: %d\n", ret); > > return ret; > > } > > > > -- > > 2.28.0.rc0.142.g3c755180ce-goog > > > >
On Tue, Jul 28, 2020 at 9:24 PM Joe Perches <joe@perches.com> wrote: > On Tue, 2020-07-28 at 21:19 +0300, Andy Shevchenko wrote: > > On Tue, Jul 28, 2020 at 6:14 PM Daniel Campello <campello@chromium.org> wrote: > > > From: Stephen Boyd <swboyd@chromium.org> > > > > > > Printks in the kernel have newlines at the end. Add them to the few > > > > Printk()s > > > > > printks in this driver. > > > > printk()s > > Random kernel pedantry. > This patch should not need to be respun for any of these. If for above I can agree with you, below is definitely subject to improvement. > > > > Reviewed-by: Daniel Campello <campello@chromium.org> > > > Reviewed-by: Douglas Anderson <dianders@chromium.org> > > > Fixes: 72ad02b15d63 ("iio: Add SEMTECH SX9310/9311 sensor driver") > > > Signed-off-by: Stephen Boyd <swboyd@chromium.org> > > > Signed-off-by: Daniel Campello <campello@chromium.org> > > > > It has ordering issues > > Should be > > > > Fixes: > > SoB: Stephen > > Rb: Douglas > > Rb: Daniel > > SoB: Daniel > > > > > > > --- > > > > > > drivers/iio/proximity/sx9310.c | 8 ++++---- > > > 1 file changed, 4 insertions(+), 4 deletions(-) > > > > > > diff --git a/drivers/iio/proximity/sx9310.c b/drivers/iio/proximity/sx9310.c > > > index 3f981d28ee4056..4553ee83a016a3 100644 > > > --- a/drivers/iio/proximity/sx9310.c > > > +++ b/drivers/iio/proximity/sx9310.c > > > @@ -809,7 +809,7 @@ static int sx9310_init_compensation(struct iio_dev *indio_dev) > > > if (ret) { > > > if (ret == -ETIMEDOUT) > > > dev_err(&data->client->dev, > > > - "0x02 << 3l compensation timed out: 0x%02x", > > > + "0x02 << 3l compensation timed out: 0x%02x\n", > > > > Looks like ping-pong style in the series, i.e. you may fix this when > > you introduced this line. > > > > Check the rest (and not only printk()s) for the similar style and > > avoid the latter. > > > > > val); > > > return ret; > > > } > > > @@ -855,7 +855,7 @@ static int sx9310_set_indio_dev_name(struct device *dev, > > > > > > ddata = (uintptr_t)device_get_match_data(dev); > > > if (ddata != whoami) { > > > - dev_err(dev, "WHOAMI does not match device data: %u", whoami); > > > + dev_err(dev, "WHOAMI does not match device data: %u\n", whoami); > > > return -ENODEV; > > > } > > > > > > @@ -867,7 +867,7 @@ static int sx9310_set_indio_dev_name(struct device *dev, > > > indio_dev->name = "sx9311"; > > > break; > > > default: > > > - dev_err(dev, "unexpected WHOAMI response: %u", whoami); > > > + dev_err(dev, "unexpected WHOAMI response: %u\n", whoami); > > > return -ENODEV; > > > } > > > > > > @@ -896,7 +896,7 @@ static int sx9310_probe(struct i2c_client *client) > > > > > > ret = regmap_read(data->regmap, SX9310_REG_WHOAMI, &data->whoami); > > > if (ret) { > > > - dev_err(dev, "error in reading WHOAMI register: %d", ret); > > > + dev_err(dev, "error in reading WHOAMI register: %d\n", ret); > > > return ret; > > > } > > > > > > -- > > > 2.28.0.rc0.142.g3c755180ce-goog > > > > > > > >
Quoting Daniel Campello (2020-07-28 08:12:48) > Uses .probe_new in place of .probe. Also uses device_get_match_data() > for whoami matching. > > Signed-off-by: Daniel Campello <campello@chromium.org> > --- Reviewed-by: Stephen Boyd <swboyd@chromium.org>
Quoting Daniel Campello (2020-07-28 08:12:50) > diff --git a/drivers/iio/proximity/sx9310.c b/drivers/iio/proximity/sx9310.c > index fb5c16f2aa6b1a..2465064971d0a7 100644 > --- a/drivers/iio/proximity/sx9310.c > +++ b/drivers/iio/proximity/sx9310.c > @@ -538,13 +540,13 @@ static void sx9310_push_events(struct iio_dev *indio_dev) > return; > } > > - for (chan = 0; chan < SX9310_NUM_CHANNELS; chan++) { > + for_each_set_bit(chan, &data->chan_event, SX9310_NUM_CHANNELS) { > int dir; > u64 ev; > - bool new_prox = val & BIT(chan); > + bool new_prox; > + > + new_prox = val & BIT(chan); > > - if (!(data->chan_event & BIT(chan))) > - continue; > if (new_prox == data->prox_stat[chan]) Why not make 'prox_stat' a bitmap too and then xor them to iterate over that bitmap instead? > /* No change on this channel. */ > continue;
Quoting Daniel Campello (2020-07-28 08:12:51) > diff --git a/drivers/iio/proximity/sx9310.c b/drivers/iio/proximity/sx9310.c > index 2465064971d0a7..3956fd679c6db9 100644 > --- a/drivers/iio/proximity/sx9310.c > +++ b/drivers/iio/proximity/sx9310.c > @@ -810,22 +810,17 @@ static int sx9310_init_compensation(struct iio_dev *indio_dev) > if (ret < 0) > return ret; > > - for (i = 100; i >= 0; i--) { > - msleep(20); > - ret = regmap_read(data->regmap, SX9310_REG_STAT1, &val); > - if (ret < 0) > - goto out; > - if (!(val & SX9310_COMPSTAT_MASK)) > - break; > - } > - > - if (i < 0) { > - dev_err(&data->client->dev, > - "initial compensation timed out: 0x%02x", val); > - ret = -ETIMEDOUT; > + ret = regmap_read_poll_timeout(data->regmap, SX9310_REG_STAT1, val, > + !(val & SX9310_REG_STAT1_COMPSTAT_MASK), > + 20000, 2000000); > + if (ret) { > + if (ret == -ETIMEDOUT) > + dev_err(&data->client->dev, > + "0x02 << 3l compensation timed out: 0x%02x", What does 0x02 << 3l mean? > + val); > + return ret; > } >
Quoting Daniel Campello (2020-07-28 08:12:53) > @@ -368,13 +368,13 @@ static int sx9310_wait_for_sample(struct sx9310_data *data) > static int sx9310_read_proximity(struct sx9310_data *data, > const struct iio_chan_spec *chan, int *val) > { > - int ret = 0; > + int ret; > __be16 rawval; > > mutex_lock(&data->mutex); > > ret = sx9310_get_read_channel(data, chan->channel); > - if (ret < 0) > + if (ret) > goto out; > > if (data->client->irq) { > @@ -394,11 +394,11 @@ static int sx9310_read_proximity(struct sx9310_data *data, > > mutex_lock(&data->mutex); > > - if (ret < 0) > + if (ret) > goto out_disable_irq; Why is this condition checked after grabbing the mutex? Shouldn't it be checked before grabbing the mutex? Or is that supposed to be a mutex_unlock()? > > ret = sx9310_read_prox_data(data, chan, &rawval); > - if (ret < 0) > + if (ret) > goto out_disable_irq; > > *val = sign_extend32(be16_to_cpu(rawval),
Quoting Daniel Campello (2020-07-28 08:12:54) > Improves readability by storing &client->dev in a local variable. > > Signed-off-by: Daniel Campello <campello@chromium.org> > --- Reviewed-by: Stephen Boyd <swboyd@chromium.org>
Quoting Daniel Campello (2020-07-28 08:12:55) > Miscellaneous format fixes throughout the whole file. > > Signed-off-by: Daniel Campello <campello@chromium.org> > --- Reviewed-by: Stephen Boyd <swboyd@chromium.org>
Quoting Daniel Campello (2020-07-28 08:12:45) > Follows spec sheet for macro declarations. > > Signed-off-by: Daniel Campello <campello@chromium.org> > --- Reviewed-by: Stephen Boyd <swboyd@chromium.org>
On Tue, Jul 28, 2020 at 12:08 PM Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > > On Tue, Jul 28, 2020 at 6:14 PM Daniel Campello <campello@chromium.org> wrote: > > > > Fixes enable/disable irq handling at various points. The driver needs to > > only enable/disable irqs if there is an actual irq handler installed. > > > - enable_irq(data->client->irq); > > + if (!ret) > > + enable_irq(data->client->irq); > > > > return ret; > > } > > Can it be a usual pattern? > > if (ret) > return ret; > ... > return 0; I think this way is more readable. The alternative would have to be something like this: .... if (ret) goto out; mutex_unlock(&data->mutex); enable_irq(data->client->irq); return 0; out: mutex_unlock(&data->mutex); return ret; > > -- > With Best Regards, > Andy Shevchenko Regards, Daniel Campello
Quoting Daniel Campello (2020-07-28 13:07:00) > On Tue, Jul 28, 2020 at 12:08 PM Andy Shevchenko > <andy.shevchenko@gmail.com> wrote: > > > > On Tue, Jul 28, 2020 at 6:14 PM Daniel Campello <campello@chromium.org> wrote: > > > > > > Fixes enable/disable irq handling at various points. The driver needs to > > > only enable/disable irqs if there is an actual irq handler installed. > > > > > - enable_irq(data->client->irq); > > > + if (!ret) > > > + enable_irq(data->client->irq); > > > > > > return ret; > > > } > > > > Can it be a usual pattern? > > > > if (ret) > > return ret; > > ... > > return 0; > > I think this way is more readable. The alternative would have to be > something like this: > > .... > if (ret) > goto out; > mutex_unlock(&data->mutex); > enable_irq(data->client->irq); > return 0; > > out: > mutex_unlock(&data->mutex); > return ret; > I think the suggestion is mutex_unlock(&data->mutex); if (ret) return ret; enable_irq(data->client->irq); return 0;
On Tue, Jul 28, 2020 at 12:09 PM Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > > On Tue, Jul 28, 2020 at 6:16 PM Daniel Campello <campello@chromium.org> wrote: > > > > Avoids unused warnings due to acpi/of table macros. > > > > At the same time I would check if mod_devicetable.h is included. I did the following and no error showed up: #ifndef LINUX_MOD_DEVICETABLE_H #error Missing include #endif > > > Signed-off-by: Daniel Campello <campello@chromium.org> > > Reported-by: kbuild test robot <lkp@intel.com> > > > -- > With Best Regards, > Andy Shevchenko Regards, Daniel Campello
On Tue, Jul 28, 2020 at 1:40 PM Stephen Boyd <swboyd@chromium.org> wrote: > > Quoting Daniel Campello (2020-07-28 08:12:53) > > @@ -368,13 +368,13 @@ static int sx9310_wait_for_sample(struct sx9310_data *data) > > static int sx9310_read_proximity(struct sx9310_data *data, > > const struct iio_chan_spec *chan, int *val) > > { > > - int ret = 0; > > + int ret; > > __be16 rawval; > > > > mutex_lock(&data->mutex); > > > > ret = sx9310_get_read_channel(data, chan->channel); > > - if (ret < 0) > > + if (ret) > > goto out; > > > > if (data->client->irq) { > > @@ -394,11 +394,11 @@ static int sx9310_read_proximity(struct sx9310_data *data, > > > > mutex_lock(&data->mutex); > > > > - if (ret < 0) > > + if (ret) > > goto out_disable_irq; > > Why is this condition checked after grabbing the mutex? Shouldn't it be > checked before grabbing the mutex? Or is that supposed to be a > mutex_unlock()? We acquire the lock before jumping to out_disable_irq which is before a mutex_unlock() > > > > > ret = sx9310_read_prox_data(data, chan, &rawval); > > - if (ret < 0) > > + if (ret) > > goto out_disable_irq; > > > > *val = sign_extend32(be16_to_cpu(rawval),
On Tue, Jul 28, 2020 at 12:11 PM Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > > On Tue, Jul 28, 2020 at 6:15 PM Daniel Campello <campello@chromium.org> wrote: > > > > Use __aligned(8) to ensure that the timestamp is correctly aligned > > when we call push_to_buffers > > > > Signed-off-by: Daniel Campello <campello@chromium.org> > > --- > > > > drivers/iio/proximity/sx9310.c | 6 +++--- > > 1 file changed, 3 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/iio/proximity/sx9310.c b/drivers/iio/proximity/sx9310.c > > index de52afd7c13333..fb5c16f2aa6b1a 100644 > > --- a/drivers/iio/proximity/sx9310.c > > +++ b/drivers/iio/proximity/sx9310.c > > @@ -131,8 +131,8 @@ struct sx9310_data { > > */ > > bool prox_stat[SX9310_NUM_CHANNELS]; > > bool trigger_enabled; > > - __be16 buffer[SX9310_NUM_CHANNELS + > > - 4]; /* 64-bit data + 64-bit timestamp */ > > + /* 64-bit data + 64-bit timestamp buffer */ > > + __be16 buffer[SX9310_NUM_CHANNELS + 4] __aligned(8); > > If the data amount (channels) is always the same, please, use struct approach. > Otherwise put a comment explaining dynamic data. I'm not sure what you mean here. I have a comment above for the size of the array. > > -- > With Best Regards, > Andy Shevchenko
Quoting Daniel Campello (2020-07-28 13:47:15) > On Tue, Jul 28, 2020 at 12:09 PM Andy Shevchenko > <andy.shevchenko@gmail.com> wrote: > > > > On Tue, Jul 28, 2020 at 6:16 PM Daniel Campello <campello@chromium.org> wrote: > > > > > > Avoids unused warnings due to acpi/of table macros. > > > > > > > At the same time I would check if mod_devicetable.h is included. > I did the following and no error showed up: > #ifndef LINUX_MOD_DEVICETABLE_H > #error Missing include > #endif That's fine, but it's usually better to avoid implicit include dependencies so that order of includes doesn't matter and so that if the headers change outside of this driver this driver doesn't have to be fixed to include something it needs like mod_devicetable.h
Quoting Daniel Campello (2020-07-28 14:23:29) > On Tue, Jul 28, 2020 at 1:40 PM Stephen Boyd <swboyd@chromium.org> wrote: > > > > Quoting Daniel Campello (2020-07-28 08:12:53) > > > @@ -368,13 +368,13 @@ static int sx9310_wait_for_sample(struct sx9310_data *data) > > > static int sx9310_read_proximity(struct sx9310_data *data, > > > const struct iio_chan_spec *chan, int *val) > > > { > > > - int ret = 0; > > > + int ret; > > > __be16 rawval; > > > > > > mutex_lock(&data->mutex); > > > > > > ret = sx9310_get_read_channel(data, chan->channel); > > > - if (ret < 0) > > > + if (ret) > > > goto out; > > > > > > if (data->client->irq) { > > > @@ -394,11 +394,11 @@ static int sx9310_read_proximity(struct sx9310_data *data, > > > > > > mutex_lock(&data->mutex); > > > > > > - if (ret < 0) > > > + if (ret) > > > goto out_disable_irq; > > > > Why is this condition checked after grabbing the mutex? Shouldn't it be > > checked before grabbing the mutex? Or is that supposed to be a > > mutex_unlock()? > We acquire the lock before jumping to out_disable_irq which is before > a mutex_unlock() Does this function need to hold the mutex lock around get/put_read_channel? It drops the lock while waiting and then regrabs it which seems to imply that another reader could come in and try to get the channel again during the wait. So put another way, it may be simpler to shorten the lock area and then bail out of this function to a place where the lock isn't held already on the return path.
On Tue, Jul 28, 2020 at 3:32 PM Stephen Boyd <swboyd@chromium.org> wrote: > > Quoting Daniel Campello (2020-07-28 14:23:29) > > On Tue, Jul 28, 2020 at 1:40 PM Stephen Boyd <swboyd@chromium.org> wrote: > > > > > > Quoting Daniel Campello (2020-07-28 08:12:53) > > > > @@ -368,13 +368,13 @@ static int sx9310_wait_for_sample(struct sx9310_data *data) > > > > static int sx9310_read_proximity(struct sx9310_data *data, > > > > const struct iio_chan_spec *chan, int *val) > > > > { > > > > - int ret = 0; > > > > + int ret; > > > > __be16 rawval; > > > > > > > > mutex_lock(&data->mutex); > > > > > > > > ret = sx9310_get_read_channel(data, chan->channel); > > > > - if (ret < 0) > > > > + if (ret) > > > > goto out; > > > > > > > > if (data->client->irq) { > > > > @@ -394,11 +394,11 @@ static int sx9310_read_proximity(struct sx9310_data *data, > > > > > > > > mutex_lock(&data->mutex); > > > > > > > > - if (ret < 0) > > > > + if (ret) > > > > goto out_disable_irq; > > > > > > Why is this condition checked after grabbing the mutex? Shouldn't it be > > > checked before grabbing the mutex? Or is that supposed to be a > > > mutex_unlock()? > > We acquire the lock before jumping to out_disable_irq which is before > > a mutex_unlock() > > Does this function need to hold the mutex lock around get/put_read_channel? Yes, both get/put_read_channel and get/put_event_channel use sx9310_update_chan_en which is updating data->chan_{read,event} bitmaps. > It drops the lock while waiting and then regrabs it which seems to > imply that another reader could come in and try to get the channel again > during the wait. So put another way, it may be simpler to shorten the > lock area and then bail out of this function to a place where the lock > isn't held already on the return path.
On Wed, Jul 29, 2020 at 12:26 AM Daniel Campello <campello@google.com> wrote: > On Tue, Jul 28, 2020 at 12:11 PM Andy Shevchenko > <andy.shevchenko@gmail.com> wrote: > > On Tue, Jul 28, 2020 at 6:15 PM Daniel Campello <campello@chromium.org> wrote: ... > > > - __be16 buffer[SX9310_NUM_CHANNELS + > > > - 4]; /* 64-bit data + 64-bit timestamp */ > > > + /* 64-bit data + 64-bit timestamp buffer */ > > > + __be16 buffer[SX9310_NUM_CHANNELS + 4] __aligned(8); > > > > If the data amount (channels) is always the same, please, use struct approach. > > Otherwise put a comment explaining dynamic data. > I'm not sure what you mean here. I have a comment above for the size > of the array. Here [1] was a discussion about commenting on the dynamic amount of data [see the cover letter and replies to it] in the buffer and the struct approach [e.g. very first patch in the series]. [1]: https://lore.kernel.org/linux-iio/MN2PR12MB43905A2256F98BB5EFCE7DD3C4770@MN2PR12MB4390.namprd12.prod.outlook.com/T/