RE: [PATCH v3 3/7] power: supply: max77658: Add ADI MAX77654/58/59 Charger Support

[Arslanbenzer, Zeynep]

On Mon, 8 May 2023 , Krzysztof Kozlowski wrote:
>On 08/05/2023 15:10, Zeynep Arslanbenzer wrote:
>> Charger driver for ADI MAX77654/58/59.
>> 
>> The MAX77654/58/59 charger is Smart Power Selector Li+/Li-Poly Charger.
>> 
>> Signed-off-by: Nurettin Bolucu <Nurettin.Bolucu@xxxxxxxxxx>
>> Signed-off-by: Zeynep Arslanbenzer <Zeynep.Arslanbenzer@xxxxxxxxxx>
>> ---
>>  drivers/power/supply/Kconfig            |   7 +
>>  drivers/power/supply/Makefile           |   1 +
>>  drivers/power/supply/max77658-charger.c | 844 
>> ++++++++++++++++++++++++
>>  3 files changed, 852 insertions(+)
>>  create mode 100644 drivers/power/supply/max77658-charger.c
>> 
>
>Actually, with small differences (register map differs by few offsets) this is max77650 charger. Several fields are exactly the same.
>
>Please merge it with existing drivers.
>
>Best regards,
>Krzysztof

Since max77650 is similar to devices supported by this patch set,
I guess I should merge the regulator and mfd drivers too with the existing max77650 drivers.

As I observed from other device drivers, adding a new device driver support to an existing driver 
should not change the existing driver code too much. But as I want to add support for 4 extra devices to max77650 drivers,
It can cause changes to the already existing driver code. I just want to be sure before sending a new patch, sorry for the long explanation.

Would it be okay to change the existing code and make the code more generic to add new devices?

For example, the regulator max77650 driver was written for a single device and
the developer made the regulator definitions separately as follows.

static struct max77650_regulator_desc max77650_LDO_desc = {
	.desc = {
		.name = "ldo",

static struct max77650_regulator_desc max77650_SBB0_desc = {
	.desc = {
		.name = "sbb0",

static struct max77650_regulator_desc max77650_SBB1_desc = {
	.desc = {
		.name "sbb1",

I want to add support for 4 regulators devices. Each of them has multiple LDOs and SBBs. This means I need to 
add almost 20 regulator_desc separately if I want to add support according to the existing driver code. Instead of this, I can
use macros as below but it causes changes in the existing driver (if I make max77650 regulator descriptions too using macros).

#define REGULATOR_DESC_LDO() {
#define REGULATOR_DESC_SBB() {

static const struct regulator_desc max77650_regulator_desc[] = {
	REGULATOR_DESC_LDO(LDO),
	REGULATOR_DESC_SBB(SBB0),
	REGULATOR_DESC_SBB(SBB1),
	REGULATOR_DESC_SBB(SBB2),
)

Similar problems occur on the mfd driver as well (irqs and mfd_cells).

Best regards,
Zeynep