| Message ID | 20230412153310.241046-1-angelogioacchino.delregno@collabora.com |
|---|---|
| Headers | show |
| Series | Add support for MT6331 and MT6332 LEDs | expand |
On 12/04/2023 17:33, AngeloGioacchino Del Regno wrote: > Add mediatek,mt6331-led compatible for the LED controller found > in the MT6331 PMIC. > > Signed-off-by: AngeloGioacchino Del Regno<angelogioacchino.delregno@collabora.com> > Acked-by: Krzysztof Kozlowski<krzysztof.kozlowski@linaro.org> Reviewed-by: Alexandre Mergnat <amergnat@baylibre.com>
Hi! > Changes in v2: > - Rebase over next-20230412 > > NOTE: Since v1 of this series was sent in Semptember 2022 and got > ignored for *7 months* with no feedback, I'm retrying the upstreaming > of this same series. > There are no changes, if not just a simple rebase and another test > run on the same hardware. > > > MT6323 is not the only PMIC that has a LEDs controller IP and it was > found that the others do have a compatible register layout, except > for some register offsets. > The logic contained in this driver can be totally reused for other > PMICs as well, so I can't see any reason to keep this specific to > the MT6323 part. > > This series brings meaningful platform data to this driver, giving > it flexibility and adding support for LED controllers found in the > MT6331 and MT6332 PMICs. > > Tested on MT6795 Sony Xperia M5 smartphone. Please cc phone-devel with phone related stuff. Can I get ls /sys/class/leds on that machine? BR, Pavel
Hi! > In order to enhance the flexibility of this driver and let it support > more than just one MediaTek LEDs IP for more than just one PMIC, > add platform data structure specifying the register offsets and > data that commonly varies between different IPs. > > This commit brings no functional changes. > @@ -63,12 +61,9 @@ > #define MT6323_ISINK_CH_EN_MASK(i) BIT(i) > #define MT6323_ISINK_CH_EN(i) BIT(i) > > -#define MT6323_MAX_PERIOD 10000 > -#define MT6323_MAX_LEDS 4 > -#define MT6323_MAX_BRIGHTNESS 6 > -#define MT6323_UNIT_DUTY 3125 > -#define MT6323_CAL_HW_DUTY(o, p) DIV_ROUND_CLOSEST((o) * 100000ul,\ > - (p) * MT6323_UNIT_DUTY) > +#define MAX_SUPPORTED_LEDS 8 > +#define MT6323_CAL_HW_DUTY(o, p, u) DIV_ROUND_CLOSEST((o) * 100000ul,\ > + (p) * (u)) > You increase number of supported leds from 4 to 8. That's ok, but I'd not call it "no functional changes". > +/** > + * struct mt6323_regs - register spec for the LED device > + * @top_ckpdn: Offset to ISINK_CKPDN[0..x] registers > + * @num_top_ckpdn: Number of ISINK_CKPDN registers > + * @top_ckcon: Offset to ISINK_CKCON[0..x] registers > + * @num_top_ckcon: Number of ISINK_CKCON registers > + * @isink_con: Offset to ISINKx_CON[0..x] registers > + * @num_isink_con: Number of ISINKx_CON registers > + * @isink_max_regs: Number of ISINK[0..x] registers > + * @isink_en_ctrl: Offset to ISINK_EN_CTRL register > + */ > +struct mt6323_regs { > + const u16 *top_ckpdn; > + u8 num_top_ckpdn; > + const u16 *top_ckcon; > + u8 num_top_ckcon; > + const u16 *isink_con; > + u8 num_isink_con; > + u8 isink_max_regs; > + u16 isink_en_ctrl; > +}; I'd use ints here. Should get similar memory usage and maybe less code size. But ... no need to resubmit just for this. > @@ -469,8 +525,31 @@ static int mt6323_led_remove(struct platform_device *pdev) > return 0; > } > > +static const struct mt6323_regs mt6323_registers = { > + .top_ckpdn = (const u16[]){ 0x102, 0x106, 0x10e }, > + .num_top_ckpdn = 3, > + .top_ckcon = (const u16[]){ 0x120, 0x126 }, > + .num_top_ckcon = 2, > + .isink_con = (const u16[]){ 0x330, 0x332, 0x334 }, > + .num_isink_con = 3, > + .isink_max_regs = 4, /* ISINK[0..3] */ > + .isink_en_ctrl = 0x356, > +}; > + > +static const struct mt6323_hwspec mt6323_spec = { > + .max_period = 10000, > + .max_leds = 4, > + .max_brightness = 6, > + .unit_duty = 3125, > +}; > + > +static const struct mt6323_data mt6323_pdata = { > + .regs = &mt6323_registers, > + .spec = &mt6323_spec, > +}; > + Acked-by: Pavel Machek <pavel@ucw.cz> BR, Pavel
Hi! > Add the register offsets for MT6331. The hwspec is the same as MT6323. > > Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com> > --- > drivers/leds/leds-mt6323.c | 17 +++++++++++++++++ > 1 file changed, 17 insertions(+) > > diff --git a/drivers/leds/leds-mt6323.c b/drivers/leds/leds-mt6323.c > index 182256ec1924..5d95dbd9a761 100644 > --- a/drivers/leds/leds-mt6323.c > +++ b/drivers/leds/leds-mt6323.c > @@ -531,6 +531,17 @@ static const struct mt6323_regs mt6323_registers = { > .isink_en_ctrl = 0x356, > }; > > +static const struct mt6323_regs mt6331_registers = { > + .top_ckpdn = (const u16[]){ 0x138, 0x13e, 0x144 }, > + .num_top_ckpdn = 3, > + .top_ckcon = (const u16[]){ 0x14c, 0x14a }, > + .num_top_ckcon = 2, > + .isink_con = (const u16[]){ 0x40c, 0x40e, 0x410, 0x412, 0x414 }, > + .num_isink_con = 5, > + .isink_max_regs = 4, /* ISINK[0..3] */ > + .isink_en_ctrl = 0x43a, > +}; > + > static const struct mt6323_hwspec mt6323_spec = { > .max_period = 10000, > .max_leds = 4, > @@ -543,8 +554,14 @@ static const struct mt6323_data mt6323_pdata = { > .spec = &mt6323_spec, > }; > > +static const struct mt6323_data mt6331_pdata = { > + .regs = &mt6331_registers, > + .spec = &mt6323_spec, > +}; > + > static const struct of_device_id mt6323_led_dt_match[] = { > { .compatible = "mediatek,mt6323-led", .data = &mt6323_pdata}, > + { .compatible = "mediatek,mt6331-led", .data = &mt6331_pdata }, > {}, " " before } is missing. But that's really detail. Acked-by: Pavel Machek <pavel@ucw.cz> Pavel
Il 13/04/23 12:49, Pavel Machek ha scritto: > Hi! > >> Changes in v2: >> - Rebase over next-20230412 >> >> NOTE: Since v1 of this series was sent in Semptember 2022 and got >> ignored for *7 months* with no feedback, I'm retrying the upstreaming >> of this same series. >> There are no changes, if not just a simple rebase and another test >> run on the same hardware. >> >> >> MT6323 is not the only PMIC that has a LEDs controller IP and it was >> found that the others do have a compatible register layout, except >> for some register offsets. >> The logic contained in this driver can be totally reused for other >> PMICs as well, so I can't see any reason to keep this specific to >> the MT6323 part. >> >> This series brings meaningful platform data to this driver, giving >> it flexibility and adding support for LED controllers found in the >> MT6331 and MT6332 PMICs. >> >> Tested on MT6795 Sony Xperia M5 smartphone. > > Please cc phone-devel with phone related stuff. Sorry, I completely forgot to :-( > > Can I get ls /sys/class/leds on that machine? Yes you can, but that will require some time, as I'm on other tasks. I should be able to provide that next week, sorry for the delay! Regards, Angelo
On 12/04/2023 17:33, AngeloGioacchino Del Regno wrote: > Add the register offsets for MT6331. The hwspec is the same as MT6323. > > Signed-off-by: AngeloGioacchino Del Regno<angelogioacchino.delregno@collabora.com> Reviewed-by: Alexandre Mergnat <amergnat@baylibre.com> Regards, Alexandre
On 12/04/2023 17:33, AngeloGioacchino Del Regno wrote: > This renames all definitions and macros to drop the MT6323_ prefix, > since it is now possible to easily add support to more PMICs in > this driver. > While at it, also fix related formatting where possible. > > This commit brings no functional changes. > > Signed-off-by: AngeloGioacchino Del Regno<angelogioacchino.delregno@collabora.com> Reviewed-by: Alexandre Mergnat <amergnat@baylibre.com> Regards, Alexandre