Re: [PATCH 2/6] mfd: Initial commit of sy7636a

On Tue, Mar 23, 2021 at 5:35 AM Lee Jones [off-list ref] wrote:

On Sat, 20 Mar 2021, Alistair Francis wrote:

quoted

On Thu, Feb 4, 2021 at 5:31 AM Lee Jones [off-list ref] wrote:

quoted

On Sat, 16 Jan 2021, Alistair Francis wrote:

quoted

Initial support for the Silergy SY7636A Power Management chip
driver.

Please remove "driver", as this is not support for the driver, it *is*
the driver which supports the chip.

Sorry for the long delay here.

I have addressed your comments.

[...]

quoted

quoted

quoted

diff --git a/drivers/mfd/sy7636a.c b/drivers/mfd/sy7636a.c
new file mode 100644
index 000000000000..39aac965d854
--- /dev/null
+++ b/drivers/mfd/sy7636a.c

@@ -0,0 +1,252 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * MFD driver for SY7636A chip

"Parent driver".

quoted

+ * Copyright (C) 2019 reMarkable AS - http://www.remarkable.com/

This is quite out of date.  Please update.

I don't own this copyright, so I would rather not change it.

I'm not comfortable taking a new driver with an old Copyright.

Maybe ask reMarkable if it's okay to bump it.

quoted

quoted

quoted

+ * Author: Lars Ivar Miljeteig [off-list ref]

Or ping this guy.

I reached out to him and have permission to bump the year.

[...]

quoted

quoted

quoted

+int set_vcom_voltage_mv(struct regmap *regmap, unsigned int vcom)
+{
+     int ret;
+     unsigned int val;
+
+     if (vcom < 0 || vcom > 5000)

Please define min/max values.

quoted

+             return -EINVAL;
+
+     val = (unsigned int)(vcom / 10) & 0x1ff;

As above.

I have used defines for all of these.

quoted

quoted

+     ret = regmap_write(regmap, SY7636A_REG_VCOM_ADJUST_CTRL_L, val);
+     if (ret)
+             return ret;
+
+     ret = regmap_write(regmap, SY7636A_REG_VCOM_ADJUST_CTRL_H, val >> 8);
+     if (ret)
+             return ret;
+
+     return 0;
+}

Who calls these?

They sysfs store and show functions.

They should be in a power/regulator driver really.

Ok, I have moved these to the regulator.

[...]

quoted

quoted

quoted

+     if (val >= ARRAY_SIZE(states)) {
+             dev_err(sy7636a->dev, "Unexpected value read from device: %u\n", val);
+             return -EINVAL;
+     }
+
+     return snprintf(buf, PAGE_SIZE, "%s\n", states[val]);
+}
+static DEVICE_ATTR(state, 0444, state_show, NULL);

You need to document new sysfs entries.

I'm not sure how to document this. Do you mind pointing out an example
I can use?

See the final paragraph in:

  Documentation/filesystems/sysfs.rst

Thanks!

[...]

quoted

quoted

quoted

+static struct attribute *sy7636a_sysfs_attrs[] = {
+     &dev_attr_state.attr,
+     &dev_attr_power_good.attr,
+     &dev_attr_vcom.attr,
+     NULL,
+};

These all look like power options?  Do they really belong here?

From what I can tell I think they do. Let me know if you don't think so.

As above, I think they should be in power or regulator.

Done.

Alistair

[...]

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