Re: [PATCH v2 1/3] media: staging: phy-rockchip-dphy-rx0: add rk3288 support to DPHY driver

[Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>]

Hey there Karthik,

Thanks for your contribution. I'm happy to see this driver
is used and tested more.

The patch looks good, just some minor comments,
in addition to Helen's comments.

Hopefully you'll be able to nail this on the v3 :-)

It would be really better for us if you could add a cover letter
(i.e. PATCH 0/3) in this series. You would be able to put
important information there such as testing information,
series changelog, so on.

See for instance: https://patchwork.linuxtv.org/cover/62830/

Also, please scripts/get_maintainers.pl next time as indication
of who to Cc your patches. For instance, I am the author
of this driver, but I'm not Cced, so I could have missed your patch.

Finally, please put a note at the top of the driver,
explaining where the RK3288 support comes from.

Something like this would work:
"'"
RK3288 support based on <file> in <git repo>, branch <b>"
"""

See below a few more comments.

On Mon, 6 Apr 2020 at 04:30, Karthik Poduval <karthik.poduval@gmail.com> wrote:
>
> add rk3288 support to Rockchip DPHY driver
>
> tested on tinkerbaord with ov5647 using command
> cam -c 1 -C -F cap
>

Nitpick: s/tinkerbaord/tinkerboard

I'd like to have proper sentences in the commit log.
This means, sentences start with capital letters and end
with a stop.

The testing information is normally left for the cover letter.
Also, keep in mind you will need to add proper context
here: for instance, where does the "cam -c 1 -C -F cap"
command come from? I guess it's libcamera, you should
mention this.

> Reported-by: Karthik Poduval <karthik.poduval@gmail.com>

As Helen already pointed out, if you are Reported-by is
incorrectly used here. Please read Documentation/process/submitting-patches.rst.

Helen mentioned this on the v1. Please make sure you
go thru all the feedback you get on each version,
before posting a new one. There's no rush :-)

> Signed-off-by: Karthik Poduval <karthik.poduval@gmail.com>
> ---
>  .../bindings/phy/rockchip-mipi-dphy-rx0.yaml  |  1 +
>  .../phy-rockchip-dphy-rx0.c                   | 69 +++++++++++++++++++
>  2 files changed, 70 insertions(+)
>
> diff --git a/drivers/staging/media/phy-rockchip-dphy-rx0/Documentation/devicetree/bindings/phy/rockchip-mipi-dphy-rx0.yaml b/drivers/staging/media/phy-rockchip-dphy-rx0/Documentation/devicetree/bindings/phy/rockchip-mipi-dphy-rx0.yaml
> index 5dacece35702..8927c36de532 100644
> --- a/drivers/staging/media/phy-rockchip-dphy-rx0/Documentation/devicetree/bindings/phy/rockchip-mipi-dphy-rx0.yaml
> +++ b/drivers/staging/media/phy-rockchip-dphy-rx0/Documentation/devicetree/bindings/phy/rockchip-mipi-dphy-rx0.yaml
> @@ -17,6 +17,7 @@ description: |
>  properties:
>    compatible:
>      const: rockchip,rk3399-mipi-dphy-rx0
> +    const: rockchip,rk3288-mipi-dphy-rx0
>
>    reg:
>      maxItems: 1
> diff --git a/drivers/staging/media/phy-rockchip-dphy-rx0/phy-rockchip-dphy-rx0.c b/drivers/staging/media/phy-rockchip-dphy-rx0/phy-rockchip-dphy-rx0.c
> index 7c4df6d48c43..36fc1b1ee7ae 100644
> --- a/drivers/staging/media/phy-rockchip-dphy-rx0/phy-rockchip-dphy-rx0.c
> +++ b/drivers/staging/media/phy-rockchip-dphy-rx0/phy-rockchip-dphy-rx0.c
> @@ -26,6 +26,15 @@
>  #include <linux/platform_device.h>
>  #include <linux/regmap.h>
>
> +#define RK3288_GRF_SOC_CON6     0x025c
> +#define RK3288_GRF_SOC_CON8     0x0264
> +#define RK3288_GRF_SOC_CON9     0x0268
> +#define RK3288_GRF_SOC_CON10    0x026c
> +#define RK3288_GRF_SOC_CON14    0x027c
> +#define RK3288_GRF_SOC_STATUS21 0x02d4
> +#define RK3288_GRF_IO_VSEL      0x0380
> +#define RK3288_GRF_SOC_CON15    0x03a4
> +
>  #define RK3399_GRF_SOC_CON9            0x6224
>  #define RK3399_GRF_SOC_CON21           0x6254
>  #define RK3399_GRF_SOC_CON22           0x6258
> @@ -47,6 +56,19 @@ struct hsfreq_range {
>         u8 cfg_bit;
>  };
>
> +static const struct hsfreq_range rk3288_mipidphy_hsfreq_ranges[] = {
> +       {  89, 0x00}, {  99, 0x10}, { 109, 0x20}, { 129, 0x01},
> +       { 139, 0x11}, { 149, 0x21}, { 169, 0x02}, { 179, 0x12},
> +       { 199, 0x22}, { 219, 0x03}, { 239, 0x13}, { 249, 0x23},
> +       { 269, 0x04}, { 299, 0x14}, { 329, 0x05}, { 359, 0x15},
> +       { 399, 0x25}, { 449, 0x06}, { 499, 0x16}, { 549, 0x07},
> +       { 599, 0x17}, { 649, 0x08}, { 699, 0x18}, { 749, 0x09},
> +       { 799, 0x19}, { 849, 0x29}, { 899, 0x39}, { 949, 0x0a},
> +       { 999, 0x1a}, {1049, 0x2a}, {1099, 0x3a}, {1149, 0x0b},
> +       {1199, 0x1b}, {1249, 0x2b}, {1299, 0x3b}, {1349, 0x0c},
> +       {1399, 0x1c}, {1449, 0x2c}, {1500, 0x3c}
> +};
> +
>  static const struct hsfreq_range rk3399_mipidphy_hsfreq_ranges[] = {
>         {   89, 0x00 }, {   99, 0x10 }, {  109, 0x20 }, {  129, 0x01 },
>         {  139, 0x11 }, {  149, 0x21 }, {  169, 0x02 }, {  179, 0x12 },
> @@ -60,6 +82,11 @@ static const struct hsfreq_range rk3399_mipidphy_hsfreq_ranges[] = {
>         { 1399, 0x1c }, { 1449, 0x2c }, { 1500, 0x3c }
>  };
>
> +static const char * const rk3288_mipidphy_clks[] = {
> +       "dphy-ref",
> +       "pclk",
> +};
> +

... are you sure that you need this pclk? The driver
doesn't seem to be accessing any peripheral interface.

Can you test without requesting the pclk ?

Stay safe!
Ezequiel