linux-kernel - Re: add mcp4728 I2C DAC driver

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <1504c4eb-9def-dd75-ceb0-bb6c32fb78f2@kernel.org>
Date:   Tue, 11 Jul 2023 09:33:18 +0200
From:   Krzysztof Kozlowski <krzk@...nel.org>
To:     Andrea Collamati <andrea.collamati@...il.com>,
        Jonathan Cameron <jic23@...nel.org>,
        Lars-Peter Clausen <lars@...afoo.de>,
        Rob Herring <robh+dt@...nel.org>
Cc:     linux-iio@...r.kernel.org, devicetree@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: add mcp4728 I2C DAC driver

Thank you for your patch. There is something to discuss/improve.

On 10/07/2023 22:43, Andrea Collamati wrote:
> From 01b156ca1b27be83f4c74c288dbc0bcad178fe0b Mon Sep 17 00:00:00 2001
> From: Andrea Collamati <andrea.collamati@...il.com>
> Date: Mon, 10 Jul 2023 16:20:40 +0200
> Subject: [PATCH] iio: add mcp4728 I2C DAC driver

1. That is not a proper patch header. I don't know how you got it, but
it's wrong. Use just b4 or git format-patch and git send-email.

2. Please use scripts/get_maintainers.pl to get a list of necessary
people and lists to CC (and consider --no-git-fallback argument). It
might happen, that command when run on an older kernel, gives you
outdated entries. Therefore please be sure you base your patches on
recent Linux kernel.


> 
> Microchip MCP4728 is a 12-bit quad channel
> digital-to-analog converter (DAC) with I2C interface.
> 
> This patch adds support for per-channel gain, power state and power down mode control.

3. Please do not use "This commit/patch", but imperative mood. See
longer explanation here:
https://elixir.bootlin.com/linux/v5.17.1/source/Documentation/process/submitting-patches.rst#L95

> Current state could be saved to on-chip EEPROM.
> Internal voltage reference and external vdd ref are supported.
> 
> ---
>  .../bindings/iio/dac/microchip,mcp4728.yaml   |  42 ++

4. Bindings are always separate patches.

5. Please run scripts/checkpatch.pl and fix reported warnings. Some
warnings can be ignored, but the code here looks like it needs a fix.
Feel free to get in touch if the warning is not clear.


>  drivers/iio/dac/Kconfig                       |  12 +
>  drivers/iio/dac/Makefile                      |   1 +
>  drivers/iio/dac/mcp4728.c                     | 641 ++++++++++++++++++
>  4 files changed, 696 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/iio/dac/microchip,mcp4728.yaml
>  create mode 100644 drivers/iio/dac/mcp4728.c
> 
> diff --git a/Documentation/devicetree/bindings/iio/dac/microchip,mcp4728.yaml b/Documentation/devicetree/bindings/iio/dac/microchip,mcp4728.yaml
> new file mode 100644
> index 000000000000..68f4e359a921
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/dac/microchip,mcp4728.yaml
> @@ -0,0 +1,42 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/iio/dac/microchip,mcp4728.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Microchip mcp4728

6. mcp or MCP? What is this? Proper title is missing...  also no
description.

> +
> +maintainers:
> +  - Andrea Collamati <andrea.collamati@...il.com>
> +
> +properties:
> +  compatible:
> +    enum:
> +      - microchip,mcp4728      

7. Blank line
8. Whitespace errors

> +  reg:
> +    maxItems: 1
> +
> +  vdd-supply:
> +    description: |
> +      Provides both power and acts as the reference supply on the mcp4728
> +      when Internal Vref is not selected.      
> +
> +required:
> +  - compatible
> +  - reg
> +  - vdd-supply
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    i2c {
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +
> +        mcp4728_dac@64 {

9. Node names should be generic. See also an explanation and list of
examples (not exhaustive) in DT specification:
https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation

Also, underscores are not allowed in node names.

Shouldn't this binding be just merged with existing mcp4725? Are you
sure it's not similar device, IOW, are you sure you do not have vref supply?


> +            compatible = "microchip,mcp4728";
> +            reg = <0x60>;
> +            vdd-supply = <&vdac_vdd>;
> +        };
> +    };

...

> +
> +static int mcp4728_probe(struct i2c_client *client,
> +             const struct i2c_device_id *id)
> +{
> +    struct mcp4728_data *data;
> +    struct iio_dev *indio_dev;
> +    int err;
> +
> +    indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*data));
> +    if (indio_dev == NULL)
> +        return -ENOMEM;

Missing blank line

> +    data = iio_priv(indio_dev);
> +    i2c_set_clientdata(client, indio_dev);
> +    data->client = client;
> +    if (dev_fwnode(&client->dev))
> +        data->id = (uintptr_t)device_get_match_data(&client->dev);
> +    else
> +        data->id = id->driver_data;

Dead code, drop.

> +
> +    data->vdd_reg = devm_regulator_get(&client->dev, "vdd");
> +    if (IS_ERR(data->vdd_reg))
> +        return PTR_ERR(data->vdd_reg);
> +
> +    err = regulator_enable(data->vdd_reg);
> +    if (err)
> +        goto err_disable_vdd_reg;
> +
> +    err = mcp4728_init_channels_data(data);
> +    if (err) {
> +        dev_err(&client->dev,
> +            "failed to read mcp4728 current configuration\n");

None of your statements look properly aligned. What's more everything
has incorrect indentation.

Run checkpatch --strict and fix ALL warnings and errors.

> +        goto err_disable_vdd_reg;
> +    }
> +
> +    indio_dev->name = id->name;
> +    indio_dev->info = &mcp4728_info;
> +    indio_dev->channels = mcp4728_channels;
> +    indio_dev->num_channels = MCP4728_N_CHANNELS;
> +    indio_dev->modes = INDIO_DIRECT_MODE;
> +
> +    err = iio_device_register(indio_dev);
> +    if (err)
> +        goto err_disable_vdd_reg;
> +
> +    return 0;
> +
> +err_disable_vdd_reg:
> +    regulator_disable(data->vdd_reg);
> +
> +    return err;
> +}
> +
> +static int mcp4728_remove(struct i2c_client *client)
> +{
> +    struct iio_dev *indio_dev = i2c_get_clientdata(client);
> +    struct mcp4728_data *data = iio_priv(indio_dev);
> +
> +    iio_device_unregister(indio_dev);
> +    regulator_disable(data->vdd_reg);
> +    return 0;
> +}
> +
> +static const struct i2c_device_id mcp4728_id[] = { { "mcp4728", MCP4728 }, {} };

That's some odd formatting. Look at existing files:
git grep i2c_device_id

> +MODULE_DEVICE_TABLE(i2c, mcp4728_id);
> +
> +static const struct of_device_id mcp4728_of_match[] = {
> +    { .compatible = "microchip,mcp4728", .data = (void *)MCP4728 },

Drop unused MCP4728.

> +    {}
> +};
> +MODULE_DEVICE_TABLE(of, mcp4728_of_match);
> +
> +static struct i2c_driver mcp4728_driver = {
> +        .driver = {
> +                .name = MCP4728_DRV_NAME,
> +                .of_match_table = mcp4728_of_match,
> +                .pm = pm_sleep_ptr(&mcp4728_pm_ops),
> +        },
> +        .probe = mcp4728_probe,
> +        .remove = mcp4728_remove,
> +        .id_table = mcp4728_id,
> +};
> +module_i2c_driver(mcp4728_driver);
> +
> +MODULE_AUTHOR("Andrea Collamati <andrea.collamati@...il.com>");
> +MODULE_DESCRIPTION("MCP4728 12-bit DAC");
> +MODULE_LICENSE("GPL");

Best regards,
Krzysztof