Hi,
On 4/6/24 4:19 PM, Andy Shevchenko wrote:
> On Sat, Apr 6, 2024 at 3:53 PM Hans de Goede <hdegoede@xxxxxxxxxx> 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
