[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <551941de-b9a4-4353-8bb2-0bfc3304ef87@gmail.com>
Date: Mon, 9 Dec 2024 18:07:55 +0800
From: Nick Chan <towinchenmi@...il.com>
To: Krzysztof Kozlowski <krzk@...nel.org>,
y@...k-bin.smtp.subspace.kernel.org
Cc: Lee Jones <lee@...nel.org>, Daniel Thompson <danielt@...nel.org>,
Jingoo Han <jingoohan1@...il.com>, Pavel Machek <pavel@....cz>,
Rob Herring <robh@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>,
Conor Dooley <conor+dt@...nel.org>, Helge Deller <deller@....de>,
Hector Martin <marcan@...can.st>, Sven Peter <sven@...npeter.dev>,
Alyssa Rosenzweig <alyssa@...enzweig.io>, dri-devel@...ts.freedesktop.org,
linux-leds@...r.kernel.org, devicetree@...r.kernel.org,
linux-kernel@...r.kernel.org, linux-fbdev@...r.kernel.org,
asahi@...ts.linux.dev, linux-arm-kernel@...ts.infradead.org
Subject: Re: [PATCH v2 2/3] backlight: dwi_bl: Add Apple DWI backlight driver
Krzysztof Kozlowski 於 2024/12/9 下午5:22 寫道:
> On Sat, Dec 07, 2024 at 09:03:15PM +0800, Nick Chan wrote:
>> Add driver for backlight controllers attached via Apple DWI 2-wire
>> interface, which is found on some Apple iPhones, iPads and iPod touches
>> with a LCD display.
>>
>> Signed-off-by: Nick Chan <towinchenmi@...il.com>
>> ---
>> drivers/video/backlight/Kconfig | 12 +++
>> drivers/video/backlight/Makefile | 1 +
>> drivers/video/backlight/dwi_bl.c | 124 +++++++++++++++++++++++++++++++
>> 3 files changed, 137 insertions(+)
>> create mode 100644 drivers/video/backlight/dwi_bl.c
>>
>> diff --git a/drivers/video/backlight/Kconfig b/drivers/video/backlight/Kconfig
>> index 3614a5d29c71..e64cc3d51ac5 100644
>> --- a/drivers/video/backlight/Kconfig
>> +++ b/drivers/video/backlight/Kconfig
>> @@ -166,6 +166,18 @@ config BACKLIGHT_EP93XX
>> To compile this driver as a module, choose M here: the module will
>> be called ep93xx_bl.
>>
>> +config BACKLIGHT_APPLE_DWI
>> + tristate "Apple DWI 2-Wire Interface Backlight Driver"
>> + depends on ARCH_APPLE || COMPILE_TEST
>> + default y
>> + help
>> + Say Y to enable the backlight driver for backlight controllers
>> + attached via the Apple DWI 2-wire interface which is found in some
>> + Apple iPhones, iPads and iPod touches.
>
> Mixed/messed indentation. Some spaces?
Ack. Will be fixed for v4.
>
>> +
>> + To compile this driver as a module, choose M here: the module will
>> + be called dwi_bl.
>> +
>> config BACKLIGHT_IPAQ_MICRO
>> tristate "iPAQ microcontroller backlight driver"
>> depends on MFD_IPAQ_MICRO
>> diff --git a/drivers/video/backlight/Makefile b/drivers/video/backlight/Makefile
>> index 8fc98f760a8a..0a569d7f0210 100644
>> --- a/drivers/video/backlight/Makefile
>> +++ b/drivers/video/backlight/Makefile
>> @@ -28,6 +28,7 @@ obj-$(CONFIG_BACKLIGHT_BD6107) += bd6107.o
>> obj-$(CONFIG_BACKLIGHT_CLASS_DEVICE) += backlight.o
>> obj-$(CONFIG_BACKLIGHT_DA903X) += da903x_bl.o
>> obj-$(CONFIG_BACKLIGHT_DA9052) += da9052_bl.o
>> +obj-$(CONFIG_BACKLIGHT_APPLE_DWI) += dwi_bl.o
>
> Please do not introduce other sorting way - it is sorted by config name.
> Which will also point you to apple_bl.o and need of explaining the
> differences, e.g. why this cannot be one driver.
apple_bl is a driver backlight controllers on a PCI bus on Intel Macs,
which is
completely different from the Samsung-derived DWI block this driver
deals with.
This will be explained in the commit message for v4.
>
>
>> obj-$(CONFIG_BACKLIGHT_EP93XX) += ep93xx_bl.o
>> obj-$(CONFIG_BACKLIGHT_GPIO) += gpio_backlight.o
>> obj-$(CONFIG_BACKLIGHT_HP680) += hp680_bl.o
>> diff --git a/drivers/video/backlight/dwi_bl.c b/drivers/video/backlight/dwi_bl.c
>> new file mode 100644
>> index 000000000000..d4bfd74b3129
>> --- /dev/null
>> +++ b/drivers/video/backlight/dwi_bl.c
>> @@ -0,0 +1,124 @@
>> +// SPDX-License-Identifier: GPL-2.0 OR MIT
>> +/*
>> + * Driver for backlight controllers attached via Apple DWI 2-wire interface
>> + *
>> + * Copyright (c) 2024 Nick Chan <towinchenmi@...il.com>
>> + */
>> +
>> +#include <linux/module.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/io.h>
>> +#include <linux/backlight.h>
>> +
>> +#define DWI_BL_CTL 0x0
>> +#define DWI_BL_CTL_SEND1 BIT(0)
>> +#define DWI_BL_CTL_SEND2 BIT(4)
>> +#define DWI_BL_CTL_SEND3 BIT(5)
>> +#define DWI_BL_CTL_LE_DATA BIT(6)
>> +/* Only used on Apple A9 and later */
>> +#define DWI_BL_CTL_SEND4 BIT(12)
>> +
>> +#define DWI_BL_CMD 0x4
>> +#define DWI_BL_CMD_TYPE GENMASK(31, 28)
>> +#define DWI_BL_CMD_TYPE_SET_BRIGHTNESS 0xa
>> +#define DWI_BL_CMD_DATA GENMASK(10, 0)
>> +
>> +#define DWI_BL_CTL_SEND (DWI_BL_CTL_SEND1 | \
>> + DWI_BL_CTL_SEND2 | \
>> + DWI_BL_CTL_SEND3 | \
>> + DWI_BL_CTL_LE_DATA | \
>> + DWI_BL_CTL_SEND4)
>> +
>> +#define DWI_BL_MAX_BRIGHTNESS 2047
>> +
>> +struct apple_dwi_bl {
>> + void __iomem *base;
>> +};
>> +
>> +static int dwi_bl_update_status(struct backlight_device *bl)
>> +{
>> + struct apple_dwi_bl *dwi_bl = bl_get_data(bl);
>> +
>> + int brightness = backlight_get_brightness(bl);
>> +
>> + u32 cmd = 0;
>> +
>> + cmd |= FIELD_PREP(DWI_BL_CMD_DATA, brightness);
>> + cmd |= FIELD_PREP(DWI_BL_CMD_TYPE, DWI_BL_CMD_TYPE_SET_BRIGHTNESS);
>> +
>> + writel(cmd, dwi_bl->base + DWI_BL_CMD);
>> + writel(DWI_BL_CTL_SEND, dwi_bl->base + DWI_BL_CTL);
>> +
>> + return 0;
>> +}
>> +
>> +static int dwi_bl_get_brightness(struct backlight_device *bl)
>> +{
>> + struct apple_dwi_bl *dwi_bl = bl_get_data(bl);
>> +
>> + u32 cmd = readl(dwi_bl->base + DWI_BL_CMD);
>> +
>> + return FIELD_GET(DWI_BL_CMD_DATA, cmd);
>> +}
>> +
>> +static const struct backlight_ops dwi_bl_ops = {
>> + .options = BL_CORE_SUSPENDRESUME,
>> + .get_brightness = dwi_bl_get_brightness,
>> + .update_status = dwi_bl_update_status
>> +};
>> +
>> +static int dwi_bl_probe(struct platform_device *dev)
>> +{
>> + struct apple_dwi_bl *dwi_bl;
>> + struct backlight_device *bl;
>> + struct backlight_properties props;
>> + struct resource *res;
>> +
>> + dwi_bl = devm_kzalloc(&dev->dev, sizeof(struct apple_dwi_bl), GFP_KERNEL);
>
> sizeof(*)
>
>> + if (!dwi_bl)
>> + return -ENOMEM;
>> +
>> + res = platform_get_resource(dev, IORESOURCE_MEM, 0);
>> + if (!res)
>> + return -ENXIO;
>> +
>> + dwi_bl->base = devm_ioremap_resource(&dev->dev, res);
>
> Use helper for these two, devm_platform_get_and_ioremap_resource() or
> similar.
Ack. Will be fixed for v4.
>
> Best regards,
> Krzysztof
>
Nick Chan
Powered by blists - more mailing lists