Received-SPF: softfail (google.com: domain of transitioning linux-kernel-owner@vger.kernel.org does not designate 23.128.96.19 as permitted sender) client-ip=23.128.96.19;
Date:   Mon, 23 May 2022 13:34:55 +0800
From:   Tzung-Bi Shih <tzungbi@kernel.org>
To:     Matthias Kaehlcke <mka@chromium.org>
Cc:     bleung@chromium.org, groeck@chromium.org, robh+dt@kernel.org,
        chrome-platform@lists.linux.dev, devicetree@vger.kernel.org,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH v3 5/5] platform/chrome: cros_kbd_led_backlight: support
 EC PWM backend
Message-ID: <Yosc/6izBDYYKpFC@google.com>
References: <20220321085547.1162312-1-tzungbi@kernel.org>
 <20220321085547.1162312-6-tzungbi@kernel.org>
 <YobHVST2Nfn+z8n6@google.com>
 <YocewB/lLJhIAuQP@google.com>
 <YoezzzLdVfb0K7Ak@google.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <YoezzzLdVfb0K7Ak@google.com>
Precedence: bulk

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.