From: Hans de Goede <hdegoede@redhat.com>
To: Andy Shevchenko <andy.shevchenko@gmail.com>
Cc: "Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>,
"Andy Shevchenko" <andy@kernel.org>,
platform-driver-x86@vger.kernel.org
Subject: Re: [PATCH 3/3] platform/x86: Add lenovo-yoga-tab2-pro-1380-fastcharger driver
Date: Mon, 22 Apr 2024 14:58:30 +0200 [thread overview]
Message-ID: <375ed445-209e-4299-91cd-6e735ec79f93@redhat.com> (raw)
In-Reply-To: <CAHp75VdXbFxzwDas6z=oRW5hKJ9=CHW3SxVEtqaFTLq6yJQP-g@mail.gmail.com>
Hi,
On 4/6/24 4:19 PM, Andy Shevchenko wrote:
> On Sat, Apr 6, 2024 at 3:53 PM Hans de Goede <hdegoede@redhat.com> wrote:
>>
>> Add a new driver for the custom fast charging protocol found on Lenovo Yoga
>> Tablet 2 1380F / 1380L models.
>
> ...
>
>> +#include <linux/delay.h>
>
> + err.h
> + errno.h
>
>> +#include <linux/extcon.h>
>> +#include <linux/gpio/consumer.h>
>
> + mod_devicetable.h
This one won't be necessary, see below.
>
>> +#include <linux/module.h>
>> +#include <linux/notifier.h>
>> +#include <linux/pinctrl/consumer.h>
>> +#include <linux/pinctrl/machine.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/serdev.h>
>
> + types.h
>
>> +#include <linux/workqueue.h>
>> +#include "serdev_helpers.h"
Ack others added for v2.
>
> ...
>
>> +#define YT2_1380_FC_PIN_SW_DELAY_US 10000
>> +#define YT2_1380_FC_UART_DRAIN_DELAY_US 50000
>> +#define YT2_1380_FC_VOLT_SW_DELAY_US 1000000
>
> Hmm... perhaps
>
> 10 * USEC_PER_MSEC
> 50 * USEC_PER_MSEC
> 1 * USEC_PER_SEC
>
> ?
Ack, changed for v2.
>
> ...
>
>> +static bool yt2_1380_fc_dedicated_charger_connected(struct yt2_1380_fc *fc)
>> +{
>> + int ret;
>> +
>> + ret = extcon_get_state(fc->extcon, EXTCON_CHG_USB_DCP);
>> + return ret > 0;
>
> This and other functions can be shorter by eliminating the ret
> variable, but it's up to you.
>
>> +}
Ack, changed for v2.
>
> ...
>
>> + ret = serdev_device_write_buf(to_serdev_device(fc->dev), "SC", 2);
>
> Hmm... I would replace magic 2 in both cases by strlen("SC") or so.
Ack, changed for v2.
>
>> + if (ret != 2) {
>> + dev_err(fc->dev, "Error %d writing to uart\n", ret);
>> + return;
>> + }
>
> ...
>
>> +static struct platform_driver yt2_1380_fc_pdev_driver = {
>> + .probe = yt2_1380_fc_pdev_probe,
>> + .remove_new = yt2_1380_fc_pdev_remove,
>> + .driver = {
>> + .name = YT2_1380_FC_PDEV_NAME,
>> + .probe_type = PROBE_PREFER_ASYNCHRONOUS,
>> + },
>
> + .id_table = ... (see below why)
>
>> +};
>
>> +MODULE_ALIAS("platform:" YT2_1380_FC_PDEV_NAME);
>
> The _standard_ MODULE_ALIAS() is not an excuse to have no ID table.
> Just provide an accurate platform device ID table instead.
I personally have no strong preference either way (although
the MODULE_ALIAS way seems to be used the most in other code),
but in this specific case adding a platform_device_id table does
not work:
drivers/platform/x86/lenovo-yoga-tab2-pro-1380-fastcharger.c:26:41: warning: initializer-string for array of ‘char’ is too long
26 | #define YT2_1380_FC_PDEV_NAME "lenovo-yoga-tab2-pro-1380-fastcharger"
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
from mod_devicetable.h:
#define PLATFORM_NAME_SIZE 20
#define PLATFORM_MODULE_PREFIX "platform:"
struct platform_device_id {
char name[PLATFORM_NAME_SIZE];
kernel_ulong_t driver_data;
};
So the driver / pdev name can only by 20 chars, I tried shortening:
"lenovo-yoga-tab2-pro-1380-fastcharger"
to:
"lenovo-yt2-pro-fastcharger"
But that still is too long, making it even shorter will make
the name really cryptic, where as without using
the platform_device_id approach things work fine,
so I'm going to stick with the MODULE_ALIAS() for v2.
Regards,
Hans
next prev parent reply other threads:[~2024-04-22 12:58 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-04-06 12:50 [PATCH 1/3] platform/x86: x86-android-tablets: Unregister devices in reverse order Hans de Goede
2024-04-06 12:50 ` [PATCH 2/3] platform/x86: x86-android-tablets: Add Lenovo Yoga Tablet 2 Pro 1380F/L data Hans de Goede
2024-04-06 12:50 ` [PATCH 3/3] platform/x86: Add lenovo-yoga-tab2-pro-1380-fastcharger driver Hans de Goede
2024-04-06 14:19 ` Andy Shevchenko
2024-04-22 12:58 ` Hans de Goede [this message]
2024-04-15 13:43 ` [PATCH 1/3] platform/x86: x86-android-tablets: Unregister devices in reverse order 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=375ed445-209e-4299-91cd-6e735ec79f93@redhat.com \
--to=hdegoede@redhat.com \
--cc=andy.shevchenko@gmail.com \
--cc=andy@kernel.org \
--cc=ilpo.jarvinen@linux.intel.com \
--cc=platform-driver-x86@vger.kernel.org \
/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).