| Message ID | 20250512-dev-adp5589-fw-v3-2-092b14b79a88@analog.com |
|---|---|
| State | New |
| Headers | show |
| Series | mfd: adp5585: support keymap events and drop legacy Input driver | expand |
On Mon, 12 May 2025, Nuno Sá via B4 Relay wrote: > From: Nuno Sá <nuno.sa@analog.com> > > Not all devices (features) of the adp5585 device are mandatory to be > used in all platforms. Hence, check what's given in FW and dynamically > create the mfd_cell array to be given to devm_mfd_add_devices(). > > Signed-off-by: Nuno Sá <nuno.sa@analog.com> > --- > drivers/mfd/adp5585.c | 45 +++++++++++++++++++++++++++++++++++++++++---- > 1 file changed, 41 insertions(+), 4 deletions(-) > > diff --git a/drivers/mfd/adp5585.c b/drivers/mfd/adp5585.c > index 160e0b38106a6d78f7d4b7c866cb603d96ea673e..02f9e8c1c6a1d8b9516c060e0024d69886e9fb7a 100644 > --- a/drivers/mfd/adp5585.c > +++ b/drivers/mfd/adp5585.c > @@ -17,7 +17,13 @@ > #include <linux/regmap.h> > #include <linux/types.h> > > -static const struct mfd_cell adp5585_devs[] = { > +enum { > + ADP5585_DEV_GPIO, > + ADP5585_DEV_PWM, > + ADP5585_DEV_MAX > +}; > + > +static const struct mfd_cell adp5585_devs[ADP5585_DEV_MAX] = { > { .name = "adp5585-gpio", }, > { .name = "adp5585-pwm", }, > }; > @@ -110,12 +116,40 @@ static const struct regmap_config adp5585_regmap_configs[] = { > }, > }; > > +static int adp5585_parse_fw(struct device *dev, struct adp5585_dev *adp5585, > + struct mfd_cell **devs) > +{ > + unsigned int has_pwm = 0, has_gpio = 0, rc = 0; > + > + if (device_property_present(dev, "#pwm-cells")) > + has_pwm = 1; This is a little sloppy. Instead of using throwaway local variables, do what you're going to do in the if statement. > + if (device_property_present(dev, "#gpio-cells")) > + has_gpio = 1; > + > + if (!has_pwm && !has_gpio) > + return -ENODEV; Are we really dictating which child devices to register based on random DT properties? Why not register them anyway and have them fail if the information they need is not available? Missing / incorrect properties usually get a -EINVAL. > + *devs = devm_kcalloc(dev, has_pwm + has_gpio, sizeof(struct mfd_cell), > + GFP_KERNEL); > + if (!*devs) > + return -ENOMEM; > + > + if (has_pwm) > + (*devs)[rc++] = adp5585_devs[ADP5585_DEV_PWM]; > + if (has_gpio) > + (*devs)[rc++] = adp5585_devs[ADP5585_DEV_GPIO]; Passing around pointers to pointers for allocation (and later, pointer to functions) is not the way we wish to operate. See how all of the other MFD drivers handle selective sub-drivers. > + return rc; > +} > + > static int adp5585_i2c_probe(struct i2c_client *i2c) > { > const struct regmap_config *regmap_config; > struct adp5585_dev *adp5585; > + struct mfd_cell *devs; > unsigned int id; > - int ret; > + int ret, n_devs; > > adp5585 = devm_kzalloc(&i2c->dev, sizeof(*adp5585), GFP_KERNEL); > if (!adp5585) > @@ -138,9 +172,12 @@ static int adp5585_i2c_probe(struct i2c_client *i2c) > return dev_err_probe(&i2c->dev, -ENODEV, > "Invalid device ID 0x%02x\n", id); > > + n_devs = adp5585_parse_fw(&i2c->dev, adp5585, &devs); > + if (n_devs < 0) > + return n_devs; > + > ret = devm_mfd_add_devices(&i2c->dev, PLATFORM_DEVID_AUTO, > - adp5585_devs, ARRAY_SIZE(adp5585_devs), > - NULL, 0, NULL); > + devs, n_devs, NULL, 0, NULL); > if (ret) > return dev_err_probe(&i2c->dev, ret, > "Failed to add child devices\n"); > > -- > 2.49.0 > >
On Tue, 2025-05-13 at 15:34 +0100, Lee Jones wrote: > On Mon, 12 May 2025, Nuno Sá via B4 Relay wrote: > > > From: Nuno Sá <nuno.sa@analog.com> > > > > Not all devices (features) of the adp5585 device are mandatory to be > > used in all platforms. Hence, check what's given in FW and dynamically > > create the mfd_cell array to be given to devm_mfd_add_devices(). > > > > Signed-off-by: Nuno Sá <nuno.sa@analog.com> > > --- > > drivers/mfd/adp5585.c | 45 +++++++++++++++++++++++++++++++++++++++++---- > > 1 file changed, 41 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/mfd/adp5585.c b/drivers/mfd/adp5585.c > > index > > 160e0b38106a6d78f7d4b7c866cb603d96ea673e..02f9e8c1c6a1d8b9516c060e0024d69886 > > e9fb7a 100644 > > --- a/drivers/mfd/adp5585.c > > +++ b/drivers/mfd/adp5585.c > > @@ -17,7 +17,13 @@ > > #include <linux/regmap.h> > > #include <linux/types.h> > > > > -static const struct mfd_cell adp5585_devs[] = { > > +enum { > > + ADP5585_DEV_GPIO, > > + ADP5585_DEV_PWM, > > + ADP5585_DEV_MAX > > +}; > > + > > +static const struct mfd_cell adp5585_devs[ADP5585_DEV_MAX] = { > > { .name = "adp5585-gpio", }, > > { .name = "adp5585-pwm", }, > > }; > > @@ -110,12 +116,40 @@ static const struct regmap_config > > adp5585_regmap_configs[] = { > > }, > > }; > > > > +static int adp5585_parse_fw(struct device *dev, struct adp5585_dev > > *adp5585, > > + struct mfd_cell **devs) > > +{ > > + unsigned int has_pwm = 0, has_gpio = 0, rc = 0; > > + > > + if (device_property_present(dev, "#pwm-cells")) > > + has_pwm = 1; > > This is a little sloppy. Instead of using throwaway local variables, do > what you're going to do in the if statement. > Then I would need to realloc my device cells... But as I realized below, this is indeed not needed. > > + if (device_property_present(dev, "#gpio-cells")) > > + has_gpio = 1; > > + > > + if (!has_pwm && !has_gpio) > > + return -ENODEV; > > Are we really dictating which child devices to register based on random > DT properties? Why not register them anyway and have them fail if the > information they need is not available? Missing / incorrect properties > usually get a -EINVAL. Well, this was something Laurent asked for... In the previous version I was registering all the devices unconditionally. > > > + *devs = devm_kcalloc(dev, has_pwm + has_gpio, sizeof(struct > > mfd_cell), > > + GFP_KERNEL); > > + if (!*devs) > > + return -ENOMEM; > > + > > + if (has_pwm) > > + (*devs)[rc++] = adp5585_devs[ADP5585_DEV_PWM]; > > + if (has_gpio) > > + (*devs)[rc++] = adp5585_devs[ADP5585_DEV_GPIO]; > > Passing around pointers to pointers for allocation (and later, pointer > to functions) is not the way we wish to operate. See how all of the > other MFD drivers handle selective sub-drivers. Any pointer from the top of your head (example driver)? Honestly, I do not see this being that bad. Pretty much is a dynamic array of struct mfd_cel but anyways, no strong feelings But... I was actually being very stupid. First I did looked at an API to only add one mfd device and failed to realize that I can use devm_mfd_add_devices() with n_devs = 1 Nevermind, will refactor in v4 > > > + return rc; > > +} > > + > > static int adp5585_i2c_probe(struct i2c_client *i2c) > > { > > const struct regmap_config *regmap_config; > > struct adp5585_dev *adp5585; > > + struct mfd_cell *devs; > > unsigned int id; > > - int ret; > > + int ret, n_devs; > > > > adp5585 = devm_kzalloc(&i2c->dev, sizeof(*adp5585), GFP_KERNEL); > > if (!adp5585) > > @@ -138,9 +172,12 @@ static int adp5585_i2c_probe(struct i2c_client *i2c) > > return dev_err_probe(&i2c->dev, -ENODEV, > > "Invalid device ID 0x%02x\n", id); > > > > + n_devs = adp5585_parse_fw(&i2c->dev, adp5585, &devs); > > + if (n_devs < 0) > > + return n_devs; > > + > > ret = devm_mfd_add_devices(&i2c->dev, PLATFORM_DEVID_AUTO, > > - adp5585_devs, ARRAY_SIZE(adp5585_devs), > > - NULL, 0, NULL); > > + devs, n_devs, NULL, 0, NULL); > > if (ret) > > return dev_err_probe(&i2c->dev, ret, > > "Failed to add child devices\n"); > > > > -- > > 2.49.0 > > > >
On Tue, 13 May 2025, Laurent Pinchart wrote: > On Tue, May 13, 2025 at 04:02:11PM +0100, Nuno Sá wrote: > > On Tue, 2025-05-13 at 15:34 +0100, Lee Jones wrote: > > > On Mon, 12 May 2025, Nuno Sá via B4 Relay wrote: > > > > > > > From: Nuno Sá <nuno.sa@analog.com> > > > > > > > > Not all devices (features) of the adp5585 device are mandatory to be > > > > used in all platforms. Hence, check what's given in FW and dynamically > > > > create the mfd_cell array to be given to devm_mfd_add_devices(). > > > > > > > > Signed-off-by: Nuno Sá <nuno.sa@analog.com> > > > > --- > > > > drivers/mfd/adp5585.c | 45 +++++++++++++++++++++++++++++++++++++++++---- > > > > 1 file changed, 41 insertions(+), 4 deletions(-) > > > > > > > > diff --git a/drivers/mfd/adp5585.c b/drivers/mfd/adp5585.c > > > > index > > > > 160e0b38106a6d78f7d4b7c866cb603d96ea673e..02f9e8c1c6a1d8b9516c060e0024d69886 > > > > e9fb7a 100644 > > > > --- a/drivers/mfd/adp5585.c > > > > +++ b/drivers/mfd/adp5585.c > > > > @@ -17,7 +17,13 @@ > > > > #include <linux/regmap.h> > > > > #include <linux/types.h> > > > > > > > > -static const struct mfd_cell adp5585_devs[] = { > > > > +enum { > > > > + ADP5585_DEV_GPIO, > > > > + ADP5585_DEV_PWM, > > > > + ADP5585_DEV_MAX > > > > +}; > > > > + > > > > +static const struct mfd_cell adp5585_devs[ADP5585_DEV_MAX] = { > > > > { .name = "adp5585-gpio", }, > > > > { .name = "adp5585-pwm", }, > > > > }; > > > > @@ -110,12 +116,40 @@ static const struct regmap_config > > > > adp5585_regmap_configs[] = { > > > > }, > > > > }; > > > > > > > > +static int adp5585_parse_fw(struct device *dev, struct adp5585_dev > > > > *adp5585, > > > > + struct mfd_cell **devs) > > > > +{ > > > > + unsigned int has_pwm = 0, has_gpio = 0, rc = 0; > > > > + > > > > + if (device_property_present(dev, "#pwm-cells")) > > > > + has_pwm = 1; > > > > > > This is a little sloppy. Instead of using throwaway local variables, do > > > what you're going to do in the if statement. > > > > Then I would need to realloc my device cells... But as I realized below, this is > > indeed not needed. > > > > > > + if (device_property_present(dev, "#gpio-cells")) > > > > + has_gpio = 1; > > > > + > > > > + if (!has_pwm && !has_gpio) > > > > + return -ENODEV; > > > > > > Are we really dictating which child devices to register based on random > > > DT properties? Why not register them anyway and have them fail if the > > The properties are not random. > > > > information they need is not available? Missing / incorrect properties > > > usually get a -EINVAL. > > > > Well, this was something Laurent asked for... In the previous version I was > > registering all the devices unconditionally. > > Registering them all means we'll get error messages in the kernel log > when the corresponding drivers will probe, while nothing is actually > wrong. That's fairly confusing for the user. > > In an ideal situation we would have child nodes in DT and only register > child devices for existing child nodes. Unfortunately the DT bindings > were not designed that way, so we have to live with the current > situation. > > > > > + *devs = devm_kcalloc(dev, has_pwm + has_gpio, sizeof(struct mfd_cell), > > > > + GFP_KERNEL); > > > > + if (!*devs) > > > > + return -ENOMEM; > > > > + > > > > + if (has_pwm) > > > > + (*devs)[rc++] = adp5585_devs[ADP5585_DEV_PWM]; > > > > + if (has_gpio) > > > > + (*devs)[rc++] = adp5585_devs[ADP5585_DEV_GPIO]; > > > > > > Passing around pointers to pointers for allocation (and later, pointer > > > to functions) is not the way we wish to operate. See how all of the > > > other MFD drivers handle selective sub-drivers. > > > > Any pointer from the top of your head (example driver)? Honestly, I do not see > > this being that bad. Pretty much is a dynamic array of struct mfd_cel but > > anyways, no strong feelings > > I don't find it that bad either. I don't think you should use > devm_kcalloc() though, as the memory should be freed as soon as it's not > needed anymore. > > > But... I was actually being very stupid. First I did looked at an API to only > > Occasionally overseeing a possible solution isn't being stupid. Or at > least I hope it isn't, otherwise I would be very stupid too. Yes, likewise. Never worry about that. In general let's try to simplify things by not using pointers to pointers and pointers to functions. There are usually much nicer, cleaner and simpler solutions. IMHO, the above is C-hackery at its best. Let's see where v4 takes us.
diff --git a/drivers/mfd/adp5585.c b/drivers/mfd/adp5585.c index 160e0b38106a6d78f7d4b7c866cb603d96ea673e..02f9e8c1c6a1d8b9516c060e0024d69886e9fb7a 100644 --- a/drivers/mfd/adp5585.c +++ b/drivers/mfd/adp5585.c @@ -17,7 +17,13 @@ #include <linux/regmap.h> #include <linux/types.h> -static const struct mfd_cell adp5585_devs[] = { +enum { + ADP5585_DEV_GPIO, + ADP5585_DEV_PWM, + ADP5585_DEV_MAX +}; + +static const struct mfd_cell adp5585_devs[ADP5585_DEV_MAX] = { { .name = "adp5585-gpio", }, { .name = "adp5585-pwm", }, }; @@ -110,12 +116,40 @@ static const struct regmap_config adp5585_regmap_configs[] = { }, }; +static int adp5585_parse_fw(struct device *dev, struct adp5585_dev *adp5585, + struct mfd_cell **devs) +{ + unsigned int has_pwm = 0, has_gpio = 0, rc = 0; + + if (device_property_present(dev, "#pwm-cells")) + has_pwm = 1; + + if (device_property_present(dev, "#gpio-cells")) + has_gpio = 1; + + if (!has_pwm && !has_gpio) + return -ENODEV; + + *devs = devm_kcalloc(dev, has_pwm + has_gpio, sizeof(struct mfd_cell), + GFP_KERNEL); + if (!*devs) + return -ENOMEM; + + if (has_pwm) + (*devs)[rc++] = adp5585_devs[ADP5585_DEV_PWM]; + if (has_gpio) + (*devs)[rc++] = adp5585_devs[ADP5585_DEV_GPIO]; + + return rc; +} + static int adp5585_i2c_probe(struct i2c_client *i2c) { const struct regmap_config *regmap_config; struct adp5585_dev *adp5585; + struct mfd_cell *devs; unsigned int id; - int ret; + int ret, n_devs; adp5585 = devm_kzalloc(&i2c->dev, sizeof(*adp5585), GFP_KERNEL); if (!adp5585) @@ -138,9 +172,12 @@ static int adp5585_i2c_probe(struct i2c_client *i2c) return dev_err_probe(&i2c->dev, -ENODEV, "Invalid device ID 0x%02x\n", id); + n_devs = adp5585_parse_fw(&i2c->dev, adp5585, &devs); + if (n_devs < 0) + return n_devs; + ret = devm_mfd_add_devices(&i2c->dev, PLATFORM_DEVID_AUTO, - adp5585_devs, ARRAY_SIZE(adp5585_devs), - NULL, 0, NULL); + devs, n_devs, NULL, 0, NULL); if (ret) return dev_err_probe(&i2c->dev, ret, "Failed to add child devices\n");