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@xxxxxxxxxxx>
> > ---
> > 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
