| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <11888c25-466b-2c28-1265-578736182846@linaro.org>
Date: Sun, 24 Sep 2023 14:55:57 +0200
From: Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>
To: Ayush Singh <ayushdevel1325@...il.com>,
greybus-dev@...ts.linaro.org
Cc: devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
gregkh@...uxfoundation.org, vaishnav@...gleboard.org,
jkridner@...gleboard.org, nm@...com,
krzysztof.kozlowski+dt@...aro.org, johan@...nel.org,
elder@...nel.org
Subject: Re: [PATCH v5 2/3] greybus: Add BeaglePlay Linux Driver
On 24/09/2023 14:32, Ayush Singh wrote:
>>> +
>>> + To compile this code as a module, chose M here: the module
>>> + will be called gb-beagleplay.ko
>>> +
>>> config GREYBUS_ES2
>>> tristate "Greybus ES3 USB host controller"
>>> depends on USB
>>> diff --git a/drivers/greybus/Makefile b/drivers/greybus/Makefile
>>> index 9bccdd229aa2..15a84a83788d 100644
>>> --- a/drivers/greybus/Makefile
>>> +++ b/drivers/greybus/Makefile
>>> @@ -18,9 +18,9 @@ obj-$(CONFIG_GREYBUS) += greybus.o
>>> # needed for trace events
>>> ccflags-y += -I$(src)
>>>
>>> +obj-$(CONFIG_GREYBUS_BEAGLEPLAY) += gb-beagleplay.o
>>> +
>>> # Greybus Host controller drivers
>>> gb-es2-y := es2.o
>>>
>>> obj-$(CONFIG_GREYBUS_ES2) += gb-es2.o
>>> -
>>> -
>> Does not look related to your patch.
> You are referring to the removal of last 2 newlines, right? In that
> case, I will fix it.
Yes.
>>
>>> diff --git a/drivers/greybus/gb-beagleplay.c b/drivers/greybus/gb-beagleplay.c
>>> new file mode 100644
>>> index 000000000000..39d87ef3b8fc
>>> --- /dev/null
>>> +++ b/drivers/greybus/gb-beagleplay.c
>>> @@ -0,0 +1,526 @@
>>> +// SPDX-License-Identifier: GPL-2.0
>>> +/*
>>> + * Beagleplay Linux Driver for Greybus
>>> + *
>>> + * Copyright (c) 2023 Ayush Singh <ayushdevel1325@...il.com>
>>> + * Copyright (c) 2023 BeagleBoard.org Foundation
>>> + */
>>> +
>>> +#include <linux/gfp.h>
>>> +#include <linux/greybus.h>
>>> +#include <linux/module.h>
>>> +#include <linux/of.h>
>>> +#include <linux/printk.h>
>>> +#include <linux/serdev.h>
>>> +#include <linux/tty.h>
>>> +#include <linux/tty_driver.h>
>>> +#include <linux/greybus/hd.h>
>>> +#include <linux/init.h>
>>> +#include <linux/device.h>
>>> +#include <linux/crc-ccitt.h>
>>> +#include <linux/circ_buf.h>
>>> +#include <linux/types.h>
>>> +#include <linux/workqueue.h>
>>> +
>>> +#define RX_HDLC_PAYLOAD 1024
>>> +#define CRC_LEN 2
>>> +#define MAX_RX_HDLC (1 + RX_HDLC_PAYLOAD + CRC_LEN)
>>> +#define TX_CIRC_BUF_SIZE 1024
>>> +
>>> +#define ADDRESS_GREYBUS 0x01
>>> +#define ADDRESS_DBG 0x02
>>> +#define ADDRESS_CONTROL 0x03
>>> +
>>> +#define HDLC_FRAME 0x7E
>>> +#define HDLC_ESC 0x7D
>>> +#define HDLC_XOR 0x20
>>> +
>>> +#define CONTROL_SVC_START 0x01
>>> +#define CONTROL_SVC_STOP 0x02
>>> +
>>> +/* The maximum number of CPorts supported by Greybus Host Device */
>>> +#define BEAGLEPLAY_GB_MAX_CPORTS 32
>>> +
>>> +/*
>> Use kerneldoc.
> Thanks, will do that.
>>> + */
>>> +struct gb_beagleplay {
>>> + struct serdev_device *serdev;
>>> +
>>> + struct gb_host_device *gb_host_device;
>>> +
>>> + struct work_struct tx_work;
>>> + /* tx_producer_lock: HDLC producer lock */
>> Do not comment in two places - kerneldoc and in-line. Only one place.
> I was getting some errors in checkpatch without those. I guess they will
> go away if I am using kerneldoc?
Check. Anyway your comment does not solve checkpatch problem. Again you
duplicate the name of member - tx producer lock is a HLDC producer lock.
That's nothing new than name of variable.
>>
>>> + spinlock_t tx_producer_lock;
>>> + /* tx_consumer_lock: HDLC consumer lock */
>>> + spinlock_t tx_consumer_lock;
>>> + struct circ_buf tx_circ_buf;
>>> + u16 tx_crc;
>>> + u8 tx_ack_seq;
>>> +
>>> + u16 rx_buffer_len;
>>> + u8 rx_in_esc;
>>> + u8 rx_buffer[MAX_RX_HDLC];
>>> +};
>>> +
>>> +struct hdlc_payload {
>>> + u16 length;
>>> + void *payload;
>>> +};
>>> +
>> ...
>>
>>> +
>>> +static int gb_serdev_init(struct gb_beagleplay *bg)
>>> +{
>>> + u32 speed = 115200;
>>> + int ret;
>>> +
>>> + serdev_device_set_drvdata(bg->serdev, bg);
>>> + serdev_device_set_client_ops(bg->serdev, &gb_beagleplay_ops);
>>> + ret = serdev_device_open(bg->serdev);
>>> + if (ret) {
>>> + return dev_err_probe(&bg->serdev->dev, ret,
>>> + "Unable to Open Serial Device");
>>> + }
>> Please run scripts/checkpatch.pl --strict and fix reported warnings.
>> Some warnings can be ignored, but the code here looks like it needs a
>> fix. Feel free to get in touch if the warning is not clear.
> So I do not actually get any errors here in checkpatch. I am running the
> follwing:
>
> `scripts/checkpatch.pl --codespell --strict patch/*`
>
> I only get a warning in coverletter due to that path of DT bindings
> being more than 75 character long and ` Lines should not end with a '('`.
>
>>> + if (!bg)
>>> + return -ENOMEM;
>>> +
>>> + bg->serdev = serdev;
>>> + ret = gb_serdev_init(bg);
>>> + if (ret)
>>> + return ret;
>>> +
>>> + ret = hdlc_init(bg);
>>> + if (ret)
>>> + goto free_serdev;
>>> +
>>> + ret = gb_greybus_init(bg);
>>> + if (ret)
>>> + goto free_hdlc;
>>> +
>>> + gb_beagleplay_start_svc(bg);
>>> +
>>> + return 0;
>>> +
>>> +free_hdlc:
>>> + hdlc_deinit(bg);
>>> +free_serdev:
>>> + gb_serdev_deinit(bg);
>>> + return ret;
>>> +}
>>> +
>>> +static void gb_beagleplay_remove(struct serdev_device *serdev)
>>> +{
>>> + struct gb_beagleplay *bg = serdev_device_get_drvdata(serdev);
>>> +
>>> + gb_greybus_deinit(bg);
>>> + gb_beagleplay_stop_svc(bg);
>>> + hdlc_deinit(bg);
>>> + gb_serdev_deinit(bg);
>>> +}
>>> +
>>> +static const struct of_device_id gb_beagleplay_of_match[] = {
>>> + {
>>> + .compatible = "beagle,play-cc1352",
>>> + },
>>> + {},
>>> +};
>>> +MODULE_DEVICE_TABLE(of, gb_beagleplay_of_match);
>>> +
>>> +static struct serdev_device_driver gb_beagleplay_driver = {
>>> + .probe = gb_beagleplay_probe,
>>> + .remove = gb_beagleplay_remove,
>>> + .driver = {
>>> + .name = "gb_beagleplay",
>>> + .of_match_table = gb_beagleplay_of_match,
>> This is still wrongly aligned. Spaces after tab. Are you sure checkpatch
>> does not complain bout it?
>
> Again, it doesn't seem to for me. Am I missing some environment
> variables or options? Or maybe something wrong with my editor config
> (neovim)?
You have spaces after tab, so how can this be properly aligned?
Best regards,
Krzysztof
Powered by blists - more mailing lists