On Tue, Apr 03, 2018 at 04:31:32PM +0300, Matan Ziv-Av wrote:
Matan, thanks for taking the time to prepare this driver and submit it
to the list. We appreciate that there are a lot of rules to follow, and
they can be especially daunting with your first patches.
In our experience it is much harder to get needed changes in after
something is merged, so please stick with us.
> On Mon, 2 Apr 2018, Andy Shevchenko wrote:
>
> > > You have too many subtle issues, such as:
> > > - redundant empty lines
> > > - missed empty lines
> > > - redundant conditionals (before kstrtox() call)
> > > - shadowing returned error codes (kstrtox() usage)
> > > - etc, I even would not like to look at
> > >
> > > So, please, run checkpatch.pl and see if it complains about anything.
>
> I fixed some of the above issues, and it passes checkpatch.pl.
>
> > >
> > > Besides that, the approaches you choose might be old or racy:
> > > - device attribute handling (use sysfs attribute group)
>
> I use those now.
>
> > > - btw, where is the ABI description?
>
> Added.
>
> > > Why did you choose so complex way to instantiate a driver? All those
>
> I guess that this question is rhetorical, but if not, I wrote this
> driver mainly by cutting and pasting from other drivers, and layer by
> layer (hot keys, then leds, then control devices).
>
> > > ACPI handlers and additional burden for platform device...
> > > Can it be simplified?
>
> I simplified it a little.
>
> > > Can you utilize WMI bus as much as possible.
>
> Of the five hot keys, four are reported as WMI events, but one only as
> an ACPI event, so I don't think it is possible to improve there.
>
> The method used for control (WMAB), has three integer inputs, but as far
> as I understand wmi_evaluate_method(), the third parameter there is only
> a buffer or a string, so I cannot use WMI to call this method.
Integers can be embedded in buffers, I'd need to see the WMAB from the
DSDT. Look for other uses of wmi_evaluate_method, and you'll find some
examples. e.g. drivers/gpu/drm/nouveau/nvkm/subdev/mxm/base.c
wmi_wmmx_mxmi() uses:
u32 mxmi_args[] = { 0x494D584D, version, 0 };
struct acpi_buffer args = {sizeof(mxmi_args), mxmi_args };
...
wmi_wvaluate_method(WMI_WMMX_GUID, 0m 0, &args, $&retn };
--
Darren Hart
VMware Open Source Technology Center
