Hi Andy, Thanks for the reviews and patience. > Subject: Re: [PATCH v1 3/3] ACPI / PMIC: Add TI PMIC TPS68470 operation > region driver > > On Wed, Jun 7, 2017 at 3:07 PM, Sakari Ailus <sakari.ailus@xxxxxx> wrote: > > >> +static int ti_tps68470_pmic_get_power(struct regmap *regmap, int reg, > >> + int bitmask, u64 *value) { > >> + int data; > > > > Shouldn't you use unsigned int here? Same in the functions below. > > +1, regmap_read() returns unsigned int. > Ack > >> +static acpi_status ti_pmic_common_handler(u32 function, > > + acpi_physical_address address, > > + u32 bits, u64 *value, > > + void *handler_context, > > > handler_context is unused. > Ack > >> + int, int, u64 *), > >> + int (*update)(struct regmap *, > >> + int, int, u64), > >> + struct ti_pmic_table *table, > >> + int table_size) > > I would even split this to have separate update() and get() paths instead of > having such a monster of parameters. > I have responded on top of Sakari's response. > >> +static acpi_status ti_pmic_clk_freq_handler(u32 function, > >> + acpi_physical_address address, > >> + u32 bits, u64 *value, > >> + void *handler_context, > >> + void *region_context) { > >> + return ti_pmic_common_handler(function, address, bits, value, > >> + handler_context, region_context, > >> + ti_tps68470_pmic_get_clk_freq, > >> + ti_tps68470_regmap_update_bits, > >> + (struct ti_pmic_table *) > >> +&clk_freq_table, > > > > You shouldn't use an explicit cast here. Instead make the function > > argument const as well and you're fine. > > +1. > Ack ��.n��������+%������w��{.n�����{�����ܨ}���Ơz�j:+v�����w����ޙ��&�)ߡ�a����z�ޗ���ݢj��w�f