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
