Hi Roderick,
On Fri, Jan 18, 2019 at 7:16 AM Roderick Colenbrander
<thunderbird2k@xxxxxxxxx> wrote:
>
> Hi Daniel,
>
> Thanks for sharing your updated driver. It is easier to understand
> than the first version. See some of my initial feedback inline.
Thanks for the review, this makes my life much simpler :)
>
> BTW one feature I missed, but please add it in a separate patch later
> on would be battery level. Use powersupply class for that later.
>
> Thanks,
> Roderick
>
>
> On Thu, Jan 17, 2019 at 7:29 PM Daniel J. Ogorchock
> <djogorchock@xxxxxxxxx> wrote:
> >
[snipped]
> > +static struct switchcon_ctlr *switchcon_ctlr_create(struct hid_device *hdev)
> > +{
> > + struct switchcon_ctlr *ctlr;
> > +
> > + ctlr = devm_kzalloc(&hdev->dev, sizeof(*ctlr), GFP_KERNEL);
> > + if (!ctlr)
> > + return ERR_PTR(-ENOMEM);
> > +
> > + switch (hdev->product) {
> > + case USB_DEVICE_ID_NINTENDO_PROCON:
> > + hid_info(hdev, "detected pro controller\n");
>
> I wonder if it makes sense to print the device. If I recall the input
> layer already prints the exact device name and other information on
> hotplug.
Yep, there is no point putting this info here (unless you expect the
device to fail afterwards, but I guess this is not an issue not
knowing which device we have).
Also, given that we have a PID/device matching, there is no point in
putting the info.
>
> > + ctlr->type = SWITCHCON_CTLR_TYPE_PROCON;
>
> I'll leave it up to the others to see what they think, but since you
> have the hdev anyway, there is no real need to set this variables, you
> can always compare against "hdev->product". Not sure what the practice
> is in other drivers. Some drivers like hid-sony, store this data in
> ".driver_data" of the hid_devices struct at the bottom of your driver.
> They then copy it out within probe. You could e.g. pass
> "hid_device_id" to this function
Well, if there is a one-to-one mapping between the PID and the device,
then using the PID is probably the best.
When there is a many-to-one, (like several PIDs for the
DUALSHOCK4_CONTROLLER_BT for example), then storing a type makes sense
as it avoids sprinkling the driver with multiple switch/case of PIDs
and forget to add one PID to the list.
Given that we reuse the information in the event processing, we need
to keep that data available somewhere. So if Nintendo allows third
party makers to provide JoyCons compatible devices, then using the
current codfe is probably the best in the long run.
Cheers,
Benjamin
