by Min Li

[permalink] [raw]

Subject: RE: [PATCH mfd v2] mfd: Add Renesas Synchronization Management Unit (SMU) support

> > diff --git a/drivers/mfd/rsmu_spi.c b/drivers/mfd/rsmu_spi.c
> > new file mode 100644
> > index 0000000..f3a087b
> > --- /dev/null
> > +++ b/drivers/mfd/rsmu_spi.c
> > @@ -0,0 +1,265 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> > +/*
> > + * SPI driver for the IDT ClockMatrix(TM) and 82P33xxx families of
> > + * timing and synchronization devices.
> > + *
> > + * Copyright (C) 2019 Integrated Device Technology, Inc., a Renesas
> Company.
> > + */
> > +
> > +#include <linux/kernel.h>
> > +#include <linux/module.h>
> > +#include <linux/init.h>
> > +#include <linux/slab.h>
> > +#include <linux/spi/spi.h>
> > +#include <linux/regmap.h>
> > +#include <linux/of.h>
> > +#include <linux/mfd/core.h>
> > +#include <linux/mfd/rsmu.h>
> > +#include "rsmu_private.h"
> > +
> > +/*
> > + * 16-bit register address: the lower 7 bits of the register address come
> > + * from the offset addr byte and the upper 9 bits come from the page
> register.
> > + */
> > +#define RSMU_CM_PAGE_ADDR 0x7C
> > +#define RSMU_SABRE_PAGE_ADDR 0x7F
> > +#define RSMU_HIGHER_ADDR_MASK 0xFF80
> > +#define RSMU_HIGHER_ADDR_SHIFT 7
> > +#define RSMU_LOWER_ADDR_MASK 0x7F
> > +
> > +static int rsmu_read_device(struct rsmu_dev *rsmu, u8 reg, u8 *buf,
> u16 bytes)
> > +{
> > + struct spi_device *client = to_spi_device(rsmu->dev);
> > + struct spi_transfer xfer = {0};
> > + struct spi_message msg;
> > + u8 cmd[256] = {0};
> > + u8 rsp[256] = {0};
> > + int ret;
> > +
> > + cmd[0] = reg | 0x80;
> > + xfer.rx_buf = rsp;
> > + xfer.len = bytes + 1;
> > + xfer.tx_buf = cmd;
> > + xfer.bits_per_word = client->bits_per_word;
> > + xfer.speed_hz = client->max_speed_hz;
> > +
> > + spi_message_init(&msg);
> > + spi_message_add_tail(&xfer, &msg);
> > +
> > + ret = spi_sync(client, &msg);
> > + if (ret >= 0)
> > + memcpy(buf, &rsp[1], xfer.len-1);
>
> What's at rsp[0]? Worth a comment?
>

I don't really know. This code is being used as is and tested working.
Can I find out and add the comment later?

>
> > +MODULE_LICENSE("GPL");
> > diff --git a/include/linux/mfd/idt82p33_reg.h
> b/include/linux/mfd/idt82p33_reg.h
> > new file mode 100644
> > index 0000000..fb41ab0
> > --- /dev/null
> > +++ b/include/linux/mfd/idt82p33_reg.h
> > @@ -0,0 +1,112 @@
> > +/* SPDX-License-Identifier: GPL-2.0+ */
> > +/* idt82p33_reg.h
>
> Remove the filename, they have a habit of becoming out of date.
>
> > + *
> > + * Register Map - AN888_SMUforIEEE_SynchEther_82P33xxx_RevH.pdf
> > + *
> > + */
>
> Copyright.
>

Sorry, what do you mean by this?

2021-06-16 23:58:30

by Lee Jones

[permalink] [raw]

Subject: Re: [PATCH mfd v2] mfd: Add Renesas Synchronization Management Unit (SMU) support

On Wed, 16 Jun 2021, Min Li wrote:

> > > diff --git a/drivers/mfd/rsmu_spi.c b/drivers/mfd/rsmu_spi.c
> > > new file mode 100644
> > > index 0000000..f3a087b
> > > --- /dev/null
> > > +++ b/drivers/mfd/rsmu_spi.c
> > > @@ -0,0 +1,265 @@
> > > +// SPDX-License-Identifier: GPL-2.0+
> > > +/*
> > > + * SPI driver for the IDT ClockMatrix(TM) and 82P33xxx families of
> > > + * timing and synchronization devices.
> > > + *
> > > + * Copyright (C) 2019 Integrated Device Technology, Inc., a Renesas
> > Company.
> > > + */
> > > +
> > > +#include <linux/kernel.h>
> > > +#include <linux/module.h>
> > > +#include <linux/init.h>
> > > +#include <linux/slab.h>
> > > +#include <linux/spi/spi.h>
> > > +#include <linux/regmap.h>
> > > +#include <linux/of.h>
> > > +#include <linux/mfd/core.h>
> > > +#include <linux/mfd/rsmu.h>
> > > +#include "rsmu_private.h"
> > > +
> > > +/*
> > > + * 16-bit register address: the lower 7 bits of the register address come
> > > + * from the offset addr byte and the upper 9 bits come from the page
> > register.
> > > + */
> > > +#define RSMU_CM_PAGE_ADDR 0x7C
> > > +#define RSMU_SABRE_PAGE_ADDR 0x7F
> > > +#define RSMU_HIGHER_ADDR_MASK 0xFF80
> > > +#define RSMU_HIGHER_ADDR_SHIFT 7
> > > +#define RSMU_LOWER_ADDR_MASK 0x7F
> > > +
> > > +static int rsmu_read_device(struct rsmu_dev *rsmu, u8 reg, u8 *buf,
> > u16 bytes)
> > > +{
> > > + struct spi_device *client = to_spi_device(rsmu->dev);
> > > + struct spi_transfer xfer = {0};
> > > + struct spi_message msg;
> > > + u8 cmd[256] = {0};
> > > + u8 rsp[256] = {0};
> > > + int ret;
> > > +
> > > + cmd[0] = reg | 0x80;
> > > + xfer.rx_buf = rsp;
> > > + xfer.len = bytes + 1;
> > > + xfer.tx_buf = cmd;
> > > + xfer.bits_per_word = client->bits_per_word;
> > > + xfer.speed_hz = client->max_speed_hz;
> > > +
> > > + spi_message_init(&msg);
> > > + spi_message_add_tail(&xfer, &msg);
> > > +
> > > + ret = spi_sync(client, &msg);
> > > + if (ret >= 0)
> > > + memcpy(buf, &rsp[1], xfer.len-1);
> >
> > What's at rsp[0]? Worth a comment?
> >
>
> I don't really know. This code is being used as is and tested working.
> Can I find out and add the comment later?

Yes, please find out before you submit the next version.

> > > +MODULE_LICENSE("GPL");
> > > diff --git a/include/linux/mfd/idt82p33_reg.h
> > b/include/linux/mfd/idt82p33_reg.h
> > > new file mode 100644
> > > index 0000000..fb41ab0
> > > --- /dev/null
> > > +++ b/include/linux/mfd/idt82p33_reg.h
> > > @@ -0,0 +1,112 @@
> > > +/* SPDX-License-Identifier: GPL-2.0+ */
> > > +/* idt82p33_reg.h
> >
> > Remove the filename, they have a habit of becoming out of date.
> >
> > > + *
> > > + * Register Map - AN888_SMUforIEEE_SynchEther_82P33xxx_RevH.pdf
> > > + *
> > > + */
> >
> > Copyright.
>
> Sorry, what do you mean by this?

This file is missing a copyright header/statement.

--
Lee Jones [李琼斯]
Senior Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog