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

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

[Alexander Stein]

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)
+#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[] = {
-- 
2.34.1

[PATCH 2/4] dt-bindings: clk: rs9: Add bindings for 9FGV0441

[Alexander Stein]

This is a 4-channel variant of 9FGV series.

Signed-off-by: Alexander Stein <alexander.stein@ew.tq-group.com>
---
 .../devicetree/bindings/clock/renesas,9series.yaml          | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/Documentation/devicetree/bindings/clock/renesas,9series.yaml b/Documentation/devicetree/bindings/clock/renesas,9series.yaml
index 6b6cec3fba52..3afdebdb52ad 100644
--- a/Documentation/devicetree/bindings/clock/renesas,9series.yaml
+++ b/Documentation/devicetree/bindings/clock/renesas,9series.yaml
@@ -16,6 +16,11 @@ description: |
   - 9FGV0241:
     0 -- DIF0
     1 -- DIF1
+  - 9FGV0441:
+    0 -- DIF0
+    1 -- DIF1
+    2 -- DIF2
+    3 -- DIF3
 
 maintainers:
   - Marek Vasut <marex@denx.de>
@@ -24,6 +29,7 @@ properties:
   compatible:
     enum:
       - renesas,9fgv0241
+      - renesas,9fgv0441
 
   reg:
     description: I2C device address
-- 
2.34.1

[PATCH 3/4] clk: rs9: Support device specific dif bit calculation

[Alexander Stein]

The calculation DIFx is BIT(n) +1 is only true for 9FGV0241. With
additional devices this is getting more complicated.
Support a base bit for the DIF calculation, currently only devices
with consecutive bits are supported, e.g. the 6-channel device needs
additional logic.

Signed-off-by: Alexander Stein <alexander.stein@ew.tq-group.com>
---
 drivers/clk/clk-renesas-pcie.c | 29 ++++++++++++++++-------------
 1 file changed, 16 insertions(+), 13 deletions(-)

diff --git a/drivers/clk/clk-renesas-pcie.c b/drivers/clk/clk-renesas-pcie.c
index 0076ed8f11b0..d19b8e759eea 100644
--- a/drivers/clk/clk-renesas-pcie.c
+++ b/drivers/clk/clk-renesas-pcie.c
@@ -18,7 +18,6 @@
 #include <linux/regmap.h>
 
 #define RS9_REG_OE				0x0
-#define RS9_REG_OE_DIF_OE(n)			BIT((n) + 1)
 #define RS9_REG_SS				0x1
 #define RS9_REG_SS_AMP_0V6			0x0
 #define RS9_REG_SS_AMP_0V7			0x1
@@ -31,9 +30,6 @@
 #define RS9_REG_SS_SSC_MASK			(3 << 3)
 #define RS9_REG_SS_SSC_LOCK			BIT(5)
 #define RS9_REG_SR				0x2
-#define RS9_REG_SR_2V0_DIF(n)			0
-#define RS9_REG_SR_3V0_DIF(n)			BIT((n) + 1)
-#define RS9_REG_SR_DIF_MASK(n)		BIT((n) + 1)
 #define RS9_REG_REF				0x3
 #define RS9_REG_REF_OE				BIT(4)
 #define RS9_REG_REF_OD				BIT(5)
@@ -62,6 +58,7 @@ struct rs9_chip_info {
 	const enum rs9_model	model;
 	unsigned int		num_clks;
 	u8			did;
+	u8			(*calc_dif)(int idx);
 };
 
 struct rs9_driver_data {
@@ -160,8 +157,14 @@ static const struct regmap_config rs9_regmap_config = {
 	.reg_read = rs9_regmap_i2c_read,
 };
 
+static u8 rs9fgv0241_calc_dif(int idx)
+{
+	return BIT(idx) + 1;
+}
+
 static int rs9_get_output_config(struct rs9_driver_data *rs9, int idx)
 {
+	u8 dif = rs9->chip_info->calc_dif(idx);
 	struct i2c_client *client = rs9->client;
 	unsigned char name[5] = "DIF0";
 	struct device_node *np;
@@ -169,8 +172,7 @@ static int rs9_get_output_config(struct rs9_driver_data *rs9, int idx)
 	u32 sr;
 
 	/* Set defaults */
-	rs9->clk_dif_sr &= ~RS9_REG_SR_DIF_MASK(idx);
-	rs9->clk_dif_sr |= RS9_REG_SR_3V0_DIF(idx);
+	rs9->clk_dif_sr |= dif;
 
 	snprintf(name, 5, "DIF%d", idx);
 	np = of_get_child_by_name(client->dev.of_node, name);
@@ -182,11 +184,9 @@ static int rs9_get_output_config(struct rs9_driver_data *rs9, int idx)
 	of_node_put(np);
 	if (!ret) {
 		if (sr == 2000000) {		/* 2V/ns */
-			rs9->clk_dif_sr &= ~RS9_REG_SR_DIF_MASK(idx);
-			rs9->clk_dif_sr |= RS9_REG_SR_2V0_DIF(idx);
+			rs9->clk_dif_sr &= ~dif;
 		} else if (sr == 3000000) {	/* 3V/ns (default) */
-			rs9->clk_dif_sr &= ~RS9_REG_SR_DIF_MASK(idx);
-			rs9->clk_dif_sr |= RS9_REG_SR_3V0_DIF(idx);
+			rs9->clk_dif_sr |= dif;
 		} else
 			ret = dev_err_probe(&client->dev, -EINVAL,
 					    "Invalid renesas,slew-rate value\n");
@@ -257,11 +257,13 @@ static void rs9_update_config(struct rs9_driver_data *rs9)
 	}
 
 	for (i = 0; i < rs9->chip_info->num_clks; i++) {
-		if (rs9->clk_dif_sr & RS9_REG_SR_3V0_DIF(i))
+		u8 dif = rs9->chip_info->calc_dif(i);
+
+		if (rs9->clk_dif_sr & dif)
 			continue;
 
-		regmap_update_bits(rs9->regmap, RS9_REG_SR, RS9_REG_SR_3V0_DIF(i),
-				   rs9->clk_dif_sr & RS9_REG_SR_3V0_DIF(i));
+		regmap_update_bits(rs9->regmap, RS9_REG_SR, dif,
+				   rs9->clk_dif_sr & dif);
 	}
 }
 
@@ -373,6 +375,7 @@ static const struct rs9_chip_info renesas_9fgv0241_info = {
 	.model		= RENESAS_9FGV0241,
 	.num_clks	= 2,
 	.did		= RS9_REG_DID_TYPE_FGV | 0x02,
+	.calc_dif	= rs9fgv0241_calc_dif,
 };
 
 static const struct i2c_device_id rs9_id[] = {
-- 
2.34.1

[PATCH 4/4] clk: rs9: Add support for 9FGV0441

[Alexander Stein]

This model is similar to 9FGV0241, but the DIFx bits start at bit 0.

Signed-off-by: Alexander Stein <alexander.stein@ew.tq-group.com>
---
 drivers/clk/clk-renesas-pcie.c | 18 +++++++++++++++++-
 1 file changed, 17 insertions(+), 1 deletion(-)

diff --git a/drivers/clk/clk-renesas-pcie.c b/drivers/clk/clk-renesas-pcie.c
index d19b8e759eea..6095eacd4f8a 100644
--- a/drivers/clk/clk-renesas-pcie.c
+++ b/drivers/clk/clk-renesas-pcie.c
@@ -6,6 +6,7 @@
  *   - 9FGV/9DBV/9DMV/9FGL/9DML/9QXL/9SQ
  * Currently supported:
  *   - 9FGV0241
+ *   - 9FGV0441
  *
  * Copyright (C) 2022 Marek Vasut <marex@denx.de>
  */
@@ -51,6 +52,7 @@
 /* Supported Renesas 9-series models. */
 enum rs9_model {
 	RENESAS_9FGV0241,
+	RENESAS_9FGV0441,
 };
 
 /* Structure to describe features of a particular 9-series model */
@@ -66,7 +68,7 @@ struct rs9_driver_data {
 	struct regmap		*regmap;
 	const struct rs9_chip_info *chip_info;
 	struct clk		*pin_xin;
-	struct clk_hw		*clk_dif[2];
+	struct clk_hw		*clk_dif[4];
 	u8			pll_amplitude;
 	u8			pll_ssc;
 	u8			clk_dif_sr;
@@ -162,6 +164,11 @@ static u8 rs9fgv0241_calc_dif(int idx)
 	return BIT(idx) + 1;
 }
 
+static u8 rs9fgv0441_calc_dif(int idx)
+{
+	return BIT(idx);
+}
+
 static int rs9_get_output_config(struct rs9_driver_data *rs9, int idx)
 {
 	u8 dif = rs9->chip_info->calc_dif(idx);
@@ -378,14 +385,23 @@ static const struct rs9_chip_info renesas_9fgv0241_info = {
 	.calc_dif	= rs9fgv0241_calc_dif,
 };
 
+static const struct rs9_chip_info renesas_9fgv0441_info = {
+	.model		= RENESAS_9FGV0441,
+	.num_clks	= 4,
+	.did		= RS9_REG_DID_TYPE_FGV | 0x04,
+	.calc_dif	= rs9fgv0441_calc_dif,
+};
+
 static const struct i2c_device_id rs9_id[] = {
 	{ "9fgv0241", .driver_data = RENESAS_9FGV0241 },
+	{ "9fgv0441", .driver_data = RENESAS_9FGV0441 },
 	{ }
 };
 MODULE_DEVICE_TABLE(i2c, rs9_id);
 
 static const struct of_device_id clk_rs9_of_match[] = {
 	{ .compatible = "renesas,9fgv0241", .data = &renesas_9fgv0241_info },
+	{ .compatible = "renesas,9fgv0441", .data = &renesas_9fgv0441_info },
 	{ }
 };
 MODULE_DEVICE_TABLE(of, clk_rs9_of_match);
-- 
2.34.1

Re: [PATCH 2/4] dt-bindings: clk: rs9: Add bindings for 9FGV0441

[Krzysztof Kozlowski]

On 03/01/2023 13:31, Alexander Stein wrote:
> This is a 4-channel variant of 9FGV series.

Subject: drop second, redundant "bindings for".


Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

Best regards,
Krzysztof

Re: [PATCH 2/4] dt-bindings: clk: rs9: Add bindings for 9FGV0441

[Alexander Stein]

Hi Krzysztof,

Am Dienstag, 3. Januar 2023, 14:25:19 CET schrieb Krzysztof Kozlowski:
> On 03/01/2023 13:31, Alexander Stein wrote:
> > This is a 4-channel variant of 9FGV series.
> 
> Subject: drop second, redundant "bindings for".
> 
> 
> Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

Thanks, will drop it on next version.

Best regards,
Alexander

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

[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 ?

> +#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;
> +	}

[...]

Re: [PATCH 3/4] clk: rs9: Support device specific dif bit calculation

[Marek Vasut]

On 1/3/23 13:31, Alexander Stein wrote:
> The calculation DIFx is BIT(n) +1 is only true for 9FGV0241. With
> additional devices this is getting more complicated.
> Support a base bit for the DIF calculation, currently only devices
> with consecutive bits are supported, e.g. the 6-channel device needs
> additional logic.
> 
> Signed-off-by: Alexander Stein <alexander.stein@ew.tq-group.com>
> ---
>   drivers/clk/clk-renesas-pcie.c | 29 ++++++++++++++++-------------
>   1 file changed, 16 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/clk/clk-renesas-pcie.c b/drivers/clk/clk-renesas-pcie.c
> index 0076ed8f11b0..d19b8e759eea 100644
> --- a/drivers/clk/clk-renesas-pcie.c
> +++ b/drivers/clk/clk-renesas-pcie.c
> @@ -18,7 +18,6 @@
>   #include <linux/regmap.h>
>   
>   #define RS9_REG_OE				0x0
> -#define RS9_REG_OE_DIF_OE(n)			BIT((n) + 1)
>   #define RS9_REG_SS				0x1
>   #define RS9_REG_SS_AMP_0V6			0x0
>   #define RS9_REG_SS_AMP_0V7			0x1
> @@ -31,9 +30,6 @@
>   #define RS9_REG_SS_SSC_MASK			(3 << 3)
>   #define RS9_REG_SS_SSC_LOCK			BIT(5)
>   #define RS9_REG_SR				0x2
> -#define RS9_REG_SR_2V0_DIF(n)			0
> -#define RS9_REG_SR_3V0_DIF(n)			BIT((n) + 1)
> -#define RS9_REG_SR_DIF_MASK(n)		BIT((n) + 1)
>   #define RS9_REG_REF				0x3
>   #define RS9_REG_REF_OE				BIT(4)
>   #define RS9_REG_REF_OD				BIT(5)
> @@ -62,6 +58,7 @@ struct rs9_chip_info {
>   	const enum rs9_model	model;
>   	unsigned int		num_clks;
>   	u8			did;
> +	u8			(*calc_dif)(int idx);
>   };
>   
>   struct rs9_driver_data {
> @@ -160,8 +157,14 @@ static const struct regmap_config rs9_regmap_config = {
>   	.reg_read = rs9_regmap_i2c_read,
>   };
>   
> +static u8 rs9fgv0241_calc_dif(int idx)
> +{
> +	return BIT(idx) + 1;

Can't we just do

if (model == ...)
  return BIT(idx) + 1
else if (model == ...)
  return BIT(idx);
...

?

[...]

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

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

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

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

[Alexander Stein]

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

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

[Alexander Stein]

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

Re: [PATCH 3/4] clk: rs9: Support device specific dif bit calculation

[Alexander Stein]

Hi Marek,

Am Dienstag, 3. Januar 2023, 15:31:21 CET schrieb Marek Vasut:
> On 1/3/23 13:31, Alexander Stein wrote:
> > The calculation DIFx is BIT(n) +1 is only true for 9FGV0241. With
> > additional devices this is getting more complicated.
> > Support a base bit for the DIF calculation, currently only devices
> > with consecutive bits are supported, e.g. the 6-channel device needs
> > additional logic.
> > 
> > Signed-off-by: Alexander Stein <alexander.stein@ew.tq-group.com>
> > ---
> > 
> >   drivers/clk/clk-renesas-pcie.c | 29 ++++++++++++++++-------------
> >   1 file changed, 16 insertions(+), 13 deletions(-)
> > 
> > diff --git a/drivers/clk/clk-renesas-pcie.c
> > b/drivers/clk/clk-renesas-pcie.c index 0076ed8f11b0..d19b8e759eea 100644
> > --- a/drivers/clk/clk-renesas-pcie.c
> > +++ b/drivers/clk/clk-renesas-pcie.c
> > @@ -18,7 +18,6 @@
> > 
> >   #include <linux/regmap.h>
> >   
> >   #define RS9_REG_OE				0x0
> > 
> > -#define RS9_REG_OE_DIF_OE(n)			BIT((n) + 1)
> > 
> >   #define RS9_REG_SS				0x1
> >   #define RS9_REG_SS_AMP_0V6			0x0
> >   #define RS9_REG_SS_AMP_0V7			0x1
> > 
> > @@ -31,9 +30,6 @@
> > 
> >   #define RS9_REG_SS_SSC_MASK			(3 << 3)
> >   #define RS9_REG_SS_SSC_LOCK			BIT(5)
> >   #define RS9_REG_SR				0x2
> > 
> > -#define RS9_REG_SR_2V0_DIF(n)			0
> > -#define RS9_REG_SR_3V0_DIF(n)			BIT((n) + 1)
> > -#define RS9_REG_SR_DIF_MASK(n)		BIT((n) + 1)
> > 
> >   #define RS9_REG_REF				0x3
> >   #define RS9_REG_REF_OE				BIT(4)
> >   #define RS9_REG_REF_OD				BIT(5)
> > 
> > @@ -62,6 +58,7 @@ struct rs9_chip_info {
> > 
> >   	const enum rs9_model	model;
> >   	unsigned int		num_clks;
> >   	u8			did;
> > 
> > +	u8			(*calc_dif)(int idx);
> > 
> >   };
> >   
> >   struct rs9_driver_data {
> > 
> > @@ -160,8 +157,14 @@ static const struct regmap_config rs9_regmap_config =
> > {> 
> >   	.reg_read = rs9_regmap_i2c_read,
> >   
> >   };
> > 
> > +static u8 rs9fgv0241_calc_dif(int idx)
> > +{
> > +	return BIT(idx) + 1;
> 
> Can't we just do
> 
> if (model == ...)
>   return BIT(idx) + 1
> else if (model == ...)
>   return BIT(idx);
> ...

I was tempted going this way. But I opted for a callback due to the fact that 
this driver might support 9FGV/9DBV/9DMV/9FGL/9DML/9QXL/9SQ as well(your 
comment in the header).
Even just considering 9FVG, 9FGV0641 has an even more complex DIF offset 
calculation. 
The mapping is
* DIF OE0 - Bit 0
* DIF OE1 - Bit 2
* DIF OE2 - Bit 3
* DIF OE3 - Bit 4
* DIF OE4 - Bit 6
* DIF OE5 - Bit 7

So the calucation might not fit into one line, so the readability benefit is 
gone.

Best regards,
Alexander

Re: [PATCH 3/4] clk: rs9: Support device specific dif bit calculation

[Marek Vasut]

On 1/4/23 11:32, Alexander Stein wrote:
> Hi Marek,

Hi,

> Am Dienstag, 3. Januar 2023, 15:31:21 CET schrieb Marek Vasut:
>> On 1/3/23 13:31, Alexander Stein wrote:
>>> The calculation DIFx is BIT(n) +1 is only true for 9FGV0241. With
>>> additional devices this is getting more complicated.
>>> Support a base bit for the DIF calculation, currently only devices
>>> with consecutive bits are supported, e.g. the 6-channel device needs
>>> additional logic.
>>>
>>> Signed-off-by: Alexander Stein <alexander.stein@ew.tq-group.com>
>>> ---
>>>
>>>    drivers/clk/clk-renesas-pcie.c | 29 ++++++++++++++++-------------
>>>    1 file changed, 16 insertions(+), 13 deletions(-)
>>>
>>> diff --git a/drivers/clk/clk-renesas-pcie.c
>>> b/drivers/clk/clk-renesas-pcie.c index 0076ed8f11b0..d19b8e759eea 100644
>>> --- a/drivers/clk/clk-renesas-pcie.c
>>> +++ b/drivers/clk/clk-renesas-pcie.c
>>> @@ -18,7 +18,6 @@
>>>
>>>    #include <linux/regmap.h>
>>>    
>>>    #define RS9_REG_OE				0x0
>>>
>>> -#define RS9_REG_OE_DIF_OE(n)			BIT((n) + 1)
>>>
>>>    #define RS9_REG_SS				0x1
>>>    #define RS9_REG_SS_AMP_0V6			0x0
>>>    #define RS9_REG_SS_AMP_0V7			0x1
>>>
>>> @@ -31,9 +30,6 @@
>>>
>>>    #define RS9_REG_SS_SSC_MASK			(3 << 3)
>>>    #define RS9_REG_SS_SSC_LOCK			BIT(5)
>>>    #define RS9_REG_SR				0x2
>>>
>>> -#define RS9_REG_SR_2V0_DIF(n)			0
>>> -#define RS9_REG_SR_3V0_DIF(n)			BIT((n) + 1)
>>> -#define RS9_REG_SR_DIF_MASK(n)		BIT((n) + 1)
>>>
>>>    #define RS9_REG_REF				0x3
>>>    #define RS9_REG_REF_OE				BIT(4)
>>>    #define RS9_REG_REF_OD				BIT(5)
>>>
>>> @@ -62,6 +58,7 @@ struct rs9_chip_info {
>>>
>>>    	const enum rs9_model	model;
>>>    	unsigned int		num_clks;
>>>    	u8			did;
>>>
>>> +	u8			(*calc_dif)(int idx);
>>>
>>>    };
>>>    
>>>    struct rs9_driver_data {
>>>
>>> @@ -160,8 +157,14 @@ static const struct regmap_config rs9_regmap_config =
>>> {>
>>>    	.reg_read = rs9_regmap_i2c_read,
>>>    
>>>    };
>>>
>>> +static u8 rs9fgv0241_calc_dif(int idx)
>>> +{
>>> +	return BIT(idx) + 1;
>>
>> Can't we just do
>>
>> if (model == ...)
>>    return BIT(idx) + 1
>> else if (model == ...)
>>    return BIT(idx);
>> ...
> 
> I was tempted going this way. But I opted for a callback due to the fact that
> this driver might support 9FGV/9DBV/9DMV/9FGL/9DML/9QXL/9SQ as well(your
> comment in the header).
> Even just considering 9FVG, 9FGV0641 has an even more complex DIF offset
> calculation.
> The mapping is
> * DIF OE0 - Bit 0
> * DIF OE1 - Bit 2
> * DIF OE2 - Bit 3
> * DIF OE3 - Bit 4
> * DIF OE4 - Bit 6
> * DIF OE5 - Bit 7
> 
> So the calucation might not fit into one line, so the readability benefit is
> gone.

You can still do

if (model == ...)
  return function1();
else if (model == ...)
  return function2();

which would work without any indirection via callback.

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

[Marek Vasut]

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.