On Fri, May 20, 2022 at 08:29:19AM -0700, Matthias Kaehlcke wrote:
> On Fri, May 20, 2022 at 12:53:20PM +0800, Tzung-Bi Shih wrote:
> > On Thu, May 19, 2022 at 03:40:21PM -0700, Matthias Kaehlcke wrote:
> > > On Mon, Mar 21, 2022 at 04:55:47PM +0800, Tzung-Bi Shih wrote:
> > > > +struct keyboard_led_private {
> > >
> > > Why 'private', isn't this more a 'cros_ec_kdb_bl' or similar?
> >
> > It is just drvdata.
>
> The data structure represents an instance of the device, as such it
> is an important part of the driver, drvdata is just a way to attach
> it to the platform device.
>
> > I would prefer to keep the original prefix "keyboard_led_" if you wouldn't
> > have strong opinion.
>
> I'm fine with 'keyboard_led', but object to the 'private' part. In the
> kernel 'private' fields are typically used when a driver consists of a
> generic part and a device specific part. The driver has a 'private'
> void* field that points to a device specific data structure about which
> the generic driver is agnostic. This data structure is only used by the
> device specific implementation. That isn't the case here, so naming the
> structure anything 'private' is misleading.
The struct in the case is device specific. I don't see a problem to name it
*private* as there are a lot of more existing examples.
$ grep -R 'struct .*_priv.* {' drivers/
drivers/pinctrl/bcm/pinctrl-bcm6358.c:struct bcm6358_priv {
$ grep -R 'struct .*_priv.* {' sound/soc/codecs/
sound/soc/codecs/rt286.c:struct rt286_priv {
I would get rid of the term "private" if it could be confusing.
> > > > +static int keyboard_led_init_ec_pwm(struct platform_device *pdev)
> > > > +{
> > > > + struct keyboard_led_private *private = platform_get_drvdata(pdev);
> > > > +
> > > > + private->ec = dev_get_drvdata(pdev->dev.parent);
> > > > + if (!private->ec) {
> > > > + dev_err(&pdev->dev, "no parent EC device\n");
> > > > + return -EINVAL;
> > > > + }
> > >
> > > The only thing this 'init' function does is assigning private->ec. Wouldn't
> > > it be clearer to do this directly in probe() from where callback is called?
> > > It could be with the condition that the device as a DT node.
> >
> > No. The probe() isn't aware of the device is from ACPI or OF.
>
> But it could be:
>
> if (pdev->dev.of_node)
> kbd_led->ec = dev_get_drvdata(pdev->dev.parent);
The 'init' callback isn't only for OF but also ACPI. I would prefer to keep
the 'init' function and let probe() have no awareness about them.
> > > Is it actually possible that the keyboard backlight device gets instantiated
> > > if there is no EC parent?
> >
> > It shouldn't be but just in case.
>
> If this can only occur due to an error in common kernel frameworks then
> the check should be omitted IMO.
The check is referenced from [1]. I would prefer to keep it instead of
crashing kernel if anything went wrong.
[1]: https://elixir.bootlin.com/linux/v5.18-rc7/source/drivers/pwm/pwm-cros-ec.c#L244
>
> > > > +static const struct keyboard_led_drvdata keyboard_led_drvdata_ec_pwm = {
> > > > + .init = keyboard_led_init_ec_pwm_null,
> > >
> > > Is this really needed?
> > >
> > > keyboard_led_probe() checks if .init is assigned before invoking the callback:
> > >
> > > if (drvdata->init) {
> > > error = drvdata->init(pdev);
> > >
> > > The whole 'else' branch could be eliminated if .of_match_table of the driver
> > > only is assigned when CONFIG_CROS_KBD_LED_BACKLIGHT_EC_PWM is set. IMO that
> > > would preferable over creating 'stubs'.
> >
> > CONFIG_CROS_KBD_LED_BACKLIGHT_EC_PWM and CONFIG_OF are independent. The stubs
> > were created to avoid compile errors if CONFIG_OF=y but
> > CONFIG_CROS_KBD_LED_BACKLIGHT_EC_PWM=n.
>
> Is there functional version of the driver that uses instantiation through the
> device tree if CONFIG_CROS_KBD_LED_BACKLIGHT_EC_PWM=n? If not .of_match_table
> should not be assigned.
CONFIG_CROS_KBD_LED_BACKLIGHT_EC_PWM and CONFIG_OF are independent.
CONFIG_CROS_KBD_LED_BACKLIGHT_EC_PWM is also designed to work with CONFIG_ACPI.
