Message-ID: <20240531095650.GD8682@google.com>
Date: Fri, 31 May 2024 10:56:50 +0100
From: Lee Jones <lee@...nel.org>
To: Yoshinori Sato <ysato@...rs.sourceforge.jp>
Cc: linux-sh@...r.kernel.org, Damien Le Moal <dlemoal@...nel.org>,
	Niklas Cassel <cassel@...nel.org>, Rob Herring <robh@...nel.org>,
	Krzysztof Kozlowski <krzk+dt@...nel.org>,
	Conor Dooley <conor+dt@...nel.org>,
	Geert Uytterhoeven <geert+renesas@...der.be>,
	Michael Turquette <mturquette@...libre.com>,
	Stephen Boyd <sboyd@...nel.org>, David Airlie <airlied@...il.com>,
	Daniel Vetter <daniel@...ll.ch>,
	Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>,
	Maxime Ripard <mripard@...nel.org>,
	Thomas Zimmermann <tzimmermann@...e.de>,
	Thomas Gleixner <tglx@...utronix.de>,
	Bjorn Helgaas <bhelgaas@...gle.com>,
	Lorenzo Pieralisi <lpieralisi@...nel.org>,
	Krzysztof Wilczyński <kw@...ux.com>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	Jiri Slaby <jirislaby@...nel.org>,
	Magnus Damm <magnus.damm@...il.com>,
	Daniel Lezcano <daniel.lezcano@...aro.org>,
	Rich Felker <dalias@...c.org>,
	John Paul Adrian Glaubitz <glaubitz@...sik.fu-berlin.de>,
	Helge Deller <deller@....de>,
	Heiko Stuebner <heiko.stuebner@...rry.de>,
	Neil Armstrong <neil.armstrong@...aro.org>,
	Chris Morgan <macromorgan@...mail.com>,
	Sebastian Reichel <sre@...nel.org>,
	Linus Walleij <linus.walleij@...aro.org>,
	Arnd Bergmann <arnd@...db.de>,
	Masahiro Yamada <masahiroy@...nel.org>, Baoquan He <bhe@...hat.com>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Guenter Roeck <linux@...ck-us.net>,
	Kefeng Wang <wangkefeng.wang@...wei.com>,
	Stephen Rothwell <sfr@...b.auug.org.au>,
	Azeem Shaikh <azeemshaikh38@...il.com>, Guo Ren <guoren@...nel.org>,
	Max Filippov <jcmvbkbc@...il.com>,
	Jernej Skrabec <jernej.skrabec@...il.com>,
	Herve Codina <herve.codina@...tlin.com>,
	Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
	Anup Patel <apatel@...tanamicro.com>,
	Jacky Huang <ychuang3@...oton.com>,
	Hugo Villeneuve <hvilleneuve@...onoff.com>,
	Jonathan Corbet <corbet@....net>,
	Wolfram Sang <wsa+renesas@...g-engineering.com>,
	Uwe Kleine-König <u.kleine-koenig@...gutronix.de>,
	Christophe JAILLET <christophe.jaillet@...adoo.fr>,
	Sam Ravnborg <sam@...nborg.org>,
	Javier Martinez Canillas <javierm@...hat.com>,
	Sergey Shtylyov <s.shtylyov@....ru>,
	Laurent Pinchart <laurent.pinchart+renesas@...asonboard.com>,
	linux-ide@...r.kernel.org, devicetree@...r.kernel.org,
	linux-kernel@...r.kernel.org, linux-renesas-soc@...r.kernel.org,
	linux-clk@...r.kernel.org, dri-devel@...ts.freedesktop.org,
	linux-pci@...r.kernel.org, linux-serial@...r.kernel.org,
	linux-fbdev@...r.kernel.org
Subject: Re: [DO NOT MERGE v8 23/36] mfd: sm501: Convert platform_data to OF
 property

On Wed, 29 May 2024, Yoshinori Sato wrote:

> Various parameters of SM501 can be set using platform_data,
> so parameters cannot be passed in the DeviceTree target.
> Expands the parameters set in platform_data so that they can be
> specified using DeviceTree properties.
> 
> Signed-off-by: Yoshinori Sato <ysato@...rs.sourceforge.jp>
> ---
>  drivers/mfd/sm501.c           | 238 ++++++++++++++++++++++++++++++++++
>  drivers/video/fbdev/sm501fb.c |  87 +++++++++++++
>  2 files changed, 325 insertions(+)
> 
> diff --git a/drivers/mfd/sm501.c b/drivers/mfd/sm501.c
> index b3592982a83b..d373aded0c3b 100644
> --- a/drivers/mfd/sm501.c
> +++ b/drivers/mfd/sm501.c
> @@ -20,6 +20,7 @@
>  #include <linux/gpio/driver.h>
>  #include <linux/gpio/machine.h>
>  #include <linux/slab.h>
> +#include <linux/clk.h>
>  
>  #include <linux/sm501.h>
>  #include <linux/sm501-regs.h>
> @@ -82,6 +83,16 @@ struct sm501_devdata {
>  	unsigned int			 rev;
>  };
>  
> +struct sm501_config_props_uint {
> +	char *name;
> +	u32 shift;
> +};
> +
> +struct sm501_config_props_flag {
> +	char *clr_name;
> +	char *set_name;
> +	u32 bit;
> +};
>  
>  #define MHZ (1000 * 1000)
>  
> @@ -1370,6 +1381,227 @@ static int sm501_init_dev(struct sm501_devdata *sm)
>  	return 0;
>  }
>  
> +#define FIELD_WIDTH 4
> +struct dt_values {
> +	char *name;
> +	unsigned int offset;
> +	unsigned int width;
> +	char *val[(1 << FIELD_WIDTH) + 1];
> +};
> +
> +#define fld(_name, _offset, _width, ...)	\
> +	{ \
> +		.name = _name, \
> +		.offset = _offset, \
> +		.width = _width,	\
> +		.val = { __VA_ARGS__, NULL},	\
> +	}
> +
> +static const struct dt_values misc_timing[] = {
> +	fld("ex", 28, 4,
> +	    "none", "16", "32", "48", "64", "80", "96", "112",
> +	    "128", "144", "160", "176", "192", "208", "224", "240"),
> +	fld("xc", 24, 2, "internal-pll", "hclk", "gpio30"),
> +	fld("us", 23, 1, "disable", "enable"),
> +	fld("ssm1", 20, 1, "288", "divider"),
> +	fld("sm1", 16, 4,
> +	    "1", "2", "4", "8", "16", "32", "64", "128",
> +	    "3", "6", "12", "24", "48", "96", "192", "384"),
> +	fld("ssm0", 12, 1, "288", "divider"),
> +	fld("sm0", 8, 4,
> +	    "1", "2", "4", "8", "16", "32", "64", "128",
> +	    "3", "6", "12", "24", "48", "96", "192", "384"),
> +	fld("deb", 7, 1, "input-reference", "output"),
> +	fld("a", 6, 1, "no-acpi", "acpi"),
> +	fld("divider", 4, 2, "336", "288", "240", "192"),
> +	fld("u", 3, 1, "normal", "simulation"),
> +	fld("delay", 0, 3, "none", "0.5", "1.0", "1.5", "2.0", "2.5"),
> +	{ .name = NULL },
> +};
> +
> +static const struct dt_values misc_control[] = {
> +	fld("pad", 30, 2, "24", "12", "8"),
> +	fld("usbclk", 28, 2, "xtal", "96", "48"),
> +	fld("ssp", 27, 1, "uart1", "ssp1"),
> +	fld("lat", 26, 1, "disable", "enable"),
> +	fld("fp", 25, 1, "18", "24"),
> +	fld("freq", 24, 1, "24", "12"),
> +	fld("refresh", 21, 2, "8", "16", "32", "64"),
> +	fld("hold", 18, 3, "fifo-empty", "8", "16", "24", "32"),
> +	fld("sh", 17, 1, "active-low", "active-high"),
> +	fld("ii", 16, 1, "normal", "inverted"),
> +	fld("pll", 15, 1, "disable", "enable"),
> +	fld("gap", 13, 2, "0"),
> +	fld("dac", 12, 1, "enable", "disable"),
> +	fld("mc", 11, 1, "cpu", "8051"),
> +	fld("bl", 10, 8, "1"),
> +	fld("usb", 9, 1, "master", "slave"),
> +	fld("vr", 4, 1, "0x1e00000", "0x3e00000"),
> +	{ .name = NULL },
> +};

I've been avoiding this set for a while now!

I appreciate the amount of work that you've put into this, but this is a
bit of a disaster.  It's a hell of lot of over-complex infrastructure
just to pull out some values from DT.

Forgive me if I have this wrong, but it looks like you're defining
various structs then populating static versions with hard-coded offsets
into DT arrays!  Then you have a bunch of hoop-jumpy functions to
firstly parse the offset-structs, then conduct look-ups to pull the
final value which in turn gets shifted into an encoded variable ready
for to write out to the registers.  Bonkers.

What does 'timing' even mean in this context?  Clocks?

What other devices require this kind of handling?  Why is this device so
different from all other supported devices to date?  Instead of
attempting to shoehorn this into a 20 year old driver, why not reshape
it to bring it into alignment with how we do things today?

E.g. handle all clocking from the clock driver, all display settings
(including timing?) from the display driver, etc.

-- 
Lee Jones [李琼斯]

Powered by blists - more mailing lists