Hi Alexander Stein,
> -----Original Message-----
> From: Alexander Stein <alexander.stein@xxxxxxxxxxxxxxx>
> Subject: Re: [PATCH 2/2] clk: rs9: Add support for 9FGV0841
>
> Hi,
>
> Am Montag, 6. November 2023, 08:49:10 CET schrieb Biju Das:
> > Hi Marek,
> >
> > Thanks for the patch.
> >
> > > -----Original Message-----
> > > From: Marek Vasut <marek.vasut+renesas@xxxxxxxxxxx>
> > > Sent: Sunday, November 5, 2023 8:08 PM
> > > Subject: [PATCH 2/2] clk: rs9: Add support for 9FGV0841
> > >
> > > This model is similar to 9FGV0441, the DIFx bits start at bit 0
> > > again, except this chip has 8 outputs. Adjust rs9_calc_dif() to
> > > special-case the
> > > 9FGV0241 where DIFx bits start at 1. Extract only vendor ID from VID
> > > register, the top 4 bits are revision ID which are not useful for
> > > the vendor ID check.
> > >
> > > Signed-off-by: Marek Vasut <marek.vasut+renesas@xxxxxxxxxxx>
> > > ---
> > > Cc: Alexander Stein <alexander.stein@xxxxxxxxxxxxxxx>
> > > Cc: Conor Dooley <conor+dt@xxxxxxxxxx>
> > > Cc: Geert Uytterhoeven <geert+renesas@xxxxxxxxx>
> > > Cc: Krzysztof Kozlowski <krzysztof.kozlowski+dt@xxxxxxxxxx>
> > > Cc: Michael Turquette <mturquette@xxxxxxxxxxxx>
> > > Cc: Rob Herring <robh+dt@xxxxxxxxxx>
> > > Cc: Stephen Boyd <sboyd@xxxxxxxxxx>
> > > Cc: devicetree@xxxxxxxxxxxxxxx
> > > Cc: linux-clk@xxxxxxxxxxxxxxx
> > > Cc: linux-renesas-soc@xxxxxxxxxxxxxxx
> > > ---
> > >
> > > drivers/clk/clk-renesas-pcie.c | 23 +++++++++++++++++++----
> > > 1 file changed, 19 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/drivers/clk/clk-renesas-pcie.c
> > > b/drivers/clk/clk-renesas- pcie.c index 6606aba253c5..f8dd79b18d5a
> > > 100644
> > > --- a/drivers/clk/clk-renesas-pcie.c
> > > +++ b/drivers/clk/clk-renesas-pcie.c
> > > @@ -7,6 +7,7 @@
> > >
> > > * Currently supported:
> > > * - 9FGV0241
> > > * - 9FGV0441
> > >
> > > + * - 9FGV0841
> > >
> > > *
> > > * Copyright (C) 2022 Marek Vasut <marex@xxxxxxx>
> > > */
> > >
> > > @@ -42,6 +43,7 @@
> > >
> > > #define RS9_REG_DID 0x6
> > > #define RS9_REG_BCP 0x7
> > >
> > > +#define RS9_REG_VID_MASK GENMASK(3, 0)
> > >
> > > #define RS9_REG_VID_IDT 0x01
> > >
> > > #define RS9_REG_DID_TYPE_FGV (0x0 <<
> RS9_REG_DID_TYPE_SHIFT)
> > >
> > > @@ -53,6 +55,7 @@
> > >
> > > enum rs9_model {
> > >
> > > RENESAS_9FGV0241,
> > > RENESAS_9FGV0441,
> > >
> > > + RENESAS_9FGV0841,
> > >
> > > };
> > >
> > > /* Structure to describe features of a particular 9-series model */
> > > @@ -
> > >
> > > 162,12 +165,15 @@ static u8 rs9_calc_dif(const struct
> > > rs9_driver_data *rs9, int idx) {
> > >
> > > enum rs9_model model = rs9->chip_info->model;
> > >
> > > + /*
> > > + * On 9FGV0241, the DIF OE0 is BIT(1) and DIF OE(1) is BIT(2),
> > > + * on 9FGV0441 and 9FGV0841 the DIF OE0 is BIT(0) and so on.
> > > + * Increment the index in the 9FGV0241 special case here.
> > > + */
> >
> > I guess model enum variable in struct rs9_chip_info can be replaced
> > with a variable for the above hardware differences(eg: BIT(idx) value
> > in struct
> > rs9_chip_inf) . Then you don't need this function at all.
>
> That's true for 9FGV family. If support for 9QXL family will ever be added
> (the header claims the support can be added), this enum is required again
> as the register model is completely different.
I may be wrong, Still all the hw differences can be handled by feature, data and functions in
struct rs9_chip_info. You don't need comparison with model all over the places.
Cheers,
Biju
>
> Best regards,
> Alexander
>
> > Cheers,
> > Biju
> >
> > > if (model == RENESAS_9FGV0241)
> > >
> > > - return BIT(idx + 1);
> > > - else if (model == RENESAS_9FGV0441)
> > > - return BIT(idx);
> > > + idx++;
> > >
> > > - return 0;
> > > + return BIT(idx);
> > >
> > > }
> > >
> > > static int rs9_get_output_config(struct rs9_driver_data *rs9, int
> > > idx) @@
> > >
> > > -333,6 +339,7 @@ static int rs9_probe(struct i2c_client *client)
> > >
> > > if (ret < 0)
> > >
> > > return ret;
> > >
> > > + vid &= RS9_REG_VID_MASK;
> > >
> > > if (vid != RS9_REG_VID_IDT || did != rs9->chip_info->did)
> > >
> > > return dev_err_probe(&client->dev, -ENODEV,
> > >
> > > "Incorrect VID/DID: %#02x, %#02x.
> > >
> > > Expected %#02x, %#02x\n", @@ -391,9 +398,16 @@ static const struct
> > > rs9_chip_info renesas_9fgv0441_info = {
> > >
> > > .did = RS9_REG_DID_TYPE_FGV | 0x04,
> > >
> > > };
> > >
> > > +static const struct rs9_chip_info renesas_9fgv0841_info = {
> > > + .model = RENESAS_9FGV0841,
> > > + .num_clks = 8,
> > > + .did = RS9_REG_DID_TYPE_FGV | 0x08,
> > > +};
> > > +
> > >
> > > static const struct i2c_device_id rs9_id[] = {
> > >
> > > { "9fgv0241", .driver_data =
> > >
> > > (kernel_ulong_t)&renesas_9fgv0241_info },
> > >
> > > { "9fgv0441", .driver_data =
> > >
> > > (kernel_ulong_t)&renesas_9fgv0441_info },
> > > + { "9fgv0841", .driver_data =
> > > (kernel_ulong_t)&renesas_9fgv0841_info },
> > >
> > > { }
> > >
> > > };
> > > MODULE_DEVICE_TABLE(i2c, rs9_id);
> > >
> > > @@ -401,6 +415,7 @@ 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 },
> > > + { .compatible = "renesas,9fgv0841", .data =
> > > &renesas_9fgv0841_info },
> > >
> > > { }
> > >
> > > };
> > > MODULE_DEVICE_TABLE(of, clk_rs9_of_match);
> > >
> > > --
> > > 2.42.0
>
