* Re: [PATCH v2] HID: nintendo: reinitialize USB Pro Controller after resuming from suspend
From: Martino Fontana @ 2023-07-22 10:29 UTC (permalink / raw)
To: Silvan Jegen; +Cc: linux-input
In-Reply-To: <38NABT0Q0GDC0.32EJOTLAGT0T2@homearch.localdomain>
It is my first patch on the Linux kernel, so I just did kinda what I
would do on GitHub (amend and force push).
What should I do here in case of trivial adjustments?
On Fri, Jul 21, 2023 at 9:02 PM Silvan Jegen <s.jegen@gmail.com> wrote:
>
> Martino Fontana <tinozzo123@gmail.com> wrote:
> > When suspending the computer, a Switch Pro Controller connected via USB will
> > lose its internal status. However, because the USB connection was technically
> > never lost, when resuming the computer, the driver will attempt to communicate
> > with the controller as if nothing happened (and fail).
> > Because of this, the user was forced to manually disconnect the controller
> > (or to press the sync button on the controller to power it off), so that it
> > can be re-initialized.
> >
> > With this patch, the controller will be automatically re-initialized after
> > resuming from suspend.
> >
> > Fixes https://bugzilla.kernel.org/show_bug.cgi?id=216233
> >
> > Signed-off-by: Martino Fontana <tinozzo123@gmail.com>
>
> It would be good to add a small section about what changed between v1
> and v2 of the patch. Here is an example for how this can be done.
>
> https://lore.kernel.org/lkml/cover.b24362332ec6099bc8db4e8e06a67545c653291d.1689842332.git-series.apopple@nvidia.com/T/#m73dd8d44f40742e67cbb0d4f030a90b6264a88d3
>
> It's probably not worth resending this patch just for this though. Just
> something to keep in mind if there will be another patch version needed.
>
> Cheers,
> Silvan
>
> > ---
> > drivers/hid/hid-nintendo.c | 178 ++++++++++++++++++++++---------------
> > 1 file changed, 106 insertions(+), 72 deletions(-)
> >
> > diff --git a/drivers/hid/hid-nintendo.c b/drivers/hid/hid-nintendo.c
> > index 250f5d2f8..a5ebe857a 100644
> > --- a/drivers/hid/hid-nintendo.c
> > +++ b/drivers/hid/hid-nintendo.c
> > @@ -2088,7 +2088,9 @@ static int joycon_read_info(struct joycon_ctlr *ctlr)
> > struct joycon_input_report *report;
> >
> > req.subcmd_id = JC_SUBCMD_REQ_DEV_INFO;
> > + mutex_lock(&ctlr->output_mutex);
> > ret = joycon_send_subcmd(ctlr, &req, 0, HZ);
> > + mutex_unlock(&ctlr->output_mutex);
> > if (ret) {
> > hid_err(ctlr->hdev, "Failed to get joycon info; ret=%d\n", ret);
> > return ret;
> > @@ -2117,6 +2119,88 @@ static int joycon_read_info(struct joycon_ctlr *ctlr)
> > return 0;
> > }
> >
> > +static int joycon_init(struct hid_device *hdev)
> > +{
> > + struct joycon_ctlr *ctlr = hid_get_drvdata(hdev);
> > + int ret = 0;
> > +
> > + mutex_lock(&ctlr->output_mutex);
> > + /* if handshake command fails, assume ble pro controller */
> > + if ((jc_type_is_procon(ctlr) || jc_type_is_chrggrip(ctlr)) &&
> > + !joycon_send_usb(ctlr, JC_USB_CMD_HANDSHAKE, HZ)) {
> > + hid_dbg(hdev, "detected USB controller\n");
> > + /* set baudrate for improved latency */
> > + ret = joycon_send_usb(ctlr, JC_USB_CMD_BAUDRATE_3M, HZ);
> > + if (ret) {
> > + hid_err(hdev, "Failed to set baudrate; ret=%d\n", ret);
> > + goto err_mutex;
> > + }
> > + /* handshake */
> > + ret = joycon_send_usb(ctlr, JC_USB_CMD_HANDSHAKE, HZ);
> > + if (ret) {
> > + hid_err(hdev, "Failed handshake; ret=%d\n", ret);
> > + goto err_mutex;
> > + }
> > + /*
> > + * Set no timeout (to keep controller in USB mode).
> > + * This doesn't send a response, so ignore the timeout.
> > + */
> > + joycon_send_usb(ctlr, JC_USB_CMD_NO_TIMEOUT, HZ/10);
> > + } else if (jc_type_is_chrggrip(ctlr)) {
> > + hid_err(hdev, "Failed charging grip handshake\n");
> > + ret = -ETIMEDOUT;
> > + goto err_mutex;
> > + }
> > +
> > + /* get controller calibration data, and parse it */
> > + ret = joycon_request_calibration(ctlr);
> > + if (ret) {
> > + /*
> > + * We can function with default calibration, but it may be
> > + * inaccurate. Provide a warning, and continue on.
> > + */
> > + hid_warn(hdev, "Analog stick positions may be inaccurate\n");
> > + }
> > +
> > + /* get IMU calibration data, and parse it */
> > + ret = joycon_request_imu_calibration(ctlr);
> > + if (ret) {
> > + /*
> > + * We can function with default calibration, but it may be
> > + * inaccurate. Provide a warning, and continue on.
> > + */
> > + hid_warn(hdev, "Unable to read IMU calibration data\n");
> > + }
> > +
> > + /* Set the reporting mode to 0x30, which is the full report mode */
> > + ret = joycon_set_report_mode(ctlr);
> > + if (ret) {
> > + hid_err(hdev, "Failed to set report mode; ret=%d\n", ret);
> > + goto err_mutex;
> > + }
> > +
> > + /* Enable rumble */
> > + ret = joycon_enable_rumble(ctlr);
> > + if (ret) {
> > + hid_err(hdev, "Failed to enable rumble; ret=%d\n", ret);
> > + goto err_mutex;
> > + }
> > +
> > + /* Enable the IMU */
> > + ret = joycon_enable_imu(ctlr);
> > + if (ret) {
> > + hid_err(hdev, "Failed to enable the IMU; ret=%d\n", ret);
> > + goto err_mutex;
> > + }
> > +
> > + mutex_unlock(&ctlr->output_mutex);
> > + return 0;
> > +
> > +err_mutex:
> > + mutex_unlock(&ctlr->output_mutex);
> > + return ret;
> > +}
> > +
> > /* Common handler for parsing inputs */
> > static int joycon_ctlr_read_handler(struct joycon_ctlr *ctlr, u8 *data,
> > int size)
> > @@ -2248,85 +2332,19 @@ static int nintendo_hid_probe(struct hid_device *hdev,
> >
> > hid_device_io_start(hdev);
> >
> > - /* Initialize the controller */
> > - mutex_lock(&ctlr->output_mutex);
> > - /* if handshake command fails, assume ble pro controller */
> > - if ((jc_type_is_procon(ctlr) || jc_type_is_chrggrip(ctlr)) &&
> > - !joycon_send_usb(ctlr, JC_USB_CMD_HANDSHAKE, HZ)) {
> > - hid_dbg(hdev, "detected USB controller\n");
> > - /* set baudrate for improved latency */
> > - ret = joycon_send_usb(ctlr, JC_USB_CMD_BAUDRATE_3M, HZ);
> > - if (ret) {
> > - hid_err(hdev, "Failed to set baudrate; ret=%d\n", ret);
> > - goto err_mutex;
> > - }
> > - /* handshake */
> > - ret = joycon_send_usb(ctlr, JC_USB_CMD_HANDSHAKE, HZ);
> > - if (ret) {
> > - hid_err(hdev, "Failed handshake; ret=%d\n", ret);
> > - goto err_mutex;
> > - }
> > - /*
> > - * Set no timeout (to keep controller in USB mode).
> > - * This doesn't send a response, so ignore the timeout.
> > - */
> > - joycon_send_usb(ctlr, JC_USB_CMD_NO_TIMEOUT, HZ/10);
> > - } else if (jc_type_is_chrggrip(ctlr)) {
> > - hid_err(hdev, "Failed charging grip handshake\n");
> > - ret = -ETIMEDOUT;
> > - goto err_mutex;
> > - }
> > -
> > - /* get controller calibration data, and parse it */
> > - ret = joycon_request_calibration(ctlr);
> > - if (ret) {
> > - /*
> > - * We can function with default calibration, but it may be
> > - * inaccurate. Provide a warning, and continue on.
> > - */
> > - hid_warn(hdev, "Analog stick positions may be inaccurate\n");
> > - }
> > -
> > - /* get IMU calibration data, and parse it */
> > - ret = joycon_request_imu_calibration(ctlr);
> > - if (ret) {
> > - /*
> > - * We can function with default calibration, but it may be
> > - * inaccurate. Provide a warning, and continue on.
> > - */
> > - hid_warn(hdev, "Unable to read IMU calibration data\n");
> > - }
> > -
> > - /* Set the reporting mode to 0x30, which is the full report mode */
> > - ret = joycon_set_report_mode(ctlr);
> > - if (ret) {
> > - hid_err(hdev, "Failed to set report mode; ret=%d\n", ret);
> > - goto err_mutex;
> > - }
> > -
> > - /* Enable rumble */
> > - ret = joycon_enable_rumble(ctlr);
> > - if (ret) {
> > - hid_err(hdev, "Failed to enable rumble; ret=%d\n", ret);
> > - goto err_mutex;
> > - }
> > -
> > - /* Enable the IMU */
> > - ret = joycon_enable_imu(ctlr);
> > + ret = joycon_init(hdev);
> > if (ret) {
> > - hid_err(hdev, "Failed to enable the IMU; ret=%d\n", ret);
> > - goto err_mutex;
> > + hid_err(hdev, "Failed to initialize controller; ret=%d\n", ret);
> > + goto err_close;
> > }
> >
> > ret = joycon_read_info(ctlr);
> > if (ret) {
> > hid_err(hdev, "Failed to retrieve controller info; ret=%d\n",
> > ret);
> > - goto err_mutex;
> > + goto err_close;
> > }
> >
> > - mutex_unlock(&ctlr->output_mutex);
> > -
> > /* Initialize the leds */
> > ret = joycon_leds_create(ctlr);
> > if (ret) {
> > @@ -2352,8 +2370,6 @@ static int nintendo_hid_probe(struct hid_device *hdev,
> > hid_dbg(hdev, "probe - success\n");
> > return 0;
> >
> > -err_mutex:
> > - mutex_unlock(&ctlr->output_mutex);
> > err_close:
> > hid_hw_close(hdev);
> > err_stop:
> > @@ -2383,6 +2399,20 @@ static void nintendo_hid_remove(struct hid_device *hdev)
> > hid_hw_stop(hdev);
> > }
> >
> > +#ifdef CONFIG_PM
> > +
> > +static int nintendo_hid_resume(struct hid_device *hdev)
> > +{
> > + int ret = joycon_init(hdev);
> > +
> > + if (ret)
> > + hid_err(hdev, "Failed to restore controller after resume");
> > +
> > + return ret;
> > +}
> > +
> > +#endif
> > +
> > static const struct hid_device_id nintendo_hid_devices[] = {
> > { HID_USB_DEVICE(USB_VENDOR_ID_NINTENDO,
> > USB_DEVICE_ID_NINTENDO_PROCON) },
> > @@ -2404,6 +2434,10 @@ static struct hid_driver nintendo_hid_driver = {
> > .probe = nintendo_hid_probe,
> > .remove = nintendo_hid_remove,
> > .raw_event = nintendo_hid_event,
> > +
> > +#ifdef CONFIG_PM
> > + .resume = nintendo_hid_resume,
> > +#endif
> > };
> > module_hid_driver(nintendo_hid_driver);
> >
>
>
^ permalink raw reply
* [PATCH v2] HID: nintendo: cleanup LED code
From: Martino Fontana @ 2023-07-22 10:24 UTC (permalink / raw)
To: s.jegen; +Cc: linux-input, tinozzo123
In-Reply-To: <209M4JB8WJRSY.2XPCG39LL8DLJ@homearch.localdomain>
- Only turn on the nth LED, instead of all the LEDs up to n. This better
matches Nintendo consoles' behaviour, as they never turn on more than
one LED.
(Note that the behavior still consinsts in increasing the player number
every time a controller is connected, never decreasing it. It should be
as is described in https://bugzilla.kernel.org/show_bug.cgi?id=216225.
However, any implementation here would stop making sense as soon as a
non-Nintendo controller is connected, which is why I'm not bothering.)
- Split part of `joycon_home_led_brightness_set` (which is called by hid)
into `joycon_set_home_led` (which is what actually sets the LEDs), for
consistency with player LEDs.
- `joycon_player_led_brightness_set` won't try it to "determine which player
led this is" anymore: it's already looking at every LED brightness value.
- Instead of first registering the `led_classdev`, then attempting to set
the LED and unregistering the `led_classdev` if it fails, first attempt
to set the LED, then register the `led_classdev` only if it succeeds
(the class is still filled up in either case).
- If setting the player LEDs fails, still attempt setting the home LED.
(I don't know there's a third party controller where this may actually
happen, but who knows...)
- Use `JC_NUM_LEDS` where appropriate instead of 4.
- Print return codes in more places.
- Use spinlock instead of mutex for `input_num`. Copy its value to a local
variable, so that it can be unlocked immediately.
- Less holding of mutexes in general.
Changes for v2:
Applied suggestions, commit message explains more stuff, restored `return ret`
when `devm_led_classdev_register` fails (since all other hid drivers do this).
If setting LED fails, `hid_warn` now explicitly says "skipping registration".
Signed-off-by: Martino Fontana <tinozzo123@gmail.com>
---
drivers/hid/hid-nintendo.c | 117 ++++++++++++++++++-------------------
1 file changed, 57 insertions(+), 60 deletions(-)
diff --git a/drivers/hid/hid-nintendo.c b/drivers/hid/hid-nintendo.c
index a5ebe857a..dd7c7fce3 100644
--- a/drivers/hid/hid-nintendo.c
+++ b/drivers/hid/hid-nintendo.c
@@ -699,6 +699,25 @@ static int joycon_set_player_leds(struct joycon_ctlr *ctlr, u8 flash, u8 on)
return joycon_send_subcmd(ctlr, req, 1, HZ/4);
}
+static int joycon_set_home_led(struct joycon_ctlr *ctlr, enum led_brightness brightness)
+{
+ struct joycon_subcmd_request *req;
+ u8 buffer[sizeof(*req) + 5] = { 0 };
+ u8 *data;
+
+ req = (struct joycon_subcmd_request *)buffer;
+ req->subcmd_id = JC_SUBCMD_SET_HOME_LIGHT;
+ data = req->data;
+ data[0] = 0x01;
+ data[1] = brightness << 4;
+ data[2] = brightness | (brightness << 4);
+ data[3] = 0x11;
+ data[4] = 0x11;
+
+ hid_dbg(ctlr->hdev, "setting home led brightness\n");
+ return joycon_send_subcmd(ctlr, req, 5, HZ/4);
+}
+
static int joycon_request_spi_flash_read(struct joycon_ctlr *ctlr,
u32 start_addr, u8 size, u8 **reply)
{
@@ -1849,7 +1868,6 @@ static int joycon_player_led_brightness_set(struct led_classdev *led,
int val = 0;
int i;
int ret;
- int num;
ctlr = hid_get_drvdata(hdev);
if (!ctlr) {
@@ -1857,21 +1875,10 @@ static int joycon_player_led_brightness_set(struct led_classdev *led,
return -ENODEV;
}
- /* determine which player led this is */
- for (num = 0; num < JC_NUM_LEDS; num++) {
- if (&ctlr->leds[num] == led)
- break;
- }
- if (num >= JC_NUM_LEDS)
- return -EINVAL;
+ for (i = 0; i < JC_NUM_LEDS; i++)
+ val |= ctlr->leds[i].brightness << i;
mutex_lock(&ctlr->output_mutex);
- for (i = 0; i < JC_NUM_LEDS; i++) {
- if (i == num)
- val |= brightness << i;
- else
- val |= ctlr->leds[i].brightness << i;
- }
ret = joycon_set_player_leds(ctlr, 0, val);
mutex_unlock(&ctlr->output_mutex);
@@ -1884,9 +1891,6 @@ static int joycon_home_led_brightness_set(struct led_classdev *led,
struct device *dev = led->dev->parent;
struct hid_device *hdev = to_hid_device(dev);
struct joycon_ctlr *ctlr;
- struct joycon_subcmd_request *req;
- u8 buffer[sizeof(*req) + 5] = { 0 };
- u8 *data;
int ret;
ctlr = hid_get_drvdata(hdev);
@@ -1894,25 +1898,13 @@ static int joycon_home_led_brightness_set(struct led_classdev *led,
hid_err(hdev, "No controller data\n");
return -ENODEV;
}
-
- req = (struct joycon_subcmd_request *)buffer;
- req->subcmd_id = JC_SUBCMD_SET_HOME_LIGHT;
- data = req->data;
- data[0] = 0x01;
- data[1] = brightness << 4;
- data[2] = brightness | (brightness << 4);
- data[3] = 0x11;
- data[4] = 0x11;
-
- hid_dbg(hdev, "setting home led brightness\n");
mutex_lock(&ctlr->output_mutex);
- ret = joycon_send_subcmd(ctlr, req, 5, HZ/4);
+ ret = joycon_set_home_led(ctlr, brightness);
mutex_unlock(&ctlr->output_mutex);
-
return ret;
}
-static DEFINE_MUTEX(joycon_input_num_mutex);
+static DEFINE_SPINLOCK(joycon_input_num_spinlock);
static int joycon_leds_create(struct joycon_ctlr *ctlr)
{
struct hid_device *hdev = ctlr->hdev;
@@ -1920,17 +1912,16 @@ static int joycon_leds_create(struct joycon_ctlr *ctlr)
const char *d_name = dev_name(dev);
struct led_classdev *led;
char *name;
- int ret = 0;
+ int ret;
int i;
- static int input_num = 1;
+ unsigned long flags;
+ int player_led_number;
+ static int input_num;
- /* Set the default controller player leds based on controller number */
- mutex_lock(&joycon_input_num_mutex);
- mutex_lock(&ctlr->output_mutex);
- ret = joycon_set_player_leds(ctlr, 0, 0xF >> (4 - input_num));
- if (ret)
- hid_warn(ctlr->hdev, "Failed to set leds; ret=%d\n", ret);
- mutex_unlock(&ctlr->output_mutex);
+ /* Set the player leds based on controller number */
+ spin_lock_irqsave(&joycon_input_num_spinlock, flags);
+ player_led_number = input_num++ % JC_NUM_LEDS;
+ spin_unlock_irqrestore(&joycon_input_num_spinlock, flags);
/* configure the player LEDs */
for (i = 0; i < JC_NUM_LEDS; i++) {
@@ -1938,31 +1929,34 @@ static int joycon_leds_create(struct joycon_ctlr *ctlr)
d_name,
"green",
joycon_player_led_names[i]);
- if (!name) {
- mutex_unlock(&joycon_input_num_mutex);
+ if (!name)
return -ENOMEM;
- }
led = &ctlr->leds[i];
led->name = name;
- led->brightness = ((i + 1) <= input_num) ? 1 : 0;
+ led->brightness = (i == player_led_number) ? 1 : 0;
led->max_brightness = 1;
led->brightness_set_blocking =
joycon_player_led_brightness_set;
led->flags = LED_CORE_SUSPENDRESUME | LED_HW_PLUGGABLE;
+ }
+ mutex_lock(&ctlr->output_mutex);
+ ret = joycon_set_player_leds(ctlr, 0, 0x1 << player_led_number);
+ mutex_unlock(&ctlr->output_mutex);
+ if (ret) {
+ hid_warn(hdev, "Failed to set players LEDs, skipping registration; ret=%d\n", ret);
+ goto home_led;
+ }
+ for (i = 0; i < JC_NUM_LEDS; i++) {
+ led = &ctlr->leds[i];
ret = devm_led_classdev_register(&hdev->dev, led);
- if (ret) {
- hid_err(hdev, "Failed registering %s LED\n", led->name);
- mutex_unlock(&joycon_input_num_mutex);
+ if (ret)
+ hid_err(hdev, "Failed to register player %d LED; ret=%d\n", i + 1, ret);
return ret;
- }
}
- if (++input_num > 4)
- input_num = 1;
- mutex_unlock(&joycon_input_num_mutex);
-
+home_led:
/* configure the home LED */
if (jc_type_has_right(ctlr)) {
name = devm_kasprintf(dev, GFP_KERNEL, "%s:%s:%s",
@@ -1978,17 +1972,20 @@ static int joycon_leds_create(struct joycon_ctlr *ctlr)
led->max_brightness = 0xF;
led->brightness_set_blocking = joycon_home_led_brightness_set;
led->flags = LED_CORE_SUSPENDRESUME | LED_HW_PLUGGABLE;
- ret = devm_led_classdev_register(&hdev->dev, led);
- if (ret) {
- hid_err(hdev, "Failed registering home led\n");
- return ret;
- }
+
/* Set the home LED to 0 as default state */
- ret = joycon_home_led_brightness_set(led, 0);
+ mutex_lock(&ctlr->output_mutex);
+ ret = joycon_set_home_led(ctlr, 0);
+ mutex_unlock(&ctlr->output_mutex);
if (ret) {
- hid_warn(hdev, "Failed to set home LED default, unregistering home LED");
- devm_led_classdev_unregister(&hdev->dev, led);
+ hid_warn(hdev, "Failed to set home LED, skipping registration; ret=%d\n", ret);
+ return 0;
}
+
+ ret = devm_led_classdev_register(&hdev->dev, led);
+ if (ret)
+ hid_err(hdev, "Failed to register home LED; ret=%d\n", ret);
+ return ret
}
return 0;
--
2.41.0
^ permalink raw reply related
* Re: [PATCH] Input: xpad - Add HyperX Clutch Gladiate support
From: Rahul Rameshbabu @ 2023-07-22 2:46 UTC (permalink / raw)
To: HP Dev; +Cc: Dmitry Torokhov, linux-input, Chris Toledanes, Carl Ng,
Max Nguyen
In-Reply-To: <20230722010532.120280-1-hphyperxdev@gmail.com>
On Fri, 21 Jul, 2023 18:05:32 -0700 HP Dev <hphyperxdev@gmail.com> wrote:
> Add HyperX controller support to xpad_device and xpad_table.
This patch appears to be a resend of a previous patch but does not
follow the right practice for patch resends.
https://docs.kernel.org/process/submitting-patches.html#don-t-get-discouraged-or-impatient
https://lore.kernel.org/linux-input/ZKxVBULWtM30ZJ7D@google.com/
>
> Reported-by: Chris Toledanes <chris.toledanes@hp.com>
I think this may not be an accurate use of the Reported-by: git trailer.
My guess is you would want to use the Suggested-by: trailer here. This
patch does not solve a "regression" in the xpad driver that was reported
by someone or some automation but rather adds a new controller to be
probed by the driver.
https://docs.kernel.org/process/submitting-patches.html#using-reported-by-tested-by-reviewed-by-suggested-by-and-fixes
> Reviewed-by: Carl Ng <carl.ng@hp.com>
> Signed-off-by: Max Nguyen <maxwell.nguyen@hp.com>
> ---
> drivers/input/joystick/xpad.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/drivers/input/joystick/xpad.c b/drivers/input/joystick/xpad.c
> index cdb193317c3b..dfddb0bba8d8 100644
> --- a/drivers/input/joystick/xpad.c
> +++ b/drivers/input/joystick/xpad.c
> @@ -130,6 +130,7 @@ static const struct xpad_device {
> { 0x0079, 0x18d4, "GPD Win 2 X-Box Controller", 0, XTYPE_XBOX360 },
> { 0x03eb, 0xff01, "Wooting One (Legacy)", 0, XTYPE_XBOX360 },
> { 0x03eb, 0xff02, "Wooting Two (Legacy)", 0, XTYPE_XBOX360 },
> + { 0x03f0, 0x0495, "HyperX Clutch Gladiate", 0, XTYPE_XBOXONE },
> { 0x044f, 0x0f00, "Thrustmaster Wheel", 0, XTYPE_XBOX },
> { 0x044f, 0x0f03, "Thrustmaster Wheel", 0, XTYPE_XBOX },
> { 0x044f, 0x0f07, "Thrustmaster, Inc. Controller", 0, XTYPE_XBOX },
> @@ -457,6 +458,8 @@ static const struct usb_device_id xpad_table[] = {
> { USB_INTERFACE_INFO('X', 'B', 0) }, /* Xbox USB-IF not-approved class */
> XPAD_XBOX360_VENDOR(0x0079), /* GPD Win 2 controller */
> XPAD_XBOX360_VENDOR(0x03eb), /* Wooting Keyboards (Legacy) */
> + XPAD_XBOX360_VENDOR(0x03f0), /* HP HyperX XBox 360 Controllers */
There are no HP HyperX Xbox 360 controllers previously supported or
added in this patch. I think the above line should be removed since you
only have a XTYPE_XBOXONE type controller. You probably do not need to
match against Xbox 360 vendor-specific class since you only implement an
Xbox One controller.
> + XPAD_XBOXONE_VENDOR(0x03f0), /* HP HyperX Xbox One Controllers */
> XPAD_XBOX360_VENDOR(0x044f), /* Thrustmaster Xbox 360 controllers */
> XPAD_XBOX360_VENDOR(0x045e), /* Microsoft Xbox 360 controllers */
> XPAD_XBOXONE_VENDOR(0x045e), /* Microsoft Xbox One controllers */
-- Rahul Rameshbabu
^ permalink raw reply
* [PATCH] Input: xpad - Add HyperX Clutch Gladiate support
From: HP Dev @ 2023-07-22 1:05 UTC (permalink / raw)
To: Dmitry Torokhov; +Cc: linux-input, HP Dev, Chris Toledanes, Carl Ng, Max Nguyen
Add HyperX controller support to xpad_device and xpad_table.
Reported-by: Chris Toledanes <chris.toledanes@hp.com>
Reviewed-by: Carl Ng <carl.ng@hp.com>
Signed-off-by: Max Nguyen <maxwell.nguyen@hp.com>
---
drivers/input/joystick/xpad.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/input/joystick/xpad.c b/drivers/input/joystick/xpad.c
index cdb193317c3b..dfddb0bba8d8 100644
--- a/drivers/input/joystick/xpad.c
+++ b/drivers/input/joystick/xpad.c
@@ -130,6 +130,7 @@ static const struct xpad_device {
{ 0x0079, 0x18d4, "GPD Win 2 X-Box Controller", 0, XTYPE_XBOX360 },
{ 0x03eb, 0xff01, "Wooting One (Legacy)", 0, XTYPE_XBOX360 },
{ 0x03eb, 0xff02, "Wooting Two (Legacy)", 0, XTYPE_XBOX360 },
+ { 0x03f0, 0x0495, "HyperX Clutch Gladiate", 0, XTYPE_XBOXONE },
{ 0x044f, 0x0f00, "Thrustmaster Wheel", 0, XTYPE_XBOX },
{ 0x044f, 0x0f03, "Thrustmaster Wheel", 0, XTYPE_XBOX },
{ 0x044f, 0x0f07, "Thrustmaster, Inc. Controller", 0, XTYPE_XBOX },
@@ -457,6 +458,8 @@ static const struct usb_device_id xpad_table[] = {
{ USB_INTERFACE_INFO('X', 'B', 0) }, /* Xbox USB-IF not-approved class */
XPAD_XBOX360_VENDOR(0x0079), /* GPD Win 2 controller */
XPAD_XBOX360_VENDOR(0x03eb), /* Wooting Keyboards (Legacy) */
+ XPAD_XBOX360_VENDOR(0x03f0), /* HP HyperX XBox 360 Controllers */
+ XPAD_XBOXONE_VENDOR(0x03f0), /* HP HyperX Xbox One Controllers */
XPAD_XBOX360_VENDOR(0x044f), /* Thrustmaster Xbox 360 controllers */
XPAD_XBOX360_VENDOR(0x045e), /* Microsoft Xbox 360 controllers */
XPAD_XBOXONE_VENDOR(0x045e), /* Microsoft Xbox One controllers */
--
2.39.3
^ permalink raw reply related
* Re: [PATCH v3] Input: synaptics-rmi4 - retry reading SMBus version on resume
From: Dmitry Torokhov @ 2023-07-21 23:53 UTC (permalink / raw)
To: Jeffery Miller
Cc: Jonathan Denose, jdenose, Lyude Paul, benjamin.tissoires,
Andrew Duggan, loic.poulain, Andrew Duggan,
Javier Martinez Canillas, Jeremy Kerr, Jonathan Cameron,
Uwe Kleine-König, linux-input, linux-kernel
In-Reply-To: <20230608210404.722123-1-jefferymiller@google.com>
Hi Jeffery,
On Thu, Jun 08, 2023 at 09:04:00PM +0000, Jeffery Miller wrote:
> On resume there can be a period of time after the
> preceding serio_resume -> psmouse_deactivate call
> where calls to rmi_smb_get_version fail with
> -ENXIO.
>
> The call path in rmi_smb_resume is rmi_smb_resume -> rmi_smb_reset ->
> rmi_smb_enable_smbus_mode -> rmi_smb_get_version where
> this failure would occur.
>
> Add a 30ms delay and retry in the ENXIO case to ensure the following
> rmi_driver_resume calls in rmi_smbus_resume can succeed.
>
> This behavior was seen on a Lenovo T440p machine that required
> a delay of approximately 7-12ms.
>
> The 30ms delay was chosen based on the linux-input message:
> Link: https://lore.kernel.org/all/BYAPR03MB47572F2C65E52ED673238D41B2439@BYAPR03MB4757.namprd03.prod.outlook.com/
I do not quite like putting these retries in RMI code. I wonder if we
should not move the delay into psmouse_smbus_reconnect():
if (smbdev->need_deactivate) {
psmouse_deactivate(psmouse);
/* Give the device time to switch to SMBus mode */
msleep(30);
}
or even factor it out into psmouse_activate_smbus_mode() and call it
from psmouse_smbus_reconnect() as well as psmouse_smbus_init().
Thanks.
or even factor it out into psmouse_activate_smbus_mode() and call it
from psmouse_smbus_reconnect() as well as psmouse_smbus_init().
Thanks.
--
Dmitry
^ permalink raw reply
* Re: [PATCH] Input: i8042 - Add quirk for polling the KBD port
From: Dmitry Torokhov @ 2023-07-21 23:44 UTC (permalink / raw)
To: Friedrich Vock; +Cc: linux-input, Mario Limonciello
In-Reply-To: <20230530153644.17262-1-friedrich.vock@gmx.de>
Hi Friedrich,
On Tue, May 30, 2023 at 05:36:44PM +0200, Friedrich Vock wrote:
> It seems like there are some devices in the ASUS TUF A16 laptops that
> just don't send any keyboard interrupts until you read from the KBD port.
I am sorry, but continuously polling keyboard port will absolutely wreck
battery life on these devices, so this can not be a real solution.
I wonder if this is yet another example of incorrect IRQ 1 polarity
override on devices with AMD chipsets (CC-ing Mario).
>
> Signed-off-by: Friedrich Vock <friedrich.vock@gmx.de>
> ---
> drivers/input/serio/i8042-acpipnpio.h | 30 +++++++++++++++--
> drivers/input/serio/i8042.c | 47 ++++++++++++++++++++++-----
> drivers/input/serio/i8042.h | 2 +-
> 3 files changed, 67 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/input/serio/i8042-acpipnpio.h b/drivers/input/serio/i8042-acpipnpio.h
> index 028e45bd050b..be2e72aaa658 100644
> --- a/drivers/input/serio/i8042-acpipnpio.h
> +++ b/drivers/input/serio/i8042-acpipnpio.h
> @@ -83,6 +83,7 @@ static inline void i8042_write_command(int val)
> #define SERIO_QUIRK_KBDRESET BIT(12)
> #define SERIO_QUIRK_DRITEK BIT(13)
> #define SERIO_QUIRK_NOPNP BIT(14)
> +#define SERIO_QUIRK_POLL_KBD BIT(15)
>
> /* Quirk table for different mainboards. Options similar or identical to i8042
> * module parameters.
> @@ -99,6 +100,26 @@ static const struct dmi_system_id i8042_dmi_quirk_table[] __initconst = {
> },
> .driver_data = (void *)(SERIO_QUIRK_NOMUX)
> },
> + /* Some laptops seem to not trigger any keyboard interrupts at all,
> + * even when there is data available. On these devices, manually
> + * polling the keyboard port is required.
> + */
> + {
> + /* ASUS TUF Gaming A16 with Ryzen 7 7735HS */
> + .matches = {
> + DMI_MATCH(DMI_SYS_VENDOR, "ASUSTeK COMPUTER INC."),
> + DMI_MATCH(DMI_PRODUCT_NAME, "FA617NS"),
> + },
> + .driver_data = (void *)(SERIO_QUIRK_POLL_KBD)
> + },
> + {
> + /* ASUS TUF Gaming A16 with Ryzen 9 7940HS */
> + .matches = {
> + DMI_MATCH(DMI_SYS_VENDOR, "ASUSTeK COMPUTER INC."),
> + DMI_MATCH(DMI_PRODUCT_NAME, "FA617XS"),
> + },
> + .driver_data = (void *)(SERIO_QUIRK_POLL_KBD)
> + },
> {
> .matches = {
> DMI_MATCH(DMI_SYS_VENDOR, "ASUSTeK COMPUTER INC."),
> @@ -1634,6 +1655,8 @@ static void __init i8042_check_quirks(void)
> if (quirks & SERIO_QUIRK_NOPNP)
> i8042_nopnp = true;
> #endif
> + if (quirks & SERIO_QUIRK_POLL_KBD)
> + i8042_poll_kbd = true;
> }
> #else
> static inline void i8042_check_quirks(void) {}
> @@ -1667,7 +1690,7 @@ static int __init i8042_platform_init(void)
>
> i8042_check_quirks();
>
> - pr_debug("Active quirks (empty means none):%s%s%s%s%s%s%s%s%s%s%s%s%s\n",
> + pr_debug("Active quirks (empty means none):%s%s%s%s%s%s%s%s%s%s%s%s%s%s\n",
> i8042_nokbd ? " nokbd" : "",
> i8042_noaux ? " noaux" : "",
> i8042_nomux ? " nomux" : "",
> @@ -1687,10 +1710,11 @@ static int __init i8042_platform_init(void)
> "",
> #endif
> #ifdef CONFIG_PNP
> - i8042_nopnp ? " nopnp" : "");
> + i8042_nopnp ? " nopnp" : "",
> #else
> - "");
> + "",
> #endif
> + i8042_poll_kbd ? "poll_kbd" : "");
>
> retval = i8042_pnp_init();
> if (retval)
> diff --git a/drivers/input/serio/i8042.c b/drivers/input/serio/i8042.c
> index 6dac7c1853a5..7212263d3a41 100644
> --- a/drivers/input/serio/i8042.c
> +++ b/drivers/input/serio/i8042.c
> @@ -115,6 +115,10 @@ module_param_named(nopnp, i8042_nopnp, bool, 0);
> MODULE_PARM_DESC(nopnp, "Do not use PNP to detect controller settings");
> #endif
>
> +static bool i8042_poll_kbd;
> +module_param_named(poll_kbd, i8042_poll_kbd, bool, 0);
> +MODULE_PARM_DESC(poll_kbd, "Continuously poll the KBD port instead of relying on interrupts");
> +
> #define DEBUG
> #ifdef DEBUG
> static bool i8042_debug;
> @@ -178,6 +182,24 @@ static irqreturn_t i8042_interrupt(int irq, void *dev_id);
> static bool (*i8042_platform_filter)(unsigned char data, unsigned char str,
> struct serio *serio);
>
> +#define POLL_TIME 1
> +static void i8042_poll_func(struct timer_list *timer)
> +{
> + unsigned char status;
> + unsigned long flags;
> +
> + do {
> + spin_lock_irqsave(&i8042_lock, flags);
> + status = i8042_read_status();
> + spin_unlock_irqrestore(&i8042_lock, flags);
> + if (status & I8042_STR_OBF)
> + i8042_interrupt(0, NULL);
> + } while (status & I8042_STR_OBF);
> + mod_timer(timer, jiffies + msecs_to_jiffies(POLL_TIME));
> +}
> +
> +DEFINE_TIMER(poll_timer, i8042_poll_func);
> +
> void i8042_lock_chip(void)
> {
> mutex_lock(&i8042_mutex);
> @@ -1437,13 +1459,15 @@ static void i8042_unregister_ports(void)
> }
> }
>
> +
> static void i8042_free_irqs(void)
> {
> if (i8042_aux_irq_registered)
> free_irq(I8042_AUX_IRQ, i8042_platform_device);
> - if (i8042_kbd_irq_registered)
> + if (i8042_poll_kbd)
> + del_timer(&poll_timer);
> + else if (i8042_kbd_irq_registered)
> free_irq(I8042_KBD_IRQ, i8042_platform_device);
> -
> i8042_aux_irq_registered = i8042_kbd_irq_registered = false;
> }
>
> @@ -1497,10 +1521,14 @@ static int i8042_setup_kbd(void)
> if (error)
> return error;
>
> - error = request_irq(I8042_KBD_IRQ, i8042_interrupt, IRQF_SHARED,
> - "i8042", i8042_platform_device);
> - if (error)
> - goto err_free_port;
> + if (i8042_poll_kbd)
> + mod_timer(&poll_timer, msecs_to_jiffies(POLL_TIME));
> + else {
> + error = request_irq(I8042_KBD_IRQ, i8042_interrupt, IRQF_SHARED,
> + "i8042", i8042_platform_device);
> + if (error)
> + goto err_free_port;
> + }
>
> error = i8042_enable_kbd_port();
> if (error)
> @@ -1510,8 +1538,11 @@ static int i8042_setup_kbd(void)
> return 0;
>
> err_free_irq:
> - free_irq(I8042_KBD_IRQ, i8042_platform_device);
> - err_free_port:
> + if (i8042_poll_kbd)
> + del_timer(&poll_timer);
> + else
> + free_irq(I8042_KBD_IRQ, i8042_platform_device);
> +err_free_port:
> i8042_free_kbd_port();
> return error;
> }
> --
> 2.40.1
>
Thanks.
--
Dmitry
^ permalink raw reply
* Re: [PATCH v2 1/2] Input: exc3000 - Simplify probe()
From: Dmitry Torokhov @ 2023-07-21 22:10 UTC (permalink / raw)
To: Biju Das
Cc: Mark Brown, Mike Looijmans, Andreas Helbech Kleist,
Geert Uytterhoeven, Uwe Kleine-König,
linux-input@vger.kernel.org, Prabhakar Mahadev Lad,
linux-renesas-soc@vger.kernel.org, Wolfram Sang, Andy Shevchenko,
linux-kernel@vger.kernel.org
In-Reply-To: <TYCPR01MB593346FBBA320260A290EAFD8639A@TYCPR01MB5933.jpnprd01.prod.outlook.com>
On Wed, Jul 19, 2023 at 06:43:47AM +0000, Biju Das wrote:
> Hi Dmitry Torokhov,
>
> Thanks for the feedback.
>
> > Subject: Re: [PATCH v2 1/2] Input: exc3000 - Simplify probe()
> >
> > On Mon, Jul 17, 2023 at 06:45:27PM +0000, Biju Das wrote:
> > > Hi Dmitry,
> > >
> > > > Subject: Re: [PATCH v2 1/2] Input: exc3000 - Simplify probe()
> > > >
> > > > On Mon, Jul 17, 2023 at 07:15:50PM +0100, Mark Brown wrote:
> > > > > On Mon, Jul 17, 2023 at 04:35:02PM +0000, Biju Das wrote:
> > > > >
> > > > > > The .device_get_match_data callbacks are missing for I2C and SPI
> > > > > > bus
> > > > subsystems.
> > > > > > Can you please throw some lights on this?
> > > > >
> > > > > It's the first time I've ever heard of that callback, I don't know
> > > > > why whoever added it wouldn't have done those buses in particular
> > > > > or if it just didn't happen. Try adding it and if it works send
> > the patches?
> > > >
> > > > I think there is a disconnect. Right now device_get_match_data
> > > > callbacks are part of fwnode_operations. I was proposing to add
> > > > another optional device_get_match_data callback to 'struct bus_type'
> > > > to allow individual buses control how match data is handled, before
> > > > (or after) jumping into the fwnode-backed device_get_match_data
> > callbacks.
> > >
> > > That is what implemented here [1] and [2] right?
>
> [1] https://elixir.bootlin.com/linux/v6.5-rc2/source/drivers/spi/spi.c#L364
>
> [2] https://elixir.bootlin.com/linux/v6.5-rc2/source/drivers/i2c/i2c-core-base.c#L117
>
> > >
> > >
> > > First it check for fwnode-backed device_get_match_data callbacks and
> > > Fallback is bus-type based match.
> > >
> > > Looks like you are proposing to unify [1] and [2] and you want the
> > > logic to be other way around. ie, first bus-type match, then
> > > fwnode-backed callbacks?
> > >
> >
> > I do not have a strong preference for the ordering, i.e. I think it is
> > perfectly fine to do the generic fwnode-based lookup and if there is no
> > match have bus method called as a fallback,
>
> That involves a bit of work.
>
> const void *device_get_match_data(const struct device *dev);
>
> const struct i2c_device_id *i2c_match_id(const struct i2c_device_id *id,
> const struct i2c_client *client);
>
> const struct spi_device_id *spi_get_device_id(const struct spi_device *sdev);
>
> Basically, the bus-client driver(such as exc3000) needs to pass struct device
> and device_get_match_data after generic fwnode-based lookup,
> needs to find the bus type based on struct device and call a new generic
> void* bus_get_match_data(void*) callback, so that each bus interface
> can do a match.
Yes, something like this (which does not seem that involved to me...):
diff --git a/drivers/base/property.c b/drivers/base/property.c
index 8c40abed7852..cc0bf7bb6f3a 100644
--- a/drivers/base/property.c
+++ b/drivers/base/property.c
@@ -1277,7 +1277,13 @@ EXPORT_SYMBOL(fwnode_graph_parse_endpoint);
const void *device_get_match_data(const struct device *dev)
{
- return fwnode_call_ptr_op(dev_fwnode(dev), device_get_match_data, dev);
+ const void *data;
+
+ data = fwnode_call_ptr_op(dev_fwnode(dev), device_get_match_data, dev);
+ if (!data && dev->bus && dev->bus->get_match_data)
+ data = dev->bus->get_match_data(dev);
+
+ return data;
}
EXPORT_SYMBOL_GPL(device_get_match_data);
diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
index 60746652fd52..5fe47bc491a6 100644
--- a/drivers/i2c/i2c-core-base.c
+++ b/drivers/i2c/i2c-core-base.c
@@ -114,6 +114,26 @@ const struct i2c_device_id *i2c_match_id(const struct i2c_device_id *id,
}
EXPORT_SYMBOL_GPL(i2c_match_id);
+static const void *i2c_device_get_match_data(const struct device *dev)
+{
+ const struct i2c_client *client = to_i2c_client(dev);
+ const struct i2c_driver *driver;
+ const struct i2c_device_id *match;
+
+ if (!dev->driver)
+ return NULL;
+
+ driver = to_i2c_driver(dev->driver);
+ if (!driver)
+ return NULL;
+
+ match = i2c_match_id(driver->id_table, client);
+ if (!match)
+ return NULL;
+
+ return (const void *)match->driver_data;
+}
+
const void *i2c_get_match_data(const struct i2c_client *client)
{
struct i2c_driver *driver = to_i2c_driver(client->dev.driver);
@@ -695,6 +715,7 @@ struct bus_type i2c_bus_type = {
.probe = i2c_device_probe,
.remove = i2c_device_remove,
.shutdown = i2c_device_shutdown,
+ .get_match_data = i2c_device_get_match_data,
};
EXPORT_SYMBOL_GPL(i2c_bus_type);
diff --git a/include/linux/device/bus.h b/include/linux/device/bus.h
index ae10c4322754..3f2cba28a1af 100644
--- a/include/linux/device/bus.h
+++ b/include/linux/device/bus.h
@@ -102,6 +102,8 @@ struct bus_type {
int (*dma_configure)(struct device *dev);
void (*dma_cleanup)(struct device *dev);
+ const void *(*get_match_data)(const struct device *dev);
+
const struct dev_pm_ops *pm;
const struct iommu_ops *iommu_ops;
Thanks.
--
Dmitry
^ permalink raw reply related
* Re: [PATCH v3] HID: Add introduction about HID for non-kernel programmers
From: Jonathan Corbet @ 2023-07-21 20:29 UTC (permalink / raw)
To: Marco Morandini, Peter Hutterer, Jiri Kosina, Benjamin Tissoires,
linux-input, linux-doc
In-Reply-To: <9c021454-bb7a-a8ce-dc2d-ee9f168c9db5@polimi.it>
Marco Morandini <marco.morandini@polimi.it> writes:
> Add an introduction about HID meant for the casual programmer
> that is trying either to fix his device or to understand
> what is going wrong.
>
> Signed-off-by: Marco Morandini <marco.morandini@polimi.it>
> Co-authored-by: Peter Hutterer <peter.hutterer@who-t.net>
> ---
> v2: https://lore.kernel.org/linux-input/70fdef05-d3b8-e24b-77be-901bd5be369e@polimi.it/
>
> changes:
> - corrections suggested in https://lore.kernel.org/linux-input/20230627060437.GA726439@quokka/
> - corrections suggested in https://lore.kernel.org/linux-input/f53e756f-7c81-1c79-23ea-b9009fdd2ef4@infradead.org/
> - corrections suggested in https://lore.kernel.org/linux-input/20230710021034.GA600582@quokka/
> - corrections suggested in https://lore.kernel.org/linux-input/20230717001544.GA129954@quokka/
> - some rewording of Documentation/hid/hidreport-parsing.rst
>
> Documentation/hid/hidintro.rst | 524 ++++++++++++++++++++++++
> Documentation/hid/hidreport-parsing.rst | 49 +++
> Documentation/hid/index.rst | 1 +
> include/linux/hid.h | 23 ++
> 4 files changed, 597 insertions(+)
> create mode 100644 Documentation/hid/hidintro.rst
> create mode 100644 Documentation/hid/hidreport-parsing.rst
Is there a plan for this patch? Please let me know if I should take it
through the docs tree.
Thanks,
jon
^ permalink raw reply
* Re: [PATCH] HID: nintendo: cleanup LED code
From: Silvan Jegen @ 2023-07-21 19:09 UTC (permalink / raw)
To: Martino Fontana; +Cc: linux-input
In-Reply-To: <CAKst+mB2wnDt6gdQhApZXydYNx6NHdGBBR8LHMng0i4RJFyY1Q@mail.gmail.com>
Hi Martino
Martino Fontana <tinozzo123@gmail.com> wrote:
> The reason I avoid returning `ret` is that, if something other than 0
> is returned, then `nintendo_hid_probe` (the function calling
> `joycon_leds_create`) would fail completely. That obviously shouldn't
> happen, LEDs aren't a vital piece for the gamepad functionality.
While this might be true, if this fails it would indicate a potential
bug in the driver. Failing loudly in this case (especially if this
was the behavior before your changes) would lead to this bug being
investigated earlier.
If we just ignore the failure, people using the code may not be aware
that the LEDs were supposed to work but failed for some reason. In that
case we may never get a bug report and the LED creation would stay broken
in this case.
That's why I would prefer to fail the probe as well if the LED creation
fails.
This is just my opinion though.
Cheers,
Silvan
> Alternatively, instead of avoiding returning anything other than 0,
> `nintendo_hid_probe` could be modified by changing `if (ret)` in that
> section to something else.
> I'm not an expert in C, so... opinions?
>
> Il giorno gio 20 lug 2023 alle ore 22:04 Silvan Jegen
> <s.jegen@gmail.com> ha scritto:
> >
> > Hi!
> >
> > Some comments inline below.
> >
> > Martino Fontana <tinozzo123@gmail.com> wrote:
> > > - Only turn on the nth LED, instead of all the LEDs up to n. This better matches Nintendo consoles' behaviour, as they never turn on more than one LED (still not a complete match, see https://bugzilla.kernel.org/show_bug.cgi?id=216225)
> > > - Split part of `joycon_home_led_brightness_set` (which is called by hid) into `joycon_set_home_led` (which is what actually sets the LEDs), for consistency with player LEDs
> > > - Instead of first registering the `led_classdev`, then attempting to set the LED and unregistering the `led_classdev` if it fails, first attempt to set the LED, then register the `led_classdev` only if it succeeds (the class is still filled up in either case)
> > > - If setting player LEDs fails, still attempt setting the home LED (I don't know if this is actually possible, but who knows...)
> > > - Use `JC_NUM_LEDS` when appropriate instead of 4
> > > - Print return codes
> >
> > Wrapping the git commit message around 78 chars or so is conventional.
> >
> >
> > > Signed-off-by: Martino Fontana <tinozzo123@gmail.com>
> > > ---
> > > drivers/hid/hid-nintendo.c | 116 ++++++++++++++++++-------------------
> > > 1 file changed, 55 insertions(+), 61 deletions(-)
> > >
> > > diff --git a/drivers/hid/hid-nintendo.c b/drivers/hid/hid-nintendo.c
> > > index 586c7f8d7..89631e19b 100644
> > > --- a/drivers/hid/hid-nintendo.c
> > > +++ b/drivers/hid/hid-nintendo.c
> > > @@ -699,6 +699,25 @@ static int joycon_set_player_leds(struct joycon_ctlr *ctlr, u8 flash, u8 on)
> > > return joycon_send_subcmd(ctlr, req, 1, HZ/4);
> > > }
> > >
> > > +static int joycon_set_home_led(struct joycon_ctlr *ctlr, enum led_brightness brightness)
> > > +{
> > > + struct joycon_subcmd_request *req;
> > > + u8 buffer[sizeof(*req) + 5] = { 0 };
> > > + u8 *data;
> > > +
> > > + req = (struct joycon_subcmd_request *)buffer;
> > > + req->subcmd_id = JC_SUBCMD_SET_HOME_LIGHT;
> > > + data = req->data;
> > > + data[0] = 0x01;
> > > + data[1] = brightness << 4;
> > > + data[2] = brightness | (brightness << 4);
> > > + data[3] = 0x11;
> > > + data[4] = 0x11;
> > > +
> > > + hid_dbg(ctlr->hdev, "setting home led brightness\n");
> > > + return joycon_send_subcmd(ctlr, req, 5, HZ/4);
> > > +}
> > > +
> > > static int joycon_request_spi_flash_read(struct joycon_ctlr *ctlr,
> > > u32 start_addr, u8 size, u8 **reply)
> > > {
> > > @@ -1849,7 +1868,6 @@ static int joycon_player_led_brightness_set(struct led_classdev *led,
> > > int val = 0;
> > > int i;
> > > int ret;
> > > - int num;
> > >
> > > ctlr = hid_get_drvdata(hdev);
> > > if (!ctlr) {
> > > @@ -1857,21 +1875,10 @@ static int joycon_player_led_brightness_set(struct led_classdev *led,
> > > return -ENODEV;
> > > }
> > >
> > > - /* determine which player led this is */
> > > - for (num = 0; num < JC_NUM_LEDS; num++) {
> > > - if (&ctlr->leds[num] == led)
> > > - break;
> > > - }
> > > - if (num >= JC_NUM_LEDS)
> > > - return -EINVAL;
> > > + for (i = 0; i < JC_NUM_LEDS; i++)
> > > + val |= ctlr->leds[i].brightness << i;
> > >
> > > mutex_lock(&ctlr->output_mutex);
> > > - for (i = 0; i < JC_NUM_LEDS; i++) {
> > > - if (i == num)
> > > - val |= brightness << i;
> > > - else
> > > - val |= ctlr->leds[i].brightness << i;
> > > - }
> > > ret = joycon_set_player_leds(ctlr, 0, val);
> > > mutex_unlock(&ctlr->output_mutex);
> > >
> > > @@ -1884,9 +1891,6 @@ static int joycon_home_led_brightness_set(struct led_classdev *led,
> > > struct device *dev = led->dev->parent;
> > > struct hid_device *hdev = to_hid_device(dev);
> > > struct joycon_ctlr *ctlr;
> > > - struct joycon_subcmd_request *req;
> > > - u8 buffer[sizeof(*req) + 5] = { 0 };
> > > - u8 *data;
> > > int ret;
> > >
> > > ctlr = hid_get_drvdata(hdev);
> > > @@ -1894,25 +1898,13 @@ static int joycon_home_led_brightness_set(struct led_classdev *led,
> > > hid_err(hdev, "No controller data\n");
> > > return -ENODEV;
> > > }
> > > -
> > > - req = (struct joycon_subcmd_request *)buffer;
> > > - req->subcmd_id = JC_SUBCMD_SET_HOME_LIGHT;
> > > - data = req->data;
> > > - data[0] = 0x01;
> > > - data[1] = brightness << 4;
> > > - data[2] = brightness | (brightness << 4);
> > > - data[3] = 0x11;
> > > - data[4] = 0x11;
> > > -
> > > - hid_dbg(hdev, "setting home led brightness\n");
> > > mutex_lock(&ctlr->output_mutex);
> > > - ret = joycon_send_subcmd(ctlr, req, 5, HZ/4);
> > > + ret = joycon_set_home_led(ctlr, brightness);
> > > mutex_unlock(&ctlr->output_mutex);
> > > -
> > > return ret;
> > > }
> > >
> > > -static DEFINE_MUTEX(joycon_input_num_mutex);
> > > +static DEFINE_SPINLOCK(joycon_input_num_spinlock);
> > > static int joycon_leds_create(struct joycon_ctlr *ctlr)
> > > {
> > > struct hid_device *hdev = ctlr->hdev;
> > > @@ -1920,17 +1912,16 @@ static int joycon_leds_create(struct joycon_ctlr *ctlr)
> > > const char *d_name = dev_name(dev);
> > > struct led_classdev *led;
> > > char *name;
> > > - int ret = 0;
> > > + int ret;
> > > int i;
> > > - static int input_num = 1;
> > > + unsigned long flags;
> > > + int player_led_number;
> > > + static int input_num;
> > >
> > > - /* Set the default controller player leds based on controller number */
> > > - mutex_lock(&joycon_input_num_mutex);
> > > - mutex_lock(&ctlr->output_mutex);
> > > - ret = joycon_set_player_leds(ctlr, 0, 0xF >> (4 - input_num));
> > > - if (ret)
> > > - hid_warn(ctlr->hdev, "Failed to set leds; ret=%d\n", ret);
> > > - mutex_unlock(&ctlr->output_mutex);
> > > + /* Set the player leds based on controller number */
> > > + spin_lock_irqsave(&joycon_input_num_spinlock, flags);
> > > + player_led_number = input_num++ % JC_NUM_LEDS;
> > > + spin_unlock_irqrestore(&joycon_input_num_spinlock, flags);
> > >
> > > /* configure the player LEDs */
> > > for (i = 0; i < JC_NUM_LEDS; i++) {
> > > @@ -1938,31 +1929,33 @@ static int joycon_leds_create(struct joycon_ctlr *ctlr)
> > > d_name,
> > > "green",
> > > joycon_player_led_names[i]);
> > > - if (!name) {
> > > - mutex_unlock(&joycon_input_num_mutex);
> > > + if (!name)
> > > return -ENOMEM;
> > > - }
> > >
> > > led = &ctlr->leds[i];
> > > led->name = name;
> > > - led->brightness = ((i + 1) <= input_num) ? 1 : 0;
> > > + led->brightness = (i == player_led_number) ? 1 : 0;
> > > led->max_brightness = 1;
> > > led->brightness_set_blocking =
> > > joycon_player_led_brightness_set;
> > > led->flags = LED_CORE_SUSPENDRESUME | LED_HW_PLUGGABLE;
> > > + }
> > > + mutex_lock(&ctlr->output_mutex);
> > > + ret = joycon_set_player_leds(ctlr, 0, 0x1 << player_led_number);
> > > + mutex_unlock(&ctlr->output_mutex);
> > > + if (ret) {
> > > + hid_warn(hdev, "Failed to set players LEDs; ret=%d\n", ret);
> > > + goto home_led;
> > > + }
> > >
> > > + for (i = 0; i < JC_NUM_LEDS; i++) {
> > > + led = &ctlr->leds[i];
> > > ret = devm_led_classdev_register(&hdev->dev, led);
> > > - if (ret) {
> > > - hid_err(hdev, "Failed registering %s LED\n", led->name);
> > > - mutex_unlock(&joycon_input_num_mutex);
> > > - return ret;
> > > - }
> > > + if (ret)
> > > + hid_err(hdev, "Failed registering %s LED; ret=%d\n", led->name, ret);
> > > }
> > >
> > > - if (++input_num > 4)
> > > - input_num = 1;
> > > - mutex_unlock(&joycon_input_num_mutex);
> > > -
> > > +home_led:
> > > /* configure the home LED */
> > > if (jc_type_has_right(ctlr)) {
> > > name = devm_kasprintf(dev, GFP_KERNEL, "%s:%s:%s",
> > > @@ -1978,17 +1971,18 @@ static int joycon_leds_create(struct joycon_ctlr *ctlr)
> > > led->max_brightness = 0xF;
> > > led->brightness_set_blocking = joycon_home_led_brightness_set;
> > > led->flags = LED_CORE_SUSPENDRESUME | LED_HW_PLUGGABLE;
> > > - ret = devm_led_classdev_register(&hdev->dev, led);
> > > - if (ret) {
> > > - hid_err(hdev, "Failed registering home led\n");
> > > - return ret;
> > > - }
> > > +
> > > /* Set the home LED to 0 as default state */
> > > - ret = joycon_home_led_brightness_set(led, 0);
> > > + mutex_lock(&ctlr->output_mutex);
> > > + ret = joycon_set_home_led(ctlr, 0);
> > > + mutex_unlock(&ctlr->output_mutex);
> >
> > We shouldn't use any spaces for indentation here.
> >
> > > if (ret) {
> > > - hid_warn(hdev, "Failed to set home LED default, unregistering home LED");
> > > - devm_led_classdev_unregister(&hdev->dev, led);
> > > + hid_warn(hdev, "Failed to set home LED; ret=%d\n", ret);
> > > + return 0;
> > > }
> >
> > Adding an empty line here would be more consistent with the rest of
> > the code.
> >
> > > + ret = devm_led_classdev_register(&hdev->dev, led);
> > > + if (ret)
> > > + hid_err(hdev, "Failed registering home led; ret=%d\n", ret);
> >
> > In the old code we returned "ret" in this case. We probably want to do
> > the same here.
> >
> > Cheers,
> > Silvan
> >
> > > }
> > >
> > > return 0;
> >
> >
^ permalink raw reply
* Re: [PATCH v2] HID: nintendo: reinitialize USB Pro Controller after resuming from suspend
From: Silvan Jegen @ 2023-07-21 19:02 UTC (permalink / raw)
To: Martino Fontana; +Cc: linux-input, tinozzo123
In-Reply-To: <20230720233150.57164-2-tinozzo123@gmail.com>
Martino Fontana <tinozzo123@gmail.com> wrote:
> When suspending the computer, a Switch Pro Controller connected via USB will
> lose its internal status. However, because the USB connection was technically
> never lost, when resuming the computer, the driver will attempt to communicate
> with the controller as if nothing happened (and fail).
> Because of this, the user was forced to manually disconnect the controller
> (or to press the sync button on the controller to power it off), so that it
> can be re-initialized.
>
> With this patch, the controller will be automatically re-initialized after
> resuming from suspend.
>
> Fixes https://bugzilla.kernel.org/show_bug.cgi?id=216233
>
> Signed-off-by: Martino Fontana <tinozzo123@gmail.com>
It would be good to add a small section about what changed between v1
and v2 of the patch. Here is an example for how this can be done.
https://lore.kernel.org/lkml/cover.b24362332ec6099bc8db4e8e06a67545c653291d.1689842332.git-series.apopple@nvidia.com/T/#m73dd8d44f40742e67cbb0d4f030a90b6264a88d3
It's probably not worth resending this patch just for this though. Just
something to keep in mind if there will be another patch version needed.
Cheers,
Silvan
> ---
> drivers/hid/hid-nintendo.c | 178 ++++++++++++++++++++++---------------
> 1 file changed, 106 insertions(+), 72 deletions(-)
>
> diff --git a/drivers/hid/hid-nintendo.c b/drivers/hid/hid-nintendo.c
> index 250f5d2f8..a5ebe857a 100644
> --- a/drivers/hid/hid-nintendo.c
> +++ b/drivers/hid/hid-nintendo.c
> @@ -2088,7 +2088,9 @@ static int joycon_read_info(struct joycon_ctlr *ctlr)
> struct joycon_input_report *report;
>
> req.subcmd_id = JC_SUBCMD_REQ_DEV_INFO;
> + mutex_lock(&ctlr->output_mutex);
> ret = joycon_send_subcmd(ctlr, &req, 0, HZ);
> + mutex_unlock(&ctlr->output_mutex);
> if (ret) {
> hid_err(ctlr->hdev, "Failed to get joycon info; ret=%d\n", ret);
> return ret;
> @@ -2117,6 +2119,88 @@ static int joycon_read_info(struct joycon_ctlr *ctlr)
> return 0;
> }
>
> +static int joycon_init(struct hid_device *hdev)
> +{
> + struct joycon_ctlr *ctlr = hid_get_drvdata(hdev);
> + int ret = 0;
> +
> + mutex_lock(&ctlr->output_mutex);
> + /* if handshake command fails, assume ble pro controller */
> + if ((jc_type_is_procon(ctlr) || jc_type_is_chrggrip(ctlr)) &&
> + !joycon_send_usb(ctlr, JC_USB_CMD_HANDSHAKE, HZ)) {
> + hid_dbg(hdev, "detected USB controller\n");
> + /* set baudrate for improved latency */
> + ret = joycon_send_usb(ctlr, JC_USB_CMD_BAUDRATE_3M, HZ);
> + if (ret) {
> + hid_err(hdev, "Failed to set baudrate; ret=%d\n", ret);
> + goto err_mutex;
> + }
> + /* handshake */
> + ret = joycon_send_usb(ctlr, JC_USB_CMD_HANDSHAKE, HZ);
> + if (ret) {
> + hid_err(hdev, "Failed handshake; ret=%d\n", ret);
> + goto err_mutex;
> + }
> + /*
> + * Set no timeout (to keep controller in USB mode).
> + * This doesn't send a response, so ignore the timeout.
> + */
> + joycon_send_usb(ctlr, JC_USB_CMD_NO_TIMEOUT, HZ/10);
> + } else if (jc_type_is_chrggrip(ctlr)) {
> + hid_err(hdev, "Failed charging grip handshake\n");
> + ret = -ETIMEDOUT;
> + goto err_mutex;
> + }
> +
> + /* get controller calibration data, and parse it */
> + ret = joycon_request_calibration(ctlr);
> + if (ret) {
> + /*
> + * We can function with default calibration, but it may be
> + * inaccurate. Provide a warning, and continue on.
> + */
> + hid_warn(hdev, "Analog stick positions may be inaccurate\n");
> + }
> +
> + /* get IMU calibration data, and parse it */
> + ret = joycon_request_imu_calibration(ctlr);
> + if (ret) {
> + /*
> + * We can function with default calibration, but it may be
> + * inaccurate. Provide a warning, and continue on.
> + */
> + hid_warn(hdev, "Unable to read IMU calibration data\n");
> + }
> +
> + /* Set the reporting mode to 0x30, which is the full report mode */
> + ret = joycon_set_report_mode(ctlr);
> + if (ret) {
> + hid_err(hdev, "Failed to set report mode; ret=%d\n", ret);
> + goto err_mutex;
> + }
> +
> + /* Enable rumble */
> + ret = joycon_enable_rumble(ctlr);
> + if (ret) {
> + hid_err(hdev, "Failed to enable rumble; ret=%d\n", ret);
> + goto err_mutex;
> + }
> +
> + /* Enable the IMU */
> + ret = joycon_enable_imu(ctlr);
> + if (ret) {
> + hid_err(hdev, "Failed to enable the IMU; ret=%d\n", ret);
> + goto err_mutex;
> + }
> +
> + mutex_unlock(&ctlr->output_mutex);
> + return 0;
> +
> +err_mutex:
> + mutex_unlock(&ctlr->output_mutex);
> + return ret;
> +}
> +
> /* Common handler for parsing inputs */
> static int joycon_ctlr_read_handler(struct joycon_ctlr *ctlr, u8 *data,
> int size)
> @@ -2248,85 +2332,19 @@ static int nintendo_hid_probe(struct hid_device *hdev,
>
> hid_device_io_start(hdev);
>
> - /* Initialize the controller */
> - mutex_lock(&ctlr->output_mutex);
> - /* if handshake command fails, assume ble pro controller */
> - if ((jc_type_is_procon(ctlr) || jc_type_is_chrggrip(ctlr)) &&
> - !joycon_send_usb(ctlr, JC_USB_CMD_HANDSHAKE, HZ)) {
> - hid_dbg(hdev, "detected USB controller\n");
> - /* set baudrate for improved latency */
> - ret = joycon_send_usb(ctlr, JC_USB_CMD_BAUDRATE_3M, HZ);
> - if (ret) {
> - hid_err(hdev, "Failed to set baudrate; ret=%d\n", ret);
> - goto err_mutex;
> - }
> - /* handshake */
> - ret = joycon_send_usb(ctlr, JC_USB_CMD_HANDSHAKE, HZ);
> - if (ret) {
> - hid_err(hdev, "Failed handshake; ret=%d\n", ret);
> - goto err_mutex;
> - }
> - /*
> - * Set no timeout (to keep controller in USB mode).
> - * This doesn't send a response, so ignore the timeout.
> - */
> - joycon_send_usb(ctlr, JC_USB_CMD_NO_TIMEOUT, HZ/10);
> - } else if (jc_type_is_chrggrip(ctlr)) {
> - hid_err(hdev, "Failed charging grip handshake\n");
> - ret = -ETIMEDOUT;
> - goto err_mutex;
> - }
> -
> - /* get controller calibration data, and parse it */
> - ret = joycon_request_calibration(ctlr);
> - if (ret) {
> - /*
> - * We can function with default calibration, but it may be
> - * inaccurate. Provide a warning, and continue on.
> - */
> - hid_warn(hdev, "Analog stick positions may be inaccurate\n");
> - }
> -
> - /* get IMU calibration data, and parse it */
> - ret = joycon_request_imu_calibration(ctlr);
> - if (ret) {
> - /*
> - * We can function with default calibration, but it may be
> - * inaccurate. Provide a warning, and continue on.
> - */
> - hid_warn(hdev, "Unable to read IMU calibration data\n");
> - }
> -
> - /* Set the reporting mode to 0x30, which is the full report mode */
> - ret = joycon_set_report_mode(ctlr);
> - if (ret) {
> - hid_err(hdev, "Failed to set report mode; ret=%d\n", ret);
> - goto err_mutex;
> - }
> -
> - /* Enable rumble */
> - ret = joycon_enable_rumble(ctlr);
> - if (ret) {
> - hid_err(hdev, "Failed to enable rumble; ret=%d\n", ret);
> - goto err_mutex;
> - }
> -
> - /* Enable the IMU */
> - ret = joycon_enable_imu(ctlr);
> + ret = joycon_init(hdev);
> if (ret) {
> - hid_err(hdev, "Failed to enable the IMU; ret=%d\n", ret);
> - goto err_mutex;
> + hid_err(hdev, "Failed to initialize controller; ret=%d\n", ret);
> + goto err_close;
> }
>
> ret = joycon_read_info(ctlr);
> if (ret) {
> hid_err(hdev, "Failed to retrieve controller info; ret=%d\n",
> ret);
> - goto err_mutex;
> + goto err_close;
> }
>
> - mutex_unlock(&ctlr->output_mutex);
> -
> /* Initialize the leds */
> ret = joycon_leds_create(ctlr);
> if (ret) {
> @@ -2352,8 +2370,6 @@ static int nintendo_hid_probe(struct hid_device *hdev,
> hid_dbg(hdev, "probe - success\n");
> return 0;
>
> -err_mutex:
> - mutex_unlock(&ctlr->output_mutex);
> err_close:
> hid_hw_close(hdev);
> err_stop:
> @@ -2383,6 +2399,20 @@ static void nintendo_hid_remove(struct hid_device *hdev)
> hid_hw_stop(hdev);
> }
>
> +#ifdef CONFIG_PM
> +
> +static int nintendo_hid_resume(struct hid_device *hdev)
> +{
> + int ret = joycon_init(hdev);
> +
> + if (ret)
> + hid_err(hdev, "Failed to restore controller after resume");
> +
> + return ret;
> +}
> +
> +#endif
> +
> static const struct hid_device_id nintendo_hid_devices[] = {
> { HID_USB_DEVICE(USB_VENDOR_ID_NINTENDO,
> USB_DEVICE_ID_NINTENDO_PROCON) },
> @@ -2404,6 +2434,10 @@ static struct hid_driver nintendo_hid_driver = {
> .probe = nintendo_hid_probe,
> .remove = nintendo_hid_remove,
> .raw_event = nintendo_hid_event,
> +
> +#ifdef CONFIG_PM
> + .resume = nintendo_hid_resume,
> +#endif
> };
> module_hid_driver(nintendo_hid_driver);
>
^ permalink raw reply
* Re: [PATCH v3 18/42] spi: ep93xx: add DT support for Cirrus EP93xx
From: Andy Shevchenko @ 2023-07-21 16:42 UTC (permalink / raw)
To: Nikita Shubin
Cc: Hartley Sweeten, Lennert Buytenhek, Alexander Sverdlin,
Russell King, Lukasz Majewski, Linus Walleij, Bartosz Golaszewski,
Rob Herring, Krzysztof Kozlowski, Conor Dooley, Michael Turquette,
Stephen Boyd, Daniel Lezcano, Thomas Gleixner, Alessandro Zummo,
Alexandre Belloni, Wim Van Sebroeck, Guenter Roeck,
Sebastian Reichel, Thierry Reding, Uwe Kleine-König,
Mark Brown, David S. Miller, Eric Dumazet, Jakub Kicinski,
Paolo Abeni, Vinod Koul, Miquel Raynal, Richard Weinberger,
Vignesh Raghavendra, Damien Le Moal, Sergey Shtylyov,
Dmitry Torokhov, Arnd Bergmann, Olof Johansson, soc,
Liam Girdwood, Jaroslav Kysela, Takashi Iwai, Michael Peters,
Kris Bahnsen, linux-arm-kernel, linux-kernel, linux-gpio,
devicetree, linux-clk, linux-rtc, linux-watchdog, linux-pm,
linux-pwm, linux-spi, netdev, dmaengine, linux-mtd, linux-ide,
linux-input, alsa-devel
In-Reply-To: <20230605-ep93xx-v3-18-3d63a5f1103e@maquefel.me>
On Thu, Jul 20, 2023 at 02:29:18PM +0300, Nikita Shubin via B4 Relay wrote:
> From: Nikita Shubin <nikita.shubin@maquefel.me>
>
> - add OF ID match table
>
> Instead of platform use_dma flag - check if DT dmas property is present to
> decide to use dma ot not.
...
> +#ifdef CONFIG_OF
Why ifdeffery?
Can't it be first checked for firmware provided info and fell back to platdata?
> +static struct ep93xx_spi_info dt_spi_info;
...
> +#endif
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply
* Re: [PATCH v3 14/42] power: reset: Add a driver for the ep93xx reset
From: Andy Shevchenko @ 2023-07-21 16:37 UTC (permalink / raw)
To: nikita.shubin
Cc: Hartley Sweeten, Lennert Buytenhek, Alexander Sverdlin,
Russell King, Lukasz Majewski, Linus Walleij, Bartosz Golaszewski,
Rob Herring, Krzysztof Kozlowski, Conor Dooley, Michael Turquette,
Stephen Boyd, Daniel Lezcano, Thomas Gleixner, Alessandro Zummo,
Alexandre Belloni, Wim Van Sebroeck, Guenter Roeck,
Sebastian Reichel, Thierry Reding, Uwe Kleine-König,
Mark Brown, David S. Miller, Eric Dumazet, Jakub Kicinski,
Paolo Abeni, Vinod Koul, Miquel Raynal, Richard Weinberger,
Vignesh Raghavendra, Damien Le Moal, Sergey Shtylyov,
Dmitry Torokhov, Arnd Bergmann, Olof Johansson, soc,
Liam Girdwood, Jaroslav Kysela, Takashi Iwai, Michael Peters,
Kris Bahnsen, linux-arm-kernel, linux-kernel, linux-gpio,
devicetree, linux-clk, linux-rtc, linux-watchdog, linux-pm,
linux-pwm, linux-spi, netdev, dmaengine, linux-mtd, linux-ide,
linux-input, alsa-devel
In-Reply-To: <20230605-ep93xx-v3-14-3d63a5f1103e@maquefel.me>
On Thu, Jul 20, 2023 at 02:29:14PM +0300, Nikita Shubin via B4 Relay wrote:
> From: Nikita Shubin <nikita.shubin@maquefel.me>
>
> Implement the reset behaviour of the various EP93xx SoCS in drivers/power/reset.
>
> It used to be located in arch/arm/mach-ep93xx.
...
> +// SPDX-License-Identifier: (GPL-2.0)
Are you sure this is correct form? Have you checked your patches?
...
> +#include <linux/of_device.h>
Do you need this?
Or maybe you need another (of*.h) one?
...
> + /* Issue the reboot */
> + ep93xx_devcfg_set_clear(priv->map, EP93XX_SYSCON_DEVCFG_SWRST, 0x00);
> + ep93xx_devcfg_set_clear(priv->map, 0x00, EP93XX_SYSCON_DEVCFG_SWRST);
> + mdelay(1000);
Atomic?! Such a huge delay must be explained, esp. why it's atomic.
> + pr_emerg("Unable to restart system\n");
> + return NOTIFY_DONE;
...
> + err = register_restart_handler(&priv->restart_handler);
> + if (err)
> + return dev_err_probe(dev, err, "can't register restart notifier\n");
> + return err;
return 0;
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply
* Re: [PATCH v3 20/42] net: cirrus: add DT support for Cirrus EP93xx
From: Andy Shevchenko @ 2023-07-21 16:32 UTC (permalink / raw)
To: nikita.shubin
Cc: Hartley Sweeten, Lennert Buytenhek, Alexander Sverdlin,
Russell King, Lukasz Majewski, Linus Walleij, Bartosz Golaszewski,
Rob Herring, Krzysztof Kozlowski, Conor Dooley, Michael Turquette,
Stephen Boyd, Daniel Lezcano, Thomas Gleixner, Alessandro Zummo,
Alexandre Belloni, Wim Van Sebroeck, Guenter Roeck,
Sebastian Reichel, Thierry Reding, Uwe Kleine-König,
Mark Brown, David S. Miller, Eric Dumazet, Jakub Kicinski,
Paolo Abeni, Vinod Koul, Miquel Raynal, Richard Weinberger,
Vignesh Raghavendra, Damien Le Moal, Sergey Shtylyov,
Dmitry Torokhov, Arnd Bergmann, Olof Johansson, soc,
Liam Girdwood, Jaroslav Kysela, Takashi Iwai, Michael Peters,
Kris Bahnsen, linux-arm-kernel, linux-kernel, linux-gpio,
devicetree, linux-clk, linux-rtc, linux-watchdog, linux-pm,
linux-pwm, linux-spi, netdev, dmaengine, linux-mtd, linux-ide,
linux-input, alsa-devel, Andrew Lunn
In-Reply-To: <20230605-ep93xx-v3-20-3d63a5f1103e@maquefel.me>
On Thu, Jul 20, 2023 at 02:29:20PM +0300, Nikita Shubin via B4 Relay wrote:
> From: Nikita Shubin <nikita.shubin@maquefel.me>
>
> - add OF ID match table
> - get phy_id from the device tree, as part of mdio
> - copy_addr is now always used, as there is no SoC/board that aren't
> - dropped platform header
...
> + base_addr = ioremap(mem->start, resource_size(mem));
> + if (!base_addr)
> + return dev_err_probe(&pdev->dev, -EIO, "Failed to ioremap ethernet registers\n");
> +
> + np = of_parse_phandle(pdev->dev.of_node, "phy-handle", 0);
Isn't it something which is done in PHY core should do for you?
Maybe Andrew Lunn can comment on this.
> + if (!np)
Yeah, right, let's leak mapped IO address space...
And so on.
> + return dev_err_probe(&pdev->dev, -ENODEV, "Please provide \"phy-handle\"\n");
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply
* Re: [PATCH v3 24/42] mtd: nand: add support for ts72xx
From: Andy Shevchenko @ 2023-07-21 16:27 UTC (permalink / raw)
To: nikita.shubin
Cc: Hartley Sweeten, Lennert Buytenhek, Alexander Sverdlin,
Russell King, Lukasz Majewski, Linus Walleij, Bartosz Golaszewski,
Rob Herring, Krzysztof Kozlowski, Conor Dooley, Michael Turquette,
Stephen Boyd, Daniel Lezcano, Thomas Gleixner, Alessandro Zummo,
Alexandre Belloni, Wim Van Sebroeck, Guenter Roeck,
Sebastian Reichel, Thierry Reding, Uwe Kleine-König,
Mark Brown, David S. Miller, Eric Dumazet, Jakub Kicinski,
Paolo Abeni, Vinod Koul, Miquel Raynal, Richard Weinberger,
Vignesh Raghavendra, Damien Le Moal, Sergey Shtylyov,
Dmitry Torokhov, Arnd Bergmann, Olof Johansson, soc,
Liam Girdwood, Jaroslav Kysela, Takashi Iwai, Michael Peters,
Kris Bahnsen, linux-arm-kernel, linux-kernel, linux-gpio,
devicetree, linux-clk, linux-rtc, linux-watchdog, linux-pm,
linux-pwm, linux-spi, netdev, dmaengine, linux-mtd, linux-ide,
linux-input, alsa-devel
In-Reply-To: <20230605-ep93xx-v3-24-3d63a5f1103e@maquefel.me>
On Thu, Jul 20, 2023 at 02:29:24PM +0300, Nikita Shubin via B4 Relay wrote:
> From: Nikita Shubin <nikita.shubin@maquefel.me>
>
> Technologic Systems has it's own nand controller implementation in CPLD.
...
+ bits.h
> +#include <linux/err.h>
> +#include <linux/io.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/slab.h>
...
> +static int ts72xx_nand_attach_chip(struct nand_chip *chip)
> +{
> + switch (chip->ecc.engine_type) {
> + case NAND_ECC_ENGINE_TYPE_SOFT:
> + if (chip->ecc.algo == NAND_ECC_ALGO_UNKNOWN)
> + chip->ecc.algo = NAND_ECC_ALGO_HAMMING;
> + break;
> + case NAND_ECC_ENGINE_TYPE_ON_HOST:
> + return -EINVAL;
> + default:
> + break;
Here it will return 0, is it a problem?
> + }
> +
> + return 0;
> +}
...
> +static int ts72xx_nand_probe(struct platform_device *pdev)
> +{
> + struct ts72xx_nand_data *data;
> + struct device_node *child;
> + struct mtd_info *mtd;
> + int err;
> + /* Allocate memory for the device structure (and zero it) */
Useless comment.
> + data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL);
> + if (!data)
> + return -ENOMEM;
> +
> + data->controller.ops = &ts72xx_nand_ops;
> + nand_controller_init(&data->controller);
> + data->chip.controller = &data->controller;
> +
> + data->io_base = devm_platform_ioremap_resource(pdev, 0);
> + if (IS_ERR(data->io_base))
> + return PTR_ERR(data->io_base);
> +
> + child = of_get_next_child(pdev->dev.of_node, NULL);
Why not using device property API from day 1?
fwnode_get_next_child_node()
> + if (!child)
> + return dev_err_probe(&pdev->dev, -ENXIO,
> + "ts72xx controller node should have exactly one child\n");
From now on you leak the reference count in error path.
> + nand_set_flash_node(&data->chip, child);
> + mtd = nand_to_mtd(&data->chip);
> + mtd->dev.parent = &pdev->dev;
> +
> + data->chip.legacy.IO_ADDR_R = data->io_base;
> + data->chip.legacy.IO_ADDR_W = data->io_base;
> + data->chip.legacy.cmd_ctrl = ts72xx_nand_hwcontrol;
> + data->chip.legacy.dev_ready = ts72xx_nand_device_ready;
> +
> + platform_set_drvdata(pdev, data);
> +
> + /*
> + * This driver assumes that the default ECC engine should be TYPE_SOFT.
> + * Set ->engine_type before registering the NAND devices in order to
> + * provide a driver specific default value.
> + */
> + data->chip.ecc.engine_type = NAND_ECC_ENGINE_TYPE_SOFT;
> +
> + /* Scan to find existence of the device */
> + err = nand_scan(&data->chip, 1);
> + if (err)
> + return err;
> +
> + err = mtd_device_parse_register(mtd, NULL, NULL, NULL, 0);
> + if (err) {
> + nand_cleanup(&data->chip);
> + return err;
> + }
> +
> + return 0;
These 4 lines can be simply
return err;
but see above.
> +}
...
> +static void ts72xx_nand_remove(struct platform_device *pdev)
> +{
> + struct ts72xx_nand_data *data = platform_get_drvdata(pdev);
> + struct nand_chip *chip = &data->chip;
> + int ret;
> +
> + ret = mtd_device_unregister(nand_to_mtd(chip));
> + WARN_ON(ret);
Why?! Is it like this in other MTD drivers?
> + nand_cleanup(chip);
> +}
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply
* Re: [PATCH v3 22/42] dma: cirrus: add DT support for Cirrus EP93xx
From: Andy Shevchenko @ 2023-07-21 16:20 UTC (permalink / raw)
To: nikita.shubin
Cc: Hartley Sweeten, Lennert Buytenhek, Alexander Sverdlin,
Russell King, Lukasz Majewski, Linus Walleij, Bartosz Golaszewski,
Rob Herring, Krzysztof Kozlowski, Conor Dooley, Michael Turquette,
Stephen Boyd, Daniel Lezcano, Thomas Gleixner, Alessandro Zummo,
Alexandre Belloni, Wim Van Sebroeck, Guenter Roeck,
Sebastian Reichel, Thierry Reding, Uwe Kleine-König,
Mark Brown, David S. Miller, Eric Dumazet, Jakub Kicinski,
Paolo Abeni, Vinod Koul, Miquel Raynal, Richard Weinberger,
Vignesh Raghavendra, Damien Le Moal, Sergey Shtylyov,
Dmitry Torokhov, Arnd Bergmann, Olof Johansson, soc,
Liam Girdwood, Jaroslav Kysela, Takashi Iwai, Michael Peters,
Kris Bahnsen, linux-arm-kernel, linux-kernel, linux-gpio,
devicetree, linux-clk, linux-rtc, linux-watchdog, linux-pm,
linux-pwm, linux-spi, netdev, dmaengine, linux-mtd, linux-ide,
linux-input, alsa-devel
In-Reply-To: <20230605-ep93xx-v3-22-3d63a5f1103e@maquefel.me>
On Thu, Jul 20, 2023 at 02:29:22PM +0300, Nikita Shubin via B4 Relay wrote:
> From: Nikita Shubin <nikita.shubin@maquefel.me>
>
> - drop subsys_initcall code
> - add OF ID match table with data
> - add of_probe for device tree
...
> +#include <linux/of_device.h>
Why?
...
> +#ifdef CONFIG_OF
Why this ugly ifdeffery?
...
> + data = of_device_get_match_data(&pdev->dev);
device_get_match_data()
> + if (!data)
> + return ERR_PTR(dev_err_probe(&pdev->dev, -ENODEV, "No device match found\n"));
...
> + edma = devm_kzalloc(&pdev->dev,
> + struct_size(edma, channels, data->num_channels),
> + GFP_KERNEL);
Something wrong with indentation. Not the first time, please check all your
patches for this kind of issues.
> + return ERR_PTR(-ENOMEM);
...
> + edmac->regs = devm_platform_ioremap_resource(pdev, i);
No check?
> + edmac->irq = platform_get_irq(pdev, i);
No check?
> + edmac->edma = edma;
> +
> + edmac->clk = of_clk_get(np, i);
> +
Redundant blank line.
Why one of devm_clk_get*() can't be called?
> + if (IS_ERR(edmac->clk)) {
> + dev_warn(&pdev->dev, "failed to get clock\n");
> + continue;
> + }
...
> + if (platform_get_device_id(pdev))
> + edma = ep93xx_init_from_pdata(pdev);
> + else
> + edma = ep93xx_dma_of_probe(pdev);
> +
Unneeded blank line.
> + if (!edma)
> + return PTR_ERR(edma);
...
> --- a/include/linux/platform_data/dma-ep93xx.h
> +++ b/include/linux/platform_data/dma-ep93xx.h
> #include <linux/types.h>
> #include <linux/dmaengine.h>
> #include <linux/dma-mapping.h>
> +#include <linux/of.h>
property.h.
...
> + if (of_device_is_compatible(dev_of_node(chan->device->dev), "cirrus,ep9301-dma-m2p"))
> + return true;
> +
device_is_compatible()
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply
* Re: [PATCH v3 28/42] input: keypad: ep93xx: add DT support for Cirrus EP93xx
From: Andy Shevchenko @ 2023-07-21 16:12 UTC (permalink / raw)
To: nikita.shubin
Cc: Hartley Sweeten, Lennert Buytenhek, Alexander Sverdlin,
Russell King, Lukasz Majewski, Linus Walleij, Bartosz Golaszewski,
Rob Herring, Krzysztof Kozlowski, Conor Dooley, Michael Turquette,
Stephen Boyd, Daniel Lezcano, Thomas Gleixner, Alessandro Zummo,
Alexandre Belloni, Wim Van Sebroeck, Guenter Roeck,
Sebastian Reichel, Thierry Reding, Uwe Kleine-König,
Mark Brown, David S. Miller, Eric Dumazet, Jakub Kicinski,
Paolo Abeni, Vinod Koul, Miquel Raynal, Richard Weinberger,
Vignesh Raghavendra, Damien Le Moal, Sergey Shtylyov,
Dmitry Torokhov, Arnd Bergmann, Olof Johansson, soc,
Liam Girdwood, Jaroslav Kysela, Takashi Iwai, Michael Peters,
Kris Bahnsen, linux-arm-kernel, linux-kernel, linux-gpio,
devicetree, linux-clk, linux-rtc, linux-watchdog, linux-pm,
linux-pwm, linux-spi, netdev, dmaengine, linux-mtd, linux-ide,
linux-input, alsa-devel
In-Reply-To: <20230605-ep93xx-v3-28-3d63a5f1103e@maquefel.me>
On Thu, Jul 20, 2023 at 02:29:28PM +0300, Nikita Shubin via B4 Relay wrote:
> From: Nikita Shubin <nikita.shubin@maquefel.me>
>
> - drop flags, they were not used anyway
> - add OF ID match table
> - process "autorepeat", "debounce-delay-ms", prescale from device tree
> - drop platform data usage and it's header
> - keymap goes from device tree now on
...
> #include <linux/bits.h>
> #include <linux/module.h>
> +#include <linux/of.h>
As Dmitry told you, but I think you also need property.h.
> #include <linux/platform_device.h>
> +#include <linux/mod_devicetable.h>
> #include <linux/interrupt.h>
> #include <linux/clk.h>
> #include <linux/io.h>
...
> #include <linux/input/matrix_keypad.h>
> #include <linux/slab.h>
> #include <linux/soc/cirrus/ep93xx.h>
> -#include <linux/platform_data/keypad-ep93xx.h>
> #include <linux/pm_wakeirq.h>
When adding new inclusions, try to squeeze them to most ordered part of
the block.
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply
* Re: [PATCH v3 09/42] clocksource: ep93xx: Add driver for Cirrus Logic EP93xx
From: Andy Shevchenko @ 2023-07-21 15:58 UTC (permalink / raw)
To: nikita.shubin
Cc: Hartley Sweeten, Lennert Buytenhek, Alexander Sverdlin,
Russell King, Lukasz Majewski, Linus Walleij, Bartosz Golaszewski,
Rob Herring, Krzysztof Kozlowski, Conor Dooley, Michael Turquette,
Stephen Boyd, Daniel Lezcano, Thomas Gleixner, Alessandro Zummo,
Alexandre Belloni, Wim Van Sebroeck, Guenter Roeck,
Sebastian Reichel, Thierry Reding, Uwe Kleine-König,
Mark Brown, David S. Miller, Eric Dumazet, Jakub Kicinski,
Paolo Abeni, Vinod Koul, Miquel Raynal, Richard Weinberger,
Vignesh Raghavendra, Damien Le Moal, Sergey Shtylyov,
Dmitry Torokhov, Arnd Bergmann, Olof Johansson, soc,
Liam Girdwood, Jaroslav Kysela, Takashi Iwai, Michael Peters,
Kris Bahnsen, linux-arm-kernel, linux-kernel, linux-gpio,
devicetree, linux-clk, linux-rtc, linux-watchdog, linux-pm,
linux-pwm, linux-spi, netdev, dmaengine, linux-mtd, linux-ide,
linux-input, alsa-devel
In-Reply-To: <20230605-ep93xx-v3-9-3d63a5f1103e@maquefel.me>
On Thu, Jul 20, 2023 at 02:29:09PM +0300, Nikita Shubin via B4 Relay wrote:
> From: Nikita Shubin <nikita.shubin@maquefel.me>
>
> This us a rewrite of EP93xx timer driver in
> arch/arm/mach-ep93xx/timer-ep93xx.c trying to do everything
> the device tree way:
>
> - Make every IO-access relative to a base address and dynamic
> so we can do a dynamic ioremap and get going.
> - Find register range and interrupt from the device tree.
...
+ bits.h
> +#include <linux/clockchips.h>
> +#include <linux/clocksource.h>
> +#include <linux/init.h>
> +#include <linux/interrupt.h>
> +#include <linux/io.h>
> +#include <linux/io-64-nonatomic-lo-hi.h>
> +#include <linux/irq.h>
> +#include <linux/kernel.h>
> +#include <linux/of_address.h>
> +#include <linux/of_irq.h>
> +#include <linux/sched_clock.h>
...
> +/*************************************************************************
Won't you marc it as a DOC: section?
> + * Timer handling for EP93xx
> + *************************************************************************
> + * The ep93xx has four internal timers. Timers 1, 2 (both 16 bit) and
> + * 3 (32 bit) count down at 508 kHz, are self-reloading, and can generate
> + * an interrupt on underflow. Timer 4 (40 bit) counts down at 983.04 kHz,
> + * is free-running, and can't generate interrupts.
> + *
> + * The 508 kHz timers are ideal for use for the timer interrupt, as the
> + * most common values of HZ divide 508 kHz nicely. We pick the 32 bit
> + * timer (timer 3) to get as long sleep intervals as possible when using
> + * CONFIG_NO_HZ.
> + *
> + * The higher clock rate of timer 4 makes it a better choice than the
> + * other timers for use as clock source and for sched_clock(), providing
> + * a stable 40 bit time base.
> + *************************************************************************
> + */
...
> +/*
> + * This read-only register contains the low word of the time stamp debug timer
> + * ( Timer4). When this register is read, the high byte of the Timer4 counter is
One too many spaces.
> + * saved in the Timer4ValueHigh register.
> + */
...
> +static irqreturn_t ep93xx_timer_interrupt(int irq, void *dev_id)
> +{
> + struct ep93xx_tcu *tcu = ep93xx_tcu;
> + struct clock_event_device *evt = dev_id;
> +
> + /* Writing any value clears the timer interrupt */
> + writel(1, tcu->base + EP93XX_TIMER3_CLEAR);
Would 0 suffice?
> + evt->event_handler(evt);
> +
> + return IRQ_HANDLED;
> +}
...
> +static int __init ep93xx_timer_of_init(struct device_node *np)
> +{
> + int irq;
> + unsigned long flags = IRQF_TIMER | IRQF_IRQPOLL;
> + struct ep93xx_tcu *tcu;
> + int ret;
> +
> + tcu = kzalloc(sizeof(*tcu), GFP_KERNEL);
> + if (!tcu)
> + return -ENOMEM;
> +
> + tcu->base = of_iomap(np, 0);
fwnode_iomap()?
See below why it might make sense.
> + if (!tcu->base) {
> + pr_err("Can't remap registers\n");
First of all, you may utilize pr_fmt().
Second, you may add %pOF for better user experience.
> + ret = -ENXIO;
> + goto out_free;
> + }
> + irq = irq_of_parse_and_map(np, 0);
fwnode_irq_get() which is better in terms of error handling.
> + if (irq == 0)
> + irq = -EINVAL;
> + if (irq < 0) {
> + pr_err("EP93XX Timer Can't parse IRQ %d", irq);
As per above.
> + goto out_free;
> + }
...
> +}
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply
* Re: [PATCH v3 05/42] pinctrl: add a Cirrus ep93xx SoC pin controller
From: Andy Shevchenko @ 2023-07-21 15:30 UTC (permalink / raw)
To: nikita.shubin
Cc: Hartley Sweeten, Lennert Buytenhek, Alexander Sverdlin,
Russell King, Lukasz Majewski, Linus Walleij, Bartosz Golaszewski,
Rob Herring, Krzysztof Kozlowski, Conor Dooley, Michael Turquette,
Stephen Boyd, Daniel Lezcano, Thomas Gleixner, Alessandro Zummo,
Alexandre Belloni, Wim Van Sebroeck, Guenter Roeck,
Sebastian Reichel, Thierry Reding, Uwe Kleine-König,
Mark Brown, David S. Miller, Eric Dumazet, Jakub Kicinski,
Paolo Abeni, Vinod Koul, Miquel Raynal, Richard Weinberger,
Vignesh Raghavendra, Damien Le Moal, Sergey Shtylyov,
Dmitry Torokhov, Arnd Bergmann, Olof Johansson, soc,
Liam Girdwood, Jaroslav Kysela, Takashi Iwai, Michael Peters,
Kris Bahnsen, linux-arm-kernel, linux-kernel, linux-gpio,
devicetree, linux-clk, linux-rtc, linux-watchdog, linux-pm,
linux-pwm, linux-spi, netdev, dmaengine, linux-mtd, linux-ide,
linux-input, alsa-devel
In-Reply-To: <20230605-ep93xx-v3-5-3d63a5f1103e@maquefel.me>
On Thu, Jul 20, 2023 at 02:29:05PM +0300, Nikita Shubin via B4 Relay wrote:
> From: Nikita Shubin <nikita.shubin@maquefel.me>
>
> This adds a pin control (only multiplexing) driver for ep93xx
> SoC so we can fully convert ep93xx to device tree.
>
> This driver is capable of muxing ep9301/ep9302/ep9307/ep9312/ep9315
> variants, this is chosen based on "compatible" in device tree.
...
> +config PINCTRL_EP93XX
> + bool
> + depends on OF && (ARCH_EP93XX || COMPILE_TEST)
The OF seems to be functional dependency and not compile time one.
Thus
depends on (OF && ARCH_EP93XX) || COMPILE_TEST
> + select PINMUX
> + select GENERIC_PINCONF
> + select MFD_SYSCON
...
> +#define EP93XX_SYSCON_DEVCFG_D1ONG BIT(30) /* not used */
> +#define EP93XX_SYSCON_DEVCFG_D0ONG BIT(29) /* not used */
> +#define EP93XX_SYSCON_DEVCFG_IONU2 BIT(28) /* not used */
> +#define EP93XX_SYSCON_DEVCFG_GONK BIT(27) /* done */
> +#define EP93XX_SYSCON_DEVCFG_TONG BIT(26) /* not used */
> +#define EP93XX_SYSCON_DEVCFG_MONG BIT(25) /* not used */
> +#define EP93XX_SYSCON_DEVCFG_A2ONG BIT(22) /* not used */
> +#define EP93XX_SYSCON_DEVCFG_A1ONG BIT(21) /* not used */
> +#define EP93XX_SYSCON_DEVCFG_HONIDE BIT(11) /* done */
> +#define EP93XX_SYSCON_DEVCFG_GONIDE BIT(10) /* done */
> +#define EP93XX_SYSCON_DEVCFG_PONG BIT(9) /* done */
> +#define EP93XX_SYSCON_DEVCFG_EONIDE BIT(8) /* done */
> +#define EP93XX_SYSCON_DEVCFG_I2SONSSP BIT(7) /* done */
> +#define EP93XX_SYSCON_DEVCFG_I2SONAC97 BIT(6) /* done */
> +#define EP93XX_SYSCON_DEVCFG_RASONP3 BIT(4) /* done */
> +#define PADS_MASK (GENMASK(30, 25) | BIT(22) | BIT(21) | GENMASK(11, 6) | BIT(4))
Seems better to spell each bit as by definition given above.
...
> +/* Ordered by bit index */
> +static const char * const ep93xx_padgroups[] = {
> + NULL, NULL, NULL, NULL,
> + "RasOnP3",
> + NULL,
> + "I2SonAC97",
> + "I2SonSSP",
> + "EonIDE",
> + "PonG",
> + "GonIDE",
> + "HonIDE",
> + NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL,
> + "A1onG",
> + "A2onG",
> + NULL, NULL,
> + "MonG",
> + "TonG",
> + "GonK",
> + "IonU2",
> + "D0onG",
> + "D1onG",
Instead of tons of NULLs, use
[NN] = "...",
which is much less error prone.
> +};
...
> +/** ep9301, ep9302*/
Is it really kernel doc (besides missing space)?
Please, run
scripts/kernel-doc -v -Wall -none $YOUR_FILE
for each file you contributed in the entire series and fix all warnings.
...
> +static const char *ep93xx_get_group_name(struct pinctrl_dev *pctldev,
> + unsigned int selector)
> +{
> + struct ep93xx_pmx *pmx = pinctrl_dev_get_drvdata(pctldev);
> +
> + switch (pmx->model) {
> + case EP93XX_9301_PINCTRL:
> + return ep9301_pin_groups[selector].grp.name;
> + case EP93XX_9307_PINCTRL:
> + return ep9307_pin_groups[selector].grp.name;
> + case EP93XX_9312_PINCTRL:
> + return ep9312_pin_groups[selector].grp.name;
> + }
> + return NULL;
Use default case instead.
> +}
...
> + /* Which bits changed */
> + before &= PADS_MASK;
> + after &= PADS_MASK;
> + expected = before & ~grp->mask;
> + expected |= grp->value;
> + expected &= PADS_MASK;
Instead of above:
expected = (before & ~grp->mask) | grp->value;
> + /* Print changed states */
> + tmp = expected ^ after;
tmp = (expected ^ after) & PADS_MASK;
> + for_each_set_bit(i, &tmp, PADS_MAXBIT) {
> + bool enabled = expected & BIT(i);
> +
> + dev_err(pmx->dev,
> + "pin group %s could not be %s: probably a hardware limitation\n",
> + ep93xx_padgroups[i], str_enabled_disabled(enabled));
Wrong indentation.
> + dev_err(pmx->dev,
> + "DeviceCfg before: %08x, after %08x, expected %08x\n",
> + before, after, expected);
Wrong indentation.
I believe this one can go to debug level.
> + }
...
> + pmx->model = (int)of_device_get_match_data(dev);
(uintptr_t) is more appropriate here.
...
> + pmx->pctl = devm_pinctrl_register(dev, &ep93xx_pmx_desc, pmx);
> + if (IS_ERR(pmx->pctl)) {
> + dev_err(dev, "could not register pinmux driver\n");
> + return PTR_ERR(pmx->pctl);
Why not dev_err_probe() as elsewhere?
> + }
...
> +static int __init ep93xx_pmx_init(void)
> +{
> + return platform_driver_register(&ep93xx_pmx_driver);
> +}
> +arch_initcall(ep93xx_pmx_init);
+ blank line.
Also add everywhere MODULE_DESCRIPTION() as modpost recently started to
complain (probably with `make W=1` which you should execute anyway for
your new code).
> +MODULE_IMPORT_NS(EP93XX_SOC);
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply
* Re: [PATCH v3 00/42] ep93xx device tree conversion
From: Krzysztof Kozlowski @ 2023-07-21 14:25 UTC (permalink / raw)
To: nikita.shubin, Hartley Sweeten, Lennert Buytenhek,
Alexander Sverdlin, Russell King, Lukasz Majewski, Linus Walleij,
Bartosz Golaszewski, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Michael Turquette, Stephen Boyd, Daniel Lezcano,
Thomas Gleixner, Alessandro Zummo, Alexandre Belloni,
Wim Van Sebroeck, Guenter Roeck, Sebastian Reichel,
Thierry Reding, Uwe Kleine-König, Mark Brown,
David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Vinod Koul, Miquel Raynal, Richard Weinberger,
Vignesh Raghavendra, Damien Le Moal, Sergey Shtylyov,
Dmitry Torokhov, Arnd Bergmann, Olof Johansson, soc,
Liam Girdwood, Jaroslav Kysela, Takashi Iwai, Andy Shevchenko,
Michael Peters, Kris Bahnsen
Cc: linux-arm-kernel, linux-kernel, linux-gpio, devicetree, linux-clk,
linux-rtc, linux-watchdog, linux-pm, linux-pwm, linux-spi, netdev,
dmaengine, linux-mtd, linux-ide, linux-input, alsa-devel,
Andy Shevchenko, Andrew Lunn
In-Reply-To: <20230605-ep93xx-v3-0-3d63a5f1103e@maquefel.me>
On 20/07/2023 13:29, Nikita Shubin via B4 Relay wrote:
> This series aims to convert ep93xx from platform to full device tree support.
>
> The main goal is to receive ACK's to take it via Arnd's arm-soc branch.
>
This approach makes patchset trickier to review with absolutely huge
Cc-list and inter-dependencies. I don't think this is correct approach.
This should be split per subsystem whenever possible.
Expect more grunts and complains from 50-other people you Cc-ed.
Best regards,
Krzysztof
^ permalink raw reply
* Re: [PATCH v3 07/42] soc: Add SoC driver for Cirrus ep93xx
From: Krzysztof Kozlowski @ 2023-07-21 14:22 UTC (permalink / raw)
To: nikita.shubin, Hartley Sweeten, Lennert Buytenhek,
Alexander Sverdlin, Russell King, Lukasz Majewski, Linus Walleij,
Bartosz Golaszewski, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Michael Turquette, Stephen Boyd, Daniel Lezcano,
Thomas Gleixner, Alessandro Zummo, Alexandre Belloni,
Wim Van Sebroeck, Guenter Roeck, Sebastian Reichel,
Thierry Reding, Uwe Kleine-König, Mark Brown,
David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Vinod Koul, Miquel Raynal, Richard Weinberger,
Vignesh Raghavendra, Damien Le Moal, Sergey Shtylyov,
Dmitry Torokhov, Arnd Bergmann, Olof Johansson, soc,
Liam Girdwood, Jaroslav Kysela, Takashi Iwai, Andy Shevchenko,
Michael Peters, Kris Bahnsen
Cc: linux-arm-kernel, linux-kernel, linux-gpio, devicetree, linux-clk,
linux-rtc, linux-watchdog, linux-pm, linux-pwm, linux-spi, netdev,
dmaengine, linux-mtd, linux-ide, linux-input, alsa-devel
In-Reply-To: <20230605-ep93xx-v3-7-3d63a5f1103e@maquefel.me>
On 20/07/2023 13:29, Nikita Shubin via B4 Relay wrote:
> From: Nikita Shubin <nikita.shubin@maquefel.me>
>
> This adds an SoC driver for the ep93xx. Currently there
> is only one thing not fitting into any other framework,
> and that is the swlock setting.
>
> It's used for clock settings and restart.
>
> Signed-off-by: Nikita Shubin <nikita.shubin@maquefel.me>
> Tested-by: Alexander Sverdlin <alexander.sverdlin@gmail.com>
> Acked-by: Alexander Sverdlin <alexander.sverdlin@gmail.com>
> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
> ---
> drivers/soc/Kconfig | 1 +
> drivers/soc/Makefile | 1 +
> drivers/soc/cirrus/Kconfig | 12 ++
> drivers/soc/cirrus/Makefile | 2 +
> drivers/soc/cirrus/soc-ep93xx.c | 231 ++++++++++++++++++++++++++++++++++++++
> include/linux/soc/cirrus/ep93xx.h | 18 ++-
> 6 files changed, 264 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/soc/Kconfig b/drivers/soc/Kconfig
> index 4e176280113a..16327b63b773 100644
> --- a/drivers/soc/Kconfig
> +++ b/drivers/soc/Kconfig
> @@ -8,6 +8,7 @@ source "drivers/soc/aspeed/Kconfig"
> source "drivers/soc/atmel/Kconfig"
> source "drivers/soc/bcm/Kconfig"
> source "drivers/soc/canaan/Kconfig"
> +source "drivers/soc/cirrus/Kconfig"
> source "drivers/soc/fsl/Kconfig"
> source "drivers/soc/fujitsu/Kconfig"
> source "drivers/soc/imx/Kconfig"
> diff --git a/drivers/soc/Makefile b/drivers/soc/Makefile
> index 3b0f9fb3b5c8..b76a03fe808e 100644
> --- a/drivers/soc/Makefile
> +++ b/drivers/soc/Makefile
> @@ -9,6 +9,7 @@ obj-y += aspeed/
> obj-$(CONFIG_ARCH_AT91) += atmel/
> obj-y += bcm/
> obj-$(CONFIG_SOC_CANAAN) += canaan/
> +obj-$(CONFIG_EP93XX_SOC) += cirrus/
> obj-$(CONFIG_ARCH_DOVE) += dove/
> obj-$(CONFIG_MACH_DOVE) += dove/
> obj-y += fsl/
> diff --git a/drivers/soc/cirrus/Kconfig b/drivers/soc/cirrus/Kconfig
> new file mode 100644
> index 000000000000..408f3343a265
> --- /dev/null
> +++ b/drivers/soc/cirrus/Kconfig
> @@ -0,0 +1,12 @@
> +# SPDX-License-Identifier: GPL-2.0
> +
> +if ARCH_EP93XX
> +
> +config EP93XX_SOC
> + bool "Cirrus EP93xx chips SoC"
> + select SOC_BUS
> + default y if !EP93XX_SOC_COMMON
> + help
> + Support SoC for Cirrus EP93xx chips.
> +
> +endif
> diff --git a/drivers/soc/cirrus/Makefile b/drivers/soc/cirrus/Makefile
> new file mode 100644
> index 000000000000..ed6752844c6f
> --- /dev/null
> +++ b/drivers/soc/cirrus/Makefile
> @@ -0,0 +1,2 @@
> +# SPDX-License-Identifier: GPL-2.0
> +obj-y += soc-ep93xx.o
> diff --git a/drivers/soc/cirrus/soc-ep93xx.c b/drivers/soc/cirrus/soc-ep93xx.c
> new file mode 100644
> index 000000000000..2fd48d900f24
> --- /dev/null
> +++ b/drivers/soc/cirrus/soc-ep93xx.c
> @@ -0,0 +1,231 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * SoC driver for Cirrus EP93xx chips.
> + * Copyright (C) 2022 Nikita Shubin <nikita.shubin@maquefel.me>
> + *
> + * Based on a rewrite of arch/arm/mach-ep93xx/core.c
> + * Copyright (C) 2006 Lennert Buytenhek <buytenh@wantstofly.org>
> + * Copyright (C) 2007 Herbert Valerio Riedel <hvr@gnu.org>
> + *
> + * Thanks go to Michael Burian and Ray Lehtiniemi for their key
> + * role in the ep93xx Linux community
> + */
> +
> +#include <linux/clk-provider.h>
> +#include <linux/init.h>
> +#include <linux/kernel.h>
> +#include <linux/mfd/syscon.h>
> +#include <linux/of.h>
> +#include <linux/regmap.h>
> +#include <linux/slab.h>
> +#include <linux/sys_soc.h>
> +#include <linux/soc/cirrus/ep93xx.h>
> +
> +#define EP93XX_EXT_CLK_RATE 14745600
> +
> +#define EP93XX_SYSCON_DEVCFG 0x80
> +
> +#define EP93XX_SWLOCK_MAGICK 0xaa
> +#define EP93XX_SYSCON_SWLOCK 0xc0
> +#define EP93XX_SYSCON_SYSCFG 0x9c
> +#define EP93XX_SYSCON_SYSCFG_REV_MASK 0xf0000000
> +#define EP93XX_SYSCON_SYSCFG_REV_SHIFT 28
> +
> +#define EP93XX_SYSCON_CLKSET1 0x20
> +#define EP93XX_SYSCON_CLKSET1_NBYP1 BIT(23)
> +#define EP93XX_SYSCON_CLKSET2 0x24
> +#define EP93XX_SYSCON_CLKSET2_NBYP2 BIT(19)
> +#define EP93XX_SYSCON_CLKSET2_PLL2_EN BIT(18)
> +
> +static DEFINE_SPINLOCK(ep93xx_swlock);
> +
> +/* EP93xx System Controller software locked register write */
> +void ep93xx_syscon_swlocked_write(struct regmap *map, unsigned int reg, unsigned int val)
> +{
> + unsigned long flags;
> +
> + spin_lock_irqsave(&ep93xx_swlock, flags);
> +
> + regmap_write(map, EP93XX_SYSCON_SWLOCK, EP93XX_SWLOCK_MAGICK);
> + regmap_write(map, reg, val);
> +
> + spin_unlock_irqrestore(&ep93xx_swlock, flags);
> +}
> +EXPORT_SYMBOL_NS_GPL(ep93xx_syscon_swlocked_write, EP93XX_SOC);
I doubt that your code compiles. Didn't you add a user of this in some
earlier patch?
Anyway, no, drop it, don't export some weird calls from core initcall to
drivers. You violate layering and driver encapsulation. There is no
dependency/probe ordering.
There is no even need for this, because this code does not use it!
> +
> +void ep93xx_devcfg_set_clear(struct regmap *map, unsigned int set_bits, unsigned int clear_bits)
> +{
> + unsigned long flags;
> + unsigned int val;
> +
> + spin_lock_irqsave(&ep93xx_swlock, flags);
> +
> + regmap_read(map, EP93XX_SYSCON_DEVCFG, &val);
> + val &= ~clear_bits;
> + val |= set_bits;
> + regmap_write(map, EP93XX_SYSCON_SWLOCK, EP93XX_SWLOCK_MAGICK);
> + regmap_write(map, EP93XX_SYSCON_DEVCFG, val);
> +
> + spin_unlock_irqrestore(&ep93xx_swlock, flags);
> +}
> +EXPORT_SYMBOL_NS_GPL(ep93xx_devcfg_set_clear, EP93XX_SOC);
No.
> +
> +void ep93xx_swlocked_update_bits(struct regmap *map, unsigned int reg,
> + unsigned int mask, unsigned int val)
> +{
> + unsigned long flags;
> + unsigned int tmp, orig;
> +
> + spin_lock_irqsave(&ep93xx_swlock, flags);
> +
> + regmap_read(map, EP93XX_SYSCON_DEVCFG, &orig);
> + tmp = orig & ~mask;
> + tmp |= val & mask;
> + if (tmp != orig) {
> + regmap_write(map, EP93XX_SYSCON_SWLOCK, EP93XX_SWLOCK_MAGICK);
> + regmap_write(map, reg, tmp);
> + }
> +
> + spin_unlock_irqrestore(&ep93xx_swlock, flags);
> +}
> +EXPORT_SYMBOL_NS_GPL(ep93xx_swlocked_update_bits, EP93XX_SOC);
No.
> +
> +unsigned int __init ep93xx_chip_revision(struct regmap *map)
Why this is visible outside? This should be static.
> +{
> + unsigned int val;
> +
> + regmap_read(map, EP93XX_SYSCON_SYSCFG, &val);
> + val &= EP93XX_SYSCON_SYSCFG_REV_MASK;
> + val >>= EP93XX_SYSCON_SYSCFG_REV_SHIFT;
> + return val;
> +}
> +
> +static const char __init *ep93xx_get_soc_rev(struct regmap *map)
> +{
> + int rev = ep93xx_chip_revision(map);
> +
> + switch (rev) {
> + case EP93XX_CHIP_REV_D0:
> + return "D0";
> + case EP93XX_CHIP_REV_D1:
> + return "D1";
> + case EP93XX_CHIP_REV_E0:
> + return "E0";
> + case EP93XX_CHIP_REV_E1:
> + return "E1";
> + case EP93XX_CHIP_REV_E2:
> + return "E2";
> + default:
> + return "unknown";
> + }
> +}
> +
> +/*
> + * PLL rate = 14.7456 MHz * (X1FBD + 1) * (X2FBD + 1) / (X2IPD + 1) / 2^PS
> + */
> +static unsigned long __init calc_pll_rate(u64 rate, u32 config_word)
> +{
> + rate *= ((config_word >> 11) & GENMASK(4, 0)) + 1; /* X1FBD */
> + rate *= ((config_word >> 5) & GENMASK(5, 0)) + 1; /* X2FBD */
> + do_div(rate, (config_word & GENMASK(4, 0)) + 1); /* X2IPD */
> + rate >>= ((config_word >> 16) & 3); /* PS */
> +
> + return rate;
> +}
> +
> +static int __init ep93xx_soc_init(void)
> +{
> + struct soc_device_attribute *attrs;
> + struct soc_device *soc_dev;
> + struct device_node *np;
> + struct regmap *map;
> + struct clk_hw *hw;
> + unsigned long clk_pll1_rate, clk_pll2_rate;
> + unsigned int clk_f_div, clk_h_div, clk_p_div, clk_usb_div;
> + const char fclk_divisors[] = { 1, 2, 4, 8, 16, 1, 1, 1 };
> + const char hclk_divisors[] = { 1, 2, 4, 5, 6, 8, 16, 32 };
> + const char pclk_divisors[] = { 1, 2, 4, 8 };
> + const char *machine = NULL;
> + u32 value;
> +
> + /* Multiplatform guard, only proceed on ep93xx */
> + if (!of_machine_is_compatible("cirrus,ep9301"))
> + return 0;
This should already be a warning sign for you...
> +
> + map = syscon_regmap_lookup_by_compatible("cirrus,ep9301-syscon");
> + if (IS_ERR(map))
> + return PTR_ERR(map);
No, not-reusable. Use devices and device nodes.
> +
> + /* Determine the bootloader configured pll1 rate */
> + regmap_read(map, EP93XX_SYSCON_CLKSET1, &value);
> + if (!(value & EP93XX_SYSCON_CLKSET1_NBYP1))
> + clk_pll1_rate = EP93XX_EXT_CLK_RATE;
> + else
> + clk_pll1_rate = calc_pll_rate(EP93XX_EXT_CLK_RATE, value);
> +
> + hw = clk_hw_register_fixed_rate(NULL, "pll1", "xtali", 0, clk_pll1_rate);
> + if (IS_ERR(hw))
> + return PTR_ERR(hw);
> +
> + /* Initialize the pll1 derived clocks */
> + clk_f_div = fclk_divisors[(value >> 25) & 0x7];
> + clk_h_div = hclk_divisors[(value >> 20) & 0x7];
> + clk_p_div = pclk_divisors[(value >> 18) & 0x3];
> +
> + hw = clk_hw_register_fixed_factor(NULL, "fclk", "pll1", 0, 1, clk_f_div);
> + if (IS_ERR(hw))
> + return PTR_ERR(hw);
> +
> + hw = clk_hw_register_fixed_factor(NULL, "hclk", "pll1", 0, 1, clk_h_div);
> + if (IS_ERR(hw))
> + return PTR_ERR(hw);
> +
> + hw = clk_hw_register_fixed_factor(NULL, "pclk", "hclk", 0, 1, clk_p_div);
> + if (IS_ERR(hw))
> + return PTR_ERR(hw);
> +
> + /* Determine the bootloader configured pll2 rate */
> + regmap_read(map, EP93XX_SYSCON_CLKSET2, &value);
> + if (!(value & EP93XX_SYSCON_CLKSET2_NBYP2))
> + clk_pll2_rate = EP93XX_EXT_CLK_RATE;
> + else if (value & EP93XX_SYSCON_CLKSET2_PLL2_EN)
> + clk_pll2_rate = calc_pll_rate(EP93XX_EXT_CLK_RATE, value);
> + else
> + clk_pll2_rate = 0;
> +
> + hw = clk_hw_register_fixed_rate(NULL, "pll2", "xtali", 0, clk_pll2_rate);
> + if (IS_ERR(hw))
> + return PTR_ERR(hw);
> +
> + regmap_read(map, EP93XX_SYSCON_CLKSET2, &value);
> + clk_usb_div = (((value >> 28) & GENMASK(3, 0)) + 1);
> + hw = clk_hw_register_fixed_factor(NULL, "usb_clk", "pll2", 0, 1, clk_usb_div);
> + if (IS_ERR(hw))
> + return PTR_ERR(hw);
> +
> + attrs = kzalloc(sizeof(*attrs), GFP_KERNEL);
> + if (!attrs)
> + return -ENOMEM;
> +
> + np = of_find_node_by_path("/");
> + of_property_read_string(np, "model", &machine);
> + if (machine)
> + attrs->machine = kstrdup(machine, GFP_KERNEL);
> + of_node_put(np);
> +
> + attrs->family = "Cirrus Logic EP93xx";
> + attrs->revision = ep93xx_get_soc_rev(map);
> +
> + soc_dev = soc_device_register(attrs);
> + if (IS_ERR(soc_dev)) {
> + kfree(attrs->soc_id);
> + kfree(attrs->serial_number);
> + kfree(attrs);
> + return PTR_ERR(soc_dev);
> + }
> +
> + pr_info("EP93xx SoC revision %s\n", attrs->revision);
> +
> + return 0;
> +}
> +core_initcall(ep93xx_soc_init);
That's not the way to add soc driver. You need a proper driver for it
Best regards,
Krzysztof
^ permalink raw reply
* Re: [PATCH v3 41/42] ARM: dts: ep93xx: Add EDB9302 DT
From: Krzysztof Kozlowski @ 2023-07-21 14:16 UTC (permalink / raw)
To: nikita.shubin, Hartley Sweeten, Lennert Buytenhek,
Alexander Sverdlin, Russell King, Lukasz Majewski, Linus Walleij,
Bartosz Golaszewski, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Michael Turquette, Stephen Boyd, Daniel Lezcano,
Thomas Gleixner, Alessandro Zummo, Alexandre Belloni,
Wim Van Sebroeck, Guenter Roeck, Sebastian Reichel,
Thierry Reding, Uwe Kleine-König, Mark Brown,
David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Vinod Koul, Miquel Raynal, Richard Weinberger,
Vignesh Raghavendra, Damien Le Moal, Sergey Shtylyov,
Dmitry Torokhov, Arnd Bergmann, Olof Johansson, soc,
Liam Girdwood, Jaroslav Kysela, Takashi Iwai, Andy Shevchenko,
Michael Peters, Kris Bahnsen
Cc: linux-arm-kernel, linux-kernel, linux-gpio, devicetree, linux-clk,
linux-rtc, linux-watchdog, linux-pm, linux-pwm, linux-spi, netdev,
dmaengine, linux-mtd, linux-ide, linux-input, alsa-devel
In-Reply-To: <20230605-ep93xx-v3-41-3d63a5f1103e@maquefel.me>
On 20/07/2023 13:29, Nikita Shubin via B4 Relay wrote:
> From: Alexander Sverdlin <alexander.sverdlin@gmail.com>
>
> Add device tree for Cirrus EDB9302.
>
> Signed-off-by: Alexander Sverdlin <alexander.sverdlin@gmail.com>
> Signed-off-by: Nikita Shubin <nikita.shubin@maquefel.me>
> ---
> arch/arm/boot/dts/cirrus/Makefile | 1 +
> arch/arm/boot/dts/cirrus/ep93xx-edb9302.dts | 178 ++++++++++++++++++++++++++++
> 2 files changed, 179 insertions(+)
>
> diff --git a/arch/arm/boot/dts/cirrus/Makefile b/arch/arm/boot/dts/cirrus/Makefile
> index 211a7e2f2115..e6015983e464 100644
> --- a/arch/arm/boot/dts/cirrus/Makefile
> +++ b/arch/arm/boot/dts/cirrus/Makefile
> @@ -4,5 +4,6 @@ dtb-$(CONFIG_ARCH_CLPS711X) += \
> dtb-$(CONFIG_ARCH_CLPS711X) += \
> ep7211-edb7211.dtb
> dtb-$(CONFIG_ARCH_EP93XX) += \
> + ep93xx-edb9302.dtb \
> ep93xx-bk3.dtb \
> ep93xx-ts7250.dtb
> diff --git a/arch/arm/boot/dts/cirrus/ep93xx-edb9302.dts b/arch/arm/boot/dts/cirrus/ep93xx-edb9302.dts
> new file mode 100644
> index 000000000000..b048fd131aa5
> --- /dev/null
> +++ b/arch/arm/boot/dts/cirrus/ep93xx-edb9302.dts
> @@ -0,0 +1,178 @@
> +// SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
> +/*
> + * Device Tree file for Cirrus Logic EDB9302 board based on EP9302 SoC
> + */
> +/dts-v1/;
> +#include "ep93xx.dtsi"
> +#include <dt-bindings/dma/cirrus,ep93xx-dma.h>
> +
> +/ {
> + #address-cells = <1>;
> + #size-cells = <1>;
> + compatible = "cirrus,edb9302", "cirrus,ep9301";
> + model = "cirrus,edb9302";
> +
> + chosen {
> + };
> +
> + memory@0 {
> + device_type = "memory";
> + /* should be set from ATAGS */
> + reg = <0x0000000 0x800000>,
> + <0x1000000 0x800000>,
> + <0x4000000 0x800000>,
> + <0x5000000 0x800000>;
> + };
> +
> + flash@60000000 {
Same problems - it's root, not soc bus.
> + compatible = "cfi-flash";
> + reg = <0x60000000 0x1000000>;
> + bank-width = <2>;
> + };
> +
Best regards,
Krzysztof
^ permalink raw reply
* Re: [PATCH v3 38/42] ata: pata_ep93xx: remove legacy pinctrl use
From: Andy Shevchenko @ 2023-07-21 14:16 UTC (permalink / raw)
To: nikita.shubin
Cc: Hartley Sweeten, Lennert Buytenhek, Alexander Sverdlin,
Russell King, Lukasz Majewski, Linus Walleij, Bartosz Golaszewski,
Rob Herring, Krzysztof Kozlowski, Conor Dooley, Michael Turquette,
Stephen Boyd, Daniel Lezcano, Thomas Gleixner, Alessandro Zummo,
Alexandre Belloni, Wim Van Sebroeck, Guenter Roeck,
Sebastian Reichel, Thierry Reding, Uwe Kleine-König,
Mark Brown, David S. Miller, Eric Dumazet, Jakub Kicinski,
Paolo Abeni, Vinod Koul, Miquel Raynal, Richard Weinberger,
Vignesh Raghavendra, Damien Le Moal, Sergey Shtylyov,
Dmitry Torokhov, Arnd Bergmann, Olof Johansson, soc,
Liam Girdwood, Jaroslav Kysela, Takashi Iwai, Michael Peters,
Kris Bahnsen, linux-arm-kernel, linux-kernel, linux-gpio,
devicetree, linux-clk, linux-rtc, linux-watchdog, linux-pm,
linux-pwm, linux-spi, netdev, dmaengine, linux-mtd, linux-ide,
linux-input, alsa-devel
In-Reply-To: <20230605-ep93xx-v3-38-3d63a5f1103e@maquefel.me>
On Thu, Jul 20, 2023 at 02:29:38PM +0300, Nikita Shubin via B4 Relay wrote:
> From: Nikita Shubin <nikita.shubin@maquefel.me>
>
> Drop legacy acquire/release since we are using pinctrl for this now.
...
> drv_data = devm_kzalloc(&pdev->dev, sizeof(*drv_data), GFP_KERNEL);
> - if (!drv_data) {
> - err = -ENOMEM;
> - goto err_rel_gpio;
> - }
> + if (!drv_data)
> + return -ENXIO;
Wrong error code. See above what you have deleted.
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply
* Re: [PATCH v3 35/42] ARM: dts: ep93xx: add ts7250 board
From: Krzysztof Kozlowski @ 2023-07-21 14:15 UTC (permalink / raw)
To: nikita.shubin, Hartley Sweeten, Lennert Buytenhek,
Alexander Sverdlin, Russell King, Lukasz Majewski, Linus Walleij,
Bartosz Golaszewski, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Michael Turquette, Stephen Boyd, Daniel Lezcano,
Thomas Gleixner, Alessandro Zummo, Alexandre Belloni,
Wim Van Sebroeck, Guenter Roeck, Sebastian Reichel,
Thierry Reding, Uwe Kleine-König, Mark Brown,
David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Vinod Koul, Miquel Raynal, Richard Weinberger,
Vignesh Raghavendra, Damien Le Moal, Sergey Shtylyov,
Dmitry Torokhov, Arnd Bergmann, Olof Johansson, soc,
Liam Girdwood, Jaroslav Kysela, Takashi Iwai, Andy Shevchenko,
Michael Peters, Kris Bahnsen
Cc: linux-arm-kernel, linux-kernel, linux-gpio, devicetree, linux-clk,
linux-rtc, linux-watchdog, linux-pm, linux-pwm, linux-spi, netdev,
dmaengine, linux-mtd, linux-ide, linux-input, alsa-devel
In-Reply-To: <20230605-ep93xx-v3-35-3d63a5f1103e@maquefel.me>
On 20/07/2023 13:29, Nikita Shubin via B4 Relay wrote:
> From: Nikita Shubin <nikita.shubin@maquefel.me>
>
> Add device tree file for Technologic Systems ts7250 board and
> Liebherr bk3 board which have many in common, both are based on
> ep9302 SoC variant.
>
> Signed-off-by: Nikita Shubin <nikita.shubin@maquefel.me>
> ---
> arch/arm/boot/dts/cirrus/Makefile | 3 +
> arch/arm/boot/dts/cirrus/ep93xx-bk3.dts | 126 +++++++++++++++++++++++++
> arch/arm/boot/dts/cirrus/ep93xx-ts7250.dts | 145 +++++++++++++++++++++++++++++
> 3 files changed, 274 insertions(+)
>
> diff --git a/arch/arm/boot/dts/cirrus/Makefile b/arch/arm/boot/dts/cirrus/Makefile
> index e944d3e2129d..211a7e2f2115 100644
> --- a/arch/arm/boot/dts/cirrus/Makefile
> +++ b/arch/arm/boot/dts/cirrus/Makefile
> @@ -3,3 +3,6 @@ dtb-$(CONFIG_ARCH_CLPS711X) += \
> ep7211-edb7211.dtb
> dtb-$(CONFIG_ARCH_CLPS711X) += \
> ep7211-edb7211.dtb
> +dtb-$(CONFIG_ARCH_EP93XX) += \
> + ep93xx-bk3.dtb \
> + ep93xx-ts7250.dtb
> diff --git a/arch/arm/boot/dts/cirrus/ep93xx-bk3.dts b/arch/arm/boot/dts/cirrus/ep93xx-bk3.dts
> new file mode 100644
> index 000000000000..91d76a1a8571
> --- /dev/null
> +++ b/arch/arm/boot/dts/cirrus/ep93xx-bk3.dts
> @@ -0,0 +1,126 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Device Tree file for Liebherr controller BK3.1 based on Cirrus EP9302 SoC
> + */
> +/dts-v1/;
> +#include "ep93xx.dtsi"
> +
> +/ {
> + model = "Liebherr controller BK3.1";
> + compatible = "liebherr,bk3", "cirrus,ep9301";
> + #address-cells = <1>;
> + #size-cells = <1>;
> +
> + chosen {
> + };
> +
> + memory@0 {
> + device_type = "memory";
> + /* should be set from ATAGS */
> + reg = <0x00000000 0x02000000>,
> + <0x000530c0 0x01fdd000>;
> + };
> +
> + nand-controller@60000000 {
> + compatible = "technologic,ts7200-nand";
> + reg = <0x60000000 0x8000000>;
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + nand@0 {
> + reg = <0>;
> + partitions {
> + compatible = "fixed-partitions";
> + #address-cells = <1>;
> + #size-cells = <1>;
> +
> + partition@0 {
> + label = "System";
> + reg = <0x00000000 0x01e00000>;
> + read-only;
> + };
> +
> + partition@1e00000 {
> + label = "Data";
> + reg = <0x01e00000 0x05f20000>;
> + };
> +
> + partition@7d20000 {
> + label = "RedBoot";
> + reg = <0x07d20000 0x002e0000>;
> + read-only;
> + };
> + };
> + };
> + };
> +
> + leds {
> + compatible = "gpio-leds";
> + led-0 {
> + label = "grled";
> + gpios = <&gpio4 0 GPIO_ACTIVE_HIGH>;
> + linux,default-trigger = "heartbeat";
> + function = LED_FUNCTION_HEARTBEAT;
> + };
> +
> + led-1 {
> + label = "rdled";
> + gpios = <&gpio4 1 GPIO_ACTIVE_HIGH>;
> + function = LED_FUNCTION_FAULT;
> + };
> + };
> +};
> +
> +ð0 {
> + phy-handle = <&phy0>;
> +};
> +
> +&i2c0 {
> + status = "okay";
> +};
> +
> +&i2s {
> + pinctrl-names = "default";
> + pinctrl-0 = <&i2s_on_ac97_pins>;
> + status = "okay";
> +};
> +
> +&gpio1 {
> + /* PWM */
> + gpio-ranges = <&pinctrl 6 163 1>;
> +};
> +
> +&gpio4 {
> + gpio-ranges = <&pinctrl 0 97 2>;
> + status = "okay";
> +};
> +
> +&gpio6 {
> + gpio-ranges = <&pinctrl 0 87 2>;
> + status = "okay";
> +};
> +
> +&gpio7 {
> + gpio-ranges = <&pinctrl 2 199 4>;
> + status = "okay";
> +};
> +
> +&mdio0 {
> + phy0: ethernet-phy@1 {
> + reg = <1>;
> + device_type = "ethernet-phy";
> + };
> +};
> +
> +&uart0 {
> + status = "okay";
> +};
> +
> +&uart1 {
> + status = "okay";
> +};
> +
> +&usb0 {
> + status = "okay";
> +};
> +
> diff --git a/arch/arm/boot/dts/cirrus/ep93xx-ts7250.dts b/arch/arm/boot/dts/cirrus/ep93xx-ts7250.dts
> new file mode 100644
> index 000000000000..625202f8cd25
> --- /dev/null
> +++ b/arch/arm/boot/dts/cirrus/ep93xx-ts7250.dts
> @@ -0,0 +1,145 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Device Tree file for Technologic Systems ts7250 board based on Cirrus EP9302 SoC
> + */
> +/dts-v1/;
> +#include "ep93xx.dtsi"
> +#include <dt-bindings/dma/cirrus,ep93xx-dma.h>
> +
> +/ {
> + compatible = "technologic,ts7250", "cirrus,ep9301";
> + model = "TS-7250 SBC";
> + #address-cells = <1>;
> + #size-cells = <1>;
> +
> + chosen {
> + };
> +
> + memory@0 {
> + device_type = "memory";
> + /* should be set from ATAGS */
> + reg = <0x00000000 0x02000000>,
> + <0x000530c0 0x01fdd000>;
> + };
> +
> + nand-controller@60000000 {
Where is this address? It does not work like that. If this is part of
SoC, then should be in DTSI and part of soc node. If not, then it is
some other bus which needs some description. Top-level is not a bus.
You should see errors when testing dtbs with W=1.
> + compatible = "technologic,ts7200-nand";
> + reg = <0x60000000 0x8000000>;
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + nand@0 {
> + reg = <0>;
> + partitions {
> + compatible = "fixed-partitions";
> + #address-cells = <1>;
> + #size-cells = <1>;
> +
> + partition@0 {
> + label = "TS-BOOTROM";
> + reg = <0x00000000 0x00020000>;
> + read-only;
> + };
> +
> + partition@20000 {
> + label = "Linux";
> + reg = <0x00020000 0x07d00000>;
> + };
> +
> + partition@7d20000 {
> + label = "RedBoot";
> + reg = <0x07d20000 0x002e0000>;
> + read-only;
> + };
> + };
> + };
> + };
> +
> + rtc1: rtc@10800000 {
Same problems
> + compatible = "st,m48t86";
> + reg = <0x10800000 0x1>,
> + <0x11700000 0x1>;
> + };
> +
> + watchdog1: watchdog@23800000 {
Same problems
> + compatible = "technologic,ts7200-wdt";
> + reg = <0x23800000 0x01>,
> + <0x23c00000 0x01>;
> + timeout-sec = <30>;
> + };
> +
> + leds {
> + compatible = "gpio-leds";
> + led-0 {
> + label = "grled";
> + gpios = <&gpio4 0 GPIO_ACTIVE_HIGH>;
> + linux,default-trigger = "heartbeat";
> + function = LED_FUNCTION_HEARTBEAT;
> + };
> +
> + led-1 {
> + label = "rdled";
> + gpios = <&gpio4 1 GPIO_ACTIVE_HIGH>;
> + function = LED_FUNCTION_FAULT;
> + };
> + };
> +};
> +
> +ð0 {
> + phy-handle = <&phy0>;
> +};
> +
> +&gpio1 {
> + /* PWM */
> + gpio-ranges = <&pinctrl 6 163 1>;
> +};
> +
> +&gpio4 {
> + gpio-ranges = <&pinctrl 0 97 2>;
> + status = "okay";
> +};
> +
> +&gpio6 {
> + gpio-ranges = <&pinctrl 0 87 2>;
> + status = "okay";
> +};
> +
> +&gpio7 {
> + gpio-ranges = <&pinctrl 2 199 4>;
> + status = "okay";
> +};
> +
> +&i2c0 {
> + status = "okay";
> +};
> +
> +&spi0 {
> + cs-gpios = <&gpio5 2 GPIO_ACTIVE_HIGH>;
> + dmas = <&dma1 EP93XX_DMA_SSP>;
> + status = "okay";
> +
> + tmp122_spi: tmp122@0 {
Node names should be generic. See also an explanation and list of
examples (not exhaustive) in DT specification:
https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation
> + compatible = "ti,tmp122";
> + reg = <0>;
> + spi-max-frequency = <2000000>;
> + };
> +};
> +
Best regards,
Krzysztof
^ permalink raw reply
* Re: [PATCH v3 07/42] soc: Add SoC driver for Cirrus ep93xx
From: Andy Shevchenko @ 2023-07-21 14:13 UTC (permalink / raw)
To: nikita.shubin
Cc: Hartley Sweeten, Lennert Buytenhek, Alexander Sverdlin,
Russell King, Lukasz Majewski, Linus Walleij, Bartosz Golaszewski,
Rob Herring, Krzysztof Kozlowski, Conor Dooley, Michael Turquette,
Stephen Boyd, Daniel Lezcano, Thomas Gleixner, Alessandro Zummo,
Alexandre Belloni, Wim Van Sebroeck, Guenter Roeck,
Sebastian Reichel, Thierry Reding, Uwe Kleine-König,
Mark Brown, David S. Miller, Eric Dumazet, Jakub Kicinski,
Paolo Abeni, Vinod Koul, Miquel Raynal, Richard Weinberger,
Vignesh Raghavendra, Damien Le Moal, Sergey Shtylyov,
Dmitry Torokhov, Arnd Bergmann, Olof Johansson, soc,
Liam Girdwood, Jaroslav Kysela, Takashi Iwai, Michael Peters,
Kris Bahnsen, linux-arm-kernel, linux-kernel, linux-gpio,
devicetree, linux-clk, linux-rtc, linux-watchdog, linux-pm,
linux-pwm, linux-spi, netdev, dmaengine, linux-mtd, linux-ide,
linux-input, alsa-devel
In-Reply-To: <20230605-ep93xx-v3-7-3d63a5f1103e@maquefel.me>
On Thu, Jul 20, 2023 at 02:29:07PM +0300, Nikita Shubin via B4 Relay wrote:
> From: Nikita Shubin <nikita.shubin@maquefel.me>
>
> This adds an SoC driver for the ep93xx. Currently there
> is only one thing not fitting into any other framework,
> and that is the swlock setting.
>
> It's used for clock settings and restart.
> +#include <linux/clk-provider.h>
> +#include <linux/init.h>
> +#include <linux/kernel.h>
> +#include <linux/mfd/syscon.h>
> +#include <linux/of.h>
> +#include <linux/regmap.h>
> +#include <linux/slab.h>
> +#include <linux/sys_soc.h>
> +#include <linux/soc/cirrus/ep93xx.h>
...
> +#define EP93XX_SYSCON_SYSCFG_REV_MASK 0xf0000000
GENMASK() (you will need bits.h, and looking below you seem missed it anyway)
...
> + spin_lock_irqsave(&ep93xx_swlock, flags);
> +
> + regmap_read(map, EP93XX_SYSCON_DEVCFG, &val);
> + val &= ~clear_bits;
> + val |= set_bits;
> + regmap_write(map, EP93XX_SYSCON_SWLOCK, EP93XX_SWLOCK_MAGICK);
> + regmap_write(map, EP93XX_SYSCON_DEVCFG, val);
Is this sequence a must?
I.o.w. can you first supply magic and then update devcfg?
> + spin_unlock_irqrestore(&ep93xx_swlock, flags);
...
> +void ep93xx_swlocked_update_bits(struct regmap *map, unsigned int reg,
> + unsigned int mask, unsigned int val)
> +{
Same Q as above.
> +}
...
> + int rev = ep93xx_chip_revision(map);
> +
> + switch (rev) {
switch(ep93xx_chip_revision(map))
?
> + case EP93XX_CHIP_REV_D0:
> + return "D0";
> + case EP93XX_CHIP_REV_D1:
> + return "D1";
> + case EP93XX_CHIP_REV_E0:
> + return "E0";
> + case EP93XX_CHIP_REV_E1:
> + return "E1";
> + case EP93XX_CHIP_REV_E2:
> + return "E2";
> + default:
> + return "unknown";
> + }
...
> +static unsigned long __init calc_pll_rate(u64 rate, u32 config_word)
> +{
> + rate *= ((config_word >> 11) & GENMASK(4, 0)) + 1; /* X1FBD */
> + rate *= ((config_word >> 5) & GENMASK(5, 0)) + 1; /* X2FBD */
> + do_div(rate, (config_word & GENMASK(4, 0)) + 1); /* X2IPD */
> + rate >>= ((config_word >> 16) & 3); /* PS */
GENMASK()
> + return rate;
> +}
...
> + /* Multiplatform guard, only proceed on ep93xx */
> + if (!of_machine_is_compatible("cirrus,ep9301"))
> + return 0;
Why?
> + map = syscon_regmap_lookup_by_compatible("cirrus,ep9301-syscon");
> + if (IS_ERR(map))
> + return PTR_ERR(map);
Is this not enough?
...
> + if (!(value & EP93XX_SYSCON_CLKSET1_NBYP1))
> + clk_pll1_rate = EP93XX_EXT_CLK_RATE;
> + else
> + clk_pll1_rate = calc_pll_rate(EP93XX_EXT_CLK_RATE, value);
Positive conditionals in if-else are easier to be read.
...
> + clk_f_div = fclk_divisors[(value >> 25) & 0x7];
> + clk_h_div = hclk_divisors[(value >> 20) & 0x7];
> + clk_p_div = pclk_divisors[(value >> 18) & 0x3];
GENMASK() in all three?
...
> + np = of_find_node_by_path("/");
> + of_property_read_string(np, "model", &machine);
> + if (machine)
> + attrs->machine = kstrdup(machine, GFP_KERNEL);
No error check?
> + of_node_put(np);
Looks like NIH of_flat_dt_get_machine_name(). Can you rather make use of it as
it's done, e.g., here arch/riscv/kernel/setup.c:251?
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply
* Re: [PATCH v3 34/42] ARM: dts: add Cirrus EP93XX SoC .dtsi
From: Krzysztof Kozlowski @ 2023-07-21 14:12 UTC (permalink / raw)
To: nikita.shubin, Hartley Sweeten, Lennert Buytenhek,
Alexander Sverdlin, Russell King, Lukasz Majewski, Linus Walleij,
Bartosz Golaszewski, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Michael Turquette, Stephen Boyd, Daniel Lezcano,
Thomas Gleixner, Alessandro Zummo, Alexandre Belloni,
Wim Van Sebroeck, Guenter Roeck, Sebastian Reichel,
Thierry Reding, Uwe Kleine-König, Mark Brown,
David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Vinod Koul, Miquel Raynal, Richard Weinberger,
Vignesh Raghavendra, Damien Le Moal, Sergey Shtylyov,
Dmitry Torokhov, Arnd Bergmann, Olof Johansson, soc,
Liam Girdwood, Jaroslav Kysela, Takashi Iwai, Andy Shevchenko,
Michael Peters, Kris Bahnsen
Cc: linux-arm-kernel, linux-kernel, linux-gpio, devicetree, linux-clk,
linux-rtc, linux-watchdog, linux-pm, linux-pwm, linux-spi, netdev,
dmaengine, linux-mtd, linux-ide, linux-input, alsa-devel
In-Reply-To: <20230605-ep93xx-v3-34-3d63a5f1103e@maquefel.me>
On 20/07/2023 13:29, Nikita Shubin via B4 Relay wrote:
> From: Nikita Shubin <nikita.shubin@maquefel.me>
>
> Add support for Cirrus Logic EP93XX SoC's family.
>
> Co-developed-by: Alexander Sverdlin <alexander.sverdlin@gmail.com>
> Signed-off-by: Alexander Sverdlin <alexander.sverdlin@gmail.com>
> Signed-off-by: Nikita Shubin <nikita.shubin@maquefel.me>
> ---
> arch/arm/boot/dts/cirrus/ep93xx.dtsi | 449 +++++++++++++++++++++++++++++++++++
> 1 file changed, 449 insertions(+)
>
> diff --git a/arch/arm/boot/dts/cirrus/ep93xx.dtsi b/arch/arm/boot/dts/cirrus/ep93xx.dtsi
> new file mode 100644
> index 000000000000..1e04f39d7b80
> --- /dev/null
> +++ b/arch/arm/boot/dts/cirrus/ep93xx.dtsi
> @@ -0,0 +1,449 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Device Tree file for Cirrus Logic systems EP93XX SoC
> + */
> +#include <dt-bindings/gpio/gpio.h>
> +#include <dt-bindings/leds/common.h>
> +#include <dt-bindings/input/input.h>
> +#include <dt-bindings/clock/cirrus,ep93xx-clock.h>
> +/ {
> + soc: soc {
> + compatible = "simple-bus";
> + ranges;
> + #address-cells = <1>;
> + #size-cells = <1>;
> +
> + syscon: syscon@80930000 {
> + compatible = "cirrus,ep9301-syscon",
> + "syscon", "simple-mfd";
Fix alignment.
> + reg = <0x80930000 0x1000>;
> +
> + eclk: clock-controller {
> + compatible = "cirrus,ep9301-clk";
> + #clock-cells = <1>;
> + clocks = <&xtali>;
> + status = "okay";
Drop statuses when not needed.
> + };
> +
> + pinctrl: pinctrl {
> + compatible = "cirrus,ep9301-pinctrl";
> +
> + spi_default_pins: pins-spi {
> + function = "spi";
> + groups = "ssp";
> + };
> +
...
> +
> + keypad: keypad@800f0000 {
> + compatible = "cirrus,ep9307-keypad";
> + reg = <0x800f0000 0x0c>;
> + interrupt-parent = <&vic0>;
> + interrupts = <29>;
> + clocks = <&eclk EP93XX_CLK_KEYPAD>;
> + pinctrl-names = "default";
> + pinctrl-0 = <&keypad_default_pins>;
> + linux,keymap =
No need for line break.
> + <KEY_UP>,
> + <KEY_DOWN>,
> + <KEY_VOLUMEDOWN>,
> + <KEY_HOME>,
> + <KEY_RIGHT>,
> + <KEY_LEFT>,
> + <KEY_ENTER>,
> + <KEY_VOLUMEUP>,
> + <KEY_F6>,
> + <KEY_F8>,
> + <KEY_F9>,
> + <KEY_F10>,
> + <KEY_F1>,
> + <KEY_F2>,
> + <KEY_F3>,
> + <KEY_POWER>;
> + };
> +
> + pwm0: pwm@80910000 {
> + compatible = "cirrus,ep9301-pwm";
> + reg = <0x80910000 0x10>;
> + clocks = <&eclk EP93XX_CLK_PWM>;
> + status = "disabled";
> + };
> +
> + pwm1: pwm@80910020 {
> + compatible = "cirrus,ep9301-pwm";
> + reg = <0x80910020 0x10>;
> + clocks = <&eclk EP93XX_CLK_PWM>;
> + pinctrl-names = "default";
> + pinctrl-0 = <&pwm1_default_pins>;
> + status = "disabled";
> + };
> +
> + rtc0: rtc@80920000 {
> + compatible = "cirrus,ep9301-rtc";
> + reg = <0x80920000 0x100>;
> + };
> +
> + spi0: spi@808a0000 {
> + compatible = "cirrus,ep9301-spi";
> + reg = <0x808a0000 0x18>;
> + #address-cells = <1>;
> + #size-cells = <0>;
> + interrupt-parent = <&vic1>;
> + interrupts = <21>;
> + clocks = <&eclk EP93XX_CLK_SPI>;
> + pinctrl-names = "default";
> + pinctrl-0 = <&spi_default_pins>;
> + status = "disabled";
> + };
> +
> + timer: timer@80810000 {
> + compatible = "cirrus,ep9301-timer";
> + reg = <0x80810000 0x100>;
> + interrupt-parent = <&vic1>;
> + interrupts = <19>;
> + };
> +
> + uart0: uart@808c0000 {
> + compatible = "arm,primecell";
This looks incomplete.
> + reg = <0x808c0000 0x1000>;
> + arm,primecell-periphid = <0x00041010>;
> + clocks = <&eclk EP93XX_CLK_UART1>, <&eclk EP93XX_CLK_UART>;
> + clock-names = "apb:uart1", "apb_pclk";
It does not look like you tested the DTS against bindings. Please run
`make dtbs_check` (see
Documentation/devicetree/bindings/writing-schema.rst or
https://www.linaro.org/blog/tips-and-tricks-for-validating-devicetree-sources-with-the-devicetree-schema/
for instructions).
> + interrupt-parent = <&vic1>;
> + interrupts = <20>;
> + status = "disabled";
> + };
> +
> + uart1: uart@808d0000 {
> + compatible = "arm,primecell";
> + reg = <0x808d0000 0x1000>;
> + arm,primecell-periphid = <0x00041010>;
> + clocks = <&eclk EP93XX_CLK_UART2>, <&eclk EP93XX_CLK_UART>;
> + clock-names = "apb:uart2", "apb_pclk";
It does not look like you tested the DTS against bindings. Please run
`make dtbs_check` (see
Documentation/devicetree/bindings/writing-schema.rst or
https://www.linaro.org/blog/tips-and-tricks-for-validating-devicetree-sources-with-the-devicetree-schema/
for instructions).
> + interrupt-parent = <&vic1>;
> + interrupts = <22>;
> + status = "disabled";
> + };
> +
> + uart2: uart@808b0000 {
> + compatible = "arm,primecell";
> + reg = <0x808b0000 0x1000>;
> + arm,primecell-periphid = <0x00041010>;
> + clocks = <&eclk EP93XX_CLK_UART3>, <&eclk EP93XX_CLK_UART>;
> + clock-names = "apb:uart3", "apb_pclk";
> + interrupt-parent = <&vic1>;
> + interrupts = <23>;
> + status = "disabled";
> + };
> +
> + usb0: usb@80020000 {
> + compatible = "generic-ohci";
> + reg = <0x80020000 0x10000>;
> + interrupt-parent = <&vic1>;
> + interrupts = <24>;
> + clocks = <&eclk EP93XX_CLK_USB>;
> + status = "disabled";
> + };
> +
> + watchdog0: watchdog@80940000 {
> + compatible = "cirrus,ep9301-wdt";
> + reg = <0x80940000 0x08>;
> + };
> + };
> +
> + xtali: oscillator {
> + compatible = "fixed-clock";
> + #clock-cells = <0>;
> + clock-frequency = <14745600>;
> + clock-output-names = "xtali";
> + };
> +
> + i2c0: i2c {
> + compatible = "i2c-gpio";
> + sda-gpios = <&gpio6 1 (GPIO_ACTIVE_HIGH|GPIO_OPEN_DRAIN)>;
> + scl-gpios = <&gpio6 0 (GPIO_ACTIVE_HIGH|GPIO_OPEN_DRAIN)>;
Are you sure this is part of SoC? It is rather unusual... I would say
this would be the first SoC, where GPIO pins must be an I2C.
Best regards,
Krzysztof
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox