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
