Re: [PATCH 7/9] riscv: dts: spacemit: Add PWM14 backlight support for BPI-F3

[Guodong Xu]

On Sat, Apr 12, 2025 at 7:28 AM Inochi Amaoto <inochiama@xxxxxxxxx> wrote:
>
> On Fri, Apr 11, 2025 at 09:23:29AM -0500, Alex Elder wrote:
> > On 4/11/25 9:05 AM, Yixun Lan wrote:
> > >
> > > On 21:14 Fri 11 Apr     , Guodong Xu wrote:
> > > > Add a PWM-based backlight node for the Banana Pi BPI-F3 board,
> > > > using PWM14. The backlight is defined as a 'pwm-backlight' device with
> > > > brightness levels and a default brightness setting. PWM14 is assigned
> > > > a period length of 2000 nanoseconds.
> > > >
> > > > This configuration was used to verify PWM driver changes, with PWM14
> > > > tested and its waveform confirmed as correct.
> > > >
> > > > The node status is set to "disabled", and should be enabled when the
> > > > display driver is ready.
> > > >
> > > .. see comments below
> > > > Signed-off-by: Guodong Xu <guodong@xxxxxxxxxxxx>
> > > > ---
> > > >   .../boot/dts/spacemit/k1-bananapi-f3.dts      | 32 +++++++++++++++++++
> > > >   1 file changed, 32 insertions(+)
> > > >
> > > > diff --git a/arch/riscv/boot/dts/spacemit/k1-bananapi-f3.dts b/arch/riscv/boot/dts/spacemit/k1-bananapi-f3.dts
> > > > index 816ef1bc358e..d04b57ddeb46 100644
> > > > --- a/arch/riscv/boot/dts/spacemit/k1-bananapi-f3.dts
> > > > +++ b/arch/riscv/boot/dts/spacemit/k1-bananapi-f3.dts
> > > > @@ -28,6 +28,32 @@ led1 {
> > > >                           default-state = "on";
> > > >                   };
> > > >           };
> > > > +
> > > > + pwm_bl: lcd_backlight {
> > > > +         compatible = "pwm-backlight";
> > > > +
> > > > +         pwms = <&pwm14 2000>;
> > > > +         brightness-levels = <
> > > > +                 0   40  40  40  40  40  40  40  40  40  40  40  40  40  40  40
> > > > +                 40  40  40  40  40  40  40  40  40  40  40  40  40  40  40  40
> > > > +                 40  40  40  40  40  40  40  40  40  41  42  43  44  45  46  47
> > > > +                 48  49  50  51  52  53  54  55  56  57  58  59  60  61  62  63
> > > > +                 64  65  66  67  68  69  70  71  72  73  74  75  76  77  78  79
> > > > +                 80  81  82  83  84  85  86  87  88  89  90  91  92  93  94  95
> > > > +                 96  97  98  99  100 101 102 103 104 105 106 107 108 109 110 111
> > > > +                 112 113 114 115 116 117 118 119 120 121 122 123 124 125 126 127
> > > > +                 128 129 130 131 132 133 134 135 136 137 138 139 140 141 142 143
> > > > +                 144 145 146 147 148 149 150 151 152 153 154 155 156 157 158 159
> > > > +                 160 161 162 163 164 165 166 167 168 169 170 171 172 173 174 175
> > > > +                 176 177 178 179 180 181 182 183 184 185 186 187 188 189 190 191
> > > > +                 192 193 194 195 196 197 198 199 200 201 202 203 204 205 206 207
> > > > +                 208 209 210 211 212 213 214 215 216 217 218 219 220 221 222 223
> > > > +                 224 225 226 227 228 229 230 231 232 233 234 235 236 237 238 239
> > > > +                 240 241 242 243 244 245 246 247 248 249 250 251 252 253 254 255
> > > > +         >;
> > > > +         default-brightness-level = <100>;
> > > > +         status = "disabled";
> > > I'm confused, has DT in board file with disabled status doesn't make sense?
> > > it doesn't really useful for placeholder, even worse that functionality may not
> > > verified, so I'd suggest sending along with display driver while at it..
> >
> > I think I suggested he include this.  Guodong tested PWM using
> > a backlight on a display connected to a Banana Pi PBI-F3 board.
> > The above numbers come directly from the downstream code, which
> > uses this PWM consistently as a display back light.
> >
> > But you're right, the exact set of numbers to use is dependent
> > on the display used, so it's better to add them when the display
> > gets integrated.
> >
> > The pwm14 node could update still be added here, but that too
> > might as well wait until there's something to use it.  So I
> > think this patch can just be dropped.
> >
>

I checked the existing boards based on SpacemiT K1 Soc, all of them are
following the same pinmux design of using pwm14 (and pinctrl group 1,
ie. pwm14_1_cfg) as the display backlight control.

That's why the code relating to pwm14, in my opinion, could be useful to
others.

> If this patch will be applied as it is after applying the display
> driver. I recommend to preserve this patch, but move out of this
> series and resend it as RFC. If this is only for test purpose, it
> is better to move this into the cover letter and address it is for
> testing.
>

Yes, it's for test purpose. So, let me move the code snippet into the
cover letter in my next version.

-Guodong

> In most case, patches with some unmeet dependency should follow
> maintainer's request, or has specific purposes. It also needed to
> be marked as RFC.
>
> Regards,
> Inochi
>