Message ID | 20240114152759.1040563-2-andriy.shevchenko@linux.intel.com |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel+bounces-25502-ouuuleilei=gmail.com@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a05:693c:2614:b0:101:6a76:bbe3 with SMTP id mm20csp1248751dyc; Sun, 14 Jan 2024 07:29:09 -0800 (PST) X-Google-Smtp-Source: AGHT+IEGQPcW0PPatsmestmXWUaOzWikCsBRdDqiI737P0Le2b6hID6tstorwnafSf75J635QT+E X-Received: by 2002:a17:907:d307:b0:a2c:d1c1:8a27 with SMTP id vg7-20020a170907d30700b00a2cd1c18a27mr2072347ejc.27.1705246148961; Sun, 14 Jan 2024 07:29:08 -0800 (PST) ARC-Seal: i=1; a=rsa-sha256; t=1705246148; cv=none; d=google.com; s=arc-20160816; b=ZxI7V7Y2eYGSEI+XxwGmKZfnB23VzyY7MYdxuewADRRqv8VC8vdsYhpzQeiWFUiUaP K/4swcj5iwhHo3FW4QfqYSvIbPcMnd8zqj7ZWRQLKTe1sQKcof1SRlNQJrrmfaH/P124 HtcgDUeu+Wdl6CBQV9XuJw7de71MJ7TZ0ZNPRaFXT3HNCjYlQJ6admrCj8pQ1I+tQRni CqIcgM9OIEjPAi6R6mX2dhuX+2N0NJWqqoafcmant5TfD610Mbis0OI0e6PK9Lh3Vrn6 liegA7pEPk4nHBXKuHNx7bhLewh/fbZCcokBLR9BmoAP0Rtvm6ex6cBJ2NLyMjI3XW1v l3WQ== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=content-transfer-encoding:mime-version:list-unsubscribe :list-subscribe:list-id:precedence:references:in-reply-to:message-id :date:subject:cc:to:from:dkim-signature; bh=5DvAe22wmD+D3W9JtGiPwnW9UYeOOt1k3g0EghLiGvM=; fh=mgFXmQvNwPRRqTo0F2bG2XoRFpGxhFpM0UrwgLf/FtA=; b=ctG2k9e/JOQQqsV9oQzUrELKeGdQk88fvTOGg8SpVzXLxYHLjDE+7p5TCX379sFwjN nQ2aej/MLguArROdnBbEooQNaCCLLsfJAg4NWao7dVYnr5lZjlkk+1BTwgVFJ6z/7WpN R4Lr4CEOtyVT9gqPK1tJ2oi4W1N7KeByQLy37xxpJ+PwEsDCZ5VHEHlSjSmqHBOTdz5+ iwk9rdnJOR65GefxpT5QbA+bCSwNi693pYoMvPN4HmWT29LlfQ0Q257vsI31VmQSH84x 7R/NyAYAWSd+B24DqFwsYsKsRxAsqPycXeyilBcqCN1qUpAA98D7Cqx889DFp520TvX/ 6DQA== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@intel.com header.s=Intel header.b=k5CtHXcP; spf=pass (google.com: domain of linux-kernel+bounces-25502-ouuuleilei=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) smtp.mailfrom="linux-kernel+bounces-25502-ouuuleilei=gmail.com@vger.kernel.org"; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=intel.com Received: from am.mirrors.kernel.org (am.mirrors.kernel.org. [147.75.80.249]) by mx.google.com with ESMTPS id bk23-20020a170906b0d700b00a2cebb54770si1821145ejb.722.2024.01.14.07.29.08 for <ouuuleilei@gmail.com> (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Sun, 14 Jan 2024 07:29:08 -0800 (PST) Received-SPF: pass (google.com: domain of linux-kernel+bounces-25502-ouuuleilei=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249; Authentication-Results: mx.google.com; dkim=pass header.i=@intel.com header.s=Intel header.b=k5CtHXcP; spf=pass (google.com: domain of linux-kernel+bounces-25502-ouuuleilei=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) smtp.mailfrom="linux-kernel+bounces-25502-ouuuleilei=gmail.com@vger.kernel.org"; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=intel.com Received: from smtp.subspace.kernel.org (wormhole.subspace.kernel.org [52.25.139.140]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by am.mirrors.kernel.org (Postfix) with ESMTPS id 921271F21479 for <ouuuleilei@gmail.com>; Sun, 14 Jan 2024 15:29:08 +0000 (UTC) Received: from localhost.localdomain (localhost.localdomain [127.0.0.1]) by smtp.subspace.kernel.org (Postfix) with ESMTP id B58B1F4EF; Sun, 14 Jan 2024 15:28:10 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=intel.com header.i=@intel.com header.b="k5CtHXcP" Received: from mgamail.intel.com (mgamail.intel.com [192.198.163.10]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 848A32581; Sun, 14 Jan 2024 15:28:06 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=linux.intel.com Authentication-Results: smtp.subspace.kernel.org; spf=none smtp.mailfrom=linux.intel.com DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=intel.com; i=@intel.com; q=dns/txt; s=Intel; t=1705246086; x=1736782086; h=from:to:cc:subject:date:message-id:in-reply-to: references:mime-version:content-transfer-encoding; bh=2P1HUQEL9BH9iq7NRkO7iC2HHKfx0MlaeZdmt4UObTc=; b=k5CtHXcPKs5UZh4vG/xKnxY5aMXq/idlYz7640AsZXFkSPxy2NAGknkC GFI3aMs74hH64Ot9UYkTdeLBdco5Y4ziEap0AeKUMozioYCPqIPKRegQj DHTD0383RYzSwreClcFXoBDPjOVMDVoYkxWF962zy3743JTK0qcH31q4u 2Lccl5mfY30HLsnL93wW3KtG4Vr4tQE1zJQbJz2OXgO/il9zsmGB/O9Ai gKtbMOcy5aXCy1mntIkiTxT8j4bJe9uK+cHqOQzxJg65f0u8f13r4SvAW 1NzIRjAYY7aSNSeB6XLoe/Xv+RZiE3m8JTsmpXtRD+QILUaVxZTTU4/lu g==; X-IronPort-AV: E=McAfee;i="6600,9927,10953"; a="6829674" X-IronPort-AV: E=Sophos;i="6.04,194,1695711600"; d="scan'208";a="6829674" Received: from orsmga005.jf.intel.com ([10.7.209.41]) by fmvoesa104.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 14 Jan 2024 07:28:06 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=McAfee;i="6600,9927,10953"; a="956601546" X-IronPort-AV: E=Sophos;i="6.04,194,1695711600"; d="scan'208";a="956601546" Received: from black.fi.intel.com ([10.237.72.28]) by orsmga005.jf.intel.com with ESMTP; 14 Jan 2024 07:28:03 -0800 Received: by black.fi.intel.com (Postfix, from userid 1003) id 239071A3; Sun, 14 Jan 2024 17:28:02 +0200 (EET) From: Andy Shevchenko <andriy.shevchenko@linux.intel.com> To: Andy Shevchenko <andriy.shevchenko@linux.intel.com>, dri-devel@lists.freedesktop.org, linux-fbdev@vger.kernel.org, linux-kernel@vger.kernel.org Cc: Lee Jones <lee@kernel.org>, Daniel Thompson <daniel.thompson@linaro.org>, Jingoo Han <jingoohan1@gmail.com>, Helge Deller <deller@gmx.de> Subject: [PATCH v1 1/4] backlight: hx8357: Make use of device properties Date: Sun, 14 Jan 2024 17:25:08 +0200 Message-ID: <20240114152759.1040563-2-andriy.shevchenko@linux.intel.com> X-Mailer: git-send-email 2.43.0.rc1.1.gbec44491f096 In-Reply-To: <20240114152759.1040563-1-andriy.shevchenko@linux.intel.com> References: <20240114152759.1040563-1-andriy.shevchenko@linux.intel.com> Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: <linux-kernel.vger.kernel.org> List-Subscribe: <mailto:linux-kernel+subscribe@vger.kernel.org> List-Unsubscribe: <mailto:linux-kernel+unsubscribe@vger.kernel.org> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-getmail-retrieved-from-mailbox: INBOX X-GMAIL-THRID: 1788080185587926398 X-GMAIL-MSGID: 1788080185587926398 |
Series |
backlight: hx8357: Clean up and make OF-independent
|
|
Commit Message
Andy Shevchenko
Jan. 14, 2024, 3:25 p.m. UTC
Convert the module to be property provider agnostic and allow
it to be used on non-OF platforms.
Include mod_devicetable.h explicitly to replace the dropped of.h
which included mod_devicetable.h indirectly.
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
drivers/video/backlight/hx8357.c | 14 ++++++++------
1 file changed, 8 insertions(+), 6 deletions(-)
Comments
Andy Shevchenko <andriy.shevchenko@linux.intel.com> writes: Hello Andy, > Convert the module to be property provider agnostic and allow > it to be used on non-OF platforms. > > Include mod_devicetable.h explicitly to replace the dropped of.h > which included mod_devicetable.h indirectly. > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > --- > drivers/video/backlight/hx8357.c | 14 ++++++++------ > 1 file changed, 8 insertions(+), 6 deletions(-) > > diff --git a/drivers/video/backlight/hx8357.c b/drivers/video/backlight/hx8357.c > index bf18337ff0c2..c7fd10d55c5d 100644 > --- a/drivers/video/backlight/hx8357.c > +++ b/drivers/video/backlight/hx8357.c > @@ -8,9 +8,9 @@ > #include <linux/delay.h> > #include <linux/gpio/consumer.h> > #include <linux/lcd.h> > +#include <linux/mod_devicetable.h> > #include <linux/module.h> > -#include <linux/of.h> > -#include <linux/of_device.h> > +#include <linux/property.h> > #include <linux/spi/spi.h> > > #define HX8357_NUM_IM_PINS 3 > @@ -564,6 +564,8 @@ static struct lcd_ops hx8357_ops = { > .get_power = hx8357_get_power, > }; > > +typedef int (*hx8357_init)(struct lcd_device *); > + This kind of typedef usage is frowned upon in the Linux coding style [0] (per my understanding at least) and indeed in my opinion it makes harder to grep. [0] https://www.kernel.org/doc/Documentation/process/coding-style.rst > static const struct of_device_id hx8357_dt_ids[] = { > { > .compatible = "himax,hx8357", > @@ -582,7 +584,7 @@ static int hx8357_probe(struct spi_device *spi) > struct device *dev = &spi->dev; > struct lcd_device *lcdev; > struct hx8357_data *lcd; > - const struct of_device_id *match; > + hx8357_init init; > int i, ret; > > lcd = devm_kzalloc(&spi->dev, sizeof(*lcd), GFP_KERNEL); > @@ -597,8 +599,8 @@ static int hx8357_probe(struct spi_device *spi) > > lcd->spi = spi; > > - match = of_match_device(hx8357_dt_ids, &spi->dev); > - if (!match || !match->data) > + init = device_get_match_data(dev); > + if (!init) > return -EINVAL; > > lcd->reset = devm_gpiod_get(dev, "reset", GPIOD_OUT_LOW); > @@ -627,7 +629,7 @@ static int hx8357_probe(struct spi_device *spi) > > hx8357_lcd_reset(lcdev); > > - ret = ((int (*)(struct lcd_device *))match->data)(lcdev); This is what I mean, before it was clear what was stored in match->data. But after you changes, what is returned by the device_get_match_data() function is opaque and you need to look at the typedef hx8357_init to figure that out. No strong opinion though and I see other drivers doing the same (but no other driver in drivers/video/backlight). Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>
On Mon, Jan 15, 2024 at 09:20:46AM +0100, Javier Martinez Canillas wrote: > Andy Shevchenko <andriy.shevchenko@linux.intel.com> writes: .. > > +typedef int (*hx8357_init)(struct lcd_device *); > > This kind of typedef usage is frowned upon in the Linux coding style [0] > (per my understanding at least) and indeed in my opinion it makes harder > to grep. > > [0] https://www.kernel.org/doc/Documentation/process/coding-style.rst Thanks for pointing this out. However, this piece does _not_ clarify typedef:s for function pointers which I personally find a good to have. .. > > - ret = ((int (*)(struct lcd_device *))match->data)(lcdev); > > This is what I mean, before it was clear what was stored in match->data. > But after you changes, what is returned by the device_get_match_data() > function is opaque and you need to look at the typedef hx8357_init to > figure that out. The above is so ugly in my opinion, that justifies using typedef:s even if you are quite skeptical about them. .. > No strong opinion though and I see other drivers doing the same (but no > other driver in drivers/video/backlight). > > Reviewed-by: Javier Martinez Canillas <javierm@redhat.com> Thank you for the review!
On Sun, Jan 21, 2024 at 03:48:05PM +0200, Andy Shevchenko wrote: > On Mon, Jan 15, 2024 at 09:20:46AM +0100, Javier Martinez Canillas wrote: > > Andy Shevchenko <andriy.shevchenko@linux.intel.com> writes: > > ... > > > > +typedef int (*hx8357_init)(struct lcd_device *); > > > > This kind of typedef usage is frowned upon in the Linux coding style [0] > > (per my understanding at least) and indeed in my opinion it makes harder > > to grep. > > > > [0] https://www.kernel.org/doc/Documentation/process/coding-style.rst > > Thanks for pointing this out. However, this piece does _not_ clarify typedef:s > for function pointers which I personally find a good to have. > > ... > > > > - ret = ((int (*)(struct lcd_device *))match->data)(lcdev); > > > > This is what I mean, before it was clear what was stored in match->data. > > But after you changes, what is returned by the device_get_match_data() > > function is opaque and you need to look at the typedef hx8357_init to > > figure that out. > > The above is so ugly in my opinion, that justifies using typedef:s even > if you are quite skeptical about them. FWIW I was pretty skeptical about it to. Largely because the three touchs (typedef, variable initialization, use) spread things around a bit too much. Can we at least name the type to make it obvious that it is a function pointer? Something like hx8357_init_fn . Daniel.
On Sun, Jan 14, 2024 at 05:25:08PM +0200, Andy Shevchenko wrote: > Convert the module to be property provider agnostic and allow > it to be used on non-OF platforms. > > Include mod_devicetable.h explicitly to replace the dropped of.h > which included mod_devicetable.h indirectly. > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > --- > drivers/video/backlight/hx8357.c | 14 ++++++++------ > 1 file changed, 8 insertions(+), 6 deletions(-) > > diff --git a/drivers/video/backlight/hx8357.c b/drivers/video/backlight/hx8357.c > index bf18337ff0c2..c7fd10d55c5d 100644 > --- a/drivers/video/backlight/hx8357.c > +++ b/drivers/video/backlight/hx8357.c > @@ -8,9 +8,9 @@ > #include <linux/delay.h> > #include <linux/gpio/consumer.h> > #include <linux/lcd.h> > +#include <linux/mod_devicetable.h> > #include <linux/module.h> > -#include <linux/of.h> > -#include <linux/of_device.h> > +#include <linux/property.h> > #include <linux/spi/spi.h> > > #define HX8357_NUM_IM_PINS 3 > @@ -564,6 +564,8 @@ static struct lcd_ops hx8357_ops = { > .get_power = hx8357_get_power, > }; > > +typedef int (*hx8357_init)(struct lcd_device *); > + > static const struct of_device_id hx8357_dt_ids[] = { > { > .compatible = "himax,hx8357", > @@ -582,7 +584,7 @@ static int hx8357_probe(struct spi_device *spi) > struct device *dev = &spi->dev; > struct lcd_device *lcdev; > struct hx8357_data *lcd; > - const struct of_device_id *match; > + hx8357_init init; As somewhere else in this thread, I'd find this a lot more readable as: hx8357_init_fn init_fn; Other than that, LGTM. Daniel.
On Wed, Jan 24, 2024 at 05:19:53PM +0000, Daniel Thompson wrote: > On Sun, Jan 14, 2024 at 05:25:08PM +0200, Andy Shevchenko wrote: .. > > +typedef int (*hx8357_init)(struct lcd_device *); > > + hx8357_init init; > > As somewhere else in this thread, I'd find this a lot more readable > as: > hx8357_init_fn init_fn; I agree with you, dunno why I haven't added _fn in the first place...
diff --git a/drivers/video/backlight/hx8357.c b/drivers/video/backlight/hx8357.c index bf18337ff0c2..c7fd10d55c5d 100644 --- a/drivers/video/backlight/hx8357.c +++ b/drivers/video/backlight/hx8357.c @@ -8,9 +8,9 @@ #include <linux/delay.h> #include <linux/gpio/consumer.h> #include <linux/lcd.h> +#include <linux/mod_devicetable.h> #include <linux/module.h> -#include <linux/of.h> -#include <linux/of_device.h> +#include <linux/property.h> #include <linux/spi/spi.h> #define HX8357_NUM_IM_PINS 3 @@ -564,6 +564,8 @@ static struct lcd_ops hx8357_ops = { .get_power = hx8357_get_power, }; +typedef int (*hx8357_init)(struct lcd_device *); + static const struct of_device_id hx8357_dt_ids[] = { { .compatible = "himax,hx8357", @@ -582,7 +584,7 @@ static int hx8357_probe(struct spi_device *spi) struct device *dev = &spi->dev; struct lcd_device *lcdev; struct hx8357_data *lcd; - const struct of_device_id *match; + hx8357_init init; int i, ret; lcd = devm_kzalloc(&spi->dev, sizeof(*lcd), GFP_KERNEL); @@ -597,8 +599,8 @@ static int hx8357_probe(struct spi_device *spi) lcd->spi = spi; - match = of_match_device(hx8357_dt_ids, &spi->dev); - if (!match || !match->data) + init = device_get_match_data(dev); + if (!init) return -EINVAL; lcd->reset = devm_gpiod_get(dev, "reset", GPIOD_OUT_LOW); @@ -627,7 +629,7 @@ static int hx8357_probe(struct spi_device *spi) hx8357_lcd_reset(lcdev); - ret = ((int (*)(struct lcd_device *))match->data)(lcdev); + ret = init(lcdev); if (ret) { dev_err(&spi->dev, "Couldn't initialize panel\n"); return ret;