linux-kernel - Re: [PATCH v2 2/2] iio: 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] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 17 Jul 2023 08:38:00 +0200
From:   Krzysztof Kozlowski <krzk@...nel.org>
To:     Andrea Collamati <andrea.collamati@...il.com>
Cc:     Jonathan Cameron <jic23@...nel.org>,
        Rob Herring <robh+dt@...nel.org>,
        Conor Dooley <conor+dt@...nel.org>,
        William Breathitt Gray <william.gray@...aro.org>,
        Angelo Dureghello <angelo.dureghello@...esys.com>,
        Fabio Estevam <festevam@...il.com>,
        Lukas Bulwahn <lukas.bulwahn@...il.com>,
        "linux-iio@...r.kernel.org" <linux-iio@...r.kernel.org>,
        "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2 2/2] iio: add mcp4728 I2C DAC driver

On 16/07/2023 23:26, Andrea Collamati wrote:
> mcp4728 is a 12-bit quad channel DAC with I2C interface.
> 
> support for:
>   * per-channel gain
>   * per-channel power state
>   * per-channel power down mode control
>   * per-channel vref selection internal/vdd
>   * store current state to on-chip EEPROM
> 
> Signed-off-by: Andrea Collamati <andrea.collamati@...il.com>
> ---

What changed? Are you saying you ignored entire review you got?

>  drivers/iio/dac/Kconfig   |  12 +
>  drivers/iio/dac/Makefile  |   1 +
>  drivers/iio/dac/mcp4728.c | 635 ++++++++++++++++++++++++++++++++++++++
>  3 files changed, 648 insertions(+)
>  create mode 100644 drivers/iio/dac/mcp4728.c
> 
> diff --git a/drivers/iio/dac/Kconfig b/drivers/iio/dac/Kconfig
> index 3acd9c3f388e..fa1516f6a285 100644
> --- a/drivers/iio/dac/Kconfig
> +++ b/drivers/iio/dac/Kconfig
> @@ -389,6 +389,18 @@ config MCP4725
>  	  To compile this driver as a module, choose M here: the module
>  	  will be called mcp4725.
>  
> +config MCP4728
> +	tristate "MCP4728 DAC driver"
> +	depends on I2C
> +	help
> +	  Say Y here if you want to build a driver for the Microchip
> +	  MCP4728 quad channel, 12-bit digital-to-analog converter (DAC)
> +	  with I2C interface.
> +
> +	  To compile this driver as a module, choose M here: the module
> +	  will be called mcp4728.
> +
> +

Why two blank lines?

>  config MCP4922
>  	tristate "MCP4902, MCP4912, MCP4922 DAC driver"
>  	depends on SPI
> diff --git a/drivers/iio/dac/Makefile b/drivers/iio/dac/Makefile
> index addd97a78838..5b2bac900d5a 100644

...

> +
> +static void 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);
> +}
> +
> +static const struct i2c_device_id mcp4728_id[] = { { "mcp4728", MCP4728 }, {} };
> +MODULE_DEVICE_TABLE(i2c, mcp4728_id);

Yeah, my feedback was ignored.

That's not how it works. Anyway, I doubt that it should be a new driver.

If Jonathan agrees to have new/duplicated drivers, then fine with me,
but then don't ignore the comments. Instead:

It seems my previous comments were not fully addressed. Maybe my
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.

Best regards,
Krzysztof