iio: adc: mcp3911: correct "microchip,device-addr" property
Subsystems (3)
Marcus Folkesson
maintainerPATCH 1/10 iio: adc: mcp3911: correct "microchip,device-addr" property
Go for the right property name that is documented in the bindings.
--- drivers/iio/adc/mcp3911.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/iio/adc/mcp3911.c +1 -1
@@ -208,7 +208,7 @@ static int mcp3911_config(struct mcp3911 *adc)u32 configreg;int ret; -device_property_read_u32 (dev,"device-addr" , &adc->dev_addr); +device_property_read_u32 (dev,"microchip,device-addr" , &adc->dev_addr);if (adc->dev_addr >3 ) { dev_err(&adc->spi->dev, "invalid device address (%i). Must be in range 0-3.\n",
Signature · 1 line
2.36.1
Marcus Folkesson
maintainerPATCH 2/10 iio: adc: mcp3911: use resource-managed version of iio_device_register
Keep using managed resources as much as possible.
--- drivers/iio/adc/mcp3911.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-)
drivers/iio/adc/mcp3911.c +1 -3
@@ -306,7 +306,7 @@ static int mcp3911_probe(struct spi_device *spi)mutex_init (&adc->lock); - ret =iio_device_register (indio_dev); + ret =devm_iio_device_register (&adc->spi->dev, indio_dev);if (ret) goto clk_disable;
@@ -326,8 +326,6 @@ static void mcp3911_remove(struct spi_device *spi)struct iio_dev *indio_dev =spi_get_drvdata (spi);struct mcp3911 *adc =iio_priv (indio_dev); -iio_device_unregister (indio_dev); -clk_disable_unprepare (adc->clki);if (adc->vref)regulator_disable (adc->vref);
Signature · 1 line
2.36.1
Re: [PATCH 02/10] iio: adc: mcp3911: use resource-managed version of iio_device_register
Quoted 2 lines · Marcus Folkesson
Keep using managed resources as much as possible.
You may not mix devm_ and non-devm_ API calls like this.
So, you rule of thumb that goto is most of the time wrong after devm_ call.
Signature · 2 lines
With Best Regards, Andy Shevchenko
Marcus Folkesson
maintainerRe: [PATCH 02/10] iio: adc: mcp3911: use resource-managed version of iio_device_register
Thank you for your comments (all of them) Andy!
Quoted 7 lines · Andy Shevchenko
Quoted 2 lines · Marcus Folkesson
Keep using managed resources as much as possible.
You may not mix
devm_and non-devm_ API calls like this.
So, you rule of thumb that goto is most of the time wrong afterdevm_call.
Can you please confirm that clocks and regulators are disabled when the resources are handed back?
I cannot see where when I'm trying to follow the code.
Best regards
Marcus Folkesson
Jonathan Cameron
maintainerRe: [PATCH 02/10] iio: adc: mcp3911: use resource-managed version of iio_device_register
Quoted 6 lines · On Fri, 24 Jun 2022 08:29:20 +0200 Marcus Folkesson…
Thank you for your comments (all of them) Andy!
Quoted 3 lines · Andy Shevchenko
Quoted 1 line · Marcus Folkesson
Quoted 8 lines
Quoted 4 lines
Quoted 1 line
Keep using managed resources as much as possible.
You may not mix
devm_and non-devm_ API calls like this.
So, you rule of thumb that goto is most of the time wrong afterdevm_call.Can you please confirm that clocks and regulators are disabled when the resources are handed back?
I cannot see where when I'm trying to follow the code.
Andy isn't arguing that the goto is wrong but rather that you cannot in general safely use devm_* calls if their failure leads to having to do any cleanup. The reason is the ordering is hard to reason about. Sometimes it's safe, but often enough causes problems that we basically refuse to think hard enough to figure out if it is. Hence basic rule is don't do it.
The issue is this.probe() {
non_devm_call_1(); ret = devm_call_2() if (ret) goto err; return 0; err: unwind_non_devm_call_1() }
remove() {
unwind_non_devm_call_1()
}remove or error path should unwind in opposite order of what happens in probe. On the rare occasion where that isn't the right choice, there should be very clear comments to say why.
Order is
remove() -> unwind_non_devm_call_1()devm_managed_cleanup() -> unwind_devm_call_2()
Whereas should be
remove()-> unwind_call_2() then unwind_call_1()
There are two ways to solve this. Either only use devm for those elements in probe() that happen before the first thing you need to unwind manually or make everything devm managed (it unwinds in reverse order of setup) devm_add_action_or_reset() allows you to use your own devm_ managed callbacks if there isn't a standard one available.
Jonathan
Quoted 3 lines
Best regards
Marcus Folkesson
Re: [PATCH 02/10] iio: adc: mcp3911: use resource-managed version of iio_device_register
Quoted 52 lines · Jonathan Cameron
Quoted 6 lines · On Fri, 24 Jun 2022 08:29:20 +0200 Marcus Folkesson…
Thank you for your comments (all of them) Andy!
Quoted 3 lines · Andy Shevchenko
Quoted 1 line · Marcus Folkesson
Quoted 8 lines
Quoted 4 lines
Quoted 1 line
Keep using managed resources as much as possible.
You may not mix
devm_and non-devm_ API calls like this.
So, you rule of thumb that goto is most of the time wrong afterdevm_call.Can you please confirm that clocks and regulators are disabled when the resources are handed back?
I cannot see where when I'm trying to follow the code.Andy isn't arguing that the goto is wrong but rather that you cannot in general safely use devm_* calls if their failure leads to having to do any cleanup. The reason is the ordering is hard to reason about. Sometimes it's safe, but often enough causes problems that we basically refuse to think hard enough to figure out if it is. Hence basic rule is don't do it.
The issue is this.
probe(){non_devm_call_1(); ret = devm_call_2() if (ret) goto err; return 0; err: unwind_non_devm_call_1() }remove() { unwind_non_devm_call_1() }remove or error path should unwind in opposite order of what happens in probe. On the rare occasion where that isn't the right choice, there should be very clear comments to say why.
Order is
remove()->unwind_non_devm_call_1()devm_managed_cleanup()->unwind_devm_call_2()Whereas should be
Quoted 8 lines
remove()->
unwind_call_2()thenunwind_call_1()There are two ways to solve this. Either only use devm for those elements in
probe()that happen before the first thing you need to unwind manually or make everything devm managed (it unwinds in reverse order of setup)devm_add_action_or_reset()allows you to use your owndevm_managed callbacks if there isn't a standard one available.
Thanks, Jonathan, that's exactly what I meant!
Signature · 2 lines
With Best Regards, Andy Shevchenko
Marcus Folkesson
maintainerPATCH 3/10 iio: adc: mcp3911: add support for buffers
In reply to Marcus FolkessonAdd support for buffers to make the driver fit for more usecases.
--- drivers/iio/adc/mcp3911.c | 58 ++++++++++++++++++++++++++++++++++++++- 1 file changed, 57 insertions(+), 1 deletion(-)
drivers/iio/adc/mcp3911.c +57 -1
@@ -9,6 +9,10 @@#include <linux/delay.h> #include <linux/err.h> #include <linux/iio/iio.h> +#include <linux/iio/buffer.h> +#include <linux/iio/triggered_buffer.h> +#include <linux/iio/trigger_consumer.h> +#include <linux/iio/trigger.h> #include <linux/module.h> #include <linux/mod_devicetable.h> #include <linux/property.h>
@@ -54,6 +58,10 @@ struct mcp3911 {struct regulator *vref;struct clk *clki;u32 dev_addr; +struct { +u32 channels[2 ]; +s64 ts__aligned(8) ; + } scan; };static int mcp3911_read (struct mcp3911 *adc,u8 reg,u32 *val,u8 len)
@@ -187,16 +195,58 @@ static int mcp3911_write_raw(struct iio_dev *indio_dev, .type = IIO_VOLTAGE, \ .indexed = 1, \ .channel = idx, \ + .scan_index = idx, \ .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | \ BIT(IIO_CHAN_INFO_OFFSET) | \ BIT(IIO_CHAN_INFO_SCALE), \ + .scan_type = { \ + .sign = 's', \ + .realbits = 24, \ + .storagebits = 32, \ + }, \ }static const struct iio_chan_spec mcp3911_channels[] = {MCP3911_CHAN (0 ),MCP3911_CHAN (1 ), +IIO_CHAN_SOFT_TIMESTAMP (2 ), }; +static irqreturn_t mcp3911_trigger_handler (int irq,void *p) +{ +struct iio_poll_func *pf = p; +struct iio_dev *indio_dev = pf->indio_dev; +struct mcp3911 *adc =iio_priv (indio_dev); +int scan_index; +int i =0 ; +u32 val; + +mutex_lock (&adc->lock); +for_each_set_bit (scan_index, indio_dev->active_scan_mask, + indio_dev->masklength) { +const struct iio_chan_spec *scan_chan = + &indio_dev->channels[scan_index]; +int ret =mcp3911_read (adc, +MCP3911_CHANNEL (scan_chan->channel), &val,3 ); + +if (ret <0 ) { +dev_warn (&adc->spi->dev, +"failed to get conversion data\n" ); + goto out; + } + + adc->scan.channels[i] = val; + i++; + } +iio_push_to_buffers_with_timestamp (indio_dev, &adc->scan, +iio_get_time_ns (indio_dev)); +out: +mutex_unlock (&adc->lock); +iio_trigger_notify_done (indio_dev->trig); + +return IRQ_HANDLED ; +} +static const struct iio_info mcp3911_info = { .read_raw = mcp3911_read_raw, .write_raw = mcp3911_write_raw,
@@ -297,7 +347,7 @@ static int mcp3911_probe(struct spi_device *spi) goto clk_disable; indio_dev->name =spi_get_device_id (spi)->name; - indio_dev->modes =INDIO_DIRECT_MODE ; + indio_dev->modes =INDIO_DIRECT_MODE |INDIO_BUFFER_TRIGGERED ; indio_dev->info = &mcp3911_info;spi_set_drvdata (spi, indio_dev);
@@ -306,6 +356,12 @@ static int mcp3911_probe(struct spi_device *spi)mutex_init (&adc->lock); + ret =devm_iio_triggered_buffer_setup (&spi->dev, indio_dev, +NULL , + mcp3911_trigger_handler,NULL ); +if (ret) + goto clk_disable; + ret =devm_iio_device_register (&adc->spi->dev, indio_dev);if (ret) goto clk_disable;
Signature · 1 line
2.36.1
Marcus Folkesson
maintainerPATCH 4/10 iio: adc: mcp3911: add support for interrupts
In reply to Marcus FolkessonMake it possible to read values upon interrupts.
Configure Data Ready Signal Output Pin to either HiZ or push-pull and use it as interrupt source.
--- drivers/iio/adc/mcp3911.c | 69 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 69 insertions(+)
drivers/iio/adc/mcp3911.c +69 -0
@@ -26,6 +26,7 @@#define MCP3911_REG_GAIN 0x09#define MCP3911_REG_STATUSCOM 0x0a +#define MCP3911_STATUSCOM_DRHIZ BIT(12)#define MCP3911_STATUSCOM_CH1_24WIDTH BIT(4)#define MCP3911_STATUSCOM_CH0_24WIDTH BIT(3)#define MCP3911_STATUSCOM_EN_OFFCAL BIT(2)
@@ -58,6 +59,7 @@ struct mcp3911 {struct regulator *vref;struct clk *clki;u32 dev_addr; +struct iio_trigger *trig;struct {u32 channels[2 ];s64 ts__aligned(8) ;
@@ -252,6 +254,17 @@ static const struct iio_info mcp3911_info = { .write_raw = mcp3911_write_raw, }; +static irqreturn_t mcp3911_interrupt (int irq,void *dev_id) +{ +struct iio_dev *indio_dev = dev_id; +struct mcp3911 *adc =iio_priv (indio_dev); + +if (iio_buffer_enabled (indio_dev)) +iio_trigger_poll (adc->trig); + +return IRQ_HANDLED ; +}; +static int mcp3911_config (struct mcp3911 *adc) {struct device *dev = &adc->spi->dev;
@@ -292,11 +305,30 @@ static int mcp3911_config(struct mcp3911 *adc)return mcp3911_write (adc,MCP3911_REG_CONFIG , configreg,2 ); } + +static int mcp3911_set_trigger_state (struct iio_trigger *trig,bool enable) +{ +struct mcp3911 *adc =iio_trigger_get_drvdata (trig); + +if (enable) +enable_irq (adc->spi->irq); +else +disable_irq (adc->spi->irq); + +return 0 ; +} + +static const struct iio_trigger_ops mcp3911_trigger_ops = { + .validate_device = iio_trigger_validate_own_device, + .set_trigger_state = mcp3911_set_trigger_state, +}; +static int mcp3911_probe (struct spi_device *spi) {struct iio_dev *indio_dev;struct mcp3911 *adc;int ret; +bool dr_hiz; indio_dev =devm_iio_device_alloc (&spi->dev,sizeof (*adc));if (!indio_dev)
@@ -346,6 +378,17 @@ static int mcp3911_probe(struct spi_device *spi)if (ret) goto clk_disable; + dr_hiz =device_property_read_bool (&adc->spi->dev,"microchip,data-ready-hiz" ); +if (dr_hiz) + ret =mcp3911_update (adc,MCP3911_REG_STATUSCOM ,MCP3911_STATUSCOM_DRHIZ , +0 ,2 ); +else + ret =mcp3911_update (adc,MCP3911_REG_STATUSCOM ,MCP3911_STATUSCOM_DRHIZ , +MCP3911_STATUSCOM_DRHIZ ,2 ); + +if (ret <0 ) + goto clk_disable; + indio_dev->name =spi_get_device_id (spi)->name; indio_dev->modes =INDIO_DIRECT_MODE |INDIO_BUFFER_TRIGGERED ; indio_dev->info = &mcp3911_info;
@@ -356,6 +399,32 @@ static int mcp3911_probe(struct spi_device *spi)mutex_init (&adc->lock); +if (spi->irq >0 ) { + adc->trig =devm_iio_trigger_alloc (&spi->dev,"%s-dev%d" , + indio_dev->name, +iio_device_id (indio_dev)); +if (!adc->trig) + goto clk_disable; + + adc->trig->ops = &mcp3911_trigger_ops; +iio_trigger_set_drvdata (adc->trig, adc); + ret =devm_iio_trigger_register (&spi->dev, adc->trig); +if (ret) + goto clk_disable; + +/* +* The device generates interrupts as long as it is powered up. +* Some platforms might not allow the option to power it down so +* don't enable the interrupt to avoid extra load on the system +*/ + ret =devm_request_irq (&spi->dev, spi->irq, + &mcp3911_interrupt, +IRQF_TRIGGER_FALLING |IRQF_NO_AUTOEN |IRQF_ONESHOT , + indio_dev->name, indio_dev); +if (ret) + goto clk_disable; + } + ret =devm_iio_triggered_buffer_setup (&spi->dev, indio_dev,NULL , mcp3911_trigger_handler,NULL );
Signature · 1 line
2.36.1
Re: [PATCH 04/10] iio: adc: mcp3911: add support for interrupts
Quoted 4 lines · Marcus Folkesson
Make it possible to read values upon interrupts.
Configure Data Ready Signal Output Pin to either HiZ or push-pull and use it as interrupt source.
...
Quoted 3 lines
}
+
Unwanted blank line.
drivers/iio/adc/mcp3911.c static int mcp3911_config(struct mcp3911 *adc)
Quoted 4 lines
drivers/iio/adc/mcp3911.c +3
-0+static int mcp3911_set_trigger_state (struct iio_trigger *trig,bool enable) +{ +bool dr_hiz;
As far as I can see you don't need this variable, just call if (device_property_...(...)).
drivers/iio/adc/mcp3911.c static int mcp3911_probe(struct spi_device *spi) · line 388
Quoted 9 lines
drivers/iio/adc/mcp3911.c +8
-0+ dr_hiz =device_property_read_bool (&adc->spi->dev,"microchip,data-ready-hiz" ); +if (dr_hiz) + ret =mcp3911_update (adc,MCP3911_REG_STATUSCOM ,MCP3911_STATUSCOM_DRHIZ , +0 ,2 ); +else + ret =mcp3911_update (adc,MCP3911_REG_STATUSCOM ,MCP3911_STATUSCOM_DRHIZ , +MCP3911_STATUSCOM_DRHIZ ,2 ); +
Unneeded blank line.
Quoted 2 lines
drivers/iio/adc/mcp3911.c +2
-0+if (ret <0 ) + goto clk_disable;
Signature · 2 lines
With Best Regards, Andy Shevchenko
Marcus Folkesson
maintainerRe: [PATCH 04/10] iio: adc: mcp3911: add support for interrupts
Quoted 2 lines · Andy Shevchenko
Unwanted blank line.
Removed
Quoted 7 lines
drivers/iio/adc/mcp3911.c static int mcp3911_config(struct mcp3911 *adc)
Quoted 4 lines
drivers/iio/adc/mcp3911.c +3
-0+static int mcp3911_set_trigger_state (struct iio_trigger *trig,bool enable) +{ +bool dr_hiz;As far as I can see you don't need this variable, just call if (device_property_...(...)).
Fixed
Quoted 2 lines
Unneeded blank line.
Removed
Thanks,
Marcus Folkesson
Marcus Folkesson
maintainerPATCH 5/10 dt-bindings: iio: adc: mcp3911: add microchip,data-ready-hiz entry
In reply to Marcus FolkessonThe Data Ready Output Pin is either hard wired to work as high impedance or push-pull. Make it configurable.
--- .../devicetree/bindings/iio/adc/microchip,mcp3911.yaml | 8 ++++++++ 1 file changed, 8 insertions(+)
Documentation/devicetree/bindings/iio/adc/microchip,mcp3911.yaml +8 -0
@@ -36,6 +36,14 @@ properties: description: IRQ line of the ADC maxItems: 1 + microchip,data-ready-hiz: + description: + Data Ready Pin Inactive State Control bit + true = The DR pin state is high-impedance when data are NOT ready + false = The DR pin state is a logic high when data are NOT ready + type: boolean + default: false + microchip,device-addr: description: Device address when multiple MCP3911 chips are present on the same SPI bus. $ref: /schemas/types.yaml#/definitions/uint32
Signature · 1 line
2.36.1
Re: [PATCH 05/10] dt-bindings: iio: adc: mcp3911: add microchip,data-ready-hiz entry
Quoted 13 lines · Marcus Folkesson
The Data Ready Output Pin is either hard wired to work as high impedance or push-pull. Make it configurable.
Signed-off-by Marcus Folkesson <[email protected]>--- .../devicetree/bindings/iio/adc/microchip,mcp3911.yaml | 8 ++++++++ 1 file changed, 8 insertions(+)Documentation/devicetree/bindings/iio/adc/microchip,mcp3911.yaml +0
-0
Documentation/devicetree/bindings/iio/adc/microchip,mcp3911.yaml properties: · line 41
Quoted 6 lines
Documentation/devicetree/bindings/iio/adc/microchip,mcp3911.yaml +3
-0@@ -36,6 +36,14 @@ properties: description: IRQ line of the ADC maxItems: 1 + microchip,data-ready-hiz: + description: + Data Ready Pin Inactive State Control bit
"Bit" of what? Do not describe the programming model but the actual feature.
Documentation/devicetree/bindings/iio/adc/microchip,mcp3911.yaml properties: · line 45
Quoted 4 lines
Documentation/devicetree/bindings/iio/adc/microchip,mcp3911.yaml +4
-0+ true = The DR pin state is high-impedance when data are NOT ready + false = The DR pin state is a logic high when data are NOT ready + type: boolean + default: false
You do not need default for bools.
Documentation/devicetree/bindings/iio/adc/microchip,mcp3911.yaml properties: · line 49
Quoted 4 lines
Documentation/devicetree/bindings/iio/adc/microchip,mcp3911.yaml +1
-0+ microchip,device-addr: description: Device address when multiple MCP3911 chips are present on the same SPI bus. $ref: /schemas/types.yaml#/definitions/uint32
Best regards,
Krzysztof
Marcus Folkesson
maintainerRe: [PATCH 05/10] dt-bindings: iio: adc: mcp3911: add microchip,data-ready-hiz entry
Quoted 6 lines · Krzysztof Kozlowski
Documentation/devicetree/bindings/iio/adc/microchip,mcp3911.yaml properties: · line 41
Quoted 3 lines
Documentation/devicetree/bindings/iio/adc/microchip,mcp3911.yaml +3
-0+ microchip,data-ready-hiz: + description: + Data Ready Pin Inactive State Control bit"Bit" of what? Do not describe the programming model but the actual feature.
You are right. I will remove "bit".
Quoted 6 lines
Documentation/devicetree/bindings/iio/adc/microchip,mcp3911.yaml properties: · line 45
Quoted 4 lines
Documentation/devicetree/bindings/iio/adc/microchip,mcp3911.yaml +4
-0+ true = The DR pin state is high-impedance when data are NOT ready + false = The DR pin state is a logic high when data are NOT ready + type: boolean + default: falseYou do not need default for bools.
OK
Quoted 9 lines
Quoted 5 lines
+
microchip,device-addr:
description: Device address when multiple MCP3911 chips are present on the same SPI bus. $ref: /schemas/types.yaml#/definitions/uint32Best regards,
Krzysztof
Thanks,
Marcus Folkesson
Rob Herring
maintainerRe: [PATCH 05/10] dt-bindings: iio: adc: mcp3911: add microchip,data-ready-hiz entry
In reply to Marcus FolkessonQuoted 8 lines · Marcus Folkesson
The Data Ready Output Pin is either hard wired to work as high impedance or push-pull. Make it configurable.
Signed-off-by Marcus Folkesson <[email protected]>--- .../devicetree/bindings/iio/adc/microchip,mcp3911.yaml | 8 ++++++++ 1 file changed, 8 insertions(+)
My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check' on your patch (DT_CHECKER_FLAGS is new in v5.13):
yamllint warnings/errors:
dtschema/dtc warnings/errors:
/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/iio/adc/microchip,mcp3911.yaml: properties:microchip,data-ready-hiz: 'oneOf' conditional failed, one must be fixed:
Additional properties are not allowed ('default' was unexpected)
hint: A vendor boolean property can use "type: boolean"
/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/iio/adc/microchip,mcp3911.yaml: properties:microchip,data-ready-hiz: 'oneOf' conditional failed, one must be fixed:
'enum' is a required property
'const' is a required property
hint: A vendor string property with exact values has an implicit type
from schema $id: http://devicetree.org/meta-schemas/vendor-props.yaml#
Additional properties are not allowed ('default', 'type' were unexpected)
hint: A vendor string property with exact values has an implicit type
/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/iio/adc/microchip,mcp3911.yaml: properties:microchip,data-ready-hiz: 'oneOf' conditional failed, one must be fixed:
'$ref' is a required property
'allOf' is a required property
hint: A vendor property needs a $ref to types.yaml
from schema $id: http://devicetree.org/meta-schemas/vendor-props.yaml#
hint: Vendor specific properties must have a type and description unless they have a defined, common suffix.
from schema $id: http://devicetree.org/meta-schemas/vendor-props.yaml#
/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/iio/adc/microchip,mcp3911.yaml: ignoring, error in schema: properties: microchip,data-ready-hiz Documentation/devicetree/bindings/iio/adc/microchip,mcp3911.example.dtb:0:0: /example-0/spi/adc@0: failed to match any schema with compatible: ['microchip,mcp3911']
doc reference errors (make refcheckdocs):
See https://patchwork.ozlabs.org/patch/
This check can fail if there are any dependencies. The base for a patch series is generally the most recent rc1.
If you already ran 'make dt_binding_check' and didn't see the above error(s), then make sure 'yamllint' is installed and dt-schema is up to date:
pip3 install dtschema --upgrade
Please check and re-submit.
Marcus Folkesson
maintainerPATCH 6/10 iio: adc: mcp3911: add support for oversampling ratio
In reply to Marcus FolkessonThe chip support oversampling ratio so expose it to userspace.
--- drivers/iio/adc/mcp3911.c | 72 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 72 insertions(+)
drivers/iio/adc/mcp3911.c +72 -0
@@ -5,6 +5,8 @@* Copyright (C) 2018 Marcus Folkesson <[email protected]> * Copyright (C) 2018 Kent Gustavsson <[email protected]> */ +#include <linux/bitfield.h> +#include <linux/bits.h> #include <linux/clk.h> #include <linux/delay.h> #include <linux/err.h>
@@ -35,6 +37,7 @@#define MCP3911_REG_CONFIG 0x0c#define MCP3911_CONFIG_CLKEXT BIT(1)#define MCP3911_CONFIG_VREFEXT BIT(2) +#define MCP3911_CONFIG_OSR GENMASK(13, 11)#define MCP3911_REG_OFFCAL_CH0 0x0e#define MCP3911_REG_GAINCAL_CH0 0x11
@@ -53,6 +56,8 @@#define MCP3911_NUM_CHANNELS 2 +static const int mcp3911_osr_table[] = {32 ,64 ,128 ,256 ,512 ,1024 ,2048 ,4096 }; +struct mcp3911 {struct spi_device *spi;struct mutex lock;
@@ -108,6 +113,22 @@ static int mcp3911_update(struct mcp3911 *adc, u8 reg, u32 mask,return mcp3911_write (adc, reg, val, len); } + +static int mcp3911_read_avail (struct iio_dev *indio_dev, +struct iio_chan_spec const *chan, +const int **vals,int *type,int *length, +long info) +{ +switch (info) { +case IIO_CHAN_INFO_OVERSAMPLING_RATIO : + *type =IIO_VAL_INT ; + *vals = mcp3911_osr_table; + *length =ARRAY_SIZE (mcp3911_osr_table); +return IIO_AVAIL_LIST ; +default : +return -EINVAL ; + } +}static int mcp3911_read_raw (struct iio_dev *indio_dev,struct iio_chan_spec const *channel,int *val,int *val2,long mask)
@@ -134,6 +155,16 @@ static int mcp3911_read_raw(struct iio_dev *indio_dev, ret =IIO_VAL_INT ;break ; +case IIO_CHAN_INFO_OVERSAMPLING_RATIO : + ret =mcp3911_read (adc, +MCP3911_REG_CONFIG , val,2 ); +if (ret) + goto out; + + *val =FIELD_GET (MCP3911_CONFIG_OSR , *val); + *val =32 << *val; + ret =IIO_VAL_INT ; +break ;case IIO_CHAN_INFO_SCALE :if (adc->vref) {
@@ -186,6 +217,42 @@ static int mcp3911_write_raw(struct iio_dev *indio_dev,MCP3911_STATUSCOM_EN_OFFCAL , MCP3911_STATUSCOM_EN_OFFCAL, 2);break ; + +case IIO_CHAN_INFO_OVERSAMPLING_RATIO : +switch (val) { +case 4096 : + val =FIELD_PREP (MCP3911_CONFIG_OSR ,0x07 ); +break ; +case 2048 : + val =FIELD_PREP (MCP3911_CONFIG_OSR ,0x06 ); +break ; +case 1024 : + val =FIELD_PREP (MCP3911_CONFIG_OSR ,0x05 ); +break ; +case 512 : + val =FIELD_PREP (MCP3911_CONFIG_OSR ,0x04 ); +break ; +case 256 : + val =FIELD_PREP (MCP3911_CONFIG_OSR ,0x03 ); +break ; +case 128 : + val =FIELD_PREP (MCP3911_CONFIG_OSR ,0x02 ); +break ; +case 64 : + val =FIELD_PREP (MCP3911_CONFIG_OSR ,0x01 ); +break ; +case 32 : + val =FIELD_PREP (MCP3911_CONFIG_OSR ,0x00 ); +break ; +default : + ret = -EINVAL ; + goto out; + } + + ret =mcp3911_update (adc,MCP3911_REG_CONFIG , +MCP3911_CONFIG_OSR , + val,2 ); +break ; } out:
@@ -198,9 +265,13 @@ static int mcp3911_write_raw(struct iio_dev *indio_dev, .indexed = 1, \ .channel = idx, \ .scan_index = idx, \ + .scan_index = idx, \ + .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO), \ .info_mask_separate =BIT (IIO_CHAN_INFO_RAW ) | \BIT (IIO_CHAN_INFO_OFFSET ) | \ BIT(IIO_CHAN_INFO_SCALE), \ + .info_mask_shared_by_type_available = \ + BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO), \ .scan_type = { \ .sign = 's', \ .realbits = 24, \
@@ -252,6 +323,7 @@ static irqreturn_t mcp3911_trigger_handler(int irq, void *p)static const struct iio_info mcp3911_info = { .read_raw = mcp3911_read_raw, .write_raw = mcp3911_write_raw, + .read_avail = mcp3911_read_avail, };static irqreturn_t mcp3911_interrupt (int irq,void *dev_id)
Signature · 1 line
2.36.1
Re: [PATCH 06/10] iio: adc: mcp3911: add support for oversampling ratio
Quoted 2 lines · Marcus Folkesson
The chip support oversampling ratio so expose it to userspace.
supports
ratio, so
...
Quoted 1 line
+static const int mcp3911_osr_table[] = {32, 64, 128, 256, 512, 1024, 2048, 4096};
Spaces inside {}.
...
Quoted 3 lines
}
+
Unwanted blank line.
...
Quoted 19 lines
drivers/iio/adc/mcp3911.c +19
-0+switch (val) { +case 4096 : + val =FIELD_PREP (MCP3911_CONFIG_OSR ,0x07 ); +break ; +case 2048 : + val =FIELD_PREP (MCP3911_CONFIG_OSR ,0x06 ); +break ; +case 1024 : + val =FIELD_PREP (MCP3911_CONFIG_OSR ,0x05 ); +break ; +case 512 : + val =FIELD_PREP (MCP3911_CONFIG_OSR ,0x04 ); +break ; +case 256 : + val =FIELD_PREP (MCP3911_CONFIG_OSR ,0x03 ); +break ; +case 128 : + val =FIELD_PREP (MCP3911_CONFIG_OSR ,0x02 ); +break ;
drivers/iio/adc/mcp3911.c static int mcp3911_write_raw(struct iio_dev *indio_dev, · line 250
Quoted 10 lines
drivers/iio/adc/mcp3911.c +10
-0+case 64 : + val =FIELD_PREP (MCP3911_CONFIG_OSR ,0x01 ); +break ; +case 32 : + val =FIELD_PREP (MCP3911_CONFIG_OSR ,0x00 ); +break ; +default : + ret = -EINVAL ; + goto out; + }
I understood why the table above, but this is a waste of resources. Use that table
...
drivers/iio/adc/mcp3911.c static int mcp3911_write_raw(struct iio_dev *indio_dev, · line 254
Quoted 3 lines
drivers/iio/adc/mcp3911.c +3
-0+ ret =mcp3911_update (adc,MCP3911_REG_CONFIG , +MCP3911_CONFIG_OSR , + val,2 );
sizeof() ?
Signature · 2 lines
With Best Regards, Andy Shevchenko
Marcus Folkesson
maintainerRe: [PATCH 06/10] iio: adc: mcp3911: add support for oversampling ratio
Quoted 10 lines · Andy Shevchenko
Quoted 2 lines · Marcus Folkesson
The chip support oversampling ratio so expose it to userspace.
supports
ratio, so
...
Thanks
Quoted 4 lines
Quoted 2 lines
+static const int mcp3911_osr_table[] = {32, 64, 128, 256, 512, 1024, 2048, 4096};
Spaces inside {}.
Not sure what you mean?
Quoted 10 lines
...
Quoted 3 lines
}
+
Unwanted blank line.
...
Removed
Quoted 31 lines
Quoted 20 lines
drivers/iio/adc/mcp3911.c +19
-0+switch (val) { +case 4096 : + val =FIELD_PREP (MCP3911_CONFIG_OSR ,0x07 ); +break ; +case 2048 : + val =FIELD_PREP (MCP3911_CONFIG_OSR ,0x06 ); +break ; +case 1024 : + val =FIELD_PREP (MCP3911_CONFIG_OSR ,0x05 ); +break ; +case 512 : + val =FIELD_PREP (MCP3911_CONFIG_OSR ,0x04 ); +break ; +case 256 : + val =FIELD_PREP (MCP3911_CONFIG_OSR ,0x03 ); +break ; +case 128 : + val =FIELD_PREP (MCP3911_CONFIG_OSR ,0x02 ); +break ;drivers/iio/adc/mcp3911.c static int mcp3911_write_raw(struct iio_dev *indio_dev, · line 250
Quoted 10 lines
drivers/iio/adc/mcp3911.c +10
-0+case 64 : + val =FIELD_PREP (MCP3911_CONFIG_OSR ,0x01 ); +break ; +case 32 : + val =FIELD_PREP (MCP3911_CONFIG_OSR ,0x00 ); +break ; +default : + ret = -EINVAL ; + goto out; + }
Quoted 2 lines
I understood why the table above, but this is a waste of resources. Use that table
Yep, I will use the table instead
Quoted 8 lines
...
drivers/iio/adc/mcp3911.c static int mcp3911_write_raw(struct iio_dev *indio_dev, · line 254
Quoted 3 lines
drivers/iio/adc/mcp3911.c +3
-0+ ret =mcp3911_update (adc,MCP3911_REG_CONFIG , +MCP3911_CONFIG_OSR , + val,2 );
sizeof()?
sizeof() what?
Thanks,
Marcus Folkesson
Re: [PATCH 06/10] iio: adc: mcp3911: add support for oversampling ratio
Quoted 3 lines · Marcus Folkesson
Quoted 2 lines · Andy Shevchenko
On Thu, Jun 23, 2022 at 9:08 PM Marcus Folkesson
<[email protected]> wrote:
...
Quoted 5 lines
Quoted 3 lines
Quoted 1 line
+static const int mcp3911_osr_table[] = {32, 64, 128, 256, 512, 1024, 2048, 4096};
Spaces inside {}.
Not sure what you mean?
int foo = { 1, 2, 3 };...
Quoted 7 lines
Quoted 5 lines
drivers/iio/adc/mcp3911.c static int mcp3911_write_raw(struct iio_dev *indio_dev, · line 254
Quoted 3 lines
drivers/iio/adc/mcp3911.c +3
-0+ ret =mcp3911_update (adc,MCP3911_REG_CONFIG , +MCP3911_CONFIG_OSR , + val,2 );
sizeof()?
sizeof()what?
sizeof(val) ?
Signature · 2 lines
With Best Regards, Andy Shevchenko
Marcus Folkesson
maintainerPATCH 7/10 iio: adc: mcp3911: use correct formula for AD conversion
In reply to Marcus FolkessonThe ADC conversion is actually not rail-to-rail but with a factor 1.5. Make use of this factor when calculating actual voltage.
--- drivers/iio/adc/mcp3911.c | 17 ++++++++++++----- 1 file changed, 12 insertions(+), 5 deletions(-)
drivers/iio/adc/mcp3911.c +12 -5
@@ -48,8 +48,8 @@#define MCP3911_CHANNEL (x) (MCP3911_REG_CHANNEL0 + x * 3)#define MCP3911_OFFCAL (x) (MCP3911_REG_OFFCAL_CH0 + x * 6) -/* Internal voltage reference in uV */ -#define MCP3911_INT_VREF_UV 1200000 +/* Internal voltage reference in mV */ +#define MCP3911_INT_VREF_MV 1200#define MCP3911_REG_READ (reg, id) ((((reg) << 1) | ((id) << 5) | (1 << 0)) & 0xff)#define MCP3911_REG_WRITE (reg, id) ((((reg) << 1) | ((id) << 5) | (0 << 0)) & 0xff)
@@ -178,11 +178,18 @@ static int mcp3911_read_raw(struct iio_dev *indio_dev, *val = ret /1000 ; }else { - *val =MCP3911_INT_VREF_UV ; + *val =MCP3911_INT_VREF_MV ; } - *val2 =24 ; - ret =IIO_VAL_FRACTIONAL_LOG2 ; +/* +* For 24bit Conversion +* Raw = ((Voltage)/(Vref) * 2^23 * Gain * 1.5 +* Voltage = Raw * (Vref)/(2^23 * Gain * 1.5) +*/ + +/* val2 = (2^23 * 1.5) */ + *val2 =12582912 ; + ret =IIO_VAL_FRACTIONAL ;break ; }
Signature · 1 line
2.36.1
Marcus Folkesson
maintainerPATCH 8/10 iio: adc: mcp3911: add support for phase
In reply to Marcus FolkessonThe MCP3911 incorporates a phase delay generator, which ensures that the two ADCs are converting the inputs with a fixed delay between them. Expose it to userspace.
--- drivers/iio/adc/mcp3911.c | 25 ++++++++++++++++++++++++- 1 file changed, 24 insertions(+), 1 deletion(-)
drivers/iio/adc/mcp3911.c +24 -1
@@ -155,6 +155,17 @@ static int mcp3911_read_raw(struct iio_dev *indio_dev, ret =IIO_VAL_INT ;break ; + +case IIO_CHAN_INFO_PHASE : + ret =mcp3911_read (adc, +MCP3911_REG_PHASE , val,2 ); +if (ret) + goto out; + + *val =sign_extend32 (*val,12 ); + ret =IIO_VAL_INT ; +break ; +case IIO_CHAN_INFO_OVERSAMPLING_RATIO : ret =mcp3911_read (adc,MCP3911_REG_CONFIG , val,2 );
@@ -225,6 +236,16 @@ static int mcp3911_write_raw(struct iio_dev *indio_dev, MCP3911_STATUSCOM_EN_OFFCAL, 2);break ; +case IIO_CHAN_INFO_PHASE : +if (val2 !=0 || val >0xfff ) { + ret = -EINVAL ; + goto out; + } + +/* Write phase */ + ret =mcp3911_write (adc,MCP3911_REG_PHASE , val, +2 ); +break ;case IIO_CHAN_INFO_OVERSAMPLING_RATIO :switch (val) {case 4096 :
@@ -273,7 +294,9 @@ static int mcp3911_write_raw(struct iio_dev *indio_dev, .channel = idx, \ .scan_index = idx, \ .scan_index = idx, \ - .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO), \ + .info_mask_shared_by_type = \ +BIT (IIO_CHAN_INFO_OVERSAMPLING_RATIO )| \ + BIT(IIO_CHAN_INFO_PHASE), \ .info_mask_separate =BIT (IIO_CHAN_INFO_RAW ) | \BIT (IIO_CHAN_INFO_OFFSET ) | \ BIT(IIO_CHAN_INFO_SCALE), \
Signature · 1 line
2.36.1
Re: [PATCH 08/10] iio: adc: mcp3911: add support for phase
Quoted 5 lines · Marcus Folkesson
The MCP3911 incorporates a phase delay generator, which ensures that the two ADCs are converting the inputs with a fixed delay between them. Expose it to userspace.
...
Quoted 8 lines
drivers/iio/adc/mcp3911.c +2
-0+case IIO_CHAN_INFO_PHASE : +if (val2 !=0 || val >0xfff ) {= BIT(12) will show better the intention
drivers/iio/adc/mcp3911.c +3
-0+ ret = -EINVAL ; + goto out; + }
...
drivers/iio/adc/mcp3911.c static int mcp3911_write_raw(struct iio_dev *indio_dev, · line 247
Quoted 2 lines
drivers/iio/adc/mcp3911.c +2
-0+ ret =mcp3911_write (adc,MCP3911_REG_PHASE , val, +2 );
One line.
Signature · 2 lines
With Best Regards, Andy Shevchenko
Marcus Folkesson
maintainerRe: [PATCH 08/10] iio: adc: mcp3911: add support for phase
Quoted 4 lines · Andy Shevchenko
drivers/iio/adc/mcp3911.c static int mcp3911_write_raw(struct iio_dev *indio_dev, · line 247
Quoted 2 lines
drivers/iio/adc/mcp3911.c +2
-0+ ret =mcp3911_write (adc,MCP3911_REG_PHASE , val, +2 );One line.
Fixed
Quoted 4 lines
Signature · 3 lines
-- With Best Regards, Andy Shevchenko
Thanks,
Marcus Folkesson
Jonathan Cameron
maintainerRe: [PATCH 08/10] iio: adc: mcp3911: add support for phase
Quoted 7 lines · On Fri, 24 Jun 2022 14:02:22 +0200 Marcus Folkesson…
Quoted 4 lines · Andy Shevchenko
drivers/iio/adc/mcp3911.c static int mcp3911_write_raw(struct iio_dev *indio_dev, · line 247
Quoted 2 lines
drivers/iio/adc/mcp3911.c +2
-0+ ret =mcp3911_write (adc,MCP3911_REG_PHASE , val, +2 );One line.
Fixed
Hi Marcus,
Grumpy maintainer moment: If you've accepted feedback don't send yet more email. Just have it incorporated in next version + in changes logs. It's on a few seconds for people to open each email, but it adds up over time!
Jonathan
Quoted 9 lines
Quoted 5 lines
Signature · 3 lines
-- With Best Regards, Andy ShevchenkoThanks,
Marcus Folkesson
Marcus Folkesson
maintainerPATCH 9/10 iio: adc: mcp3911: make use of the sign bit
In reply to Marcus FolkessonThe device supports negative values as well.
--- drivers/iio/adc/mcp3911.c | 2 ++ 1 file changed, 2 insertions(+)
drivers/iio/adc/mcp3911.c +2 -0
@@ -144,6 +144,8 @@ static int mcp3911_read_raw(struct iio_dev *indio_dev,if (ret) goto out; + *val =sign_extend32 (*val,23 ); + ret =IIO_VAL_INT ;break ;
Signature · 1 line
2.36.1
Marcus Folkesson
maintainerPATCH 10/10 iio: adc: mcp3911: add support to set PGA
In reply to Marcus FolkessonAdd support for setting the Programmable Gain Amplifiers by adjust the scale value.
--- drivers/iio/adc/mcp3911.c | 135 +++++++++++++++++++++++++++++++------- 1 file changed, 110 insertions(+), 25 deletions(-)
drivers/iio/adc/mcp3911.c +110 -25
@@ -26,6 +26,8 @@#define MCP3911_REG_MOD 0x06#define MCP3911_REG_PHASE 0x07#define MCP3911_REG_GAIN 0x09 +#define MCP3911_GAIN_MASK (ch) (0x7 << 3*ch) +#define MCP3911_GAIN_VAL (ch, val) ((val << 3*ch) & MCP3911_GAIN_MASK(ch))#define MCP3911_REG_STATUSCOM 0x0a#define MCP3911_STATUSCOM_DRHIZ BIT(12)
@@ -55,8 +57,10 @@#define MCP3911_REG_WRITE (reg, id) ((((reg) << 1) | ((id) << 5) | (0 << 0)) & 0xff)#define MCP3911_NUM_CHANNELS 2 +#define MCP3911_NUM_SCALES 6static const int mcp3911_osr_table[] = {32 ,64 ,128 ,256 ,512 ,1024 ,2048 ,4096 }; +static u32 mcp3911_scale_table[MCP3911_NUM_SCALES ][2 ];struct mcp3911 {struct spi_device *spi;
@@ -65,6 +69,7 @@ struct mcp3911 {struct clk *clki;u32 dev_addr;struct iio_trigger *trig; +u32 gain[MCP3911_NUM_CHANNELS ];struct {u32 channels[2 ];s64 ts__aligned(8) ;
@@ -113,7 +118,35 @@ static int mcp3911_update(struct mcp3911 *adc, u8 reg, u32 mask,return mcp3911_write (adc, reg, val, len); } +static int mcp3911_get_gain (struct mcp3911 *adc,u8 channel,u32 *val) +{ +int ret =mcp3911_read (adc,MCP3911_REG_GAIN , val,1 ); + +if (ret) +return ret; + *val >>= channel *3 ; + *val &=0x07 ; + *val = (1 << *val); + +return 0 ; +} + +static int mcp3911_write_raw_get_fmt (struct iio_dev *indio_dev, +struct iio_chan_spec const *chan, +long mask) +{ +switch (mask) { +case IIO_CHAN_INFO_SCALE : +return IIO_VAL_INT_PLUS_NANO ; +case IIO_CHAN_INFO_OVERSAMPLING_RATIO : +return IIO_VAL_INT ; +case IIO_CHAN_INFO_PHASE : +return IIO_VAL_INT ; +default : +return IIO_VAL_INT_PLUS_NANO ; + } +} static int mcp3911_read_avail(struct iio_dev *indio_dev, struct iio_chan_spec const *chan, const int **vals, int *type, int *length,
@@ -125,6 +158,11 @@ static int mcp3911_read_avail(struct iio_dev *indio_dev, *vals = mcp3911_osr_table; *length =ARRAY_SIZE (mcp3911_osr_table);return IIO_AVAIL_LIST ; +case IIO_CHAN_INFO_SCALE : + *type =IIO_VAL_INT_PLUS_NANO ; + *vals = (int *)mcp3911_scale_table; + *length =ARRAY_SIZE (mcp3911_scale_table) *2 ; +return IIO_AVAIL_LIST ;default :return -EINVAL ; }
@@ -180,29 +218,9 @@ static int mcp3911_read_raw(struct iio_dev *indio_dev,break ;case IIO_CHAN_INFO_SCALE : -if (adc->vref) { - ret =regulator_get_voltage (adc->vref); -if (ret <0 ) { -dev_err (indio_dev->dev.parent, -"failed to get vref voltage: %d\n" , - ret); - goto out; - } - - *val = ret /1000 ; - }else { - *val =MCP3911_INT_VREF_MV ; - } - -/* -* For 24bit Conversion -* Raw = ((Voltage)/(Vref) * 2^23 * Gain * 1.5 -* Voltage = Raw * (Vref)/(2^23 * Gain * 1.5) -*/ - -/* val2 = (2^23 * 1.5) */ - *val2 =12582912 ; - ret =IIO_VAL_FRACTIONAL ; + *val = mcp3911_scale_table[ilog2 (adc->gain[channel->channel])][0 ]; + *val2 = mcp3911_scale_table[ilog2 (adc->gain[channel->channel])][1 ]; + ret =IIO_VAL_INT_PLUS_NANO ;break ; }
@@ -220,6 +238,19 @@ static int mcp3911_write_raw(struct iio_dev *indio_dev,mutex_lock (&adc->lock);switch (mask) { +case IIO_CHAN_INFO_SCALE : +for (int i =0 ; i <MCP3911_NUM_SCALES ; i++) { +if (val == mcp3911_scale_table[i][0 ] && + val2 == mcp3911_scale_table[i][1 ]) { + + adc->gain[channel->channel] =BIT (i); + ret =mcp3911_update (adc,MCP3911_REG_GAIN , +MCP3911_GAIN_MASK (channel->channel), +MCP3911_GAIN_VAL (channel->channel, + i),1 ); + } + } +break ;case IIO_CHAN_INFO_OFFSET :if (val2 !=0 ) { ret = -EINVAL ;
@@ -290,6 +321,49 @@ static int mcp3911_write_raw(struct iio_dev *indio_dev,return ret; } +static int mcp3911_calc_scale_table (struct mcp3911 *adc) +{ +u32 ref =MCP3911_INT_VREF_MV ; +u32 div; +int ret =0 ; +int tmp0, tmp1; +s64 tmp2; + +if (adc->vref) { + ret =regulator_get_voltage (adc->vref); +if (ret <0 ) { +dev_err (&adc->spi->dev, +"failed to get vref voltage: %d\n" , + ret); + goto out; + } + + ref = ret /1000 ; + } + +/* +* For 24bit Conversion +* Raw = ((Voltage)/(Vref) * 2^23 * Gain * 1.5 +* Voltage = Raw * (Vref)/(2^23 * Gain * 1.5) +*/ + +/* ref = Reference voltage +* div = (2^23 * 1.5 * gain) = 12582912 * gain +*/ +for (int i =0 ; i <MCP3911_NUM_SCALES ; i++) { + div =12582912 *BIT (i); + tmp2 =div_s64 ((s64 )ref *1000000000LL , div); + tmp1 = div; + tmp0 = (int )div_s64_rem (tmp2,1000000000 , &tmp1); + + mcp3911_scale_table[i][0 ] =0 ; + mcp3911_scale_table[i][1 ] = tmp1; + } + +out: +return ret; +} +#define MCP3911_CHAN (idx) { \ .type = IIO_VOLTAGE, \ .indexed = 1, \
@@ -302,8 +376,10 @@ static int mcp3911_write_raw(struct iio_dev *indio_dev, .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | \BIT (IIO_CHAN_INFO_OFFSET ) | \ BIT(IIO_CHAN_INFO_SCALE), \ - .info_mask_shared_by_type_available = \ - BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO), \ + .info_mask_shared_by_type_available = \ +BIT (IIO_CHAN_INFO_OVERSAMPLING_RATIO ) \ + .info_mask_separate_available = \ + BIT(IIO_CHAN_INFO_SCALE), \ .scan_type = { \ .sign = 's', \ .realbits = 24, \
@@ -356,6 +432,7 @@ static const struct iio_info mcp3911_info = { .read_raw = mcp3911_read_raw, .write_raw = mcp3911_write_raw, .read_avail = mcp3911_read_avail, + .write_raw_get_fmt = mcp3911_write_raw_get_fmt, };static irqreturn_t mcp3911_interrupt (int irq,void *dev_id)
@@ -482,6 +559,14 @@ static int mcp3911_probe(struct spi_device *spi)if (ret) goto clk_disable; + ret =mcp3911_calc_scale_table (adc); +if (ret) + goto clk_disable; + +/* Store gain values to better calculate scale values */ +mcp3911_get_gain (adc,0 , &adc->gain[0 ]); +mcp3911_get_gain (adc,1 , &adc->gain[1 ]); + dr_hiz =device_property_read_bool (&adc->spi->dev,"microchip,data-ready-hiz" );if (dr_hiz) ret = mcp3911_update(adc, MCP3911_REG_STATUSCOM, MCP3911_STATUSCOM_DRHIZ,
Signature · 1 line
2.36.1
Marcus Folkesson
maintainerRe: [PATCH 10/10] iio: adc: mcp3911: add support to set PGA
drivers/iio/adc/mcp3911.c static int mcp3911_write_raw(struct iio_dev *indio_dev, · line 380
Quoted 4 lines · Marcus Folkesson
drivers/iio/adc/mcp3911.c +2
-2- .info_mask_shared_by_type_available = \ - BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO), \ + .info_mask_shared_by_type_available = \ +BIT (IIO_CHAN_INFO_OVERSAMPLING_RATIO ) \
This change was not supposed to sneak in. Removed in v2.
/Marcus
Re: [PATCH 10/10] iio: adc: mcp3911: add support to set PGA
In reply to Marcus FolkessonQuoted 3 lines · Marcus Folkesson
Add support for setting the Programmable Gain Amplifiers by adjust the scale value.
...
drivers/iio/adc/mcp3911.c static int mcp3911_update(struct mcp3911 *adc, u8 reg, u32 mask, · line 126
Quoted 4 lines
drivers/iio/adc/mcp3911.c +4
-0+int ret =mcp3911_read (adc,MCP3911_REG_GAIN , val,1 ); + +if (ret) +return ret;
Please split the assignment.
int ret;
ret = ...
if (ret)
...
drivers/iio/adc/mcp3911.c static int mcp3911_update(struct mcp3911 *adc, u8 reg, u32 mask, · line 129
Quoted 2 lines
drivers/iio/adc/mcp3911.c +2
-0+ *val >>= channel *3 ; + *val &=0x07 ;
GENMASK() ?
Quoted 1 line
+ *val = (1 << *val);
Unneeded parentheses, perhaps BIT()?
...
drivers/iio/adc/mcp3911.c static int mcp3911_write_raw(struct iio_dev *indio_dev, · line 250
Quoted 4 lines
drivers/iio/adc/mcp3911.c +4
-0+ ret =mcp3911_update (adc,MCP3911_REG_GAIN , +MCP3911_GAIN_MASK (channel->channel), +MCP3911_GAIN_VAL (channel->channel, + i),1 );
This is not good indentation, at least i), should be on the previous line.
...
drivers/iio/adc/mcp3911.c static int mcp3911_write_raw(struct iio_dev *indio_dev, · line 328
Quoted 6 lines
drivers/iio/adc/mcp3911.c +5
-0+static int mcp3911_calc_scale_table (struct mcp3911 *adc) +{ +u32 ref =MCP3911_INT_VREF_MV ; +u32 div; +int ret =0 ;
Useless assignment.
drivers/iio/adc/mcp3911.c static int mcp3911_write_raw(struct iio_dev *indio_dev, · line 338
Quoted 11 lines
drivers/iio/adc/mcp3911.c +10
-0+int tmp0, tmp1; +s64 tmp2; + +if (adc->vref) { + ret =regulator_get_voltage (adc->vref); +if (ret <0 ) { +dev_err (&adc->spi->dev, +"failed to get vref voltage: %d\n" , + ret); + goto out;
Return directly.
Quoted 15 lines
drivers/iio/adc/mcp3911.c +15
-0+ } + + ref = ret /1000 ; + } + +/* +* For 24bit Conversion +* Raw = ((Voltage)/(Vref) * 2^23 * Gain * 1.5 +* Voltage = Raw * (Vref)/(2^23 * Gain * 1.5) +*/ + +/* ref = Reference voltage +* div = (2^23 * 1.5 * gain) = 12582912 * gain +*/ +for (int i =0 ; i <MCP3911_NUM_SCALES ; i++) {
drivers/iio/adc/mcp3911.c static int mcp3911_write_raw(struct iio_dev *indio_dev, · line 363
Quoted 10 lines
drivers/iio/adc/mcp3911.c +9
-0+ div =12582912 *BIT (i); + tmp2 =div_s64 ((s64 )ref *1000000000LL , div); + tmp1 = div; + tmp0 = (int )div_s64_rem (tmp2,1000000000 , &tmp1); + + mcp3911_scale_table[i][0 ] =0 ; + mcp3911_scale_table[i][1 ] = tmp1; + } +out:
The useless label.
Quoted 1 line
+ return ret;
return 0;
Quoted 1 line
+}
Signature · 2 lines
With Best Regards, Andy Shevchenko
Marcus Folkesson
maintainerRe: [PATCH 10/10] iio: adc: mcp3911: add support to set PGA
Quoted 13 lines · Andy Shevchenko
Quoted 3 lines · Marcus Folkesson
Add support for setting the Programmable Gain Amplifiers by adjust the scale value.
...
drivers/iio/adc/mcp3911.c static int mcp3911_update(struct mcp3911 *adc, u8 reg, u32 mask, · line 126
Quoted 4 lines
drivers/iio/adc/mcp3911.c +4
-0+int ret =mcp3911_read (adc,MCP3911_REG_GAIN , val,1 ); + +if (ret) +return ret;
Quoted 6 lines
Please split the assignment.
int ret;
ret = ...
if (ret)
OK
Quoted 8 lines
...
drivers/iio/adc/mcp3911.c static int mcp3911_update(struct mcp3911 *adc, u8 reg, u32 mask, · line 129
Quoted 2 lines
drivers/iio/adc/mcp3911.c +2
-0+ *val >>= channel *3 ; + *val &=0x07 ;
GENMASK()?
I will use GENMASK
Quoted 4 lines
Quoted 2 lines
+ *val = (1 << *val);
Unneeded parentheses, perhaps
BIT()?
I will switch to BIT()
Quoted 9 lines
...
drivers/iio/adc/mcp3911.c static int mcp3911_write_raw(struct iio_dev *indio_dev, · line 250
Quoted 4 lines
drivers/iio/adc/mcp3911.c +4
-0+ ret =mcp3911_update (adc,MCP3911_REG_GAIN , +MCP3911_GAIN_MASK (channel->channel), +MCP3911_GAIN_VAL (channel->channel, + i),1 );This is not good indentation, at least i), should be on the previous line.
Agree
Quoted 11 lines
...
drivers/iio/adc/mcp3911.c static int mcp3911_write_raw(struct iio_dev *indio_dev, · line 328
Quoted 6 lines
drivers/iio/adc/mcp3911.c +5
-0+static int mcp3911_calc_scale_table (struct mcp3911 *adc) +{ +u32 ref =MCP3911_INT_VREF_MV ; +u32 div; +int ret =0 ;Useless assignment.
Agree
Quoted 3 lines
Return directly.
OK
Quoted 3 lines
The useless label.
Agree
Quoted 3 lines
Quoted 2 lines
+ return ret;
Thanks,
Marcus Folkesson
Quoted 2 lines · Marcus Folkesson
Go for the right property name that is documented in the bindings.
If the driver is already for a while in the kernel, I'm afraid we may not do this, since it's part of ABI (firmware <--> OS). You can add a new property and try it first.
Signature · 2 lines
With Best Regards, Andy Shevchenko