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

[Andy Shevchenko <andy.shevchenko@gmail.com>]

On Mon, Aug 9, 2021 at 3:29 PM Chris <chrisrblake93@gmail.com> wrote:
> On Mon, Aug 9, 2021 at 7:22 AM Andy Shevchenko
> <andy.shevchenko@gmail.com> wrote:
> > On Mon, Aug 9, 2021 at 3:13 PM Chris Blake <chrisrblake93@gmail.com> 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?

> > > +       return 0;
> > > +}


-- 
With Best Regards,
Andy Shevchenko