On Sat, Jan 10, 2015 at 12:20:29PM +0100, Christophe Ricard wrote:
For reference:
> -static int tpm_stm_i2c_remove(struct i2c_client *client)
> -{
> - struct tpm_chip *chip =
> - (struct tpm_chip *) i2c_get_clientdata(client);
> -
> - if (chip)
> - tpm_chip_unregister(chip);
> -
> - return 0;
> -}
Became:
> +static int st33zp24_i2c_remove(struct i2c_client *client)
> +{
> + void *tpm_data = i2c_get_clientdata(client);
> +
> + if (tpm_data)
> + st33zp24_remove(tpm_data);
> +
> + return 0;
> +}
The value of i2c_get_clientdata hasn't/can't be changed, it must be
the tpm_chip, so tpm_data should be tpm_chip *, not void *.
The 'if (tpm_data)' is an anti-pattern, please remove it
Same comment applies to patch 9
> +int st33zp24_remove(void *tpm_data)
> +{
> + struct st33zp24_dev *tpm_dev = (struct st33zp24_dev *)tpm_data;
> + struct tpm_chip *chip = tpm_dev->chip;
> +
> + if (chip)
> + tpm_chip_unregister(chip);
> +
> + return 0;
> +}
Now this looks wrong. st33zp24_dev is the TPM_VPRIV of the chip, but
tpm_data is the tpm_chip already, so that cast is surely wrong.
Again, remove the 'if (chip)' anti-pattern.
> +static struct st33zp24_phy_ops i2c_phy_ops = {
> + .send = st33zp24_i2c_send,
> + .recv = st33zp24_i2c_recv,
> +};
Should be 'static const struct', same comment for patch 9
> + tpm_dev = devm_kzalloc(dev, sizeof(struct st33zp24_dev),
> + GFP_KERNEL);
> + if (!tpm_dev)
> + return -ENOMEM;
> +
> + chip = tpmm_chip_alloc(dev, &st33zp24_tpm);
> + if (IS_ERR(chip))
> + return PTR_ERR(chip);
It is idomatic in the TPM drivers for the tpmm_chip_alloc to be first,
then the priv allocation to be second. Someday we might merge the two
calls.
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
