On Mon, Mar 13, 2017 at 02:12:54PM +0200, Andy Shevchenko wrote:
> On Mon, Mar 13, 2017 at 1:11 PM, Eva Rachel Retuya <eraretuya@xxxxxxxxx> wrote:
>
Hello Andy,
Thanks for the review.
> Missed commit message is no-no!
>
> > Signed-off-by: Eva Rachel Retuya <eraretuya@xxxxxxxxx>
>
> > -int adxl345_core_probe(struct device *dev, struct regmap *regmap,
> > - const char *name);
> > +int adxl345_core_probe(struct device *dev, struct regmap *regmap, int irq,
> > + const char *name, bool use_int2);
>
> Hmm... And tomorrow you will add another flag and another.
>
> No, consider to use something like
>
> struct adxl345_chip {
> struct device *dev;
> struct regmap *regmap;
> const char *name;
> }
>
> Convert your probe to use it, and after extend for your needs.
>
> > #define ADXL345_DEVID 0xE5
> >
> > +#define ADXL345_IRQ_NAME "adxl345_event"
>
>
> > struct adxl345_data {
> > + struct iio_trigger *drdy_trig;
> > struct regmap *regmap;
> > + bool drdy_trig_on;
> > u8 data_range;
>
> drdy -> data_ready
>
> > +static irqreturn_t adxl345_irq(int irq, void *p)
> > +{
> > + struct iio_dev *indio_dev = p;
> > + struct adxl345_data *data = iio_priv(indio_dev);
> > + int ret = IRQ_NONE;
> > + u32 int_stat;
> > +
>
> > + ret = regmap_read(data->regmap, ADXL345_REG_INT_SOURCE, &int_stat);
>
> > + if (ret < 0)
> > + return ret;
>
> It makes little sense AFAIU.
>
Can you please elaborate further your comment regarding this?
> > +
> > + if (int_stat & ADXL345_INT_DATA_READY) {
> > + iio_trigger_poll(data->drdy_trig);
> > + ret = IRQ_HANDLED;
> > + }
> > +
> > + return ret;
>
> Useless variable ret. You may return values directly.
>
> > +}
> > +
> > +static int adxl345_drdy_trigger_set_state(struct iio_trigger *trig, bool state)
> > +{
> > + struct iio_dev *indio_dev = iio_trigger_get_drvdata(trig);
> > + struct adxl345_data *data = iio_priv(indio_dev);
> > + struct device *dev;
> > + int ret;
> > +
>
> > + dev = regmap_get_device(data->regmap);
>
> This may be moved to definition block.
>
> > + ret = regmap_update_bits(data->regmap, ADXL345_REG_INT_ENABLE,
>
> > + ADXL345_INT_DATA_READY, (state ?
> > + ADXL345_INT_DATA_READY : 0));
>
> No way:
> Don't split lines like this.
> Remove extra parens.
>
> > +static const struct iio_trigger_ops adxl345_trigger_ops = {
>
> > + .owner = THIS_MODULE,
>
> I dunno if we still need this.
>
> > static const struct iio_info adxl345_info = {
>
> > .driver_module = THIS_MODULE,
>
> Ditto.
>
> > .read_raw = adxl345_read_raw,
> > };
>
> > + /*
> > + * Any bits set to 0 send their respective interrupts to the INT1 pin,
> > + * whereas bits set to 1 send their respective interrupts to the INT2
> > + * pin. Map all interrupts to the specified pin.
> > + */
>
> > + if (!use_int2)
> > + ret = regmap_write(data->regmap, ADXL345_REG_INT_MAP, 0x00);
> > + else
> > + ret = regmap_write(data->regmap, ADXL345_REG_INT_MAP, 0xFF);
>
> I would create a temporary variable to hold the value and call
> regmap_write() unconditionally.
>
>
> > - return iio_device_register(indio_dev);
>
> You are not supposed to ping-pong changes in your series. Make clear
> your goal either you do like above or like below. If you choose
> latter, don't alter it in previous patch.
>
Ack. Will revise carefully with consideration to your comments in this
whole series.
> > + if (irq > 0) {
>
> > + ret =
> > + devm_request_threaded_irq(dev, irq, NULL, adxl345_irq,
>
> Don't split lines like this.
>
> > + IRQF_TRIGGER_HIGH | IRQF_ONESHOT,
>
> Are you sure you have threaded IRQ handler?
>
> > + ret = iio_device_register(indio_dev);
> > + if (ret < 0) {
> > + dev_err(dev, "iio_device_register failed: %d\n", ret);
> > + goto err_trigger_unregister;
> > + }
> > +
> > + return ret;
>
> > +err_trigger_unregister:
> > + if (data->drdy_trig)
> > + iio_trigger_unregister(data->drdy_trig);
> > +
> > + return ret;
>
> So, doesn't devm_iio_*() provide a facility to avoid this?
>
> > @@ -229,6 +334,8 @@ int adxl345_core_remove(struct device *dev)
> > struct adxl345_data *data = iio_priv(indio_dev);
> >
> > iio_device_unregister(indio_dev);
> > + if (data->drdy_trig)
> > + iio_trigger_unregister(data->drdy_trig);
>
> Ditto.
>
> > --- a/drivers/iio/accel/adxl345_i2c.c
> > +++ b/drivers/iio/accel/adxl345_i2c.c
>
> > - return adxl345_core_probe(&client->dev, regmap, id ? id->name : NULL);
> > + irq = of_irq_get_byname(client->dev.of_node, "INT2");
> > + if (irq == client->irq)
> > + use_int2 = true;
>
> Can't you use platform_get_irq() instead?
>
>
> > - return adxl345_core_probe(&spi->dev, regmap, id->name);
> > + irq = of_irq_get_byname(spi->dev.of_node, "INT2");
> > + if (irq == spi->irq)
> > + use_int2 = true;
>
> Ditto.
>
> P.S. Are you doing this stuff on your own or you are working for some
> company? If the latter applies, please, consider to do *internal*
> review first.
>
I'm doing it on my own as a project for Outreachy and have mentors to ask
for advice and/or questions.
Eva
> --
> With Best Regards,
> Andy Shevchenko
--
To unsubscribe from this list: send the line "unsubscribe linux-iio" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
