linux-kernel - Re: [PATCH] HID: i2c-hid: Use device properties (instead of device tree)

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Sun, 01 Oct 2017 19:18:50 +0300
From:   Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
To:     Rajat Jain <rajatja@...gle.com>, Jiri Kosina <jikos@...nel.org>,
        Benjamin Tissoires <benjamin.tissoires@...hat.com>,
        David Arcari <darcari@...hat.com>,
        Mika Westerberg <mika.westerberg@...ux.intel.com>,
        HungNien Chen <hn.chen@...dahitech.com>,
        Hans de Goede <hdegoede@...hat.com>,
        Brian Norris <briannorris@...omium.org>,
        Dmitry Torokhov <dmitry.torokhov@...il.com>, dtor@...gle.com,
        linux-input@...r.kernel.org, linux-kernel@...r.kernel.org,
        rajatxjain@...il.com
Subject: Re: [PATCH] HID: i2c-hid: Use device properties (instead of device
 tree)

On Fri, 2017-09-29 at 15:44 -0700, Rajat Jain wrote:
> Use the device properties (that can be provided by ACPI systems
> as well as non ACPI systems) instead of device tree properties
> (that are not provided ACPI systems). This required some minor
> code restructuring.
> 

> I don't think its a big deal, but just FYI, this changes the order in
> which we
> look for HID register address from
> (device tree -> platform_data -> ACPI) to
> (platform data -> device tree -> ACPI)

I do.

We would like to discourage use of legacy platform data in favour
of Device Tree / ACPI. 

> +static int i2c_hid_fwnode_probe(struct i2c_client *client,
>  		struct i2c_hid_platform_data *pdata)
>  {
>  	struct device *dev = &client->dev;
>  	u32 val;
>  	int ret;
>  
> -	ret = of_property_read_u32(dev->of_node, "hid-descr-addr",
> &val);
> -	if (ret) {
> -		dev_err(&client->dev, "HID register address not
> provided\n");
> -		return -ENODEV;
> -	}
> -	if (val >> 16) {
> -		dev_err(&client->dev, "Bad HID register address:
> 0x%08x\n",
> -			val);
> -		return -EINVAL;
> +	ret = device_property_read_u32(dev, "hid-descr-addr", &val);
> +	if (ret || val >> 16) {
> +		/* Couldn't read using fwnode, try ACPI next */
> +		if (!i2c_hid_acpi_pdata(client, pdata)) {
> +			dev_err(dev, "Bad/Not provided HID register
> address\n");
> +			return -ENODEV;
> +		}

Why not just replace of_ calls by device_ ones?

>  	}
>  	pdata->hid_descriptor_address = val;
>  
> -	ret = of_property_read_u32(dev->of_node, "post-power-on-
> delay-ms",
> -				   &val);
> +	ret = device_property_read_u32(dev, "post-power-on-delay-ms", 
> &val);
>  	if (!ret)
>  		pdata->post_power_delay_ms = val;
>  
>  	return 0;
>  }
> 

Looking how ACPI support is established in the driver, I would rather
NAK this change. Is there any _actual_ hardware on the wild with such
properties?

HID protocol for ACPI is described in [1] where nothing is about _DSD.

[1]: https://docs.microsoft.com/en-us/windows-hardware/drivers/hid/plug-
and-play-support-and-power-management

-- 
Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
Intel Finland Oy