Re: [PATCH] leds: leds-mc13783: Fix MC13892 keypad led access

[Jacek Anaszewski <j.anaszewski@xxxxxxxxxxx>]

On 10/10/2016 02:22 PM, Alexander Shiyan wrote:
> > Понедельник, 10 октября 2016, 15:14 +03:00 от Jacek Anaszewski <j.anaszewski@xxxxxxxxxxx>:
> >
> > Hi Alexander,
> >
> > Thanks for the patch.
> >
> > On 10/09/2016 03:37 PM, Alexander Kurz wrote:
> > > Fix the register access shift argument calculation introduced with
> > > commit a59ce6584d56 ("leds: leds-mc13783: Add MC34708 LED support")
> > > and re-enable access to the "keypad" led for MC13892 MFC devices.
> > >
> > > Signed-off-by: Alexander Kurz < akurz@xxxxxxxx >
> > > ---
> > >  drivers/leds/leds-mc13783.c | 5 +++--
> > >  1 file changed, 3 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/leds/leds-mc13783.c b/drivers/leds/leds-mc13783.c
> > > index a2e4c17..2421cf10 100644
> > > --- a/drivers/leds/leds-mc13783.c
> > > +++ b/drivers/leds/leds-mc13783.c
> > > @@ -84,8 +84,9 @@ static int mc13xxx_led_set(struct led_classdev *led_cdev,
> > >  case MC13892_LED_MD:
> > >  case MC13892_LED_AD:
> > >  case MC13892_LED_KP:
> > > -reg = (led->id - MC13892_LED_MD) / 2;
> > > -shift = 3 + (led->id - MC13892_LED_MD) * 12;
> > > +off = led->id - MC13892_LED_MD;
> > > +reg = off / 2;
> > > +shift = 3 + (off - reg * 2) * 12;
> > >  break;
> > >  case MC13892_LED_R:
> > >  case MC13892_LED_G:
> >
> > While trying to analyze these calculations I noticed a discrepancy
> > between DT documentation and the enum in the header file.
> >
> > Documentation/devicetree/bindings/mfd/mc13xxx.txt:
> >
> > MC13892 LED IDs:
> >      0  : Main display
> >      1  : AUX display
> >      2  : Keypad
> >      3  : Red
> >      4  : Green
> >      5  : Blue
> >
> > include/linux/mfd/mc13xxx.h:
> >
> > enum {
> >          /* MC13783 LED IDs */
> >          MC13783_LED_MD,
> >          MC13783_LED_AD,
> >          MC13783_LED_KP,
> >          MC13783_LED_R1,
> >          MC13783_LED_G1,
> >          MC13783_LED_B1,
> >          MC13783_LED_R2,
> >          MC13783_LED_G2,
> >          MC13783_LED_B2,
> >          MC13783_LED_R3,
> >          MC13783_LED_G3,
> >          MC13783_LED_B3,
> >          /* MC13892 LED IDs */
> >          MC13892_LED_MD,
> >          MC13892_LED_AD,
> >          MC13892_LED_KP,
> >          MC13892_LED_R,
> >          MC13892_LED_G,
> >          MC13892_LED_B,
> >          /* MC34708 LED IDs */
> >          MC34708_LED_R,
> >          MC34708_LED_G,
> > }
> >
> > Keypad LED enum identifier is 15, but documentation states
> > that it is 2. The code seems to expect the former:
>
> The driver uses calculation:
>
> if (of_property_read_u32(child, "reg", &tmp))
>   continue;
> pdata->led[i].id = leds->devtype->led_min + tmp;
>
> Where led_min = MC13892_LED_MD for MC13892.

Right, thanks for spotting this.

--
Best regards,
Jacek Anaszewski
--
To unsubscribe from this list: send the line "unsubscribe linux-leds" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html