LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Jonathan Cameron <Jonathan.Cameron@Huawei.com>
To: Lucas Stankus <lucas.p.stankus@gmail.com>
Cc: Jonathan Cameron <jic23@kernel.org>,
	Lars-Peter Clausen <lars@metafoo.de>,
	"Hennerich, Michael" <Michael.Hennerich@analog.com>,
	<robh+dt@kernel.org>, "Bogdan, Dragos" <Dragos.Bogdan@analog.com>,
	"Berghe, Darius" <Darius.Berghe@analog.com>,
	linux-iio <linux-iio@vger.kernel.org>,
	<devicetree@vger.kernel.org>,
	Linux Kernel Mailing List <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH 2/2] iio: accel: Add driver support for ADXL313
Date: Tue, 3 Aug 2021 18:18:18 +0100	[thread overview]
Message-ID: <20210803181818.00001743@Huawei.com> (raw)
In-Reply-To: <CACKVXZD9nnAYb1bHNp32dzoGE3xnBPd6NR=9PNXkPVBSaPpaKw@mail.gmail.com>

On Mon, 2 Aug 2021 14:15:53 -0300
Lucas Stankus <lucas.p.stankus@gmail.com> wrote:

> On Sun, Aug 1, 2021 at 3:09 PM Jonathan Cameron <jic23@kernel.org> wrote:
> >
> > On Sat, 31 Jul 2021 17:36:48 -0300
> > Lucas Stankus <lucas.p.stankus@gmail.com> wrote:
> >  
> > > ADXL313 is a small, thin, low power, 3-axis accelerometer with high
> > > resolution measurement up to +/-4g. It includes an integrated 32-level
> > > FIFO and has activity and inactivity sensing capabilities.
> > >
> > > Datasheet: https://www.analog.com/media/en/technical-documentation/data-sheets/ADXL313.pdf
> > >
> > > Signed-off-by: Lucas Stankus <lucas.p.stankus@gmail.com>  
> >
> > Very nice.  A few really minor things inline.
> >
> > Jonathan
> >  
> 
> Thanks for the feedback!
> 
> I'll change the minor things for the v2.
> 

...

> > > +
> > > +static int adxl313_setup(struct device *dev, struct adxl313_data *data)
> > > +{
> > > +     unsigned int regval;
> > > +     int ret;
> > > +
> > > +     /* Ensures the device is in a consistent state after start up */
> > > +     ret = regmap_write(data->regmap, ADXL313_REG_SOFT_RESET,
> > > +                        ADXL313_SOFT_RESET);
> > > +     if (ret)
> > > +             return ret;
> > > +
> > > +     if (device_property_read_bool(dev, "spi-3wire")) {  
> >
> > Rather odd to see spi specific stuff in here.  Perhaps provide a callback to
> > common probe if it needs to be done at this point in bringing the device up.
> > However, I think you can just do this before calling the common_probe()
> >  
> 
> I'm doing this here because of the device reset, so whatever I write
> to the register before it would be overwritten in setup. The datasheet
> doesn't say that resetting the device is strictly necessary, but I
> figured it would be better to do so to force consistency.
> 
> If I drop the reset, I'd be able to do it before the core probe call
> and, from what I'm seeing here, the startup seems to be consistent
> without it. Do you think it's best to drop the device reset?
Ah. I'd missed that.  Fair enough.

In this case, I'd pass in a function pointer from the spi module. If the
function pointer is provided, call it to spi specific setup, if NULL
don't call it (so we don't need to provide an empty stub in the i2c side of things).

> 
> > > +             ret = regmap_write(data->regmap, ADXL313_REG_DATA_FORMAT,
> > > +                                ADXL313_SPI_3WIRE);
> > > +             if (ret)
> > > +                     return ret;
> > > +     }
> > > +
> > > +     ret = regmap_read(data->regmap, ADXL313_REG_DEVID0, &regval);
> > > +     if (ret)
> > > +             return ret;
> > > +
> > > +     if (regval != ADXL313_DEVID0) {
> > > +             dev_err(dev, "Invalid manufacturer ID: 0x%02x\n", regval);
> > > +             return -ENODEV;
> > > +     }
> > > +
> > > +     ret = regmap_read(data->regmap, ADXL313_REG_DEVID1, &regval);
> > > +     if (ret)
> > > +             return ret;
> > > +

...

> > > diff --git a/drivers/iio/accel/adxl313_spi.c b/drivers/iio/accel/adxl313_spi.c
> > > new file mode 100644
> > > index 000000000000..7c58c9ff8985
> > > --- /dev/null
> > > +++ b/drivers/iio/accel/adxl313_spi.c
> > > @@ -0,0 +1,74 @@
> > > +// SPDX-License-Identifier: GPL-2.0-only
> > > +/*
> > > + * ADXL313 3-Axis Digital Accelerometer
> > > + *
> > > + * Copyright (c) 2021 Lucas Stankus <lucas.p.stankus@gmail.com>
> > > + *
> > > + * Datasheet: https://www.analog.com/media/en/technical-documentation/data-sheets/ADXL313.pdf
> > > + */
> > > +
> > > +#include <linux/module.h>
> > > +#include <linux/regmap.h>
> > > +#include <linux/spi/spi.h>
> > > +
> > > +#include "adxl313.h"
> > > +
> > > +static const struct regmap_config adxl313_spi_regmap_config = {
> > > +     .reg_bits       = 8,
> > > +     .val_bits       = 8,
> > > +     .rd_table       = &adxl313_readable_regs_table,
> > > +     .wr_table       = &adxl313_writable_regs_table,
> > > +     .max_register   = 0x39,
> > > +      /* Setting bits 7 and 6 enables multiple-byte read */
> > > +     .read_flag_mask = BIT(7) | BIT(6)
> > > +};
> > > +
> > > +static int adxl313_spi_probe(struct spi_device *spi)
> > > +{
> > > +     const struct spi_device_id *id = spi_get_device_id(spi);
> > > +     struct regmap *regmap;
> > > +     int ret;
> > > +
> > > +     regmap = devm_regmap_init_spi(spi, &adxl313_spi_regmap_config);
> > > +     if (IS_ERR(regmap)) {
> > > +             dev_err(&spi->dev, "Error initializing spi regmap: %ld\n",
> > > +                     PTR_ERR(regmap));
> > > +             return PTR_ERR(regmap);
> > > +     }
> > > +
> > > +     ret = adxl313_core_probe(&spi->dev, regmap, id->name);
> > > +     if (ret < 0)
> > > +             return ret;
> > > +
> > > +     return regmap_update_bits(regmap, ADXL313_REG_POWER_CTL,
> > > +                               ADXL313_I2C_DISABLE, ADXL313_I2C_DISABLE);  
> >
> > Why is this only done after the rest of probe?  Needs a comment perhaps.
> > Normally I'd expect the core probe and hence exposure of the device
> > to userspace etc to be the last thing done.
> >  
> 
> I'm doing this here for the same reason as for the spi-3wire setup. So
> if I drop the reset in the probe, the bits could be updated before it.

Put that in the function that you pass as a pointer to core_probe() - then you
can do them both at the same time and at an appropriate point.


Jonathan

  reply	other threads:[~2021-08-03 17:18 UTC|newest]

Thread overview: 14+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-07-31 20:36 [PATCH 0/2] iio: accel: Add support for ADXL313 accelerometer Lucas Stankus
2021-07-31 20:36 ` [PATCH 1/2] dt-bindings: iio: accel: Add binding documentation for ADXL313 Lucas Stankus
2021-08-01 18:04   ` Jonathan Cameron
2021-08-02 16:49     ` Lucas Stankus
2021-08-02 13:39   ` Rob Herring
2021-07-31 20:36 ` [PATCH 2/2] iio: accel: Add driver support " Lucas Stankus
2021-08-01 18:12   ` Jonathan Cameron
2021-08-02  6:57     ` Alexandru Ardelean
2021-08-02 17:28       ` Lucas Stankus
2021-08-03  7:15         ` Alexandru Ardelean
2021-08-02 17:15     ` Lucas Stankus
2021-08-03 17:18       ` Jonathan Cameron [this message]
2021-08-02  7:04   ` Alexandru Ardelean
2021-08-02 17:30     ` Lucas Stankus

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20210803181818.00001743@Huawei.com \
    --to=jonathan.cameron@huawei.com \
    --cc=Darius.Berghe@analog.com \
    --cc=Dragos.Bogdan@analog.com \
    --cc=Michael.Hennerich@analog.com \
    --cc=devicetree@vger.kernel.org \
    --cc=jic23@kernel.org \
    --cc=lars@metafoo.de \
    --cc=linux-iio@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=lucas.p.stankus@gmail.com \
    --cc=robh+dt@kernel.org \
    --subject='Re: [PATCH 2/2] iio: accel: Add driver support for ADXL313' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).