Message ID | 20231228100208.2932-2-karelb@gimli.ms.mff.cuni.cz |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel+bounces-12551-ouuuleilei=gmail.com@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a05:7301:6f82:b0:100:9c79:88ff with SMTP id tb2csp1900397dyb; Thu, 28 Dec 2023 02:02:57 -0800 (PST) X-Google-Smtp-Source: AGHT+IHlxjYzxgfV2V+RoEBOuxZU4/cbQ33phPjFpfYg9nnEpkBe1U44KWvdu8qtUwChUNU6JCk5 X-Received: by 2002:a05:6358:6501:b0:175:b65:c8f1 with SMTP id v1-20020a056358650100b001750b65c8f1mr1597350rwg.37.1703757777458; Thu, 28 Dec 2023 02:02:57 -0800 (PST) ARC-Seal: i=1; a=rsa-sha256; t=1703757777; cv=none; d=google.com; s=arc-20160816; b=Zu8azacDX34xmonEyTQfrlTAE4FhLVpyzavZ7R7Y7NsybHjfupEgk2pvIO/ECRM69t zz4oeyM6hae/wBQu5X6tUGrfmzBXlZK26JzXiNkBWN1pRJuhnMxwIrq2iDiNEGTghxRX 87eD+81yiAH1DQZHKOcKO1cmfvS6LW0ZbnCW4QFLJ8FudzJmirc5no55o073Y2z2BFGb y45wPy4hlF5A3I6YxHkEPxNbXCwN2PYB7MG3ttJw2Hc4mwBiC+EZAhSbb//lYev4gTL2 i8JfblYXCMfnU8fDWKQntcoGj2YyIMaQTkqEVPXuJ82MTVUhXobVMD/78ZolsEYkB7lQ i6qw== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=content-transfer-encoding:mime-version:list-unsubscribe :list-subscribe:list-id:precedence:references:in-reply-to:message-id :date:subject:cc:to:from:dkim-signature; bh=reD/I3eBv9lwzRcVumL6wA4HHqcgwCGXR849XP7JFqk=; fh=jTCjoJ+Y+apFiqJqgoUFlazJ2LdeNccwdUnBdvX3V30=; b=Wmvx/sjDh36aBlBXwLg6jVUfwUAe9VjdtpZkWLhjmQPh35atFkURFtyyRx3C3/0arS GTWsPjqfJObgwKICpWHPyrIKtsrUQY59SWlyUfli8ICpdWYhfgWEKR89WFLKurLgCoHP QYeGSbZxtQ6ppHGHN7buIuKJLd53PFkPD6R4jWBDlaaqfwSTOCrk1ojin+LoT8PeHppT 2PVIIDyLxauWrTE69n6io58QaWoQLwu4gLtn1t35uchbNuS65/GZEMPBI3Ut5vN9OWBr IVucqwaQbXyi8vg/hR6Mx3hTZg9pn4pgP7wE6UuHkCGqCSQbjy5AZWb7S4KaUdfRXrwB nB/g== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@gimli.ms.mff.cuni.cz header.s=gen1 header.b=NtB0Z3WX; spf=pass (google.com: domain of linux-kernel+bounces-12551-ouuuleilei=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) smtp.mailfrom="linux-kernel+bounces-12551-ouuuleilei=gmail.com@vger.kernel.org"; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=gimli.ms.mff.cuni.cz Received: from sv.mirrors.kernel.org (sv.mirrors.kernel.org. [2604:1380:45e3:2400::1]) by mx.google.com with ESMTPS id e5-20020a636905000000b005ce087e004dsi8191930pgc.799.2023.12.28.02.02.57 for <ouuuleilei@gmail.com> (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 28 Dec 2023 02:02:57 -0800 (PST) Received-SPF: pass (google.com: domain of linux-kernel+bounces-12551-ouuuleilei=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1; Authentication-Results: mx.google.com; dkim=pass header.i=@gimli.ms.mff.cuni.cz header.s=gen1 header.b=NtB0Z3WX; spf=pass (google.com: domain of linux-kernel+bounces-12551-ouuuleilei=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) smtp.mailfrom="linux-kernel+bounces-12551-ouuuleilei=gmail.com@vger.kernel.org"; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=gimli.ms.mff.cuni.cz Received: from smtp.subspace.kernel.org (wormhole.subspace.kernel.org [52.25.139.140]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by sv.mirrors.kernel.org (Postfix) with ESMTPS id 2C77728562E for <ouuuleilei@gmail.com>; Thu, 28 Dec 2023 10:02:57 +0000 (UTC) Received: from localhost.localdomain (localhost.localdomain [127.0.0.1]) by smtp.subspace.kernel.org (Postfix) with ESMTP id 396FA7460; Thu, 28 Dec 2023 10:02:32 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; dkim=pass (1024-bit key) header.d=gimli.ms.mff.cuni.cz header.i=@gimli.ms.mff.cuni.cz header.b="NtB0Z3WX" X-Original-To: linux-kernel@vger.kernel.org Received: from nikam.ms.mff.cuni.cz (nikam.ms.mff.cuni.cz [195.113.20.16]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id BBF7F63C6; Thu, 28 Dec 2023 10:02:25 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=gimli.ms.mff.cuni.cz Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=gimli.ms.mff.cuni.cz Received: from gimli.ms.mff.cuni.cz (gimli.ms.mff.cuni.cz [195.113.20.176]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by nikam.ms.mff.cuni.cz (Postfix) with ESMTPS id EC6E628C556; Thu, 28 Dec 2023 11:02:23 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gimli.ms.mff.cuni.cz; s=gen1; t=1703757743; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=reD/I3eBv9lwzRcVumL6wA4HHqcgwCGXR849XP7JFqk=; b=NtB0Z3WXtMxwoAgv6bocv8Vo273oBsXVko1c5uru6XFjKos2fIN6Y0QEXBbsP2yeMduL5P n8RLKsCcDx6vJfVhMojAhszQEeM9TRazAWvzWcA6V9dz2EPKEEXPH0Qzj3oGeGK8c9ZKQ0 +DRSBUOFYPQ9mFL7obXKLYWIoOYj5HE= Received: from localhost (internet5.mraknet.com [185.200.108.250]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange ECDHE (P-256) server-signature RSA-PSS (2048 bits) server-digest SHA256) (Client did not present a certificate) (Authenticated sender: karelb) by gimli.ms.mff.cuni.cz (Postfix) with ESMTPSA id CB00D4423CE; Thu, 28 Dec 2023 11:02:23 +0100 (CET) From: Karel Balej <karelb@gimli.ms.mff.cuni.cz> To: Karel Balej <balejk@matfyz.cz>, Lee Jones <lee@kernel.org>, Rob Herring <robh+dt@kernel.org>, Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>, Conor Dooley <conor+dt@kernel.org>, Liam Girdwood <lgirdwood@gmail.com>, Mark Brown <broonie@kernel.org>, devicetree@vger.kernel.org, linux-kernel@vger.kernel.org Cc: =?utf-8?q?Duje_Mihanovi=C4=87?= <duje.mihanovic@skole.hr>, ~postmarketos/upstreaming@lists.sr.ht, phone-devel@vger.kernel.org Subject: [RFC PATCH 1/5] mfd: 88pm88x: differences with respect to the PMIC RFC series Date: Thu, 28 Dec 2023 10:39:10 +0100 Message-ID: <20231228100208.2932-2-karelb@gimli.ms.mff.cuni.cz> X-Mailer: git-send-email 2.43.0 In-Reply-To: <20231228100208.2932-1-karelb@gimli.ms.mff.cuni.cz> References: <20231228100208.2932-1-karelb@gimli.ms.mff.cuni.cz> Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: <linux-kernel.vger.kernel.org> List-Subscribe: <mailto:linux-kernel+subscribe@vger.kernel.org> List-Unsubscribe: <mailto:linux-kernel+unsubscribe@vger.kernel.org> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-getmail-retrieved-from-mailbox: INBOX X-GMAIL-THRID: 1786519515579505089 X-GMAIL-MSGID: 1786519515579505089 |
Series |
regulator: support for Marvell 88PM886 LDOs and bucks
|
|
Commit Message
Karel Balej
Dec. 28, 2023, 9:39 a.m. UTC
From: Karel Balej <balejk@matfyz.cz> Signed-off-by: Karel Balej <balejk@matfyz.cz> --- drivers/mfd/88pm88x.c | 14 ++++++++------ include/linux/mfd/88pm88x.h | 2 ++ 2 files changed, 10 insertions(+), 6 deletions(-)
Comments
The subject needs work. Please tell us what the patches is doing. On Thu, 28 Dec 2023, Karel Balej wrote: > From: Karel Balej <balejk@matfyz.cz> A full an complete commit message is a must. > Signed-off-by: Karel Balej <balejk@matfyz.cz> > --- > drivers/mfd/88pm88x.c | 14 ++++++++------ > include/linux/mfd/88pm88x.h | 2 ++ > 2 files changed, 10 insertions(+), 6 deletions(-) > > diff --git a/drivers/mfd/88pm88x.c b/drivers/mfd/88pm88x.c > index 5db6c65b667d..3424d88a58f6 100644 > --- a/drivers/mfd/88pm88x.c > +++ b/drivers/mfd/88pm88x.c > @@ -57,16 +57,16 @@ static struct reg_sequence pm886_presets[] = { > REG_SEQ0(PM88X_REG_BK_OSC_CTRL3, 0xc0), > }; > > -static struct resource onkey_resources[] = { > +static struct resource pm88x_onkey_resources[] = { > DEFINE_RES_IRQ_NAMED(PM88X_IRQ_ONKEY, "88pm88x-onkey"), > }; > > -static struct mfd_cell pm88x_devs[] = { > +static struct mfd_cell pm886_devs[] = { > { > .name = "88pm88x-onkey", > - .num_resources = ARRAY_SIZE(onkey_resources), > - .resources = onkey_resources, > - .id = -1, > + .of_compatible = "marvell,88pm88x-onkey", > + .num_resources = ARRAY_SIZE(pm88x_onkey_resources), > + .resources = pm88x_onkey_resources, > }, > }; > > @@ -74,6 +74,8 @@ static struct pm88x_data pm886_a1_data = { > .whoami = PM886_A1_WHOAMI, > .presets = pm886_presets, > .num_presets = ARRAY_SIZE(pm886_presets), > + .devs = pm886_devs, > + .num_devs = ARRAY_SIZE(pm886_devs), > }; > > static const struct regmap_config pm88x_i2c_regmap = { > @@ -157,7 +159,7 @@ static int pm88x_probe(struct i2c_client *client) > if (ret) > return ret; > > - ret = devm_mfd_add_devices(&client->dev, 0, pm88x_devs, ARRAY_SIZE(pm88x_devs), > + ret = devm_mfd_add_devices(&client->dev, 0, chip->data->devs, chip->data->num_devs, > NULL, 0, regmap_irq_get_domain(chip->irq_data)); > if (ret) { > dev_err(&client->dev, "Failed to add devices: %d\n", ret); > diff --git a/include/linux/mfd/88pm88x.h b/include/linux/mfd/88pm88x.h > index a34c57447827..9a335f6b9c07 100644 > --- a/include/linux/mfd/88pm88x.h > +++ b/include/linux/mfd/88pm88x.h > @@ -49,6 +49,8 @@ struct pm88x_data { > unsigned int whoami; > struct reg_sequence *presets; > unsigned int num_presets; > + struct mfd_cell *devs; > + unsigned int num_devs; Why are you adding extra abstraction? > }; > > struct pm88x_chip { > -- > 2.43.0 >
Lee, On Thu Jan 11, 2024 at 11:54 AM CET, Lee Jones wrote: > The subject needs work. Please tell us what the patches is doing. > > On Thu, 28 Dec 2023, Karel Balej wrote: > > > From: Karel Balej <balejk@matfyz.cz> > > A full an complete commit message is a must. I have not provided a detailed description here because as I have noted in the cover letter, this patch will be squashed into the MFD series. I sent it only as a bridge between the two series, sorry for the confusion. > > diff --git a/include/linux/mfd/88pm88x.h b/include/linux/mfd/88pm88x.h > > index a34c57447827..9a335f6b9c07 100644 > > --- a/include/linux/mfd/88pm88x.h > > +++ b/include/linux/mfd/88pm88x.h > > @@ -49,6 +49,8 @@ struct pm88x_data { > > unsigned int whoami; > > struct reg_sequence *presets; > > unsigned int num_presets; > > + struct mfd_cell *devs; > > + unsigned int num_devs; > > Why are you adding extra abstraction? Right, this is probably not necessary now since I'm only implementing support for one of the chips - it's just that I keep thinking about it as a driver for both of them and thus tend to write it a bit more abstractly. Shall I then drop this and also the `presets` member which is also chip-specific? Thank you, best regards, K. B.
On Thu, 11 Jan 2024, Karel Balej wrote: > Lee, > > On Thu Jan 11, 2024 at 11:54 AM CET, Lee Jones wrote: > > The subject needs work. Please tell us what the patches is doing. > > > > On Thu, 28 Dec 2023, Karel Balej wrote: > > > > > From: Karel Balej <balejk@matfyz.cz> > > > > A full an complete commit message is a must. > > I have not provided a detailed description here because as I have noted > in the cover letter, this patch will be squashed into the MFD series. I > sent it only as a bridge between the two series, sorry for the > confusion. > > > > diff --git a/include/linux/mfd/88pm88x.h b/include/linux/mfd/88pm88x.h > > > index a34c57447827..9a335f6b9c07 100644 > > > --- a/include/linux/mfd/88pm88x.h > > > +++ b/include/linux/mfd/88pm88x.h > > > @@ -49,6 +49,8 @@ struct pm88x_data { > > > unsigned int whoami; > > > struct reg_sequence *presets; > > > unsigned int num_presets; > > > + struct mfd_cell *devs; > > > + unsigned int num_devs; > > > > Why are you adding extra abstraction? > > Right, this is probably not necessary now since I'm only implementing > support for one of the chips - it's just that I keep thinking about it > as a driver for both of them and thus tend to write it a bit more > abstractly. Shall I then drop this and also the `presets` member which > is also chip-specific? Even if you were to support multiple devices, this strategy is unusual and isn't likely to be accepted. With respect to the other variables, you are in a better position to know if they are still required. By the sounds of it, I'd suggest it might be better to remove them.
On Thu Jan 11, 2024 at 4:25 PM CET, Lee Jones wrote: [...] > > > > diff --git a/include/linux/mfd/88pm88x.h b/include/linux/mfd/88pm88x.h > > > > index a34c57447827..9a335f6b9c07 100644 > > > > --- a/include/linux/mfd/88pm88x.h > > > > +++ b/include/linux/mfd/88pm88x.h > > > > @@ -49,6 +49,8 @@ struct pm88x_data { > > > > unsigned int whoami; > > > > struct reg_sequence *presets; > > > > unsigned int num_presets; > > > > + struct mfd_cell *devs; > > > > + unsigned int num_devs; > > > > > > Why are you adding extra abstraction? > > > > Right, this is probably not necessary now since I'm only implementing > > support for one of the chips - it's just that I keep thinking about it > > as a driver for both of them and thus tend to write it a bit more > > abstractly. Shall I then drop this and also the `presets` member which > > is also chip-specific? > > Even if you were to support multiple devices, this strategy is unusual > and isn't likely to be accepted. May I please ask what the recommended strategy is then? `switch`ing on the chip ID? I have taken this approach because it seemed to produce a cleaner/more straightforward code in comparison to that. Or are you only talking about the chip cells/subdevices in particular? Thank you, K. B.
On Thu, 11 Jan 2024, Karel Balej wrote: > On Thu Jan 11, 2024 at 4:25 PM CET, Lee Jones wrote: > > [...] > > > > > > diff --git a/include/linux/mfd/88pm88x.h b/include/linux/mfd/88pm88x.h > > > > > index a34c57447827..9a335f6b9c07 100644 > > > > > --- a/include/linux/mfd/88pm88x.h > > > > > +++ b/include/linux/mfd/88pm88x.h > > > > > @@ -49,6 +49,8 @@ struct pm88x_data { > > > > > unsigned int whoami; > > > > > struct reg_sequence *presets; > > > > > unsigned int num_presets; > > > > > + struct mfd_cell *devs; > > > > > + unsigned int num_devs; > > > > > > > > Why are you adding extra abstraction? > > > > > > Right, this is probably not necessary now since I'm only implementing > > > support for one of the chips - it's just that I keep thinking about it > > > as a driver for both of them and thus tend to write it a bit more > > > abstractly. Shall I then drop this and also the `presets` member which > > > is also chip-specific? > > > > Even if you were to support multiple devices, this strategy is unusual > > and isn't likely to be accepted. > > May I please ask what the recommended strategy is then? `switch`ing on > the chip ID? I have taken this approach because it seemed to produce a > cleaner/more straightforward code in comparison to that. Or are you only > talking about the chip cells/subdevices in particular? I'd have to see the implementation, but the general exception I'm taking here is storing the use-once MFD cell data inside a data structure. If you were to match on device ID it's *sometimes* acceptable to store the pointers in local variables.
diff --git a/drivers/mfd/88pm88x.c b/drivers/mfd/88pm88x.c index 5db6c65b667d..3424d88a58f6 100644 --- a/drivers/mfd/88pm88x.c +++ b/drivers/mfd/88pm88x.c @@ -57,16 +57,16 @@ static struct reg_sequence pm886_presets[] = { REG_SEQ0(PM88X_REG_BK_OSC_CTRL3, 0xc0), }; -static struct resource onkey_resources[] = { +static struct resource pm88x_onkey_resources[] = { DEFINE_RES_IRQ_NAMED(PM88X_IRQ_ONKEY, "88pm88x-onkey"), }; -static struct mfd_cell pm88x_devs[] = { +static struct mfd_cell pm886_devs[] = { { .name = "88pm88x-onkey", - .num_resources = ARRAY_SIZE(onkey_resources), - .resources = onkey_resources, - .id = -1, + .of_compatible = "marvell,88pm88x-onkey", + .num_resources = ARRAY_SIZE(pm88x_onkey_resources), + .resources = pm88x_onkey_resources, }, }; @@ -74,6 +74,8 @@ static struct pm88x_data pm886_a1_data = { .whoami = PM886_A1_WHOAMI, .presets = pm886_presets, .num_presets = ARRAY_SIZE(pm886_presets), + .devs = pm886_devs, + .num_devs = ARRAY_SIZE(pm886_devs), }; static const struct regmap_config pm88x_i2c_regmap = { @@ -157,7 +159,7 @@ static int pm88x_probe(struct i2c_client *client) if (ret) return ret; - ret = devm_mfd_add_devices(&client->dev, 0, pm88x_devs, ARRAY_SIZE(pm88x_devs), + ret = devm_mfd_add_devices(&client->dev, 0, chip->data->devs, chip->data->num_devs, NULL, 0, regmap_irq_get_domain(chip->irq_data)); if (ret) { dev_err(&client->dev, "Failed to add devices: %d\n", ret); diff --git a/include/linux/mfd/88pm88x.h b/include/linux/mfd/88pm88x.h index a34c57447827..9a335f6b9c07 100644 --- a/include/linux/mfd/88pm88x.h +++ b/include/linux/mfd/88pm88x.h @@ -49,6 +49,8 @@ struct pm88x_data { unsigned int whoami; struct reg_sequence *presets; unsigned int num_presets; + struct mfd_cell *devs; + unsigned int num_devs; }; struct pm88x_chip {