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

[Andrea Collamati <andrea.collamati@gmail.com>]

On 7/17/23 08:38, Krzysztof Kozlowski wrote:
> 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?
I didn't ignored, but I didn't list the changes. I will look at other emails to understand how to write v2 v3 ... changes..Sorry.
>> 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);

Sorry.. I fixed but then I code formatter replace the old one. I will pay more attention..

besides clang-format and checkpatch are there any other tools to avoid these formatting errors?

Thank you

> 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.
Ok.
> Best regards,
> Krzysztof
>