Re: [PATCH v2 1/2] leds: rgb: s2m: Wire up of_match_table in platform driver

[Kaustabh Chakraborty]

On 2026-07-22 21:45 +05:30, kr494167 wrote:
> From: Surendra <kr494167@xxxxxxxxx>
>
> The driver defines and registers s2m_rgb_of_match_table via
> MODULE_DEVICE_TABLE(of, ...) but never assigns it to the platform
> driver's .of_match_table field. As a result, the kernel never matches
> this driver against a DT node with compatible "samsung,s2mu005-rgb",
> making the MODULE_DEVICE_TABLE entry dead code.
>
> Wire the table up so that DT-based probing and module auto-loading
> work as intended. In addition, validate dev->parent in s2m_rgb_probe()
> to prevent a NULL pointer dereference if probed without parent driver data.
>
> Fixes: 63ccd117f425 ("leds: rgb: Add support for Samsung S2M series PMIC RGB LED device")
> Signed-off-by: Surendra <kr494167@xxxxxxxxx>
> ---
>  drivers/leds/rgb/leds-s2m-rgb.c | 10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/leds/rgb/leds-s2m-rgb.c b/drivers/leds/rgb/leds-s2m-rgb.c
> index d239f54eee90..7ac2b1160c1e 100644
> --- a/drivers/leds/rgb/leds-s2m-rgb.c
> +++ b/drivers/leds/rgb/leds-s2m-rgb.c
> @@ -363,11 +363,18 @@ static const struct mc_subled s2mu005_rgb_subled_info[] = {
>  static int s2m_rgb_probe(struct platform_device *pdev)
>  {
>  	struct device *dev = &pdev->dev;
> -	struct sec_pmic_dev *pmic_drvdata = dev_get_drvdata(dev->parent);
> +	struct sec_pmic_dev *pmic_drvdata;

This seems to be a separate change. I guess it's preferable having it
as a separate patch.

>  	struct s2m_rgb *rgb;
>  	struct led_init_data init_data = {};
>  	int ret;
>  
> +	if (!dev->parent)
> +		return -ENODEV;
> +
> +	pmic_drvdata = dev_get_drvdata(dev->parent);
> +	if (!pmic_drvdata)
> +		return -ENODEV;
> +

1. This is a MFD sub-device, dev->parent shall always be present. But
there's no harm in being extra careful here.

2. For error codes other than -ENOMEM, we do not return from probe this
way, instead:

	return dev_err_probe(dev, -ENODEV, "error message\n");

>  	rgb = devm_kzalloc(dev, sizeof(*rgb), GFP_KERNEL);
>  	if (!rgb)
>  		return -ENOMEM;
> @@ -415,6 +422,7 @@ MODULE_DEVICE_TABLE(of, s2m_rgb_of_match_table);
>  static struct platform_driver s2m_rgb_driver = {
>  	.driver = {
>  		.name = "s2m-rgb",
> +		.of_match_table = s2m_rgb_of_match_table,

By the way, it works just fine without it, when built as a module.

The reason it wasn't added was in a comment in a previous revision [1],
but it was removed after reviews.

[1] https://lore.kernel.org/all/20260225-s2mu005-pmic-v3-9-b4afee947603@xxxxxxxxxxx/

>  	},
>  	.probe = s2m_rgb_probe,
>  	.id_table = s2m_rgb_id_table,