On Tue, Mar 23, 2021 at 5:35 AM Lee Jones <lee.jones@xxxxxxxxxx> wrote: > > On Sat, 20 Mar 2021, Alistair Francis wrote: > > > On Thu, Feb 4, 2021 at 5:31 AM Lee Jones <lee.jones@xxxxxxxxxx> wrote: > > > > > > On Sat, 16 Jan 2021, Alistair Francis wrote: > > > > > > > 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. > > [...] > > > > > 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". > > > > > > > + * 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. > > > > > + * Author: Lars Ivar Miljeteig <lars.ivar.miljeteig@xxxxxxxxxxxxxx> > > Or ping this guy. I reached out to him and have permission to bump the year. > > [...] > > > > > +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. > > > > > > > + return -EINVAL; > > > > + > > > > + val = (unsigned int)(vcom / 10) & 0x1ff; > > > > > > As above. > > > > I have used defines for all of these. > > > > > > > > > + 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. > > [...] > > > > > + 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! > > [...] > > > > > +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
