Re: [PATCH v3 2/7] mfd: Add max7360 support

["Mathieu Dubois-Briand" <mathieu.dubois-briand@xxxxxxxxxxx>]

On Wed Jan 15, 2025 at 4:42 PM CET, Lee Jones wrote:
> On Mon, 13 Jan 2025, mathieu.dubois-briand@xxxxxxxxxxx wrote:
>
> > From: Kamel Bouhara <kamel.bouhara@xxxxxxxxxxx>
> > 
> > Add core driver to support MAX7360 i2c chip, multi function device
> > with keypad, gpio, pwm, gpo and rotary encoder submodules.
> > 
> > Signed-off-by: Kamel Bouhara <kamel.bouhara@xxxxxxxxxxx>
> > Co-developed-by: Mathieu Dubois-Briand <mathieu.dubois-briand@xxxxxxxxxxx>
> > Signed-off-by: Mathieu Dubois-Briand <mathieu.dubois-briand@xxxxxxxxxxx>
> > ---
...
> > +static int max7360_set_gpos_count(struct max7360_mfd *max7360_mfd)
> > +{
> > +	/*
> > +	 * Max7360 COL0 to COL7 pins can be used either as keypad columns,
> > +	 * general purpose output or a mix of both.
> > +	 * Get the number of pins requested by the corresponding drivers, ensure
> > +	 * they are compatible with each others and apply the corresponding
> > +	 * configuration.
> > +	 */
> > +	struct device_node *np;
> > +	u32 gpos = 0;
> > +	u32 columns = 0;
> > +	unsigned int val;
> > +	int ret;
> > +
> > +	np = of_get_compatible_child(max7360_mfd->dev->of_node, GPO_COMPATIBLE);
>
> Why don't you do all of this in the GPO driver?
>

I first did this here, so the configuration was still done if the GPO
driver was missing. But on a second thought, we can just set the GPO
count to 0 here, and let the GPO driver handle all of this.

I will move this function to the GPO driver.

> > +	if (np) {
> > +		ret = of_property_read_u32(np, "ngpios", &gpos);
> > +		if (ret < 0) {
> > +			dev_err(max7360_mfd->dev, "Failed to read gpos count\n");
> > +			return ret;
> > +		}
> > +	}
> > +
> > +	ret = device_property_read_u32(max7360_mfd->dev,
> > +				       "keypad,num-columns", &columns);
> > +	if (ret < 0) {
> > +		dev_err(max7360_mfd->dev, "Failed to read columns count\n");
> > +		return ret;
> > +	}
> > +
> > +	if (gpos > MAX7360_MAX_GPO ||
> > +	    (gpos + columns > MAX7360_MAX_KEY_COLS)) {
> > +		dev_err(max7360_mfd->dev,
> > +			"Incompatible gpos and columns count (%u, %u)\n",
> > +			gpos, columns);
> > +		return -EINVAL;
> > +	}
> > +
> > +	/*
> > +	 * MAX7360_REG_DEBOUNCE contains configuration both for keypad debounce
> > +	 * timings and gpos/keypad columns repartition. Only the later is
> > +	 * modified here.
> > +	 */
> > +	val = FIELD_PREP(MAX7360_PORTS, gpos);
> > +	ret = regmap_write_bits(max7360_mfd->regmap, MAX7360_REG_DEBOUNCE,
> > +				MAX7360_PORTS, val);
> > +	if (ret) {
> > +		dev_err(max7360_mfd->dev,
> > +			"Failed to write max7360 columns/gpos configuration");
> > +		return ret;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +int max7360_port_pin_request(struct device *dev, unsigned int pin, bool request)
>
> This whole function is rough.  What are you trying to achieve?
>

Some pins can be used either for PWM, rotary encoder or GPIO. The goal
here is to allow corresponding drivers to request the pin, making sure
only one driver use a given pin at some point.

> > +{
> > +	struct i2c_client *client;
> > +	struct max7360_mfd *max7360_mfd;
> > +	unsigned long flags;
> > +	int ret = 0;
> > +
> > +	client = to_i2c_client(dev);
> > +	max7360_mfd = i2c_get_clientdata(client);
> > +
> > +	spin_lock_irqsave(&request_lock, flags);
> > +	if (request) {
> > +		if (max7360_mfd->requested_ports & BIT(pin))
> > +			ret = -EBUSY;
> > +		else
> > +			max7360_mfd->requested_ports |= BIT(pin);
> > +	} else {
> > +		max7360_mfd->requested_ports &= ~BIT(pin);
> > +	}
> > +	spin_unlock_irqrestore(&request_lock, flags);
> > +
> > +	return ret;
> > +}
> > +EXPORT_SYMBOL_GPL(max7360_port_pin_request);
...
> > +static int max7360_reset(struct max7360_mfd *max7360_mfd)
> > +{
> > +	int err;
> > +
> > +	/*
> > +	 * Set back the default values.
> > +	 * We do not use GPIO reset function here, as it does not work reliably.
>
> Why?  What's wrong with it?
>

I was going to update this comment to add details, but after some extra
testing, this was wrong actually. Reset function of the chip is working
correctly, it was just a caching issue. I will rework the whole
function.

> > +	 */
> > +	err = regmap_write(max7360_mfd->regmap, MAX7360_REG_GPIODEB, 0x00);
> > +	if (err) {
> > +		dev_err(max7360_mfd->dev, "Failed to set configuration\n");
> > -- 
> > 2.39.5
> > 

Thanks for your review. I fixed all other points listed in your mail.

-- 
Mathieu Dubois-Briand, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com