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

[Lee Jones <lee.jones@linaro.org>]

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