Re: [DO NOT MERGE v8 23/36] mfd: sm501: Convert platform_data to OF property

[Lee Jones]

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@xxxxxxxxxxxxxxxxxxxx>
> ---
>  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 [李琼斯]