RE: [PATCH v2 6/6] ACPI: bus-multi-instantiate: Add SPI support

[Stefan Binding]

Hi,

> -----Original Message-----
> From: Hans de Goede <hdegoede@xxxxxxxxxx>
> Sent: 21 December 2021 18:32
> To: Stefan Binding <sbinding@xxxxxxxxxxxxxxxxxxxxx>; Mark Brown
> <broonie@xxxxxxxxxx>; Rafael J . Wysocki <rafael@xxxxxxxxxx>; Len Brown
> <lenb@xxxxxxxxxx>; Mark Gross <markgross@xxxxxxxxxx>
> Cc: linux-kernel@xxxxxxxxxxxxxxx; linux-spi@xxxxxxxxxxxxxxx; linux-
> acpi@xxxxxxxxxxxxxxx; platform-driver-x86@xxxxxxxxxxxxxxx;
> patches@xxxxxxxxxxxxxxxxxxxxx
> Subject: Re: [PATCH v2 6/6] ACPI: bus-multi-instantiate: Add SPI support

> > +	ret = bmi_spi_count_resources(adev);
> > +	if (ret <= 0)
> > +		return ret;
> > +	count = ret;
> 
> Ok, so why not do the following here instead (and drop a whole bunch of
> functions above):
> 
> 	ret = acpi_dev_get_resources(adev, &r, bmi_spi_count, &count);
> 	if (ret < 0)
> 		return ret;
> 
> 	if (count <= 0) {
> 		acpi_dev_free_resource_list(&r);
> 		return count;
> 	}
> 
> 	/* Note we are not freeing the resource list yet here !!! */
> 
> > +
> > +	bmi->spi_devs = devm_kcalloc(dev, count, sizeof(*bmi->spi_devs),
> GFP_KERNEL);
> > +	if (!bmi->spi_devs)
> > +		return -ENOMEM;
> > +
> > +	acpi_data = bmi_spi_get_resources(dev, adev, count);
> > +	if (!acpi_data)
> > +		return -ENOMEM;
> 
> Remove the bmi_spi_get_resources() call here.
> 
> > +
> > +	for (i = 0; i < count && inst_array[i].type; i++) {
> 
> Write a new:
> 
> int bmi_get_spi_resource_by_index(list_head *resource_list, struct
> acpi_resource_spi_serialbus *sb_ret, int index)
> {}
> 
> Helper which walks the list and fills in *sb_ret with the Nth (matching index)
> SpiSerialBus resource found in the
> list.
> 
> And then do:
> 
> 		ret = bmi_get_spi_resource_by_index(&r, &sb, i);
> 		if (ret)
> 			return ret;
> 
> 		ctrl =
> bmi_find_spi_controller(sb.resource_source.string_ptr);
> 
> 
> > +		ctlr = bmi_find_spi_controller(acpi_data-
> >acpi_data[i].resource_source);
> > +		if (!ctlr) {
> > +			ret = -EPROBE_DEFER;
> > +			goto error;
> > +		}
> > +
> > +		spi_dev = spi_alloc_device(ctlr);
> > +		if (!spi_dev) {
> > +			dev_err(&ctlr->dev, "failed to allocate SPI device for
> %s\n",
> > +				dev_name(&adev->dev));
> > +			ret = -ENOMEM;
> > +			goto error;
> > +		}
> > +
> > +		strscpy(spi_dev->modalias, inst_array[i].type,
> sizeof(spi_dev->modalias));
> > +
> 
> And replace all the "acpi_data->acpi_data[i].sb." reference below with
> simple "sb.".
> 
> 
> > +		if (ctlr->fw_translate_cs) {
> > +			ret = ctlr->fw_translate_cs(ctlr,
> > +						    acpi_data-
> >acpi_data[i].sb.device_selection);
> > +			if (ret < 0) {
> > +				spi_dev_put(spi_dev);
> > +				goto error;
> > +			}
> > +			spi_dev->chip_select = ret;
> > +		} else {
> > +			spi_dev->chip_select = acpi_data-
> >acpi_data[i].sb.device_selection;
> > +		}
> > +
> > +		spi_dev->max_speed_hz = acpi_data-
> >acpi_data[i].sb.connection_speed;
> > +		spi_dev->bits_per_word = acpi_data-
> >acpi_data[i].sb.data_bit_length;
> > +
> > +		if (acpi_data->acpi_data[i].sb.clock_phase ==
> ACPI_SPI_SECOND_PHASE)
> > +			spi_dev->mode |= SPI_CPHA;
> > +		if (acpi_data->acpi_data[i].sb.clock_polarity ==
> ACPI_SPI_START_HIGH)
> > +			spi_dev->mode |= SPI_CPOL;
> > +		if (acpi_data->acpi_data[i].sb.device_polarity ==
> ACPI_SPI_ACTIVE_HIGH)
> > +			spi_dev->mode |= SPI_CS_HIGH;
> > +
> > +		ret = bmi_get_irq(pdev, adev, &inst_array[i]);
> > +		if (ret < 0) {
> > +			spi_dev_put(spi_dev);
> > +			goto error;
> > +		}
> > +		spi_dev->irq = ret;
> > +
> > +		snprintf(name, sizeof(name), "%s-%s-%s.%d",
> dev_name(&ctlr->dev), dev_name(dev),
> > +			 inst_array[i].type, i);
> > +		spi_dev->dev.init_name = name;
> > +
> > +		ret = spi_add_device(spi_dev);
> > +		if (ret) {
> > +			dev_err(&ctlr->dev, "failed to add SPI device %s from
> ACPI: %d\n",
> > +				dev_name(&adev->dev), ret);
> > +			spi_dev_put(spi_dev);
> > +			goto error;
> > +		}
> > +
> > +		dev_dbg(dev, "SPI device %s using chip select %u", name,
> spi_dev->chip_select);
> > +
> > +		bmi->spi_devs[i] = spi_dev;
> > +		bmi->spi_num++;
> > +	}
> > +
> > +	if (bmi->spi_num < count) {
> > +		dev_err(dev, "Error finding driver, idx %d\n", i);
> > +		ret = -ENODEV;
> > +		goto error;
> > +	}
> > +
> > +	dev_info(dev, "Instantiate %d SPI devices.\n", bmi->spi_num);
> 
> And here replace the bmi_spi_res_free(acpi_data); call in both exit paths
> with:
> acpi_dev_free_resource_list(&r); .
> 
> To me this way, simply using the already allocated resources from the list,
> rather then making a temp copy of them and throwing that away seems like
> a simpler solution ?
> 
> If you go this route, please also remove the struct bmi_spi_acpi and
> struct bmi_spi_sb_acpi data types which you now no longer need.
> 

I tried to implement this idea, and reuse the resource list, but I hit an issue. 
The resources saved in the list are not "struct acpi_resource", but instead the 
generic "struct resource".
We need the acpi_resource structure to pull the parameters from to be able to
create the spi devices.
As far as I know there is no way to convert the "struct resource" into a
"struct acpi_resource". Is there another way to do this?

Thanks,
Stefan