Re: [PATCH 1/4] clk: rs9: Check for vendor/device ID

[Marek Vasut <marex@denx.de>]

On 1/3/23 13:31, Alexander Stein wrote:
> This is in preparation to support additional devices which have different
> IDs as well as a slightly different register layout.
> 
> Signed-off-by: Alexander Stein <alexander.stein@ew.tq-group.com>
> ---
>   drivers/clk/clk-renesas-pcie.c | 24 ++++++++++++++++++++++++
>   1 file changed, 24 insertions(+)
> 
> diff --git a/drivers/clk/clk-renesas-pcie.c b/drivers/clk/clk-renesas-pcie.c
> index e6247141d0c0..0076ed8f11b0 100644
> --- a/drivers/clk/clk-renesas-pcie.c
> +++ b/drivers/clk/clk-renesas-pcie.c
> @@ -45,6 +45,13 @@
>   #define RS9_REG_DID				0x6
>   #define RS9_REG_BCP				0x7
>   
> +#define RS9_REG_VID_IDT				0x01
> +
> +#define RS9_REG_DID_TYPE_FGV			(0x0 << RS9_REG_DID_TYPE_SHIFT)
> +#define RS9_REG_DID_TYPE_DBV			(0x1 << RS9_REG_DID_TYPE_SHIFT)
> +#define RS9_REG_DID_TYPE_DMV			(0x2 << RS9_REG_DID_TYPE_SHIFT)

I'm not entirely sure whether this shouldn't be using the BIT() macro, 
what do you think ?

> +#define RS9_REG_DID_TYPE_SHIFT			0x6
> +
>   /* Supported Renesas 9-series models. */
>   enum rs9_model {
>   	RENESAS_9FGV0241,
> @@ -54,6 +61,7 @@ enum rs9_model {
>   struct rs9_chip_info {
>   	const enum rs9_model	model;
>   	unsigned int		num_clks;
> +	u8			did;

Should this be const (and also the num_clks) ?

>   };
>   
>   struct rs9_driver_data {
> @@ -270,6 +278,7 @@ static int rs9_probe(struct i2c_client *client)
>   {
>   	unsigned char name[5] = "DIF0";
>   	struct rs9_driver_data *rs9;
> +	unsigned int vid, did;
>   	struct clk_hw *hw;
>   	int i, ret;
>   
> @@ -306,6 +315,20 @@ static int rs9_probe(struct i2c_client *client)
>   	if (ret < 0)
>   		return ret;
>   
> +	ret = regmap_read(rs9->regmap, RS9_REG_VID, &vid);
> +	if (ret < 0)
> +		return ret;

Newline here.

> +	ret = regmap_read(rs9->regmap, RS9_REG_DID, &did);
> +	if (ret < 0)
> +		return ret;
> +
> +	if ((vid != RS9_REG_VID_IDT) || (did != rs9->chip_info->did)) {

Drop the unnecessary inner () parenthesis .

> +		dev_err(&client->dev,

return dev_err_probe() might work better here ?

> +			"Incorrect VID/DID: %#02x, %#02x. Expected %#02x, %#02x\n",
> +			vid, did, RS9_REG_VID_IDT, rs9->chip_info->did);
> +		return -ENODEV;
> +	}

[...]