On Mon, Oct 13, 2014 at 10:23:30PM +0200, Christophe Ricard wrote:
> Add tpm_stm_st33_i2c dts structure keeping backward compatibility
> with static platform_data support as well.
> In the mean time the code is made much simpler by:
> - Moving all gpio_request to devm_gpio_request_one primitive
> - Moving request_irq to devm_request_threaded_irq
This should move to patch 11, and it doesn't look like threaded_irq is
necessary anymore?
> + pr_err("Failed to retrieve lpcpd-gpios from dts.\n");
All these prints should be dev_err, I think, clinet->dev looks like it
must be valid at this point.
This is a general comment actually, all the pr_* prints look like they
have access to a struct device and should be dev_* prints.
> + /* GPIO request and configuration */
> + r = devm_gpio_request_one(&client->dev, gpio,
> + GPIOF_OUT_INIT_HIGH, "TPM IO LPCPD");
> + if (r) {
> + pr_err("Failed to request lpcpd pin\n");
Ditto
> + return -ENODEV;
Return r?
> + pdata = client->dev.platform_data;
> + if (pdata == NULL) {
> + pr_err("No platform data\n");
> + return -EINVAL;
> + }
It would be nice if the platform data was optional since it is now
the case that the only content (io_lpcd) is optional.
> + if (r) {
> + pr_err("%s : reset gpio_request failed\n", __FILE__);
> + return -ENODEV;
Ditto
> tpm_dev = devm_kzalloc(&client->dev, sizeof(struct tpm_stm_dev),
> GFP_KERNEL);
> if (!tpm_dev) {
> dev_info(&client->dev, "cannot allocate memory for tpm
> data\n");
The print is redundant, kzalloc failure already prints lots of stuff
> + platform_data = client->dev.platform_data;
> + if (!platform_data && client->dev.of_node) {
> + r = tpm_stm_i2c_of_request_resources(chip);
> + if (r) {
> + pr_err("No platform data\n");
If we go down this branch then tpm_stm_i2c_of_request_resources
already printed, so this print is redundant
> + goto _tpm_clean_answer;
> + }
> + } else if (platform_data) {
> + r = tpm_stm_i2c_request_resources(client, chip);
> + if (r) {
> + pr_err("Cannot get platform resources\n");
Ditto
> + goto _tpm_clean_answer;
> + }
> + } else {
> + pr_err("tpm_stm_st33 platform resources not available\n");
> + goto _tpm_clean_answer;
Again, would be nice if platform data was optional
> intmask |= TPM_INTF_CMD_READY_INT
> - | TPM_INTF_FIFO_AVALAIBLE_INT
> - | TPM_INTF_WAKE_UP_READY_INT
> - | TPM_INTF_LOCALITY_CHANGE_INT
> | TPM_INTF_STS_VALID_INT
> | TPM_INTF_DATA_AVAIL_INT;
This hunk should also go to patch 11, I think..
Jason
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
