Re: [PATCH v3] platform/x86: add meraki-mx100 platform driver

[Chris <chrisrblake93@xxxxxxxxx>]

 On Mon, Aug 9, 2021 at 7:32 AM Andy Shevchenko
<andy.shevchenko@xxxxxxxxx> wrote:
>
> On Mon, Aug 9, 2021 at 3:29 PM Chris <chrisrblake93@xxxxxxxxx> wrote:
> > On Mon, Aug 9, 2021 at 7:22 AM Andy Shevchenko
> > <andy.shevchenko@xxxxxxxxx> wrote:
> > > On Mon, Aug 9, 2021 at 3:13 PM Chris Blake <chrisrblake93@xxxxxxxxx> wrote:
>
> (I left below context on purpose)
>
> > > > +static struct platform_device * __init tink_create_dev(
> > > > +       const char *name, const void *pdata, size_t sz)
> > > > +{
> > > > +       struct platform_device *pdev;
> > > > +
> > > > +       pdev = platform_device_register_data(NULL,
> > > > +               name, PLATFORM_DEVID_NONE, pdata, sz);
> > > > +       if (IS_ERR(pdev))
> > > > +               pr_err("failed registering %s: %ld\n", name, PTR_ERR(pdev));
> > > > +
> > > > +       return pdev;
> > > > +}
> > > > +
> > > > +static int __init tink_board_init(void)
> > > > +{
> > > > +       if (!dmi_first_match(tink_systems))
> > > > +               return -ENODEV;
> > > > +
> > > > +       /*
> > > > +        * We need to make sure that GPIO60 isn't set to native mode as is default since it's our
> > > > +        * Reset Button. To do this, write to GPIO_USE_SEL2 to have GPIO60 set to GPIO mode.
> > > > +        * This is documented on page 1609 of the PCH datasheet, order number 327879-005US
> > > > +        */
> > > > +       outl(inl(0x530) | BIT(28), 0x530);
> > > > +
> > > > +       gpiod_add_lookup_table(&tink_leds_table);
> > > > +       gpiod_add_lookup_table(&tink_keys_table);
> > > > +
> > > > +       tink_leds_pdev = tink_create_dev("leds-gpio",
> > > > +               &tink_leds_pdata, sizeof(tink_leds_pdata));
> > >
> > > Seems you forgot to add
> > > if (IS_ERR())
> > >   return PTR_ERR();
> > >
> > > here...
> > >
> > > > +       tink_keys_pdev = tink_create_dev("gpio-keys-polled",
> > > > +               &tink_buttons_pdata, sizeof(tink_buttons_pdata));
> > >
> > > and
> > >
> > > if (IS_ERR()) {
> > >  pdev_unreg();
> > >  return PTR_ERR();
> > > }
> > >
> > > here.
> >
> > Are these IS_ERR catches needed since they are done in the
> > tink_create_dev struct? This is the same structure as currently done
> > in drivers/platform/x86/pcengines-apuv2.c. Adding these 2x additions
> > feels a bit redundant, but if I am wrong please correct me.
>
> Care to describe how it's supposed to be taken into account in your opinion?

I don't mind adding those blocks in, I am just trying to understand
the benefit. In tink_create_dev() we do if (IS_ERR(pdev)) right after
platform_device_register_data is called, so I am just curious why we
would add another if (IS_ERR()) conditional after the 2x calls to
tink_create_dev(). I don't have a ton of work in the kernel, so
knowing why this was requested will help me improve any future PRs.

>
> > > > +       return 0;
> > > > +}
>
>
> --
> With Best Regards,
> Andy Shevchenko