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

[Alexander Stein <alexander.stein@ew.tq-group.com>]

Am Dienstag, 3. Januar 2023, 15:28:16 CET schrieb Marek Vasut:
> 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 ?

As Geert already pointed out these are not just one-bit values.

> > +#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) ?

Does this make a difference? chip_info in rs9_driver_data is already const, so 
you can't write into it anyway.

> >   };
> >   
> >   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.

Okay, will do.

> > +	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 .

Okay, will remove them.

> > +		dev_err(&client->dev,
> 
> return dev_err_probe() might work better here ?

How? This error branch always returns -ENODEV, so no deferred probing will 
occur at all.

Best regards,
Alexander

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