| Message ID | 1600276053-3336-1-git-send-email-jeff@labundy.com |
|---|---|
| State | New |
| Headers | show |
| Series | [RESEND] input: iqs62x-keys: Remove superfluous function parameter | expand |
Hi Jeff, On Wed, Sep 16, 2020 at 12:07:33PM -0500, Jeff LaBundy wrote: > It is not necessary to pass iqs62x_keys to iqs62x_keys_parse_prop, > because it can already be derived from the platform_device cookie. Yes, it can be derived, but why is better to have it derived rather than passed in? Is the code smaller this way? > > Signed-off-by: Jeff LaBundy <jeff@labundy.com> > --- > drivers/input/keyboard/iqs62x-keys.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/drivers/input/keyboard/iqs62x-keys.c b/drivers/input/keyboard/iqs62x-keys.c > index 93446b2..e2a2b38 100644 > --- a/drivers/input/keyboard/iqs62x-keys.c > +++ b/drivers/input/keyboard/iqs62x-keys.c > @@ -42,9 +42,9 @@ struct iqs62x_keys_private { > u8 interval; > }; > > -static int iqs62x_keys_parse_prop(struct platform_device *pdev, > - struct iqs62x_keys_private *iqs62x_keys) > +static int iqs62x_keys_parse_prop(struct platform_device *pdev) > { > + struct iqs62x_keys_private *iqs62x_keys = platform_get_drvdata(pdev); > struct fwnode_handle *child; > unsigned int val; > int ret, i; > @@ -258,7 +258,7 @@ static int iqs62x_keys_probe(struct platform_device *pdev) > > platform_set_drvdata(pdev, iqs62x_keys); > > - ret = iqs62x_keys_parse_prop(pdev, iqs62x_keys); > + ret = iqs62x_keys_parse_prop(pdev); > if (ret) > return ret; > > -- > 2.7.4 > Thanks.
Hi Dmitry, Thanks for taking a look. On Wed, Sep 16, 2020 at 10:25:59AM -0700, Dmitry Torokhov wrote: > Hi Jeff, > > On Wed, Sep 16, 2020 at 12:07:33PM -0500, Jeff LaBundy wrote: > > It is not necessary to pass iqs62x_keys to iqs62x_keys_parse_prop, > > because it can already be derived from the platform_device cookie. > > Yes, it can be derived, but why is better to have it derived rather than > passed in? Is the code smaller this way? > I think it is better practice to limit the function's parameters to only those that are minimally necessary. In this particular case we only want to populate data in the same iqs62x_keys struct that was allocated using the original pdev->dev instance, so it seems safer to enforce this by only offering a single parameter instead of allowing them to be separate. That's all but guaranteed not to be a problem in this driver; it simply caught my attention while re-using this code in another project. I would be happy to add more details in the commit message, but it is also fine for this patch to be dropped if you prefer. > > > > Signed-off-by: Jeff LaBundy <jeff@labundy.com> > > --- > > drivers/input/keyboard/iqs62x-keys.c | 6 +++--- > > 1 file changed, 3 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/input/keyboard/iqs62x-keys.c b/drivers/input/keyboard/iqs62x-keys.c > > index 93446b2..e2a2b38 100644 > > --- a/drivers/input/keyboard/iqs62x-keys.c > > +++ b/drivers/input/keyboard/iqs62x-keys.c > > @@ -42,9 +42,9 @@ struct iqs62x_keys_private { > > u8 interval; > > }; > > > > -static int iqs62x_keys_parse_prop(struct platform_device *pdev, > > - struct iqs62x_keys_private *iqs62x_keys) > > +static int iqs62x_keys_parse_prop(struct platform_device *pdev) > > { > > + struct iqs62x_keys_private *iqs62x_keys = platform_get_drvdata(pdev); > > struct fwnode_handle *child; > > unsigned int val; > > int ret, i; > > @@ -258,7 +258,7 @@ static int iqs62x_keys_probe(struct platform_device *pdev) > > > > platform_set_drvdata(pdev, iqs62x_keys); > > > > - ret = iqs62x_keys_parse_prop(pdev, iqs62x_keys); > > + ret = iqs62x_keys_parse_prop(pdev); > > if (ret) > > return ret; > > > > -- > > 2.7.4 > > > > Thanks. > > -- > Dmitry Kind regards, Jeff LaBundy
diff --git a/drivers/input/keyboard/iqs62x-keys.c b/drivers/input/keyboard/iqs62x-keys.c index 93446b2..e2a2b38 100644 --- a/drivers/input/keyboard/iqs62x-keys.c +++ b/drivers/input/keyboard/iqs62x-keys.c @@ -42,9 +42,9 @@ struct iqs62x_keys_private { u8 interval; }; -static int iqs62x_keys_parse_prop(struct platform_device *pdev, - struct iqs62x_keys_private *iqs62x_keys) +static int iqs62x_keys_parse_prop(struct platform_device *pdev) { + struct iqs62x_keys_private *iqs62x_keys = platform_get_drvdata(pdev); struct fwnode_handle *child; unsigned int val; int ret, i; @@ -258,7 +258,7 @@ static int iqs62x_keys_probe(struct platform_device *pdev) platform_set_drvdata(pdev, iqs62x_keys); - ret = iqs62x_keys_parse_prop(pdev, iqs62x_keys); + ret = iqs62x_keys_parse_prop(pdev); if (ret) return ret;
It is not necessary to pass iqs62x_keys to iqs62x_keys_parse_prop, because it can already be derived from the platform_device cookie. Signed-off-by: Jeff LaBundy <jeff@labundy.com> --- drivers/input/keyboard/iqs62x-keys.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)