[1/4] leds: cpcap: add support for the keyboard light channel

The keyboard light channel is used on xt875 for the touchscreen
button lights.
This commit also adds a checks for the sucessfull return of
device_get_match_data.

Signed-off-by: Carl Philipp Klemm <philipp@uvos.xyz>
---
 drivers/leds/leds-cpcap.c    | 15 +++++++++++++++
 drivers/mfd/motorola-cpcap.c |  4 ++++
 2 files changed, 19 insertions(+)

Hi,

On Sun, Dec 04, 2022 at 11:43:10AM +0100, Carl Philipp Klemm wrote:
> The keyboard light channel is used on xt875 for the touchscreen
> button lights.
> This commit also adds a checks for the sucessfull return of
> device_get_match_data.

"this commit also adds ..." means, that the commit should be split
:)

> Signed-off-by: Carl Philipp Klemm <philipp@uvos.xyz>
> ---
>  drivers/leds/leds-cpcap.c    | 15 +++++++++++++++
>  drivers/mfd/motorola-cpcap.c |  4 ++++
>  2 files changed, 19 insertions(+)
> 
> diff --git a/drivers/leds/leds-cpcap.c b/drivers/leds/leds-cpcap.c
> index 7d41ce8c9bb1..11a9b857d8ea 100644
> --- a/drivers/leds/leds-cpcap.c
> +++ b/drivers/leds/leds-cpcap.c
> @@ -58,6 +58,15 @@ static const struct cpcap_led_info cpcap_led_cp = {
>  	.init_val	= 0x0008,
>  };
>  
> +/* keyboard led */
> +static const struct cpcap_led_info cpcap_led_kl = {
> +	.reg		= CPCAP_REG_KLC,
> +	.mask		= 0x0001,
> +	.limit		= 1,
> +	.init_mask	= 0x07FF,
> +	.init_val	= 0x07F0,
> +};
> +
>  struct cpcap_led {
>  	struct led_classdev led;
>  	const struct cpcap_led_info *info;
> @@ -152,6 +161,7 @@ static const struct of_device_id cpcap_led_of_match[] = {
>  	{ .compatible = "motorola,cpcap-led-blue",  .data = &cpcap_led_blue },
>  	{ .compatible = "motorola,cpcap-led-adl", .data = &cpcap_led_adl },
>  	{ .compatible = "motorola,cpcap-led-cp", .data = &cpcap_led_cp },
> +	{ .compatible = "motorola,cpcap-led-kl", .data = &cpcap_led_kl },
>  	{},
>  };
>  MODULE_DEVICE_TABLE(of, cpcap_led_of_match);
> @@ -168,6 +178,11 @@ static int cpcap_led_probe(struct platform_device *pdev)
>  	led->info = device_get_match_data(&pdev->dev);
>  	led->dev = &pdev->dev;
>  
> +	if (!led->info) {
> +		dev_warn(led->dev, "Can't get match data");
> +		return -ENODEV;
> +	}

If it's fatal, it should be dev_err and not dev_warn:

if (!led->info)
    return dev_err_probe(led->dev, -ENODEV, "Can't get match data");

-- Sebastian

> +
>  	if (led->info->reg == 0x0000) {
>  		dev_err(led->dev, "Unsupported LED");
>  		return -ENODEV;
> diff --git a/drivers/mfd/motorola-cpcap.c b/drivers/mfd/motorola-cpcap.c
> index 265464b5d7cc..57b3378a8829 100644
> --- a/drivers/mfd/motorola-cpcap.c
> +++ b/drivers/mfd/motorola-cpcap.c
> @@ -285,6 +285,10 @@ static const struct mfd_cell cpcap_mfd_devices[] = {
>  		.name          = "cpcap-led",
>  		.id            = 4,
>  		.of_compatible = "motorola,cpcap-led-cp",
> +	}, {
> +		.name          = "cpcap-led",
> +		.id            = 5,
> +		.of_compatible = "motorola,cpcap-led-kl",
>  	}, {
>  		.name          = "cpcap-codec",
>  	}
> -- 
> 2.38.1
> 
>

Hi,

On Sun, Dec 04, 2022 at 11:43:11AM +0100, Carl Philipp Klemm wrote:
> Previously led-cpcap devices where defined statically in mfd_cell
> of the parent mdf device. This causes issues for devices like
> xt875 that have less and different leds than xt894. Splitting the
> device like this is posssible, as in reality the cpcap led ip block
> is totaly independent from the mdf device and cpcap core.
> 
> Signed-off-by: Carl Philipp Klemm <philipp@uvos.xyz>

I don't follow. Can't you just use 'status = "disabled;"' for the
unavailable nodes?

-- Sebastian

Hi,

> I don't follow. Can't you just use 'status = "disabled;"' for the
> unavailable nodes?

Sure but unless im missing something, adding some nodes to a dts, one
for eatch led the device in question really has feals mutch better to
me than going arround and setting every led channel disabled for eatch
device that dosent use it. xt894 being the outlier here amoung the
cpcap devices we intend to support (xt894, xt860, xt875, xt910, xt912,
mb865, mz609 and mz617) in that it uses most of the extra led channels,
most of these use at most one (xt875, xt910, xt912, mb865) or zero
(mz609, mz617) extra channels.

Carl

Hi,

On Mon, Dec 05, 2022 at 07:15:48PM +0100, Carl Philipp Klemm wrote:
> > I don't follow. Can't you just use 'status = "disabled;"' for the
> > unavailable nodes?
> 
> Sure but unless im missing something, adding some nodes to a dts, one
> for eatch led the device in question really has feals mutch better to
> me than going arround and setting every led channel disabled for eatch
> device that dosent use it. xt894 being the outlier here amoung the
> cpcap devices we intend to support (xt894, xt860, xt875, xt910, xt912,
> mb865, mz609 and mz617) in that it uses most of the extra led channels,
> most of these use at most one (xt875, xt910, xt912, mb865) or zero
> (mz609, mz617) extra channels.

Just mark them all status = 'disabled'; in the common include and
then enable them on a per board basis. Just the same way as all the
SoC peripherals are handled nowadays.

-- Sebastian

On Sun, Dec 04, 2022 at 11:43:13AM +0100, Carl Philipp Klemm wrote:
> Removes non-functional leds from xt875 and adds its touchscreen
> button light
> 
> Signed-off-by: Carl Philipp Klemm <philipp@uvos.xyz>
> ---
>  .../arm/boot/dts/motorola-cpcap-mapphone.dtsi | 47 ++++++++-----------
>  .../arm/boot/dts/omap4-droid-bionic-xt875.dts |  7 +++
>  arch/arm/boot/dts/omap4-droid4-xt894.dts      | 14 ++++++
>  3 files changed, 41 insertions(+), 27 deletions(-)

Looks like you are breaking the ABI with this series. What happens in a 
system with only the DT change or only the kernel change?

Rob