Re: [DIGImend-devel] [PATCH 4/4] HID: uclogic: name the input nodes based on their tool

[Benjamin T. <ben...@re...>]

Hi Dan,

On Feb 25 2015 or thereabouts, Dan Roberts wrote:
> If you'll please excuse my ignorance, I'm not in a position to thoroughly
> confirm this for a few hours, and I wanted to comment ASAP. I believe
> hid_input->name does not get freed. It directly points to the embedded
> array in the hid_device, assigned in hidinput_allocate (). So I think it
> makes more sense to modify hid_device->name?

No, and no :)

So, the string will get freed because it is allocated with the managed
ressource (devm_kzalloc). As soon as the device gets destroyed, it will
free all of the associated memory.

The problem of changing hid_device->name is that the name is shared
between all subdevices that are created. We create one subdevice per
report, so they will all have a different name. Last, the hid name is
the one presented for the hidraw node and in some cases (not here
actually), the kernel can show the hidraw node first and then later the
input node. If we change the hidraw node in the back of the user, that
would not be of the best effect.

Cheers,
Benjamin

> 
> Thanks,
> Dan
> We append "Pen", "Pad", "Mouse" or "Keyboard" suffix to the appropriate
> input node to match what the Wacom driver does and be more convenient for
> the user to know which one is which.
> 
> Signed-off-by: Benjamin Tissoires <ben...@re...>
> ---
>  drivers/hid/hid-uclogic.c | 40 ++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 40 insertions(+)
> 
> diff --git a/drivers/hid/hid-uclogic.c b/drivers/hid/hid-uclogic.c
> index c03e076..7042238 100644
> --- a/drivers/hid/hid-uclogic.c
> +++ b/drivers/hid/hid-uclogic.c
> @@ -713,6 +713,45 @@ static __u8 *uclogic_report_fixup(struct hid_device
> *hdev, __u8 *rdesc,
>         return rdesc;
>  }
> 
> +static void uclogic_input_configured(struct hid_device *hdev,
> +               struct hid_input *hi)
> +{
> +       char *name;
> +       const char *suffix = NULL;
> +       struct hid_field *field;
> +       size_t len;
> +
> +       /* no report associated (HID_QUIRK_MULTI_INPUT not set) */
> +       if (!hi->report)
> +               return;
> +
> +       field = hi->report->field[0];
> +
> +       switch (field->application) {
> +       case HID_GD_KEYBOARD:
> +               suffix = "Keyboard";
> +               break;
> +       case HID_GD_MOUSE:
> +               suffix = "Mouse";
> +               break;
> +       case HID_GD_KEYPAD:
> +               suffix = "Pad";
> +               break;
> +       case HID_DG_PEN:
> +               suffix = "Pen";
> +               break;
> +       }
> +
> +       if (suffix) {
> +               len = strlen(hdev->name) + 2 + strlen(suffix);
> +               name = devm_kzalloc(&hi->input->dev, len, GFP_KERNEL);
> +               if (name) {
> +                       snprintf(name, len, "%s %s", hdev->name, suffix);
> +                       hi->input->name = name;
> +               }
> +       }
> +}
> +
>  /**
>   * Enable fully-functional tablet mode and determine device parameters.
>   *
> @@ -906,6 +945,7 @@ static struct hid_driver uclogic_driver = {
>         .probe = uclogic_probe,
>         .report_fixup = uclogic_report_fixup,
>         .raw_event = uclogic_raw_event,
> +       .input_configured = uclogic_input_configured,
>  };
>  module_hid_driver(uclogic_driver);
> 
> --
> 2.1.0
> 
> 
> ------------------------------------------------------------------------------
> Dive into the World of Parallel Programming The Go Parallel Website,
> sponsored
> by Intel and developed in partnership with Slashdot Media, is your hub for
> all
> things parallel software development, from weekly thought leadership blogs
> to
> news, videos, case studies, tutorials and more. Take a look and join the
> conversation now. http://goparallel.sourceforge.net/
> _______________________________________________
> DIGImend-devel mailing list
> DIG...@li...
> https://lists.sourceforge.net/lists/listinfo/digimend-devel