Re: [PATCH v2 2/2] iio: add mcp4728 I2C DAC driver

[Krzysztof Kozlowski <krzk@kernel.org>]

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@gmail.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