From: Hans de Goede <hdegoede@redhat.com>
To: Stefan Binding <sbinding@opensource.cirrus.com>,
'Mark Brown' <broonie@kernel.org>,
"'Rafael J . Wysocki'" <rafael@kernel.org>,
'Len Brown' <lenb@kernel.org>,
'Mark Gross' <markgross@kernel.org>
Cc: linux-kernel@vger.kernel.org, linux-spi@vger.kernel.org,
linux-acpi@vger.kernel.org, platform-driver-x86@vger.kernel.org,
patches@opensource.cirrus.com
Subject: Re: [PATCH v2 6/6] ACPI: bus-multi-instantiate: Add SPI support
Date: Tue, 11 Jan 2022 10:20:34 +0100 [thread overview]
Message-ID: <a066f0bc-7b27-771c-544d-cacd15aa2374@redhat.com> (raw)
In-Reply-To: <00af01d8062f$75aed010$610c7030$@opensource.cirrus.com>
Hi,
On 1/10/22 15:36, Stefan Binding wrote:
> Hi,
>
>> -----Original Message-----
>> From: Hans de Goede <hdegoede@redhat.com>
>> Sent: 21 December 2021 18:32
>> To: Stefan Binding <sbinding@opensource.cirrus.com>; Mark Brown
>> <broonie@kernel.org>; Rafael J . Wysocki <rafael@kernel.org>; Len Brown
>> <lenb@kernel.org>; Mark Gross <markgross@kernel.org>
>> Cc: linux-kernel@vger.kernel.org; linux-spi@vger.kernel.org; linux-
>> acpi@vger.kernel.org; platform-driver-x86@vger.kernel.org;
>> patches@opensource.cirrus.com
>> 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?
Ugh, you're right. Sorry about that. I still don't realy like the code
from your original v2 patch for this.
So maybe this comment from my second reply on this patch can help
clean things up:
"So thinking a bit more about this, then looking up the nth SpiSerialBus
resource, and then turning that into a spi_client is something which
the SPI core ACPI code should already be doing for index==0. So I think
that you should be able to modify the SPI core ACPI code to take index
as a parameter and then have it export a helper for this which you
can use rather then duplicate the SPI core ACPI code ? Note this is
also what the I2C code is already doing.
And if you go that route you may also want to consider to add the SPI
equivalent of the i2c_acpi_client_count() helper."
Maybe that is a possible route to go to clean this up?
Note there are also 2 other small remarks pending:
1. My comment about adding the _t_ at the end of detect
2. + Mark's remark about patch 3/6 missing your Signed-off-by.
Regards,
Hans
next prev parent reply other threads:[~2022-01-11 9:20 UTC|newest]
Thread overview: 17+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-12-10 15:40 [PATCH v2 0/6] Support Spi in i2c-multi-instantiate driver Stefan Binding
2021-12-10 15:40 ` [PATCH v2 1/6] spi: Export acpi_spi_find_controller_by_adev to be used externally Stefan Binding
2021-12-10 15:40 ` [PATCH v2 2/6] spi: Make spi_alloc_device and spi_add_device public again Stefan Binding
2021-12-10 15:40 ` [PATCH v2 3/6] platform/x86: i2c-multi-instantiate: Move it to drivers/acpi folder Stefan Binding
2021-12-10 17:59 ` Mark Brown
2021-12-15 10:26 ` Hans de Goede
2021-12-10 15:40 ` [PATCH v2 4/6] ACPI: i2c-multi-instantiate: Rename it for a generic bus driver name Stefan Binding
2021-12-10 15:40 ` [PATCH v2 5/6] ACPI: bus-multi-instantiate: Reorganize I2C functions Stefan Binding
2021-12-10 15:40 ` [PATCH v2 6/6] ACPI: bus-multi-instantiate: Add SPI support Stefan Binding
2021-12-21 18:32 ` Hans de Goede
2021-12-21 19:22 ` Hans de Goede
2022-01-10 14:36 ` Stefan Binding
2022-01-11 9:20 ` Hans de Goede [this message]
2021-12-10 16:54 ` [PATCH v2 0/6] Support Spi in i2c-multi-instantiate driver Hans de Goede
2021-12-13 11:40 ` Stefan Binding
2021-12-15 10:22 ` Hans de Goede
2021-12-21 18:16 ` Hans de Goede
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=a066f0bc-7b27-771c-544d-cacd15aa2374@redhat.com \
--to=hdegoede@redhat.com \
--cc=broonie@kernel.org \
--cc=lenb@kernel.org \
--cc=linux-acpi@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-spi@vger.kernel.org \
--cc=markgross@kernel.org \
--cc=patches@opensource.cirrus.com \
--cc=platform-driver-x86@vger.kernel.org \
--cc=rafael@kernel.org \
--cc=sbinding@opensource.cirrus.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).