| Message ID | 20230103123154.3424817-1-alexander.stein@ew.tq-group.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
| Series | [1/4] clk: rs9: Check for vendor/device ID | expand |
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; > + } [...]
Hi Marek, On Tue, Jan 3, 2023 at 4:45 PM Marek Vasut <marex@denx.de> wrote: > 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 ? They're not one-bit values (which bit does RS9_REG_DID_TYPE_FGV set? ;-), but values in a bitfield. So using FIELD_PREP() and friends would make more sense to me. Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
Hi Geert, Am Dienstag, 3. Januar 2023, 17:08:36 CET schrieb Geert Uytterhoeven: > Hi Marek, > > On Tue, Jan 3, 2023 at 4:45 PM Marek Vasut <marex@denx.de> wrote: > > 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 ? > > They're not one-bit values (which bit does RS9_REG_DID_TYPE_FGV set? ;-), > but values in a bitfield. > > So using FIELD_PREP() and friends would make more sense to me. FIELD_PREP() seems pretty nice, but unless I miss something it can't be used for initializing struct members. See renesas_9fgv0241_info. Best regards, Alexander > Gr{oetje,eeting}s, > > Geert > > -- > Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- > geert@linux-m68k.org > > In personal conversations with technical people, I call myself a hacker. But > when I'm talking to journalists I just say "programmer" or something like > that. -- Linus Torvalds
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; > > + } > > [...]
On 1/4/23 11:27, Alexander Stein wrote: [...] >>> + 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. It's not about deferred probing, just that you wouldn't have two lines of dev_err() + return -ENODEV, but just one return dev_err_probe() line.
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) +#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; }; 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; + 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)) { + dev_err(&client->dev, + "Incorrect VID/DID: %#02x, %#02x. Expected %#02x, %#02x\n", + vid, did, RS9_REG_VID_IDT, rs9->chip_info->did); + return -ENODEV; + } + /* Register clock */ for (i = 0; i < rs9->chip_info->num_clks; i++) { snprintf(name, 5, "DIF%d", i); @@ -349,6 +372,7 @@ static int __maybe_unused rs9_resume(struct device *dev) static const struct rs9_chip_info renesas_9fgv0241_info = { .model = RENESAS_9FGV0241, .num_clks = 2, + .did = RS9_REG_DID_TYPE_FGV | 0x02, }; static const struct i2c_device_id rs9_id[] = {
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(+)