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@xxxxxxxxxxxxxxx>
> > ---
> >
> > 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;
> > + }
>
> [...]
