Message-ID: <Yo03b5GtGgF/kVb3@google.com>
Date:   Tue, 24 May 2022 12:52:15 -0700
From:   Matthias Kaehlcke <mka@...omium.org>
To:     Tzung-Bi Shih <tzungbi@...nel.org>
Cc:     bleung@...omium.org, groeck@...omium.org, robh+dt@...nel.org,
        chrome-platform@...ts.linux.dev, devicetree@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3 5/5] platform/chrome: cros_kbd_led_backlight: support
 EC PWM backend

On Mon, May 23, 2022 at 01:34:55PM +0800, Tzung-Bi Shih wrote:
> 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.

Ok, I hadn't come across that use of 'private' yet, but apparently it's not
that uncommon. Personally I prefer variables with specific names, but maybe
for others 'private' just spells out clearly what a struct is about ¯\_(ツ)_/¯

> > > > > +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.

Ah ok, then it makes sense, I think my brain conflated this with the unnecessary
keyboard_led_init_acpi_null stub.

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

Ok, maybe there are cases where the EC parent could go away. Let's hope it
doesn't happen after the keyboard backlight got probed :)

> > 
> > > > > +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.

I understand that CONFIG_CROS_KBD_LED_BACKLIGHT_EC_PWM also works with
CONFIG_ACPI, but in that case the driver uses .acpi_match_table, not
.of_match_table. So you wouldn't have to define 'keyboard_led_of_match'
if you did this:

static struct platform_driver keyboard_led_driver = {
        .driver         = {
                .name   = "chromeos-keyboard-leds",
                .acpi_match_table = ACPI_PTR(keyboard_led_acpi_match),
#ifdef CONFIG_OF
		.of_match_table = of_match_ptr(keyboard_led_of_match),
#endif
        },
        .probe          = keyboard_led_probe,
;

Powered by blists - more mailing lists