Date:   Thu, 16 Feb 2023 12:17:09 +1100
From:   Orlando Chamberlain <orlandoch.dev@gmail.com>
To:     Andy Shevchenko <andy@infradead.org>
Cc:     Aditya Garg <gargaditya08@live.com>,
        Jiri Kosina <jikos@kernel.org>,
        "jkosina@suse.cz" <jkosina@suse.cz>,
        "benjamin.tissoires@redhat.com" <benjamin.tissoires@redhat.com>,
        LKML <linux-kernel@vger.kernel.org>,
        "linux-input@vger.kernel.org" <linux-input@vger.kernel.org>,
        "ronald@innovation.ch" <ronald@innovation.ch>,
        "kekrby@gmail.com" <kekrby@gmail.com>
Subject: Re: [PATCH 3/3] HID: apple-magic-backlight: Add driver for keyboard
 backlight on internal Magic Keyboards
Message-ID: <20230216121709.04c76b9d@redecorated-mbp>
In-Reply-To: <Y+jLCebet+aekGRv@smile.fi.intel.com>
References: <E5D8BEBA-3C5B-460F-BD2C-39470A793CC3@live.com>
        <40274C3D-4F4F-479C-944C-EEBDC78F959C@live.com>
        <868AA58D-2399-4E4A-A6C6-73F88DB13992@live.com>
        <7D70F1FE-7F54-4D0A-8922-5466AA2AD364@live.com>
        <Y+jLCebet+aekGRv@smile.fi.intel.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Precedence: bulk

On Sun, 12 Feb 2023 13:18:33 +0200
Andy Shevchenko <andy@infradead.org> wrote:

> On Fri, Feb 10, 2023 at 03:45:15AM +0000, Aditya Garg wrote:
> > From: Orlando Chamberlain <orlandoch.dev@gmail.com>
> > 
> > This driver adds support for the keyboard backlight on Intel T2 Macs
> > with internal Magic Keyboards (MacBookPro16,x and MacBookAir9,1)  
> 
> ...
> 
> > +#include <linux/hid.h>
> > +#include <linux/usb.h>  
> 
> Seems lack of some header inclusions, e.g. where struct led_classdev
> is defined or -ERRNO codes.
> 
> > +#include "hid-ids.h"  
> 
> ...
> 
> > +static void apple_magic_backlight_power_set(struct
> > apple_magic_backlight *backlight,
> > +					   char power, char rate)  
> 
> char is a beast, can we use u8 here and in similar cases?
> 
> ...
> 
> > +	/* Ensure this usb endpoint is for the keyboard backlight,
> > not touchbar
> > +	 * backlight.
> > +	 */  
> 
> /*
>  * Multi-line comment style
>  * goes like this.
>  */
> 
> ...
> 
> > +	backlight = devm_kzalloc(&hdev->dev, sizeof(*backlight),
> > GFP_KERNEL);  
> 
> > +  
> 
> Redundant blank line.
> 
> > +	if (!backlight)
> > +		return -ENOMEM;  
> 
> ...
> 
> > +static struct hid_driver apple_magic_backlight_hid_driver = {
> > +	.name = "apple-magic-backlight",
> > +	.id_table = apple_magic_backlight_hid_ids,
> > +	.probe = apple_magic_backlight_probe,
> > +	.remove = apple_magic_backlight_remove,
> > +};  
> 
> > +  
> 
> Redundant blank line.
> 
> > +module_hid_driver(apple_magic_backlight_hid_driver);  
> 

Thanks for pointing out all these, I'll make those changes in v2.