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@xxxxxxxxx> 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@xxxxxxxxx> 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@xxxxxxxxx> > --- > .../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
