| Message ID | 20231009172923.2457844-19-robh@kernel.org (mailing list archive) |
|---|---|
| State | Handled Elsewhere, archived |
| Headers | show |
| Series | power: reset: vexpress: Use device_get_match_data() | expand |
Hi Rob, On Mon, Oct 09, 2023 at 12:29:14PM -0500, Rob Herring wrote: > Use preferred device_get_match_data() instead of of_match_device() to > get the driver match data. With this, adjust the includes to explicitly > include the correct headers. > > Signed-off-by: Rob Herring <robh@kernel.org> > --- > drivers/power/reset/vexpress-poweroff.c | 11 ++++------- > 1 file changed, 4 insertions(+), 7 deletions(-) > > diff --git a/drivers/power/reset/vexpress-poweroff.c b/drivers/power/reset/vexpress-poweroff.c > index 17064d7b19f6..bb22b2db5907 100644 > --- a/drivers/power/reset/vexpress-poweroff.c > +++ b/drivers/power/reset/vexpress-poweroff.c > @@ -7,8 +7,8 @@ > #include <linux/delay.h> > #include <linux/notifier.h> > #include <linux/of.h> > -#include <linux/of_device.h> > #include <linux/platform_device.h> > +#include <linux/property.h> > #include <linux/reboot.h> > #include <linux/stat.h> > #include <linux/vexpress.h> > @@ -108,20 +108,17 @@ static int _vexpress_register_restart_handler(struct device *dev) > > static int vexpress_reset_probe(struct platform_device *pdev) > { > - const struct of_device_id *match = > - of_match_device(vexpress_reset_of_match, &pdev->dev); > + enum vexpress_reset_func func; > struct regmap *regmap; > int ret = 0; > > - if (!match) > - return -EINVAL; > - > regmap = devm_regmap_init_vexpress_config(&pdev->dev); > if (IS_ERR(regmap)) > return PTR_ERR(regmap); > dev_set_drvdata(&pdev->dev, regmap); > > - switch ((uintptr_t)match->data) { > + func = (uintptr_t)device_get_match_data(&pdev->dev); > + switch (func) { > case FUNC_SHUTDOWN: > vexpress_power_off_device = &pdev->dev; > pm_power_off = vexpress_power_off; device_get_match_data() is badly un-documented but I think it can still return NULL if no match. At the moment we're checking for a match earlier and avoid calling devm_regmap_init_vexpress_config() needlessly, with your patch not so. Can you not replace each line with the equivalent code and keep the NULL check? Best regards, Liviu > -- > 2.42.0 >
On Mon, Oct 09, 2023 at 11:07:33PM +0100, Liviu Dudau wrote: > Hi Rob, > > On Mon, Oct 09, 2023 at 12:29:14PM -0500, Rob Herring wrote: > > Use preferred device_get_match_data() instead of of_match_device() to > > get the driver match data. With this, adjust the includes to explicitly > > include the correct headers. > > > > Signed-off-by: Rob Herring <robh@kernel.org> > > --- > > drivers/power/reset/vexpress-poweroff.c | 11 ++++------- > > 1 file changed, 4 insertions(+), 7 deletions(-) > > > > diff --git a/drivers/power/reset/vexpress-poweroff.c b/drivers/power/reset/vexpress-poweroff.c > > index 17064d7b19f6..bb22b2db5907 100644 > > --- a/drivers/power/reset/vexpress-poweroff.c > > +++ b/drivers/power/reset/vexpress-poweroff.c > > @@ -7,8 +7,8 @@ > > #include <linux/delay.h> > > #include <linux/notifier.h> > > #include <linux/of.h> > > -#include <linux/of_device.h> > > #include <linux/platform_device.h> > > +#include <linux/property.h> > > #include <linux/reboot.h> > > #include <linux/stat.h> > > #include <linux/vexpress.h> > > @@ -108,20 +108,17 @@ static int _vexpress_register_restart_handler(struct device *dev) > > > > static int vexpress_reset_probe(struct platform_device *pdev) > > { > > - const struct of_device_id *match = > > - of_match_device(vexpress_reset_of_match, &pdev->dev); > > + enum vexpress_reset_func func; > > struct regmap *regmap; > > int ret = 0; > > > > - if (!match) > > - return -EINVAL; > > - > > regmap = devm_regmap_init_vexpress_config(&pdev->dev); > > if (IS_ERR(regmap)) > > return PTR_ERR(regmap); > > dev_set_drvdata(&pdev->dev, regmap); > > > > - switch ((uintptr_t)match->data) { > > + func = (uintptr_t)device_get_match_data(&pdev->dev); > > + switch (func) { > > case FUNC_SHUTDOWN: > > vexpress_power_off_device = &pdev->dev; > > pm_power_off = vexpress_power_off; > > device_get_match_data() is badly un-documented but I think it can still > return NULL if no match. At the moment we're checking for a match earlier > and avoid calling devm_regmap_init_vexpress_config() needlessly, with your > patch not so. Can you not replace each line with the equivalent code and > keep the NULL check? In contrast, I always questioned/wondered why we needed the NULL check for the match above as probe would have not got called if there was no match. While I agree with the less clear documentation of device_get_match_data(), I am fine with the change as such Acked-by: Sudeep Holla <sudeep.holla@arm.com> Also check similar response from Rob here[1] Sebastian, I assume you will pick this up. -- Regards, Sudeep [1] https://lore.kernel.org/all/CAL_JsqKi8PXVqHgVxqMN+mbX8U-ZGsCMUFqbxmjeFmj1xKTrjw@mail.gmail.com/
On Mon, 09 Oct 2023 12:29:14 -0500, Rob Herring wrote: > Use preferred device_get_match_data() instead of of_match_device() to > get the driver match data. With this, adjust the includes to explicitly > include the correct headers. > > Applied, thanks! [1/1] power: reset: vexpress: Use device_get_match_data() commit: 469d31745b9fb3a87424b311abb7cb530611404f Best regards,
diff --git a/drivers/power/reset/vexpress-poweroff.c b/drivers/power/reset/vexpress-poweroff.c index 17064d7b19f6..bb22b2db5907 100644 --- a/drivers/power/reset/vexpress-poweroff.c +++ b/drivers/power/reset/vexpress-poweroff.c @@ -7,8 +7,8 @@ #include <linux/delay.h> #include <linux/notifier.h> #include <linux/of.h> -#include <linux/of_device.h> #include <linux/platform_device.h> +#include <linux/property.h> #include <linux/reboot.h> #include <linux/stat.h> #include <linux/vexpress.h> @@ -108,20 +108,17 @@ static int _vexpress_register_restart_handler(struct device *dev) static int vexpress_reset_probe(struct platform_device *pdev) { - const struct of_device_id *match = - of_match_device(vexpress_reset_of_match, &pdev->dev); + enum vexpress_reset_func func; struct regmap *regmap; int ret = 0; - if (!match) - return -EINVAL; - regmap = devm_regmap_init_vexpress_config(&pdev->dev); if (IS_ERR(regmap)) return PTR_ERR(regmap); dev_set_drvdata(&pdev->dev, regmap); - switch ((uintptr_t)match->data) { + func = (uintptr_t)device_get_match_data(&pdev->dev); + switch (func) { case FUNC_SHUTDOWN: vexpress_power_off_device = &pdev->dev; pm_power_off = vexpress_power_off;
Use preferred device_get_match_data() instead of of_match_device() to get the driver match data. With this, adjust the includes to explicitly include the correct headers. Signed-off-by: Rob Herring <robh@kernel.org> --- drivers/power/reset/vexpress-poweroff.c | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-)