diff mbox series

[v6,1/6] dt-bindings: iio: adc: mcp3911: add support for the whole MCP39xx family

Message ID 20230817120518.153728-1-marcus.folkesson@gmail.com
State Not Applicable, archived
Headers show
Series [v6,1/6] dt-bindings: iio: adc: mcp3911: add support for the whole MCP39xx family | expand

Checks

Context Check Description
robh/checkpatch warning total: 0 errors, 1 warnings, 13 lines checked
robh/patch-applied success
robh/dtbs-check warning build log
robh/dt-meta-schema success

Commit Message

Marcus Folkesson Aug. 17, 2023, 12:05 p.m. UTC 
Microchip does have many similar chips, add those to the compatible
string as the driver support is extended.

The new supported chips are:
  - microchip,mcp3910
  - microchip,mcp3912
  - microchip,mcp3913
  - microchip,mcp3914
  - microchip,mcp3918
  - microchip,mcp3919

Signed-off-by: Marcus Folkesson <marcus.folkesson@gmail.com>
Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
---

Notes:
    v2:
        - No changes
    v3:
        - No changes
    v4:
        - No changes
    v5:
        - No changes
    v6:
        - No changes

 .../devicetree/bindings/iio/adc/microchip,mcp3911.yaml      | 6 ++++++
 1 file changed, 6 insertions(+)

Comments

Andy Shevchenko Aug. 17, 2023, 12:42 p.m. UTC | #1 
On Thu, Aug 17, 2023 at 02:05:13PM +0200, Marcus Folkesson wrote:
> Microchip does have many similar chips, add those to the compatible
> string as the driver support is extended.

With properly formed cover letter (and using --base) a maintainer may safe
their time by utilizing some features of `b4`. I.e. the

	b4 -slt -M ...

will treat the series as a PR with summary in cover letter taking into merge
commit. This is really cool feature and from now on I will require people to
submit the series with mandatory cover-letter and --base in use.

BUT. I'm not the maintainer here, so up to Jonathan to decide.
Andy Shevchenko Aug. 17, 2023, 12:42 p.m. UTC | #2 
On Thu, Aug 17, 2023 at 03:42:21PM +0300, Andy Shevchenko wrote:
> On Thu, Aug 17, 2023 at 02:05:13PM +0200, Marcus Folkesson wrote:
> > Microchip does have many similar chips, add those to the compatible
> > string as the driver support is extended.
> 
> With properly formed cover letter (and using --base) a maintainer may safe
> their time by utilizing some features of `b4`. I.e. the
> 
> 	b4 -slt -M ...

	b4 shazam -slt -M ...

> will treat the series as a PR with summary in cover letter taking into merge
> commit. This is really cool feature and from now on I will require people to
> submit the series with mandatory cover-letter and --base in use.
> 
> BUT. I'm not the maintainer here, so up to Jonathan to decide.
Andy Shevchenko Aug. 17, 2023, 12:48 p.m. UTC | #3 
On Thu, Aug 17, 2023 at 02:05:15PM +0200, Marcus Folkesson wrote:
> Replace the usage of `adc->spi->dev` with `dev` to make the code prettier.

...

> @@ -277,9 +278,7 @@ static int mcp3911_calc_scale_table(struct mcp3911 *adc)

>  		ret = regulator_get_voltage(adc->vref);
>  		if (ret < 0) {
> -			dev_err(&adc->spi->dev,
> -				"failed to get vref voltage: %d\n",
> -			       ret);
> +			dev_err(dev, "failed to get vref voltage: %d\n", ret);

So, this can be part of the previous patch as this function is the part of the
->probe() stage.

>  			return ret;
>  		}

...

> @@ -396,12 +395,10 @@ static int mcp3911_config(struct mcp3911 *adc)
>  	if (ret)
>  		device_property_read_u32(dev, "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",
> -			adc->dev_addr);
> +		dev_err(dev, "invalid device address (%i). Must be in range 0-3.\n", adc->dev_addr);
>  		return -EINVAL;

Ditto.

>  	}

...

> +		adc->trig = devm_iio_trigger_alloc(dev, "%s-dev%d",
> +						   indio_dev->name,
> +						   iio_device_id(indio_dev));

Seem you already switched to longer lines, hence this can be compressed
to two lines.

>  		if (!adc->trig)
>  			return -ENOMEM;

...

> +		ret = devm_request_irq(dev, spi->irq,
> +				       &iio_trigger_generic_data_rdy_poll,

In the similar way as above.

> +				       IRQF_NO_AUTOEN | IRQF_ONESHOT,
> +				       indio_dev->name, adc->trig);
>  		if (ret)
>  			return ret;
Andy Shevchenko Aug. 17, 2023, 12:48 p.m. UTC | #4 
On Thu, Aug 17, 2023 at 02:05:16PM +0200, Marcus Folkesson wrote:
> The whole file does not make use of indentation properly.
> Do something about it.

Okay,
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Andy Shevchenko Aug. 17, 2023, 12:49 p.m. UTC | #5 
On Thu, Aug 17, 2023 at 02:05:17PM +0200, Marcus Folkesson wrote:
> Name macro parameters after what they represent instead of 'x'.

"And make sure the evaluation of that will have no side effects."

With that (or similar) added,
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Andy Shevchenko Aug. 17, 2023, 12:58 p.m. UTC | #6 
On Thu, Aug 17, 2023 at 02:05:18PM +0200, Marcus Folkesson wrote:
> Microchip does have many similar chips, add support for those.
> 
> The new supported chips are:
>   - microchip,mcp3910
>   - microchip,mcp3912
>   - microchip,mcp3913
>   - microchip,mcp3914
>   - microchip,mcp3918
>   - microchip,mcp3919

...

> +static int mcp3910_set_offset(struct mcp3911 *adc, int channel, int val)
> +{

	unsigned int mask = MCP3910_CONFIG0_EN_OFFCAL;

> +	int ret;
> +
> +	/* Write offset */
> +	ret = mcp3911_write(adc, MCP3910_OFFCAL(channel), val, 3);
> +	if (ret)
> +		return ret;
> +
> +	/* Enable offset */
> +	return mcp3911_update(adc, MCP3910_REG_CONFIG0,
> +			      MCP3910_CONFIG0_EN_OFFCAL,
> +			      MCP3910_CONFIG0_EN_OFFCAL, 3);

	return mcp3911_update(adc, MCP3910_REG_CONFIG0, mask, mask, 3);

> +}

...

> +static int mcp3911_set_offset(struct mcp3911 *adc, int channel, int val)
> +{

As per above.

> +}

...

> +static int mcp3910_set_osr(struct mcp3911 *adc, u32 val)
> +{

	unsigned int mask = MCP3910_CONFIG0_OSR;

> +	int osr = FIELD_PREP(MCP3910_CONFIG0_OSR, val);
> +
> +	return mcp3911_update(adc, MCP3910_REG_CONFIG0,
> +			      MCP3910_CONFIG0_OSR, osr, 3);

	return mcp3911_update(adc, MCP3910_REG_CONFIG0, mask, osr, 3);

> +}

...

> +static int mcp3911_set_osr(struct mcp3911 *adc, u32 val)

In the similar way.

...

> +static int mcp3910_set_scale(struct mcp3911 *adc, int channel, u32 val)
> +{
> +	return mcp3911_update(adc, MCP3910_REG_GAIN,
> +			      MCP3911_GAIN_MASK(channel),
> +			      MCP3911_GAIN_VAL(channel, val), 3);
> +}
> +
> +static int mcp3911_set_scale(struct mcp3911 *adc, int channel, u32 val)
> +{
> +	return mcp3911_update(adc, MCP3911_REG_GAIN,
> +			      MCP3911_GAIN_MASK(channel),
> +			      MCP3911_GAIN_VAL(channel, val), 1);
> +}

These can be also converted, but I don't see much difference
(same LoC amount, similar readability).

...

> +	/* Disable offset to ignore any old values in offset register */
> +	return mcp3911_update(adc, MCP3910_REG_CONFIG0,
> +			      MCP3910_CONFIG0_EN_OFFCAL,
> +			      MCP3910_CONFIG0_EN_OFFCAL, 3);

This is a dup code with some of mcp3910_set_offset(). Perhaps a helper?
Jonathan Cameron Aug. 27, 2023, 6:24 p.m. UTC | #7 
On Thu, 17 Aug 2023 15:42:20 +0300
Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:

> On Thu, Aug 17, 2023 at 02:05:13PM +0200, Marcus Folkesson wrote:
> > Microchip does have many similar chips, add those to the compatible
> > string as the driver support is extended.  
> 
> With properly formed cover letter (and using --base) a maintainer may safe
> their time by utilizing some features of `b4`. I.e. the
> 
> 	b4 -slt -M ...
> 
> will treat the series as a PR with summary in cover letter taking into merge
> commit. This is really cool feature and from now on I will require people to
> submit the series with mandatory cover-letter and --base in use.
> 
> BUT. I'm not the maintainer here, so up to Jonathan to decide.
> 
Not today, but I'll keep the option in mind.  Thanks for highlighting it.

Jonathan
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/iio/adc/microchip,mcp3911.yaml b/Documentation/devicetree/bindings/iio/adc/microchip,mcp3911.yaml
index f7b3fde4115a..06951ec5f5da 100644
--- a/Documentation/devicetree/bindings/iio/adc/microchip,mcp3911.yaml
+++ b/Documentation/devicetree/bindings/iio/adc/microchip,mcp3911.yaml
@@ -18,7 +18,13 @@  description: |
 properties:
   compatible:
     enum:
+      - microchip,mcp3910
       - microchip,mcp3911
+      - microchip,mcp3912
+      - microchip,mcp3913
+      - microchip,mcp3914
+      - microchip,mcp3918
+      - microchip,mcp3919
 
   reg:
     maxItems: 1