| Message ID | 20250512-dev-adp5589-fw-v3-0-092b14b79a88@analog.com |
|---|---|
| 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:39 +0100, Lee Jones wrote: > On Mon, 12 May 2025, Nuno Sá via B4 Relay wrote: > > > From: Nuno Sá <nuno.sa@analog.com> > > > > Use the helper macro. No functional change intended... > > > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > Signed-off-by: Nuno Sá <nuno.sa@analog.com> > > --- > > drivers/mfd/adp5585.c | 5 +++-- > > 1 file changed, 3 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/mfd/adp5585.c b/drivers/mfd/adp5585.c > > index > > d693b1ead05128e02f671ca465f9c72cab3b3395..19d4a0ab1bb4c261e82559630624059529 > > 765fbd 100644 > > --- a/drivers/mfd/adp5585.c > > +++ b/drivers/mfd/adp5585.c > > @@ -4,6 +4,7 @@ > > * > > * Copyright 2022 NXP > > * Copyright 2024 Ideas on Board Oy > > + * Copyright 2025 Analog Devices Inc. > > If you're going to sneak in irrelevant changes, at least mention it in > passing in the change log. I actually thought this was needy and is present since v1... Can mention it in the commit message in the next version. > > > */ > > > > #include <linux/array_size.h> > > @@ -24,8 +25,8 @@ enum { > > }; > > > > static const struct mfd_cell adp5585_devs[ADP5585_DEV_MAX] = { > > - { .name = "adp5585-gpio", }, > > - { .name = "adp5585-pwm", }, > > + MFD_CELL_NAME("adp5585-gpio"), > > + MFD_CELL_NAME("adp5585-pwm"), > > }; > > > > static const struct regmap_range adp5585_volatile_ranges[] = { > > > > -- > > 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, 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. > 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"); > > >
On Tue, 2025-05-13 at 17:19 +0200, 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..02f9e8c1c6a1d8b9516c060e0024d6 > > > > 9886 > > > > 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. > Agreed... Sometimes I do forget we also have __free(kfree) even though I used it in the keyboard driver. > > 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. > I know :)... > > 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"); > > > >
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.
On Mon, 12 May 2025, Nuno Sá via B4 Relay wrote: > Hi all, > > Here it goes v3. There was some major refactoring in this version due to > Lee's and Laurent's feedback. There are some splits (and some explicit > requests) resulting in new patches being added. The biggest change is the > effort in trying to minimize the usage of specific child device bits in > the top level device (mainly stuff related to the keymap). I think now > it's fairly self contained and the only thing that we really need to > handle in the top device are the unlock and reset events as those can be > supported through both the input and gpio devices (via gpio_keys). This > results in a bit of more runtime complexity but well, that's life... > > Another change is Lee's suggestion of making use of templates (for > regmap and chip specific data) and fill things up at probe. > > I also refactored a bit the event handling so it's more generic now. > There were lot's of changes so odds are that I might have forgotten some > feedback and so, my apologies in advance :). > > I also dropped the tags in: > > patch 16/22 ("gpio: adp5585: support gpi events") as it has some > significant changes (replacing .init_valid_masks() with .request() and > .free()) Please run this set through checkpatch.pl before submitting again. Not sure if we've discussed this, but W=1 wouldn't hurt either.
On Wed, 2025-05-14 at 09:25 +0100, Lee Jones wrote: > On Mon, 12 May 2025, Nuno Sá via B4 Relay wrote: > > > Hi all, > > > > Here it goes v3. There was some major refactoring in this version due to > > Lee's and Laurent's feedback. There are some splits (and some explicit > > requests) resulting in new patches being added. The biggest change is the > > effort in trying to minimize the usage of specific child device bits in > > the top level device (mainly stuff related to the keymap). I think now > > it's fairly self contained and the only thing that we really need to > > handle in the top device are the unlock and reset events as those can be > > supported through both the input and gpio devices (via gpio_keys). This > > results in a bit of more runtime complexity but well, that's life... > > > > Another change is Lee's suggestion of making use of templates (for > > regmap and chip specific data) and fill things up at probe. > > > > I also refactored a bit the event handling so it's more generic now. > > There were lot's of changes so odds are that I might have forgotten some > > feedback and so, my apologies in advance :). > > > > I also dropped the tags in: > > > > patch 16/22 ("gpio: adp5585: support gpi events") as it has some > > significant changes (replacing .init_valid_masks() with .request() and > > .free()) > > Please run this set through checkpatch.pl before submitting again. > I've done that... It gave some issues but not sure there's anything to be done: ● 26ffbc19b2ce: mfd: adp5585: refactor how regmap defaults are handled ● checkpatch.pl: drivers/mfd/adp5585.c:94: WARNING: struct regmap_config should normally be const ● checkpatch.pl: drivers/mfd/adp5585.c:149: WARNING: struct regmap_config should normally be const The above is something we need given that we want to fill reg defaults during probe. Maybe we can go around that but not sure if it's worth it. ● 6183fc9ef938: gpio: adp5585: add support for the adp5589 expander ● checkpatch.pl: drivers/gpio/gpio-adp5585.c:26: CHECK: Macro argument reuse 'n' - possible side-effects? Again, not sure it's worth it to "fix" it and it was just copy pasted from the header. ● 3e0f3ba80ea5: Input: adp5589: remove the driver ● checkpatch.pl: drivers/input/keyboard/adp5589-keys.c:14: WARNING: added, moved or deleted file(s), does MAINTAINERS need updating? the above is lack of an entry in the original file... > Not sure if we've discussed this, but W=1 wouldn't hurt either. I used the kernel test bot... I assume W=1 is part of the builds. - Nuno Sá
Hi all, Here it goes v3. There was some major refactoring in this version due to Lee's and Laurent's feedback. There are some splits (and some explicit requests) resulting in new patches being added. The biggest change is the effort in trying to minimize the usage of specific child device bits in the top level device (mainly stuff related to the keymap). I think now it's fairly self contained and the only thing that we really need to handle in the top device are the unlock and reset events as those can be supported through both the input and gpio devices (via gpio_keys). This results in a bit of more runtime complexity but well, that's life... Another change is Lee's suggestion of making use of templates (for regmap and chip specific data) and fill things up at probe. I also refactored a bit the event handling so it's more generic now. There were lot's of changes so odds are that I might have forgotten some feedback and so, my apologies in advance :). I also dropped the tags in: patch 16/22 ("gpio: adp5585: support gpi events") as it has some significant changes (replacing .init_valid_masks() with .request() and .free()) Thanks! - Nuno Sá --- Changes in v3: - Patch 2: * New patch (only add devices given in DT). - Patch 5: * Don't include adp5585-keys (still not present at this point). - Patch 6: * Alphabetical order for compatibles. - Patch 7: * New patch. Refactor regmap_config and fill variant differences at probe. - Patch 8: * Rework according to changes introduced in patch 7; * Drop the regs struct in this patch. - Patch 9: * New patch. Add a per chip register structure. - Patch 10: * Moved the per variant gpio register into the gpio driver; * Moved ADP558[59]_GPIO_{BANK_BIT} into the gpio driver; * Moved ADP5589_GPIO_MAX and dropped the max_{col|row}. - Patch 11: * Moved the per variant pwm register into the pwm driver (hence adding a chip_info struct. - Patch 12: * Renamed the -keys suffix in the unlock/reset to events as that's the code we give in dt. - Patch 13: * New patch (add event handling in a more generic way). - Patch 14: * Support reset and unlock events in a separate patch; * Reworked how these events are validated. - Patch 15: * Add support for input devices in it's own patch; * Add a bitmap for marking pins busy so there's no overlaps between the input and gpio devices. - Patch 16: * Drop .init_valid_mask() and use .free() and .request() for checking pin availability; * Drop max_gpios variables as that info is now available from the top device; * Adapt events handling to the new code. - Patch 17: * Moved DT pin parsing into the input driver; * Validate reset/unlock events that are generated by the keymap; * Use error instead of ret; * Drop call to input_set_drvdata(); * Adapt events handling to the new code. - Patch 20: * Add a comment on the reset sleep time. - Link to v2: https://lore.kernel.org/r/20250415-dev-adp5589-fw-v2-0-3a799c3ed812@analog.com - Link to v1: https://lore.kernel.org/r/20250313-dev-adp5589-fw-v1-0-20e80d4bd4ea@analog.com --- Nuno Sá (22): dt-bindings: mfd: adp5585: ease on the required properties mfd: adp5585: only add devices given in FW mfd: adp5585: enable oscilator during probe pwm: adp5585: don't control OSC_EN in the pwm driver mfd: adp5585: make use of MFD_CELL_NAME() dt-bindings: mfd: adp5585: document adp5589 I/O expander mfd: adp5585: refactor how regmap defaults are handled mfd: adp5585: add support for adp5589 mfd: adp5585: add a per chip reg struture gpio: adp5585: add support for the adp5589 expander pwm: adp5585: add support for adp5589 dt-bindings: mfd: adp5585: add properties for input events mfd: adp5585: add support for event handling mfd: adp5585: support reset and unlock events mfd: adp5585: add support for input devices gpio: adp5585: support gpi events Input: adp5585: Add Analog Devices ADP5585/89 support Input: adp5589: remove the driver mfd: adp5585: support getting vdd regulator dt-bindings: mfd: adp5585: document reset gpio mfd: adp5585: add support for a reset pin pwm: adp5585: make sure to include mod_devicetable.h .../devicetree/bindings/mfd/adi,adp5585.yaml | 240 ++++- .../devicetree/bindings/trivial-devices.yaml | 2 - MAINTAINERS | 1 + drivers/gpio/Kconfig | 1 + drivers/gpio/gpio-adp5585.c | 348 ++++++- drivers/input/keyboard/Kconfig | 21 +- drivers/input/keyboard/Makefile | 2 +- drivers/input/keyboard/adp5585-keys.c | 356 +++++++ drivers/input/keyboard/adp5589-keys.c | 1066 -------------------- drivers/mfd/adp5585.c | 794 ++++++++++++++- drivers/pwm/pwm-adp5585.c | 79 +- include/linux/mfd/adp5585.h | 148 ++- 12 files changed, 1852 insertions(+), 1206 deletions(-) --- base-commit: 407f60a151df3c44397e5afc0111eb9b026c38d3 change-id: 20250311-dev-adp5589-fw-e04cfd945286 -- Thanks! - Nuno Sá