| Message ID | 20231228100208.2932-2-karelb@gimli.ms.mff.cuni.cz |
|---|---|
| State | New |
| Headers | show |
| Series | regulator: support for Marvell 88PM886 LDOs and bucks | expand |
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 {