Pass a function setup() as pointer from SPI/I2C specific modules
to the core module. Implement setup() to pass the spi-3wire bus
option, if declared in the device-tree.
In the core module, then update data_format register
configuration bits instead of overwriting it. The changes allow
to remove a data_range field.
Signed-off-by: Lothar Rubusch <[email protected]>
---
V1 -> V2: split into spi-3wire and refactoring
V2 -> V3: split further, focus on needed changesets
V3 -> V4: drop "Remove single info instances";
split "Group bus configuration" into separat
comment patch; reorder patch set
Lothar Rubusch (7):
iio: accel: adxl345: Make data_range obsolete
iio: accel: adxl345: Group bus configuration
iio: accel: adxl345: Move defines to header
dt-bindings: iio: accel: adxl345: Add spi-3wire
iio: accel: adxl345: Pass function pointer to core
iio: accel: adxl345: Add comment to probe
iio: accel: adxl345: Add spi-3wire option
.../bindings/iio/accel/adi,adxl345.yaml | 2 +
drivers/iio/accel/adxl345.h | 35 ++++++++++-
drivers/iio/accel/adxl345_core.c | 63 ++++++++-----------
drivers/iio/accel/adxl345_i2c.c | 2 +-
drivers/iio/accel/adxl345_spi.c | 12 +++-
5 files changed, 74 insertions(+), 40 deletions(-)
--
2.25.1
Add spi-3wire because the driver optionally supports spi-3wire.
Signed-off-by: Lothar Rubusch <[email protected]>
---
Documentation/devicetree/bindings/iio/accel/adi,adxl345.yaml | 2 ++
1 file changed, 2 insertions(+)
diff --git a/Documentation/devicetree/bindings/iio/accel/adi,adxl345.yaml b/Documentation/devicetree/bindings/iio/accel/adi,adxl345.yaml
index 07cacc3f6..280ed479e 100644
--- a/Documentation/devicetree/bindings/iio/accel/adi,adxl345.yaml
+++ b/Documentation/devicetree/bindings/iio/accel/adi,adxl345.yaml
@@ -32,6 +32,8 @@ properties:
spi-cpol: true
+ spi-3wire: true
+
interrupts:
maxItems: 1
--
2.25.1
Add a comment to the probe() function to describe the
passed function pointer setup in particular.
Signed-off-by: Lothar Rubusch <[email protected]>
---
drivers/iio/accel/adxl345_core.c | 10 ++++++++++
1 file changed, 10 insertions(+)
diff --git a/drivers/iio/accel/adxl345_core.c b/drivers/iio/accel/adxl345_core.c
index 476d729bc..cc3da4ece 100644
--- a/drivers/iio/accel/adxl345_core.c
+++ b/drivers/iio/accel/adxl345_core.c
@@ -168,6 +168,16 @@ static void adxl345_powerdown(void *regmap)
regmap_write(regmap, ADXL345_REG_POWER_CTL, ADXL345_POWER_CTL_STANDBY);
}
+/**
+ * adxl345_core_probe() - probe and setup for the adxl345 accelerometer,
+ * also covers the adlx375 accelerometer
+ * @dev: Driver model representation of the device
+ * @regmap: Regmap instance for the device
+ * @setup: Setup routine to be executed right before the standard device
+ * setup
+ *
+ * Return: 0 on success, negative errno on error
+ */
int adxl345_core_probe(struct device *dev, struct regmap *regmap,
int (*setup)(struct device*, struct regmap*))
{
--
2.25.1
Add a setup function implementation to the spi module to enable
spi-3wire as option when specified in the device-tree.
Signed-off-by: Lothar Rubusch <[email protected]>
---
drivers/iio/accel/adxl345.h | 1 +
drivers/iio/accel/adxl345_spi.c | 12 +++++++++++-
2 files changed, 12 insertions(+), 1 deletion(-)
diff --git a/drivers/iio/accel/adxl345.h b/drivers/iio/accel/adxl345.h
index 620a2e0f0..55a72ca38 100644
--- a/drivers/iio/accel/adxl345.h
+++ b/drivers/iio/accel/adxl345.h
@@ -36,6 +36,7 @@
#define ADXL345_DATA_FORMAT_8G 2
#define ADXL345_DATA_FORMAT_16G 3
#define ADXL345_DATA_FORMAT_MSK ~((u8) BIT(6)) /* ignore spi-3wire */
+#define ADXL345_DATA_FORMAT_SPI_3WIRE BIT(6)
#define ADXL345_DEVID 0xE5
diff --git a/drivers/iio/accel/adxl345_spi.c b/drivers/iio/accel/adxl345_spi.c
index 1c0513bd3..f145d5c1d 100644
--- a/drivers/iio/accel/adxl345_spi.c
+++ b/drivers/iio/accel/adxl345_spi.c
@@ -20,6 +20,16 @@ static const struct regmap_config adxl345_spi_regmap_config = {
.read_flag_mask = BIT(7) | BIT(6),
};
+static int adxl345_spi_setup(struct device *dev, struct regmap *regmap)
+{
+ struct spi_device *spi = container_of(dev, struct spi_device, dev);
+
+ if (spi->mode & SPI_3WIRE)
+ return regmap_write(regmap, ADXL345_REG_DATA_FORMAT,
+ ADXL345_DATA_FORMAT_SPI_3WIRE);
+ return 0;
+}
+
static int adxl345_spi_probe(struct spi_device *spi)
{
struct regmap *regmap;
@@ -33,7 +43,7 @@ static int adxl345_spi_probe(struct spi_device *spi)
if (IS_ERR(regmap))
return dev_err_probe(&spi->dev, PTR_ERR(regmap), "Error initializing regmap\n");
- return adxl345_core_probe(&spi->dev, regmap, NULL);
+ return adxl345_core_probe(&spi->dev, regmap, adxl345_spi_setup);
}
static const struct adxl345_chip_info adxl345_spi_info = {
--
2.25.1
In the probe function group bus configuration and the
indio_dev initialization to improve readability.
Signed-off-by: Lothar Rubusch <[email protected]>
---
drivers/iio/accel/adxl345_core.c | 13 +++++++------
1 file changed, 7 insertions(+), 6 deletions(-)
diff --git a/drivers/iio/accel/adxl345_core.c b/drivers/iio/accel/adxl345_core.c
index be6758015..469015e9c 100644
--- a/drivers/iio/accel/adxl345_core.c
+++ b/drivers/iio/accel/adxl345_core.c
@@ -218,22 +218,23 @@ int adxl345_core_probe(struct device *dev, struct regmap *regmap)
data = iio_priv(indio_dev);
data->regmap = regmap;
- /* Enable full-resolution mode */
+
data->info = device_get_match_data(dev);
if (!data->info)
return -ENODEV;
- ret = regmap_update_bits(regmap, ADXL345_REG_DATA_FORMAT,
- ADXL345_DATA_FORMAT_MSK, ADXL345_DATA_FORMAT_FULL_RES);
- if (ret)
- return dev_err_probe(dev, ret, "Failed to set data range\n");
-
indio_dev->name = data->info->name;
indio_dev->info = &adxl345_info;
indio_dev->modes = INDIO_DIRECT_MODE;
indio_dev->channels = adxl345_channels;
indio_dev->num_channels = ARRAY_SIZE(adxl345_channels);
+ /* Enable full-resolution mode */
+ ret = regmap_update_bits(regmap, ADXL345_REG_DATA_FORMAT,
+ ADXL345_DATA_FORMAT_MSK, ADXL345_DATA_FORMAT_FULL_RES);
+ if (ret)
+ return dev_err_probe(dev, ret, "Failed to set data range\n");
+
/* Enable measurement mode */
ret = adxl345_powerup(data->regmap);
if (ret < 0)
--
2.25.1
Replace write() data_format by regmap_update_bits(), because
bus specific pre-configuration may have happened before on
the same register. Changes then need to be masked.
Remove the data_range field from the struct adxl345_data,
because it is not used anymore.
Signed-off-by: Lothar Rubusch <[email protected]>
---
drivers/iio/accel/adxl345_core.c | 9 ++++-----
1 file changed, 4 insertions(+), 5 deletions(-)
diff --git a/drivers/iio/accel/adxl345_core.c b/drivers/iio/accel/adxl345_core.c
index 8bd30a23e..be6758015 100644
--- a/drivers/iio/accel/adxl345_core.c
+++ b/drivers/iio/accel/adxl345_core.c
@@ -42,13 +42,13 @@
#define ADXL345_DATA_FORMAT_4G 1
#define ADXL345_DATA_FORMAT_8G 2
#define ADXL345_DATA_FORMAT_16G 3
+#define ADXL345_DATA_FORMAT_MSK ~((u8) BIT(6)) /* ignore spi-3wire */
#define ADXL345_DEVID 0xE5
struct adxl345_data {
const struct adxl345_chip_info *info;
struct regmap *regmap;
- u8 data_range;
};
#define ADXL345_CHANNEL(index, axis) { \
@@ -219,14 +219,13 @@ int adxl345_core_probe(struct device *dev, struct regmap *regmap)
data = iio_priv(indio_dev);
data->regmap = regmap;
/* Enable full-resolution mode */
- data->data_range = ADXL345_DATA_FORMAT_FULL_RES;
data->info = device_get_match_data(dev);
if (!data->info)
return -ENODEV;
- ret = regmap_write(data->regmap, ADXL345_REG_DATA_FORMAT,
- data->data_range);
- if (ret < 0)
+ ret = regmap_update_bits(regmap, ADXL345_REG_DATA_FORMAT,
+ ADXL345_DATA_FORMAT_MSK, ADXL345_DATA_FORMAT_FULL_RES);
+ if (ret)
return dev_err_probe(dev, ret, "Failed to set data range\n");
indio_dev->name = data->info->name;
--
2.25.1
On 25/03/2024 16:33, Lothar Rubusch wrote:
> Add spi-3wire because the driver optionally supports spi-3wire.
This is a friendly reminder during the review process.
It seems my or other reviewer's previous comments were not fully
addressed. Maybe the feedback got lost between the quotes, maybe you
just forgot to apply it. Please go back to the previous discussion and
either implement all requested changes or keep discussing them.
Thank you.
>
> Signed-off-by: Lothar Rubusch <[email protected]>
> ---
This is a friendly reminder during the review process.
It looks like you received a tag and forgot to add it.
If you do not know the process, here is a short explanation:
Please add Acked-by/Reviewed-by/Tested-by tags when posting new
versions, under or above your Signed-off-by tag. Tag is "received", when
provided in a message replied to you on the mailing list. Tools like b4
can help here. However, there's no need to repost patches *only* to add
the tags. The upstream maintainer will do that for tags received on the
version they apply.
https://elixir.bootlin.com/linux/v6.5-rc3/source/Documentation/process/submitting-patches.rst#L577
If a tag was not added on purpose, please state why and what changed.
Best regards,
Krzysztof
On Mon, 25 Mar 2024 15:33:50 +0000
Lothar Rubusch <[email protected]> wrote:
> Replace write() data_format by regmap_update_bits(), because
> bus specific pre-configuration may have happened before on
> the same register. Changes then need to be masked.
>
> Remove the data_range field from the struct adxl345_data,
> because it is not used anymore.
>
> Signed-off-by: Lothar Rubusch <[email protected]>
> ---
> drivers/iio/accel/adxl345_core.c | 9 ++++-----
> 1 file changed, 4 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/iio/accel/adxl345_core.c b/drivers/iio/accel/adxl345_core.c
> index 8bd30a23e..be6758015 100644
> --- a/drivers/iio/accel/adxl345_core.c
> +++ b/drivers/iio/accel/adxl345_core.c
> @@ -42,13 +42,13 @@
> #define ADXL345_DATA_FORMAT_4G 1
> #define ADXL345_DATA_FORMAT_8G 2
> #define ADXL345_DATA_FORMAT_16G 3
> +#define ADXL345_DATA_FORMAT_MSK ~((u8) BIT(6)) /* ignore spi-3wire */
I'm not keen on seeing masking of a bit we don't yet
handle done by value. Can we instead build this up by what we 'want' to
write rather than don't. Will need a few more defines perhaps to cover
the masks of SELF_TEST, INT_INVERT, FULL_RES, Justify and Range.
>
> #define ADXL345_DEVID 0xE5
>
> struct adxl345_data {
> const struct adxl345_chip_info *info;
> struct regmap *regmap;
> - u8 data_range;
> };
>
> #define ADXL345_CHANNEL(index, axis) { \
> @@ -219,14 +219,13 @@ int adxl345_core_probe(struct device *dev, struct regmap *regmap)
> data = iio_priv(indio_dev);
> data->regmap = regmap;
> /* Enable full-resolution mode */
> - data->data_range = ADXL345_DATA_FORMAT_FULL_RES;
> data->info = device_get_match_data(dev);
> if (!data->info)
> return -ENODEV;
>
> - ret = regmap_write(data->regmap, ADXL345_REG_DATA_FORMAT,
> - data->data_range);
> - if (ret < 0)
> + ret = regmap_update_bits(regmap, ADXL345_REG_DATA_FORMAT,
> + ADXL345_DATA_FORMAT_MSK, ADXL345_DATA_FORMAT_FULL_RES);
> + if (ret)
> return dev_err_probe(dev, ret, "Failed to set data range\n");
>
> indio_dev->name = data->info->name;
On Mon, 25 Mar 2024 15:33:56 +0000
Lothar Rubusch <[email protected]> wrote:
> Add a setup function implementation to the spi module to enable
> spi-3wire as option when specified in the device-tree.
>
> Signed-off-by: Lothar Rubusch <[email protected]>
> ---
> drivers/iio/accel/adxl345.h | 1 +
> drivers/iio/accel/adxl345_spi.c | 12 +++++++++++-
> 2 files changed, 12 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/iio/accel/adxl345.h b/drivers/iio/accel/adxl345.h
> index 620a2e0f0..55a72ca38 100644
> --- a/drivers/iio/accel/adxl345.h
> +++ b/drivers/iio/accel/adxl345.h
> @@ -36,6 +36,7 @@
> #define ADXL345_DATA_FORMAT_8G 2
> #define ADXL345_DATA_FORMAT_16G 3
> #define ADXL345_DATA_FORMAT_MSK ~((u8) BIT(6)) /* ignore spi-3wire */
> +#define ADXL345_DATA_FORMAT_SPI_3WIRE BIT(6)
If you argue in favour of keeping the MSK as negation of this bit, then
reorder these two and define FORMAT_MSK in terms of SPI_3WIRE.
Note the name of FORMAT_MASK is also an issue now you actually write the 3wire bit.
So I'd get rid of that define in favour of positive defines of all the fields that
make it up used inline as it's only accessed in one place anyway.
That avoids the need for a mask of 'not' something.
>
> #define ADXL345_DEVID 0xE5
>
> diff --git a/drivers/iio/accel/adxl345_spi.c b/drivers/iio/accel/adxl345_spi.c
> index 1c0513bd3..f145d5c1d 100644
> --- a/drivers/iio/accel/adxl345_spi.c
> +++ b/drivers/iio/accel/adxl345_spi.c
> @@ -20,6 +20,16 @@ static const struct regmap_config adxl345_spi_regmap_config = {
> .read_flag_mask = BIT(7) | BIT(6),
> };
>
> +static int adxl345_spi_setup(struct device *dev, struct regmap *regmap)
> +{
> + struct spi_device *spi = container_of(dev, struct spi_device, dev);
> +
> + if (spi->mode & SPI_3WIRE)
> + return regmap_write(regmap, ADXL345_REG_DATA_FORMAT,
> + ADXL345_DATA_FORMAT_SPI_3WIRE);
> + return 0;
> +}
> +
> static int adxl345_spi_probe(struct spi_device *spi)
> {
> struct regmap *regmap;
> @@ -33,7 +43,7 @@ static int adxl345_spi_probe(struct spi_device *spi)
> if (IS_ERR(regmap))
> return dev_err_probe(&spi->dev, PTR_ERR(regmap), "Error initializing regmap\n");
>
> - return adxl345_core_probe(&spi->dev, regmap, NULL);
> + return adxl345_core_probe(&spi->dev, regmap, adxl345_spi_setup);
> }
>
> static const struct adxl345_chip_info adxl345_spi_info = {
Add a function pointer argument to the probe function in
the core module to provide a way to pre-configure the bus.
The passed setup function can be prepared in the bus
specific spi or the i2c module, or NULL otherwise. It shall
then be executed in the bus independent core module.
Signed-off-by: Lothar Rubusch <[email protected]>
---
drivers/iio/accel/adxl345.h | 3 ++-
drivers/iio/accel/adxl345_core.c | 10 +++++++++-
drivers/iio/accel/adxl345_i2c.c | 2 +-
drivers/iio/accel/adxl345_spi.c | 2 +-
4 files changed, 13 insertions(+), 4 deletions(-)
diff --git a/drivers/iio/accel/adxl345.h b/drivers/iio/accel/adxl345.h
index ee169fed4..620a2e0f0 100644
--- a/drivers/iio/accel/adxl345.h
+++ b/drivers/iio/accel/adxl345.h
@@ -59,6 +59,7 @@ struct adxl345_chip_info {
int uscale;
};
-int adxl345_core_probe(struct device *dev, struct regmap *regmap);
+int adxl345_core_probe(struct device *dev, struct regmap *regmap,
+ int (*setup)(struct device*, struct regmap*));
#endif /* _ADXL345_H_ */
diff --git a/drivers/iio/accel/adxl345_core.c b/drivers/iio/accel/adxl345_core.c
index eba9c048a..476d729bc 100644
--- a/drivers/iio/accel/adxl345_core.c
+++ b/drivers/iio/accel/adxl345_core.c
@@ -168,13 +168,21 @@ static void adxl345_powerdown(void *regmap)
regmap_write(regmap, ADXL345_REG_POWER_CTL, ADXL345_POWER_CTL_STANDBY);
}
-int adxl345_core_probe(struct device *dev, struct regmap *regmap)
+int adxl345_core_probe(struct device *dev, struct regmap *regmap,
+ int (*setup)(struct device*, struct regmap*))
{
struct adxl345_data *data;
struct iio_dev *indio_dev;
u32 regval;
int ret;
+ /* Perform optional initial bus specific configuration */
+ if (setup) {
+ ret = setup(dev, regmap);
+ if (ret)
+ return ret;
+ }
+
ret = regmap_read(regmap, ADXL345_REG_DEVID, ®val);
if (ret < 0)
return dev_err_probe(dev, ret, "Error reading device ID\n");
diff --git a/drivers/iio/accel/adxl345_i2c.c b/drivers/iio/accel/adxl345_i2c.c
index a3084b0a8..4065b8f7c 100644
--- a/drivers/iio/accel/adxl345_i2c.c
+++ b/drivers/iio/accel/adxl345_i2c.c
@@ -27,7 +27,7 @@ static int adxl345_i2c_probe(struct i2c_client *client)
if (IS_ERR(regmap))
return dev_err_probe(&client->dev, PTR_ERR(regmap), "Error initializing regmap\n");
- return adxl345_core_probe(&client->dev, regmap);
+ return adxl345_core_probe(&client->dev, regmap, NULL);
}
static const struct adxl345_chip_info adxl345_i2c_info = {
diff --git a/drivers/iio/accel/adxl345_spi.c b/drivers/iio/accel/adxl345_spi.c
index 93ca349f1..1c0513bd3 100644
--- a/drivers/iio/accel/adxl345_spi.c
+++ b/drivers/iio/accel/adxl345_spi.c
@@ -33,7 +33,7 @@ static int adxl345_spi_probe(struct spi_device *spi)
if (IS_ERR(regmap))
return dev_err_probe(&spi->dev, PTR_ERR(regmap), "Error initializing regmap\n");
- return adxl345_core_probe(&spi->dev, regmap);
+ return adxl345_core_probe(&spi->dev, regmap, NULL);
}
static const struct adxl345_chip_info adxl345_spi_info = {
--
2.25.1
On Mon, Mar 25, 2024 at 7:32 PM Krzysztof Kozlowski
<[email protected]> wrote:
>
> On 25/03/2024 16:33, Lothar Rubusch wrote:
> > Add spi-3wire because the driver optionally supports spi-3wire.
>
> This is a friendly reminder during the review process.
>
> It seems my or other reviewer's previous comments were not fully
> addressed. Maybe the feedback got lost between the quotes, maybe you
> just forgot to apply it. Please go back to the previous discussion and
> either implement all requested changes or keep discussing them.
>
> Thank you.
>
You refer yourself to the above mentioned wording. Would replacing
"driver" by "device" in the dt-bindings patch comment be sufficient?
Did I miss something else?
> >
> > Signed-off-by: Lothar Rubusch <[email protected]>
> > ---
>
> This is a friendly reminder during the review process.
>
> It looks like you received a tag and forgot to add it.
>
> If you do not know the process, here is a short explanation:
> Please add Acked-by/Reviewed-by/Tested-by tags when posting new
> versions, under or above your Signed-off-by tag. Tag is "received", when
> provided in a message replied to you on the mailing list. Tools like b4
> can help here. However, there's no need to repost patches *only* to add
Just for confirmation: when I receive a feedback, requesting a change.
And, I accept the change request. This means, I received a tag
"Reviewed-by" which I have to mention in the upcoming patch version
where this change is implemented and in that particular patch?
> the tags. The upstream maintainer will do that for tags received on the
> version they apply.
>
I'm pretty sure we will still see further iterations. So, I apply the
tags in the next version, already scheduled. Ok?
> https://elixir.bootlin.com/linux/v6.5-rc3/source/Documentation/process/submitting-patches.rst#L577
>
Going over the books I feel it does not make sense to still mention
feedback ("Reveiewed-by") for the v1 or v2 of the patch here in a v5,
does it? Your link mentiones "However if the patch has changed
substantially in followin version, these tags might not be applicable
anymore"
https://elixir.bootlin.com/linux/v6.5-rc3/source/Documentation/process/submitting-patches.rst#L579
> If a tag was not added on purpose, please state why and what changed.
>
> Best regards,
> Krzysztof
>
I give it a try with b4. Let's see.
On Mon, Mar 25, 2024 at 10:40 PM Krzysztof Kozlowski
<[email protected]> wrote:
>
> On 25/03/2024 22:05, Lothar Rubusch wrote:
> > On Mon, Mar 25, 2024 at 7:32 PM Krzysztof Kozlowski
> > <[email protected]> wrote:
> >>
> >> On 25/03/2024 16:33, Lothar Rubusch wrote:
> >>> Add spi-3wire because the driver optionally supports spi-3wire.
> >>
> >> This is a friendly reminder during the review process.
> >>
> >> It seems my or other reviewer's previous comments were not fully
> >> addressed. Maybe the feedback got lost between the quotes, maybe you
> >> just forgot to apply it. Please go back to the previous discussion and
> >> either implement all requested changes or keep discussing them.
> >>
> >> Thank you.
> >>
> >
> > You refer yourself to the above mentioned wording. Would replacing
> > "driver" by "device" in the dt-bindings patch comment be sufficient?
> > Did I miss something else?
>
> Yes, the wording, but isn't the device require 3-wire mode? Don't just
> replace one word with another, but write the proper rationale for your
> hardware.
>
It does not require 3-wire SPI. By default the device communicates
regular SPI. It can be configured, though, to communicate 3-wire. The
given patch offers this as option in the DT.
> >
> >>>
> >>> Signed-off-by: Lothar Rubusch <[email protected]>
> >>> ---
> >>
> >> This is a friendly reminder during the review process.
> >>
> >> It looks like you received a tag and forgot to add it.
> >>
> >> If you do not know the process, here is a short explanation:
> >> Please add Acked-by/Reviewed-by/Tested-by tags when posting new
> >> versions, under or above your Signed-off-by tag. Tag is "received", when
> >> provided in a message replied to you on the mailing list. Tools like b4
> >> can help here. However, there's no need to repost patches *only* to add
> >
> > Just for confirmation: when I receive a feedback, requesting a change.
> > And, I accept the change request. This means, I received a tag
> > "Reviewed-by" which I have to mention in the upcoming patch version
> > where this change is implemented and in that particular patch?
>
> Please go through the docs. Yes, you received a tag which should be
> included with the change.
>
> Reviewer's feedback should not be ignored.
>
>
> >
> >> the tags. The upstream maintainer will do that for tags received on the
> >> version they apply.
> >>
> >
> > I'm pretty sure we will still see further iterations. So, I apply the
> > tags in the next version, already scheduled. Ok?
> >
> >> https://elixir.bootlin.com/linux/v6.5-rc3/source/Documentation/process/submitting-patches.rst#L577
> >>
> >
> > Going over the books I feel it does not make sense to still mention
> > feedback ("Reveiewed-by") for the v1 or v2 of the patch here in a v5,
> > does it? Your link mentiones "However if the patch has changed
>
> I don't understand. When did you receive the tag? v3, right? So what do
> you mean by v1 and v2?
>
V1: The first version of the 3wire patch. I have split the single
patch upon some feedback (yours?!) - V2... So, my current
interpretation is, that every feedback I need to mention as
Reviewed-by tag, no?
>
> Best regards,
> Krzysztof
>
Move defines to the header file. This keeps the defines block
together in one location, when extended by new shared defines
which then have to be in the header file.
Signed-off-by: Lothar Rubusch <[email protected]>
---
drivers/iio/accel/adxl345.h | 31 +++++++++++++++++++++++++++++++
drivers/iio/accel/adxl345_core.c | 29 -----------------------------
2 files changed, 31 insertions(+), 29 deletions(-)
diff --git a/drivers/iio/accel/adxl345.h b/drivers/iio/accel/adxl345.h
index 284bd387c..ee169fed4 100644
--- a/drivers/iio/accel/adxl345.h
+++ b/drivers/iio/accel/adxl345.h
@@ -8,6 +8,37 @@
#ifndef _ADXL345_H_
#define _ADXL345_H_
+#include <linux/iio/iio.h>
+
+#define ADXL345_REG_DEVID 0x00
+#define ADXL345_REG_OFSX 0x1E
+#define ADXL345_REG_OFSY 0x1F
+#define ADXL345_REG_OFSZ 0x20
+#define ADXL345_REG_OFS_AXIS(index) (ADXL345_REG_OFSX + (index))
+#define ADXL345_REG_BW_RATE 0x2C
+#define ADXL345_REG_POWER_CTL 0x2D
+#define ADXL345_REG_DATA_FORMAT 0x31
+#define ADXL345_REG_DATAX0 0x32
+#define ADXL345_REG_DATAY0 0x34
+#define ADXL345_REG_DATAZ0 0x36
+#define ADXL345_REG_DATA_AXIS(index) \
+ (ADXL345_REG_DATAX0 + (index) * sizeof(__le16))
+
+#define ADXL345_BW_RATE GENMASK(3, 0)
+#define ADXL345_BASE_RATE_NANO_HZ 97656250LL
+
+#define ADXL345_POWER_CTL_MEASURE BIT(3)
+#define ADXL345_POWER_CTL_STANDBY 0x00
+
+#define ADXL345_DATA_FORMAT_FULL_RES BIT(3) /* Up to 13-bits resolution */
+#define ADXL345_DATA_FORMAT_2G 0
+#define ADXL345_DATA_FORMAT_4G 1
+#define ADXL345_DATA_FORMAT_8G 2
+#define ADXL345_DATA_FORMAT_16G 3
+#define ADXL345_DATA_FORMAT_MSK ~((u8) BIT(6)) /* ignore spi-3wire */
+
+#define ADXL345_DEVID 0xE5
+
/*
* In full-resolution mode, scale factor is maintained at ~4 mg/LSB
* in all g ranges.
diff --git a/drivers/iio/accel/adxl345_core.c b/drivers/iio/accel/adxl345_core.c
index 469015e9c..eba9c048a 100644
--- a/drivers/iio/accel/adxl345_core.c
+++ b/drivers/iio/accel/adxl345_core.c
@@ -17,35 +17,6 @@
#include "adxl345.h"
-#define ADXL345_REG_DEVID 0x00
-#define ADXL345_REG_OFSX 0x1e
-#define ADXL345_REG_OFSY 0x1f
-#define ADXL345_REG_OFSZ 0x20
-#define ADXL345_REG_OFS_AXIS(index) (ADXL345_REG_OFSX + (index))
-#define ADXL345_REG_BW_RATE 0x2C
-#define ADXL345_REG_POWER_CTL 0x2D
-#define ADXL345_REG_DATA_FORMAT 0x31
-#define ADXL345_REG_DATAX0 0x32
-#define ADXL345_REG_DATAY0 0x34
-#define ADXL345_REG_DATAZ0 0x36
-#define ADXL345_REG_DATA_AXIS(index) \
- (ADXL345_REG_DATAX0 + (index) * sizeof(__le16))
-
-#define ADXL345_BW_RATE GENMASK(3, 0)
-#define ADXL345_BASE_RATE_NANO_HZ 97656250LL
-
-#define ADXL345_POWER_CTL_MEASURE BIT(3)
-#define ADXL345_POWER_CTL_STANDBY 0x00
-
-#define ADXL345_DATA_FORMAT_FULL_RES BIT(3) /* Up to 13-bits resolution */
-#define ADXL345_DATA_FORMAT_2G 0
-#define ADXL345_DATA_FORMAT_4G 1
-#define ADXL345_DATA_FORMAT_8G 2
-#define ADXL345_DATA_FORMAT_16G 3
-#define ADXL345_DATA_FORMAT_MSK ~((u8) BIT(6)) /* ignore spi-3wire */
-
-#define ADXL345_DEVID 0xE5
-
struct adxl345_data {
const struct adxl345_chip_info *info;
struct regmap *regmap;
--
2.25.1
On 25/03/2024 22:05, Lothar Rubusch wrote:
> On Mon, Mar 25, 2024 at 7:32 PM Krzysztof Kozlowski
> <[email protected]> wrote:
>>
>> On 25/03/2024 16:33, Lothar Rubusch wrote:
>>> Add spi-3wire because the driver optionally supports spi-3wire.
>>
>> This is a friendly reminder during the review process.
>>
>> It seems my or other reviewer's previous comments were not fully
>> addressed. Maybe the feedback got lost between the quotes, maybe you
>> just forgot to apply it. Please go back to the previous discussion and
>> either implement all requested changes or keep discussing them.
>>
>> Thank you.
>>
>
> You refer yourself to the above mentioned wording. Would replacing
> "driver" by "device" in the dt-bindings patch comment be sufficient?
> Did I miss something else?
Yes, the wording, but isn't the device require 3-wire mode? Don't just
replace one word with another, but write the proper rationale for your
hardware.
>
>>>
>>> Signed-off-by: Lothar Rubusch <[email protected]>
>>> ---
>>
>> This is a friendly reminder during the review process.
>>
>> It looks like you received a tag and forgot to add it.
>>
>> If you do not know the process, here is a short explanation:
>> Please add Acked-by/Reviewed-by/Tested-by tags when posting new
>> versions, under or above your Signed-off-by tag. Tag is "received", when
>> provided in a message replied to you on the mailing list. Tools like b4
>> can help here. However, there's no need to repost patches *only* to add
>
> Just for confirmation: when I receive a feedback, requesting a change.
> And, I accept the change request. This means, I received a tag
> "Reviewed-by" which I have to mention in the upcoming patch version
> where this change is implemented and in that particular patch?
Please go through the docs. Yes, you received a tag which should be
included with the change.
Reviewer's feedback should not be ignored.
>
>> the tags. The upstream maintainer will do that for tags received on the
>> version they apply.
>>
>
> I'm pretty sure we will still see further iterations. So, I apply the
> tags in the next version, already scheduled. Ok?
>
>> https://elixir.bootlin.com/linux/v6.5-rc3/source/Documentation/process/submitting-patches.rst#L577
>>
>
> Going over the books I feel it does not make sense to still mention
> feedback ("Reveiewed-by") for the v1 or v2 of the patch here in a v5,
> does it? Your link mentiones "However if the patch has changed
I don't understand. When did you receive the tag? v3, right? So what do
you mean by v1 and v2?
Best regards,
Krzysztof
On Mon, 25 Mar 2024 15:33:51 +0000
Lothar Rubusch <[email protected]> wrote:
> In the probe function group bus configuration and the
> indio_dev initialization to improve readability.
>
> Signed-off-by: Lothar Rubusch <[email protected]>
> ---
> drivers/iio/accel/adxl345_core.c | 13 +++++++------
> 1 file changed, 7 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/iio/accel/adxl345_core.c b/drivers/iio/accel/adxl345_core.c
> index be6758015..469015e9c 100644
> --- a/drivers/iio/accel/adxl345_core.c
> +++ b/drivers/iio/accel/adxl345_core.c
> @@ -218,22 +218,23 @@ int adxl345_core_probe(struct device *dev, struct regmap *regmap)
>
> data = iio_priv(indio_dev);
> data->regmap = regmap;
> - /* Enable full-resolution mode */
The naming of the written value gives this comment away + it's obviously misplaced
after previous patch. I'd just drop the comment in patch 1.
> +
> data->info = device_get_match_data(dev);
> if (!data->info)
> return -ENODEV;
>
> - ret = regmap_update_bits(regmap, ADXL345_REG_DATA_FORMAT,
> - ADXL345_DATA_FORMAT_MSK, ADXL345_DATA_FORMAT_FULL_RES);
> - if (ret)
> - return dev_err_probe(dev, ret, "Failed to set data range\n");
> -
> indio_dev->name = data->info->name;
> indio_dev->info = &adxl345_info;
> indio_dev->modes = INDIO_DIRECT_MODE;
> indio_dev->channels = adxl345_channels;
> indio_dev->num_channels = ARRAY_SIZE(adxl345_channels);
>
> + /* Enable full-resolution mode */
> + ret = regmap_update_bits(regmap, ADXL345_REG_DATA_FORMAT,
> + ADXL345_DATA_FORMAT_MSK, ADXL345_DATA_FORMAT_FULL_RES);
> + if (ret)
> + return dev_err_probe(dev, ret, "Failed to set data range\n");
> +
> /* Enable measurement mode */
> ret = adxl345_powerup(data->regmap);
> if (ret < 0)
On 25/03/2024 23:09, Lothar Rubusch wrote:
>>
>>
>>>
>>>> the tags. The upstream maintainer will do that for tags received on the
>>>> version they apply.
>>>>
>>>
>>> I'm pretty sure we will still see further iterations. So, I apply the
>>> tags in the next version, already scheduled. Ok?
>>>
>>>> https://elixir.bootlin.com/linux/v6.5-rc3/source/Documentation/process/submitting-patches.rst#L577
>>>>
>>>
>>> Going over the books I feel it does not make sense to still mention
>>> feedback ("Reveiewed-by") for the v1 or v2 of the patch here in a v5,
>>> does it? Your link mentiones "However if the patch has changed
>>
>> I don't understand. When did you receive the tag? v3, right? So what do
>> you mean by v1 and v2?
>>
>
> V1: The first version of the 3wire patch. I have split the single
> patch upon some feedback (yours?!) - V2... So, my current
> interpretation is, that every feedback I need to mention as
> Reviewed-by tag, no?
What? Feedback is not review. It's clearly explained in submitting
patches. Please read it.
Best regards,
Krzysztof
On Tue, Mar 26, 2024 at 7:30 AM Krzysztof Kozlowski
<[email protected]> wrote:
>
> On 25/03/2024 23:09, Lothar Rubusch wrote:
> >>
> >>
> >>>
> >>>> the tags. The upstream maintainer will do that for tags received on the
> >>>> version they apply.
> >>>>
> >>>
> >>> I'm pretty sure we will still see further iterations. So, I apply the
> >>> tags in the next version, already scheduled. Ok?
> >>>
> >>>> https://elixir.bootlin.com/linux/v6.5-rc3/source/Documentation/process/submitting-patches.rst#L577
> >>>>
> >>>
> >>> Going over the books I feel it does not make sense to still mention
> >>> feedback ("Reveiewed-by") for the v1 or v2 of the patch here in a v5,
> >>> does it? Your link mentiones "However if the patch has changed
> >>
> >> I don't understand. When did you receive the tag? v3, right? So what do
> >> you mean by v1 and v2?
> >>
> >
> > V1: The first version of the 3wire patch. I have split the single
> > patch upon some feedback (yours?!) - V2... So, my current
> > interpretation is, that every feedback I need to mention as
> > Reviewed-by tag, no?
>
> What? Feedback is not review. It's clearly explained in submitting
> patches. Please read it.
>
Exactly. My missunderstanding here is this: Why did you send me a
reminder that I forgot to add "Reviewed-by" tag in your last mail?
Could you please clarify your last mail? You wrote:
"(...)
This is a friendly reminder during the review process.
It looks like you received a tag and forgot to add it.
If you do not know the process, here is a short explanation:
Please add Acked-by/Reviewed-by/Tested-by tags when posting new
versions, (...)"
AFAIK noone literally had told me: "please add a Reviewed-by me tag",
or did I miss something? I'm a bit lost here, sorry.
> Best regards,
> Krzysztof
>
On Mon, Mar 25, 2024 at 9:32 PM Jonathan Cameron <[email protected]> wrote:
>
> On Mon, 25 Mar 2024 15:33:50 +0000
> Lothar Rubusch <[email protected]> wrote:
>
> > Replace write() data_format by regmap_update_bits(), because
> > bus specific pre-configuration may have happened before on
> > the same register. Changes then need to be masked.
> >
> > Remove the data_range field from the struct adxl345_data,
> > because it is not used anymore.
> >
> > Signed-off-by: Lothar Rubusch <[email protected]>
> > ---
> > drivers/iio/accel/adxl345_core.c | 9 ++++-----
> > 1 file changed, 4 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/iio/accel/adxl345_core.c b/drivers/iio/accel/adxl345_core.c
> > index 8bd30a23e..be6758015 100644
> > --- a/drivers/iio/accel/adxl345_core.c
> > +++ b/drivers/iio/accel/adxl345_core.c
> > @@ -42,13 +42,13 @@
> > #define ADXL345_DATA_FORMAT_4G 1
> > #define ADXL345_DATA_FORMAT_8G 2
> > #define ADXL345_DATA_FORMAT_16G 3
> > +#define ADXL345_DATA_FORMAT_MSK ~((u8) BIT(6)) /* ignore spi-3wire */
>
> I'm not keen on seeing masking of a bit we don't yet
> handle done by value. Can we instead build this up by what we 'want' to
> write rather than don't. Will need a few more defines perhaps to cover
> the masks of SELF_TEST, INT_INVERT, FULL_RES, Justify and Range.
>
Good point. Anyway, there is also an input driver implementation for
the adxl345, mainly dealing with the interrupt feature as input
device. Thus, for the iio implementation I would suggest to reduce the
mask just to cover SELF_TEST and FULL_RES and leave INT_INVERT out. Is
this ok?
> >
> > #define ADXL345_DEVID 0xE5
> >
> > struct adxl345_data {
> > const struct adxl345_chip_info *info;
> > struct regmap *regmap;
> > - u8 data_range;
> > };
> >
> > #define ADXL345_CHANNEL(index, axis) { \
> > @@ -219,14 +219,13 @@ int adxl345_core_probe(struct device *dev, struct regmap *regmap)
> > data = iio_priv(indio_dev);
> > data->regmap = regmap;
> > /* Enable full-resolution mode */
> > - data->data_range = ADXL345_DATA_FORMAT_FULL_RES;
> > data->info = device_get_match_data(dev);
> > if (!data->info)
> > return -ENODEV;
> >
> > - ret = regmap_write(data->regmap, ADXL345_REG_DATA_FORMAT,
> > - data->data_range);
> > - if (ret < 0)
> > + ret = regmap_update_bits(regmap, ADXL345_REG_DATA_FORMAT,
> > + ADXL345_DATA_FORMAT_MSK, ADXL345_DATA_FORMAT_FULL_RES);
> > + if (ret)
> > return dev_err_probe(dev, ret, "Failed to set data range\n");
> >
> > indio_dev->name = data->info->name;
>
On 26/03/2024 21:17, Lothar Rubusch wrote:
>>>
>>> V1: The first version of the 3wire patch. I have split the single
>>> patch upon some feedback (yours?!) - V2... So, my current
>>> interpretation is, that every feedback I need to mention as
>>> Reviewed-by tag, no?
>>
>> What? Feedback is not review. It's clearly explained in submitting
>> patches. Please read it.
>>
>
> Exactly. My missunderstanding here is this: Why did you send me a
> reminder that I forgot to add "Reviewed-by" tag in your last mail?
> Could you please clarify your last mail? You wrote:
> "(...)
> This is a friendly reminder during the review process.
>
> It looks like you received a tag and forgot to add it.
>
> If you do not know the process, here is a short explanation:
> Please add Acked-by/Reviewed-by/Tested-by tags when posting new
> versions, (...)"
>
> AFAIK noone literally had told me: "please add a Reviewed-by me tag",
> or did I miss something? I'm a bit lost here, sorry.
>
What was this then:
https://lore.kernel.org/all/[email protected]/
Best regards,
Krzysztof
On Tue, 26 Mar 2024 21:59:34 +0100
Lothar Rubusch <[email protected]> wrote:
> On Mon, Mar 25, 2024 at 9:32 PM Jonathan Cameron <[email protected]> wrote:
> >
> > On Mon, 25 Mar 2024 15:33:50 +0000
> > Lothar Rubusch <[email protected]> wrote:
> >
> > > Replace write() data_format by regmap_update_bits(), because
> > > bus specific pre-configuration may have happened before on
> > > the same register. Changes then need to be masked.
> > >
> > > Remove the data_range field from the struct adxl345_data,
> > > because it is not used anymore.
> > >
> > > Signed-off-by: Lothar Rubusch <[email protected]>
> > > ---
> > > drivers/iio/accel/adxl345_core.c | 9 ++++-----
> > > 1 file changed, 4 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/drivers/iio/accel/adxl345_core.c b/drivers/iio/accel/adxl345_core.c
> > > index 8bd30a23e..be6758015 100644
> > > --- a/drivers/iio/accel/adxl345_core.c
> > > +++ b/drivers/iio/accel/adxl345_core.c
> > > @@ -42,13 +42,13 @@
> > > #define ADXL345_DATA_FORMAT_4G 1
> > > #define ADXL345_DATA_FORMAT_8G 2
> > > #define ADXL345_DATA_FORMAT_16G 3
> > > +#define ADXL345_DATA_FORMAT_MSK ~((u8) BIT(6)) /* ignore spi-3wire */
> >
> > I'm not keen on seeing masking of a bit we don't yet
> > handle done by value. Can we instead build this up by what we 'want' to
> > write rather than don't. Will need a few more defines perhaps to cover
> > the masks of SELF_TEST, INT_INVERT, FULL_RES, Justify and Range.
> >
>
> Good point. Anyway, there is also an input driver implementation for
> the adxl345, mainly dealing with the interrupt feature as input
> device. Thus, for the iio implementation I would suggest to reduce the
> mask just to cover SELF_TEST and FULL_RES and leave INT_INVERT out. Is
> this ok?
yes, that sounds fine
>
> > >
> > > #define ADXL345_DEVID 0xE5
> > >
> > > struct adxl345_data {
> > > const struct adxl345_chip_info *info;
> > > struct regmap *regmap;
> > > - u8 data_range;
> > > };
> > >
> > > #define ADXL345_CHANNEL(index, axis) { \
> > > @@ -219,14 +219,13 @@ int adxl345_core_probe(struct device *dev, struct regmap *regmap)
> > > data = iio_priv(indio_dev);
> > > data->regmap = regmap;
> > > /* Enable full-resolution mode */
> > > - data->data_range = ADXL345_DATA_FORMAT_FULL_RES;
> > > data->info = device_get_match_data(dev);
> > > if (!data->info)
> > > return -ENODEV;
> > >
> > > - ret = regmap_write(data->regmap, ADXL345_REG_DATA_FORMAT,
> > > - data->data_range);
> > > - if (ret < 0)
> > > + ret = regmap_update_bits(regmap, ADXL345_REG_DATA_FORMAT,
> > > + ADXL345_DATA_FORMAT_MSK, ADXL345_DATA_FORMAT_FULL_RES);
> > > + if (ret)
> > > return dev_err_probe(dev, ret, "Failed to set data range\n");
> > >
> > > indio_dev->name = data->info->name;
> >