Skip to contentSkip to conversation

fpga: mgr: altera-ps-spi: enable usage on non-dt platforms

PATCH Superseded Anatolij Gustschin 2 participants 4 messages Nov 1, 2018 – Nov 16, 2018 FPGA manager framework

v3 of 5 Superseded Latest is v5 →

lore
Share & tools
lore
#0 lore raw

PATCH v3 1/1 fpga: mgr: altera-ps-spi: enable usage on non-dt platforms

Driver probing fails on non-dt platforms since of_match_device() always returns NULL here. Add device names and matching driver data to the spi_device_id table. This allows driver binding to dynamically added PS-SPI devices (e.g. added via spi_new_device() after hot-plugging).

---
Changes in v3:
- don't use pointers to device data struct in driver_data

Changes in v2:
- removed not necessary braces {}
- dropped not needed !id check

 drivers/fpga/altera-ps-spi.c | 36 +++++++++++++++++++++++++++++++-----
 1 file changed, 31 insertions(+), 5 deletions(-)
drivers/fpga/altera-ps-spi.c +31 -5
index 33aafda50af5..78d809194b7d 100644
@@ -75,6 +75,11 @@ static struct altera_ps_data a10_data = {
 	.t_st2ck_us = 10, /* min(t_ST2CK) */
 };
 
+static const struct altera_ps_data *altera_ps_data_map[] = {
+	&c5_data,
+	&a10_data,
+};
+
 static const struct of_device_id of_ef_match[] = {
 	{ .compatible = "altr,fpga-passive-serial", .data = &c5_data },
 	{ .compatible = "altr,fpga-arria10-passive-serial", .data = &a10_data },
@@ -234,6 +239,19 @@ static const struct fpga_manager_ops altera_ps_ops = {
 	.write_complete = altera_ps_write_complete,
 };
 
+static const struct altera_ps_data *id_to_data(const struct spi_device_id *id)
+{
+	const struct altera_ps_data *data;
+	unsigned int i;
+
+	for (i = 0; i < ARRAY_SIZE(altera_ps_data_map); i++) {
+		data = altera_ps_data_map[i];
+		if (data && data->devtype == id->driver_data)
+			return data;
+	}
+	return NULL;
+}
+
 static int altera_ps_probe(struct spi_device *spi)
 {
 	struct altera_ps_conf *conf;
@@ -244,11 +262,17 @@ static int altera_ps_probe(struct spi_device *spi)
 	if (!conf)
 		return -ENOMEM;
 
-	of_id = of_match_device(of_ef_match, &spi->dev);
-	if (!of_id)
-		return -ENODEV;
+	if (spi->dev.of_node) {
+		of_id = of_match_device(of_ef_match, &spi->dev);
+		if (!of_id)
+			return -ENODEV;
+		conf->data = of_id->data;
+	} else {
+		conf->data = id_to_data(spi_get_device_id(spi));
+		if (!conf->data)
+			return -ENODEV;
+	}
 
-	conf->data = of_id->data;
 	conf->spi = spi;
 	conf->config = devm_gpiod_get(&spi->dev, "nconfig", GPIOD_OUT_LOW);
 	if (IS_ERR(conf->config)) {
@@ -294,7 +318,9 @@ static int altera_ps_remove(struct spi_device *spi)
 }
 
 static const struct spi_device_id altera_ps_spi_ids[] = {
-	{"cyclone-ps-spi", 0},
+	{ "cyclone-ps-spi", CYCLONE5 },
+	{ "fpga-passive-serial", CYCLONE5 },
+	{ "fpga-arria10-passive-serial", ARRIA10 },
 	{}
 };
 MODULE_DEVICE_TABLE(spi, altera_ps_spi_ids);
Signature · 1 line
2.17.1
#1 lore raw

Hi Alan,

Quoted 10 lines · On Thu, 1 Nov 2018 22:22:31 +0100 Anatolij Gustschin [email protected] wrote:

Driver probing fails on non-dt platforms since of_match_device() always returns NULL here. Add device names and matching driver data to the spi_device_id table. This allows driver binding to dynamically added PS-SPI devices (e.g. added via spi_new_device() after hot-plugging).

---
Changes in v3:
- don't use pointers to device data struct in driver_data

Ping. Could this be queued for -next?

Thanks,
Anatolij

#2 lore raw
In reply to Anatolij Gustschin

On Thu, Nov 1, 2018 at 4:22 PM Anatolij Gustschin <[email protected]> wrote:

Hi Anatolij,

Quoted 6 lines

Driver probing fails on non-dt platforms since of_match_device() always returns NULL here. Add device names and matching driver data to the spi_device_id table. This allows driver binding to dynamically added PS-SPI devices (e.g. added via spi_new_device() after hot-plugging).

Need to update the header to explain what has been added since v2 (added spi ids and an array and code to map spi ids to data).

Quoted 18 lines

---
Changes in v3:
- don't use pointers to device data struct in driver_data

Changes in v2:
- removed not necessary braces {}
- dropped not needed !id check

 drivers/fpga/altera-ps-spi.c | 36 +++++++++++++++++++++++++++++++-----
 1 file changed, 31 insertions(+), 5 deletions(-)
drivers/fpga/altera-ps-spi.c +0 -0
index 33aafda50af5..78d809194b7d 100644

drivers/fpga/altera-ps-spi.c static struct altera_ps_data a10_data = { · line 78

Quoted 4 lines
drivers/fpga/altera-ps-spi.c +1 -0
@@ -75,6 +75,11 @@ static struct altera_ps_data a10_data = {
        .t_st2ck_us = 10, /* min(t_ST2CK) */
 };
 
+static const struct altera_ps_data *altera_ps_data_map[] = {

Please add a comment that array index is enum altera_ps_devtype or something like that. Make it easy for future generations to do the right thing. :)

Quoted 8 lines
drivers/fpga/altera-ps-spi.c +4 -0
+       &c5_data,
+       &a10_data,
+};
+
 static const struct of_device_id of_ef_match[] = {
        { .compatible = "altr,fpga-passive-serial", .data = &c5_data },
        { .compatible = "altr,fpga-arria10-passive-serial", .data = &a10_data },

drivers/fpga/altera-ps-spi.c static const struct fpga_manager_ops altera_ps_ops = { · line 247

Quoted 9 lines
drivers/fpga/altera-ps-spi.c +6 -0
@@ -234,6 +239,19 @@ static const struct fpga_manager_ops altera_ps_ops = {
        .write_complete = altera_ps_write_complete,
 };
 
+static const struct altera_ps_data *id_to_data(const struct spi_device_id *id)
+{
+       const struct altera_ps_data *data;
+       unsigned int i;
+
+       for (i = 0; i < ARRAY_SIZE(altera_ps_data_map); i++) {

At this point, id->driver_data == CYCLONE5 or ARRIA10 (more added in the future) so we don't need a loop to search the mapping array for a
matching id. Would something like the following work instead of
this for loop? I understand you wanted to do some checking, this accomplishes some checking but will exit NULL if someone extends the array wrongly.

kernel_ulong_t devtype = id->driver_data;

/* someone added a altera_ps_devtype without adding to the map array */ if (devtype >= ARRAY_SIZE(altera_ps_data_map))
return NULL;

data = altera_ps_data_map[devtype];
if (!data || data->devtype != devtype)

    return NULL;

return data;

Quoted 44 lines
drivers/fpga/altera-ps-spi.c +20 -6
+               data = altera_ps_data_map[i];
+               if (data && data->devtype == id->driver_data)
+                       return data;
+       }
+       return NULL;
+}
+
 static int altera_ps_probe(struct spi_device *spi)
 {
        struct altera_ps_conf *conf;
@@ -244,11 +262,17 @@ static int altera_ps_probe(struct spi_device *spi)
        if (!conf)
                return -ENOMEM;
 
-       of_id = of_match_device(of_ef_match, &spi->dev);
-       if (!of_id)
-               return -ENODEV;
+       if (spi->dev.of_node) {
+               of_id = of_match_device(of_ef_match, &spi->dev);
+               if (!of_id)
+                       return -ENODEV;
+               conf->data = of_id->data;
+       } else {
+               conf->data = id_to_data(spi_get_device_id(spi));
+               if (!conf->data)
+                       return -ENODEV;
+       }
 
-       conf->data = of_id->data;
        conf->spi = spi;
        conf->config = devm_gpiod_get(&spi->dev, "nconfig", GPIOD_OUT_LOW);
        if (IS_ERR(conf->config)) {
@@ -294,7 +318,9 @@ static int altera_ps_remove(struct spi_device *spi)
 }
 
 static const struct spi_device_id altera_ps_spi_ids[] = {
-       {"cyclone-ps-spi", 0},
+       { "cyclone-ps-spi", CYCLONE5 },
+       { "fpga-passive-serial", CYCLONE5 },
+       { "fpga-arria10-passive-serial", ARRIA10 },
        {}
 };
 MODULE_DEVICE_TABLE(spi, altera_ps_spi_ids);
--
Quoted 1 line

2.17.1

Thanks,
Alan

Quoted 1 line
#3 lore raw

Hi Alan,

On Thu, 15 Nov 2018 16:24:21 -0600
Alan Tull [email protected] wrote:
...

Quoted 2 lines

Need to update the header to explain what has been added since v2 (added spi ids and an array and code to map spi ids to data).

Done in v4.

...

Quoted 5 lines
Quoted 1 line

+static const struct altera_ps_data *altera_ps_data_map[] = {

Please add a comment that array index is enum altera_ps_devtype or something like that. Make it easy for future generations to do the right thing. :)

Done in v4.

...

Quoted 5 lines
Quoted 5 lines
drivers/fpga/altera-ps-spi.c +5 -0
+static const struct altera_ps_data *id_to_data(const struct spi_device_id *id)
+{
+       const struct altera_ps_data *data;
+       unsigned int i;
+
Quoted 8 lines
Quoted 1 line

+ for (i = 0; i < ARRAY_SIZE(altera_ps_data_map); i++) {

At this point, id->driver_data == CYCLONE5 or ARRIA10 (more added in the future) so we don't need a loop to search the mapping array for a
matching id. Would something like the following work instead of
this for loop? I understand you wanted to do some checking, this accomplishes some checking but will exit NULL if someone extends the array wrongly.

Yes, this works.

Quoted 11 lines

kernel_ulong_t devtype = id->driver_data;

/* someone added a altera_ps_devtype without adding to the map array */ if (devtype >= ARRAY_SIZE(altera_ps_data_map))
return NULL;

data = altera_ps_data_map[devtype];
if (!data || data->devtype != devtype)
return NULL;

return data;

Okay, v4 uses this code.

Thanks,
Anatolij